cleanup: Remove deprecated containerGC flags from kubelet. #129391

zhifei92 · Dec 25, 2024

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Remove deprecated containerGC flags from kubelet, to encourage the use of the eviction mechanism instead.

Which issue(s) this PR fixes:

Fixes #127157

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Removed several deprecated kubelet command line arguments for container garbage collection: `--minimum-container-ttl-duration`, `--maximum-dead-containers`, and `--maximum-dead-containers-per-container`. These arguments were previously deprecated and are now fully removed. Users who utilized these arguments should update their configurations before upgrading.

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

k8s-ci-robot · Dec 25, 2024

Hi @zhifei92. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

bart0sh · Dec 27, 2024

pkg/kubelet/kubelet.go

-		MaxContainers:      int(maxContainerCount),
+		MinAge:             time.Duration(0),
+		MaxPerPodContainer: 1,
+		MaxContainers:      -1,


Is this policy used somewhere outside of GC? If not, should we remove it?

Is this policy used somewhere outside of GC? If not, should we remove it?

The policy is not used outside of GC, but removing it would affect the current behavior of GC.

Initializing directly within NewContainerGC might be more reasonable.

As all fields are set to static values, we may want to refactor GC code or even get rid of (some of) it if possible. For example MinAge 0 is asking for refactoring of func (cgc *containerGC) evictableContainers, with MaxContainer -1 (cgc *containerGC) evictContainers could be simplified, etc .

agree with this

bart0sh · Dec 27, 2024

/triage accepted
/priority important-longterm
/ok-to-test

sftim · Dec 27, 2024

Changelog tweak suggestion

-Removed deprecated kubelet containerGC flags: `--minimum-container-ttl-duration`, `--maximum-dead-containers`, and `--maximum-dead-containers-per-container`. These options were previously deprecated and are now fully removed to enhance kubelet performance. Users who utilized these flags should update their configurations before upgrading.
+Removed several deprecated kubelet command line arguments for container garbage collection: `--minimum-container-ttl-duration`, `--maximum-dead-containers`, and `--maximum-dead-containers-per-container`. These arguments were previously deprecated and are now fully removed. Users who utilized these arguments should update their configurations before upgrading.

k8s-ci-robot · Jan 7, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhifei92
Once this PR has been reviewed and has the lgtm label, please assign marseel, yujuhong for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

Shubham82 · Feb 13, 2025

@zhifei92, Is there any milestone(v1.33 or later release) we are targeting for this PR to merge?

Shubham82 · Feb 17, 2025

Hi @derekwaynecarr @matthyx @bart0sh
Could you please take a look, so that this PR will merge?

Thanks!

matthyx · Feb 18, 2025

/lgtm

k8s-ci-robot · Feb 18, 2025

LGTM label has been added.

Git tree hash: 9463749ef2c35f86d7bbbadd02e4e37d17249b3c

Shubham82 · Feb 27, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhifei92 Once this PR has been reviewed and has the lgtm label, please assign marseel, yujuhong for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

cmd/kubelet/OWNERS

pkg/kubelet/OWNERS

pkg/kubemark/OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

Hi @matthyx, as mentioned above, this PR needs approval from the sig-node approver. Could you please take a look?

matthyx · Feb 27, 2025

@SergeyKanzhelev PTAL

Shubham82 · Mar 5, 2025

/test pull-kubernetes-cmd

SergeyKanzhelev · Mar 6, 2025

cmd/kubelet/app/options/options.go

-	fs.MarkDeprecated("minimum-container-ttl-duration", "Use --eviction-hard or --eviction-soft instead. Will be removed in a future version.")
-	fs.Int32Var(&f.MaxPerPodContainerCount, "maximum-dead-containers-per-container", f.MaxPerPodContainerCount, "Maximum number of old instances to retain per container.  Each container takes up some disk space.")
-	fs.MarkDeprecated("maximum-dead-containers-per-container", "Use --eviction-hard or --eviction-soft instead. Will be removed in a future version.")
-	fs.Int32Var(&f.MaxContainerCount, "maximum-dead-containers", f.MaxContainerCount, "Maximum number of old instances of containers to retain globally.  Each container takes up some disk space. To disable, set to a negative number.")
 	fs.MarkDeprecated("maximum-dead-containers", "Use --eviction-hard or --eviction-soft instead. Will be removed in a future version.")


why keep this marking as deprecated?

Missed this one; it has been deleted.

SergeyKanzhelev · Mar 6, 2025

Fixes #127157

Before accepting this PR, can you please write a small document on how to migrate from using these flags to the eviction? How much functionality is being lost with this removal?

Perhaps it can be an extension of this update: kubernetes/website#48001

fix: unit test UnknownStateContainerGC

k8s-ci-robot · Mar 7, 2025

New changes are detected. LGTM label has been removed.

zhifei92 · Mar 7, 2025

/test pull-kubernetes-e2e-gce

ffromani · Mar 7, 2025

Fixes #127157

