Bug 1450291 - Improve logs in image pruning #14405

soltysh · 2017-05-30T10:32:07Z

This partially address problem from https://bugzilla.redhat.com/show_bug.cgi?id=1450291 and cleans up the logging from my own experience going through them several times past few weeks.

@miminar @mfojtik ptal

soltysh · 2017-05-30T10:32:12Z

[test]

mfojtik · 2017-05-30T11:48:40Z

LGTM

miminar · 2017-05-30T12:52:03Z

pkg/image/prune/prune.go

 	return deleteFromRegistry(registryClient, fmt.Sprintf("%s/v2/%s/manifests/%s", registryURL, repoName, manifest))
 }
+
+func getName(o metav1.ObjectMeta) string {


Could this take runtime.Object instead and access the attributes via o.(metav1.ObjectMetaAccessor).GetObjectMeta()?

Hmmm.... lemme check this approach.

accessor, err := meta.Accessor(obj) and then using accessor.GetName() and accessor.GetNamespace is probably better. Will got with that.

miminar · 2017-05-30T12:53:44Z

pkg/image/prune/prune.go

-			return fmt.Errorf("error sending request: %v", err)
+			if proto != "https" && strings.Contains(err.Error(), "malformed HTTP response") {
+				return fmt.Errorf("%v.\n* Are you trying to connect to a TLS-enabled registry without TLS?", err)
+			}


This will conflict with #14114.

I'll update this now that the other one is in place.

Or maybe I should ask in what way it will conflict? This will give you a hint that you should use https when remote end supports that. #14114 will force you to use that, so this error should be seen very rarely, in some weird cases, I guess.

👍 for the hint. Nevertheless, this should have been captured by the ping() call.

I must have forgotten to modify this code same the way as the ping() (see

origin/pkg/image/prune/prune.go

Line 199 in 90592c5

if drp.insecure || netutils.IsPrivateAddress(registry) {

) - to exclude HTTP connection from attempts when secure connection is requested. If you want, you can amend it here, otherwise I'll do it after this lands.

By the way, the hint would be useful in the pinger as well.

You can move it as a followup, in that case.

miminar · 2017-05-30T12:54:32Z

pkg/image/prune/prune.go

@@ -891,7 +889,7 @@ func (p *pruner) Prune(
 	glog.V(1).Infof("Using registry: %s", registryURL)

 	if err := p.registryPinger.ping(registryURL); err != nil {
-		return fmt.Errorf("error communicating with registry: %v", err)
+		return fmt.Errorf("error communicating with registry %s: %v", registryURL, err)


Is the url always present in the err?

In some it might, in some it won't. I'd rather having it twice sometimes, than none in those cases.

I find these messages already too long and they are quite common. Also I will need to update documentation here. Looking at the current logging, it seems like this statement will sometimes triple the url. Could you please either remove it from the pinger or from here to make sure it's logged at least once but no more than twice?

soltysh · 2017-06-01T11:59:42Z

Rebased and addressed comments. @miminar ptal once again

miminar · 2017-06-05T10:06:19Z

pkg/image/prune/prune.go


 		if pod.Status.Phase != kapi.PodRunning && pod.Status.Phase != kapi.PodPending {
 			age := metav1.Now().Sub(pod.CreationTimestamp.Time)
 			if age >= algorithm.keepYoungerThan {
-				glog.V(4).Infof("Pod %s/%s is not running or pending and age is at least minimum pruning age - skipping", pod.Namespace, pod.Name)
-				// not pending or running, age is at least minimum pruning age, skip
+				glog.V(4).Infof("Pod %s is not running nor pending and age exceeds keepYoungerThan (%v) - skipping", getName(pod), age)


s/age exceeds/age is within/

The name of the variable is misleading 😄.

Ha, I was struggling with that log message for a while. Your suggestion is definitely the best!

Actually, no, what I said above is wrong. It has to say exceeds, because we're referring to pods that are not added to graph (skipped), which will allow the images to be pruned (because of lack of strong references).

miminar · 2017-06-05T10:31:06Z

Cross-linking the future documentation PR openshift/openshift-docs#4471.

openshift-bot · 2017-06-05T11:21:38Z

Evaluated for origin test up to 7edd2ba

miminar

LGTM, I'll do the followup for the deleter.

soltysh · 2017-06-05T11:27:21Z

[merge]

openshift-bot · 2017-06-05T13:05:25Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1927/) (Base Commit: 90592c5)

soltysh · 2017-06-05T13:21:26Z

Flake #13559
[merge]

openshift-bot · 2017-06-05T13:29:25Z

Evaluated for origin merge up to 7edd2ba

openshift-bot · 2017-06-08T05:53:42Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/923/) (Base Commit: cf5c12f) (Image: devenv-rhel7_6328)

Automatic merge from submit-queue Image pruner: Determine protocol just once 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

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 reviewed May 30, 2017

View reviewed changes

soltysh force-pushed the bug1450291 branch from 4785403 to 30f12d7 Compare June 1, 2017 11:59

soltysh assigned miminar Jun 1, 2017

miminar reviewed Jun 5, 2017

View reviewed changes

miminar mentioned this pull request Jun 5, 2017

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

Merged

Bug 1450291 - Improve logs in image pruning

7edd2ba

soltysh force-pushed the bug1450291 branch from 30f12d7 to 7edd2ba Compare June 5, 2017 11:17

miminar approved these changes Jun 5, 2017

View reviewed changes

openshift-bot merged commit 08cc91a into openshift:master Jun 8, 2017

soltysh deleted the bug1450291 branch June 8, 2017 12:09

miminar mentioned this pull request Jun 27, 2017

Image pruner: Determine protocol just once #14914

Merged

miminar mentioned this pull request Sep 5, 2017

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1450291 - Improve logs in image pruning #14405

Bug 1450291 - Improve logs in image pruning #14405

soltysh commented May 30, 2017

soltysh commented May 30, 2017

mfojtik commented May 30, 2017

miminar May 30, 2017

soltysh Jun 1, 2017

soltysh Jun 1, 2017

miminar May 30, 2017

soltysh Jun 1, 2017

soltysh Jun 1, 2017

miminar Jun 5, 2017

miminar Jun 5, 2017

soltysh Jun 5, 2017

miminar May 30, 2017 •

edited

Loading

soltysh Jun 1, 2017

miminar Jun 5, 2017

soltysh commented Jun 1, 2017

miminar Jun 5, 2017

soltysh Jun 5, 2017

soltysh Jun 5, 2017

miminar commented Jun 5, 2017

openshift-bot commented Jun 5, 2017

miminar left a comment

soltysh commented Jun 5, 2017

openshift-bot commented Jun 5, 2017

soltysh commented Jun 5, 2017

openshift-bot commented Jun 5, 2017

openshift-bot commented Jun 8, 2017 •

edited

Loading

Bug 1450291 - Improve logs in image pruning #14405

Bug 1450291 - Improve logs in image pruning #14405

Conversation

soltysh commented May 30, 2017

soltysh commented May 30, 2017

mfojtik commented May 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

Choose a reason for hiding this comment

miminar May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miminar commented Jun 5, 2017

openshift-bot commented Jun 5, 2017

miminar left a comment

Choose a reason for hiding this comment

soltysh commented Jun 5, 2017

openshift-bot commented Jun 5, 2017

soltysh commented Jun 5, 2017

openshift-bot commented Jun 5, 2017

openshift-bot commented Jun 8, 2017 • edited Loading

miminar May 30, 2017 •

edited

Loading

openshift-bot commented Jun 8, 2017 •

edited

Loading