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

Add "Warnings" to /info endpoint, and move detection to the daemon #37502

Merged
merged 1 commit into from
Aug 21, 2018

Conversation

thaJeztah
Copy link
Member

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)

vampire13-660x495

image source

@@ -204,6 +204,7 @@ type Info struct {
RuncCommit Commit
InitCommit Commit
SecurityOptions []string
Warnings []string
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

codecov bot commented Jul 19, 2018

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
Copy link
Contributor

olljanat commented Jul 26, 2018

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
Copy link
Member Author

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
Copy link
Contributor

@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
Copy link
Member Author

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
Copy link
Member Author

@tiborvass rebased to resolve the conflict

@thaJeztah
Copy link
Member Author

Rebased again..

Copy link
Contributor

@tiborvass tiborvass left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -68,6 +69,80 @@ func (daemon *Daemon) fillPlatformInfo(v *types.Info, sysInfo *sysinfo.SysInfo)
logrus.Warnf("failed to retrieve %s version: %s", defaultInitBinary, err)
v.InitCommit.ID = "N/A"
}

if !v.MemoryLimit {
v.Warnings = append(v.Warnings, "WARNING: No memory limit support")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

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 <github@gone.nl>
@thaJeztah
Copy link
Member Author

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

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

6 participants