From 6f648c15a2737f2da9b01c5346e5c1c4a85aafb5 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Fri, 19 Jan 2024 16:10:30 -0500 Subject: [PATCH] Add retry around create Kubernetes-commit: a05db0dd22a68a9c443a9f01cc1b8f6397fd6a9f --- pkg/features/kube_features.go | 8 ++ pkg/registry/generic/registry/store.go | 48 ++++++++++- pkg/registry/generic/registry/store_test.go | 93 +++++++++++++++++++++ 3 files changed, 148 insertions(+), 1 deletion(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index d3a21dc84..6d484c0a8 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -189,6 +189,12 @@ const ( // clients. UnauthenticatedHTTP2DOSMitigation featuregate.Feature = "UnauthenticatedHTTP2DOSMitigation" + // owner: @jpbetz + // alpha: v1.30 + // Resource create requests using generateName are retried automatically by the apiserver + // if the generated name conflicts with an existing resource name, up to a maximum number of 7 retries. + RetryGenerateName featuregate.Feature = "RetryGenerateName" + // owner: @caesarxuchao @roycaihw // alpha: v1.20 // @@ -294,6 +300,8 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS RemainingItemCount: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.32 + RetryGenerateName: {Default: false, PreRelease: featuregate.Alpha}, + ServerSideApply: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.29 ServerSideFieldValidation: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.29 diff --git a/pkg/registry/generic/registry/store.go b/pkg/registry/generic/registry/store.go index 028053952..a8e01708a 100644 --- a/pkg/registry/generic/registry/store.go +++ b/pkg/registry/generic/registry/store.go @@ -23,6 +23,8 @@ import ( "sync" "time" + "sigs.k8s.io/structured-merge-diff/v4/fieldpath" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/validation" @@ -40,15 +42,16 @@ import ( "k8s.io/apimachinery/pkg/watch" "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/registry/generic" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage" storeerr "k8s.io/apiserver/pkg/storage/errors" "k8s.io/apiserver/pkg/storage/etcd3/metrics" "k8s.io/apiserver/pkg/util/dryrun" + utilfeature "k8s.io/apiserver/pkg/util/feature" flowcontrolrequest "k8s.io/apiserver/pkg/util/flowcontrol/request" "k8s.io/client-go/tools/cache" - "sigs.k8s.io/structured-merge-diff/v4/fieldpath" "k8s.io/klog/v2" ) @@ -392,11 +395,54 @@ func (e *Store) ListPredicate(ctx context.Context, p storage.SelectionPredicate, // finishNothing is a do-nothing FinishFunc. func finishNothing(context.Context, bool) {} +// maxNameGenerationCreateAttempts is the maximum number of +// times create will be attempted when generateName is used +// and create attempts fails due to name conflict errors. +// Each attempt uses a newly randomly generated name. +// 8 was selected as the max because it is sufficient to generate +// 1 million names per generateName prefix, with only a 0.1% +// probability of any generated name conflicting with existing names. +// Without retry, a 0.1% probability occurs at ~500 +// generated names and a 50% probability occurs at ~4500 +// generated names. +const maxNameGenerationCreateAttempts = 8 + // Create inserts a new item according to the unique key from the object. // Note that registries may mutate the input object (e.g. in the strategy // hooks). Tests which call this might want to call DeepCopy if they expect to // be able to examine the input and output objects for differences. func (e *Store) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { + if utilfeature.DefaultFeatureGate.Enabled(features.RetryGenerateName) && needsNameGeneration(obj) { + return e.createWithGenerateNameRetry(ctx, obj, createValidation, options) + } + + return e.create(ctx, obj, createValidation, options) +} + +// needsNameGeneration returns true if the obj has a generateName but no name. +func needsNameGeneration(obj runtime.Object) bool { + if objectMeta, err := meta.Accessor(obj); err == nil { + if len(objectMeta.GetGenerateName()) > 0 && len(objectMeta.GetName()) == 0 { + return true + } + } + return false +} + +// createWithGenerateNameRetry attempts to create obj up to maxNameGenerationCreateAttempts +// when create fails due to a name conflict error. Each attempt randomly generates a new +// name based on generateName. +func (e *Store) createWithGenerateNameRetry(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (resultObj runtime.Object, err error) { + for i := 0; i < maxNameGenerationCreateAttempts; i++ { + resultObj, err = e.create(ctx, obj.DeepCopyObject(), createValidation, options) + if err == nil || !apierrors.IsAlreadyExists(err) { + return resultObj, err + } + } + return resultObj, err +} + +func (e *Store) create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { var finishCreate FinishFunc = finishNothing // Init metadata as early as possible. diff --git a/pkg/registry/generic/registry/store_test.go b/pkg/registry/generic/registry/store_test.go index 61c05e423..01f8b9db2 100644 --- a/pkg/registry/generic/registry/store_test.go +++ b/pkg/registry/generic/registry/store_test.go @@ -30,6 +30,7 @@ import ( "time" fuzz "github.com/google/gofuzz" + "k8s.io/apimachinery/pkg/api/apitesting" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" @@ -48,6 +49,7 @@ import ( "k8s.io/apiserver/pkg/apis/example" examplev1 "k8s.io/apiserver/pkg/apis/example/v1" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/registry/generic" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage" @@ -56,7 +58,9 @@ import ( "k8s.io/apiserver/pkg/storage/names" "k8s.io/apiserver/pkg/storage/storagebackend/factory" storagetesting "k8s.io/apiserver/pkg/storage/testing" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/tools/cache" + featuregatetesting "k8s.io/component-base/featuregate/testing" ) var scheme = runtime.NewScheme() @@ -383,6 +387,95 @@ func TestStoreCreate(t *testing.T) { } } +// sequentialNameGenerator generates names by appending a monotonically-increasing integer to the base. +type sequentialNameGenerator struct { + seq int +} + +func (m *sequentialNameGenerator) GenerateName(base string) string { + generated := fmt.Sprintf("%s%d", base, m.seq) + m.seq++ + return generated +} + +func TestStoreCreateWithRetryNameGenerate(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RetryGenerateName, true)() + + namedObj := func(id int) *example.Pod { + return &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("prefix-%d", id), Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + } + + generateNameObj := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{GenerateName: "prefix-", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + + seqNameGenerator := &sequentialNameGenerator{} + registry.CreateStrategy = &testRESTStrategy{scheme, seqNameGenerator, true, false, true} + + for i := 0; i < 7; i++ { + _, err := registry.Create(testContext, namedObj(i), rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + } + generated, err := registry.Create(testContext, generateNameObj, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + generatedMeta, err := meta.Accessor(generated) + if err != nil { + t.Fatal(err) + } + if generatedMeta.GetName() != "prefix-7" { + t.Errorf("Expected prefix-7 but got %s", generatedMeta.GetName()) + } + + // Now that 8 generated names (0..7) are claimed, 8 name generation attempts will not be enough + // and create should return an already exists error. + seqNameGenerator.seq = 0 + _, err = registry.Create(testContext, generateNameObj, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err == nil || !errors.IsAlreadyExists(err) { + t.Error("Expected already exists error") + } +} + +func TestStoreCreateWithRetryNameGenerateFeatureDisabled(t *testing.T) { + namedObj := func(id int) *example.Pod { + return &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("prefix-%d", id), Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + } + + generateNameObj := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{GenerateName: "prefix-", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + + registry.CreateStrategy = &testRESTStrategy{scheme, &sequentialNameGenerator{}, true, false, true} + + _, err := registry.Create(testContext, namedObj(0), rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + _, err = registry.Create(testContext, generateNameObj, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err == nil || !errors.IsAlreadyExists(err) { + t.Error("Expected already exists error") + } +} + func TestNewCreateOptionsFromUpdateOptions(t *testing.T) { f := fuzz.New().NilChance(0.0).NumElements(1, 1)