Merge pull request #116554 from atiratree/eviction-resource-version-fix

API-initiated eviction: handle deleteOptions correctly

pkg/registry/core/pod/storage/eviction.go

@@ -25,6 +25,7 @@ import (
 	policyv1 "k8s.io/api/policy/v1"
 	policyv1beta1 "k8s.io/api/policy/v1beta1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -308,11 +309,30 @@ func (r *EvictionREST) Create(ctx context.Context, name string, obj runtime.Obje
 }
 
 func addConditionAndDeletePod(r *EvictionREST, ctx context.Context, name string, validation rest.ValidateObjectFunc, options *metav1.DeleteOptions) error {
-	if feature.DefaultFeatureGate.Enabled(features.PodDisruptionConditions) {
-		pod, err := getPod(r, ctx, name)
-		if err != nil {
-			return err
+	if !dryrun.IsDryRun(options.DryRun) && feature.DefaultFeatureGate.Enabled(features.PodDisruptionConditions) {
+		getLatestPod := func(_ context.Context, _, oldObj runtime.Object) (runtime.Object, error) {
+			// Throwaway the newObj. We care only about the latest pod obtained from etcd (oldObj).
+			// So we can add DisruptionTarget condition in conditionAppender without conflicts.
+			latestPod := oldObj.(*api.Pod).DeepCopy()
+			if options.Preconditions != nil {
+				if uid := options.Preconditions.UID; uid != nil && len(*uid) > 0 && *uid != latestPod.UID {
+					return nil, errors.NewConflict(
+						schema.GroupResource{Group: "", Resource: "Pod"},
+						latestPod.Name,
+						fmt.Errorf("the UID in the precondition (%s) does not match the UID in record (%s). The object might have been deleted and then recreated", *uid, latestPod.UID),
+					)
+				}
+				if rv := options.Preconditions.ResourceVersion; rv != nil && len(*rv) > 0 && *rv != latestPod.ResourceVersion {
+					return nil, errors.NewConflict(
+						schema.GroupResource{Group: "", Resource: "Pod"},
+						latestPod.Name,
+						fmt.Errorf("the ResourceVersion in the precondition (%s) does not match the ResourceVersion in record (%s). The object might have been modified", *rv, latestPod.ResourceVersion),
+					)
+				}
+			}
+			return latestPod, nil
 		}
+
 		conditionAppender := func(_ context.Context, newObj, _ runtime.Object) (runtime.Object, error) {
 			podObj := newObj.(*api.Pod)
 			podutil.UpdatePodCondition(&podObj.Status, &api.PodCondition{
@@ -324,11 +344,22 @@ func addConditionAndDeletePod(r *EvictionREST, ctx context.Context, name string,
 			return podObj, nil
 		}
 
-		podCopyUpdated := rest.DefaultUpdatedObjectInfo(pod, conditionAppender)
+		podUpdatedObjectInfo := rest.DefaultUpdatedObjectInfo(nil, getLatestPod, conditionAppender) // order important
 
-		if _, _, err = r.store.Update(ctx, name, podCopyUpdated, rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}); err != nil {
+		updatedPodObject, _, err := r.store.Update(ctx, name, podUpdatedObjectInfo, rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{})
+		if err != nil {
 			return err
 		}
+
+		if !resourceVersionIsUnset(options) {
+			newResourceVersion, err := meta.NewAccessor().ResourceVersion(updatedPodObject)
+			if err != nil {
+				return err
+			}
+			// bump the resource version, since we are the one who modified it via the update
+			options = options.DeepCopy()
+			options.Preconditions.ResourceVersion = &newResourceVersion
+		}
 	}
 	_, _, err := r.store.Delete(ctx, name, rest.ValidateAllObjectFunc, options)
 	return err

pkg/registry/core/pod/storage/eviction_test.go

@@ -709,6 +709,109 @@ func TestEvictionPDBStatus(t *testing.T) {
 	}
 }
 
+func TestAddConditionAndDelete(t *testing.T) {
+	cases := []struct {
+		name              string
+		initialPod        bool
+		makeDeleteOptions func(*api.Pod) *metav1.DeleteOptions
+		expectErr         string
+	}{
+		{
+			name:              "simple",
+			initialPod:        true,
+			makeDeleteOptions: func(pod *api.Pod) *metav1.DeleteOptions { return &metav1.DeleteOptions{} },
+		},
+		{
+			name:              "missing",
+			initialPod:        false,
+			makeDeleteOptions: func(pod *api.Pod) *metav1.DeleteOptions { return &metav1.DeleteOptions{} },
+			expectErr:         "not found",
+		},
+		{
+			name:       "valid uid",
+			initialPod: true,
+			makeDeleteOptions: func(pod *api.Pod) *metav1.DeleteOptions {
+				return &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &pod.UID}}
+			},
+		},
+		{
+			name:       "invalid uid",
+			initialPod: true,
+			makeDeleteOptions: func(pod *api.Pod) *metav1.DeleteOptions {
+				badUID := pod.UID + "1"
+				return &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &badUID}}
+			},
+			expectErr: "The object might have been deleted and then recreated",
+		},
+		{
+			name:       "valid resourceVersion",
+			initialPod: true,
+			makeDeleteOptions: func(pod *api.Pod) *metav1.DeleteOptions {
+				return &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{ResourceVersion: &pod.ResourceVersion}}
+			},
+		},
+		{
+			name:       "invalid resourceVersion",
+			initialPod: true,
+			makeDeleteOptions: func(pod *api.Pod) *metav1.DeleteOptions {
+				badRV := pod.ResourceVersion + "1"
+				return &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{ResourceVersion: &badRV}}
+			},
+			expectErr: "The object might have been modified",
+		},
+	}
+
+	testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), metav1.NamespaceDefault)
+
+	storage, _, _, server := newStorage(t)
+	defer server.Terminate(t)
+	defer storage.Store.DestroyFunc()
+
+	client := fake.NewSimpleClientset()
+	evictionRest := newEvictionStorage(storage.Store, client.PolicyV1())
+
+	for _, tc := range cases {
+		for _, conditionsEnabled := range []bool{true, false} {
+			name := fmt.Sprintf("%s_conditions=%v", tc.name, conditionsEnabled)
+			t.Run(name, func(t *testing.T) {
+				defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodDisruptionConditions, conditionsEnabled)()
+				var deleteOptions *metav1.DeleteOptions
+				if tc.initialPod {
+					newPod := validNewPod()
+					createdObj, err := storage.Create(testContext, newPod, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
+					if err != nil {
+						t.Fatal(err)
+					}
+					t.Cleanup(func() {
+						zero := int64(0)
+						storage.Delete(testContext, newPod.Name, rest.ValidateAllObjectFunc, &metav1.DeleteOptions{GracePeriodSeconds: &zero})
+					})
+					deleteOptions = tc.makeDeleteOptions(createdObj.(*api.Pod))
+				} else {
+					deleteOptions = tc.makeDeleteOptions(nil)
+				}
+				if deleteOptions == nil {
+					deleteOptions = &metav1.DeleteOptions{}
+				}
+
+				err := addConditionAndDeletePod(evictionRest, testContext, "foo", rest.ValidateAllObjectFunc, deleteOptions)
+				if err == nil {
+					if tc.expectErr != "" {
+						t.Fatalf("expected err containing %q, got none", tc.expectErr)
+					}
+					return
+				}
+				if tc.expectErr == "" {
+					t.Fatalf("unexpected err: %v", err)
+				}
+				if !strings.Contains(err.Error(), tc.expectErr) {
+					t.Fatalf("expected err containing %q, got %v", tc.expectErr, err)
+				}
+			})
+		}
+	}
+}
+
 func resource(resource string) schema.GroupResource {
 	return schema.GroupResource{Group: "", Resource: resource}
 }
@@ -765,7 +868,7 @@ func (ms *mockStore) Watch(ctx context.Context, options *metainternalversion.Lis
 }
 
 func (ms *mockStore) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) {
-	return nil, false, nil
+	return ms.pod, false, nil
 }
 
 func (ms *mockStore) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {