do not commit default volumes from container #699
Conversation
libpod/container_commit.go
Outdated
| @@ -24,6 +25,8 @@ type ContainerCommitOptions struct { | |||
| Changes []string | |||
| } | |||
|
|
|||
| var DefaultVolumeGroups = []string{"cgroup", "devpts", "mqueue", "proc", "shm", "sysfs", "tmpfs"} | |||
There was a problem hiding this comment.
This seems rather flimsy, and likely to break as new Default Volumes get added. Any way we can generate this list form the spec or from libcontainer?
There was a problem hiding this comment.
@rhatdan We should be able to get the originals if we add them in cmd/podman because we do store the original create CLI
baude
force-pushed
the
dontcommitdefvolumes
branch
from
0cb7273 to
c4bb0c7
Compare
cmd/podman/commit.go
Outdated
| @@ -5,6 +5,7 @@ import ( | |||
| "io" | |||
| "os" | |||
| "strings" | |||
| "encoding/json" | |||
|
LGTM |
baude
force-pushed
the
dontcommitdefvolumes
branch
2 times, most recently
from
119ab8e to
f586b80
Compare
cmd/podman/commit.go
Outdated
| artifact, err := ctr.GetArtifact("create-config") | ||
| if err == nil { | ||
| if err := json.Unmarshal(artifact, &createArtifact); err != nil { | ||
| return nil |
There was a problem hiding this comment.
any value to adding a log.debug to display more info on error?
There was a problem hiding this comment.
Probably a good idea to drop a warning here, yeah
There was a problem hiding this comment.
actually this is a hard failure ... adding return err
baude
force-pushed
the
dontcommitdefvolumes
branch
from
f586b80 to
0258f64
Compare
| @@ -120,7 +121,17 @@ func commitCmd(c *cli.Context) error { | |||
| Changes: c.StringSlice("change"), | |||
| Author: c.String("author"), | |||
| } | |||
| newImage, err := ctr.Commit(getContext(), reference, options) | |||
| var createArtifact createConfig | |||
| artifact, err := ctr.GetArtifact("create-config") | |||
There was a problem hiding this comment.
If error is not nil on this, maybe a logrus.Info saying we can't retrieve information on volumes, command, entrypoint?
There was a problem hiding this comment.
i prefer the hard fail... the user expects this to work, continuing without doing it should be a hard fail.
There was a problem hiding this comment.
Right now, we only unmarshal the artifact if it exists - if err is not nil on this line, we do nothing. This could happen with CRI-O containers, where we don't have enough info saved right now to reliably commit.
Maybe we should start storing all of this in the DB after all... I have a PR open to add some related things to the DB already, maybe it should be extended to also add the full volumes, entrypoint, command from when the container was created, so we can reliably handle things like CRI-O containers?
libpod/container_commit.go
Outdated
| @@ -74,11 +74,11 @@ func (c *Container) Commit(ctx context.Context, destImage string, options Contai | |||
| // add it to the resulting image. | |||
|
|
|||
| // Entrypoint - always set this first or cmd will get wiped out | |||
| importBuilder.SetEntrypoint(c.Spec().Process.Args) | |||
| importBuilder.SetEntrypoint(entryPoint) | |||
There was a problem hiding this comment.
Should we check if entrypoint and command are not "" before we set them? We do for mounts, so it seems like it would make sense
There was a problem hiding this comment.
i only do that because of the strings split.
There was a problem hiding this comment.
Probably safest to ignore them if they're "", I think they might be in some cases where the user didn't set one or the other
when performing a container commit, we should not add the default list of volumes for a container to the resulting image. it will cause the resulting image to crash when run subsequently. Signed-off-by: baude <[email protected]>
baude
force-pushed
the
dontcommitdefvolumes
branch
from
0258f64 to
f845757
Compare
|
bot, retest this please |
|
LGTM |
|
@mheon @umohnani8 PTAL |
|
LGTM. I have ideas for improving this, but they can come later. |
|
📌 Commit f845757 has been approved by |
|
LGTM |
|
@rh-atomic-bot retry |
|
☀️ Test successful - status-papr |
github-actions
bot
added
the
locked - please file new issue/PR
when performing a container commit, we should not add the default list of volumes
for a container to the resulting image. it will cause the resulting image to crash
when run subsequently.
Signed-off-by: baude [email protected]