KEP-4210: Add ImageGCMaximumAge KEP #4211

haircommander · 2023-09-14T20:56:46Z

One-line PR description:
Add Kubelet option to specify the maximum age an image will be kept around before it is garbage collected

Issue link: kubelet image GC after a maximum age #4210

Other comments:

BenTheElder

What about the current system critical images?
Won't images like kube-proxy etc wind up getting marked eventually? Do we have a way to exclude these yet other than the sandbox image?

pacoxu · 2023-09-15T09:58:44Z

What about the current system critical images?
Won't images like kube-proxy etc wind up getting marked eventually? Do we have a way to exclude these yet other than the sandbox image?

We may pin images with cluster-critical and node-crictil priority like kubernetes/kubernetes#118544 the sandbox image.

keps/sig-node/4210-max-image-gc-age/README.md

kannon92 · 2023-09-15T22:05:42Z

I was curious. Since this is mostly adding a new policy to ImageGC and it should be completely opt-in. Would there be possibility in skipping the alpha phase?

I remember that there are some examples of KEPs that go directly into beta phase. Its mostly a thought for @haircommander and sig-node about the complexity of this KEP once it is finished.

haircommander · 2023-09-18T15:33:26Z

What about the current system critical images?
Won't images like kube-proxy etc wind up getting marked eventually? Do we have a way to exclude these yet other than the sandbox image?

I don't think so. Here I define used as "no container running with that image in the specified time". The idea is to define a default so that any "critical" images would be used more frequently than the default. Besides, in the bad case here the image will be made available by an image pull

We may pin images with cluster-critical and node-crictil priority like kubernetes/kubernetes#118544 the sandbox image.

This is up to the CRI to do, though both CRI implementations have support for pinning images AFAIU so if users wanted this feature but were anxious about a very big image or something they could pin it

keps/sig-node/4210-max-image-gc-age/README.md

kannon92 · 2023-09-21T18:57:41Z

keps/sig-node/4210-max-image-gc-age/README.md

