Add fixes to newImageDestination to allow kpod save to write to a file when stdout is redirected #313

umohnani8 · 2017-07-20T20:01:11Z

Need this fix for kpod load so that stdout can write to a file when redirected. Checks if the file exists and is empty first as the file is created then written to.

Signed-off-by: umohnani8 umohnani@redhat.com

rhatdan · 2017-07-20T20:05:17Z

This patch fixes the issue where a user executes
kpod save > fedora.tar
In this situation the bash command creates an empty file, which the image layer should not reject.

rhatdan · 2017-07-20T20:08:15Z

docker/archive/dest.go

+	} else {
+		fhStat, err := fh.Stat()
+		if err != nil {
+			return nil, err


Probably should wrap this err with some more useful message.
errors.Wrapf(err, "docker-archive file %v can not be stat'd", ref.path)

mtrmac · 2017-07-20T20:33:24Z

docker/archive/dest.go

+	// tries to open a file first to accomodate for load when using stdout
+	// as the file is created first then written to
+	// if the file doesn't exist then it creates a new file for exclusive use
+	// if the file exists, it checks to ensure the size is 0


“to accomodate for load” is difficult to understand for me as a non-native speaker.

(In general, don’t write comments describing what every line does; that should be obvious from the code, and such comments are over time more likely to become obsolete and misleading than to be helpful; instead, write comments saying what is the code trying to do, most importantly the situation/problem (“Note that this needs to account for $this_weird_case”), and, if non-obvious, the solution.)

Yes lets remove the comment about load, and just state that the file needs to be created new and open for write, if the file prexisted it needs to be null for now. @mtrmac @umohnani8 is going to work on the append to the tar ball soon, but we need to get this in now.

append to the tar ball soon

You mean to update an archive by adding new files to the end of an existing tarball? That works with the “tar as tape processed at once” model (and seems to be documented at https://www.gnu.org/software/tar/manual/html_node/multiple.html ), but it's not obviously interoperable (in particular, the reader in this package stops at the first match). Of course that particular reader can be updated, but in general?

Ultimately, what is the use case here? Why would anyone want to keep updating a set of images in a giant single file with O(N) seeking and no way to delete a layer in the middle, as opposed to using either a registry or the OCI expanded dirs+files format?

We are trying to match the docker save command, their use case is to put multiple versions of the same image/tags into a tar ball at the same time.

fedora:rawhide
fedora:latest
fedora:f26
fedora:f25

If we decide this is a bad idea, we can drop support for it. Currently kpod load is able to read the images created with multiple tags, not sure if it works correctly or not.

docker save --help Usage: docker save [OPTIONS] IMAGE [IMAGE...] Save one or more images to a tar archive (streamed to STDOUT by default) Options: --help Print usage -o, --output string Write to a file, instead of STDOUT

It is curious that the documentation for docker load seems to indicate one image.

docker load --help Usage: docker load [OPTIONS] Load an image from a tar archive or STDIN Options: --help Print usage -i, --input string Read from tar archive file, instead of STDIN -q, --quiet Suppress the load output

Right, I meant it works for multiple tags of the same image. So if we save
alpine:2.6
alpine:2.7
into the same docker archive and load that using kpod, it will load both alpine:2.6 and alpine:2.7

The error you are getting above is because the loadTarManifest was throwing an error if it received more than one object. I changed that in containers/image to not throw an error if there is more than one so that kpod load can load all the tags in one archive.
This was the pull request for that change that got accepted and merged #309

(To be precise, what matters is that the image (config+layers) is the same. The names may differ, both in the tag and in the repo (busybox:latest and example.com/personalproject/myspecialcopy:today), as long as they point to the same contents.)

#309 only allows listing the various tags; but actually copying the images out still fails with the quoted error (see also the warning above; the code does not allow choosing which image to read). (Just clarifying the current implementation state. The “load” path is fixable easily enough—we insist on seekable input, so it’s quite possible to choose one of the images; it’s the “save” part that is problematic.)

I guess short-term the easiest way to get the “save multiple” case working would be to create a temporary directory, copy all of the images in there one at a time using the oci: transport, and then tar it up. That bypasses the need to edit tar archives. Downsides would be:

Extra code happening outside of containers/image, arguably working around its inflexible model

OCI does not currently support multiple images either ( oci transport will remove existing images #306 ), though that should be trivially fixable

I guess short-term the easiest way to get the “save multiple” case working

… for the OCI format, not for the docker-compatible tar one; but, if anything, we actually need the docker-compatible more, to preserve compatibility with existing workflows.

Okay, so after I changed loadTarManifest (#309) I was able to get the list of tags and it pulled all the tags in. It is possible that it pulled the same image multiple times and named it differently as the tags are different - basically loading multiple images does not fail for me.
Right, getting the docker one working is more important to match their functionality.

I say if an image has multiple tags we should saver the tags, but not do anything with multiple images.

mtrmac · 2017-07-20T20:40:19Z

docker/archive/dest.go

-		return nil, err
-	}

+		if fhStat.Size() > 0 {


Awkwardly, AFAICT it is not actually documented anywhere that struct stat.st_size has any particular value for pipes and FIFOs. I guess this could explicitly check for (fhStat.Mode()&ModeType) == ModeNamedPipe || (fhStat.Mode().IsRegular() && fhStat.Size() == 0) or something like that.

mtrmac · 2017-07-20T20:46:49Z

docker/archive/dest.go

-		//        it and then do a rename(2).
-		if os.IsExist(err) {
-			err = errors.New("docker-archive doesn't support modifying existing images")
+		fh, err = os.OpenFile(ref.path, os.O_WRONLY|os.O_EXCL|os.O_CREATE, 0644)


I don’t think using O_EXCL with the same path again is all that valuable; either err was an unrelated failure and this should fail again in the same way, or we already know that a file did not exist, so if it exists now and O_EXCL causes us to fail, we are racing against something else modifying the filesystem and failing is not necessarily the right thing to do.

Couldn’t this be simplified to a single os.OpenFile(…O_WRONLY | O_CREATE) call followed by the “size of the file (whether pre-existing or newly created) is 0” check? (That’s also racy against concurrent writes to the same filename, though differently, but at least it is trivially simple.)

Yup, sounds like a better idea. Will do that.

rhatdan · 2017-07-22T09:52:27Z

docker/archive/dest.go

-			err = errors.New("docker-archive doesn't support modifying existing images")
-		}
-		return nil, err
+		return nil, errors.Wrapf(err, "error retrieving stat of file")


What file?

+ return nil, errors.Wrapf(err, "error statting file %q", ref.path)

rhatdan · 2017-07-24T13:10:31Z

@mtrmac PTAL, I think we are in agreement, and this is needed to complete kpod save.

mtrmac · 2017-07-24T14:38:42Z

docker/archive/dest.go

-	fh, err := os.OpenFile(ref.path, os.O_WRONLY|os.O_EXCL|os.O_CREATE, 0644)
+
+	// the file needs to be created for write only
+	// if the file prexisted it needs to be empty


This comment still says “what” but not “why”. Perhaps something like

ref.path may be either a regular file or a pipe. In the pipe case we must be able to open a pre-existing file.

OTOH for regular files we don’t want to overwrite a pre-existing file, if there is any,
so we check for Size() == 0 below. (This is racy, but using O_EXCL would also be racy,
only in a different way. Either way, it’s up to the user not to have two writers to the same path.)

Changed the comment to this.

mtrmac · 2017-07-24T14:50:27Z

docker/archive/dest.go

+		return nil, errors.Wrapf(err, "error statting file %q", ref.path)
+	}
+
+	if fhStat.Mode()&os.ModeNamedPipe != 0 || (fhStat.Mode().IsRegular() && fhStat.Size() == 0) {


Please use (fhStat.Mode() & os.ModeType) == os.ModeNamedPipe. The code above is correct (unlike UNIX, os.FileMode uses individual bits for the various non-regular-file file types), but unexpectedly tricky because the negation of the condition is not … == 0 (that would also include regular files).

(Also, as a matter of personal? style, if (the error case) { return error } /* normal processing */ would be more idiomatic.)

Fixed and negated the if to return the error instead.

umohnani8 · 2017-07-25T19:18:35Z

@rhatdan PTAL

rhatdan · 2017-07-25T19:39:26Z

docker/archive/dest.go

-	fh, err := os.OpenFile(ref.path, os.O_WRONLY|os.O_EXCL|os.O_CREATE, 0644)
+
+	// ref.path can be either a pipe or a regular file
+	// in the case of a pipe, we must be able to open a pre-existing file


// in the case of a pipe, we must be able to open a pre-existing file
This line is wrong.
//in the case of a pipe, we require that we can open it for write

rhatdan · 2017-07-25T19:47:02Z

LGTM
@mtrmac PTAL

mtrmac · 2017-07-26T13:45:25Z

docker/archive/dest.go

+		return nil, errors.Wrapf(err, "error statting file %q", ref.path)
+	}
+
+	if fhStat.Mode()&os.ModeType != os.ModeNamedPipe && (fhStat.Mode().IsRegular() && fhStat.Size() != 0) {


This is not actually a mathematical negation of the original, and fhStat.Mode().IsRegular() always implies fhStat.Mode()&os.ModeType != os.ModeNamedPipe.

We could either just ignore other file types, and say

if fhStat.Mode().IsRegular() && fhStat.Size() != 0 { /* fail */ }

, or something like

if fhStat.Mode().IsRegular() && fhStat.Size() != 0 { /* fail as above */ } else if fhStat.Mode()&os.ModeType != os.ModeNamedPipe { return nil, fmt.Errorf("Unsupported file type with mode %x", fhstat.Mode()") }

Looking at the history of this PR, all we really wanted to check for is the non-zero size, so the shorter variant should be enough.

I guess I can live with the code as is, so 👍 in the interest of speeding things up. But I’d prefer this to be cleaned up—and we are waiting for @runcom anyway.

Okay, changed it to the if - else if method.

This fails tests.

The else if was causing it to fail. I just took it out so it is like the shorter version. Hope that is fine.

Sure. Thanks!

mtrmac · 2017-07-26T13:45:43Z

👍

…directed Signed-off-by: umohnani8 <umohnani@redhat.com>

rhatdan · 2017-07-26T17:46:05Z

@runcom PTAL

runcom · 2017-07-27T10:41:02Z

LGTM

umohnani8 mentioned this pull request Jul 20, 2017

Add 'kpod save' command cri-o/cri-o#678

Merged

rhatdan reviewed Jul 20, 2017

View reviewed changes

mtrmac reviewed Jul 20, 2017

View reviewed changes

umohnani8 force-pushed the master branch 2 times, most recently from f662f7e to 230921c Compare July 21, 2017 15:55

rhatdan reviewed Jul 22, 2017

View reviewed changes

umohnani8 force-pushed the master branch from 230921c to 2a1601f Compare July 24, 2017 13:05

mtrmac reviewed Jul 24, 2017

View reviewed changes

umohnani8 changed the title ~~Add fixes to newImageDestination to allow kpod load to write to a file when stdout is redirected~~ Add fixes to newImageDestination to allow kpod save to write to a file when stdout is redirected Jul 25, 2017

umohnani8 force-pushed the master branch from 2a1601f to 0c98118 Compare July 25, 2017 19:16

rhatdan reviewed Jul 25, 2017

View reviewed changes

umohnani8 force-pushed the master branch from 0c98118 to 1e2815d Compare July 25, 2017 19:43

mtrmac reviewed Jul 26, 2017

View reviewed changes

umohnani8 force-pushed the master branch from 1e2815d to f5ecd68 Compare July 26, 2017 13:51

Add fixes to newImageDestination to write to a file when stdout is re…

c2b06ca

…directed Signed-off-by: umohnani8 <umohnani@redhat.com>

umohnani8 force-pushed the master branch from f5ecd68 to c2b06ca Compare July 26, 2017 14:12

runcom merged commit 1066078 into containers:master Jul 27, 2017

mtrmac mentioned this pull request Mar 18, 2019

fix Bug 1688041 - podman image save removes existing image #604

Closed

Add fixes to newImageDestination to allow kpod save to write to a file when stdout is redirected #313

Add fixes to newImageDestination to allow kpod save to write to a file when stdout is redirected #313

Uh oh!

Conversation

umohnani8 commented Jul 20, 2017

Uh oh!

rhatdan commented Jul 20, 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!

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!

rhatdan commented Jul 24, 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!

umohnani8 commented Jul 25, 2017

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Jul 25, 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!

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

Uh oh!

Uh oh!

rhatdan commented Jul 26, 2017

Uh oh!

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

rhatdan Jul 25, 2017 •

edited

Loading

mtrmac commented Jul 26, 2017 •

edited by runcom

Loading

runcom commented Jul 27, 2017 •

edited

Loading