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.

Sign up for GitHub

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

Jump to bottom

Use warnings provided by daemon #1225

Merged
merged 2 commits into from
Aug 21, 2018
Merged

Use warnings provided by daemon #1225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7da7132
bump docker/docker
thaJeztah Jul 19, 2018
3c27ce2
Use warnings provided by daemon
thaJeztah Jul 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 64 additions & 40 deletions cli/command/system/info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,45 +206,7 @@ func prettyPrintInfo(dockerCli command.Cli, info types.Info) error {
fmt.Fprintln(dockerCli.Out(), "Live Restore Enabled:", info.LiveRestoreEnabled)
fmt.Fprint(dockerCli.Out(), "\n")

// Only output these warnings if the server does not support these features
if info.OSType != "windows" {
printStorageDriverWarnings(dockerCli, info)

if !info.MemoryLimit {
fmt.Fprintln(dockerCli.Err(), "WARNING: No memory limit support")
}
if !info.SwapLimit {
fmt.Fprintln(dockerCli.Err(), "WARNING: No swap limit support")
}
if !info.KernelMemory {
fmt.Fprintln(dockerCli.Err(), "WARNING: No kernel memory limit support")
}
if !info.OomKillDisable {
fmt.Fprintln(dockerCli.Err(), "WARNING: No oom kill disable support")
}
if !info.CPUCfsQuota {
fmt.Fprintln(dockerCli.Err(), "WARNING: No cpu cfs quota support")
}
if !info.CPUCfsPeriod {
fmt.Fprintln(dockerCli.Err(), "WARNING: No cpu cfs period support")
}
if !info.CPUShares {
fmt.Fprintln(dockerCli.Err(), "WARNING: No cpu shares support")
}
if !info.CPUSet {
fmt.Fprintln(dockerCli.Err(), "WARNING: No cpuset support")
}
if !info.IPv4Forwarding {
fmt.Fprintln(dockerCli.Err(), "WARNING: IPv4 forwarding is disabled")
}
if !info.BridgeNfIptables {
fmt.Fprintln(dockerCli.Err(), "WARNING: bridge-nf-call-iptables is disabled")
}
if !info.BridgeNfIP6tables {
fmt.Fprintln(dockerCli.Err(), "WARNING: bridge-nf-call-ip6tables is disabled")
}
}

printWarnings(dockerCli, info)
return nil
}

Expand Down Expand Up @@ -315,11 +277,73 @@ func printSwarmInfo(dockerCli command.Cli, info types.Info) {
}
}

func printWarnings(dockerCli command.Cli, info types.Info) {
if len(info.Warnings) > 0 {
fmt.Fprintln(dockerCli.Err(), strings.Join(info.Warnings, "\n"))
return
}
// daemon didn't return warnings. Fallback to old behavior
printStorageDriverWarnings(dockerCli, info)
printWarningsLegacy(dockerCli, info)
}

