kubelet/image: improve error handling on noImageToCleanup #117981

haitch · 2023-05-12T18:29:02Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

basically move 2 condition checks ahead, to scanning phase, instead of delete phase.

LastUsed condition check
FirstDetected condition check

then we can elegantly report no image to cleanup, in stead of failed to garbage collect required amount of images. Wanted to free 473842483 bytes, but freed 0 bytes

after the change, you either get no image to cleanup or image cleanup error.

Which issue(s) this PR fixes:

fixes #71869

Special notes for your reviewer:

.freeSpace is internal method, update to return error on NoImageToCleanup
only 2 caller on this method
- GarbageCollect
- DeleteUnusedImages
both caller been update to aware of this error pattern

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2023-05-12T18:29:11Z

Welcome @haitch!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-05-12T18:29:12Z

Hi @haitch. 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/test-infra repository.

helayoty · 2023-05-12T20:49:20Z

/ok-to-test

bart0sh · 2023-05-15T12:00:00Z

/triage accepted
/priority important-longterm
/cc

haitch · 2023-05-19T20:49:14Z

@Random-Liu @dchen1107 @derekwaynecarr @yujuhong @sjenning @mrunalp @klueska

feiskyer

Thanks for the fix! The changes look good.

/lgtm

k8s-ci-robot · 2023-06-01T05:06:56Z

LGTM label has been added.

Git tree hash: a791702cae8b85aee38a386ebac494dbb6a3b417

bart0sh · 2023-06-01T09:22:30Z

/assign @mrunalp @derekwaynecarr @dchen1107

mrunalp · 2023-10-26T00:22:58Z

@haircommander ptal

haircommander · 2023-10-26T13:42:25Z

pkg/kubelet/images/image_gc_manager.go

@@ -39,6 +39,10 @@ import (
 	"k8s.io/kubernetes/pkg/kubelet/util/sliceutils"
 )

