Merge pull request #3239 from smarterclayton/healthy_registry_should_decorate

pkg/registry/minion/healthy_registry.go

@@ -22,8 +22,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/health"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/watch"
-
-	"github.com/golang/glog"
 )
 
 type HealthyRegistry struct {
@@ -41,22 +39,9 @@ func NewHealthyRegistry(delegate Registry, client client.KubeletHealthChecker) R
 func (r *HealthyRegistry) GetMinion(ctx api.Context, minionID string) (*api.Node, error) {
 	minion, err := r.delegate.GetMinion(ctx, minionID)
 	if err != nil {
-		return minion, err
-	}
-	if minion == nil {
-		return nil, ErrDoesNotExist
-	}
-	if err != nil {
 		return nil, err
 	}
-	status, err := r.client.HealthCheck(minionID)
-	if err != nil {
-		return nil, err
-	}
-	if status == health.Unhealthy {
-		return nil, ErrNotHealty
-	}
-	return minion, nil
+	return r.checkMinion(minion), nil
 }
 
 func (r *HealthyRegistry) DeleteMinion(ctx api.Context, minionID string) error {
@@ -72,26 +57,42 @@ func (r *HealthyRegistry) UpdateMinion(ctx api.Context, minion *api.Node) error
 }
 
 func (r *HealthyRegistry) ListMinions(ctx api.Context) (currentMinions *api.NodeList, err error) {
-	result := &api.NodeList{}
 	list, err := r.delegate.ListMinions(ctx)
 	if err != nil {
-		return result, err
+		return nil, err
 	}
-	for _, minion := range list.Items {
-		status, err := r.client.HealthCheck(minion.Name)
-		if err != nil {
-			glog.V(1).Infof("%#v failed health check with error: %v", minion, err)
-			continue
-		}
-		if status == health.Healthy {
-			result.Items = append(result.Items, minion)
-		} else {
-			glog.Errorf("%#v failed a health check, ignoring.", minion)
-		}
+	for i := range list.Items {
+		list.Items[i] = *r.checkMinion(&list.Items[i])
 	}
-	return result, nil
+	return list, nil
 }
 
 func (r *HealthyRegistry) WatchMinions(ctx api.Context, label, field labels.Selector, resourceVersion string) (watch.Interface, error) {
-	return r.delegate.WatchMinions(ctx, label, field, resourceVersion)
+	w, err := r.delegate.WatchMinions(ctx, label, field, resourceVersion)
+	if err != nil {
+		return nil, err
+	}
+	return watch.Filter(w, watch.FilterFunc(func(in watch.Event) (watch.Event, bool) {
+		if node, ok := in.Object.(*api.Node); ok && node != nil {
+			in.Object = r.checkMinion(node)
+		}
+		return in, true
+	})), nil
+}
+
+func (r *HealthyRegistry) checkMinion(node *api.Node) *api.Node {
+	condition := api.ConditionFull
+	switch status, err := r.client.HealthCheck(node.Name); {
+	case err != nil:
+		condition = api.ConditionUnknown
+	case status == health.Unhealthy:
+		condition = api.ConditionNone
+	}
+	// TODO: distinguish other conditions like Reachable/Live, and begin storing this
+	// data on nodes directly via sync loops.
+	node.Status.Conditions = append(node.Status.Conditions, api.NodeCondition{
+		Kind:   api.NodeReady,
+		Status: condition,
+	})
+	return node
 }

pkg/registry/minion/healthy_registry_test.go

@@ -86,19 +86,23 @@ func TestFiltering(t *testing.T) {
 		delegate: mockMinionRegistry,
 		client:   &notMinion{minion: "m1"},
 	}
-	expected := []string{"m2", "m3"}
+	expected := []string{"m1", "m2", "m3"}
 	list, err := healthy.ListMinions(ctx)
 	if err != nil {
 		t.Errorf("unexpected error: %v", err)
 	}
-	if !reflect.DeepEqual(list, registrytest.MakeMinionList(expected, api.NodeResources{})) {
+	expectedMinions := registrytest.MakeMinionList(expected, api.NodeResources{})
+	expectedMinions.Items[0].Status.Conditions = []api.NodeCondition{{Kind: api.NodeReady, Status: api.ConditionNone}}
+	expectedMinions.Items[1].Status.Conditions = []api.NodeCondition{{Kind: api.NodeReady, Status: api.ConditionFull}}
+	expectedMinions.Items[2].Status.Conditions = []api.NodeCondition{{Kind: api.NodeReady, Status: api.ConditionFull}}
+	if !reflect.DeepEqual(list, expectedMinions) {
 		t.Errorf("Expected %v, Got %v", expected, list)
 	}
 	minion, err := healthy.GetMinion(ctx, "m1")
-	if err == nil {
-		t.Errorf("unexpected non-error")
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
 	}
-	if minion != nil {
-		t.Errorf("Unexpected presence of 'm1'")
+	if minion == nil {
+		t.Errorf("Unexpected empty 'm1'")
 	}
 }

pkg/registry/minion/rest_test.go

@@ -34,8 +34,8 @@ func TestMinionRegistryREST(t *testing.T) {
 	if obj, err := ms.Get(ctx, "bar"); err != nil || obj.(*api.Node).Name != "bar" {
 		t.Errorf("missing expected object")
 	}
-	if _, err := ms.Get(ctx, "baz"); err != ErrDoesNotExist {
-		t.Errorf("has unexpected object")
+	if _, err := ms.Get(ctx, "baz"); !errors.IsNotFound(err) {
+		t.Errorf("has unexpected error: %v", err)
 	}
 
 	c, err := ms.Create(ctx, &api.Node{ObjectMeta: api.ObjectMeta{Name: "baz"}})
@@ -61,8 +61,8 @@ func TestMinionRegistryREST(t *testing.T) {
 	if s, ok := obj.Object.(*api.Status); !ok || s.Status != api.StatusSuccess {
 		t.Errorf("delete return value was weird: %#v", obj)
 	}
-	if _, err := ms.Get(ctx, "bar"); err != ErrDoesNotExist {
-		t.Errorf("delete didn't actually delete")
+	if _, err := ms.Get(ctx, "bar"); !errors.IsNotFound(err) {
+		t.Errorf("delete didn't actually delete: %v", err)
 	}
 
 	_, err = ms.Delete(ctx, "bar")
@@ -105,8 +105,8 @@ func TestMinionRegistryHealthCheck(t *testing.T) {
 	if m, ok := result.Object.(*api.Node); !ok || m.Name != "m1" {
 		t.Errorf("insert return value was weird: %#v", result)
 	}
-	if _, err := ms.Get(ctx, "m1"); err == nil {
-		t.Errorf("node is unhealthy, expect no result from apiserver")
+	if _, err := ms.Get(ctx, "m1"); err != nil {
+		t.Errorf("node is unhealthy, expect no error: %v", err)
 	}
 }
 

pkg/registry/registrytest/minion.go

@@ -20,6 +20,7 @@ import (
 	"sync"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/watch"
 )
@@ -80,12 +81,15 @@ func (r *MinionRegistry) UpdateMinion(ctx api.Context, minion *api.Node) error {
 func (r *MinionRegistry) GetMinion(ctx api.Context, minionID string) (*api.Node, error) {
 	r.Lock()
 	defer r.Unlock()
+	if r.Err != nil {
+		return nil, r.Err
+	}
 	for _, node := range r.Minions.Items {
 		if node.Name == minionID {
-			return &node, r.Err
+			return &node, nil
 		}
 	}
-	return nil, r.Err
+	return nil, errors.NewNotFound("node", minionID)
 }
 
 func (r *MinionRegistry) DeleteMinion(ctx api.Context, minionID string) error {