Cleanup messages on podman load #2674

rhatdan · 2019-03-15T21:27:58Z

If user does not specify file or redirect for stdin, then
throw an error

Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2019-03-15T21:28:08Z

mheon · 2019-03-15T21:37:05Z

cmd/podman/load.go

@@ -81,7 +82,7 @@ func loadCmd(c *cliconfig.LoadValues) error {
 			defer os.Remove(outFile.Name())
 			defer outFile.Close()

-			inFile, err := os.OpenFile(input, 0, 0666)
+			inFile, err := os.OpenFile("/dev/stdin", 0, 0666)


Shouldn't this be c.Input instead of the unconditional "/dev/stdin"

No this is the case where the user did not specify a --input file, it will pull the image from stdin, create a temporary file and then send the temporary file to containers/storage.

TomSweeneyRedHat · 2019-03-15T21:39:19Z

cmd/podman/load.go

-
+	if input == "" {
+		if terminal.IsTerminal(int(os.Stdin.Fd())) {
+			return errors.Errorf("can not use terminal, redirect the archive or using the --input flag required")


I'd like @edsantiago to take a look at this line, the bit after the 'or' needs help. I'd suggest "or use the --input option as required"

Some suggestions:

Cannot read from terminal. Use command-line redirection or the --input flag. Will not read from terminal. Use the --input flag, or redirect from a file/pipe. No archive file specified; use --input to specify one.

I'm fine with Ed's suggestion except 'flag' s/b 'optoin'. If you pull up the man page, you'll find --input listed under "options" not "flags"

TomSweeneyRedHat · 2019-03-15T21:40:02Z

commands.md

@@ -34,7 +34,7 @@
 | [podman-info(1)](/docs/podman-info.1.md)                 | Display system information                                                |[![...](/docs/play.png)](https://asciinema.org/a/yKbi5fQ89y5TJ8e1RfJd4ivTD)|
 | [podman-inspect(1)](/docs/podman-inspect.1.md)           | Display the configuration of a container or image                         |[![...](/docs/play.png)](https://asciinema.org/a/133418)|
 | [podman-kill(1)](/docs/podman-kill.1.md)                 | Kill the main process in one or more running containers                   |[![...](/docs/play.png)](https://asciinema.org/a/3jNos0A5yzO4hChu7ddKkUPw7)|
-| [podman-load(1)](/docs/podman-load.1.md)                 | Load an image from docker archive or oci                                  |[![...](/docs/play.png)](https://asciinema.org/a/kp8kOaexEhEa20P1KLZ3L5X4g)|
+| [podman-load(1)](/docs/podman-load.1.md)                 | Load an image from container image archive                                |[![...](/docs/play.png)](https://asciinema.org/a/kp8kOaexEhEa20P1KLZ3L5X4g)|


s/from container/from a container/

TomSweeneyRedHat · 2019-03-15T21:40:30Z

docs/podman-load.1.md

@@ -1,22 +1,24 @@
 % podman-load(1)

 ## NAME
-podman\-load - Load an image from docker archive
+podman\-load - Load an image from container image archive into container storage


s/from container/from a container/

TomSweeneyRedHat · 2019-03-15T21:41:04Z

docs/podman-load.1.md


 ## DESCRIPTION
-**podman load** copies an image from either **docker-archive** or **oci-archive** stored
-on the local machine. **podman load** reads from stdin by default or a file if the **input** flag is set.
+**podman load** loads an image from either an **oci-archive** or **docker-archive** stored on the local machine into container storage. **podman load** reads from stdin by default or a file if the **input** flag is set.


s/flag is set/option is set/

TomSweeneyRedHat · 2019-03-15T21:41:29Z

docs/podman-save.1.md

@@ -1,14 +1,13 @@
 % podman-save(1)

 ## NAME
-podman\-save - Save an image to docker-archive or oci-archive
+podman\-save - Save an image to container archive


s/to container/to a container/

rh-atomic-bot · 2019-03-15T21:54:44Z

☔ The latest upstream changes (presumably #2643) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2019-03-16T12:27:27Z

@edsantiago @TomSweeneyRedHat @vrothberg @giuseppe @mheon @baude PTAL

edsantiago

A few suggestions, and one showstopper:

$ podman  load <sdfsdfsdf                                                                                                                   pr/2674 (1)
panic: runtime error: slice bounds out of range

goroutine 1 [running]:
github.com/containers/libpod/libpod/image.(*Runtime).pullGoalFromImageReference(0xc0008a5c50, 0x1894d00, 0xc0000d0038, 0x18a1a60, 0xc00026cda0, 0xc0006f9b0c, 0x4, 0xc0001058c0, 0x0, 0x0, ...)
        /home/esm/go/src/github.com/containers/libpod/libpod/image/pull.go:188 +0xcf5
github.com/containers/libpod/libpod/image.(*Runtime).pullImageFromReference(0xc0008a5c50, 0x1894d00, 0xc0000d0038, 0x18a1a60, 0xc00026cda0, 0x1881a20, 0xc0000d4010, 0x0, 0x0, 0x0, ...)
        /home/esm/go/src/github.com/containers/libpod/libpod/image/pull.go:229 +0x1b4
github.com/containers/libpod/libpod/image.(*Runtime).LoadFromArchiveReference(0xc0008a5c50, 0x1894d00, 0xc0000d0038, 0x18a1a60, 0xc00026cda0, 0x0, 0x0, 0x1881a20, 0xc0000d4010, 0x0, ...)
        /home/esm/go/src/github.com/containers/libpod/libpod/image/image.go:182 +0x108
github.com/containers/libpod/libpod.(*Runtime).LoadImage(0xc0006f2620, 0x1894d00, 0xc0000d0038, 0x0, 0x0, 0x0, 0x0, 0x1881a20, 0xc0000d4010, 0x0, ...)
        /home/esm/go/src/github.com/containers/libpod/libpod/runtime_img.go:264 +0x323
github.com/containers/libpod/pkg/adapter.(*LocalRuntime).LoadImage(0xc0001ba1a0, 0x1894d00, 0xc0000d0038, 0x0, 0x0, 0x28318a0, 0x58b327, 0xc0007badc0, 0xc0006f60c0, 0x17)
        /home/esm/go/src/github.com/containers/libpod/pkg/adapter/runtime.go:329 +0xc4
main.loadCmd(0x28318a0, 0x0, 0x0)
        /home/esm/go/src/github.com/containers/libpod/cmd/podman/load.go:103 +0x165
main.glob..func32(0x2689d80, 0x28524a8, 0x0, 0x0, 0x0, 0x0)
        /home/esm/go/src/github.com/containers/libpod/cmd/podman/load.go:28 +0x7f
github.com/containers/libpod/vendor/github.com/spf13/cobra.(*Command).execute(0x2689d80, 0xc0000da010, 0x0, 0x0, 0x2689d80, 0xc0000da010)
        /home/esm/go/src/github.com/containers/libpod/vendor/github.com/spf13/cobra/command.go:762 +0x473
github.com/containers/libpod/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0x268e4c0, 0xc, 0x2854040, 0xc000774000)
        /home/esm/go/src/github.com/containers/libpod/vendor/github.com/spf13/cobra/command.go:852 +0x2fd
github.com/containers/libpod/vendor/github.com/spf13/cobra.(*Command).Execute(0x268e4c0, 0xc000056420, 0xc0000bc058)
        /home/esm/go/src/github.com/containers/libpod/vendor/github.com/spf13/cobra/command.go:800 +0x2b
main.main()
        /home/esm/go/src/github.com/containers/libpod/cmd/podman/main.go:231 +0x3c

I don't have time right now to diagnose and track down the cause, sorry. I suspect it will be much easier for you to track down than for me, anyway.

(FWIW the sdfsdf archive loads fine using the -i flag)

edsantiago · 2019-03-16T13:48:11Z

cmd/podman/load.go

 	_loadCommand    = &cobra.Command{
-		Use:   "load [flags] [PATH]",
-		Short: "Load an image from docker archive",
+		Use:   "load [flags] [*name*[:*tag*]]",


For consistency with other Use lines, could you make this [NAME[:TAG]] (caps)?

edsantiago · 2019-03-16T13:53:54Z

cmd/podman/load.go

 )

 var (
 	loadCommand cliconfig.LoadValues

-	loadDescription = "Loads the image from docker-archive stored on the local machine."
+	loadDescription = "Loads an image from container image archive stored on the local machine into container storage."


This wording makes my brain hurt a bit. What would you think of any of these?

Loads a pre-saved image from a local file into container storage. Loads an image from a locally stored archive (tar file) into container storage. Loads a local archive file, containing a saved image, into container storage.

edsantiago · 2019-03-16T13:57:25Z

cmd/podman/load.go

-
+	if input == "" {
+		if terminal.IsTerminal(int(os.Stdin.Fd())) {
+			return errors.Errorf("can not use terminal, redirect the archive or using the --input flag required")


Some suggestions:

Cannot read from terminal. Use command-line redirection or the --input flag. Will not read from terminal. Use the --input flag, or redirect from a file/pipe. No archive file specified; use --input to specify one.

edsantiago · 2019-03-16T14:00:47Z

cmd/podman/load.go

-		input = "/dev/stdin"
-		c.Input = input
-
+	if input == "" {


So, the reason I did len(input) == 0 is because of that usage three lines up. For the sake of consistency, and minimizing maintenance programmer confusion (like, someone some day wondering "why is it this way in one place and the other way in the other?"), could you consistentize those two?

Actually, I think it might be even more readable as

if (whichever way you choose to check if string is empty) { if runtime.Remote { return errors.New() } ...

I actually do not know why the runtime.Remote check is here. We should be able to support this on a remote system?

Removing check.
@baude @jwhonce WDYT?

edsantiago · 2019-03-16T14:04:31Z

cmd/podman/load.go

@@ -92,7 +93,7 @@ func loadCmd(c *cliconfig.LoadValues) error {
 				return errors.Errorf("error copying file %v", err)
 			}

-			input = outFile.Name()
+			c.Input = outFile.Name()


The reason I set input is because of the parse.ValidateFileName() below. Will "" validate? I can't test right now because I'm seeing a major crash in this form of usage and am very low on time right now (will have more time this afternoon).

I removed input altogether, now I just if c.Input > 0, then validate, otherwise I generate it from a stdin.

edsantiago · 2019-03-16T14:07:07Z

docs/podman-load.1.md

-on the local machine. **podman load** reads from stdin by default or a file if the **input** flag is set.
-The **quiet** flag suppresses the output when set.
+**podman load** loads an image from either an **oci-archive** or **docker-archive** stored on the local machine into container storage. **podman load** reads from stdin by default or a file if the **input** option is set.
+You can also specify a name for the image if the archive does not contain a named reference.


I still find the name option confusing. It is always ignored AFAICT (probably because my saved archive includes a name). Would this make more sense?

If the archive does not include a name (FIXME: why would that be? Is that OCI vs Docker format?), you can specify a **name** and optional **tag**; these will be silently ignored if the archive already specifies them.

(And, in the spirit of not silently ignoring, would it be possible for the code to check for this condition, and say "I am completely ignoring the name:tag you gave me"?)

I don't know why this happens, and could not figure it out from the code. Who coded this up originally?

edsantiago · 2019-03-16T21:21:16Z

Update: so the catch is here:

        // checking if loading from pipe
        if !fi.Mode().IsRegular() {

This triggers in a pipe: cat file | podman load. It does not trigger on redirection, as in podman load < existing-file.

Although I realize this can look like duplication, I humbly suggest that one reasonable fix is to un-delete lines 68-69, the ones in which I set both input and c.Input to "/dev/stdin". I did that because both variables are referenced further down in the code, and I wanted to guarantee that they had been properly initialized.

I think if you accept my suggestion about moving the remote client code into the if, we can completely eliminate the redundant input variable and make the code less fragile.

rhatdan · 2019-03-18T12:29:12Z

@edsantiago Take a look again.

edsantiago

Passes everything I can try. LGTM aside from my request in the Use message; and I think @baude should confirm whether the -remote check should stay or go because it might require code changes in the client.

edsantiago · 2019-03-18T12:43:09Z

cmd/podman/load.go

+	loadDescription = "Loads an image from a locally stored archive (tar file) into container storage."
+
+	_loadCommand = &cobra.Command{
+		Use:   "load [flags] [*NAME*[:*TAG*]]",


Could you get rid of the asterisks? No other usage message includes them; that's a convention in .md files.

edsantiago · 2019-03-18T13:35:48Z

Well, I've run into a snag but it doesn't seem to be related to your code. First: podman save IID does not seem to save a name in the archive; I don't know if that's expected or not (1.1.2 has the same behavior). Second, podman loadNAME:TAG <saved-file-by-iid does not actually do anything with NAME:TAG

$ iid=$(podman pull alpine:latest)
$ podman save $iid >| /tmp/alpine.tar
$ podman rmi -a
5cb3aa00f89934411ffba5c063a9bc98ace875d8f92e77d0029543d9f2ef4ad0
$ podman load alpine:latest </tmp/alpine.tar
Getting image source signatures
Copying blob bcf2f368fe23 [======================================] 5.5MiB / 5.5MiB
Copying config 5cb3aa00f8 [======================================] 1.5KiB / 1.5KiB
Writing manifest to image destination
Storing signatures
Loaded image(s): @5cb3aa00f89934411ffba5c063a9bc98ace875d8f92e77d0029543d9f2ef4ad0
$ podman images -a
REPOSITORY   TAG      IMAGE ID       CREATED       SIZE
<none>       <none>   5cb3aa00f899   10 days ago   5.79 MB

Again, podman 1.1.2 behaves the same way, I'm just wondering if that's the intention. The NAME:TAG argument still confuses me.

jwhonce

LGTM

jwhonce · 2019-03-18T16:06:18Z

cmd/podman/load.go


-		fi, err := os.Stdin.Stat()
+		inFile, err := os.OpenFile("/dev/stdin", 0, 0666)


For portability, can you read from os.Stdin rather than opening /dev/stdin?

Actually that simplifies the code. Thanks @jwhonce.
Fixed

If user does not specify file or redirect for stdin, then throw an error Signed-off-by: Daniel J Walsh <[email protected]>

openshift-ci-robot · 2019-03-18T17:32:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, jwhonce, rhatdan

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

edsantiago · 2019-03-18T17:35:40Z

/lgtm

New: - podman exec - podman load (requires containers#2674) - CLI parsing (regression test for containers#2574) Improved: - help: test "podman NoSuchCommand", and subcommands - help: test "podman cmd" without required args - pod: start with --infra=false; this allows running rootless - log: also run 'logs' after container is run - log: test -f with two containers Also, use helpful descriptions for skip_if_rootless Tested on f29, root and rootless. As soon as podman-remote supports rm, I'll start testing that too. Signed-off-by: Ed Santiago <[email protected]>

openshift-ci-robot requested review from mrunalp and umohnani8 March 15, 2019 21:28

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M labels Mar 15, 2019

mheon reviewed Mar 15, 2019

View reviewed changes

TomSweeneyRedHat reviewed Mar 15, 2019

View reviewed changes

rhatdan force-pushed the load branch 2 times, most recently from 41f4bd9 to c0459d8 Compare March 15, 2019 21:45

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2019

mheon added the Release Notes 1.2.0 label Mar 16, 2019

rhatdan force-pushed the load branch from c0459d8 to 6de3094 Compare March 16, 2019 10:48

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2019

edsantiago suggested changes Mar 16, 2019

View reviewed changes

edsantiago mentioned this pull request Mar 18, 2019

podman load: confusing command-line usability #2667

Closed

rhatdan force-pushed the load branch 2 times, most recently from 5c324cf to 155554c Compare March 18, 2019 12:26

edsantiago suggested changes Mar 18, 2019

View reviewed changes

rhatdan force-pushed the load branch from 155554c to b310607 Compare March 18, 2019 13:19

rhatdan force-pushed the load branch from b310607 to 785a776 Compare March 18, 2019 14:36

openshift-ci-robot removed the size/M label Mar 18, 2019

openshift-ci-robot added the size/L label Mar 18, 2019

jwhonce approved these changes Mar 18, 2019

View reviewed changes

Cleanup messages on podman load

d0ee203

If user does not specify file or redirect for stdin, then throw an error Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the load branch from 785a776 to d0ee203 Compare March 18, 2019 16:23

openshift-ci-robot added size/M and removed size/L labels Mar 18, 2019

edsantiago approved these changes Mar 18, 2019

View reviewed changes

openshift-ci-robot assigned edsantiago Mar 18, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2019

openshift-merge-robot merged commit 9289ecd into containers:master Mar 18, 2019

edsantiago mentioned this pull request Mar 18, 2019

BATS: new tests, and improvements to existing ones #2703

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023


		fi, err := os.Stdin.Stat()
		inFile, err := os.OpenFile("/dev/stdin", 0, 0666)

Cleanup messages on podman load #2674

Cleanup messages on podman load #2674

Conversation

rhatdan commented Mar 15, 2019

rhatdan commented Mar 15, 2019

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

Choose a reason for hiding this comment

rh-atomic-bot commented Mar 15, 2019

rhatdan commented Mar 16, 2019

edsantiago 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

Choose a reason for hiding this comment

edsantiago commented Mar 16, 2019

rhatdan commented Mar 18, 2019

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Mar 18, 2019

jwhonce 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

openshift-ci-robot commented Mar 18, 2019

edsantiago commented Mar 18, 2019