info: improve handling of empty Info #3594

thaJeztah · 2022-05-06T14:21:40Z

relates to docker info --format panic #3593
relates to docker info: skip API connection if possible #3179

Before this change, the function could print an error in some cases, for example;

docker -H tcp://127.0.0.1:2375 info --format '{{.LoggingDriver}}'

template: :1:2: executing "" at <.LoggingDriver>: reflect: indirection through nil pointer to embedded struct field Info

With this patch applied, the error is handled gracefully, and when failing to
connect with the daemon, the error is logged;

docker -H tcp://127.0.0.1:2375 info --format '{{.LoggingDriver}}'
Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running?

docker -H tcp://127.0.0.1:2375 info --format '{{json .}}'
Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running?
{"ID":"","Containers":0,"..."}}

Note that the connection error is also included in the JSON ServerErrors field,
so that the information does not get lost, even if STDERR would be redirected;

docker -H tcp://127.0.0.1:2375 info --format '{{json .ServerErrors}}' 2> /dev/null
["Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running?"]

Signed-off-by: Sebastiaan van Stijn github@gone.nl

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

Before this change, the function could print an error in some cases, for example; ```bash docker -H tcp://127.0.0.1:2375 info --format '{{.LoggingDriver}}' template: :1:2: executing "" at <.LoggingDriver>: reflect: indirection through nil pointer to embedded struct field Info ``` With this patch applied, the error is handled gracefully, and when failing to connect with the daemon, the error is logged; ```bash docker -H tcp://127.0.0.1:2375 info --format '{{.LoggingDriver}}' Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running? docker -H tcp://127.0.0.1:2375 info --format '{{json .}}' Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running? {"ID":"","Containers":0,"..."}} ``` Note that the connection error is also included in the JSON `ServerErrors` field, so that the information does not get lost, even if STDERR would be redirected; ```bash docker -H tcp://127.0.0.1:2375 info --format '{{json .ServerErrors}}' 2> /dev/null ["Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running?"] ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

codecov-commenter · 2022-05-06T14:24:06Z

Codecov Report

Merging #3594 (e96e17d) into master (4cc4385) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3594      +/-   ##
==========================================
- Coverage   59.47%   59.46%   -0.01%     
==========================================
  Files         287      287              
  Lines       24174    24176       +2     
==========================================
  Hits        14377    14377              
- Misses       8931     8933       +2     
  Partials      866      866

rumpl · 2022-05-09T12:59:50Z

cli/command/system/info.go

@@ -84,6 +85,7 @@ func runInfo(cmd *cobra.Command, dockerCli command.Cli, opts *infoOptions) error
 		if dinfo, err := dockerCli.Client().Info(ctx); err == nil {
 			info.Info = &dinfo
 		} else {
+			fmt.Fprintln(dockerCli.Err(), err)


Shouldn't we return on error?

I think this case was to allow at least the Client info to be printed (Server info would not be printed, but "failed to connect") 🤔

Gave it a quick try; here's if we don't return an error

docker -H tcp://127.0.0.1:2375 info Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running? Client: Context: default Debug Mode: false Plugins: buildx: Docker Buildx (Docker Inc., v0.8.2) compose: Docker Compose (Docker Inc., v2.4.1) sbom: View the packaged-based Software Bill Of Materials (SBOM) for an image (Anchore Inc., 0.6.0) scan: Docker Scan (Docker Inc., v0.17.0) Server: Containers: 0 Running: 0 Paused: 0 Stopped: 0 Images: 0 Plugins: Volume: Network: Log: Swarm: NodeID: Is Manager: false Node Address: CPUs: 0 Total Memory: 0B Docker Root Dir: Debug Mode: false Experimental: false Live Restore Enabled: false WARNING: No memory limit support WARNING: No swap limit support WARNING: No oom kill disable support WARNING: No cpu cfs quota support WARNING: No cpu cfs period support WARNING: No cpu shares support WARNING: No cpuset support WARNING: IPv4 forwarding is disabled WARNING: bridge-nf-call-iptables is disabled WARNING: bridge-nf-call-ip6tables is disabled ERROR: Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running? errors pretty printing info

And if we return the error;

docker -H tcp://127.0.0.1:2375 info Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running?

From some perspective "either" can make sense, although I think it's useful to still have the Client information (even if we can't connect to the daemon), which is the same behavior as we have for docker version.

That said; to further improve;

We should not print the WARNING output if we can't connect to the daemon (as they're inaccurate in that case). Those warnings are printed by printServerWarningsLegacy(), which is only to be used for older daemons (and should not be called if we have no daemon to connect with).

Similar to the docker version output, we should consider not printing the Server: output if we can't connect, and instead only print the error for that part.

^^ perhaps an exception to the above would be if a --format flag is specified (?) (e.g. --format '{{.Containers}}') (not sure)

Before this patch, the Server output would be printed even if we failed to connect (including WARNINGS): ```bash docker -H tcp://127.0.0.1:2375 info Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running? Client: Context: default Debug Mode: false Plugins: buildx: Docker Buildx (Docker Inc., v0.8.2) compose: Docker Compose (Docker Inc., v2.4.1) sbom: View the packaged-based Software Bill Of Materials (SBOM) for an image (Anchore Inc., 0.6.0) scan: Docker Scan (Docker Inc., v0.17.0) Server: Containers: 0 Running: 0 Paused: 0 Stopped: 0 Images: 0 Plugins: Volume: Network: Log: Swarm: NodeID: Is Manager: false Node Address: CPUs: 0 Total Memory: 0B Docker Root Dir: Debug Mode: false Experimental: false Live Restore Enabled: false WARNING: No memory limit support WARNING: No swap limit support WARNING: No oom kill disable support WARNING: No cpu cfs quota support WARNING: No cpu cfs period support WARNING: No cpu shares support WARNING: No cpuset support WARNING: IPv4 forwarding is disabled WARNING: bridge-nf-call-iptables is disabled WARNING: bridge-nf-call-ip6tables is disabled ERROR: Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running? errors pretty printing info ``` With this patch; ```bash docker -H tcp://127.0.0.1:2375 info Client: Context: default Debug Mode: false Plugins: buildx: Docker Buildx (Docker Inc., v0.8.2) compose: Docker Compose (Docker Inc., v2.4.1) sbom: View the packaged-based Software Bill Of Materials (SBOM) for an image (Anchore Inc., 0.6.0) scan: Docker Scan (Docker Inc., v0.17.0) Server: ERROR: Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running? errors pretty printing info ``` And if a custom format is used: ```bash docker -H tcp://127.0.0.1:2375 info --format '{{.Containers}}' Cannot connect to the Docker daemon at tcp://127.0.0.1:2375. Is the docker daemon running? 0 ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2022-05-10T10:14:58Z

