diff --git a/pkg/registry/etcd/etcd.go b/pkg/registry/etcd/etcd.go index 7c8b5815ee5..36a5f8feb62 100644 --- a/pkg/registry/etcd/etcd.go +++ b/pkg/registry/etcd/etcd.go @@ -430,17 +430,14 @@ func (r *Registry) CreateMinion(ctx api.Context, minion *api.Minion) error { return etcderr.InterpretCreateError(err, "minion", minion.ID) } -func (r *Registry) ContainsMinion(ctx api.Context, minionID string) (bool, error) { +func (r *Registry) GetMinion(ctx api.Context, minionID string) (*api.Minion, error) { var minion api.Minion key := makeMinionKey(minionID) err := r.ExtractObj(key, &minion, false) - if err == nil { - return true, nil - } else if tools.IsEtcdNotFound(err) { - return false, nil - } else { - return false, etcderr.InterpretGetError(err, "minion", minion.ID) + if err != nil { + return nil, etcderr.InterpretGetError(err, "minion", minion.ID) } + return &minion, nil } func (r *Registry) DeleteMinion(ctx api.Context, minionID string) error { diff --git a/pkg/registry/etcd/etcd_test.go b/pkg/registry/etcd/etcd_test.go index 648c8d7d1b2..8a02c921e46 100644 --- a/pkg/registry/etcd/etcd_test.go +++ b/pkg/registry/etcd/etcd_test.go @@ -1116,22 +1116,22 @@ func TestEtcdCreateMinion(t *testing.T) { } } -func TestEtcdContainsMinion(t *testing.T) { +func TestEtcdGetMinion(t *testing.T) { ctx := api.NewContext() fakeClient := tools.NewFakeEtcdClient(t) fakeClient.Set("/registry/minions/foo", runtime.EncodeOrDie(latest.Codec, &api.Minion{TypeMeta: api.TypeMeta{ID: "foo"}}), 0) registry := NewTestEtcdRegistry(fakeClient) - contains, err := registry.ContainsMinion(ctx, "foo") + minion, err := registry.GetMinion(ctx, "foo") if err != nil { t.Errorf("unexpected error: %v", err) } - if contains == false { - t.Errorf("Expected true, but got false") + if minion.ID != "foo" { + t.Errorf("Unexpected minion: %#v", minion) } } -func TestEtcdContainsMinionNotFound(t *testing.T) { +func TestEtcdGetMinionNotFound(t *testing.T) { ctx := api.NewContext() fakeClient := tools.NewFakeEtcdClient(t) fakeClient.Data["/registry/minions/foo"] = tools.EtcdResponseWithError{ @@ -1141,14 +1141,10 @@ func TestEtcdContainsMinionNotFound(t *testing.T) { E: tools.EtcdErrorNotFound, } registry := NewTestEtcdRegistry(fakeClient) - contains, err := registry.ContainsMinion(ctx, "foo") + _, err := registry.GetMinion(ctx, "foo") - if err != nil { - t.Errorf("unexpected error: %v", err) - } - - if contains == true { - t.Errorf("Expected false, but got true") + if !errors.IsNotFound(err) { + t.Errorf("Unexpected error returned: %#v", err) } } diff --git a/pkg/registry/minion/caching_registry.go b/pkg/registry/minion/caching_registry.go index ce0a09dac4b..382c43092b5 100644 --- a/pkg/registry/minion/caching_registry.go +++ b/pkg/registry/minion/caching_registry.go @@ -57,21 +57,20 @@ func NewCachingRegistry(delegate Registry, ttl time.Duration) (Registry, error) }, nil } -func (r *CachingRegistry) ContainsMinion(ctx api.Context, nodeID string) (bool, error) { +func (r *CachingRegistry) GetMinion(ctx api.Context, nodeID string) (*api.Minion, error) { if r.expired() { if err := r.refresh(ctx, false); err != nil { - return false, err + return nil, err } } - // block updates in the middle of a contains. r.lock.RLock() defer r.lock.RUnlock() for _, node := range r.nodes.Items { if node.ID == nodeID { - return true, nil + return &node, nil } } - return false, nil + return nil, ErrDoesNotExist } func (r *CachingRegistry) DeleteMinion(ctx api.Context, nodeID string) error { diff --git a/pkg/registry/minion/cloud_registry.go b/pkg/registry/minion/cloud_registry.go index 6197b7f71d4..d3b8b2f290b 100644 --- a/pkg/registry/minion/cloud_registry.go +++ b/pkg/registry/minion/cloud_registry.go @@ -37,18 +37,18 @@ func NewCloudRegistry(cloud cloudprovider.Interface, matchRE string, staticResou }, nil } -func (r *CloudRegistry) ContainsMinion(ctx api.Context, nodeID string) (bool, error) { +func (r *CloudRegistry) GetMinion(ctx api.Context, nodeID string) (*api.Minion, error) { instances, err := r.ListMinions(ctx) if err != nil { - return false, err + return nil, err } for _, node := range instances.Items { if node.ID == nodeID { - return true, nil + return &node, nil } } - return false, nil + return nil, ErrDoesNotExist } func (r CloudRegistry) DeleteMinion(ctx api.Context, nodeID string) error { diff --git a/pkg/registry/minion/cloud_registry_test.go b/pkg/registry/minion/cloud_registry_test.go index e0fecfc66a3..5dce9240f85 100644 --- a/pkg/registry/minion/cloud_registry_test.go +++ b/pkg/registry/minion/cloud_registry_test.go @@ -46,7 +46,7 @@ func TestCloudList(t *testing.T) { } } -func TestCloudContains(t *testing.T) { +func TestCloudGet(t *testing.T) { ctx := api.NewContext() instances := []string{"m1", "m2"} fakeCloud := fake_cloud.FakeCloud{ @@ -57,21 +57,21 @@ func TestCloudContains(t *testing.T) { t.Errorf("unexpected error: %v", err) } - contains, err := registry.ContainsMinion(ctx, "m1") + minion, err := registry.GetMinion(ctx, "m1") if err != nil { t.Errorf("unexpected error: %v", err) } - if !contains { + if minion == nil { t.Errorf("Unexpected !contains") } - contains, err = registry.ContainsMinion(ctx, "m100") - if err != nil { - t.Errorf("unexpected error: %v", err) + minion, err = registry.GetMinion(ctx, "m100") + if err == nil { + t.Errorf("unexpected non error") } - if contains { + if minion != nil { t.Errorf("Unexpected contains") } } diff --git a/pkg/registry/minion/healthy_registry.go b/pkg/registry/minion/healthy_registry.go index 3fdb14a8c74..64e7831ec20 100644 --- a/pkg/registry/minion/healthy_registry.go +++ b/pkg/registry/minion/healthy_registry.go @@ -40,22 +40,22 @@ func NewHealthyRegistry(delegate Registry, client *http.Client) Registry { } } -func (r *HealthyRegistry) ContainsMinion(ctx api.Context, minion string) (bool, error) { - contains, err := r.delegate.ContainsMinion(ctx, minion) - if err != nil { - return false, err +func (r *HealthyRegistry) GetMinion(ctx api.Context, minionID string) (*api.Minion, error) { + minion, err := r.delegate.GetMinion(ctx, minionID) + if minion == nil { + return nil, ErrDoesNotExist } - if !contains { - return false, nil - } - status, err := health.DoHTTPCheck(r.makeMinionURL(minion), r.client) if err != nil { - return false, err + return nil, err + } + status, err := health.DoHTTPCheck(r.makeMinionURL(minionID), r.client) + if err != nil { + return nil, err } if status == health.Unhealthy { - return false, nil + return nil, ErrNotHealty } - return true, nil + return minion, nil } func (r *HealthyRegistry) DeleteMinion(ctx api.Context, minionID string) error { diff --git a/pkg/registry/minion/healthy_registry_test.go b/pkg/registry/minion/healthy_registry_test.go index 0ac9076743d..d71f02f5314 100644 --- a/pkg/registry/minion/healthy_registry_test.go +++ b/pkg/registry/minion/healthy_registry_test.go @@ -60,18 +60,18 @@ func TestBasicDelegation(t *testing.T) { if err != nil { t.Errorf("unexpected error: %v", err) } - ok, err := healthy.ContainsMinion(ctx, "m1") + minion, err := healthy.GetMinion(ctx, "m1") if err != nil { t.Errorf("unexpected error: %v", err) } - if !ok { + if minion == nil { t.Errorf("Unexpected absence of 'm1'") } - ok, err = healthy.ContainsMinion(ctx, "m5") - if err != nil { - t.Errorf("unexpected error: %v", err) + minion, err = healthy.GetMinion(ctx, "m5") + if err == nil { + t.Errorf("unexpected non-error") } - if ok { + if minion != nil { t.Errorf("Unexpected presence of 'm5'") } } @@ -104,11 +104,11 @@ func TestFiltering(t *testing.T) { if !reflect.DeepEqual(list, registrytest.MakeMinionList(expected, api.NodeResources{})) { t.Errorf("Expected %v, Got %v", expected, list) } - ok, err := healthy.ContainsMinion(ctx, "m1") - if err != nil { - t.Errorf("unexpected error: %v", err) + minion, err := healthy.GetMinion(ctx, "m1") + if err == nil { + t.Errorf("unexpected non-error") } - if ok { + if minion != nil { t.Errorf("Unexpected presence of 'm1'") } } diff --git a/pkg/registry/minion/registry.go b/pkg/registry/minion/registry.go index 129f8d98a9e..640d5411d4e 100644 --- a/pkg/registry/minion/registry.go +++ b/pkg/registry/minion/registry.go @@ -22,6 +22,6 @@ import "github.com/GoogleCloudPlatform/kubernetes/pkg/api" type Registry interface { ListMinions(ctx api.Context) (*api.MinionList, error) CreateMinion(ctx api.Context, minion *api.Minion) error - ContainsMinion(ctx api.Context, minionID string) (bool, error) + GetMinion(ctx api.Context, minionID string) (*api.Minion, error) DeleteMinion(ctx api.Context, minionID string) error } diff --git a/pkg/registry/minion/rest.go b/pkg/registry/minion/rest.go index e16d2dc5254..9c6e177a7a5 100644 --- a/pkg/registry/minion/rest.go +++ b/pkg/registry/minion/rest.go @@ -39,6 +39,7 @@ func NewREST(m Registry) *REST { } var ErrDoesNotExist = fmt.Errorf("The requested resource does not exist.") +var ErrNotHealty = fmt.Errorf("The requested minion is not healthy.") func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan runtime.Object, error) { minion, ok := obj.(*api.Minion) @@ -56,20 +57,20 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (<-chan runtime.Obje if err != nil { return nil, err } - contains, err := rs.registry.ContainsMinion(ctx, minion.ID) + minion, err := rs.registry.GetMinion(ctx, minion.ID) + if minion == nil { + return nil, ErrDoesNotExist + } if err != nil { return nil, err } - if contains { - return rs.toApiMinion(minion.ID), nil - } - return nil, fmt.Errorf("unable to add minion %#v", minion) + return minion, nil }), nil } func (rs *REST) Delete(ctx api.Context, id string) (<-chan runtime.Object, error) { - exists, err := rs.registry.ContainsMinion(ctx, id) - if !exists { + minion, err := rs.registry.GetMinion(ctx, id) + if minion == nil { return nil, ErrDoesNotExist } if err != nil { @@ -81,11 +82,11 @@ func (rs *REST) Delete(ctx api.Context, id string) (<-chan runtime.Object, error } func (rs *REST) Get(ctx api.Context, id string) (runtime.Object, error) { - exists, err := rs.registry.ContainsMinion(ctx, id) - if !exists { + minion, err := rs.registry.GetMinion(ctx, id) + if minion == nil { return nil, ErrDoesNotExist } - return rs.toApiMinion(id), err + return minion, err } func (rs *REST) List(ctx api.Context, label, field labels.Selector) (runtime.Object, error) { diff --git a/pkg/registry/registrytest/minion.go b/pkg/registry/registrytest/minion.go index 1cf756cb619..db1200c0380 100644 --- a/pkg/registry/registrytest/minion.go +++ b/pkg/registry/registrytest/minion.go @@ -60,15 +60,15 @@ func (r *MinionRegistry) CreateMinion(ctx api.Context, minion *api.Minion) error return r.Err } -func (r *MinionRegistry) ContainsMinion(ctx api.Context, minionID string) (bool, error) { +func (r *MinionRegistry) GetMinion(ctx api.Context, minionID string) (*api.Minion, error) { r.Lock() defer r.Unlock() for _, node := range r.Minions.Items { if node.ID == minionID { - return true, r.Err + return &node, r.Err } } - return false, r.Err + return nil, r.Err } func (r *MinionRegistry) DeleteMinion(ctx api.Context, minionID string) error {