Image pruner: Determine protocol just once #14914

miminar · 2017-06-27T15:56:52Z

Determine the registry protocol once. Do not change to other protocol during the run. This will produce nicer output without unrelated protocol fallback errors.

A follow-up for #14114 and #14405
Resolves #15291

miminar · 2017-06-27T15:58:44Z

[test][testextended][extended:core(registry|imageapi|ImagePrune|ImageQuota)]

soltysh

Overall looks good, some nits.

soltysh · 2017-06-28T09:58:10Z

pkg/image/prune/prune.go

-func (drp *defaultRegistryPinger) ping(registry string) error {
-	healthCheck := func(proto, registry string) error {
+func (drp *defaultRegistryPinger) ping(registry string) (string, error) {
+	registryURL, err := tryProtocolsWithRegistryURL(registry, drp.insecure, func(u url.URL) error {
 		// TODO: `/healthz` route is deprecated by `/`; remove it in future versions


Can't we check / and fallback to /healthz only if the former fail? This way we'll support both.

We can. Resolved.

soltysh · 2017-06-28T10:00:58Z

pkg/image/prune/prune.go

+		protos = []string{"https"}
+	}
+	registry = url.Host
+	glog.V(4).Infof("url: %#+v", url)


This looks like a development debug, can you make it more friendly?

You're right. Ditched.

soltysh · 2017-06-28T10:01:59Z

pkg/image/prune/prune.go

+			glog.V(4).Infof("Trying protocol %s for the registry url %s", proto, registry)
+		default:
+			glog.V(4).Infof("Falling back to protocol %s for the registry url %s", proto, registry)
+		}


This is bad, just log the fact, you'll figure out when looking at logs, that this is a retry. There's no point in hardcoding this switch.

As you wish. I liked it better this way though.

soltysh · 2017-06-28T10:02:40Z

pkg/image/prune/prune.go

+		}
+
+		if err != nil {
+			// TODO: remove the first


Yep. Removed.

soltysh · 2017-06-28T10:04:55Z

pkg/image/prune/prune.go

+// tryProtocolsWithRegistryURL runs given callback with different protocols until no error is returned. The
+// https protocol is the first attempt. If it fails and allowInsecure is true, http will be the next. Obtained
+// errors will be concatenated and returned.
+func tryProtocolsWithRegistryURL(registry string, allowInsecure bool, callback func(registryURL url.URL) error) (*url.URL, error) {


callback here suggests an actual callback method, but it's not. It's the heart of this method, an action performed by this function. I'd suggest renaming the variable.

action maybe?

Good suggesion. Changed.

soltysh · 2017-06-28T10:08:53Z

Although it looks like your changes are failing tests, so you need to address that one.

miminar · 2017-06-29T10:52:39Z

Tests should be passing now.

soltysh · 2017-06-29T12:45:20Z

LGTM!

miminar · 2017-06-29T13:32:44Z

One last unit test fix.

miminar · 2017-07-03T09:29:29Z

Flake #14129 re-[test]

miminar · 2017-07-03T11:40:07Z

~~@mfojtik merge please.~~ Waiting for code unfreeze.

miminar · 2017-07-03T15:23:50Z

I reformatted logging messages again and made a small change to defaulting to insecure connection.

Since now internal docker registry URL may be a DNS, let's not default to insecure connection if --registry-url override is empty. As long as a local IP is used as internal URL, insecure fall-back will be allowed.

miminar · 2017-07-24T12:18:31Z

Fixed broken formatting of help message.

openshift-bot · 2017-07-24T13:15:58Z

Evaluated for origin testextended up to 5cc9d12

openshift-bot · 2017-07-24T14:15:56Z

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/906/) (Base Commit: 74121fa) (PR Branch Commit: 5cc9d12) (Extended Tests: core(registry|imageapi|ImagePrune|ImageQuota))

fabianofranz · 2017-08-07T18:29:04Z

/test extended_templates

miminar · 2017-08-08T10:26:53Z

Yum flakes #8571 and #10162. Flake #15432

miminar · 2017-08-08T10:26:59Z

/retest

fabianofranz · 2017-08-08T18:41:22Z

/approve

miminar · 2017-08-14T11:10:59Z

Jenkins job not found.
/retest

miminar · 2017-08-14T11:11:27Z

/test extended_image_registry

miminar · 2017-08-14T14:55:36Z

Flakes #15432 and #15763

miminar · 2017-08-14T14:56:18Z

/retest

openshift-bot · 2017-08-14T19:01:41Z

/retest

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

openshift-bot · 2017-08-14T23:03:40Z

/retest

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

openshift-bot · 2017-08-15T03:05:40Z

/retest

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

openshift-bot · 2017-08-15T07:07:39Z

/retest

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

miminar · 2017-08-15T10:10:13Z

Flake #15432 again.
/retest

openshift-ci-robot · 2017-08-15T11:31:09Z

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

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/check	`47551a8`	link	`/test check`
ci/openshift-jenkins/extended_image_registry	`90e575c`	link	`/test extended_image_registry`

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.

miminar · 2017-08-15T14:41:07Z

/test extended_conformance_gce
/test check

Determine the registry protocol once. Do not change to other protocol during the run. This will produce nicer output without unrelated protocol fallback errors. Do not default to insecure connection when the --registry-url is empty. Move registry client initialization just before the start of the pruner - so we can precisely determine whether to allow for insecure fall-back based on collected images and image streams. Move ping() outside of pruner. Instead, determine the registry URL before the pruner starts and assume it won't change during the run. Signed-off-by: Michal Minář <[email protected]>

Signed-off-by: Michal Minář <[email protected]>

miminar · 2017-08-16T10:09:31Z

Rebased. @soltysh may I have your lgtm one more time?

soltysh · 2017-08-16T22:01:59Z

/lgtm

openshift-merge-robot · 2017-08-16T22:02:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabianofranz, miminar, soltysh

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

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

~~pkg/image/OWNERS~~ [soltysh]
~~pkg/oc/OWNERS~~ [fabianofranz]

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

openshift-merge-robot · 2017-08-16T22:27:30Z

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

openshift-merge-robot · 2017-08-17T00:22:24Z

Automatic merge from submit-queue

Automatic merge from submit-queue [3.6][Backport] Prune images (not)securely Back-porting: - #14114 - #14405 - #14914 - #15899 Resolves [bz#1476779](https://bugzilla.redhat.com/show_bug.cgi?id=1476779)

miminar changed the title ~~imagesprune: Determina protocol just once~~ Image pruner: Determine protocol just once Jun 27, 2017

miminar force-pushed the secure-image-prune branch from c3d693f to d9af60d Compare June 27, 2017 15:58

miminar mentioned this pull request Jun 27, 2017

Secure vs insecure image pruning openshift/openshift-docs#4471

Merged

soltysh approved these changes Jun 28, 2017

View reviewed changes

miminar force-pushed the secure-image-prune branch from d9af60d to 178a139 Compare June 29, 2017 10:50

miminar force-pushed the secure-image-prune branch 3 times, most recently from 30b4a6c to 093d03b Compare June 29, 2017 11:11

miminar force-pushed the secure-image-prune branch from 093d03b to 7f24389 Compare June 29, 2017 13:32

miminar force-pushed the secure-image-prune branch 2 times, most recently from 704189d to 58876ea Compare July 3, 2017 15:15

openshift deleted a comment from openshift-bot Jul 3, 2017

miminar force-pushed the secure-image-prune branch 5 times, most recently from 7de7ae1 to c3bfb0b Compare July 4, 2017 16:00

miminar force-pushed the secure-image-prune branch from c3bfb0b to 2a03fe4 Compare July 24, 2017 12:17

miminar force-pushed the secure-image-prune branch from a14089d to 5cc9d12 Compare July 24, 2017 13:10

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

Michal Minář added 2 commits August 15, 2017 16:56

image-pruner: Reenable registry-url validation

7fd22bb

Signed-off-by: Michal Minář <[email protected]>

miminar force-pushed the secure-image-prune branch from 90e575c to 7fd22bb Compare August 15, 2017 14:56

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2017

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

openshift-merge-robot merged commit 86abb1f into openshift:master Aug 17, 2017

miminar mentioned this pull request Sep 5, 2017

[3.6][Backport] Prune images (not)securely #16138

Merged

miminar deleted the secure-image-prune branch October 10, 2017 09:54

Image pruner: Determine protocol just once #14914

Image pruner: Determine protocol just once #14914

Conversation

miminar commented Jun 27, 2017 • edited Loading

miminar commented Jun 27, 2017

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

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

soltysh commented Jun 28, 2017

miminar commented Jun 29, 2017

soltysh commented Jun 29, 2017

miminar commented Jun 29, 2017

miminar commented Jul 3, 2017

miminar commented Jul 3, 2017 • edited Loading

miminar commented Jul 3, 2017

miminar commented Jul 24, 2017

openshift-bot commented Jul 24, 2017

openshift-bot commented Jul 24, 2017

fabianofranz commented Aug 7, 2017

miminar commented Aug 8, 2017

miminar commented Aug 8, 2017

fabianofranz commented Aug 8, 2017

miminar commented Aug 14, 2017

miminar commented Aug 14, 2017

miminar commented Aug 14, 2017

miminar commented Aug 14, 2017

openshift-bot commented Aug 14, 2017

openshift-bot commented Aug 14, 2017

openshift-bot commented Aug 15, 2017

openshift-bot commented Aug 15, 2017

miminar commented Aug 15, 2017

openshift-ci-robot commented Aug 15, 2017 • edited Loading

miminar commented Aug 15, 2017

miminar commented Aug 16, 2017

soltysh commented Aug 16, 2017

openshift-merge-robot commented Aug 16, 2017

openshift-merge-robot commented Aug 16, 2017

openshift-merge-robot commented Aug 17, 2017

miminar commented Jun 27, 2017 •

edited

Loading

miminar commented Jul 3, 2017 •

edited

Loading

openshift-ci-robot commented Aug 15, 2017 •

edited

Loading