@rumpl I pushed a commit that improves the "failed to connect to server" situation; it now no longer prints the Server information (and instead only shows the error if no --format flag is printed)

thaJeztah added the status/2-code-review label May 6, 2022

thaJeztah added this to the 22.04.0 milestone May 6, 2022

thaJeztah mentioned this pull request May 6, 2022

docker info --format panic #3593

Closed

thaJeztah requested a review from rumpl May 6, 2022 15:01

rumpl approved these changes May 9, 2022

View reviewed changes

rumpl reviewed May 9, 2022

View reviewed changes

ndeloof approved these changes May 11, 2022

View reviewed changes

ndeloof merged commit 8b49584 into docker:master May 11, 2022

thaJeztah deleted the improve_info_formatting branch May 11, 2022 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

info: improve handling of empty Info #3594

info: improve handling of empty Info #3594

thaJeztah commented May 6, 2022

codecov-commenter commented May 6, 2022 •

edited

Loading

rumpl May 9, 2022

thaJeztah May 9, 2022

rumpl May 9, 2022

thaJeztah May 10, 2022

thaJeztah May 10, 2022

thaJeztah commented May 10, 2022

info: improve handling of empty Info #3594

info: improve handling of empty Info #3594

Conversation

thaJeztah commented May 6, 2022

codecov-commenter commented May 6, 2022 • edited Loading

Codecov Report

rumpl May 9, 2022

Choose a reason for hiding this comment

thaJeztah May 9, 2022

Choose a reason for hiding this comment

rumpl May 9, 2022

Choose a reason for hiding this comment

thaJeztah May 10, 2022

Choose a reason for hiding this comment

thaJeztah May 10, 2022

Choose a reason for hiding this comment

thaJeztah commented May 10, 2022

codecov-commenter commented May 6, 2022 •

edited

Loading