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

WIP: Components downstream of the pod worker should use it for state #115342

Closed
wants to merge 7 commits into from

Conversation

smarterclayton
Copy link
Contributor

This is a continuation of #113145 to ensure that components that react to "actual" pods (pods that can be running on the node) instead of the "desired" pods for the node (any pod scheduled to the node). A significant realization over the last few years is that pods can have significant lifecycle after they are force deleted - the control plane is unaware of those pods, but nodes must ensure cleanup and accurately react to the use of resources by those still running pods. Our first step was to make the pod worker the source of truth for the runtime state of pods and to have components like volume manager check the pod worker state machine for each pod before taking actions - pods that are terminating may still have running containers, pods that are terminated have no running containers, etc. We then ensured that the kubelet status manager correctly reported the transition of pods from non-terminal to terminal phase after the pod reached specific lifecycle phases.

Most recently, we addressed numerous issues in static pod shutdown to ensure they are properly gracefully shut down as multiple ecosystem distributions depend on static pods for critical lifecycle behavior. A static pod, when updated, changes UID, but two static pods with the same fullname cannot be running at the same time in the Kubelet. This means that components must continue to be aware of those updated static pods after they are updated until they reach final termination, and do so consistently.

#113145 fixes an issue that needs state, but #114994 identified the general class of problem represented by downstreams using "pod manager" (the "desired state of pods") vs "pod workers" (the "actual state of pods"). This PR cleans up the pod manager interface, separates various use cases, and then enables status manager to consult pod worker as truth for pods rather than pod manager.

TODO:

  • Need e2e test demonstrating the problem

/kind bug

@k8s-ci-robot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. 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/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/kubelet area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/node Categorizes an issue or PR as relevant to SIG Node. 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 Jan 26, 2023
@k8s-ci-robot k8s-ci-robot requested review from caesarxuchao, cheftako and a team January 26, 2023 18:35
type mutablePodManager interface {
AddPod(*v1.Pod)
UpdatePod(*v1.Pod)
DeletePod(*v1.Pod)
Copy link
Member

Choose a reason for hiding this comment

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

Other areas of the Kubelet use RemovePod terminology... Probably makes sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see 7 mentions of RemovePod, and slightly more for pod manager's DeletePod. Kubelet config loop uses DeletePods to mean a "pod has been requested for deletion", and "RemovePods" to mean "pod is gone".

Agree Remove should be used, I'll add a new commit.

@smarterclayton
Copy link
Contributor Author

@smarterclayton
Copy link
Contributor Author

/retest

@cici37
Copy link
Contributor

cici37 commented Jan 31, 2023

/triage accepted

bertinatto pushed a commit to bertinatto/kubernetes that referenced this pull request Jul 19, 2023
We can drop this patch after the following two PRs merge (or their
equivalent):

* kubernetes#115342
* kubernetes#113145

UPSTREAM: <carry>: kubelet: fix readiness probes with pod termination
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants