From 34de08a3c9761ce2b9161e4c1e685c67012b244b Mon Sep 17 00:00:00 2001 From: knight42 Date: Tue, 12 Nov 2019 17:19:37 +0800 Subject: [PATCH 1/2] feat(scale): update CR in UpdatedObjectInfo impl --- .../pkg/registry/customresource/etcd.go | 102 +++++++++++------- 1 file changed, 62 insertions(+), 40 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go index f8261196407..a2e7557449a 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go @@ -237,48 +237,17 @@ func (r *ScaleREST) Get(ctx context.Context, name string, options *metav1.GetOpt } func (r *ScaleREST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) { - obj, err := r.store.Get(ctx, name, &metav1.GetOptions{}) - if err != nil { - return nil, false, err - } - cr := obj.(*unstructured.Unstructured) - - const invalidSpecReplicas = -2147483648 // smallest int32 - oldScale, replicasFound, err := scaleFromCustomResource(cr, r.specReplicasPath, r.statusReplicasPath, r.labelSelectorPath) - if err != nil { - return nil, false, err - } - if !replicasFound { - oldScale.Spec.Replicas = invalidSpecReplicas // signal that this was not set before + scaleObjInfo := &scaleUpdatedObjectInfo{ + reqObjInfo: objInfo, + specReplicasPath: r.specReplicasPath, + labelSelectorPath: r.labelSelectorPath, + statusReplicasPath: r.statusReplicasPath, } - obj, err = objInfo.UpdatedObject(ctx, oldScale) - if err != nil { - return nil, false, err - } - if obj == nil { - return nil, false, apierrors.NewBadRequest(fmt.Sprintf("nil update passed to Scale")) - } - - scale, ok := obj.(*autoscalingv1.Scale) - if !ok { - return nil, false, apierrors.NewBadRequest(fmt.Sprintf("wrong object passed to Scale update: %v", obj)) - } - - if scale.Spec.Replicas == invalidSpecReplicas { - return nil, false, apierrors.NewBadRequest(fmt.Sprintf("the spec replicas field %q cannot be empty", r.specReplicasPath)) - } - - specReplicasPath := strings.TrimPrefix(r.specReplicasPath, ".") // ignore leading period - if err = unstructured.SetNestedField(cr.Object, int64(scale.Spec.Replicas), strings.Split(specReplicasPath, ".")...); err != nil { - return nil, false, err - } - cr.SetResourceVersion(scale.ResourceVersion) - - obj, _, err = r.store.Update( + obj, _, err := r.store.Update( ctx, - cr.GetName(), - rest.DefaultUpdatedObjectInfo(cr), + name, + scaleObjInfo, toScaleCreateValidation(createValidation, r.specReplicasPath, r.statusReplicasPath, r.labelSelectorPath), toScaleUpdateValidation(updateValidation, r.specReplicasPath, r.statusReplicasPath, r.labelSelectorPath), false, @@ -287,12 +256,13 @@ func (r *ScaleREST) Update(ctx context.Context, name string, objInfo rest.Update if err != nil { return nil, false, err } - cr = obj.(*unstructured.Unstructured) + cr := obj.(*unstructured.Unstructured) newScale, _, err := scaleFromCustomResource(cr, r.specReplicasPath, r.statusReplicasPath, r.labelSelectorPath) if err != nil { return nil, false, apierrors.NewBadRequest(err.Error()) } + return newScale, false, err } @@ -372,3 +342,55 @@ func scaleFromCustomResource(cr *unstructured.Unstructured, specReplicasPath, st return scale, foundSpecReplicas, nil } + +type scaleUpdatedObjectInfo struct { + reqObjInfo rest.UpdatedObjectInfo + specReplicasPath string + statusReplicasPath string + labelSelectorPath string +} + +func (i *scaleUpdatedObjectInfo) Preconditions() *metav1.Preconditions { + return i.reqObjInfo.Preconditions() +} + +func (i *scaleUpdatedObjectInfo) UpdatedObject(ctx context.Context, oldObj runtime.Object) (runtime.Object, error) { + cr := oldObj.DeepCopyObject().(*unstructured.Unstructured) + const invalidSpecReplicas = -2147483648 // smallest int32 + oldScale, replicasFound, err := scaleFromCustomResource(cr, i.specReplicasPath, i.statusReplicasPath, i.labelSelectorPath) + if err != nil { + return nil, err + } + if !replicasFound { + oldScale.Spec.Replicas = invalidSpecReplicas // signal that this was not set before + } + + obj, err := i.reqObjInfo.UpdatedObject(ctx, oldScale) + if err != nil { + return nil, err + } + if obj == nil { + return nil, apierrors.NewBadRequest(fmt.Sprintf("nil update passed to Scale")) + } + + scale, ok := obj.(*autoscalingv1.Scale) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("wrong object passed to Scale update: %v", obj)) + } + + if scale.Spec.Replicas == invalidSpecReplicas { + return nil, apierrors.NewBadRequest(fmt.Sprintf("the spec replicas field %q cannot be empty", i.specReplicasPath)) + } + + specReplicasPath := strings.TrimPrefix(i.specReplicasPath, ".") // ignore leading period + + if err := unstructured.SetNestedField(cr.Object, int64(scale.Spec.Replicas), strings.Split(specReplicasPath, ".")...); err != nil { + return nil, err + } + if len(scale.ResourceVersion) != 0 { + // The client provided a resourceVersion precondition. + // Set that precondition and return any conflict errors to the client. + cr.SetResourceVersion(scale.ResourceVersion) + } + return cr, nil +} From da246010c3d875f01f9c61ff220923c4e2be936b Mon Sep 17 00:00:00 2001 From: knight42 Date: Tue, 12 Nov 2019 17:27:00 +0800 Subject: [PATCH 2/2] test(cr::update): add corresponding tests Signed-off-by: knight42 --- .../pkg/registry/customresource/etcd_test.go | 180 ++++++++++++++++++ 1 file changed, 180 insertions(+) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd_test.go index bbf5c9c742e..132b486d6eb 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd_test.go @@ -17,6 +17,8 @@ limitations under the License. package customresource_test import ( + "context" + "fmt" "reflect" "strings" "testing" @@ -526,6 +528,184 @@ func TestScaleUpdateWithoutSpecReplicas(t *testing.T) { } } +func TestScaleUpdateWithoutResourceVersion(t *testing.T) { + storage, server := newStorage(t) + defer server.Terminate(t) + defer storage.CustomResource.Store.DestroyFunc() + + name := "foo" + + var cr unstructured.Unstructured + ctx := genericapirequest.WithNamespace(genericapirequest.NewContext(), metav1.NamespaceDefault) + key := "/noxus/" + metav1.NamespaceDefault + "/" + name + if err := storage.CustomResource.Storage.Create(ctx, key, &validCustomResource, &cr, 0, false); err != nil { + t.Fatalf("error setting new custom resource (key: %s) %v: %v", key, validCustomResource, err) + } + + replicas := int32(8) + update := autoscalingv1.Scale{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: autoscalingv1.ScaleSpec{ + Replicas: replicas, + }, + } + + if _, _, err := storage.Scale.Update(ctx, update.Name, rest.DefaultUpdatedObjectInfo(&update), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}); err != nil { + t.Fatalf("error updating scale %v: %v", update, err) + } + + obj, err := storage.Scale.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + t.Fatalf("error fetching scale for %s: %v", name, err) + } + scale := obj.(*autoscalingv1.Scale) + if scale.Spec.Replicas != replicas { + t.Errorf("wrong replicas count: expected: %d got: %d", replicas, scale.Spec.Replicas) + } +} + +func TestScaleUpdateWithoutResourceVersionWithConflicts(t *testing.T) { + storage, server := newStorage(t) + defer server.Terminate(t) + defer storage.CustomResource.Store.DestroyFunc() + + name := "foo" + + var cr unstructured.Unstructured + ctx := genericapirequest.WithNamespace(genericapirequest.NewContext(), metav1.NamespaceDefault) + key := "/noxus/" + metav1.NamespaceDefault + "/" + name + if err := storage.CustomResource.Storage.Create(ctx, key, &validCustomResource, &cr, 0, false); err != nil { + t.Fatalf("error setting new custom resource (key: %s) %v: %v", key, validCustomResource, err) + } + + fetchObject := func(name string) (*unstructured.Unstructured, error) { + gotObj, err := storage.CustomResource.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + return nil, fmt.Errorf("error fetching custom resource %s: %v", name, err) + } + return gotObj.(*unstructured.Unstructured), nil + } + + applyPatch := func(labelName, labelValue string) rest.TransformFunc { + return func(_ context.Context, _, currentObject runtime.Object) (objToUpdate runtime.Object, patchErr error) { + o := currentObject.(metav1.Object) + o.SetLabels(map[string]string{ + labelName: labelValue, + }) + return currentObject, nil + } + } + + errs := make(chan error, 1) + rounds := 100 + go func() { + // continuously submits a patch that updates a label and verifies the label update was effective + labelName := "timestamp" + for i := 0; i < rounds; i++ { + expectedLabelValue := fmt.Sprint(i) + update, err := fetchObject(name) + if err != nil { + errs <- err + return + } + setNestedField(update, expectedLabelValue, "metadata", "labels", labelName) + if _, _, err := storage.CustomResource.Update(ctx, name, rest.DefaultUpdatedObjectInfo(nil, applyPatch(labelName, fmt.Sprint(i))), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}); err != nil { + + errs <- fmt.Errorf("error updating custom resource label: %v", err) + return + } + + gotObj, err := fetchObject(name) + if err != nil { + errs <- err + return + } + gotLabelValue, _, err := unstructured.NestedString(gotObj.Object, "metadata", "labels", labelName) + if err != nil { + errs <- fmt.Errorf("error getting label %s of custom resource %s: %v", labelName, name, err) + return + } + if gotLabelValue != expectedLabelValue { + errs <- fmt.Errorf("wrong label value: expected: %s, got: %s", expectedLabelValue, gotLabelValue) + return + } + } + }() + + replicas := int32(0) + update := autoscalingv1.Scale{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + } + // continuously submits a scale update without a resourceVersion for a monotonically increasing replica value + // and verifies the scale update was effective + for i := 0; i < rounds; i++ { + select { + case err := <-errs: + t.Fatal(err) + default: + replicas++ + update.Spec.Replicas = replicas + if _, _, err := storage.Scale.Update(ctx, update.Name, rest.DefaultUpdatedObjectInfo(&update), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}); err != nil { + t.Fatalf("error updating scale %v: %v", update, err) + } + + obj, err := storage.Scale.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + t.Fatalf("error fetching scale for %s: %v", name, err) + } + scale := obj.(*autoscalingv1.Scale) + if scale.Spec.Replicas != replicas { + t.Errorf("wrong replicas count: expected: %d got: %d", replicas, scale.Spec.Replicas) + } + } + } +} + +func TestScaleUpdateWithResourceVersionWithConflicts(t *testing.T) { + storage, server := newStorage(t) + defer server.Terminate(t) + defer storage.CustomResource.Store.DestroyFunc() + + name := "foo" + + var cr unstructured.Unstructured + ctx := genericapirequest.WithNamespace(genericapirequest.NewContext(), metav1.NamespaceDefault) + key := "/noxus/" + metav1.NamespaceDefault + "/" + name + if err := storage.CustomResource.Storage.Create(ctx, key, &validCustomResource, &cr, 0, false); err != nil { + t.Fatalf("error setting new custom resource (key: %s) %v: %v", key, validCustomResource, err) + } + + obj, err := storage.Scale.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + t.Fatalf("error fetching scale for %s: %v", name, err) + } + scale, ok := obj.(*autoscalingv1.Scale) + if !ok { + t.Fatalf("%v is not of the type autoscalingv1.Scale", scale) + } + + replicas := int32(12) + update := autoscalingv1.Scale{ + ObjectMeta: scale.ObjectMeta, + Spec: autoscalingv1.ScaleSpec{ + Replicas: replicas, + }, + } + update.ResourceVersion = "1" + + _, _, err = storage.Scale.Update(ctx, update.Name, rest.DefaultUpdatedObjectInfo(&update), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if err == nil { + t.Fatal("expecting an update conflict error") + } + if !errors.IsConflict(err) { + t.Fatalf("unexpected error, expecting an update conflict but got %v", err) + } +} + func setSpecReplicas(u *unstructured.Unstructured, replicas int64) { setNestedField(u, replicas, "spec", "replicas") }