Update new systemd unit file with changes from upstream #179
Conversation
This file contains hidden or 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
Member
Author
|
ping @crosbymichael @seemethere @antonybichon17 @dhiltgen PTAL |
|
|
||
| # Comment TasksMax if your systemd version does not supports it. | ||
| # Only systemd 226 and above support this version. | ||
| TasksMax=infinity |
Contributor
There was a problem hiding this comment.
i think we will need this in containerd as well
Contributor
|
LGTM |
thaJeztah
force-pushed
the
update_systemd_unit
branch
from
f1f6873 to
c414876
Compare
seemethere
previously approved these changes
Aug 31, 2018
Contributor
seemethere
left a comment
seemethere
left a comment
There was a problem hiding this comment.
Does this work with systems 219? I believe that's the oldest we currently support.
set LimitCORE=infinity to ensure complete core creation, allows extraction of as much information as possible. Signed-off-by: Sebastiaan van Stijn <[email protected]>
There is a not-insignificant performance overhead for all containers (if containerd is a child of Docker, which is the current setup) if systemd sets rlimits on the main Docker daemon process (because the limits propogate to all children). Signed-off-by: Sebastiaan van Stijn <[email protected]>
Systemd sets a default of 512 tasks, which is far too low to run many containers. Note that TasksMax is only supported on systemd 226 and above. Signed-off-by: Sebastiaan van Stijn <[email protected]>
We need to add delegate yes to docker's service file so that it can
manage the cgroups of the processes that it launches without systemd
interfering with them and moving the processes after it is reloaded.
Delegate=
Turns on delegation of further resource control partitioning to
processes of the unit. For unprivileged services (i.e. those
using the User= setting), this allows processes to create a
subhierarchy beneath its control group path. For privileged
services and scopes, this ensures the processes will have all
control group controllers enabled.
This is the proper fix for issue moby/moby#20152
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Change the kill mode to process so that systemd does not kill container processes when the daemon is shutdown but only the docker daemon Signed-off-by: Sebastiaan van Stijn <[email protected]>
This adds support for reloading the docker daemon
(SIGHIUP) so that changes in '/etc/docker/daemon.json'
can be loaded at runtime by reloading the service
through systemd ('systemctl reload docker')
Before this change, systemd would output an error
that "reloading" is not supported for the docker
service;
systemctl reload docker
Failed to reload docker.service: Job type reload is not applicable for unit docker.service.
After this change, the docker daemon can be reloaded
through 'systemctl reload docker', which reloads
the configuration;
journalctl -f -u docker.service
May 02 03:49:20 testing systemd[1]: Reloading Docker Application Container Engine.
May 02 03:49:20 testing docker[28496]: time="2016-05-02T03:49:20.143964103-04:00" level=info msg="Got signal to reload configuration, reloading from: /etc/docker/daemon.json"
May 02 03:49:20 testing systemd[1]: Reloaded Docker Application Container Engine.
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
update_systemd_unit
branch
from
c414876 to
051b900
Compare
Member
Author
|
just pushed again to fix a typo in one comment 😊 Would be good if we could test if this fixes all issues before merging yes. Unknown options should be ignored by systemd (so perhaps we can remove the comment about TasksMax) |
thaJeztah
force-pushed
the
update_systemd_unit
branch
from
051b900 to
c400753
Compare
Note that StartLimit* options were moved from "Service" to "Unit" in systemd 229 (systemd/systemd@6bf0f40) both the old, and new location are accepted by systemd 229 and up, so using the old location to make them work for either version of systemd. StartLimitInterval was renamed to StartLimitIntervalSec in systemd 230 (systemd/systemd@f0367da) both the old, and new name are accepted by systemd 230 and up, so using the old name to make this option work for either version of systemd. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
update_systemd_unit
branch
from
c400753 to
2c2bfea
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The new systemd unit file was missing various options; this PR is porting those options. I wasn't sure if all options should be set in the new situation, so I included each change in a separate commit, keeping the original upstream commit message where applicable;
bump open files (original PR: bump container limit via systemd conf file moby/moby#4455)Disable timeout for systemd (original PR: Disable timeout for systemd moby/moby#18408)always, but added the burst limitsI can drop commits that are not needed 👍