+	ImageMaximumGCAge metav1.Duration
+```
+
+To begin, this option will be set to the maximum duration (math.MaxInt64), which effectively disables.


Wouldn't it make more sense that if this isn't explicitly set, then just don't run this policy?

That seems to be more aligned with the API way of adding new fields?

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#new-field-in-existing-api-version

I don't believe there is an unset for it, as metav1.Duration just wraps and marshals a time.Duration, which is just an int64--no pointers involved. Other fields have +optional on, but they default to 0. We could default to 0 in this case, interpreting it as "off" but that would prevent users from setting ImageMaximumGCAge to zero: meaning GC immediately

I've decided to set it to 0 by default and add +optional, along with adding a note that says 0 will disable the field

SGTM. API review might point out that all new fields should be pointers but I am not sure if that is followed for configurations. THis would be the case for new API fields.

keps/sig-node/4210-max-image-gc-age/README.md

keps/sig-node/4210-max-image-gc-age/kep.yaml

SergeyKanzhelev · 2023-10-03T20:58:22Z

keps/sig-node/4210-max-image-gc-age/README.md

+#### Beta
+
+- Gather feedback from users
+- Depending on PRR review and SIG-Node opt-in, this feature could start as a disabled Beta.


it is not a good practice. Let's not do it

mrunalp · 2023-10-03T21:09:09Z

keps/sig-node/4210-max-image-gc-age/README.md

+
+Currently, all image garbage collection the Kubelet is triggered by disk usage going over a threshold (ImageGCLowThresholdPercent).
+However, there are cases that additional conditions could be considered useful. One such condition is maximum age of an image.
+If an image is unused for a long time (the exact amount of time will be decided, but on the order of weeks is what comes to mind),


Is that time for the default?

haircommander · 2023-10-04T15:44:16Z

updated based on feedback--I added some sections to Alternatives and to the Proposal

soltysh

A few nits, the main concern is lack of a feature gate as explained in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/feature-gates.md

keps/sig-node/4210-max-image-gc-age/README.md

soltysh · 2023-10-04T16:30:14Z

keps/sig-node/4210-max-image-gc-age/README.md

+
+###### How can this feature be enabled / disabled in a live cluster?
+
+- [ ] Feature gate (also fill in values in `kep.yaml`)


afaict we still require a feature gate, even for optional fields like this one.

haircommander · 2023-10-04T19:23:27Z

thanks @soltysh ! updated

johnbelamaric

Minor comment but I am ok with this for PRR

keps/sig-node/4210-max-image-gc-age/README.md

SergeyKanzhelev · 2023-10-04T23:49:51Z

keps/sig-node/4210-max-image-gc-age/README.md

+    - Good testing will mitigate/fix any errors
+- New, undiscovered races
+    - If the max image gc age is set very low, will the kubelet race with itself and remove the image right after pulling it?
+    - May need to define a minimum maximum gc age to prevent races like this.


I think this is important question to answer on design stage. We can explicitly say that current iteration will set the high threshold.

what do you mean by the high threshold @SergeyKanzhelev ?

sorry. Let's set the minimul available as 30 minuutes in alpha. And say we can allow less in future

keps/sig-node/4210-max-image-gc-age/README.md

Signed-off-by: Peter Hunt <[email protected]>

kannon92 · 2023-10-05T16:08:19Z

/lgtm

SergeyKanzhelev

/lgtm

there is still a small comment on the behavior of sandbox image that wasn't marked as Pinned. But it feels to be something we may discover and protect from in beta.

If you can put it explicitly as something to protect from - it will be great.

johnbelamaric · 2023-10-05T18:33:26Z

/approve

k8s-ci-robot · 2023-10-05T18:33:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, johnbelamaric, mrunalp, SergeyKanzhelev

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:

~~keps/prod-readiness/OWNERS~~ [johnbelamaric]
~~keps/sig-node/OWNERS~~ [johnbelamaric,mrunalp]

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

haircommander · 2023-10-05T18:36:06Z

there is still a small comment on the behavior of sandbox image that wasn't marked as Pinned. But it feels to be something we may discover and protect from in beta.

I would be shocked if the pause image managed to be unused for long enough to qualify TBH

SergeyKanzhelev · 2023-10-05T18:41:51Z

there is still a small comment on the behavior of sandbox image that wasn't marked as Pinned. But it feels to be something we may discover and protect from in beta.

I would be shocked if the pause image managed to be unused for long enough to qualify TBH

How will kubelet know it is used? Kubelet only knows about the user containers, not the sandbox?

haircommander · 2023-10-05T18:43:43Z

there is still a small comment on the behavior of sandbox image that wasn't marked as Pinned. But it feels to be something we may discover and protect from in beta.

I would be shocked if the pause image managed to be unused for long enough to qualify TBH

How will kubelet know it is used? Kubelet only knows about the user containers, not the sandbox?

ah true good point. yeah I will investigate that and double check it isn't qualified.

k8s-ci-robot requested review from derekwaynecarr and mrunalp September 14, 2023 20:57

haircommander marked this pull request as draft September 14, 2023 21:00

haircommander changed the title ~~WIP KEP-4210: Begin scaffolding ImageMaxGCAge KEP~~ KEP-4210: Add ImageGCMaximumAge KEP Sep 14, 2023

BenTheElder reviewed Sep 15, 2023

View reviewed changes

harshanarayana reviewed Sep 15, 2023

View reviewed changes

keps/sig-node/4210-max-image-gc-age/README.md Show resolved Hide resolved

keps/sig-node/4210-max-image-gc-age/README.md Show resolved Hide resolved

haircommander force-pushed the max-image-gc branch from 8c99be6 to f10c442 Compare September 18, 2023 20:13

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 18, 2023

bsdnet reviewed Sep 19, 2023

View reviewed changes

keps/sig-node/4210-max-image-gc-age/README.md Show resolved Hide resolved

keps/sig-node/4210-max-image-gc-age/README.md Show resolved Hide resolved

keps/sig-node/4210-max-image-gc-age/README.md Show resolved Hide resolved

haircommander marked this pull request as ready for review September 19, 2023 15:18

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2023

haircommander force-pushed the max-image-gc branch 2 times, most recently from 57dffa7 to 9dacaf0 Compare September 19, 2023 15:44

kannon92 reviewed Sep 21, 2023

View reviewed changes

haircommander force-pushed the max-image-gc branch from 9dacaf0 to 074a238 Compare September 26, 2023 20:17

kannon92 reviewed Sep 26, 2023

View reviewed changes

keps/sig-node/4210-max-image-gc-age/README.md Outdated Show resolved Hide resolved

haircommander force-pushed the max-image-gc branch from 074a238 to 9b82d66 Compare September 27, 2023 14:38

kannon92 reviewed Sep 27, 2023

View reviewed changes

keps/sig-node/4210-max-image-gc-age/kep.yaml Show resolved Hide resolved

kannon92 reviewed Sep 27, 2023

View reviewed changes

keps/sig-node/4210-max-image-gc-age/kep.yaml Show resolved Hide resolved

SergeyKanzhelev reviewed Oct 3, 2023

View reviewed changes

mrunalp reviewed Oct 3, 2023

View reviewed changes

haircommander force-pushed the max-image-gc branch from 0cb2ca0 to 7fa9936 Compare October 4, 2023 15:43

soltysh reviewed Oct 4, 2023

View reviewed changes

haircommander force-pushed the max-image-gc branch from 5a94c5a to cee9750 Compare October 4, 2023 19:23

haircommander force-pushed the max-image-gc branch from cee9750 to 7dd76a1 Compare October 4, 2023 19:29

johnbelamaric reviewed Oct 4, 2023

View reviewed changes

keps/sig-node/4210-max-image-gc-age/README.md Outdated Show resolved Hide resolved

SergeyKanzhelev reviewed Oct 4, 2023

View reviewed changes

keps/sig-node/4210-max-image-gc-age/README.md Show resolved Hide resolved

haircommander added 2 commits October 5, 2023 11:37

KEP-4210: Fill in KEP for ImageGCMaximumAge

78f1bf8

Signed-off-by: Peter Hunt <[email protected]>

KEP 4210: add a feature gate

9cabce4

Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the max-image-gc branch from 7dd76a1 to 9cabce4 Compare October 5, 2023 15:38

k8s-ci-robot assigned kannon92 Oct 5, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2023

SergeyKanzhelev approved these changes Oct 5, 2023

View reviewed changes

k8s-ci-robot assigned SergeyKanzhelev Oct 5, 2023

mrunalp approved these changes Oct 5, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2023

k8s-ci-robot merged commit a1fa456 into kubernetes:master Oct 5, 2023

k8s-ci-robot added this to the v1.29 milestone Oct 5, 2023

soltysh mentioned this pull request Apr 3, 2024

Add soltysh to prod-readiness-approvers #4566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-4210: Add ImageGCMaximumAge KEP #4211

KEP-4210: Add ImageGCMaximumAge KEP #4211

haircommander commented Sep 14, 2023

BenTheElder left a comment

pacoxu commented Sep 15, 2023

kannon92 commented Sep 15, 2023

haircommander commented Sep 18, 2023

kannon92 Sep 21, 2023

kannon92 Sep 21, 2023

haircommander Sep 26, 2023

haircommander Sep 26, 2023

kannon92 Sep 26, 2023

SergeyKanzhelev Oct 3, 2023

mrunalp Oct 3, 2023

haircommander Oct 3, 2023

haircommander commented Oct 4, 2023

soltysh left a comment

soltysh Oct 4, 2023

haircommander commented Oct 4, 2023

johnbelamaric left a comment

SergeyKanzhelev Oct 4, 2023

haircommander Oct 5, 2023

SergeyKanzhelev Oct 5, 2023

kannon92 commented Oct 5, 2023

SergeyKanzhelev left a comment

johnbelamaric commented Oct 5, 2023

k8s-ci-robot commented Oct 5, 2023

haircommander commented Oct 5, 2023

SergeyKanzhelev commented Oct 5, 2023

haircommander commented Oct 5, 2023


		###### How can this feature be enabled / disabled in a live cluster?

		- [ ] Feature gate (also fill in values in `kep.yaml`)

KEP-4210: Add ImageGCMaximumAge KEP #4211

KEP-4210: Add ImageGCMaximumAge KEP #4211

Conversation

haircommander commented Sep 14, 2023

BenTheElder left a comment

Choose a reason for hiding this comment

pacoxu commented Sep 15, 2023

kannon92 commented Sep 15, 2023

haircommander commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented Oct 4, 2023

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented Oct 4, 2023

johnbelamaric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannon92 commented Oct 5, 2023

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

johnbelamaric commented Oct 5, 2023

k8s-ci-robot commented Oct 5, 2023

haircommander commented Oct 5, 2023

SergeyKanzhelev commented Oct 5, 2023

haircommander commented Oct 5, 2023