Close with error #236

erikh · 2017-02-06T07:16:26Z

This patch augments the interfaces' close methods with an error return which is
only actually useful in one or two spots for now. However, it can be utilized
later as things fill in.

Fixes #82

mtrmac

Thanks!

skopeo should be similarly updated to handle the errors.

I am still not sold on this being truly worthwhile (all this only to report a failure in daemonImageSource when we fail to remove a temporary file which will be very likely cleaned up by systemd-tmpfiles in the worst case?), but if we are doing this, let’s actually do it right.

mtrmac · 2017-02-06T15:31:42Z

docker/daemon/daemon_dest.go

+	var err error
 	if !d.committed {
 		logrus.Debugf("docker-daemon: Closing tar stream to abort loading")
+		err = errors.New("Aborting upload, daemonImageDestination closed without a previous .Commit()")


This is not an error in .Close(); if the caller decides to abort without a .Commit(), and the upload is successfully aborted, .Close() should indicate success.

This is still outstanding.

mtrmac · 2017-02-06T15:37:20Z

types/types.go

 	Reference() ImageReference
 	// Close removes resources associated with an initialized ImageDestination, if any.
-	Close()
+	Close() error


The caller in copy.Image should be updated to handle the error.

mtrmac · 2017-02-06T15:40:01Z

types/types.go

 	Reference() ImageReference
 	// Close removes resources associated with an initialized UnparsedImage, if any.
-	Close()
+	Close() error


The callers in copy.Image (both for UnparseImage and Image) should be updated to handle the error.

And perhaps doc.go as well?

mtrmac · 2017-02-06T15:43:28Z

To link the two, this fixes #82.

erikh · 2017-02-06T18:31:55Z

Will make adjustments, and file a PR on skopeo in the next few days.

erikh · 2017-02-09T02:59:44Z

Ok I updated this to handle close errors, I also changed the return convention (read the comments at the inner-top of copy.Image) to use output variables. All is explained in comments, but if this feels too cumbersome for the benefits we can talk about alternatives.

mtrmac · 2017-02-10T16:21:45Z

copy/copy.go

+// source image admissibility.
+func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageReference, options *Options) (retErr error) {
+	// XXX this function uses an output parameter for the error return value.
+	// Setting this and returning is the ideal way to return an error.


Looking at https://golang.org/ref/spec#Return_statements , even with a named return value it would be perfectly fine to just return somerrorvalue AFAICS.

(Also, doesn’t XXX mean “this is bad”? Perhaps just NOTE: or something like that.)

Sure. I typically use it to say "WARNING" or something of the like: "pay attention" basically.

mtrmac · 2017-02-10T16:22:35Z

copy/copy.go

-	defer dest.Close()
+	defer func() {
+		if err := dest.Close(); err != nil {
+			retErr = errors.Wrapf(retErr, "dest: %v", err)


This should only happen if retErr == nil; the original value of retErr was the primary cause we are failing; that dest.Close() has failed may be completely unrelated, or it may be a side effect of the original retErr failure.

ok sure, I was just thinking that maybe it'd be nice to have all the error context in the event of a copy failure.

Oh, sorry, I wasn’t paying enough attention to notice that this preserves both. Let me check…

… something like this would be nice if we can make that human-readable. See how the other errors.Wrapf calls in here try to tell the user what is going on. I am not sure that this is workable here:

errors.Wrapf(retErr, "Error closing destination: %v, after primary failure", err)

is bordering on incomprehensible, especially if err were long / created by several errors.Wrapf calls. (We can make the underlying data type situation better by using something like aggregateErr from openshift-copies.go, but that is probably even worse for readability.)

If we can’t make that human-readable, I think preserving a non-nil retErr is more important than the failure from .Close.

@runcom ?

Sure, I get the concern.

What if I did something like this:

errors.Wrapf(retErr, "(destination error %v) ", err)

That way each indepednent error would be wrapped by parens. I could also add newlines.

I think the errors will be valuable context in the face of other, higher level errors.

That’s certainly better than my attempt.

The code as is (making daemonImageSource.Close fail using an invalid path):

$ sudo ./x docker-daemon:busybox:latest dir:d FATA[0000] Error copying: source: remove makethisfail//var/tmp/docker-daemon-tar266436759: no such file or directory: Error writing blob: open d/dir-put-blob135662858: no such file or directory

quite misleading about the cause/effect relationships

“primary error” wording:

FATA[0000] Error copying: Error closing source: remove makethisfail//var/tmp/docker-daemon-tar088311212: no such file or directory, after primary failure: Error writing blob: open d/dir-put-blob812853531: no such file or directory

Still focusing on the wrong thing (closing failure)

With the parentheses:

FATA[0000] Error copying: (source error remove makethisfail//var/tmp/docker-daemon-tar460112488: no such file or directory): Error writing blob: open d/dir-put-blob983238823: no such file or directory

Certainly better. Good enough?

Newlines are attractive (they would even allow us to build a bulleted list :) ) but risky, the caller is fairly likely not expecting them.

At this point I am really not sure, I’ll let @runcom weigh in.

mtrmac · 2017-02-10T16:22:59Z

copy/copy.go

 		if unparsedImage != nil {
-			unparsedImage.Close()
+			if err := unparsedImage.Close(); err != nil {
+				retErr = errors.Wrapf(retErr, "unparsed: %v", err)


mtrmac · 2017-02-10T16:23:15Z

copy/copy.go

-	defer src.Close()
+	defer func() {
+		if err := src.Close(); err != nil {
+			retErr = errors.Wrapf(retErr, "source: %v", err)


mtrmac · 2017-02-10T16:23:43Z

docker/daemon/daemon_dest.go

+	var err error
 	if !d.committed {
 		logrus.Debugf("docker-daemon: Closing tar stream to abort loading")
+		err = errors.New("Aborting upload, daemonImageDestination closed without a previous .Commit()")


This is still outstanding.

cyphar · 2017-02-20T17:12:07Z

copy/copy.go

 	if err != nil {
-		return errors.Wrapf(err, "Error initializing destination %s", transports.ImageName(destRef))
+		retErr = errors.Wrapf(err, "Error initializing destination %s", transports.ImageName(destRef))
+		return


This change is not necessary. return X will set retErr = x implicitly.

erikh · 2017-02-20T17:27:23Z

PTAL

changes:

using parens for inner errors now
retErr code was adjusted based on return semantics.

erikh · 2017-02-21T12:16:33Z

#240 is blocking this.

mtrmac · 2017-02-21T19:06:33Z

LGTM pending tests. (Shouldn't test-skopeo work even after the API change? If not, the way we deal with API interdependencies is usually to

Temporarily modify the corresponding skopeo PR to vendor from the proposed branch, show that that passes tests
Then merge the containers/image PR, ignoring the test failure
Update the skopeo PR to vendor from the updated containers/image master again, and merge it.

mtrmac · 2017-02-21T19:06:42Z

👍

erikh · 2017-02-25T10:44:17Z

rebased

runcom · 2017-02-25T11:31:49Z

@erikh sorry needs one last rebase before I merge this

This allows us to provide in the image interfaces a method of providing an error at close time. This is only currently used in a few situations. Signed-off-by: Erik Hollensbe <github@hollensbe.org>

erikh · 2017-02-25T12:02:24Z

done

…

On Sat, Feb 25, 2017 at 3:31 AM, Antonio Murdaca ***@***.***> wrote: @erikh <https://github.com/erikh> sorry needs one last rebase before I merge this — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#236 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6-qzkfc8JfWeurltFgnwDkS-3Iyyks5rgBElgaJpZM4L32DS> .

runcom · 2017-02-25T12:09:24Z

👍

mtrmac reviewed Feb 6, 2017

View reviewed changes

erikh force-pushed the close-with-error branch 2 times, most recently from 484c771 to 2682f2f Compare February 9, 2017 02:58

erikh force-pushed the close-with-error branch from 2682f2f to 5bcdf40 Compare February 9, 2017 03:01

mtrmac reviewed Feb 10, 2017

View reviewed changes

mtrmac requested changes Feb 10, 2017

View reviewed changes

cyphar reviewed Feb 20, 2017

View reviewed changes

erikh force-pushed the close-with-error branch from 5bcdf40 to 33ad61a Compare February 20, 2017 17:26

erikh mentioned this pull request Feb 20, 2017

cmd/skopeo: check errors on close functions for image handling. containers/skopeo#309

Merged

erikh force-pushed the close-with-error branch 2 times, most recently from 1c797ef to eca0900 Compare February 21, 2017 12:14

mtrmac approved these changes Feb 21, 2017

View reviewed changes

erikh force-pushed the close-with-error branch from eca0900 to 9d270bc Compare February 25, 2017 10:44

Close with error in all interfaces

cdb838d

This allows us to provide in the image interfaces a method of providing an error at close time. This is only currently used in a few situations. Signed-off-by: Erik Hollensbe <github@hollensbe.org>

erikh force-pushed the close-with-error branch from 9d270bc to cdb838d Compare February 25, 2017 12:02

runcom merged commit 3d6f4f6 into containers:master Feb 25, 2017

mtrmac mentioned this pull request Aug 26, 2017

Add oci-archive transport that creates a tar archive of an image #314

Merged

Close with error #236

Close with error #236

Uh oh!

Conversation

erikh commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Feb 6, 2017

Uh oh!

erikh commented Feb 6, 2017

Uh oh!

erikh commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikh commented Feb 20, 2017

Uh oh!

erikh commented Feb 21, 2017

Uh oh!

mtrmac commented Feb 21, 2017 • edited by runcom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Feb 21, 2017

Uh oh!

erikh commented Feb 25, 2017

Uh oh!

runcom commented Feb 25, 2017

Uh oh!

erikh commented Feb 25, 2017 via email

Uh oh!

runcom commented Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erikh commented Feb 6, 2017 •

edited

Loading

mtrmac commented Feb 21, 2017 •

edited by runcom

Loading

runcom commented Feb 25, 2017 •

edited

Loading