Bug 1860163: Optimized Image Pruner #510

dmage · 2020-07-30T15:20:19Z

No description provided.

openshift-ci-robot · 2020-07-30T15:20:25Z

@dmage: This pull request references Bugzilla bug 1860163, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

[WIP] Bug 1860163: Optimized Image Pruner

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.

pkg/cli/admin/prune/imageprune/prune_test.go

pkg/cli/admin/prune/imageprune/prune.go

pkg/cli/admin/prune/imageprune/prune_test.go

hongkailiu · 2020-09-02T17:42:38Z

@dmage

Our image-pruning job started to fail again.
https://prow.ci.openshift.org/job-history/gs/origin-ci-test/logs/periodic-ci-image-pruner

Could you please rebase the code so that we can get an oc-binary for the job before this PR lands?

pkg/cli/admin/prune/images/images.go

dmage · 2020-09-16T09:32:19Z

/retest
/assign @ricardomaraschini @soltysh

soltysh

I haven't reviewed the overall code that carefully, but I'm a bit hesitant to accept a major re-write at this time. How about we push this over to 4.7 and when proved working backport to 4.6?

pkg/cli/admin/prune/images/images.go

dmage · 2020-09-16T11:36:26Z

@soltysh I'm more concerned to have 4.6 with the current pruner. This pruner was already used on CI (one of its WIP versions) because the current one doesn't work on CI, and CI needs a pruner. Ask @stevekuznetsov if he is ok to wait.

soltysh · 2020-09-16T12:45:07Z

I'll add that for tomorrows review, to carefully examine the changes, I'm fairly familiar with the entire pruner code.

pkg/cli/admin/prune/imageprune/helper.go

pkg/cli/admin/prune/imageprune/prune.go

ricardomaraschini · 2020-09-16T11:23:48Z

pkg/cli/admin/prune/imageprune/prune.go

@@ -101,33 +177,31 @@ type ImageDeleter interface {
 type ImageStreamDeleter interface {


I can't help but finding this name unfortunate. On other Deleters we actually have a "Delete" method but not on this one. I guess that it might make sense once I move on with the review, let's see.

this object is intended to delete records from image stream statuses
ImageStreamStatusUpdater?

pkg/cli/admin/prune/imageprune/prune.go

pkg/helpers/image/test/util.go

pkg/cli/admin/prune/images/images.go

soltysh

I left some comments. What I dislike is also the fact that pkg/cli/admin/prune/imageprune/prune.go is such a huge file, have you considered splitting that into smaller, logical chunks?

pkg/cli/admin/prune/images/images.go

soltysh · 2020-09-17T10:51:01Z

pkg/cli/admin/prune/images/images.go

 		return nil
 	}); err != nil {
 		return err
 	}
+	allImagesUntyped = nil // We don't need it anymore


The above code seems overly convoluted, have a look at https://github.com/kubernetes/kubernetes/blob/51184187b9cc8e54bb51dc921bfb80de3d638635/pkg/controller/cronjob/cronjob_controller.go#L135-L143 and please refactor that so that it's more readable.

The defaults in pager.SimplePageFunc are reasonable, I wouldn't tweak them.

cool, I didn't know about EachListItem, fixed.

btw, the example that you gave me also needs to be updated. SimplePageFunc is needed for list functions that don't accept ctx. And the list function uses context.TODO(), but it could use ctx from the pager.

pkg/cli/admin/prune/imageprune/prune.go

soltysh · 2020-09-17T11:19:03Z

pkg/cli/admin/prune/imageprune/prune.go