// printWarningsLegacy generates warnings based on information returned by the daemon.
// DEPRECATED: warnings are now generated by the daemon, and returned in
// info.Warnings. This function is used to provide backward compatibility with
// daemons that do not provide these warnings. No new warnings should be added
// here.
func printWarningsLegacy(dockerCli command.Cli, info types.Info) {
if info.OSType == "windows" {
return
}
if !info.MemoryLimit {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I know it is a deprecated function, but we could do some little factorization here 😇

func printWarningLegacy(stdErr io.Writer, property bool, message string){
    if !property{
        fmt.Println(stdErr, "WARNING: "+message)
    }
}

func printWarningsLegacy(dockerCli command.Cli, info types.Info) {
    if info.OSType == "windows" {
        return
    }
    stdErr := dockerCli.Err()
    printWarningLegacy(stdErr, info.MemoryLimit, "No memory limit support")
    printWarningLegacy(stdErr, info. SwapLimit, "No swap limit support")
...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Was considering that at some point (also for the warnings returned by the daemon); at least for the daemon-returned warnings, I decided to print them as-is, that way we could use those warnings for other types of messages (INFO: your coffee is ready!, ERROR: we ran out of milk!).

We could "engineer" that further (info.level: info, info.message: "your coffee is ready!"), but that felt like taking it too far

Also noticed there's another warning that possibly could be moved; https://github.com/thaJeztah/cli/blob/02f48b838f1c61e251d7f9eb6128f589acc84949/cli/command/system/info.go#L127-L130, and wasn't sure if there would be others I'd find

Copy link
Contributor

Choose a reason for hiding this comment

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

Right and notice the discreet "tabulation" in this warning message 😄
And by the way it was only a nit, I'm fine keeping it as is!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, need to get moby/moby#37502 merged first as well 😅

fmt.Fprintln(dockerCli.Err(), "WARNING: No memory limit support")
}
if !info.SwapLimit {
fmt.Fprintln(dockerCli.Err(), "WARNING: No swap limit support")
}
if !info.KernelMemory {
fmt.Fprintln(dockerCli.Err(), "WARNING: No kernel memory limit support")
}
if !info.OomKillDisable {
fmt.Fprintln(dockerCli.Err(), "WARNING: No oom kill disable support")
}
if !info.CPUCfsQuota {
fmt.Fprintln(dockerCli.Err(), "WARNING: No cpu cfs quota support")
}
if !info.CPUCfsPeriod {
fmt.Fprintln(dockerCli.Err(), "WARNING: No cpu cfs period support")
}
if !info.CPUShares {
fmt.Fprintln(dockerCli.Err(), "WARNING: No cpu shares support")
}
if !info.CPUSet {
fmt.Fprintln(dockerCli.Err(), "WARNING: No cpuset support")
}
if !info.IPv4Forwarding {
fmt.Fprintln(dockerCli.Err(), "WARNING: IPv4 forwarding is disabled")
}
if !info.BridgeNfIptables {
fmt.Fprintln(dockerCli.Err(), "WARNING: bridge-nf-call-iptables is disabled")
}
if !info.BridgeNfIP6tables {
fmt.Fprintln(dockerCli.Err(), "WARNING: bridge-nf-call-ip6tables is disabled")
}
}

// printStorageDriverWarnings generates warnings based on storage-driver information
// returned by the daemon.
// DEPRECATED: warnings are now generated by the daemon, and returned in
// info.Warnings. This function is used to provide backward compatibility with
// daemons that do not provide these warnings. No new warnings should be added
// here.
func printStorageDriverWarnings(dockerCli command.Cli, info types.Info) {
if info.OSType == "windows" {
return
}
if info.DriverStatus == nil {
return
}

for _, pair := range info.DriverStatus {
if pair[0] == "Data loop file" {
fmt.Fprintf(dockerCli.Err(), "WARNING: %s: usage of loopback devices is "+
Expand Down
43 changes: 35 additions & 8 deletions cli/command/system/info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,32 +207,59 @@ func TestPrettyPrintInfo(t *testing.T) {
infoWithWarningsLinux.BridgeNfIptables = false
infoWithWarningsLinux.BridgeNfIP6tables = false

sampleInfoDaemonWarnings := sampleInfoNoSwarm
sampleInfoDaemonWarnings.Warnings = []string{
"WARNING: No memory limit support",
"WARNING: No swap limit support",
"WARNING: No kernel memory 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",
}

for _, tc := range []struct {
doc string
dockerInfo types.Info
expectedGolden string
warningsGolden string
}{
{
doc: "info without swarm",
dockerInfo: sampleInfoNoSwarm,
expectedGolden: "docker-info-no-swarm",
},
{
doc: "info with swarm",
dockerInfo: infoWithSwarm,
expectedGolden: "docker-info-with-swarm",
},
{
doc: "info with legacy warnings",
dockerInfo: infoWithWarningsLinux,
expectedGolden: "docker-info-no-swarm",
warningsGolden: "docker-info-warnings",
},
{
doc: "info with daemon warnings",
dockerInfo: sampleInfoDaemonWarnings,
expectedGolden: "docker-info-no-swarm",
warningsGolden: "docker-info-warnings",
},
} {
cli := test.NewFakeCli(&fakeClient{})
assert.NilError(t, prettyPrintInfo(cli, tc.dockerInfo))
golden.Assert(t, cli.OutBuffer().String(), tc.expectedGolden+".golden")
if tc.warningsGolden != "" {
golden.Assert(t, cli.ErrBuffer().String(), tc.warningsGolden+".golden")
} else {
assert.Check(t, is.Equal("", cli.ErrBuffer().String()))
}
t.Run(tc.doc, func(t *testing.T) {
cli := test.NewFakeCli(&fakeClient{})
assert.NilError(t, prettyPrintInfo(cli, tc.dockerInfo))
golden.Assert(t, cli.OutBuffer().String(), tc.expectedGolden+".golden")
if tc.warningsGolden != "" {
golden.Assert(t, cli.ErrBuffer().String(), tc.warningsGolden+".golden")
} else {
assert.Check(t, is.Equal("", cli.ErrBuffer().String()))
}
})
}
}
2 changes: 1 addition & 1 deletion vendor.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ github.com/cpuguy83/go-md2man v1.0.8
github.com/davecgh/go-spew 346938d642f2ec3594ed81d874461961cd0faa76 # v1.1.0
github.com/dgrijalva/jwt-go a2c85815a77d0f951e33ba4db5ae93629a1530af
github.com/docker/distribution 83389a148052d74ac602f5f1d62f86ff2f3c4aa5
github.com/docker/docker 1800883bd16664846db1572b8c8fbe8c85892cee
github.com/docker/docker 2629fe93266e82751af4f1c7568e21060f065b73
github.com/docker/docker-credential-helpers 5241b46610f2491efdf9d1c85f1ddf5b02f6d962
# the docker/go package contains a customized version of canonical/json
# and is used by Notary. The package is periodically rebased on current Go versions.
Expand Down
1 change: 1 addition & 0 deletions vendor/github.com/docker/docker/api/types/types.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.