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

[1.30] Remove depracated pod-infra-container-image kubelet flag #7409

Merged
merged 1 commit into from
May 6, 2024

Conversation

dereknola
Copy link
Contributor

@dereknola dereknola commented May 2, 2023

Proposed Changes

  • Stops passing pause image to kubelet as a flag (deprecated in v1.29.0)

Types of Changes

CLI

Verification

Start k3s with --pause-image=rancher/mirrored-pause:3.5 and check the logs for the kubelet command. Should see no complaints from kubelet.

Testing

Linked Issues

#7408

User-Facing Change


Further Comments

@dereknola dereknola requested a review from a team as a code owner May 2, 2023 19:21
@brandond
Copy link
Contributor

brandond commented May 2, 2023

We should confirm that the containerd CRI implementation actually exposes this and it is used by the kubelet; CRI related warnings from the kubelet are not always in sync with the reality of CRI implementation maturity.

@dereknola dereknola marked this pull request as draft May 2, 2023 21:12
@dereknola
Copy link
Contributor Author

Good call Brad, looking around, it appears that containerd has the proper implementation, but crio and cri-dockerd are not yet fully implemented. Seems this has been pushed off until 1.28 for upstream. I am converting this to a draft until that has been resolved.

crio: cri-o/cri-o#6816
cri-dockerd: Mirantis/cri-dockerd#178

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.73%. Comparing base (fe7d114) to head (4126aa6).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7409      +/-   ##
==========================================
- Coverage   48.92%   40.73%   -8.19%     
==========================================
  Files         158      158              
  Lines       14032    14030       -2     
==========================================
- Hits         6865     5715    -1150     
- Misses       5885     7272    +1387     
+ Partials     1282     1043     -239     
Flag Coverage Δ
e2etests 35.82% <ø> (-10.97%) ⬇️
inttests 32.18% <ø> (?)
unittests 16.58% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cwayne18
Copy link
Collaborator

Should we check with @Oats87 and co to ensure they're prepared for this

@dereknola dereknola marked this pull request as ready for review December 13, 2023 17:14
@dereknola dereknola changed the title Remove depracated pod-infra-container-image kubelet flag [1.29] Remove depracated pod-infra-container-image kubelet flag Dec 13, 2023
@dereknola dereknola force-pushed the pod-infra-dep branch 3 times, most recently from 0f52e02 to 5a07405 Compare December 20, 2023 03:33
@brandond
Copy link
Contributor

You mentioned earlier that cri-dockerd didn't support this yet, do we need to bump the embedded cri-dockerd version as part of this?

@dereknola
Copy link
Contributor Author

Good catch, it seems that they still have not implemented this and that full removal has moved back to 1.30
CRI-DOCKERD issue: Mirantis/cri-dockerd#39
K8s Deprecation Update (mentioning 1.30 now): kubernetes/kubernetes@d5690f1

@dereknola dereknola changed the title [1.29] Remove depracated pod-infra-container-image kubelet flag [1.30] Remove depracated pod-infra-container-image kubelet flag Dec 20, 2023
@afbjorklund
Copy link

afbjorklund commented Dec 21, 2023

There is a PR, if you want to patch your fork. Main issue is that cri-dockerd is still using the 1.22 API

@brandond
Copy link
Contributor

@afbjorklund we might take a look at pulling that in. Our fork already upgrades the CRI API and stubs out the metrics calls - although we do it by just returning an Unimplemented error, instead of returning empty/zero metrics like the current upstream PR appears to do.

@afbjorklund
Copy link

afbjorklund commented Dec 22, 2023

Probably a better idea, it's been a few months now since that initial hack

@dereknola
Copy link
Contributor Author

cri-dockerd has support as of v0.3.10, so 1.30 will see this merged.

Signed-off-by: Derek Nola <derek.nola@suse.com>
Copy link

@VestigeJ VestigeJ left a comment

Choose a reason for hiding this comment

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

LGTM

@dereknola dereknola merged commit 6531fb7 into k3s-io:master May 6, 2024
27 checks passed
@dereknola dereknola deleted the pod-infra-dep branch July 9, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants