Add "Warnings" to /info endpoint, and move detection to the daemon by thaJeztah · Pull Request #37502 · moby/moby

thaJeztah · 2018-07-19T11:50:50Z

When requesting information about the daemon's configuration through the /info
endpoint, missing features (or non-recommended settings) may have to be presented
to the user.

Detecting these situations, and printing warnings currently is handled by the
cli, which results in some complications:

duplicated effort: each client has to re-implement detection and warnings.
it's not possible to generate warnings for reasons outside of the information
returned in the /info response.
cli-side detection has to be updated for new conditions. This means that an
older cli connecting to a new daemon may not print all warnings (due to
it not detecting the new conditions)
some warnings (in particular, warnings about storage-drivers) depend on
driver-status (DriverStatus) information. The format of the information
returned in this field is not part of the API specification and can change
over time, resulting in cli-side detection no longer being functional.

This patch adds a new Warnings field to the /info response. This field is
to return warnings to be presented by the user.

Existing warnings that are currently handled by the CLI are copied to the daemon
as part of this patch; This change is backward-compatible with existing
clients; old client can continue to use the client-side warnings, whereas new
clients can skip client-side detection, and print warnings that are returned by
the daemon.

Example response with this patch applied;

curl --unix-socket /var/run/docker.sock http://localhost/info | jq .Warnings

[
  "WARNING: bridge-nf-call-iptables is disabled",
  "WARNING: bridge-nf-call-ip6tables is disabled"
]

- Description for the changelog

* The `GET /info` API endpoint now returns a `Warnings` field, containing warnings
  and informational messages about missing features, or issues related to the daemon
  configuration.

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

image source

thaJeztah · 2018-07-19T11:53:39Z

api/types/types.go

I kept this simple, and only return a list of strings; if we think it's important we can make the information more rich (return level and message for each warning, e.g. level=info, level=warning), but doing so felt like over-engineering, over-complicating.

codecov · 2018-07-19T17:56:24Z

Codecov Report

Merging #37502 into master will increase coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #37502      +/-   ##
==========================================
+ Coverage   36.05%    36.1%   +0.04%     
==========================================
  Files         609      610       +1     
  Lines       44993    45102     +109     
==========================================
+ Hits        16223    16282      +59     
- Misses      26541    26576      +35     
- Partials     2229     2244      +15

olljanat · 2018-07-26T17:45:31Z

Now you are only warning Linux users about Windows specific features.

Shouldn't we also warn Windows users about Linux specific features?
Example endpoint mode VIP is currenlyt only supported by Windows: https://docs.microsoft.com/en-us/virtualization/windowscontainers/manage-containers/swarm-mode#deploying-services-to-a-swarm

thaJeztah · 2018-07-26T18:51:59Z

This PR doesn't change what warnings are generated; non of these currently could be generated on a Windows daemon (eg memory limits etc, because Windows native daemons don't use cgroup for that)

I deliberately didn't add new warnings to this PR, only migrating what's currently there, and provide th mechanism to return warnings through the API; follow up PR's can add other warnings where needed

tiborvass · 2018-08-17T15:56:02Z

@thaJeztah small nit but I don't think WARNING: prefix should be part of it, that can be prepended by the client.

Otherwise this looks sane to me +1 !

Just need to fix conflict.

thaJeztah · 2018-08-17T21:54:26Z

I don't think WARNING: prefix should be part of it

@tiborvass reason I went for this approach is to be able to print the warnings/messages verbatim by the client; perhaps we want a message to print "ALERT" or something else; using a struct with "level" or "type" felt like overkill for that.

I'll rebase this to fix the conflict

thaJeztah · 2018-08-17T23:30:28Z

@tiborvass rebased to resolve the conflict

thaJeztah · 2018-08-20T17:26:27Z

Rebased again..

tiborvass

LGTM

cpuguy83 · 2018-08-20T17:35:07Z

daemon/info_unix.go

Minor nit, are these "WARNING:" prefixes redudnent?

@cpuguy83 in general, or this one in particular? also see #37502 (comment)

Ah, sorry just looked at the code before... It's fine I guess.

cpuguy83

LGTM

thaJeztah · 2018-08-20T21:19:30Z

We can ignore s390x for now. but I'll wait with merging until #37558 is in; it'll likely conflict in the API version history. Once that PR is merged, I'll resolve that conflict here, and merge (as it's green now)

