API-initiated eviction: handle deleteOptions correctly #116554

atiratree · 2023-03-13T21:53:41Z

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

when adding a DisruptionTarget condition into a pod that will be deleted by an eviction

handle ResourceVersion and Preconditions correctly
handle DryRun option correctly

Which issue(s) this PR fixes:

Fixes #116552

Special notes for your reviewer:

more details in the issue

Does this PR introduce a user-facing change?

Fixed two regressions introduced by the PodDisruptionConditions feature (on by default in 1.26):
* pod eviction API calls returned spurious precondition errors and required a second evict API call to succeed
* dry-run eviction API calls persisted a DisruptionTarget condition into the pod being evicted

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

https://github.com/kubernetes/enhancements/issues/3329

atiratree · 2023-03-13T22:00:33Z

/triage accepted
/priority important-soon

This is a regression since, it requires more eviction requests for the kubectl drain to drain a node. And it could affect other tools that do eviction.

@liggitt @soltysh can you please review?

atiratree · 2023-03-13T22:01:23Z

cc @ravisantoshgudimetla

liggitt · 2023-03-13T22:06:03Z

This is a regression since, it requires more eviction requests for the kubectl drain to drain a node. And it could affect other tools that do eviction.

what PR regressed this?

atiratree · 2023-03-13T22:12:10Z

it was added in #110959 as an alpha and promoted to a beta and changed to default in #113360

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

atiratree · 2023-03-13T23:54:09Z

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

 	}
-	_, _, err := r.store.Delete(ctx, name, rest.ValidateAllObjectFunc, options)
+	_, _, err = r.store.Delete(ctx, name, rest.ValidateAllObjectFunc, deleteOptionsCopy)


one think that could be problematic is if somebody does update after our update, before we try to delete the pod. In that case we should do a rollback of the condition. Do we care about such a race? Should I implement the rollback? @liggitt

I don't know... I thought we had a controller reconciling bogus conditions

We have a disruption controller which cleans up stale DisruptionTarget conditions:

kubernetes/pkg/controller/disruption/disruption.go

Line 729 in 152876a

func (dc *DisruptionController) syncStalePodDisruption(ctx context.Context, key string) error {

. By stale we mean conditions which are 2min old if there was no deletion in the meanwhile.

Cool, I think, because it is an unlikely race, it should be enough to leave it to the controller

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

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

liggitt · 2023-03-14T02:15:32Z

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

 	}
-	_, _, err := r.store.Delete(ctx, name, rest.ValidateAllObjectFunc, options)
+	_, _, err = r.store.Delete(ctx, name, rest.ValidateAllObjectFunc, deleteOptionsCopy)


I don't know... I thought we had a controller reconciling bogus conditions

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

mimowo · 2023-03-14T08:56:47Z

@atiratree thank you for working on the fix. I think we should cherry-pick it to 1.26.

atiratree · 2023-03-14T12:44:34Z

+1 for the cherry-pick

liggitt · 2023-03-17T00:18:21Z

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/116554/pull-kubernetes-unit/1636476265563164672

bobbypage · 2023-03-17T00:45:20Z

Thanks for the link. Is this a kubelet issue with kubemark?

It seems like the test that failed is cmd/kubemark/app TestHollowNode/kubelet (3.00s) and possibly related to kubemark, which is a new test added as part of #116645

bobbypage · 2023-03-17T00:50:07Z

Filed #116696 for the race

liggitt · 2023-03-17T00:53:00Z

kubemark

I think it's kubemark or the kubemark unit test

liggitt · 2023-03-17T02:05:54Z

test/integration/evictions/evictions_test.go

+			}
+
+			eviction := newV1Eviction(ns.Name, updatedPod.Name, deleteOption)
+			err = clientSet.CoreV1().RESTClient().Post().


can use noRetriesRESTClient here as well and call the normal clientset Evict method

updated, thanks!

alculquicondor

not an apiserver expert, but:
/lgtm

k8s-ci-robot · 2023-03-17T15:27:32Z

LGTM label has been added.

Git tree hash: ccfc0a2e2ad90f7a66fcea47e11ddaa2ef8a40c7

alculquicondor · 2023-03-17T15:27:58Z

/sig api-machinery

test/integration/evictions/evictions_test.go

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

liggitt · 2023-03-17T21:36:00Z

/lgtm
/approve

k8s-ci-robot · 2023-03-17T21:36:07Z

LGTM label has been added.

Git tree hash: 0a0e050fc81b12e21b5c9fe7efeb0f55dc36e63e

k8s-ci-robot · 2023-03-17T21:36:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/registry/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt · 2023-03-17T21:36:28Z

once this passes tests, go ahead and open a cherry-pick to release-1.26

atiratree · 2023-03-17T22:29:13Z

flaky test Test_Run_OneVolumeDetachFailNodeWithReadWriteOnce
/test pull-kubernetes-unit

atiratree · 2023-03-20T10:21:21Z

opened a cherry-pick #116750

…16554-upstream-release-1.26 Automated cherry pick of #116554: API-initiated eviction: handle deleteOptions correctly

