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

kubelet: ensure static pod UIDs are unique #87461

Merged
merged 1 commit into from Dec 1, 2020
Merged

kubelet: ensure static pod UIDs are unique #87461

merged 1 commit into from Dec 1, 2020

Conversation

bboreham
Copy link
Contributor

@bboreham bboreham commented Jan 22, 2020 •
edited by liggitt

What this PR does / why we need it:
Fix the issue where pods like kube-proxy, created from the same static manifest across a cluster, all have the same UID. This breaks the concept of a unique ID and confuses some tools.

The underlying issue is a simple clash of behaviours: kubelet uses a md5 hasher which was changed to reset the hash in DeepHashObject. The fix is to re-order the calls so that one goes first.
This does have the side-effect that all pod UIDs will change.

Special notes for your reviewer:
This is a re-opening of #43420. It and the similar #57135 were held up and ultimately abandoned over concerns that all pods would get restarted during upgrade.

Since then, several Kubernetes upgrades have restarted all pods, and there is a statement at #84443 that this is expected behaviour, so this should not be a reason to delay the fix.

The calculation of pod UIDs for static pods has changed to ensure each static pod gets a unique value - this will cause all static pod containers to be recreated/restarted if an in-place kubelet upgrade from 1.20 to 1.21 is performed. Note that draining pods before upgrading the kubelet across minor versions is the supported upgrade path.

/kind bug
/sig node

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. sig/node Categorizes an issue or PR as relevant to SIG Node. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 22, 2020
@bboreham bboreham mentioned this pull request Jan 22, 2020
Closed
@bboreham
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

@bboreham
Copy link
Contributor Author

/retest

Failed tests kinda look like storage mount errors so let's see what happens if I roll the dice again.

mattjmcnaughton
mattjmcnaughton reviewed Jan 24, 2020
View reviewed changes
Copy link
Contributor

@mattjmcnaughton mattjmcnaughton left a comment

Choose a reason for hiding this comment

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

Thanks for your pr :) Think this bug has a long history :) Trying to tag the folks who have been most involved in the conversation.

cc @tallclair @yujuhong @Random-Liu @dchen1107

@roffe
Copy link

roffe commented Feb 16, 2020

Another stale copy of this issue?

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 16, 2020
@bboreham
Copy link
Contributor Author

Any advice on how to move this forward?
Is there a meeting I should attend?

@bboreham
Copy link
Contributor Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 16, 2020
@roffe
Copy link

roffe commented Jun 18, 2020

can't we just merge it and be done with it?

@roffe
Copy link

roffe commented Jul 1, 2020

this is slowly turning into comedy gold

@dims
Copy link
Member

dims commented Jul 1, 2020

/assign @sjenning @dashpole
/test all

@dashpole
Copy link
Contributor

dashpole commented Jul 6, 2020

Change the calculation of pod UIDs so that static pods get a unique value - will cause all containers to be killed and recreated after in-place upgrade.

Does it cause all pods to be restarted, or just static pods?

@nammn nammn mentioned this pull request Jul 7, 2020
Merged
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 4, 2020
@bboreham
Copy link
Contributor Author

bboreham commented Oct 5, 2020 •
edited

Does it cause all pods to be restarted, or just static pods?

Sorry I missed your question. I believe it's all pods, because the line that moves resets the hasher, so all source information is missing from the pod hash currently, and when we add that in the hash will change.

/remove-lifecycle stale

@bboreham
Copy link
Contributor Author

bboreham commented Oct 5, 2020

/retest
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2020
@dashpole
Copy link
Contributor

dashpole commented Nov 4, 2020

/lgtm

@derekwaynecarr
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bboreham, derekwaynecarr, dims

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@bboreham
kubelet: ensure pod UIDs are unique
19de431 
Ensure node name and file path are included in the hash which produces
the pod UID, otherwise all pods created from the same manifest have
the same UID across the cluster.

The real author of this code is Yu-Ju Hong <yjhong@google.com>.
I am resurrecting an abandoned PR, and changed the git author to pass
CLA check.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2020
@bboreham
Copy link
Contributor Author

Rebased on latest main branch

@bboreham
Copy link
Contributor Author

/retest

@bboreham
Copy link
Contributor Author

@dashpole @derekwaynecarr could you please approve again after rebase. Tests passed on last re-run.

@dashpole
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2020
@derekwaynecarr
Copy link
Member

impacts deployments that run control plane as static pods.

/milestone v1.20

@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Dec 1, 2020
@k8s-ci-robot k8s-ci-robot merged commit 61dc69a into kubernetes:master Dec 1, 2020
@rphillips rphillips mentioned this pull request Dec 1, 2020
Merged
@bboreham bboreham mentioned this pull request Dec 3, 2020
Closed
@jeremyrickard
Copy link
Contributor

impacts deployments that run control plane as static pods.

/milestone v1.20

Hey there, just wanted to mention that this was after test freeze and the branch cut. If you’d like this in 1.20, it needs to be cherry picked and the release is tomorrow so this probably won’t make it until 1.20.1

@jberkus
Copy link

jberkus commented Dec 8, 2020

Seems like this is going to have to wait until 1.21, then? Given that it forces a clusterwide restart.

Also, we can't be sure that no users are (ab)using this bug and treating it as a feature.

@jeremyrickard
Copy link
Contributor

/milestone v1.21

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.20, v1.21 Dec 8, 2020
@github-actions github-actions bot mentioned this pull request Dec 8, 2020
Open
@roffe
Copy link

roffe commented Jan 4, 2021

Milestone indeed. one of the oldest known bugs in Kubernetes has finally been fixed 🎉

@MOZGIII MOZGIII mentioned this pull request Jan 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattjmcnaughton mattjmcnaughton mattjmcnaughton left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@dims dims Awaiting requested review from dims

Assignees

@dims dims

@sjenning sjenning

@dashpole dashpole

@derekwaynecarr derekwaynecarr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. 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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

None yet