Add --pidfile flag to podman start#28983
Conversation
nomarek
force-pushed
the
start-pidfile
branch
2 times, most recently
from
9c49a69 to
5ed9c08
Compare
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
Thanks! I have some comments.
| Unlike **podman create** and **podman run**, the path is not stored in the | ||
| container configuration and is not reported by `podman inspect`. |
There was a problem hiding this comment.
I would mention that in pidfile.md.
There was a problem hiding this comment.
Moved it into pidfile.md so it sits with the shared option.
| return fmt.Errorf("retrieving PID of container %q: %w", c.ID(), err) | ||
| } | ||
| if pid == 0 { | ||
| return fmt.Errorf("container %q is no longer running, cannot write pidfile", c.ID()) |
There was a problem hiding this comment.
| return fmt.Errorf("container %q is no longer running, cannot write pidfile", c.ID()) | |
| return fmt.Errorf("container %q exited before its PID could be written to the pidfile", c.ID()) |
|
|
||
| content, err := os.ReadFile(pidfile) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| filePID, err := strconv.Atoi(strings.Split(string(content), "\n")[0]) |
There was a problem hiding this comment.
CreateIDFile doesn't append a newline.
There was a problem hiding this comment.
Dropped the split, just parsing the bare PID now.
|
|
||
| It("podman start --pidfile with --attach fails", func() { | ||
| pidfile := filepath.Join(tempdir, "start-pidfile") | ||
| session := podmanTest.Podman([]string{"create", ALPINE, "top"}) |
There was a problem hiding this comment.
Please use PodmanExitCleanly.
There was a problem hiding this comment.
Switched the setup calls over to PodmanExitCleanly.
|
|
||
| @@option pidfile | ||
|
|
||
| Unlike **podman create** and **podman run**, the path is not stored in the |
There was a problem hiding this comment.
If this is the case, and I do a podman create --pidfile then a podman start pidfile, will podman inspect show the wrong pidfile value?
There was a problem hiding this comment.
start --pidfile doesn't touch the stored config, so inspect keeps showing the create-time path (or the default), not the one passed to start. So it's not a wrong value, it just won't reflect the start path. I spelled that out in pidfile.md now.
|
Please squash your commits once you are ready for merge. |
create and run already take --pidfile, but the path is fixed at create time and reused on every start. Toolbx wants a fresh path per start so it can read the PID of the container's main process and wait on it before running podman exec. Add the same flag to start. Once the container is running we read its PID with Container.PID() and write it to the requested path. It only works for a single container, is rejected together with --attach, and is hidden on the remote client, the same as create and run. Fixes: podman-container-tools#25849 Signed-off-by: Marek Nogacki <no.marek@gmail.com>
|
Squashed it down to a single commit. |
|
@Luap99 I seem to remember you being opposed to this approach at some point in the past - am I misremembering? |
|
see the linked issue #25849 I am not exactly a fan of this, if others see a strong need for this sure but looking at the code this is super racy which I would oppose. |
| pid, err := c.PID() | ||
| if err != nil { | ||
| return fmt.Errorf("retrieving PID of container %q: %w", c.ID(), err) | ||
| } | ||
| if pid == 0 { | ||
| return fmt.Errorf("container %q exited before its PID could be written to the pidfile", c.ID()) | ||
| } |
There was a problem hiding this comment.
Well that is to some extend just not good design, we should not expose such issues to user and you do not really prevent the other problem of the container could have been restarted in between meaning the pid is different from the command which actually started the container.
Because this is doing c.PID() means anything between the actual start and that anything can happen. The better design would be to really go in the internals and make the start code return the pid from the time when it was actually started while we gold the proper lock to avoid the concurrent modification situation.
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
5 participants
podman createandpodman runalready have--pidfile, but the path is fixed at create time and reused on every start. Toolbx wants a fresh path per start so it can read the PID of the container's main process and wait on it before runningpodman exec. This adds the same--pidfileflag topodman start.After the container starts, its PID is read with libpod's
Container.PID()and written to the given path. As oncreate/run, the flag only works for a single container, is rejected with--attach, and is hidden on the remote client.Tests: new e2e specs check that the PID written to the file matches
.State.Pid, both for a freshly started and for an already-running container, plus error cases for starting more than one container (by argument and by--filter) and for--attach.Checklist
git commit -s).Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?