+var (
+	errNoImageToCleanup = goerrors.New("NoImageToCleanup")


I'm not seeing a reason to have this look like an event type. Maybe it would be better as "no images to cleanup" and then the error condition below would read more like a sentence?

haircommander · 2023-10-26T13:42:39Z

pkg/kubelet/images/image_gc_manager.go

@@ -323,6 +327,11 @@ func (im *realImageGCManager) GarbageCollect(ctx context.Context) error {
 		klog.InfoS("Disk usage on image filesystem is over the high threshold, trying to free bytes down to the low threshold", "usage", usagePercent, "highThreshold", im.policy.HighThresholdPercent, "amountToFree", amountToFree, "lowThreshold", im.policy.LowThresholdPercent)
 		freed, err := im.freeSpace(ctx, amountToFree, time.Now())
 		if err != nil {
+			if goerrors.Is(err, errNoImageToCleanup) {
+				klog.InfoS("No images eligible for garbage collection")
+				im.recorder.Eventf(im.nodeRef, v1.EventTypeWarning, events.FreeDiskSpaceFailed, err.Error())


why only an event here?

k8s-ci-robot · 2023-10-26T16:38:27Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2023-10-26T16:38:58Z

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found 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/test-infra repository.

k8s-ci-robot · 2023-10-26T16:39:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: feiskyer, haitch
Once this PR has been reviewed and has the lgtm label, please ask for approval from dchen1107. For more information see the Kubernetes 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:

pkg/kubelet/OWNERS

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 · 2023-10-26T16:42:21Z

@haitch: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-unit	`3957432`	link	true	`/test pull-kubernetes-unit`
pull-kubernetes-integration	`3957432`	link	true	`/test pull-kubernetes-integration`
pull-kubernetes-dependencies	`3957432`	link	true	`/test pull-kubernetes-dependencies`
pull-kubernetes-conformance-kind-ga-only-parallel	`3957432`	link	true	`/test pull-kubernetes-conformance-kind-ga-only-parallel`
pull-kubernetes-e2e-gce	`3957432`	link	true	`/test pull-kubernetes-e2e-gce`
pull-kubernetes-verify	`3957432`	link	true	`/test pull-kubernetes-verify`
pull-kubernetes-typecheck	`3957432`	link	true	`/test pull-kubernetes-typecheck`
pull-kubernetes-node-e2e-containerd	`3957432`	link	true	`/test pull-kubernetes-node-e2e-containerd`
pull-kubernetes-e2e-kind	`3957432`	link	true	`/test pull-kubernetes-e2e-kind`
pull-kubernetes-linter-hints	`3957432`	link	false	`/test pull-kubernetes-linter-hints`
pull-kubernetes-e2e-kind-ipv6	`3957432`	link	true	`/test pull-kubernetes-e2e-kind-ipv6`
pull-kubernetes-verify-lint	`3957432`	link	false	`/test pull-kubernetes-verify-lint`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2023-10-28T04:23:10Z

PR needs rebase.

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/test-infra repository.

improve error handling on noImageToCleanup

b7d022a

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 12, 2023

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label May 12, 2023

k8s-ci-robot requested review from feiskyer and Random-Liu May 12, 2023 18:30

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 12, 2023

fix unittests

28de336

haitch changed the title ~~[WIP] kubelet/image: improve error handling on noImageToCleanup~~ kubelet/image: improve error handling on noImageToCleanup May 12, 2023

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 May 12, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 12, 2023

kubernetes deleted a comment from haitch May 12, 2023

kubernetes deleted a comment from k8s-ci-robot May 12, 2023

bart0sh added this to Triage in SIG Node PR Triage May 14, 2023

k8s-ci-robot requested a review from bart0sh May 15, 2023 12:00

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels May 15, 2023

feiskyer approved these changes Jun 1, 2023

View reviewed changes

k8s-ci-robot assigned feiskyer Jun 1, 2023

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

k8s-ci-robot assigned dchen1107, derekwaynecarr and mrunalp Jun 1, 2023

bart0sh moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Jun 1, 2023

haircommander reviewed Oct 26, 2023

View reviewed changes

haitch added 2 commits October 26, 2023 16:35

Merge branch 'master' into haitao/image_gc_no_target

38fb525

tweaks on error message

3957432

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

k8s-ci-robot requested review from dchen1107, derekwaynecarr, feiskyer and mrunalp October 26, 2023 16:38

k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Oct 26, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2023

dims added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 4, 2024

haitch closed this Jan 30, 2024

SIG Node PR Triage automation moved this from Needs Approver to Done Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet/image: improve error handling on noImageToCleanup #117981

kubelet/image: improve error handling on noImageToCleanup #117981

haitch commented May 12, 2023 •

edited

k8s-ci-robot commented May 12, 2023

k8s-ci-robot commented May 12, 2023

helayoty commented May 12, 2023

bart0sh commented May 15, 2023

haitch commented May 19, 2023

feiskyer left a comment

k8s-ci-robot commented Jun 1, 2023

bart0sh commented Jun 1, 2023

mrunalp commented Oct 26, 2023

haircommander Oct 26, 2023

haircommander Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

k8s-ci-robot commented Oct 28, 2023

kubelet/image: improve error handling on noImageToCleanup #117981

kubelet/image: improve error handling on noImageToCleanup #117981

Conversation

haitch commented May 12, 2023 • edited

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.:

k8s-ci-robot commented May 12, 2023

k8s-ci-robot commented May 12, 2023

helayoty commented May 12, 2023

bart0sh commented May 15, 2023

haitch commented May 19, 2023

feiskyer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 1, 2023

bart0sh commented Jun 1, 2023

mrunalp commented Oct 26, 2023

haircommander Oct 26, 2023

Choose a reason for hiding this comment

haircommander Oct 26, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

k8s-ci-robot commented Oct 28, 2023

haitch commented May 12, 2023 •

edited