Skip to content

run with pidfile option#311

Merged
AkihiroSuda merged 2 commits into
containerd:masterfrom
developer-guy:master
Aug 18, 2021
Merged

run with pidfile option#311
AkihiroSuda merged 2 commits into
containerd:masterfrom
developer-guy:master

Conversation

@developer-guy
Copy link
Copy Markdown
Contributor

Signed-off-by: Batuhan Apaydın batuhan.apaydin@trendyol.com

I'm not so sure but It might fix the second option of issue #215.

Comment thread cmd/nerdctl/run.go Outdated
Comment thread cmd/nerdctl/run.go Outdated
@developer-guy developer-guy changed the title run with pid-file option run with pidfile option Aug 10, 2021
Comment thread README.md Outdated
@developer-guy
Copy link
Copy Markdown
Contributor Author

I think this time we are ready for the big moment @AkihiroSuda, this will be my first PR for the nerdctl and I'm so excited about this 🥳😋

@AkihiroSuda AkihiroSuda added this to the v0.12.0 milestone Aug 12, 2021
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this has to be implemented in pkg/ocihook to support restarting like podman:

$ podman run -d --name foo --restart=always --pidfile /tmp/foo.pid nginx:alpine
$ cat /tmp/foo.pid 
41021
$ kill -9 41021
$ cat /tmp/foo.pid
41100

@developer-guy
Copy link
Copy Markdown
Contributor Author

Sorry, this has to be implemented in pkg/ocihook to support restarting like podman:

$ podman run -d --name foo --restart=always --pidfile /tmp/foo.pid nginx:alpine
$ cat /tmp/foo.pid 
41021
$ kill -9 41021
$ cat /tmp/foo.pid
41100

I did something about OCI hook for supporting pidfile option @AkihiroSuda , but I'm not so sure again :(

Comment thread cmd/nerdctl/internal_oci_hook.go Outdated
Comment thread pkg/ocihook/ocihook.go Outdated
Comment thread cmd/nerdctl/run.go Outdated
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@developer-guy
Copy link
Copy Markdown
Contributor Author

any updates @AkihiroSuda 🤝

Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and pushed a small fix

Sorry for my recent delay in reviewing PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants