From cc310e0e71087131e41942bad71dbc9f781c8b96 Mon Sep 17 00:00:00 2001 From: Deyuan Deng Date: Sat, 15 Nov 2014 16:32:59 -0500 Subject: [PATCH 1/2] Support node label update. --- pkg/registry/etcd/etcd.go | 6 ++++ pkg/registry/minion/healthy_registry.go | 4 +++ pkg/registry/minion/registry.go | 1 + pkg/registry/minion/rest.go | 19 +++++++++-- pkg/registry/minion/rest_test.go | 44 +++++++++++++++++++++++++ pkg/registry/registrytest/minion.go | 12 +++++++ test/integration/auth_test.go | 2 +- 7 files changed, 85 insertions(+), 3 deletions(-) diff --git a/pkg/registry/etcd/etcd.go b/pkg/registry/etcd/etcd.go index c190b5b9c5f..ef07d648b85 100644 --- a/pkg/registry/etcd/etcd.go +++ b/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) diff --git a/pkg/registry/minion/healthy_registry.go b/pkg/registry/minion/healthy_registry.go index 6c436029b29..ea6895806db 100644 --- a/pkg/registry/minion/healthy_registry.go +++ b/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) diff --git a/pkg/registry/minion/registry.go b/pkg/registry/minion/registry.go index 640d5411d4e..b76e87f0e3e 100644 --- a/pkg/registry/minion/registry.go +++ b/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 } diff --git a/pkg/registry/minion/rest.go b/pkg/registry/minion/rest.go index cf33df78fdb..495f5c10333 100644 --- a/pkg/registry/minion/rest.go +++ b/pkg/registry/minion/rest.go @@ -100,8 +100,23 @@ func (rs *REST) New() runtime.Object { return &api.Minion{} } -func (rs *REST) Update(ctx api.Context, minion runtime.Object) (<-chan apiserver.RESTResult, error) { - return nil, fmt.Errorf("Minions can only be created (inserted) and deleted.") +func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan apiserver.RESTResult, error) { + minion, ok := obj.(*api.Minion) + if !ok { + return nil, fmt.Errorf("not a minion: %#v", obj) + } + if errs := validation.ValidateMinion(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 + } + // TODO: GetMinion will health check the minion, but we shouldn't require the minion to be + // running for updating labels. + return rs.registry.GetMinion(ctx, minion.Name) + }), nil } func (rs *REST) toApiMinion(name string) *api.Minion { diff --git a/pkg/registry/minion/rest_test.go b/pkg/registry/minion/rest_test.go index 72baddbef56..37e6eb0976d 100644 --- a/pkg/registry/minion/rest_test.go +++ b/pkg/registry/minion/rest_test.go @@ -17,6 +17,7 @@ limitations under the License. package minion import ( + "reflect" "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -119,6 +120,49 @@ func contains(nodes *api.MinionList, nodeID string) bool { return false } +func TestMinionRegistryUpdate(t *testing.T) { + minionRegistry := registrytest.NewMinionRegistry([]string{}, api.NodeResources{}) + ms := NewREST(minionRegistry) + + ctx := api.NewContext() + + expected_labels := map[string]string{"foo": "bar"} + c, err := ms.Create(ctx, &api.Minion{ + ObjectMeta: api.ObjectMeta{Name: "m1"}, + Labels: expected_labels, + }) + if err != nil { + t.Errorf("insert failed") + } + result := <-c + + created, ok := result.Object.(*api.Minion) + if !ok || created.Name != "m1" { + t.Errorf("insert return value was weird: %#v", result) + } + if !reflect.DeepEqual(expected_labels, created.Labels) { + t.Errorf("unexpected labels: %#v", created.Labels) + } + + update := new(api.Minion) + *update = *created + update_labels := map[string]string{"bar": "foo"} + update.Labels = update_labels + + c, err = ms.Update(ctx, update) + if err != nil { + t.Errorf("update failed") + } + result = <-c + updated, ok := result.Object.(*api.Minion) + if !ok || updated.Name != "m1" { + t.Errorf("update return value was weird: %#v", result) + } + if !reflect.DeepEqual(update_labels, updated.Labels) { + t.Errorf("unexpected labels: %#v", updated.Labels) + } +} + func TestMinionStorageValidatesCreate(t *testing.T) { storage := NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{})) ctx := api.NewContext() diff --git a/pkg/registry/registrytest/minion.go b/pkg/registry/registrytest/minion.go index b7439e656f5..536cc37d31f 100644 --- a/pkg/registry/registrytest/minion.go +++ b/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() diff --git a/test/integration/auth_test.go b/test/integration/auth_test.go index ec57a4a5d75..13171c89a6a 100644 --- a/test/integration/auth_test.go +++ b/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, code409}, // See #2115 about why 409 {"GET", "/api/v1beta1/minions", "", code200}, {"GET", "/api/v1beta1/minions/a", "", code200}, {"DELETE", "/api/v1beta1/minions/a" + syncFlags, "", code200}, From c20ceea170bef6d723c8597b64e907b825ea75e8 Mon Sep 17 00:00:00 2001 From: Deyuan Deng Date: Mon, 17 Nov 2014 13:22:27 -0500 Subject: [PATCH 2/2] Add more validation for updating node. --- pkg/api/validation/validation.go | 12 ++++++ pkg/api/validation/validation_test.go | 59 +++++++++++++++++++++++++++ pkg/registry/minion/rest.go | 12 ++++-- pkg/registry/minion/rest_test.go | 59 ++++++++++++--------------- test/integration/auth_test.go | 2 +- 5 files changed, 107 insertions(+), 37 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 21eba4ad026..1dc8562a09a 100644 --- a/pkg/api/validation/validation.go +++ b/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 +} diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 5dae2a6957b..c75bad7de36 100644 --- a/pkg/api/validation/validation_test.go +++ b/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") + } + } +} diff --git a/pkg/registry/minion/rest.go b/pkg/registry/minion/rest.go index 495f5c10333..07093e63a38 100644 --- a/pkg/registry/minion/rest.go +++ b/pkg/registry/minion/rest.go @@ -105,16 +105,22 @@ func (rs *REST) Update(ctx api.Context, obj runtime.Object) (<-chan apiserver.RE if !ok { return nil, fmt.Errorf("not a minion: %#v", obj) } - if errs := validation.ValidateMinion(minion); len(errs) > 0 { + + // 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 } - // TODO: GetMinion will health check the minion, but we shouldn't require the minion to be - // running for updating labels. return rs.registry.GetMinion(ctx, minion.Name) }), nil } diff --git a/pkg/registry/minion/rest_test.go b/pkg/registry/minion/rest_test.go index 37e6eb0976d..4153f7be3db 100644 --- a/pkg/registry/minion/rest_test.go +++ b/pkg/registry/minion/rest_test.go @@ -17,7 +17,6 @@ limitations under the License. package minion import ( - "reflect" "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -88,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, @@ -120,46 +119,40 @@ func contains(nodes *api.MinionList, nodeID string) bool { return false } -func TestMinionRegistryUpdate(t *testing.T) { - minionRegistry := registrytest.NewMinionRegistry([]string{}, api.NodeResources{}) - ms := NewREST(minionRegistry) - +func TestMinionStorageInvalidUpdate(t *testing.T) { + storage := NewREST(registrytest.NewMinionRegistry([]string{"foo", "bar"}, api.NodeResources{})) ctx := api.NewContext() - - expected_labels := map[string]string{"foo": "bar"} - c, err := ms.Create(ctx, &api.Minion{ - ObjectMeta: api.ObjectMeta{Name: "m1"}, - Labels: expected_labels, - }) + obj, err := storage.Get(ctx, "foo") if err != nil { - t.Errorf("insert failed") + t.Errorf("Unexpected error: %v", err) } - result := <-c - - created, ok := result.Object.(*api.Minion) - if !ok || created.Name != "m1" { - t.Errorf("insert return value was weird: %#v", result) + minion, ok := obj.(*api.Minion) + if !ok { + t.Fatalf("Object is not a minion: %#v", obj) } - if !reflect.DeepEqual(expected_labels, created.Labels) { - t.Errorf("unexpected labels: %#v", created.Labels) + minion.HostIP = "1.2.3.4" + if _, err = storage.Update(ctx, minion); err == nil { + t.Error("Unexpected non-error.") } +} - update := new(api.Minion) - *update = *created - update_labels := map[string]string{"bar": "foo"} - update.Labels = update_labels - - c, err = ms.Update(ctx, update) +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("update failed") + t.Errorf("Unexpected error: %v", err) } - result = <-c - updated, ok := result.Object.(*api.Minion) - if !ok || updated.Name != "m1" { - t.Errorf("update return value was weird: %#v", result) + minion, ok := obj.(*api.Minion) + if !ok { + t.Fatalf("Object is not a minion: %#v", obj) } - if !reflect.DeepEqual(update_labels, updated.Labels) { - t.Errorf("unexpected labels: %#v", updated.Labels) + minion.Labels = map[string]string{ + "foo": "bar", + "baz": "home", + } + if _, err = storage.Update(ctx, minion); err != nil { + t.Error("Unexpected error: %v", err) } } diff --git a/test/integration/auth_test.go b/test/integration/auth_test.go index 13171c89a6a..f25eacce0ac 100644 --- a/test/integration/auth_test.go +++ b/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, code409}, // See #2115 about why 409 + {"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},