Before accepting this PR, can you please write a small document on how to migrate from using these flags to the eviction? How much functionality is being lost with this removal?

Perhaps it can be an extension of this update: kubernetes/website#48001

was this addressed yet?

zhifei92 · Mar 7, 2025

Fixes #127157

Before accepting this PR, can you please write a small document on how to migrate from using these flags to the eviction? How much functionality is being lost with this removal?

Perhaps it can be an extension of this update: kubernetes/website#48001

There is already a part of the explanation in kubernetes/website#48001. Should I open a new PR to cover the content?

Shubham82 · Mar 10, 2025

There is already a part of the explanation in kubernetes/website#48001. Should I open a new PR to cover the content?

@zhifei92, as there has been no update on the PR for a long time, it would be good if you open a new docs PR for it so that this PR will be merged soon.

zhifei92 · Mar 14, 2025

The Eviction Manager does indeed use ContainerGC for managing container resources. As for how to achieve similar effects through Eviction Manager, I haven't figured that out yet and need more time to think about it. Currently, ContainerGC is effective in two places at the same time, which is indeed unnecessary.

SergeyKanzhelev · Mar 19, 2025

The Eviction Manager does indeed use ContainerGC for managing container resources. As for how to achieve similar effects through Eviction Manager, I haven't figured that out yet and need more time to think about it. Currently, ContainerGC is effective in two places at the same time, which is indeed unnecessary.

Since we do not know whether this functionality is still being used, some transitioning instructions are needed before removing those options. The PR: kubernetes/website#48001 doesn't contain those details

Shubham82 · Mar 24, 2025

JFI: As it is mentioned here we can use --eviction-hard or --eviction-soft instead of these deprecated flags.

cc @derekwaynecarr (as the author of Kubelet - Eviction Policy) could please take a look?

k8s-ci-robot requested review from derekwaynecarr and matthyx December 25, 2024 07:16

bart0sh reviewed Dec 27, 2024

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 6, 2025

zhifei92 force-pushed the remove-deprecated-flags branch from 091c32d to 441836e Compare January 7, 2025 13:04

k8s-ci-robot assigned matthyx Feb 18, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2025

SergeyKanzhelev reviewed Mar 6, 2025

View reviewed changes

zhifei92 added 5 commits March 7, 2025 10:30

chore: Remove deprecated containerGC flags from kubelet.

acbc993

Remove container gc policy.

110dda1

Fix unit test.

da1b7f8

fix: unit test UnknownStateContainerGC

chore: fix comments.

ac49d4f

Remove the deprecated maximum-dead-containers flag.

383cb7b

zhifei92 force-pushed the remove-deprecated-flags branch from d002961 to 383cb7b Compare March 7, 2025 02:42

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2025

SergeyKanzhelev moved this from PRs - Needs Approver to PRs Waiting on Author in SIG Node CI/Test Board Mar 19, 2025

esotsal mentioned this pull request Mar 27, 2025

Updated container garbage collection with accurate information kubernetes/website#48001

Open

haircommander moved this from PRs Waiting on Author to PRs - Needs Reviewer in SIG Node CI/Test Board Apr 30, 2025

SergeyKanzhelev moved this from Needs Approver to Needs Reviewer in SIG Node: code and documentation PRs May 31, 2025

Search code, repositories, users, issues, pull requests...

cleanup: Remove deprecated containerGC flags from kubelet. #129391

Are you sure you want to change the base?

cleanup: Remove deprecated containerGC flags from kubelet. #129391

Uh oh!

Conversation

zhifei92 commented Dec 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Uh oh!

k8s-ci-robot commented Dec 25, 2024

Uh oh!

bart0sh Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

zhifei92 Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

zhifei92 Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

bart0sh Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

zhifei92 Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

bart0sh commented Dec 27, 2024

Uh oh!

sftim commented Dec 27, 2024

Uh oh!

k8s-ci-robot commented Jan 7, 2025

Uh oh!

Shubham82 commented Feb 13, 2025

Uh oh!

Shubham82 commented Feb 17, 2025

Uh oh!

matthyx commented Feb 18, 2025

Uh oh!

k8s-ci-robot commented Feb 18, 2025

Uh oh!

Shubham82 commented Feb 27, 2025

Uh oh!

matthyx commented Feb 27, 2025

Uh oh!

Shubham82 commented Mar 5, 2025

Uh oh!

SergeyKanzhelev Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

zhifei92 Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 7, 2025

Uh oh!

zhifei92 commented Mar 7, 2025

Uh oh!

ffromani commented Mar 7, 2025

Uh oh!

zhifei92 commented Mar 7, 2025

Uh oh!

Shubham82 commented Mar 10, 2025

Uh oh!

zhifei92 commented Mar 14, 2025

Uh oh!

SergeyKanzhelev commented Mar 19, 2025

Uh oh!

Shubham82 commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

zhifei92 commented Dec 25, 2024 •

edited

Loading

SergeyKanzhelev commented Mar 6, 2025 •

edited

Loading

Shubham82 commented Mar 24, 2025 •

edited

Loading