diff --git a/go.mod b/go.mod index 16aa2a795..426794266 100644 --- a/go.mod +++ b/go.mod @@ -47,7 +47,7 @@ require ( k8s.io/api v0.0.0-20210806000319-499b6f90564c k8s.io/apimachinery v0.0.0-20210805051055-f7769293e6f1 k8s.io/client-go v0.0.0-20210806000600-0f5acb8c39dd - k8s.io/component-base v0.0.0-20210805120716-92bebfd2c985 + k8s.io/component-base v0.0.0-20210806040719-3cb663ee50af k8s.io/klog/v2 v2.9.0 k8s.io/kube-openapi v0.0.0-20210421082810-95288971da7e k8s.io/utils v0.0.0-20210707171843-4b05e18ac7d9 @@ -60,5 +60,5 @@ replace ( k8s.io/api => k8s.io/api v0.0.0-20210806000319-499b6f90564c k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20210805051055-f7769293e6f1 k8s.io/client-go => k8s.io/client-go v0.0.0-20210806000600-0f5acb8c39dd - k8s.io/component-base => k8s.io/component-base v0.0.0-20210805120716-92bebfd2c985 + k8s.io/component-base => k8s.io/component-base v0.0.0-20210806040719-3cb663ee50af ) diff --git a/go.sum b/go.sum index 609e21393..33ffe86cb 100644 --- a/go.sum +++ b/go.sum @@ -786,8 +786,8 @@ k8s.io/apimachinery v0.0.0-20210805051055-f7769293e6f1 h1:cVpwhaGeh/tNPBeYbFff3t k8s.io/apimachinery v0.0.0-20210805051055-f7769293e6f1/go.mod h1:O3oNtNadZdeOMxHFVxOreoznohCpy0z6mocxbZr7oJ0= k8s.io/client-go v0.0.0-20210806000600-0f5acb8c39dd h1:rJtT7gZ4LkZG+xtM6/iJvnzOx5XW+fNicRxMOYT0u/w= k8s.io/client-go v0.0.0-20210806000600-0f5acb8c39dd/go.mod h1:AptIKtgqoSsdZsOhDruqHKBn7GeBrnbH5CgVkI7tpYU= -k8s.io/component-base v0.0.0-20210805120716-92bebfd2c985 h1:5AICcEUyTnjI3rCqtY3n7sjZRLrCtI8Ejt9jnY+24kQ= -k8s.io/component-base v0.0.0-20210805120716-92bebfd2c985/go.mod h1:uuSJv3vcTE8Ays2CdTgQdaDyg2nYrkK3jVrLDSV3uAo= +k8s.io/component-base v0.0.0-20210806040719-3cb663ee50af h1:TvSosXU6sV/t6L77kj/UEyYFTzmLdxRnKIOJN5UcAls= +k8s.io/component-base v0.0.0-20210806040719-3cb663ee50af/go.mod h1:68YquEVTuJRDLUMJsRvsUyaFu7wBQnbVZlAV1oMkH58= k8s.io/gengo v0.0.0-20200413195148-3a45101e95ac/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0= k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE= k8s.io/klog/v2 v2.9.0 h1:D7HV+n1V57XeZ0m6tdRkfknthUaM06VFbWldOFh8kzM= diff --git a/pkg/registry/generic/registry/store_test.go b/pkg/registry/generic/registry/store_test.go index 74d57b185..f946a3894 100644 --- a/pkg/registry/generic/registry/store_test.go +++ b/pkg/registry/generic/registry/store_test.go @@ -1158,6 +1158,8 @@ func TestStoreUpdateHooksInnerRetry(t *testing.T) { registry.Decorator = tc.decorator ttlFailDone = false registry.TTLFunc = tc.ttl + // force storage to use a cached object with a non-matching resourceVersion to guarantee a live lookup + retry + created.(*example.Pod).ResourceVersion += "0" registry.Storage.Storage = &staleGuaranteedUpdateStorage{Interface: registry.Storage.Storage, cachedObj: created} _, _, err = registry.Update(testContext, pod.Name, rest.DefaultUpdatedObjectInfo(pod), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) if err != nil && !tc.expectError { diff --git a/pkg/storage/etcd3/store.go b/pkg/storage/etcd3/store.go index 5f36b7108..7f558a588 100644 --- a/pkg/storage/etcd3/store.go +++ b/pkg/storage/etcd3/store.go @@ -230,12 +230,22 @@ func (s *store) conditionalDelete( } // It's possible we're working with stale data. + // Remember the revision of the potentially stale data and the resulting update error + cachedRev := origState.rev + cachedUpdateErr := err + // Actually fetch origState, err = getCurrentState() if err != nil { return err } origStateIsCurrent = true + + // it turns out our cached data was not stale, return the error + if cachedRev == origState.rev { + return cachedUpdateErr + } + // Retry continue } @@ -246,12 +256,22 @@ func (s *store) conditionalDelete( } // It's possible we're working with stale data. + // Remember the revision of the potentially stale data and the resulting update error + cachedRev := origState.rev + cachedUpdateErr := err + // Actually fetch origState, err = getCurrentState() if err != nil { return err } origStateIsCurrent = true + + // it turns out our cached data was not stale, return the error + if cachedRev == origState.rev { + return cachedUpdateErr + } + // Retry continue } @@ -345,12 +365,22 @@ func (s *store) GuaranteedUpdate( } // It's possible we were working with stale data + // Remember the revision of the potentially stale data and the resulting update error + cachedRev := origState.rev + cachedUpdateErr := err + // Actually fetch origState, err = getCurrentState() if err != nil { return err } origStateIsCurrent = true + + // it turns out our cached data was not stale, return the error + if cachedRev == origState.rev { + return cachedUpdateErr + } + // Retry continue } diff --git a/pkg/storage/etcd3/store_test.go b/pkg/storage/etcd3/store_test.go index 041dc1708..5af5f4b93 100644 --- a/pkg/storage/etcd3/store_test.go +++ b/pkg/storage/etcd3/store_test.go @@ -453,15 +453,20 @@ func TestValidateDeletionWithSuggestion(t *testing.T) { key, originalPod := testPropogateStore(ctx, t, store, &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "name"}}) - // Check that validaing fresh object fails. + // Check that validaing fresh object fails is called once and fails. + validationCalls := 0 validationError := fmt.Errorf("validation error") validateNothing := func(_ context.Context, _ runtime.Object) error { + validationCalls++ return validationError } out := &example.Pod{} if err := store.Delete(ctx, key, out, nil, validateNothing, originalPod); err != validationError { t.Errorf("Unexpected failure during deletion: %v", err) } + if validationCalls != 1 { + t.Errorf("validate function should have been called once, called %d", validationCalls) + } // First update, so originalPod is outdated. updatedPod := &example.Pod{} @@ -961,6 +966,35 @@ func TestGuaranteedUpdateWithSuggestionAndConflict(t *testing.T) { if updatedPod2.Name != "foo-3" { t.Errorf("unexpected pod name: %q", updatedPod2.Name) } + + // Third, update using a current version as the suggestion. + // Return an error and make sure that SimpleUpdate is NOT called a second time, + // since the live lookup shows the suggestion was already up to date. + attempts := 0 + updatedPod3 := &example.Pod{} + err = store.GuaranteedUpdate(ctx, key, updatedPod3, false, nil, + storage.SimpleUpdate(func(obj runtime.Object) (runtime.Object, error) { + pod := obj.(*example.Pod) + if pod.Name != updatedPod2.Name || pod.ResourceVersion != updatedPod2.ResourceVersion { + t.Errorf( + "unexpected live object (name=%s, rv=%s), expected name=%s, rv=%s", + pod.Name, + pod.ResourceVersion, + updatedPod2.Name, + updatedPod2.ResourceVersion, + ) + } + attempts++ + return nil, fmt.Errorf("validation or admission error") + }), + updatedPod2, + ) + if err == nil { + t.Fatalf("expected error, got none") + } + if attempts != 1 { + t.Errorf("expected 1 attempt, got %d", attempts) + } } func TestTransformationFailure(t *testing.T) {