Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

systemd generator: force run container detached if CreateCommand has no detach param #5439

Merged

Conversation

ttys3
Copy link
Contributor

@ttys3 ttys3 commented Mar 10, 2020

since we use systemd service Type=forking
when we generated systemd service file using the --new param like

 sudo podman generate systemd --name --new ctrName

we got ExecStart=/usr/bin/podman run --conmon-pidfile ...
if somehow the container's CreateCommand has no -d or --detach param,
podman will run the container in default attached mode.
so systemd will wait the podman command exit until failed with timeout error.

quick reproduce the problem:

sudo podman pull nginxdemos/hello
sudo podman create --name ngxdemo -p 8086:80 nginxdemos/hello
cd /etc/systemd/system/
sudo podman generate systemd --name --new --restart-policy on-failure -t 3 --files ngxdemo
sudo podman rm ngxdemo

sudo systemctl start container-ngxdemo.service
Job for container-ngxdemo.service failed because a timeout was exceeded.
See "systemctl status container-ngxdemo.service" and "journalctl -xe" for details.

how to avoid the problem:

solution 1: merge this PR -_- and everything goes fine

solution 2: warn user to remember add -d when create the container
and user need to do is just add the -d param to the create command:

sudo podman pull nginxdemos/hello
sudo podman create -d --name ngxdemo -p 8086:80 nginxdemos/hello
cd /etc/systemd/system/
sudo podman generate systemd --name --new --restart-policy on-failure -t 3 --files ngxdemo
sudo podman rm ngxdemo
sudo systemctl start container-ngxdemo.service
# everything goes fine

@ttys3 ttys3 changed the title systemd generator: for run container detached if CreateCommand has no detach param systemd generator: force run container detached if CreateCommand has no detach param Mar 10, 2020
@ttys3 ttys3 force-pushed the fixup-systemdgen-with-new-param branch from 48ba61f to 8c4bc85 Compare March 10, 2020 05:47
@ttys3
Copy link
Contributor Author

ttys3 commented Mar 10, 2020

/assign @mheon

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Unit tests are not happy (see go test -v github.com/containers/libpod/pkg/systemd/generate). Also please add a test to test/e2e/generate_systemd_test.go where we create a container without -d, then generate the output and look for --cgroups=no-conmon -d in the output.

Adding a sentence or two to docs/source/markdown/podman-generate-systemd.1.md would be great to explain why we enforce detaching.

pkg/systemd/generate/systemdgen.go Outdated Show resolved Hide resolved
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Also please explain the changes in the commit message.

@rhatdan
Copy link
Member

rhatdan commented Mar 10, 2020

Thanks @ttys3 Interesting change.

@mheon
Copy link
Member

mheon commented Mar 10, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, ttys3

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2020
@mheon
Copy link
Member

mheon commented Mar 10, 2020

LGTM once comments from @vrothberg are addressed

@ttys3 ttys3 force-pushed the fixup-systemdgen-with-new-param branch from 8c4bc85 to a5513ec Compare March 12, 2020 17:06
@ttys3 ttys3 force-pushed the fixup-systemdgen-with-new-param branch from a5513ec to 91f08f1 Compare March 12, 2020 17:11
@ttys3 ttys3 requested a review from vrothberg March 12, 2020 17:19
@ttys3 ttys3 force-pushed the fixup-systemdgen-with-new-param branch 2 times, most recently from be887a1 to 75a6adc Compare March 12, 2020 17:34
@vrothberg
Copy link
Member

e2e tests are not yet happy:

[+1126s] [Fail] Podman generate systemd [It] podman generate systemd --new with explicit detaching param in middle
[+1126s] /var/tmp/go/src/github.com/containers/libpod/test/e2e/generate_systemd_test.go:223

…etach param

the podman generated systemd service file has `Type=forking` service,
so the command after `ExecStart=` should not run in front.
if someone created a container and has the detach(`-d`) param missing
like this
```
podman create --name ngxdemo -P nginxdemos/hello
```
and generate the file with `--new` param:
```
podman generate systemd --name --new ngxdemo
```
because `podman run xxx` has no `-d` param,
so the container is not run in background and nerver exit.
and systemd will fail to start the service:
```
sudo systemctl start container-ngxdemo.service
Job for container-ngxdemo.service failed because a timeout was exceeded.
See "systemctl status container-ngxdemo.service" and "journalctl -xe" for details.
```

Signed-off-by: 荒野無燈 <[email protected]>
@ttys3 ttys3 force-pushed the fixup-systemdgen-with-new-param branch from 75a6adc to 194723f Compare March 14, 2020 13:54
@ttys3
Copy link
Contributor Author

ttys3 commented Mar 14, 2020

e2e tests are not yet happy:

[+1126s] [Fail] Podman generate systemd [It] podman generate systemd --new with explicit detaching param in middle
[+1126s] /var/tmp/go/src/github.com/containers/libpod/test/e2e/generate_systemd_test.go:223

fixed.
but I see another two tests failed (seems not related to my PR):

special_testing_rootless name:remote Failing after 21m — Task Summary
test fedora-30 name:remote Failing after 25m — Task Summary

@mheon
Copy link
Member

mheon commented Mar 14, 2020

F30 remote is a known flake.

Rootless might be a real but. Restarted to see if it was a flake, but wouldn't be surprised if it reoccurs.

@vrothberg
Copy link
Member

Another restart :)

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2020
@openshift-merge-robot openshift-merge-robot merged commit 412a114 into containers:master Mar 16, 2020
@edsantiago
Copy link
Member

This has broken CI. Is anyone working on a fix?

edsantiago added a commit to edsantiago/libpod that referenced this pull request Mar 16, 2020
PR collision with containers#5427

Solution: add 'default.target' to WantedBy

Signed-off-by: Ed Santiago <[email protected]>
@vrothberg
Copy link
Member

This has broken CI. Is anyone working on a fix?

Entirely my bad as we should have rebased this PR before merging. #5514 is fixing CI.

@github-actions 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 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants