Imagestream tag exclude from pruning #16580

legionus · 2017-09-27T13:57:02Z

Related RFE: https://bugzilla.redhat.com/show_bug.cgi?id=1410018

legionus · 2017-09-27T13:58:46Z

/test extended_image_registry

miminar · 2017-09-27T14:27:27Z

The regular expression can get very long and obfuscated.

Instead of adding new flag to the prune command. Could we allow to mark images and imagestream(tag)s as not prunable? For example with annotation like openshift.io/image.noPrune=true?

In case of the flag added to the prune command. I'd prefer the solution suggested by the customer - let the users specify a list of imagestream(tag)s in a file. I find the annotation approach more convenient though.

bparees · 2017-09-27T14:34:42Z

@miminar the annotation approach was discussed and rejected during planning and noted in the trello card:
"The better option would probably be to allow the imagestream itself to indicate it should be immune from pruning, but that's a big security problem (who's allowed to set that annotation/indicator?)"

we don't have good security models around annotation editing. So this was the short path to provide the capability being requested.

The intent of the card was to include support for a file listing images to be excluded(in addition to the regex support), if that's not in this PR it should be added (I haven't reviewed it yet).

bparees · 2017-09-27T14:55:10Z

pkg/oc/admin/prune/images.go

@@ -143,6 +145,7 @@ func NewCmdPruneImages(f *clientcmd.Factory, parentName, name string, out io.Wri
 	cmd.Flags().BoolVar(opts.AllImages, "all", *opts.AllImages, "Include images that were not pushed to the registry but have been mirrored by pullthrough.")
 	cmd.Flags().DurationVar(opts.KeepYoungerThan, "keep-younger-than", *opts.KeepYoungerThan, "Specify the minimum age of an image for it to be considered a candidate for pruning.")
 	cmd.Flags().IntVar(opts.KeepTagRevisions, "keep-tag-revisions", *opts.KeepTagRevisions, "Specify the number of image revisions for a tag in an image stream that will be preserved.")
+	cmd.Flags().StringVar(&opts.ExcludeImageStreamTag, "exclude-imagestream-tag", "", "The regular expression that will be applied to the ImageStream Tag to preserve it from pruning.")


Add an arg that takes a file containing a list of regexes to exclude (probably won't be true regexes normally, will just be a list of image names, but no reason not to allow them to be regexes, unless you have performance concerns)

@bparees Would it suit you if there are two options (this on and another with file as an argument) ?

yes, two args is what i'd expect.

@bparees done

exclude-imagestreamtags?

legionus · 2017-09-27T14:56:27Z

The intent of the card was to include support for a file listing images to be excluded(in addition to the regex support), if that's not in this PR it should be added (I haven't reviewed it yet).

@bparees I thought we agreed to add an option with a regular expression and not a file.
Well. I'll add another option.

miminar · 2017-09-27T15:23:38Z

@miminar the annotation approach was discussed and rejected during planning and noted in the trello card:
"The better option would probably be to allow the imagestream itself to indicate it should be immune from pruning, but that's a big security problem (who's allowed to set that annotation/indicator?)"

I see. I'm sorry for not participating in the discussion. I could imagine allowing at least a marking of images for those cluster-wide roles that already allow to edit images (e.g. image-auditor). I don't see security implications in that. Marking of imagestream(tag)s would be more challenging for sure.

bparees · 2017-09-27T19:18:15Z

@bparees I thought we agreed to add an option with a regular expression and not a file.

Sorry there was confusion, I have a different recollection but either way I think it speaks to the fact that we need to do a better job of updating the trello cards during planning or immediately after, with exactly what we intend to implement for the card (checklists are probably the best way to do this, rather than just putting it in the description). I'll try to do better with that, but I definitely will need help from everyone else as well.

(those updates would also help QE drafting TCs and also bystanders who sometimes watch our cards and wonder what we're doing).

bparees · 2017-09-27T19:20:34Z

I could imagine allowing at least a marking of images for those cluster-wide roles that already allow to edit images (e.g. image-auditor). I don't see security implications in that. Marking of imagestream(tag)s would be more challenging for sure.

Sure, that seems reasonable. However for now, this is exactly the behavior the customer requested in their RFE and I think it has some advantages (and some disadvantages, certainly) over having to mark the images, so i think it's worth doing and if we get additional requests for functionality in the future we can always do more.

legionus · 2017-09-28T11:59:22Z

/retest

legionus · 2017-09-28T13:11:32Z

/test extended_image_registry

miminar · 2017-09-28T13:11:20Z

pkg/image/prune/prune.go

-			glog.V(4).Infof("Image %q is younger than minimum pruning age, skipping (age=%v)", image.Name, age)
-			continue
-		}
-


So if these images were not added to the graph, the algorithm could remove blobs referenced by them, could it not?

@miminar yes. The layers will not have links in the graph.

That's a serious bug. And we need to back-port this pretty far. The mystery of blobs disappearing from the registry is solved!

Could you please capture the case of layers being removed when they should be kept in some unit/extended test? This is pretty important gap we should cover.

@miminar I'm on it. BZ 1487408

miminar · 2017-09-28T13:27:40Z

pkg/oc/admin/prune/images.go

@@ -143,6 +147,8 @@ func NewCmdPruneImages(f *clientcmd.Factory, parentName, name string, out io.Wri
 	cmd.Flags().BoolVar(opts.AllImages, "all", *opts.AllImages, "Include images that were not pushed to the registry but have been mirrored by pullthrough.")
 	cmd.Flags().DurationVar(opts.KeepYoungerThan, "keep-younger-than", *opts.KeepYoungerThan, "Specify the minimum age of an image for it to be considered a candidate for pruning.")
 	cmd.Flags().IntVar(opts.KeepTagRevisions, "keep-tag-revisions", *opts.KeepTagRevisions, "Specify the number of image revisions for a tag in an image stream that will be preserved.")
+	cmd.Flags().StringVar(&opts.ExcludeImageStreamTag, "exclude-imagestream-tag", "", "The regular expression that will be applied to the ImageStream Tag to preserve it from pruning.")
+	cmd.Flags().StringVar(&opts.ExcludeImageStreamTagFile, "exclude-imagestream-tag-file", "", "The filename that contains the regular expressions that will be applied to the ImageStream Tag to preserve it from pruning.")


s/that will be applied to the ImageStream Tag to preserve it from pruning./matching ImageStreamTags excluded from pruning./

Add an example and point to regular expression syntax accepted.

Also describe the string you're matching against (<namespace>/<name>:<tag>).

miminar · 2017-09-28T13:32:35Z

pkg/oc/admin/prune/images.go

@@ -143,6 +145,7 @@ func NewCmdPruneImages(f *clientcmd.Factory, parentName, name string, out io.Wri
 	cmd.Flags().BoolVar(opts.AllImages, "all", *opts.AllImages, "Include images that were not pushed to the registry but have been mirrored by pullthrough.")
 	cmd.Flags().DurationVar(opts.KeepYoungerThan, "keep-younger-than", *opts.KeepYoungerThan, "Specify the minimum age of an image for it to be considered a candidate for pruning.")
 	cmd.Flags().IntVar(opts.KeepTagRevisions, "keep-tag-revisions", *opts.KeepTagRevisions, "Specify the number of image revisions for a tag in an image stream that will be preserved.")
+	cmd.Flags().StringVar(&opts.ExcludeImageStreamTag, "exclude-imagestream-tag", "", "The regular expression that will be applied to the ImageStream Tag to preserve it from pruning.")


exclude-imagestreamtags?

miminar · 2017-09-28T13:36:50Z

Also another extended test for the patterns file would be nice.

legionus · 2017-09-28T13:39:13Z

Also another extended test for the patterns file would be nice.

@miminar Both of these options fill same array. If one works, then filtering works.

legionus · 2017-09-28T14:29:09Z

@miminar comments addressed.

dmage

We should not limit the length of regexps.

dmage · 2017-09-28T14:46:41Z

pkg/image/prune/prune.go

-					if exceedsLimits(stream, imageNode.Image, limits) {
-						kind = WeakReferencedImageEdgeKind
+
+				if !istExcluded {


if istExcluded { ... } else if algorithm.pruneOverSizeLimit { ... } else {

Do we want to add log messages for other cases?

dmage · 2017-09-28T14:50:08Z

pkg/image/prune/prune.go

-						kind = ReferencedImageEdgeKind
-					}
+					glog.V(4).Infof("ImageStreamTag %q is excluded by the regular expression", dockerImageReference)
+					kind = ReferencedImageEdgeKind


Shouldn't it be WeakReferencedImageEdgeKind?

@dmage No. https://github.com/legionus/origin/blob/304d2b846cbaecc6b30a5b692d9b343cf2c95660/pkg/image/prune/prune.go#L635

OK, I thought excluded=pruned. s/excluded/protected/ or kept.

dmage · 2017-09-28T14:54:21Z

pkg/oc/admin/prune/images.go

@@ -74,6 +76,10 @@ var (
 	  # To actually perform the prune operation, the confirm flag must be appended
 	  %[1]s %[2]s --keep-tag-revisions=3 --keep-younger-than=60m --confirm

+	  # To exclude an imagestream from pruning you can specify a regular expression
+	  # which will be applied to the '<namespace>/<name>:<tag>' string.
+	  %[1]s %[2]s --exclude-imagestreamtag=myproject/hello-openshift


It may be worth mentioning the anchoring (^ and $).

Or at least change the example to --exclude-imagestreamtag=^myproject/hello-openshift:

And I'd change the option name --keep-imagestreamtag=^myproject/hello-openshift:

dmage · 2017-09-28T14:57:50Z

pkg/oc/admin/prune/images.go

+
+	var patterns []*regexp.Regexp
+
+	scanner := bufio.NewScanner(file)


Don't use bufio.NewScanner for reading lines. It limits the length of tokens (iow. lines).
Use file.ReadString('\n') instead.

bparees · 2017-11-15T16:46:18Z

/lgtm
/retest

openshift-merge-robot · 2017-11-15T16:46:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, dmage, legionus

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

Needs approval from an approver in each of these OWNERS Files:

~~contrib/completions/OWNERS~~ [bparees]
~~pkg/image/OWNERS~~ [bparees]
~~pkg/oc/OWNERS~~ [bparees]
~~test/extended/OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

legionus · 2017-11-15T16:47:28Z

Test extended_image_registry is broken:

Failure summary:


  1. Hosts:    localhost
     Play:     Create OpenShift certificates for master hosts
     Task:     Set Default scheduler predicates and priorities
     Message:  An unhandled exception occurred while running the lookup plugin 'openshift_master_facts_default_predicates'. Error was a <class 'ansible.errors.AnsibleError'>, original message: Unknown short_version 3.8

bparees · 2017-11-15T16:53:54Z

@legionus i worked with @stevekuznetsov on that this morning, it should be resolved now, hopefully the current run will pass.

bparees · 2017-11-16T00:23:53Z

/retest

openshift-bot · 2017-11-16T09:25:28Z

/retest

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

openshift-merge-robot · 2017-11-16T10:47:25Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-ci-robot · 2017-11-16T11:17:17Z

@legionus: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install_update	`b70983d`	link	`/test extended_conformance_install_update`

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.

openshift-merge-robot · 2017-11-16T11:48:15Z

Automatic merge from submit-queue (batch tested with PRs 16487, 16580).

mfojtik · 2017-11-16T12:49:37Z

@bparees i'm going to revert this commit because registry/util was removed from rebase branch so you can't use that helper anymore. Second option would be to disable the test.

mfojtik · 2017-11-16T12:49:51Z

See: https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/17115/test_pull_request_origin_extended_networking_minimal/10598/

mfojtik · 2017-11-16T13:00:18Z

@bparees i reverted this but realized with Alex that we moved that registry helper func into different package, so you just need to remove that import and strip the registryutil. in the test. Anyway, can you please resubmit this after the rebase lands? I really hate to rebase 80 commits to adapt this to post-rebase world.

deads2k · 2017-11-16T13:25:22Z

The use a regex here appears to make the command hard to use. In addition, if I'm reading the code correctly, the caller either has to specify specific namespaces in his regex to avoid accidentally matching image streams owned by other people that should be pruned.

Given those two things, why not just make it official and simply making some namespaces immune to pruning for a command. That could be done via label selection of namespaces (users can't label namespaces), it wouldn't require an additional flag, and it wouldn't be vulnerable to less than obvious matching problems.

bparees · 2017-11-16T18:16:08Z

The use a regex here appears to make the command hard to use.

if you're doing this, you have a need for the complexity.

In addition, if I'm reading the code correctly, the caller either has to specify specific namespaces in his regex to avoid accidentally matching image streams owned by other people that should be pruned.

yup.

Given those two things, why not just make it official and simply making some namespaces immune to pruning for a command.

"some namespaces"? what namespaces?

That could be done via label selection of namespaces (users can't label namespaces), it wouldn't require an additional flag, and it wouldn't be vulnerable to less than obvious matching problems.

because you're assuming that the exclusion is scoped to a particular namespace. What if i want to prune some but not all images in a particular namespace?

This feature was implemented based on the specific request of a customer in fulfillment of how they wanted it to work. You can read the RFE.

bparees · 2017-11-17T18:11:24Z

fyi i'm deferring this suggestion. @legionus implemented sufficient functionality to address the customer need and we're moving on, i'm not willing to waste his time rewriting it when we have more pressing concerns.

Certainly in the future we can introduce the ability to exclude pruning by a namespace label. Providing this (more flexible) capability today does not preclude providing that capability in the future.

once the rebase has landed we'll be re-merging this feature.

smarterclayton · 2017-11-28T21:49:48Z

I'm ok with this as it is - I understand the improvement David wants to make, I think it's a good long term thing to do, but I don't see a justification to go with that approach over the current approach that is worth rewriting over.

Also, this is good design feedback that would be better earlier in the process. What can we do to make these suggestions earlier?

miminar · 2017-11-29T09:30:07Z

Also, this is good design feedback that would be better earlier in the process. What can we do to make these suggestions earlier?

Well, the suggestion was raised much earlier and was discussed in planning as well but was turned down.

deads2k · 2017-11-29T14:05:17Z

I'm ok with this as it is - I understand the improvement David wants to make, I think it's a good long term thing to do, but I don't see a justification to go with that approach over the current approach that is worth rewriting over.

Just so we can avoid the concern about cost, I put together a pull which uses label and field selection to protect image. It is complete, including tests, without any API changes here: #17511.

Also, this is good design feedback that would be better earlier in the process. What can we do to make these suggestions earlier?

I'm sure it was an oversight, but I don't see @juanvallejo, @fabianofranz, @openshift/cli-review, or @openshift/api-review tagged in this pull or in the bugzilla or in the trello card for review or comment. I suspect that any of those actors are familiar with the over-arching design principles of kubectl and oc regarding object selection via the REST API and the CLI tools.

ctrought · 2020-05-14T17:38:40Z

/reopen

PR was reverted, I don't think any more granularity has been provided from any other options yet. Would still like to see more control over pruning image tags.

openshift-ci-robot · 2020-05-14T17:38:54Z

@ctrought: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

PR was reverted, I don't think any more granularity has been provided from any other options yet. Would still like to see more control over pruning image tags.

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.

legionus added the component/image label Sep 27, 2017

legionus self-assigned this Sep 27, 2017

legionus requested review from dmage, miminar and bparees September 27, 2017 13:57

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 27, 2017

legionus assigned mfojtik Sep 27, 2017

bparees requested changes Sep 27, 2017

View reviewed changes

legionus changed the title ~~Imagestream tag exclude from prunning~~ [WIP] Imagestream tag exclude from prunning Sep 27, 2017

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2017

legionus force-pushed the image-exclude-from-prune branch from 939bbb9 to b6375e6 Compare September 27, 2017 15:56

miminar suggested changes Sep 28, 2017

View reviewed changes

legionus force-pushed the image-exclude-from-prune branch from b6375e6 to 304d2b8 Compare September 28, 2017 14:22

legionus changed the title ~~[WIP] Imagestream tag exclude from prunning~~ Imagestream tag exclude from prunning Sep 28, 2017

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 28, 2017

dmage suggested changes Sep 28, 2017

View reviewed changes

legionus force-pushed the image-exclude-from-prune branch 2 times, most recently from 4c0492e to 84cf280 Compare September 28, 2017 15:54

bparees approved these changes Nov 15, 2017

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2017

bparees changed the title ~~Imagestream tag exclude from prunning~~ Imagestream tag exclude from pruning Nov 16, 2017

openshift-merge-robot merged commit 8c5b43d into openshift:master Nov 16, 2017

mfojtik mentioned this pull request Nov 16, 2017

Revert "Imagestream tag exclude from pruning" #17344

Merged

legionus mentioned this pull request Nov 20, 2017

Add documentation for --exclude-imagestreamtag and --exclude-imagestreamtag-file openshift/openshift-docs#6429

Closed

deads2k mentioned this pull request Nov 28, 2017

add way to set imagestreamtag labels openshift/api#9

Closed

legionus mentioned this pull request Nov 29, 2017

Add new option to exclude imagestream tag from pruning by regular expression #17509

Closed

deads2k mentioned this pull request Nov 29, 2017

Imagestream tag exclude from pruning - using object selection #17511

Closed


		var patterns []*regexp.Regexp

		scanner := bufio.NewScanner(file)

Imagestream tag exclude from pruning #16580

Imagestream tag exclude from pruning #16580

Conversation

legionus commented Sep 27, 2017

legionus commented Sep 27, 2017

miminar commented Sep 27, 2017

bparees commented Sep 27, 2017

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

legionus commented Sep 27, 2017

miminar commented Sep 27, 2017

bparees commented Sep 27, 2017

bparees commented Sep 27, 2017

legionus commented Sep 28, 2017

legionus commented Sep 28, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miminar commented Sep 28, 2017

legionus commented Sep 28, 2017

legionus commented Sep 28, 2017

dmage 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

Choose a reason for hiding this comment

bparees commented Nov 15, 2017

openshift-merge-robot commented Nov 15, 2017

legionus commented Nov 15, 2017

bparees commented Nov 15, 2017

bparees commented Nov 16, 2017

openshift-bot commented Nov 16, 2017

openshift-merge-robot commented Nov 16, 2017

openshift-ci-robot commented Nov 16, 2017 • edited Loading

openshift-merge-robot commented Nov 16, 2017

mfojtik commented Nov 16, 2017

mfojtik commented Nov 16, 2017

mfojtik commented Nov 16, 2017

deads2k commented Nov 16, 2017

bparees commented Nov 16, 2017

bparees commented Nov 17, 2017

smarterclayton commented Nov 28, 2017 • edited Loading

miminar commented Nov 29, 2017

deads2k commented Nov 29, 2017

ctrought commented May 14, 2020

openshift-ci-robot commented May 14, 2020

openshift-ci-robot commented Nov 16, 2017 •

edited

Loading

smarterclayton commented Nov 28, 2017 •

edited

Loading