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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: update the runtime configuration section #4344

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

dvdksn
Copy link
Contributor

@dvdksn dvdksn commented Jun 12, 2023

relates to: docker/docs#17517

- What I did

Updated the section about runtime options, it now includes how to configure
containerd shims using the runtimeType and options fields.

Because there are now two ways to configure runtimes, it also demanded that I
refactor the entire section.

I've tried to add justification for when to use runtimeType, and when to use
path. It's quite difficult to accurately capture the distinction, while also
keeping the section somewhat intelligible. I tried.

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

馃尛

@dvdksn dvdksn requested a review from thaJeztah as a code owner June 12, 2023 20:25
@dvdksn dvdksn force-pushed the docs/dockerd-runtimes-refresh branch from ed75b50 to b3c8d63 Compare June 12, 2023 21:05
@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2023

Codecov Report

Merging #4344 (6c7d17f) into master (f26ac47) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4344      +/-   ##
==========================================
- Coverage   59.29%   59.29%   -0.01%     
==========================================
  Files         288      288              
  Lines       24769    24769              
==========================================
- Hits        14688    14687       -1     
- Misses       9197     9198       +1     
  Partials      884      884              

@dvdksn dvdksn force-pushed the docs/dockerd-runtimes-refresh branch from 9760f3f to 02ea3ae Compare June 15, 2023 07:53
@dvdksn dvdksn force-pushed the docs/dockerd-runtimes-refresh branch 2 times, most recently from 1289063 to 9381bc4 Compare June 15, 2023 15:35
Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

Looking good! Just needs a couple of tweaks first

docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
@dvdksn dvdksn force-pushed the docs/dockerd-runtimes-refresh branch from 9381bc4 to 5d4fc39 Compare June 20, 2023 08:18
@dvdksn
Copy link
Contributor Author

dvdksn commented Jun 20, 2023

Thanks @corhere and @thaJeztah, I've addressed the comments, hopefully we can merge now

@thaJeztah thaJeztah added this to the 25.0.0 milestone Jun 22, 2023
@dvdksn
Copy link
Contributor Author

dvdksn commented Jun 22, 2023

@thaJeztah this needs to be backported to 24.0 as well (features are 23.0 and 24.0, nothing new in 25.0)

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.

Left some comments (sorry!) but not all of them are show-stoppers, so feel free to leave some for follow-ups (the anchor ones may be relevant if you need to link to these sections from elsewhere and if we don't want to update those links "again")

docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
configuration section in
[CRI Plugin Config Guide](https://github.com/containerd/containerd/blob/main/docs/cri/config.md#full-configuration).

##### Configure runc drop-in replacements
Copy link
Member

Choose a reason for hiding this comment

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

Probably for a follow-up; we don't use the regular template for this page (yet), but perhaps we should already add the anchor (and maybe manually create an Options table with links);

Suggested change
##### Configure runc drop-in replacements
##### <a name="add-runtime"></a> Configure runc drop-in replacements (--add-runtime)

For an example configuration for a runc drop-in replacment, see
[Alternative container runtimes > youki](https://docs.docker.com/engine/alternative-runtimes/#youki)

##### Configure the default container runtime
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
##### Configure the default container runtime
##### <a name="default-runtime"></a> Configure the default container runtime (--default-runtime)

docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
By default, the Docker daemon automatically starts `containerd`. If you want to
control `containerd` startup, manually start `containerd` and pass the path to
the `containerd` socket using the `--containerd` flag. For example:
Copy link
Member

Choose a reason for hiding this comment

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

This is not entirely true; automatic starting is mostly still there for backward-compatibility (and debugging), but the daemon will first check if /run/containerd/containerd.sock (default location for containerd's socket) is present. If that's not the case, it assumes containerd is not managed / running as a service, and it falls back to starting its own instance.

An unfortunate side-effect of that is that if (for any reason) the containerd service is not (or not YET) running, dockerd will start its own instance, which uses it's own storage location (different from the default) and other options. This can also lead to 2 instances of containerd running on the host (if the actual containerd service is started).

Setting the --containerd socket helps with such situations (race conditions, or containerd not running where it should be), as it disables the feature that automatically starts a new instance. This is also why we added this option as a default in the systemd unit we ship for docker engine; see

(I should mention that that there's things being discussed in this area, and we may consider using different defaults, and/or to make "starting containerd" an opt-in option (instead of opt-out)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, this is quite good information. I didn't actually change this section when updating the doc; I only moved it around. So for that reason (and because this is still under discussion) I will leave it as is for now.

docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved

The `native.cgroupdriver` option specifies the management of the container's
cgroups. You can only specify `cgroupfs` or `systemd`. If you specify
#### Use a manually controlled containerd daemon
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#### Use a manually controlled containerd daemon
#### <a name="containerd"></a> Use a manually controlled containerd daemon (--containerd)

(see my other comment) not sure if "manually controlled" is the right wording here; this option would usually be if containerd is running as its own service (which is the default nowadays).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Run containerd standalone"?

By default, the Docker daemon automatically starts `containerd`. If you want to
control `containerd` startup, manually start `containerd` and pass the path to
the `containerd` socket using the `--containerd` flag. For example:
#### Configure container runtimes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#### Configure container runtimes
#### <a name="runtime"></a> Configure container runtimes (--runtime)

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

I don't have time for a complete review yet, but I'd like to register my intention to dig in to this in the next day or so and provide feedback.

docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
Signed-off-by: David Karlsson <35727626+dvdksn@users.noreply.github.com>
@dvdksn dvdksn force-pushed the docs/dockerd-runtimes-refresh branch from 5d4fc39 to 6c7d17f Compare June 22, 2023 19:15
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 thaJeztah merged commit 41384b0 into docker:master Jun 26, 2023
74 checks passed
@dvdksn dvdksn deleted the docs/dockerd-runtimes-refresh branch June 26, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants