Imagestream tag exclude from pruning - using object selection #17511

deads2k · 2017-11-29T14:02:40Z

Alternative to #16580.

To help clarify comments in #16580, https://bugzilla.redhat.com/show_bug.cgi?id=1410018, and openshift/api#9 I have put together a complete pull which adds the ability to protect namespaces and/or individual imagestreamtags based on our standard label and field selection techniques. This pull allows flows like this:

# old stock behavior
oc adm prune images <normal args>

# prune images, but only in unprotected namespaces (label of my choosing)
oc adm prune images <normal args> -l 'cluster-admin-protection-status notin (special)'

# prune images, but only gold level, then prune silver with different args
oc adm prune images <normal args> -l 'plan=gold'
oc adm prune images <more restrictive args> -l 'plan=silver'

# protect particular images, you don't get to choose this label.  myspecial:latest isn't pruned
oc label ns/openshift "image.openshift.io/allow-prune-immunity=true"
oc annotate -n openshift istag/myspecial:latest "image.openshift.io/prune-immunity=true"
oc adm prune images <normal args> [optional selector]

I'd still prefer to label the istag instead of using the equivalent of a field selector, but this gets the job done without any API changes.

@bparees @smarterclayton @mfojtik @openshift/cli-review

bparees · 2017-11-29T16:45:51Z

@deads2k are you going to write the docs for this approach (@legionus already wrote docs for the earlier implementation)?

Are you going to work w/ QE to update their tests and shepherd it through the rest of the process since the design has changed?

I know you think this is trivial, but you're creating significant churn on our side that you seem unwilling to acknowledge.

Also from what I see this approach does not currently offer a way to ignore immunity(let alone selectively ignore it), which is another advantage of the regex approach.

deads2k · 2017-11-29T17:40:09Z

Also from what I see this approach does not currently offer a way to ignore immunity(let alone selectively ignore it), which is another advantage of the regex approach.

Ignoring immunity is essentially the same as removing a label or an annotation. With either implementation it requires a modification to perform selectively. To perform non-selectively, you can simply remove labels globally. As with the rest of our API objects, the ACL boundary is the namespace.

@deads2k are you going to write the docs for this approach (@legionus already wrote docs for the earlier implementation)?

Are you going to work w/ QE to update their tests and shepherd it through the rest of the process since the design has changed?

If need be, yes. I would prefer to do that and gain the benefits described in the bugzilla along with consistency of experience when using the CLI.

My preference would be for broad CLI changes introducing new patterns or deviating from existing patterns to be subject to review in the broad sense and individual commands to maintained separately. This would mirror the approach upstream, where additions using existing patterns are subject to cursory review by sig-cli-leads and involved changes are more heavily scrutinized. I don't think that juan or fabiano are pruning experts, but the experience they are trying to cultivate and maintain for consistency within oc is something they are aware of.

If I need to own the card end-to-end, I can do so. However, I think we'd be better served in the end to grow the knowledge about common patterns and user experience across the teams which own particular commands. Consistency in these areas makes using new commands easier and knowledge of the general patterns gives more confidence when adding CLI features.

bparees · 2017-11-29T17:42:43Z

Ignoring immunity is essentially the same as removing a label or an annotation. With either implementation it requires a modification to perform selectively. To perform non-selectively, you can simply remove labels globally. As with the rest of our API objects, the ACL boundary is the namespace.

i still don't agree that finding and updating N server side objects is easier or better than tweaking a few regexes locally (or not passing the regex at all to disable immunity).

miminar · 2017-11-30T08:59:59Z

i still don't agree that finding and updating N server side objects is easier or better than tweaking a few regexes locally (or not passing the regex at all to disable immunity).

@bparees without the immunity being set on namespaces/imagestreams, each time the user attempts to prune manually, he needs to remember all namespaces/imagestreams he wants to preserve and write it down into a regexp on command-line, which gets tedious. I know that being able to write this regexps into a file and passing it to the command helps, but the user still needs to remember he has some namespaces/istags he wants to protect.

IMHO the namespace immunity won't change easily over time - company will want particular namespaces/imagestreams to protect against pruning in any case. Setting the label once and not needing to care about it anymore is a much more convenient solution to the problem.

miminar · 2017-11-30T09:18:45Z

