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.

Sign up for GitHub

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

Use Patch instead of SSA for Pod Disruption condition #121103

Merged
merged 1 commit into from
Oct 19, 2023
Merged

Use Patch instead of SSA for Pod Disruption condition #121103

merged 1 commit into from
Oct 19, 2023

Conversation

mimowo
Copy link
Contributor

@mimowo mimowo commented Oct 10, 2023 •
edited

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #118261

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixed a regression in default configurations, which enabled PodDisruptionConditions by default, 
that prevented the control plane's pod garbage collector from deleting pods that contained duplicated field keys (env. variables with repeated keys or container ports).

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

https://github.com/kubernetes/enhancements/tree/master/keps/sig-apps/3329-retriable-and-non-retriable-failures

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/regression Categorizes issue or PR as related to a regression from a prior release. 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. labels Oct 10, 2023
@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. 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. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 10, 2023
@mimowo mimowo force-pushed the dont-use-ssa-for-podgc branch 2 times, most recently from 76819e1 to 8e34a95 Compare October 10, 2023 10:47
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 10, 2023
@mimowo
Copy link
Contributor Author

mimowo commented Oct 10, 2023

For the record, failing integration test after 8e34a95:
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/121103/pull-kubernetes-integration/1711694973037973504.

@mimowo mimowo changed the title WIP: Do not use SSA to update pod status in PodGC Do not use SSA to update pod status in PodGC Oct 10, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 10, 2023
@mimowo
Copy link
Contributor Author

mimowo commented Oct 10, 2023

/assign @alculquicondor

@mimowo mimowo mentioned this pull request Oct 10, 2023
Closed
mimowo added a commit to mimowo/cloud-provider-gcp that referenced this pull request Oct 11, 2023
@mimowo
Don't use SSA in gcp-controller manager PodGC
7c252f8 
This change in analogous to: kubernetes/kubernetes#121103.

Required because of kubernetes/kubernetes#118261
@mimowo mimowo mentioned this pull request Oct 11, 2023
Merged
mimowo added a commit to mimowo/cloud-provider-gcp that referenced this pull request Oct 11, 2023
@mimowo
Don't use SSA in gcp-controller manager PodGC
84d41b3 
This change in analogous to: kubernetes/kubernetes#121103.

Required because of kubernetes/kubernetes#118261
@liggitt
Copy link
Member

liggitt commented Oct 11, 2023

/kind regression

Thanks for the PR. Please include "Fixes a regression in default configurations in $version-that-enabled-disruption-condition-by-default ..."

@mimowo
Copy link
Contributor Author

mimowo commented Oct 11, 2023 •
edited

Thanks for the PR. Please include "Fixes a regression in default configurations in $version-that-enabled-disruption-condition-by-default ..."

Sure, added to the release note. I want to cherry-pick this down to 1.26.

@mimowo
Copy link
Contributor Author

mimowo commented Oct 19, 2023

@mimowo happy to lgtm again when you squash by hand

Thanks, got the notification, I will soon.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2023
@mimowo
Copy link
Contributor Author

mimowo commented Oct 19, 2023

Ready, thank you all for reviewing 🎉

@alculquicondor
Copy link
Member

/hold cancel
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 19, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: dce2e98adc820b4abca8b03f3a8cf11b09e1cb7f

@k8s-ci-robot k8s-ci-robot merged commit 88d9573 into kubernetes:master Oct 19, 2023
15 checks passed
SIG Node CI/Test Board automation moved this from Archive-it to Done Oct 19, 2023
SIG Node PR Triage automation moved this from Triage to Done Oct 19, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Oct 19, 2023
@liggitt
Copy link
Member

liggitt commented Oct 20, 2023

thanks, go ahead and open the picks back to 1.26/1.27/1.28

This was referenced Oct 20, 2023
Closed
Merged
Merged
Merged
@mimowo
Copy link
Contributor Author

mimowo commented Oct 20, 2023

thanks, go ahead and open the picks back to 1.26/1.27/1.28

Sure, opened:

This was referenced Oct 20, 2023
Open
Closed
Open
k8s-ci-robot added a commit that referenced this pull request Oct 23, 2023
@k8s-ci-robot
Merge pull request #121379 from mimowo/automated-cherry-pick-of-#1211…
9cb0244 
…03-upstream-release-1.28

Automated cherry pick of #121103: Use Patch instead of SSA for Pod Disruption condition
k8s-ci-robot added a commit that referenced this pull request Oct 23, 2023
@k8s-ci-robot
Merge pull request #121381 from mimowo/automated-cherry-pick-of-#1211…
746dfad 
…03-upstream-release-1.26

Automated cherry pick of #121103: Use Patch instead of SSA for Pod Disruption condition
k8s-ci-robot added a commit that referenced this pull request Oct 23, 2023
@k8s-ci-robot
Merge pull request #121380 from mimowo/automated-cherry-pick-of-#1211…
ab300fb 
…03-upstream-release-1.27

Automated cherry pick of #121103: Use Patch instead of SSA for Pod Disruption condition
@sftim
Copy link
Contributor

sftim commented Oct 25, 2023

Changelog suggestion

Fixed a regression in default configurations in that enabled PodDisruptionConditions by default, 
that then prevented the control plane's pod garbage collector from deleting pods that contained duplicated field keys (env. variables with repeated keys or container ports).

@mimowo
Copy link
Contributor Author

mimowo commented Oct 25, 2023

Changelog suggestion

Fixed a regression in default configurations in that enabled PodDisruptionConditions by default, 
that then prevented the control plane's pod garbage collector from deleting pods that contained duplicated field keys (env. variables with repeated keys or container ports).

Thanks, applied the note, just rephrased slightly the first part. PTAL.

@alculquicondor alculquicondor mentioned this pull request Nov 1, 2023
Open
@mimowo mimowo deleted the dont-use-ssa-for-podgc branch November 29, 2023 15:00
@sdodson sdodson mentioned this pull request Dec 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k Awaiting requested review from deads2k

@mortent mortent Awaiting requested review from mortent

@apelisse apelisse Awaiting requested review from apelisse

@soltysh soltysh Awaiting requested review from soltysh

@atiratree atiratree Awaiting requested review from atiratree

@dims dims Awaiting requested review from dims

@tzneal tzneal Awaiting requested review from tzneal

@alculquicondor alculquicondor Awaiting requested review from alculquicondor

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@dims dims

@tzneal tzneal

@soltysh soltysh

@liggitt liggitt

@alculquicondor alculquicondor

@apelisse apelisse

@atiratree atiratree

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. 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
SIG Apps
Archived in project
SIG Auth
Archived in project
SIG Node CI/Test Board
Done
SIG Node PR Triage
Archived in project
SIG Node PR Triage
Done
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.

Pod Garbage collector fails to clean up PODs from nodes that are not running anymore