-// long as the image is managed by OpenShift.
-func (p *pruner) addPodSpecToGraph(referrer *corev1.ObjectReference, spec *corev1.PodSpec, predecessor gonum.Node) []error {
-	var errs []error
+type referenceCounts struct {


Why not making a single struct holding the counters instead of embeding one and then another inside?
Why not having one with higher level methods reponsible for adding blobs and manifests rather than directly modify underlying structures?

to make it easier, stringsCounter is super simple to review

soltysh · 2020-09-17T11:22:04Z

pkg/cli/admin/prune/imageprune/prune.go

+			Name:      rs.Name,
+		}
+		klog.V(4).Infof("Examining %s", ref)
+		errs = append(errs, p.analyzeReferencesFromPodSpec(ref, &rs.Spec.Template.Spec)...)


You should probably also verify k8s object's annotations looking for alpha.image.policy.openshift.io/resolve-names which will be pointing to image streams.

yes, it'd be nice to support this annotation, I created a BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1880068

pkg/cli/admin/prune/imageprune/prune.go

The new implementation of the image pruner does not use graphs. The graph was replaced by two maps. The pruner works in two steps: The first step deletes image stream items (aka revisions) that are not used by the cluster components (pods, builds, etc.) and are not protected by the pruner options (keep-younger-than, keep-tag-revisions, etc.) The second step deletes image objects that are not used by image streams.

dmage · 2020-09-17T16:28:04Z

@soltysh one day I want to split this pruner into two parts. One is for image streams, another one is for for images. Do you want prune.go to be splitted in this PR?

dmage · 2020-09-21T08:41:51Z

/retest

dmage · 2020-09-21T12:14:50Z

@soltysh can you take a look once again?

ricardomaraschini

/lgtm

soltysh · 2020-09-23T18:24:33Z

/approve
The missing bits will be followed through BZ.

openshift-ci-robot · 2020-09-23T18:24:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, ricardomaraschini, soltysh

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:

~~OWNERS~~ [soltysh]

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

openshift-bot · 2020-09-23T21:06:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-23T22:50:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-09-24T02:22:57Z

@dmage: All pull requests linked via external trackers have merged:

openshift/oc#510

Bugzilla bug 1860163 has been moved to the MODIFIED state.

In response to this:

Bug 1860163: Optimized Image Pruner

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.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. labels Jul 30, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jul 30, 2020

openshift-ci-robot requested review from bparees and deads2k July 30, 2020 15:20

dmage force-pushed the image-pruner branch from 6451c3f to 415ba05 Compare July 30, 2020 18:15

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2020

dmage force-pushed the image-pruner branch from 415ba05 to febc99b Compare August 4, 2020 14:11

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2020

dmage force-pushed the image-pruner branch from febc99b to b7245a4 Compare August 4, 2020 16:01

dmage force-pushed the image-pruner branch from b5cd6e1 to 7743763 Compare August 21, 2020 12:20

dmage commented Aug 21, 2020

View reviewed changes

pkg/cli/admin/prune/imageprune/prune_test.go Show resolved Hide resolved

dmage commented Aug 21, 2020

View reviewed changes

pkg/cli/admin/prune/imageprune/prune_test.go Outdated Show resolved Hide resolved

dmage commented Aug 21, 2020

View reviewed changes

pkg/cli/admin/prune/imageprune/prune_test.go Show resolved Hide resolved

dmage commented Aug 21, 2020

View reviewed changes

pkg/cli/admin/prune/imageprune/prune_test.go Show resolved Hide resolved

dmage commented Aug 21, 2020

View reviewed changes

pkg/cli/admin/prune/imageprune/prune_test.go Show resolved Hide resolved

dmage commented Aug 21, 2020

View reviewed changes

pkg/cli/admin/prune/imageprune/prune_test.go Show resolved Hide resolved

dmage commented Aug 21, 2020

View reviewed changes

pkg/cli/admin/prune/imageprune/prune_test.go Show resolved Hide resolved

ricardomaraschini reviewed Aug 24, 2020

View reviewed changes

dmage force-pushed the image-pruner branch 2 times, most recently from b1690ac to e82f7e7 Compare September 15, 2020 10:36

dmage force-pushed the image-pruner branch from e82f7e7 to 2e84af3 Compare September 15, 2020 14:41

dmage changed the title ~~[WIP] Bug 1860163: Optimized Image Pruner~~ Bug 1860163: Optimized Image Pruner Sep 15, 2020

openshift-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 15, 2020

dmage commented Sep 15, 2020

View reviewed changes

pkg/cli/admin/prune/images/images.go Show resolved Hide resolved

openshift-ci-robot assigned ricardomaraschini and soltysh Sep 16, 2020

soltysh reviewed Sep 16, 2020

View reviewed changes

pkg/cli/admin/prune/images/images.go Show resolved Hide resolved

ricardomaraschini suggested changes Sep 16, 2020

View reviewed changes

soltysh requested changes Sep 17, 2020

View reviewed changes

dmage force-pushed the image-pruner branch 5 times, most recently from 2804e92 to 7e0cb05 Compare September 17, 2020 16:08

dmage force-pushed the image-pruner branch from 7e0cb05 to 59abafc Compare September 17, 2020 16:22

ricardomaraschini approved these changes Sep 22, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2020

openshift-merge-robot merged commit 61364f0 into openshift:master Sep 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1860163: Optimized Image Pruner #510

Bug 1860163: Optimized Image Pruner #510

dmage commented Jul 30, 2020

openshift-ci-robot commented Jul 30, 2020

hongkailiu commented Sep 2, 2020

dmage commented Sep 16, 2020

soltysh left a comment

dmage commented Sep 16, 2020

soltysh commented Sep 16, 2020

ricardomaraschini Sep 16, 2020

dmage Sep 17, 2020

soltysh left a comment

soltysh Sep 17, 2020

soltysh Sep 17, 2020

dmage Sep 17, 2020

soltysh Sep 17, 2020

dmage Sep 17, 2020

soltysh Sep 17, 2020

dmage Sep 17, 2020

dmage commented Sep 17, 2020

dmage commented Sep 21, 2020

dmage commented Sep 21, 2020

ricardomaraschini left a comment

soltysh commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

openshift-bot commented Sep 23, 2020

openshift-bot commented Sep 23, 2020

openshift-ci-robot commented Sep 24, 2020

		@@ -101,33 +177,31 @@ type ImageDeleter interface {
		type ImageStreamDeleter interface {

Bug 1860163: Optimized Image Pruner #510

Bug 1860163: Optimized Image Pruner #510

Conversation

dmage commented Jul 30, 2020

openshift-ci-robot commented Jul 30, 2020

hongkailiu commented Sep 2, 2020

dmage commented Sep 16, 2020

soltysh left a comment

Choose a reason for hiding this comment

dmage commented Sep 16, 2020

soltysh commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh 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

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

dmage commented Sep 17, 2020

dmage commented Sep 21, 2020

dmage commented Sep 21, 2020

ricardomaraschini left a comment

Choose a reason for hiding this comment

soltysh commented Sep 23, 2020

openshift-ci-robot commented Sep 23, 2020

openshift-bot commented Sep 23, 2020

openshift-bot commented Sep 23, 2020

openshift-ci-robot commented Sep 24, 2020