Skip to content

Automated cherry pick of #110256: kubelet: Mark ready condition as false explicitly for terminal pods#110480

Merged
k8s-ci-robot merged 2 commits into
kubernetes:release-1.23from
bobbypage:automated-cherry-pick-of-#110256-upstream-release-1.23
Jun 9, 2022
Merged

Automated cherry pick of #110256: kubelet: Mark ready condition as false explicitly for terminal pods#110480
k8s-ci-robot merged 2 commits into
kubernetes:release-1.23from
bobbypage:automated-cherry-pick-of-#110256-upstream-release-1.23

Conversation

@bobbypage

@bobbypage bobbypage commented Jun 9, 2022 •
edited by liggitt
Loading

Copy link
Copy Markdown
Member

Cherry pick of #110256 on release-1.23.

#110256: kubelet: Mark ready condition as false explicitly for terminal pods

For details on the cherry pick process, see the cherry pick requests page.

Fixed a 1.22 regression kubelet issue that could result in invalid pod status updates to be sent to the api-server where pods would be reported in a terminal phase but also report a ready condition of true in some cases.

@bobbypage
kubelet: Mark ready condition as false explicitly for terminal pods
96ddfd6 
Terminal pods may continue to report a ready condition of true because
there is a delay in reconciling the ready condition of the containers
from the runtime with the pod status. It should be invalid for kubelet
to report a terminal phase with a true ready condition. To fix the
issue, explicitly override the ready condition to false for terminal
pods during status updates.

Signed-off-by: David Porter <david@porter.me>
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Jun 9, 2022
@k8s-ci-robot k8s-ci-robot added do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. 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. labels Jun 9, 2022
@bobbypage

bobbypage commented Jun 9, 2022

Copy link
Copy Markdown
Member Author

Cherrypick was not fully clean - the only manual conflict was on the e2e test update which required updating the go imports.

$ git diff
diff --cc test/e2e_node/node_shutdown_linux_test.go
index baa2fdf3b6c,32ff9ccb549..00000000000
--- a/test/e2e_node/node_shutdown_linux_test.go
+++ b/test/e2e_node/node_shutdown_linux_test.go
@@@ -24,11 -24,16 +24,19 @@@ import
        "fmt"
        "os"
        "path/filepath"
 +      "strconv"
        "time"

 -      apierrors "k8s.io/apimachinery/pkg/api/errors"
        "k8s.io/apimachinery/pkg/fields"
+       "k8s.io/apimachinery/pkg/watch"
+       "k8s.io/client-go/tools/cache"
+       watchtools "k8s.io/client-go/tools/watch"
        "k8s.io/kubectl/pkg/util/podutils"
++<<<<<<< HEAD
++=======
+
+       admissionapi "k8s.io/pod-security-admission/api"
++>>>>>>> test: update graceful node shutdown e2e with watch

        "github.com/onsi/ginkgo"
        "github.com/onsi/gomega"

@k8s-ci-robot k8s-ci-robot requested a review from davidopp June 9, 2022 05:54
@k8s-ci-robot k8s-ci-robot added area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 9, 2022
@bobbypage

bobbypage commented Jun 9, 2022

Copy link
Copy Markdown
Member Author

/triage accepted
/kind bug
/assign @mrunalp @rphillips @dchen1107 @thockin

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jun 9, 2022
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jun 9, 2022
@bobbypage bobbypage changed the title Automated cherry pick of #110256: kubelet: Mark ready condition as false explicitly for Automated cherry pick of #110256: kubelet: Mark ready condition as false explicitly for terminal pods Jun 9, 2022
hussain2022
hussain2022 reviewed Jun 9, 2022
View reviewed changes

@hussain2022 hussain2022 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hussain حسين المنهالي

@bobbypage

bobbypage commented Jun 9, 2022

Copy link
Copy Markdown
Member Author

/retest

@bobbypage bobbypage mentioned this pull request Jun 9, 2022
Merged
@bobbypage bobbypage force-pushed the automated-cherry-pick-of-#110256-upstream-release-1.23 branch from 810338e to b23c229 Compare June 9, 2022 17:51
@bobbypage
test: update graceful node shutdown e2e with watch
772494d 
Use a watch to detect invalid pod status updates in graceful node
shutdown node e2e test. By using a watch, all pod updates will be
captured while the previous logic required polling the api-server which
could miss some intermediate updates.

Signed-off-by: David Porter <david@porter.me>
@bobbypage bobbypage force-pushed the automated-cherry-pick-of-#110256-upstream-release-1.23 branch from b23c229 to 772494d Compare June 9, 2022 17:52
@bobbypage bobbypage mentioned this pull request Jun 9, 2022
Closed
@rphillips

rphillips commented Jun 9, 2022

Copy link
Copy Markdown
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2022
@bobbypage

bobbypage commented Jun 9, 2022

Copy link
Copy Markdown
Member Author

Unit tests failed

=== Failed
=== FAIL: pkg/controller/statefulset TestStatefulSetControlOnDeleteUpdate/Retain/StatefulSetAutoDeletePVCEnabled (0.67s)
W0609 18:07:25.452596   53150 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
W0609 18:07:25.452826   53150 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
W0609 18:07:25.452929   53150 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
W0609 18:07:25.453061   53150 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
W0609 18:07:26.063499   53150 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
W0609 18:07:26.063840   53150 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
W0609 18:07:26.064044   53150 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
W0609 18:07:26.064267   53150 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
    stateful_set_control_test.go:924: monotonic image update and scale up: claim for Pod foo-0 was not created
    --- FAIL: TestStatefulSetControlOnDeleteUpdate/Retain/StatefulSetAutoDeletePVCEnabled (0.67s)

=== FAIL: pkg/controller/statefulset TestStatefulSetControlOnDeleteUpdate (23.80s)

Same as flaky test report - #106663. Fixed in #107443, but it was not cherrypicked to 1.23.

/retest

@thockin

thockin commented Jun 9, 2022

Copy link
Copy Markdown
Member

/approve

@k8s-ci-robot

k8s-ci-robot commented Jun 9, 2022

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobbypage, thockin

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

The pull request process is described here

Details 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2022
@bobbypage

bobbypage commented Jun 9, 2022

Copy link
Copy Markdown
Member Author

ping @kubernetes/release-managers

for cherrypick approvals.

Thank you!

@puerco

puerco commented Jun 9, 2022

Copy link
Copy Markdown
Member

Thanks @bobbypage !
/priority important-soon
/lgtm

@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 needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 9, 2022
@puerco puerco added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jun 9, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Jun 9, 2022
@k8s-ci-robot k8s-ci-robot merged commit 6f31631 into kubernetes:release-1.23 Jun 9, 2022
@liggitt liggitt added the kind/regression Categorizes issue or PR as related to a regression from a prior release. label Jan 30, 2023
@liggitt liggitt removed the kind/regression Categorizes issue or PR as related to a regression from a prior release. label Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidopp davidopp Awaiting requested review from davidopp

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

1 more reviewer

@hussain2022 hussain2022 hussain2022 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@rphillips rphillips

@mrunalp mrunalp

@puerco puerco

@thockin thockin

@dchen1107 dchen1107

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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/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.23

Development

Successfully merging this pull request may close these issues.

9 participants

@bobbypage @rphillips @thockin @k8s-ci-robot @puerco @hussain2022 @mrunalp @liggitt @dchen1107