Skip to content

Commit

Permalink
API-initiated eviction: handle deleteOptions correctly
Browse files Browse the repository at this point in the history 
when adding a DisruptionTarget condition into a pod that will be deleted

- handle ResourceVersion and Preconditions correctly
- handle DryRun option correctly

Co-authored-by: Jordan Liggitt jordan@liggitt.net
  • Loading branch information
@atiratree
atiratree committed Mar 17, 2023
1 parent de9ce03 commit 51c0e23
Show file tree
Hide file tree
Showing 3 changed files with 285 additions and 10 deletions.
43 changes: 37 additions & 6 deletions pkg/registry/core/pod/storage/eviction.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand All @@ -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
Expand Down
105 changes: 104 additions & 1 deletion pkg/registry/core/pod/storage/eviction_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 51c0e23

Please sign in to comment.