Skip to content

Move containerd.service from Requires= to Wants= #42622

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
Merged
justincormack merged 1 commit into from
Jul 13, 2021
Merged

Move containerd.service from Requires= to Wants= #42622

justincormack merged 1 commit into from
Jul 13, 2021

Conversation

@aiordache
Copy link

@aiordache aiordache commented Jul 12, 2021

Carries docker/docker-ce-packaging#512

Per the systemd.unit documentation:

If this unit gets activated, the units listed will be activated as well. If one of the other units fails to activate, and an ordering dependency After= on the failing unit is set, this unit will not be started. Besides, with or without specifying After=, this unit will be stopped if one of the other units is explicitly stopped.

Often, it is a better choice to use Wants= instead of Requires= in order to achieve a system that is more robust when dealing with failing services.

This should also be generally "safe" given we added --containerd=/run/containerd/containerd.sock to the flags we pass to dockerd.

@thaJeztah thaJeztah added this to the 21.xx milestone Jul 12, 2021
@thaJeztah thaJeztah added the area/runtime Runtime label Jul 12, 2021
thaJeztah
thaJeztah approved these changes Jul 12, 2021
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah
Copy link
Member

thaJeztah commented Jul 12, 2021

@tianon @AkihiroSuda @cpuguy83 PTAL

@thaJeztah
Copy link
Member

thaJeztah commented Jul 12, 2021

Looks like failure on the OpenSUSE package repositories. Not sure if an issue with those repos or something in our CI

/cc @cyphar

[2021-07-12T09:08:03.716Z] #42 https://download.opensuse.org/repositories/devel:/tools:/criu/Debian_10/Release.key
[2021-07-12T09:08:03.716Z] #42 sha256:4ebf95ef235c2a9565f1170d2820bf979767f762364b8c31dc05ef47e15c9784
[2021-07-12T09:08:25.629Z] #42 ERROR: Get https://download.opensuse.org/repositories/devel:/tools:/criu/Debian_10/Release.key: net/http: TLS handshake timeout
[2021-07-12T09:08:25.629Z] ------
[2021-07-12T09:08:25.629Z]  > https://download.opensuse.org/repositories/devel:/tools:/criu/Debian_10/Release.key:
[2021-07-12T09:08:25.629Z] ------
[2021-07-12T09:08:25.629Z] failed to load cache key: Get https://download.opensuse.org/repositories/devel:/tools:/criu/Debian_10/Release.key: net/http: TLS handshake timeout

Can pull the key locally (with curl);

curl -fsSL https://download.opensuse.org/repositories/devel:/tools:/criu/Debian_10/Release.key
-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: GnuPG v1.4.5 (GNU/Linux)

mQENBFVGB2oBCACy/imYheDMT8xxhNlXZyBm/lvDzFYgqALLHwQWukcMDK/F9xj/
N4EVxrt68CWLmClFcjIowTDy5+DrNGO2kBIE1TGljVuK6gBOIF041Hki2sWZ0gFo
Uers3iMQM9xxgsc3iMDQySZxtRbmuPRAxqpKJwJbcMZtnKHn1+PPBbfFlye/ihjL
YWFM9vd+5wOwpeae5FK2Q38XQBT0zLsWyx+jFdVmX8i+2m+eCr/hG68+3nAtGRiU
mQWf1Z8eDMVr5M712AsqJdVzZqStJYyoig/7FLwozaqbjjaHTPvqqMqv2/n1/eiK
SVdoKW0JM0U0jNzKSMt8mywceiEYBJUPFl8BABEBAAG0OGRldmVsOnRvb2xzIE9C
UyBQcm9qZWN0IDxkZXZlbDp0b29sc0BidWlsZC5vcGVuc3VzZS5vcmc+iQE+BBMB
AgAoBQJdSB8cAhsDBQkMIMeyBgsJCAcDAgYVCAIJCgsEFgIDAQIeAQIXgAAKCRAw
qDQ6SY1aI499B/42mqYhL0L3Zlf55dcpqibPlRMlYf0P7CLoeHmMEft01YDQBxRP
AGKtzcnyh9HmsCxNLbidoWB6pPBl8/WcjAt4SZ8LcqIHANaLbucIGzZ5DrPktVZj
yBoy7rA1NhvalVPf/zC/l9Q40+ESGTGbx8FMMgOuLda1nVnkkjD20s91Bnh/1LYf
1xNX6i4AA3cgWeMKuaNQol6gH8qxutAyRJ+2wYQOOeiiccoPSoQTdcTFNYpTjDue
wF89zIimHJ45NaRnV4p/Ahik9q7hoqPAJPe2mcVfHTW6hXnuhg6+Lmivg2+NJf3A
DOiV412wP71bJMfDSwElrcTqnzOu0qWiu0sFiEYEExECAAYFAlVGB2oACgkQOzAR
t2udZSMFFwCeKAEb41puy81dRATC/XgZs15SUHcAoJ4ge3z04vKfGgV/xJbTW/+9
2ZpB
=gJsZ
-----END PGP PUBLIC KEY BLOCK-----

Let me kick CI again

@tianon
Move containerd.service from Requires= to Wants=
a985655 
Per the systemd.unit documentation:

> If this unit gets activated, the units listed will be activated as well. If one of the other units fails to activate, and an ordering dependency After= on the failing unit is set, this unit will not be started. Besides, with or without specifying After=, this unit will be stopped if one of the other units is explicitly stopped.
>
> Often, it is a better choice to use Wants= instead of Requires= in order to achieve a system that is more robust when dealing with failing services.

This should also be generally "safe" given we added `--containerd=/run/containerd/containerd.sock` to the flags we pass to `dockerd`.

Signed-off-by: Tianon Gravi <[email protected]>
Signed-off-by: Anca Iordache <[email protected]>
tianon
tianon approved these changes Jul 12, 2021
View reviewed changes
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

LGTM (obvs.)

@justincormack justincormack merged commit cf73a45 into moby:master Jul 13, 2021
sir-xw pushed a commit to openkylin/docker.io that referenced this pull request Apr 22, 2025
Order docker.service after containerd.service
7e93a88 
Origin: upstream, cherry-picked parts of moby/moby#42373
 and moby/moby#42622
Bug-Debian: https://bugs.debian.org/989490

Fixes proper shutdown of containers.

Gbp-Pq: Name engine-systemd-service-after-containerd.patch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tianon tianon tianon approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@justincormack justincormack justincormack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/packaging area/runtime Runtime area/systemd impact/changelog status/2-code-review

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@aiordache @thaJeztah @tianon @justincormack