Merge pull request #2397 from ddysher/node-label

Support node label update

pkg/api/validation/validation.go

@@ -535,3 +535,15 @@ func ValidateMinion(minion *api.Minion) errs.ValidationErrorList {
 	allErrs = append(allErrs, validateLabels(minion.Labels)...)
 	return allErrs
 }
+
+// ValidateMinionUpdate tests to make sure a minion update can be applied.  Modifies oldMinion.
+func ValidateMinionUpdate(oldMinion *api.Minion, minion *api.Minion) errs.ValidationErrorList {
+	allErrs := errs.ValidationErrorList{}
+	// TODO: why we need two labels for minion.
+	oldMinion.Labels = minion.Labels
+	oldMinion.ObjectMeta.Labels = minion.ObjectMeta.Labels
+	if !reflect.DeepEqual(oldMinion, minion) {
+		allErrs = append(allErrs, fmt.Errorf("Update contains more than labels changes"))
+	}
+	return allErrs
+}

pkg/api/validation/validation_test.go

@@ -1065,3 +1065,62 @@ func TestValidateMinion(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateMinionUpdate(t *testing.T) {
+	tests := []struct {
+		oldMinion api.Minion
+		minion    api.Minion
+		valid     bool
+	}{
+		{api.Minion{}, api.Minion{}, true},
+		{api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name: "foo"}},
+			api.Minion{
+				ObjectMeta: api.ObjectMeta{
+					Name: "bar"},
+			}, false},
+		{api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo",
+				Labels: map[string]string{"foo": "bar"},
+			},
+		}, api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo",
+				Labels: map[string]string{"foo": "baz"},
+			},
+		}, true},
+		{api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name: "foo",
+			},
+		}, api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo",
+				Labels: map[string]string{"foo": "baz"},
+			},
+		}, true},
+		{api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo",
+				Labels: map[string]string{"bar": "foo"},
+			},
+		}, api.Minion{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo",
+				Labels: map[string]string{"foo": "baz"},
+			},
+		}, true},
+	}
+	for _, test := range tests {
+		errs := ValidateMinionUpdate(&test.oldMinion, &test.minion)
+		if test.valid && len(errs) > 0 {
+			t.Errorf("Unexpected error: %v", errs)
+			t.Logf("%#v vs %#v", test.oldMinion.ObjectMeta, test.minion.ObjectMeta)
+		}
+		if !test.valid && len(errs) == 0 {
+			t.Errorf("Unexpected non-error")
+		}
+	}
+}

pkg/registry/etcd/etcd.go

@@ -581,6 +581,12 @@ func (r *Registry) CreateMinion(ctx api.Context, minion *api.Minion) error {
 	return etcderr.InterpretCreateError(err, "minion", minion.Name)
 }
 
+func (r *Registry) UpdateMinion(ctx api.Context, minion *api.Minion) error {
+	// TODO: Add some validations.
+	err := r.SetObj(makeMinionKey(minion.Name), minion)
+	return etcderr.InterpretUpdateError(err, "minion", minion.Name)
+}
+
 func (r *Registry) GetMinion(ctx api.Context, minionID string) (*api.Minion, error) {
 	var minion api.Minion
 	key := makeMinionKey(minionID)

pkg/registry/minion/healthy_registry.go

@@ -62,6 +62,10 @@ func (r *HealthyRegistry) CreateMinion(ctx api.Context, minion *api.Minion) erro
 	return r.delegate.CreateMinion(ctx, minion)
 }
 
+func (r *HealthyRegistry) UpdateMinion(ctx api.Context, minion *api.Minion) error {
+	return r.delegate.UpdateMinion(ctx, minion)
+}
+
 func (r *HealthyRegistry) ListMinions(ctx api.Context) (currentMinions *api.MinionList, err error) {
 	result := &api.MinionList{}
 	list, err := r.delegate.ListMinions(ctx)

pkg/registry/minion/registry.go

@@ -22,6 +22,7 @@ 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
+	UpdateMinion(ctx api.Context, minion *api.Minion) error
 	GetMinion(ctx api.Context, minionID string) (*api.Minion, error)
 	DeleteMinion(ctx api.Context, minionID string) error
 }

pkg/registry/minion/rest.go

@@ -100,8 +100,29 @@ func (rs *REST) New() runtime.Object {
 	return &api.Minion{}
 }
 
