Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add worker to clean up stale DisruptionTarget condition #111475

Merged
merged 2 commits into from Aug 2, 2022
Merged

Add worker to clean up stale DisruptionTarget condition #111475

merged 2 commits into from Aug 2, 2022

Conversation

alculquicondor
Copy link
Member

@alculquicondor alculquicondor commented Jul 27, 2022 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

This is a requirement from #110959.

It is possible that controllers that add the DisruptionTarget condition and fail before being able to Delete a pod. When the controller restarts, it might take a different decision, leaving a stale condition. This worker, added to the disruption controller, clears this condition if a DeletionTimestamp is not added to the pod after 2 minutes.

Which issue(s) this PR fixes:

Ref kubernetes/enhancements#3329

Special notes for your reviewer:

The first commit introduces a clock interface to the controller in order to write the unit tests more accurately. I proposed it initially as a separate PR in #111447, but it seems small enough to be in this PR as a separate commit.

This functionality is purposely not gated. This is because if the feature gate PodDisruptionConditions is disabled, we still want to clear the condition.

Does this PR introduce a user-facing change?

If a Pod has a DisruptionTarget condition with status=True for more than 2 minutes without getting a DeletionTimestamp, the control plane resets it to status=False

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/test labels Jul 27, 2022
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 27, 2022
@alculquicondor
Copy link
Member Author

/sig apps
/assign @liggitt @soltysh

cc @mimowo

@alculquicondor
Copy link
Member Author

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 27, 2022
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jul 27, 2022
@alculquicondor alculquicondor mentioned this pull request Jul 27, 2022
Open
8 tasks
@mimowo mimowo mentioned this pull request Jul 28, 2022
Merged
soltysh
soltysh approved these changes Jul 28, 2022
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nits, controller changes
/lgtm
/approve

pkg/controller/disruption/disruption.go Outdated
return
}
dc.stalePodDisruptionQueue.AddAfter(key, d)
klog.InfoS("Enqueued pod for stale DisruptionTarget condition cleanup", "pod", klog.KObj(pod))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: most of the debugging info are at level 4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, left over from my debugging.

pkg/controller/disruption/disruption.go Outdated
dc.queue.Forget(key)
return true
}
utilruntime.HandleError(fmt.Errorf("syncing Pod %v to clear DisruptionTarget condition, requeueing: %v", key.(string), err))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
utilruntime.HandleError(fmt.Errorf("syncing Pod %v to clear DisruptionTarget condition, requeueing: %v", key.(string), err))
utilruntime.HandleError(fmt.Errorf("error syncing Pod %v to clear DisruptionTarget condition, requeueing: %v", key.(string), err))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somewhat disagree on this. This is already an error, so it shouldn't need the word error again. And if some day the implementation of HandleError changes and wraps the given error, it wouldn't read well.

But sure, given the current implementation of HandleError, your recommendation makes sense.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2022
@alculquicondor
Copy link
Member Author

/hold
to prevent premature merge

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2022
aojea
aojea reviewed Aug 2, 2022
View reviewed changes
pkg/controller/disruption/disruption_test.go
@@ -181,9 +187,8 @@ func newFakeDisruptionController() (*disruptionController, *pdbStates) {
dc.rsListerSynced = alwaysReady
dc.dListerSynced = alwaysReady
dc.ssListerSynced = alwaysReady
ctx := context.TODO()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from where ctx.Done() is coming after removing this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's coming from the test. See newFakeDisruptionControllerWithTime

@alculquicondor alculquicondor mentioned this pull request Aug 2, 2022
Merged
@alculquicondor
Add clock interface to disruption controller
dad8454 
To be able to write more precise unit tests in the future

Change-Id: I8f45947dfacca501acd856849bd978fad0f735cd
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 2, 2022
@alculquicondor
Add worker to clean up stale DisruptionTarget condition
4188d9b 
Change-Id: I907fbdf01e7ff08d823fb23aa168ff271d8ff1ee
@alculquicondor
Copy link
Member Author

/assign @lavalamp
for staging/src/k8s.io/client-go/util/workqueue/rate_limiting_queue.go

@lavalamp
Copy link
Member

lavalamp commented Aug 2, 2022

/approve

for client

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2022
@alculquicondor
Copy link
Member Author

/hold cancel
/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 2, 2022
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@mimowo
Copy link
Contributor

mimowo commented Aug 2, 2022

/remove-kind api-change

@k8s-ci-robot k8s-ci-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 2, 2022
soltysh
soltysh approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, janetkuo, lavalamp, soltysh

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:

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

@k8s-ci-robot k8s-ci-robot merged commit bc4c493 into kubernetes:master Aug 2, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Aug 2, 2022
@harshanarayana harshanarayana mentioned this pull request Aug 3, 2022
Merged
@mimowo mimowo mentioned this pull request Aug 9, 2022
Closed
9 tasks
@mimowo mimowo mentioned this pull request Sep 13, 2022
Merged
@aojea aojea mentioned this pull request Sep 20, 2022
Closed
@mimowo mimowo mentioned this pull request Nov 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea aojea left review comments

@mimowo mimowo mimowo left review comments

@soltysh soltysh soltysh approved these changes

@janetkuo janetkuo janetkuo approved these changes

@krmayankk krmayankk Awaiting requested review from krmayankk

Assignees

@soltysh soltysh

@lavalamp lavalamp

@janetkuo janetkuo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@alculquicondor @liggitt @leilajal @mimowo @k8s-ci-robot @lavalamp @k8s-triage-robot @soltysh @janetkuo @aojea