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.

Sign up for GitHub

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

Jump to bottom

generate systemd: do not set KillMode #8889

Merged
merged 1 commit into from
Jan 5, 2021
Merged

generate systemd: do not set KillMode #8889

merged 1 commit into from
Jan 5, 2021

Conversation

vrothberg
Copy link
Member

KillMode=none has been deprecated in systemd and is now throwing big
warnings when being used. Users have reported the issues upstream
(see #8615) and on the mailing list.

This deprecation was mainly motivated by an abusive use of third-party
vendors causing all kinds of undesired side-effects. For instance, busy
mounts that delay reboot.

After talking to the systemd team, we came up with the following plan:

Short term: we can use TimeoutStopSec and remove KillMode=none which
will default to cgroup.

Long term: we want to change the type to sdnotify. The plumbing for
Podman is done but we need it for conmon. Once sdnotify is working, we
can get rid of the pidfile handling etc. and let Podman handle it.
Michal Seklatar came up with a nice idea that Podman increase the time
out on demand. That's a much cleaner way than hard-coding the time out
in the unit as suggest in the short-term solution.

This change is executing the short-term plan and sets a minimum timeout
of 60 seconds. User-specified timeouts are added to that.

Fixes: #8615
Signed-off-by: Valentin Rothberg rothberg@redhat.com

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2021
@vrothberg
Copy link
Member Author

@giuseppe @msekletar PTAL

rhatdan
rhatdan approved these changes Jan 5, 2021
View reviewed changes
Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

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

LGTM

@vrothberg vrothberg changed the title generate systemd: do not set KillMode WIP - generate systemd: do not set KillMode Jan 5, 2021
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 5, 2021
@vrothberg
Copy link
Member Author

vrothberg commented Jan 5, 2021 •
edited
Loading

Marked as WIP as I want to perform some manual tests.

giuseppe
giuseppe approved these changes Jan 5, 2021
View reviewed changes
Copy link
Member

@giuseppe giuseppe 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
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan, 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:
  • OWNERS [giuseppe,rhatdan,vrothberg]

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

@vrothberg
generate systemd: do not set KillMode
219c69e 
`KillMode=none` has been deprecated in systemd and is now throwing big
warnings when being used.  Users have reported the issues upstream
(see containers#8615) and on the mailing list.

This deprecation was mainly motivated by an abusive use of third-party
vendors causing all kinds of undesired side-effects.  For instance, busy
mounts that delay reboot.

After talking to the systemd team, we came up with the following plan:

 **Short term**: we can use TimeoutStopSec and remove KillMode=none which
 will default to cgroup.

 **Long term**: we want to change the type to sdnotify. The plumbing for
 Podman is done but we need it for conmon. Once sdnotify is working, we
 can get rid of the pidfile handling etc. and let Podman handle it.
 Michal Seklatar came up with a nice idea that Podman increase the time
 out on demand. That's a much cleaner way than hard-coding the time out
 in the unit as suggest in the short-term solution.

This change is executing the short-term plan and sets a minimum timeout
of 60 seconds.  User-specified timeouts are added to that.

Fixes: containers#8615
Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@vrothberg vrothberg changed the title WIP - generate systemd: do not set KillMode generate systemd: do not set KillMode Jan 5, 2021
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 5, 2021
@vrothberg
Copy link
Member Author

@containers/podman-maintainers PTAL

@rhatdan
Copy link
Member

rhatdan commented Jan 5, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2021
@openshift-merge-robot openshift-merge-robot merged commit 1f59276 into containers:master Jan 5, 2021
sshnaidm
sshnaidm reviewed Oct 13, 2021
View reviewed changes
pkg/systemd/generate/containers_test.go
ExecStartPre=/bin/rm -f %t/jadda-jadda.pid %t/jadda-jadda.ctr-id
ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon -d --replace --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN "foo=arg \"with \" space"
ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 42
ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 10
Copy link
Member

Choose a reason for hiding this comment

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

If we have TimeoutStopSec now, what's the point of leaving ExecStop then?

Copy link
Member Author

Choose a reason for hiding this comment

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

We want Podman to take care of stopping and to clean up properly. TimeoutStopSec is the big hammer of systemd to nuke everything in the cgroup and may leave resources behind.

openstack-mirroring pushed a commit to openstack-archive/tripleo-ansible that referenced this pull request Oct 14, 2021
@bogdando @sshnaidm
Wrap stopping podman -t with systemd timeouts
badd5a3 
Currently with KillMode=none, if podman -t hangs, the command repeats
leaving the process and its cgroup around.
But if a stop/start command hangs, we should not start another one.

Instead time it out properly via TimeoutStopSec set to the 2x of
the timeout given to the managed podman action. Then if it expires,
kill its cgroup all the way (KillMode=control-group is a default)
before rerunning the same operation. Also note that using KilMode
process is not recommended by systemd man pages.

Increase the grace stop timeout defaults 10->42s to align it with:
containers/podman#8889

Closes-bug: #1945791
Change-Id: Iefe861f91cefe2a9cf773cae98b2440566ae8b5e
Signed-off-by: Bogdan Dobrelya <bdobreli@redhat.com>
openstack-mirroring pushed a commit to openstack/openstack that referenced this pull request Oct 14, 2021
@bogdando @openstack-gerrit
Update git submodules
e7257a2 
* Update tripleo-ansible from branch 'master'
  to badd5a3c10f0af9e1c69a9b88064e743a36d4f53
  - Wrap stopping podman -t with systemd timeouts
    
    Currently with KillMode=none, if podman -t hangs, the command repeats
    leaving the process and its cgroup around.
    But if a stop/start command hangs, we should not start another one.
    
    Instead time it out properly via TimeoutStopSec set to the 2x of
    the timeout given to the managed podman action. Then if it expires,
    kill its cgroup all the way (KillMode=control-group is a default)
    before rerunning the same operation. Also note that using KilMode
    process is not recommended by systemd man pages.
    
    Increase the grace stop timeout defaults 10->42s to align it with:
    containers/podman#8889
    
    Closes-bug: #1945791
    Change-Id: Iefe861f91cefe2a9cf773cae98b2440566ae8b5e
    Signed-off-by: Bogdan Dobrelya <bdobreli@redhat.com>
openstack-mirroring pushed a commit to openstack/openstack that referenced this pull request Oct 14, 2021
@sshnaidm @openstack-gerrit
Update git submodules
7b0066d 
* Update tripleo-ansible from branch 'master'
  to fdfc59cd1d1024d132e6046f3ba4746fed23c8c2
  - Remove execpost and align systemd timeouts
    
    Align systemd timeouts to podman[1].
    Remove stop command with ExecStopPost command, because it won't
    help much to run same command twice if it's stuck.
    Set default time for stop command to 50 if not defined otherwise.
    In case it's stuck, systemd will nuke everything (TimeoutStopSec=60)
    60 seconds = 50 seconds of trying to stop + 10 seconds for avoiding
    the race.
    
    [1] containers/podman#8889
    Change-Id: Ife90b6c45b7f4a3fd829b60f9aa785fc580a31b8
openstack-mirroring pushed a commit to openstack-archive/tripleo-ansible that referenced this pull request Oct 14, 2021
@sshnaidm
Remove execpost and align systemd timeouts
fdfc59c 
Align systemd timeouts to podman[1].
Remove stop command with ExecStopPost command, because it won't
help much to run same command twice if it's stuck.
Set default time for stop command to 50 if not defined otherwise.
In case it's stuck, systemd will nuke everything (TimeoutStopSec=60)
60 seconds = 50 seconds of trying to stop + 10 seconds for avoiding
the race.

[1] containers/podman#8889
Change-Id: Ife90b6c45b7f4a3fd829b60f9aa785fc580a31b8
openstack-mirroring pushed a commit to openstack-archive/tripleo-ansible that referenced this pull request Nov 4, 2021
@bogdando
Wrap stopping podman -t with systemd timeouts
f34dc54 
Currently with KillMode=none, if podman -t hangs, the command repeats
leaving the process and its cgroup around.
But if a stop/start command hangs, we should not start another one.

Instead time it out properly via TimeoutStopSec set to the 2x of
the timeout given to the managed podman action. Then if it expires,
kill its cgroup all the way (KillMode=control-group is a default)
before rerunning the same operation. Also note that using KilMode
process is not recommended by systemd man pages.

Increase the grace stop timeout defaults 10->42s to align it with:
containers/podman#8889

Closes-bug: #1945791
Change-Id: Iefe861f91cefe2a9cf773cae98b2440566ae8b5e
Signed-off-by: Bogdan Dobrelya <bdobreli@redhat.com>
(cherry picked from commit badd5a3)
openstack-mirroring pushed a commit to openstack-archive/tripleo-ansible that referenced this pull request Nov 4, 2021
@bogdando
Wrap stopping podman -t with systemd timeouts
3670032 
Currently with KillMode=none, if podman -t hangs, the command repeats
leaving the process and its cgroup around.
But if a stop/start command hangs, we should not start another one.

Instead time it out properly via TimeoutStopSec set to the 2x of
the timeout given to the managed podman action. Then if it expires,
kill its cgroup all the way (KillMode=control-group is a default)
before rerunning the same operation. Also note that using KilMode
process is not recommended by systemd man pages.

Increase the grace stop timeout defaults 10->42s to align it with:
containers/podman#8889

Closes-bug: #1945791
Change-Id: Iefe861f91cefe2a9cf773cae98b2440566ae8b5e
Signed-off-by: Bogdan Dobrelya <bdobreli@redhat.com>
(cherry picked from commit badd5a3)
openstack-mirroring pushed a commit to openstack-archive/tripleo-ansible that referenced this pull request Nov 4, 2021
@bogdando
Wrap stopping podman -t with systemd timeouts
a17d78b 
Currently with KillMode=none, if podman -t hangs, the command repeats
leaving the process and its cgroup around.
But if a stop/start command hangs, we should not start another one.

Instead time it out properly via TimeoutStopSec set to the 2x of
the timeout given to the managed podman action. Then if it expires,
kill its cgroup all the way (KillMode=control-group is a default)
before rerunning the same operation. Also note that using KilMode
process is not recommended by systemd man pages.

Increase the grace stop timeout defaults 10->42s to align it with:
containers/podman#8889

Closes-bug: #1945791
Change-Id: Iefe861f91cefe2a9cf773cae98b2440566ae8b5e
Signed-off-by: Bogdan Dobrelya <bdobreli@redhat.com>
(cherry picked from commit badd5a3)
openstack-mirroring pushed a commit to openstack-archive/paunch that referenced this pull request Nov 9, 2021
@bogdando
Wrap stopping podman -t with systemd timeouts
a783594 
Currently with KillMode=none, if podman -t hangs, the command repeats
leaving the process and its cgroup around.
But if a stop/start command hangs, we should not start another one.

Instead time it out properly via TimeoutStopSec set to the 2x of
the timeout given to the managed podman action. Then if it expires,
kill its cgroup all the way (KillMode=control-group is a default)
before rerunning the same operation. Also note that using KilMode
process is not recommended by systemd man pages.

Increase the grace stop timeout defaults 10->42s to align it with:
containers/podman#8889

Closes-bug: #1945791
Change-Id: Iefe861f91cefe2a9cf773cae98b2440566ae8b5e
Signed-off-by: Bogdan Dobrelya <bdobreli@redhat.com>
(cherry picked from commit badd5a3c10f0af9e1c69a9b88064e743a36d4f53)
openstack-mirroring pushed a commit to openstack-archive/tripleo-ansible that referenced this pull request May 10, 2022
@bogdando
Wrap stopping podman -t with systemd timeouts (squash)
8417892 
1) Currently with KillMode=none, if podman -t hangs, the command repeats
leaving the process and its cgroup around.
But if a stop/start command hangs, we should not start another one.

Instead time it out properly via TimeoutStopSec set to the 2x of
the timeout given to the managed podman action. Then if it expires,
kill its cgroup all the way (KillMode=control-group is a default)
before rerunning the same operation. Also note that using KilMode
process is not recommended by systemd man pages.

Increase the grace stop timeout defaults 10->42s to align it with:
containers/podman#8889

2) Make KillMode configurable for service units

Libvirt container is a special beast that needs custom
killmode value for its tripleo-manager service unit.
Add kill_mode for the container values that defaults to
control-group.

3) Fix TimeoutStopSec to integer convertion

Related: rhbz#2010135
Closes-bug: #1945791
Change-Id: Iefe861f91cefe2a9cf773cae98b2440566ae8b5e
Signed-off-by: Bogdan Dobrelya <bdobreli@redhat.com>
(cherry picked from commit badd5a3)
@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sshnaidm sshnaidm sshnaidm left review comments

@giuseppe giuseppe giuseppe approved these changes

@rhatdan rhatdan rhatdan approved these changes

Assignees

@rhatdan rhatdan

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
Milestone
No milestone
6 participants
@vrothberg @openshift-ci-robot @rhatdan @giuseppe @sshnaidm @openshift-merge-robot