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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: refactor cli run reference #4615

Merged
merged 24 commits into from
Dec 13, 2023

Conversation

dvdksn
Copy link
Contributor

@dvdksn dvdksn commented Oct 20, 2023

- What I did

Improving & consolidating the docker run reference instructions.

  • docs/reference/run.md

    Make this a conceptual/introductory page for understanding how docker run works, and the different options at a very high level.

  • docs/reference/commandline/run.md

    Make this the canonical page for looking up the different flags/options for the docker run command.

Related:

- How I did it

- How to verify it

- Description for the changelog

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

docs/reference/run.md Outdated Show resolved Hide resolved
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.

just some quick comments

docs/reference/run.md Outdated Show resolved Hide resolved
docs/reference/run.md Show resolved Hide resolved
docs/reference/run.md Outdated Show resolved Hide resolved
@dvdksn dvdksn force-pushed the docs-cli-reference-refresh branch 2 times, most recently from b59f6d1 to 0d1582e Compare November 7, 2023 12:00
@dvdksn dvdksn force-pushed the docs-cli-reference-refresh branch from 22e4d2b to d06e3a7 Compare November 23, 2023 12:59
@dvdksn dvdksn self-assigned this Nov 23, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2023

Codecov Report

Merging #4615 (f8dd8f0) into master (4dd5c23) will decrease coverage by 0.09%.
Report is 11 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4615      +/-   ##
==========================================
- Coverage   59.77%   59.68%   -0.09%     
==========================================
  Files         287      287              
  Lines       24830    24865      +35     
==========================================
  Hits        14841    14841              
- Misses       9103     9138      +35     
  Partials      886      886              

@dvdksn dvdksn force-pushed the docs-cli-reference-refresh branch 2 times, most recently from 1dfbda9 to 43dda63 Compare November 28, 2023 10:27
@dvdksn dvdksn marked this pull request as ready for review November 28, 2023 10:27
@dvdksn dvdksn requested a review from thaJeztah November 28, 2023 10:29
@thaJeztah thaJeztah added this to the 25.0.0 milestone Nov 28, 2023
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.

I'm gonna take a break for a bit; I looked at commits until "docs: rewrite section on overriding image defaults"

Most comments are probably for follow-ups, but some could be part of this PR (for some follow-ups we may want to create tracking tickets)

docs/reference/run.md Outdated Show resolved Hide resolved
docs/reference/run.md Outdated Show resolved Hide resolved
Comment on lines 22 to 23
```console
$ docker run [OPTIONS] IMAGE[:TAG|@DIGEST] [COMMAND] [ARG...]
```
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this gets removed in a later commit, and not a show-stopper in any way, but it just occurred to me that this "usage" output may add to the confusion around docker run reference vs run (command) reference, as both outline the USAGE

docs/reference/run.md Outdated Show resolved Hide resolved
docs/reference/run.md Outdated Show resolved Hide resolved
Comment on lines +884 to +917
The default init process used is the first `docker-init` executable found in the
system path of the Docker daemon process. This `docker-init` binary, included in
the default installation, is backed by [tini](https://github.com/krallin/tini).

Copy link
Member

Choose a reason for hiding this comment

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

For a follow-up; I think the init-binary to use is configurable on the daemon (if so, we may need a link to that part of the daemon config)

Comment on lines +924 to +959
## <a name="cgroup-parent"></a> Specify custom cgroups

Using the `--cgroup-parent` flag, you can pass a specific cgroup to run a
container in. This allows you to create and manage cgroups on their own. You can
define custom resources for those cgroups and put containers under a common
parent group.
Copy link
Member

Choose a reason for hiding this comment

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

For a follow-up; this section may need a rewrite (but it's pretty-much "power-user" area). There's some (not so) subtle issues with group-parent when using cgroups v2, besides currently this section doesn't describe at all how to use it 😞 (We may have a tracking issue for that somewhere, but not sure if it's still open)

docs/reference/commandline/run.md Outdated Show resolved Hide resolved
docs/reference/commandline/run.md Outdated Show resolved Hide resolved
docs/reference/commandline/run.md Outdated Show resolved Hide resolved
@dvdksn dvdksn force-pushed the docs-cli-reference-refresh branch 3 times, most recently from ad6338d to 01dbbf0 Compare November 29, 2023 12:10
@dvdksn dvdksn requested a review from thaJeztah November 29, 2023 12:10
Create an easier to digest introduction to container networking,
move the bulk of information to the networking overview page.

Signed-off-by: David Karlsson <[email protected]>
@dvdksn dvdksn force-pushed the docs-cli-reference-refresh branch from 01dbbf0 to 99072a2 Compare December 11, 2023 21:19
docs/reference/run.md Show resolved Hide resolved
docs/reference/run.md Outdated Show resolved Hide resolved
docs/reference/run.md Show resolved Hide resolved
docs/reference/run.md Show resolved Hide resolved
docs/reference/run.md Outdated Show resolved Hide resolved
@dvdksn dvdksn force-pushed the docs-cli-reference-refresh branch from 99072a2 to 2e394eb Compare December 12, 2023 09:45
@dvdksn dvdksn requested a review from thaJeztah December 12, 2023 09:45
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!!

docs/reference/commandline/run.md Show resolved Hide resolved
Docker supports two main categories of mounts:

- Volume mounts
- Bind mounts
Copy link
Member

Choose a reason for hiding this comment

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

Related to my previous comment. Looks like this section is not as complete as https://docs.docker.com/storage/, and misses at least the tmpfs mounts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I didn't intend to make everything on this page exhaustive; it could put us back where this page started. My thinking is:

  • General information here
  • All the details in the reference docs

docs/reference/run.md Show resolved Hide resolved
docs/reference/run.md Show resolved Hide resolved
Comment on lines +523 to +525
The range of ports are within an *ephemeral port range* defined by
`/proc/sys/net/ipv4/ip_local_port_range`. Use the `-p` flag to explicitly map a
single port or range of ports.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Sorry for being picky on that one, but I know it's been troublesome in the past, so trying to preserve the information.

Related to that, I need to check with @akerouanton and @robmry what options we currently have to influence these; I know there's some ports used by Windows itself that are within the ephemeral port range, and I know there's been issues with Swarm services picking their own range(s).

Perhaps we need configuration options for this on the daemon (not just a "range", but also options to exclude range(s) or individual ports from being used).

Comment on lines +496 to +497
> If you don't specify an IP address (i.e., `-p 80:80` instead of `-p
> 127.0.0.1:80:80`) when publishing a container's ports, Docker publishes the
Copy link
Member

Choose a reason for hiding this comment

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

Also for a follow-up; we need to make a pass at reviewing examples like this in light of IPv6 (i.e., both 127.0.0.x and ::1 becoming more relevant). /cc @akerouanton @robmry

@thaJeztah thaJeztah merged commit 103840e into docker:master Dec 13, 2023
76 checks passed
@dvdksn dvdksn deleted the docs-cli-reference-refresh branch December 13, 2023 12:17
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.

3 participants