-func (rs *REST) Update(ctx api.Context, minion runtime.Object) (<-chan apiserver.RESTResult, error) {
+func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan apiserver.RESTResult, error) {
-	return nil, fmt.Errorf("Minions can only be created (inserted) and deleted.")
+	minion, ok := obj.(*api.Minion)
+	if !ok {
+		return nil, fmt.Errorf("not a minion: %#v", obj)
+	}
+
+	// TODO: GetMinion will health check the minion, but we shouldn't require the minion to be
+	// running for updating labels.
+	oldMinion, err := rs.registry.GetMinion(ctx, minion.Name)
+	if err != nil {
+		return nil, err
+	}
+	if errs := validation.ValidateMinionUpdate(oldMinion, minion); len(errs) > 0 {
+		return nil, kerrors.NewInvalid("minion", minion.Name, errs)
+	}
+
+	return apiserver.MakeAsync(func() (runtime.Object, error) {
+		err := rs.registry.UpdateMinion(ctx, minion)
+		if err != nil {
+			return nil, err
+		}
+		return rs.registry.GetMinion(ctx, minion.Name)
+	}), nil
 }
 
 func (rs *REST) toApiMinion(name string) *api.Minion {

pkg/registry/minion/rest_test.go

@@ -87,7 +87,7 @@ func TestMinionREST(t *testing.T) {
 	}
 }
 
-func TestMinionRESTWithHealthCheck(t *testing.T) {
+func TestMinionStorageWithHealthCheck(t *testing.T) {
 	minionRegistry := registrytest.NewMinionRegistry([]string{}, api.NodeResources{})
 	minionHealthRegistry := HealthyRegistry{
 		delegate: minionRegistry,
@@ -119,6 +119,43 @@ func contains(nodes *api.MinionList, nodeID string) bool {
 	return false
 }
 
+func TestMinionStorageInvalidUpdate(t *testing.T) {
+	storage := NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}))
+	ctx := api.NewContext()
+	obj, err := storage.Get(ctx, "foo")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	minion, ok := obj.(*api.Minion)
+	if !ok {
+		t.Fatalf("Object is not a minion: %#v", obj)
+	}
+	minion.HostIP = "1.2.3.4"
+	if _, err = storage.Update(ctx, minion); err == nil {
+		t.Error("Unexpected non-error.")
+	}
+}
+
+func TestMinionStorageValidUpdate(t *testing.T) {
+	storage := NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}))
+	ctx := api.NewContext()
+	obj, err := storage.Get(ctx, "foo")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	minion, ok := obj.(*api.Minion)
+	if !ok {
+		t.Fatalf("Object is not a minion: %#v", obj)
+	}
+	minion.Labels = map[string]string{
+		"foo": "bar",
+		"baz": "home",
+	}
+	if _, err = storage.Update(ctx, minion); err != nil {
+		t.Error("Unexpected error: %v", err)
+	}
+}
+
 func TestMinionStorageValidatesCreate(t *testing.T) {
 	storage := NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{}))
 	ctx := api.NewContext()

pkg/registry/registrytest/minion.go

@@ -60,6 +60,18 @@ func (r *MinionRegistry) CreateMinion(ctx api.Context, minion *api.Minion) error
 	return r.Err
 }
 
+func (r *MinionRegistry) UpdateMinion(ctx api.Context, minion *api.Minion) error {
+	r.Lock()
+	defer r.Unlock()
+	for i, node := range r.Minions.Items {
+		if node.Name == minion.Name {
+			r.Minions.Items[i] = *minion
+			return r.Err
+		}
+	}
+	return r.Err
+}
+
 func (r *MinionRegistry) GetMinion(ctx api.Context, minionID string) (*api.Minion, error) {
 	r.Lock()
 	defer r.Unlock()

test/integration/auth_test.go

@@ -306,7 +306,7 @@ func getTestRequests() []struct {
 		// Normal methods on minions
 		{"GET", "/api/v1beta1/minions", "", code200},
 		{"POST", "/api/v1beta1/minions" + syncFlags, aMinion, code200},
-		{"PUT", "/api/v1beta1/minions/a" + syncFlags, aMinion, code500}, // See #2114 about why 500
+		{"PUT", "/api/v1beta1/minions/a" + syncFlags, aMinion, code422}, // TODO: GET and put back server-provided fields to avoid a 422
 		{"GET", "/api/v1beta1/minions", "", code200},
 		{"GET", "/api/v1beta1/minions/a", "", code200},
 		{"DELETE", "/api/v1beta1/minions/a" + syncFlags, "", code200},