generate systemd: use --cidfile again #11315
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 70801b3. It turns out that letting systemd handle stopping the container is not working as I thought it will. Conmon is receiving the stop/kill signals and may exit non-zero, which in turn lets the systemd service transition into the `failed` state. We need to get back to letting Podman stop the containers and do a partial revert of commit 9ac5267 which removed using --cidfile. Happening in a following commit. Signed-off-by: Valentin Rothberg <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg 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 |
openshift-ci
bot
added
the
approved
|
/lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
|
@vrothberg Worth a v3.3.1 to get this fix out into the wild quickly? |
Thanks for checking, Matt! I think we have a deadline next Monday for RHEL. Maybe we can bundle it together with other issues that may have piled up? |
|
Works for me. |
Commit 9ac5267 changed the type of the generated systemd units from `forking` to `notify`. It further stopped using `--cidfile` and instead intended systemd to take care of stopping the container, which turned out to be a bad idea. Systemd will send the stop/kill signals to conmon which in turn may exit non-zero, depending on the signal, and ultimately breaking container cleanup. Hence, we need to use --cidfile again and let podman stop and remove the container to make sure that everything's in order. Fixes: containers#11304 Signed-off-by: Valentin Rothberg <[email protected]>
|
Repushed. Had to fix a fart in the system test. |
|
/lgtm |
openshift-ci
bot
removed
the
do-not-merge/hold
|
Backport to 3.3: #11320 |
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit 9ac5267 changed the type of the generated systemd units from
forkingtonotify. It further stopped using--cidfileand insteadintended systemd to take care of stopping the container, which turned
out to be a bad idea.
Systemd will send the stop/kill signals to conmon which in turn may exit
non-zero, depending on the signal, and ultimately breaking container
cleanup.
Hence, we need to use --cidfile again and let podman stop and remove the
container to make sure that everything's in order.
Fixes: #11304
Signed-off-by: Valentin Rothberg [email protected]
Note that the first commit reverts commit 70801b3.
@giuseppe @Luap99 @mheon @containers/podman-maintainers PTAL