When requesting information about the daemon's configuration through the `/info` endpoint, missing features (or non-recommended settings) may have to be presented to the user. Detecting these situations, and printing warnings currently is handled by the cli, which results in some complications: - duplicated effort: each client has to re-implement detection and warnings. - it's not possible to generate warnings for reasons outside of the information returned in the `/info` response. - cli-side detection has to be updated for new conditions. This means that an older cli connecting to a new daemon may not print all warnings (due to it not detecting the new conditions) - some warnings (in particular, warnings about storage-drivers) depend on driver-status (`DriverStatus`) information. The format of the information returned in this field is not part of the API specification and can change over time, resulting in cli-side detection no longer being functional. This patch adds a new `Warnings` field to the `/info` response. This field is to return warnings to be presented by the user. Existing warnings that are currently handled by the CLI are copied to the daemon as part of this patch; This change is backward-compatible with existing clients; old client can continue to use the client-side warnings, whereas new clients can skip client-side detection, and print warnings that are returned by the daemon. Example response with this patch applied; ```bash curl --unix-socket /var/run/docker.sock http://localhost/info | jq .Warnings ``` ```json [ "WARNING: bridge-nf-call-iptables is disabled", "WARNING: bridge-nf-call-ip6tables is disabled" ] ``` Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2018-08-21T09:37:02Z

Rebased to fix conflict in api version history (as expected);

diff --cc docs/api/version-history.md
index 7c18799fc5,3e7474ce79..0000000000
--- a/docs/api/version-history.md
+++ b/docs/api/version-history.md
@@@ -21,9 -21,8 +21,14 @@@ keywords: "API, Docker, rcli, REST, doc
    and `OperatingSystem` if the daemon was unable to obtain this information.
  * `GET /info` now returns information about the product license, if a license
    has been applied to the daemon.
++<<<<<<< HEAD
 +* `POST /swarm/init` now accepts a `DefaultAddrPool` property to set global scope default address pool
 +* `POST /swarm/init` now accepts a `SubnetSize` property to set global scope networks by giving the
 +  length of the subnet masks for every such network
++=======
+ * `GET /info` now returns a `Warnings` field, containing warnings and informational
+   messages about missing features, or issues related to the daemon configuration.
++>>>>>>> Add "Warnings" to /info endpoint, and move detection to the daemon
  
  ## V1.38 API changes

CI was green before that change, so I'll merge this as mentioned above

GordonTheTurtle added the status/0-triage label Jul 19, 2018

thaJeztah added status/1-design-review impact/api impact/changelog and removed status/0-triage labels Jul 19, 2018

thaJeztah commented Jul 19, 2018

View reviewed changes

thaJeztah force-pushed the you_have_been_warned branch 2 times, most recently from 5584711 to ce47c88 Compare July 19, 2018 17:55

thaJeztah mentioned this pull request Jul 19, 2018

Use warnings provided by daemon docker/cli#1225

Merged

thaJeztah force-pushed the you_have_been_warned branch from ce47c88 to 6b6f862 Compare August 2, 2018 13:53

thaJeztah added rebuild/janky labels Aug 2, 2018

GordonTheTurtle removed rebuild/experimental labels Aug 2, 2018

GordonTheTurtle assigned mlaventure Aug 3, 2018

tiborvass added status/2-code-review and removed status/1-design-review labels Aug 17, 2018

thaJeztah force-pushed the you_have_been_warned branch from 6b6f862 to bd888c4 Compare August 17, 2018 23:29

thaJeztah force-pushed the you_have_been_warned branch from bd888c4 to f6b3a78 Compare August 20, 2018 17:26

tiborvass approved these changes Aug 20, 2018

View reviewed changes

cpuguy83 reviewed Aug 20, 2018

View reviewed changes

cpuguy83 approved these changes Aug 20, 2018

View reviewed changes

cpuguy83 added the status/4-merge label Aug 20, 2018

cpuguy83 removed the status/2-code-review label Aug 20, 2018

thaJeztah force-pushed the you_have_been_warned branch from f6b3a78 to a3d4238 Compare August 21, 2018 09:36

thaJeztah merged commit 2629fe9 into moby:master Aug 21, 2018

thaJeztah deleted the you_have_been_warned branch August 21, 2018 09:37

thaJeztah mentioned this pull request Oct 10, 2018

Windows: Client: Allow process isolation [RS5+] #38000

Merged

thaJeztah mentioned this pull request Dec 13, 2018

Fix warnings not being printed on "create", only on "run" docker/cli#1571

Merged

thaJeztah mentioned this pull request Jan 20, 2024

info: remove printServerWarningsLegacy docker/cli#4808

Merged

Conversation

thaJeztah commented Jul 19, 2018

Uh oh!

thaJeztah Jul 19, 2018

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

olljanat commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jul 26, 2018

Uh oh!

tiborvass commented Aug 17, 2018

Uh oh!

thaJeztah commented Aug 17, 2018

Uh oh!

thaJeztah commented Aug 17, 2018

Uh oh!

thaJeztah commented Aug 20, 2018

Uh oh!

tiborvass left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 20, 2018

Uh oh!

thaJeztah commented Aug 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Jul 19, 2018 •

edited

Loading

olljanat commented Jul 26, 2018 •

edited

Loading