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
prevents garbage collection from removing pinned images #103299
Conversation
Is there a KEP or issue tracking this? |
b51f03b
to
9c683d1
Compare
this was combined with CRI beta kep: kubernetes/enhancements#2694 |
/retest |
/priority important-soon |
@@ -193,6 +193,25 @@ func TestDeleteUnusedImagesExemptSandboxImage(t *testing.T) { | |||
require.NoError(t, err) | |||
} | |||
|
|||
func TestDeletePinnedImage(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend adding cases for:
- List with no pinned images
- List with only pinned images
- List with mixed images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added!
For the CRI implementation to be able to indicate to the Kubelet that an image should not be garbage collected. Signed-off-by: Peter Hunt <pehunt@redhat.com>
@pacoxu sorry this took so long! Ptal! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic lgtm, minor test cleanup comments
@@ -206,6 +206,105 @@ func TestDeleteUnusedImagesExemptSandboxImage(t *testing.T) { | |||
require.NoError(t, err) | |||
} | |||
|
|||
func TestDeletePinnedImage(t *testing.T) { | |||
mockCtrl := gomock.NewController(t) | |||
defer mockCtrl.Finish() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to call Finish
when passing a testing.T into NewController as of go1.14 (https://pkg.go.dev/github.com/golang/mock/gomock#Controller.Finish)
defer mockCtrl.Finish() |
require.NoError(t, err) | ||
assert.EqualValues(0, spaceFreed) | ||
assert.Len(fakeRuntime.ImageList, 2) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Seems good now.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrunalp, wgahnagl 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
prevents garbage collection from removing pinned images
Which issue(s) this PR fixes:
#101808
implements this
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: