Merge pull request #1674 from ddysher/minion-api

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 {

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)
 	}
 }
 

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 {

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 {

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")
 	}
 }

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
+	if err != nil {
+		return nil, err
 	}
-	status, err := health.DoHTTPCheck(r.makeMinionURL(minion), r.client)
+	status, err := health.DoHTTPCheck(r.makeMinionURL(minionID), r.client)
 	if err != nil {
-		return false, err
+		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 {

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'")
 	}
 }

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
 }

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) {

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 {