Building upon this approach also allows for per namespace/is pruning options. E.g. keep this X tag revisions in proj/myapp and X+2 revisions in proj/myprecious.

miminar · 2017-11-30T09:22:09Z

pkg/image/prune/prune.go

@@ -351,8 +365,18 @@ func (p *pruner) addImageStreamsToGraph(streams *imageapi.ImageStreamList, limit
 					continue
 				}

+				isNamespaceEligible := p.namespacesToPrune.Has(stream.Namespace)
+				doesNamespaceAllowImmunity := p.namespacesToAllowImmunity.Has(stream.Namespace)
+				isISTagImmune := stream.Spec.Tags[tag].Annotations[protectionAnnotation] == "true"


I'd welcome per-imagestream control as well. Where istag's annotation overrides is' annotation.

I'd welcome per-imagestream control as well. Where istag's annotation overrides is' annotation.

It's easy enough to add, but the initial use-case didn't request it. @miminar could it be a followup?

@soltysh @fabianofranz @juanvallejo add it or no?

👍 for adding it (can be followup). IIRC discussions in the topic it was about both IS and ISTag to be excluded from pruning.

👍 for a follow-up

miminar · 2017-11-30T09:25:05Z

pkg/oc/admin/prune/images.go

@@ -162,6 +170,7 @@ func NewCmdPruneImages(f *clientcmd.Factory, parentName, name string, out io.Wri
 	cmd.Flags().StringVar(&opts.CABundle, "certificate-authority", opts.CABundle, "The path to a certificate authority bundle to use when communicating with the managed Docker registries. Defaults to the certificate authority data from the current user's config file. It cannot be used together with --force-insecure.")
 	cmd.Flags().StringVar(&opts.RegistryUrlOverride, "registry-url", opts.RegistryUrlOverride, "The address to use when contacting the registry, instead of using the default value. This is useful if you can't resolve or reach the registry (e.g.; the default is a cluster-internal URL) but you do have an alternative route that works. Particular transport protocol can be enforced using '<scheme>://' prefix.")
 	cmd.Flags().BoolVar(&opts.ForceInsecure, "force-insecure", opts.ForceInsecure, "If true, allow an insecure connection to the docker registry that is hosted via HTTP or has an invalid HTTPS certificate. Whenever possible, use --certificate-authority instead of this dangerous option.")
+	cmd.Flags().StringVarP(&opts.NamespacesToPruneSelector, "selector", "l", "", "Selector (label query) to filter on, not including uninitialized ones, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2).")


I would turn the e.g. part into a full example.

I would turn the e.g. part into a full example.

Not sure I understand. Those are valid examples.

miminar · 2017-11-30T11:10:05Z

pkg/oc/admin/prune/images.go

@@ -104,19 +106,24 @@ var (
 	defaultKeepYoungerThan         = 60 * time.Minute
 	defaultKeepTagRevisions        = 3
 	defaultPruneImageOverSizeLimit = false
+
+	// TODO choose final name and promote to API
+	protectionLabel = "image.openshift.io/allow-prune-immunity"


Can it be more general? (e.g. openshift.io/allow-prune-immunity). We have pruners for resources other than image.

Can it be more general? (e.g. openshift.io/allow-prune-immunity). We have pruners for resources other than image.

I would rather start specific and move out to generality. Pruning isn't a well explored space yet. See how deployments do it as a counter example.

wanghaoran1988 · 2017-12-05T14:33:43Z

/cc @geliu2016

deads2k · 2017-12-18T20:56:42Z

Building upon this approach also allows for per namespace/is pruning options. E.g. keep this X tag revisions in proj/myapp and X+2 revisions in proj/myprecious.

Yep.

deads2k · 2017-12-18T21:12:29Z

@soltysh @miminar I have rebased.

I like the suggestion of respecting the flag set on an imagestream, but I'd like to move the bulk of the work and circle back to it. Perhaps @juanvallejo could pick it up and work through the test matrix?

deads2k · 2017-12-19T16:29:06Z

#17879
#17880

deads2k · 2017-12-19T23:55:46Z

/retest

deads2k · 2017-12-20T13:20:57Z

/retest

deads2k · 2017-12-20T15:28:37Z

/retest

soltysh · 2017-12-21T11:52:01Z

pkg/image/prune/prune.go

@@ -55,6 +55,9 @@ const (
 	// ReferencedImageLayerEdgeKind defines an edge from an ImageStreamNode or an
 	// ImageNode to an ImageComponentNode.
 	ReferencedImageLayerEdgeKind = "ReferencedImageLayer"
+
+	// TODO choose final name and promote to API
+	protectionAnnotation = "image.openshift.io/prune-immunity"


@openshift/api-review ptal (yes David I know it's you, but also others 😉)

@openshift/api-review ptal (yes David I know it's you, but also others )

Fair. :)

soltysh · 2017-12-21T11:52:57Z

The implementation itself lgtm, I'm ok with @juanvallejo working on the followup. Just need to make it in a working state.

deads2k · 2017-12-21T13:31:33Z

/retest

to see if it stumbles on the same imagestreams.

miminar

Just a nit, LGTM otherwise.

miminar · 2018-01-02T13:58:00Z

pkg/oc/admin/prune/images.go

@@ -166,6 +186,7 @@ func NewCmdPruneImages(f *clientcmd.Factory, parentName, name string, out io.Wri
 	cmd.Flags().StringVar(&opts.RegistryUrlOverride, "registry-url", opts.RegistryUrlOverride, "The address to use when contacting the registry, instead of using the default value. This is useful if you can't resolve or reach the registry (e.g.; the default is a cluster-internal URL) but you do have an alternative route that works. Particular transport protocol can be enforced using '<scheme>://' prefix.")
 	cmd.Flags().BoolVar(&opts.ForceInsecure, "force-insecure", opts.ForceInsecure, "If true, allow an insecure connection to the docker registry that is hosted via HTTP or has an invalid HTTPS certificate. Whenever possible, use --certificate-authority instead of this dangerous option.")
 	cmd.Flags().BoolVar(opts.PruneRegistry, "prune-registry", *opts.PruneRegistry, "If false, the prune operation will clean up image API objects, but the none of the associated content in the registry is removed.  Note, if only image API objects are cleaned up through use of this flag, the only means for subsequently cleaning up registry data corresponding to those image API objects is to employ the 'hard prune' administrative task.")
+	cmd.Flags().StringVarP(&opts.NamespacesToPruneSelector, "selector", "l", "", "Selector (label query) to filter on, not including uninitialized ones, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2).")


Help should mention that the selector selects namespaces.

juanvallejo · 2018-01-08T18:55:11Z

/test end_to_end

mfojtik · 2018-01-17T12:03:51Z

@deads2k digged into the failed test, I think all 4 image streams that failed to update have one thing in common. They were all docker push-ed and the image stream tag was created by registry...

I wonder if we default the referencePolicy.type in API or if registry can somehow bypass this validation check.

I guess this is a separate issue, so we can opt-out the failing image streams (grep -v or something) for now to move this PR forward. Make an issue for the @openshift/sig-developer-experience or @smarterclayton (who added referencePolicy) to investigate how it is possible that those tag have that field set to an empty string.

Wild guess: we don't validate the ImageStreamMappings for the referencePolicy.type, which allows registry to create IST that has the field set to "" ?

openshift-merge-robot · 2018-01-24T08:10:55Z

@deads2k PR needs rebase

openshift-ci-robot · 2018-01-24T14:39:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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~~ [deads2k]
~~pkg/cmd/OWNERS~~ [deads2k]
~~pkg/image/OWNERS~~ [deads2k]
~~pkg/oc/OWNERS~~ [deads2k]
~~test/end-to-end/OWNERS~~ [deads2k]
~~test/testdata/OWNERS~~ [deads2k]

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

mfojtik · 2018-02-27T12:00:21Z

@bparees @openshift/sig-developer-experience were there any progress on determining the root cause of why certain image streams cannot be annotated (iow. we store the corrupted object in etcd) ?

miminar · 2018-02-27T12:02:30Z

/retest

openshift-ci-robot · 2018-02-27T12:09:22Z

@deads2k: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/service-catalog	`7f60fe8`	link	`/test service-catalog`
ci/openshift-jenkins/gcp	`7f60fe8`	link	`/test gcp`
ci/openshift-jenkins/verify	`7f60fe8`	link	`/test verify`
ci/openshift-jenkins/extended_image_registry	`7f60fe8`	link	`/test extended_image_registry`
ci/openshift-jenkins/extended_conformance_install	`7f60fe8`	link	`/test extended_conformance_install`
ci/openshift-jenkins/cmd	`7f60fe8`	link	`/test cmd`
ci/openshift-jenkins/unit	`7f60fe8`	link	`/test unit`
ci/openshift-jenkins/integration	`7f60fe8`	link	`/test integration`
ci/openshift-jenkins/extended_networking_minimal	`7f60fe8`	link	`/test extended_networking_minimal`
ci/openshift-jenkins/end_to_end	`7f60fe8`	link	`/test end_to_end`

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.

dmage · 2018-02-27T12:55:57Z

@mfojtik #18519

bparees · 2018-02-27T13:45:08Z

@bparees @openshift/sig-developer-experience were there any progress on determining the root cause of why certain image streams cannot be annotated (iow. we store the corrupted object in etcd) ?

@mfojtik it has always been this way, see my comment here where this was found and "worked around" (by setting the reference policy explicitly):
#12731 (review)

it's effectively by design, see the entire discussion (including comment from clayton) here:
#18519 (I tagged you in this issue, btw, are you not getting my notifications? You haven't responded to a number of them)

and my PR to at least make the error message clearer:
#18532

openshift-bot · 2018-05-28T18:36:58Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot · 2018-06-27T18:38:41Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2018-07-27T19:40:48Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

deads2k mentioned this pull request Nov 29, 2017

Imagestream tag exclude from pruning #16580

Merged

miminar suggested changes Nov 30, 2017

View reviewed changes

miminar mentioned this pull request Nov 30, 2017

add image soft prune (api object yes, registry no) #17480

Merged

openshift-ci-robot requested a review from geliu2016 December 5, 2017 14:33

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2017

deads2k force-pushed the image-11-prune branch from 2dca02b to 2bfa8b9 Compare December 18, 2017 21:04

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 18, 2017

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2017

deads2k assigned soltysh and miminar Dec 18, 2017

deads2k added 2 commits December 19, 2017 09:08

add label selection for image prune

70d88f8

generated

30a2a0e

deads2k force-pushed the image-11-prune branch from 2bfa8b9 to 30a2a0e Compare December 19, 2017 14:08

debug

74337de

deads2k force-pushed the image-11-prune branch 2 times, most recently from 681aa6b to f8f07d0 Compare December 20, 2017 14:56

debug

7f60fe8

deads2k force-pushed the image-11-prune branch from f8f07d0 to 7f60fe8 Compare December 20, 2017 20:03

soltysh reviewed Dec 21, 2017

View reviewed changes

miminar approved these changes Jan 2, 2018

View reviewed changes

openshift-ci-robot added the sig/developer-experience label Jan 17, 2018

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2018

deads2k closed this Jan 24, 2018

deads2k deleted the image-11-prune branch January 24, 2018 14:38

deads2k restored the image-11-prune branch January 24, 2018 14:38

deads2k reopened this Jan 24, 2018

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 28, 2018

openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 27, 2018

openshift-ci-robot closed this Jul 27, 2018

Imagestream tag exclude from pruning - using object selection #17511

Imagestream tag exclude from pruning - using object selection #17511

Conversation

deads2k commented Nov 29, 2017

bparees commented Nov 29, 2017

deads2k commented Nov 29, 2017

bparees commented Nov 29, 2017

miminar commented Nov 30, 2017 • edited Loading

miminar commented Nov 30, 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

wanghaoran1988 commented Dec 5, 2017

deads2k commented Dec 18, 2017

deads2k commented Dec 18, 2017

deads2k commented Dec 19, 2017

deads2k commented Dec 19, 2017

deads2k commented Dec 20, 2017

deads2k commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Dec 21, 2017

deads2k commented Dec 21, 2017

miminar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Jan 8, 2018

mfojtik commented Jan 17, 2018 • edited Loading

openshift-merge-robot commented Jan 24, 2018

openshift-ci-robot commented Jan 24, 2018

mfojtik commented Feb 27, 2018

miminar commented Feb 27, 2018

openshift-ci-robot commented Feb 27, 2018

dmage commented Feb 27, 2018

bparees commented Feb 27, 2018

openshift-bot commented May 28, 2018

openshift-bot commented Jun 27, 2018

openshift-bot commented Jul 27, 2018

miminar commented Nov 30, 2017 •

edited

Loading

mfojtik commented Jan 17, 2018 •

edited

Loading