Skip to content

[19.03 backport] systemd unit fixes/updates - #511

Closed
thaJeztah wants to merge 2 commits into
docker:19.03from
thaJeztah:19.03_backport_carry_365
Closed

[19.03 backport] systemd unit fixes/updates#511
thaJeztah wants to merge 2 commits into
docker:19.03from
thaJeztah:19.03_backport_carry_365

Conversation

@thaJeztah

@thaJeztahthaJeztah commented Dec 2, 2020

Copy link
Copy Markdown
Member

IsaiahGraceand others added 2 commits December 2, 2020 12:16
@IsaiahGrace@thaJeztah
systemd: add multi-user.target to After list
afd5bf5
Signed-off-by: Isaiah Grace <irgkenya4@gmail.com>
(cherry picked from commit 36bb015)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@mihao@thaJeztah
Do not "Bind" docker "To" containerd.
317ba05
relates to docker/for-linux#678
When using the BindTo directive, Docker is permanently stopped by systemd
when containerd is temporarily killed and restarted;
Using `Requires` achieves mostly the same, but defines a weaker dependency;
https://www.freedesktop.org/software/systemd/man/systemd.unit.html#Requires=
> Requires=
>
> .. 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.
We may want to look into using `Wants=` instead of `Requires=`, because
that allows docker to continue running if containerd is restarted, quoting
the systemd documentation:
> 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.
Given that docker will likely still fail if the containerd socket is not
present, startup will fail if containerd is not running, but if containerd
is restarted, the docker daemon may be able to try reconnecting.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 22f15d4)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

thaJeztah commented Dec 2, 2020

Copy link
Copy Markdown
MemberAuthor

@tianon@chris-crone@tiborvass PTAL

FWIW; I can drop the first cherry-pick, and resolve the merge conflict if we don't want that change

@thaJeztahthaJeztah mentioned this pull request Dec 2, 2020
Closed
3 tasks
@tianon

tianon commented Dec 2, 2020

Copy link
Copy Markdown
Contributor

cc @mwhudson@mitechie - I think you (or someone in your sphere) was doing some testing around this to verify that it's sufficient? Any update you can report?

@thaJeztah

thaJeztah commented Dec 2, 2020

Copy link
Copy Markdown
MemberAuthor

from docker/for-linux#1155 (comment) it looks like the original issue would only be hit my users running the distro -packages, but definitely interested if there's other recommendations/fixes we should have

@tianon

tianon commented Dec 5, 2020

Copy link
Copy Markdown
Contributor

See https://code.launchpad.net/~bryce/ubuntu/+source/docker.io/+git/docker.io/+merge/394913 (@bryceharrington 👋) -- it sounds like in Canonical's testing, Wants=containerd.service (instead of Requires=) has the more appropriate behavior?

@tianon

tianon commented Dec 5, 2020

Copy link
Copy Markdown
Contributor

(Basically, allowing containerd to restart freely without causing Docker to stop/restart also, which AFAIR Docker is supposed to be tolerant of?)

@sergiodj

sergiodj commented Dec 5, 2020

Copy link
Copy Markdown

See https://code.launchpad.net/~bryce/ubuntu/+source/docker.io/+git/docker.io/+merge/394913 (@bryceharrington wave) -- it sounds like in Canonical's testing, Wants=containerd.service (instead of Requires=) has the more appropriate behavior?

Yeah, this is what we found to work best. We've also identified a problem in docker.io's package that makes it not honour the "don't restart the docker service when upgrading the package" setting, so we're fixing that as well.

@tianon

tianon commented Dec 5, 2020

Copy link
Copy Markdown
Contributor

Right, that one's a Canonical/Ubuntu-specific one though -- the upstream packages restart the daemon (intentionally) because otherwise there are often issues connecting to/managing it, or it has issues starting new containers, etc. and the official stance is that containers that should be restarted should have an appropriate restart policy. 😅

(Thanks for commenting/confirming; really appreciate it! 👍)

@sergiodj

sergiodj commented Dec 5, 2020

Copy link
Copy Markdown

See https://code.launchpad.net/~bryce/ubuntu/+source/docker.io/+git/docker.io/+merge/394913 (@bryceharrington wave) -- it sounds like in Canonical's testing, Wants=containerd.service (instead of Requires=) has the more appropriate behavior?

Yeah, this is what we found to work best. We've also identified a problem in docker.io's package that makes it not honour the "don't restart the docker service when upgrading the package" setting, so we're fixing that as well.

To better clarify my answer, we noticed that even with Requires= the relationship between docker and containerd was too strong, and it was making docker stop when containerd was being upgraded.

@sergiodj

sergiodj commented Dec 5, 2020

Copy link
Copy Markdown

Right, that one's a Canonical/Ubuntu-specific one though -- the upstream packages restart the daemon (intentionally) because otherwise there are often issues connecting to/managing it, or it has issues starting new containers, etc. and the official stance is
that containers that should be restarted should have an appropriate restart policy. sweat_smile

Right, this is Ubuntu-specific :-).

(Thanks for commenting/confirming; really appreciate it! +1)

No problem!

@tianontianon mentioned this pull request Dec 5, 2020
Closed
@tianon

tianon commented Dec 5, 2020

Copy link
Copy Markdown
Contributor

Opened #512 👍

@thaJeztah

thaJeztah commented Dec 5, 2020

Copy link
Copy Markdown
MemberAuthor

To better clarify my answer, we noticed that even with Requires= the relationship between docker and containerd was too strong, and it was making docker stop when containerd was being upgraded.

I was discussing that earlier this week, abs was reminded that with one of the containerd updates this was actually required (otherwise an old client was kept in memory)

@thaJeztah

thaJeztah commented Dec 5, 2020

Copy link
Copy Markdown
MemberAuthor

oh, actually, that problem was with containerd docker/containerd-packaging#113

@pvizeli

pvizeli commented Dec 10, 2020

Copy link
Copy Markdown

Please don't merge this. It's just not correct for systemd: moby/moby#41772

@thaJeztah

thaJeztah commented Dec 10, 2020

Copy link
Copy Markdown
MemberAuthor

Thanks! Yes, we're aware of that issue, and will be publishing updated packages for 20.10 with the change reverted.

This PR is a backport for 19.03 (if we do another patch release, but there's no ETA yet for that), but the revert of that change will be included here before it would go out.

@thaJeztah

thaJeztah commented Dec 10, 2020

Copy link
Copy Markdown
MemberAuthor

I'm pushing a PR to revert for master / 20.10 later

@thaJeztah

thaJeztah commented Feb 8, 2022

Copy link
Copy Markdown
MemberAuthor

closing this, as there's no more releases planned for 19.03

@thaJeztah
thaJeztah deleted the 19.03_backport_carry_365 branch February 8, 2022 16:09
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@thaJeztah@tianon@sergiodj@pvizeli@IsaiahGrace@mihao