k8s-ci-robot requested review from deads2k and wojtek-t March 13, 2023 21:54

atiratree mentioned this pull request Mar 13, 2023

Add DaemonSet support in PDB #98307

Closed

atiratree mentioned this pull request Mar 13, 2023

Retriable and non-retriable Pod failures for Jobs kubernetes/enhancements#3329

Open

8 tasks

liggitt reviewed Mar 13, 2023

View reviewed changes

atiratree force-pushed the eviction-resource-version-fix branch from a7e51d6 to 4ca6333 Compare March 13, 2023 23:37

atiratree commented Mar 13, 2023

View reviewed changes

liggitt reviewed Mar 14, 2023

View reviewed changes

atiratree force-pushed the eviction-resource-version-fix branch from 4ca6333 to 2e5222c Compare March 14, 2023 12:15

liggitt reviewed Mar 17, 2023

View reviewed changes

atiratree force-pushed the eviction-resource-version-fix branch from fbdd4e3 to bda1607 Compare March 17, 2023 14:37

alculquicondor reviewed Mar 17, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Mar 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2023

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 17, 2023

liggitt reviewed Mar 17, 2023

View reviewed changes

test/integration/evictions/evictions_test.go Show resolved Hide resolved

API-initiated eviction: handle deleteOptions correctly

51c0e23

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

atiratree force-pushed the eviction-resource-version-fix branch from bda1607 to 51c0e23 Compare March 17, 2023 21:28

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2023

k8s-ci-robot requested review from alculquicondor and liggitt March 17, 2023 21:28

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2023

liggitt moved this from Changes requested to API review completed, 1.27 in API Reviews Mar 17, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2023

k8s-ci-robot merged commit fe91bc2 into kubernetes:master Mar 17, 2023
13 checks passed

atiratree mentioned this pull request Mar 20, 2023

Automated cherry pick of #116554: API-initiated eviction: handle deleteOptions correctly #116750

Merged

k8s-ci-robot added a commit that referenced this pull request Mar 30, 2023

Merge pull request #116750 from atiratree/automated-cherry-pick-of-#1…

863386a

…16554-upstream-release-1.26 Automated cherry pick of #116554: API-initiated eviction: handle deleteOptions correctly

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API-initiated eviction: handle deleteOptions correctly #116554

API-initiated eviction: handle deleteOptions correctly #116554

atiratree commented Mar 13, 2023 •

edited by liggitt

atiratree commented Mar 13, 2023

atiratree commented Mar 13, 2023

liggitt commented Mar 13, 2023

atiratree commented Mar 13, 2023

atiratree Mar 13, 2023

liggitt Mar 14, 2023

mimowo Mar 14, 2023

atiratree Mar 14, 2023

liggitt Mar 14, 2023

mimowo commented Mar 14, 2023

atiratree commented Mar 14, 2023

liggitt commented Mar 17, 2023

bobbypage commented Mar 17, 2023 •

edited

bobbypage commented Mar 17, 2023

liggitt commented Mar 17, 2023

liggitt Mar 17, 2023

atiratree Mar 17, 2023

alculquicondor left a comment

k8s-ci-robot commented Mar 17, 2023

alculquicondor commented Mar 17, 2023

liggitt commented Mar 17, 2023

k8s-ci-robot commented Mar 17, 2023

k8s-ci-robot commented Mar 17, 2023

liggitt commented Mar 17, 2023

atiratree commented Mar 17, 2023

atiratree commented Mar 20, 2023

API-initiated eviction: handle deleteOptions correctly #116554

API-initiated eviction: handle deleteOptions correctly #116554

Conversation

atiratree commented Mar 13, 2023 • edited by liggitt

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

atiratree commented Mar 13, 2023

atiratree commented Mar 13, 2023

liggitt commented Mar 13, 2023

atiratree commented Mar 13, 2023

atiratree Mar 13, 2023

Choose a reason for hiding this comment

liggitt Mar 14, 2023

Choose a reason for hiding this comment

mimowo Mar 14, 2023

Choose a reason for hiding this comment

atiratree Mar 14, 2023

Choose a reason for hiding this comment

liggitt Mar 14, 2023

Choose a reason for hiding this comment

mimowo commented Mar 14, 2023

atiratree commented Mar 14, 2023

liggitt commented Mar 17, 2023

bobbypage commented Mar 17, 2023 • edited

bobbypage commented Mar 17, 2023

liggitt commented Mar 17, 2023

liggitt Mar 17, 2023

Choose a reason for hiding this comment

atiratree Mar 17, 2023

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 17, 2023

alculquicondor commented Mar 17, 2023

liggitt commented Mar 17, 2023

k8s-ci-robot commented Mar 17, 2023

k8s-ci-robot commented Mar 17, 2023

liggitt commented Mar 17, 2023

atiratree commented Mar 17, 2023

atiratree commented Mar 20, 2023

atiratree commented Mar 13, 2023 •

edited by liggitt

bobbypage commented Mar 17, 2023 •

edited