Skip to content

Commit b397391

Browse files
thaJeztahthaJeztah
committed
cli: perform feature detection lazily
- Perform feature detection when actually needed, instead of during initializing - Version negotiation is performed either when making an API request, or when (e.g.) running `docker help` (to hide unsupported features) - Use a 2 second timeout when 'pinging' the daemon; this should be sufficient for most cases, and when feature detection failed, the daemon will still perform validation (and produce an error if needed) - context.WithTimeout doesn't currently work with ssh connections (connhelper), so we're only applying this timeout for tcp:// connections, otherwise keep the old behavior. Before this change: time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker help &> /dev/null' real 0m32.919s user 0m0.370s sys 0m0.227s time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker context ls &> /dev/null' real 0m32.072s user 0m0.029s sys 0m0.023s After this change: time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker help &> /dev/null' real 0m 2.28s user 0m 0.03s sys 0m 0.03s time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker context ls &> /dev/null' real 0m 0.13s user 0m 0.02s sys 0m 0.02s Signed-off-by: Sebastiaan van Stijn <[email protected]>
1 parent a88a1be commit b397391

2 files changed

Lines changed: 81 additions & 74 deletions

File tree

‎cli/command/cli.go‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ import (
88
"path/filepath"
99
"runtime"
1010
"strconv"
11+
"strings"
12+
"sync"
13+
"time"
1114

1215
"github.com/docker/cli/cli/config"
1316
cliconfig "github.com/docker/cli/cli/config"
@@ -133,9 +136,12 @@ func (cli *DockerCli) loadConfigFile() {
133136
cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err)
134137
}
135138

139+
var fetchServerInfo sync.Once
140+
136141
// ServerInfo returns the server version details for the host this client is
137142
// connected to
138143
func (cli *DockerCli) ServerInfo() ServerInfo {
144+
fetchServerInfo.Do(cli.initializeFromClient)
139145
return cli.serverInfo
140146
}
141147

@@ -270,11 +276,6 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize
270276
return err
271277
}
272278
}
273-
err = cli.loadClientInfo()
274-
if err != nil {
275-
return err
276-
}
277-
cli.initializeFromClient()
278279
return nil
279280
}
280281

@@ -365,7 +366,16 @@ func isEnabled(value string) (bool, error) {
365366
}
366367

367368
func (cli *DockerCli) initializeFromClient() {
368-
ping, err := cli.client.Ping(context.Background())
369+
ctx := context.Background()
370+
if strings.HasPrefix(cli.DockerEndpoint().Host, "tcp://") {
371+
// @FIXME context.WithTimeout doesn't work with connhelper / ssh connections
372+
// time="2020-04-10T10:16:26Z" level=warning msg="commandConn.CloseWrite: commandconn: failed to wait: signal: killed"
373+
var cancel func()
374+
ctx, cancel = context.WithTimeout(ctx, 2*time.Second)
375+
defer cancel()
376+
}
377+
378+
ping, err := cli.client.Ping(ctx)
369379
if err != nil {
370380
// Default to true if we fail to connect to daemon
371381
cli.serverInfo = ServerInfo{HasExperimental: true}

‎cmd/docker/docker.go‎

Lines changed: 65 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -312,63 +312,65 @@ type versionDetails interface {
312312
ServerInfo() command.ServerInfo
313313
}
314314

315-
func hideFeatureFlag(f *pflag.Flag, hasFeature bool, annotation string) {
316-
if hasFeature {
315+
func hideFlagIf(f *pflag.Flag, condition func(string) bool, annotation string) {
316+
if f.Hidden {
317317
return
318318
}
319-
if _, ok := f.Annotations[annotation]; ok {
320-
f.Hidden = true
319+
if values, ok := f.Annotations[annotation]; ok && len(values) > 0 {
320+
if condition(values[0]) {
321+
f.Hidden = true
322+
}
321323
}
322324
}
323325

324-
func hideFeatureSubCommand(subcmd *cobra.Command, hasFeature bool, annotation string) {
325-
if hasFeature {
326+
func hideSubcommandIf(subcmd *cobra.Command, condition func(string) bool, annotation string) {
327+
if subcmd.Hidden {
326328
return
327329
}
328-
if _, ok := subcmd.Annotations[annotation]; ok {
329-
subcmd.Hidden = true
330+
if v, ok := subcmd.Annotations[annotation]; ok {
331+
if condition(v) {
332+
subcmd.Hidden = true
333+
}
330334
}
331335
}
332336

333337
func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) error {
334-
clientVersion := details.Client().ClientVersion()
335-
osType := details.ServerInfo().OSType
336-
hasExperimental := details.ServerInfo().HasExperimental
337-
hasExperimentalCLI := details.ClientInfo().HasExperimental
338-
hasBuildKit, err := command.BuildKitEnabled(details.ServerInfo())
339-
if err != nil {
340-
return err
341-
}
338+
var (
339+
buildKitDisabled = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return !v }
340+
buildKitEnabled = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return v }
341+
notExperimental = func(_ string) bool { return !details.ServerInfo().HasExperimental }
342+
notExperimentalCLI = func(_ string) bool { return !details.ClientInfo().HasExperimental }
343+
notOSType = func(v string) bool { return v != details.ServerInfo().OSType }
344+
versionOlderThan = func(v string) bool { return versions.LessThan(details.Client().ClientVersion(), v) }
345+
)
342346

343347
cmd.Flags().VisitAll(func(f *pflag.Flag) {
344-
hideFeatureFlag(f, hasExperimental, "experimental")
345-
hideFeatureFlag(f, hasExperimentalCLI, "experimentalCLI")
346-
hideFeatureFlag(f, hasBuildKit, "buildkit")
347-
hideFeatureFlag(f, !hasBuildKit, "no-buildkit")
348348
// hide flags not supported by the server
349-
if !isOSTypeSupported(f, osType) || !isVersionSupported(f, clientVersion) {
350-
f.Hidden = true
351-
}
352349
// root command shows all top-level flags
353350
if cmd.Parent() != nil {
354-
if commands, ok := f.Annotations["top-level"]; ok {
355-
f.Hidden = !findCommand(cmd, commands)
351+
if cmds, ok := f.Annotations["top-level"]; ok {
352+
f.Hidden = !findCommand(cmd, cmds)
353+
}
354+
if f.Hidden {
355+
return
356356
}
357357
}
358+
359+
hideFlagIf(f, buildKitDisabled, "buildkit")
360+
hideFlagIf(f, buildKitEnabled, "no-buildkit")
361+
hideFlagIf(f, notExperimental, "experimental")
362+
hideFlagIf(f, notExperimentalCLI, "experimentalCLI")
363+
hideFlagIf(f, notOSType, "ostype")
364+
hideFlagIf(f, versionOlderThan, "version")
358365
})
359366

360367
for _, subcmd := range cmd.Commands() {
361-
hideFeatureSubCommand(subcmd, hasExperimental, "experimental")
362-
hideFeatureSubCommand(subcmd, hasExperimentalCLI, "experimentalCLI")
363-
hideFeatureSubCommand(subcmd, hasBuildKit, "buildkit")
364-
hideFeatureSubCommand(subcmd, !hasBuildKit, "no-buildkit")
365-
// hide subcommands not supported by the server
366-
if subcmdVersion, ok := subcmd.Annotations["version"]; ok && versions.LessThan(clientVersion, subcmdVersion) {
367-
subcmd.Hidden = true
368-
}
369-
if v, ok := subcmd.Annotations["ostype"]; ok && v != osType {
370-
subcmd.Hidden = true
371-
}
368+
hideSubcommandIf(subcmd, buildKitDisabled, "buildkit")
369+
hideSubcommandIf(subcmd, buildKitEnabled, "no-buildkit")
370+
hideSubcommandIf(subcmd, notExperimental, "experimental")
371+
hideSubcommandIf(subcmd, notExperimentalCLI, "experimentalCLI")
372+
hideSubcommandIf(subcmd, notOSType, "ostype")
373+
hideSubcommandIf(subcmd, versionOlderThan, "version")
372374
}
373375
return nil
374376
}
@@ -394,31 +396,31 @@ func isSupported(cmd *cobra.Command, details versionDetails) error {
394396
}
395397

396398
func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {
397-
clientVersion := details.Client().ClientVersion()
398-
osType := details.ServerInfo().OSType
399-
hasExperimental := details.ServerInfo().HasExperimental
400-
hasExperimentalCLI := details.ClientInfo().HasExperimental
401-
402399
errs := []string{}
403400

404401
cmd.Flags().VisitAll(func(f *pflag.Flag) {
405-
if f.Changed {
406-
if !isVersionSupported(f, clientVersion) {
407-
errs = append(errs, fmt.Sprintf("\"--%s\" requires API version %s, but the Docker daemon API version is %s", f.Name, getFlagAnnotation(f, "version"), clientVersion))
408-
return
409-
}
410-
if !isOSTypeSupported(f, osType) {
411-
errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s", f.Name, getFlagAnnotation(f, "ostype"), osType))
412-
return
413-
}
414-
if _, ok := f.Annotations["experimental"]; ok && !hasExperimental {
415-
errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker daemon with experimental features enabled", f.Name))
416-
}
417-
if _, ok := f.Annotations["experimentalCLI"]; ok && !hasExperimentalCLI {
418-
errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker cli with experimental cli features enabled", f.Name))
419-
}
420-
// buildkit-specific flags are noop when buildkit is not enabled, so we do not add an error in that case
402+
if !f.Changed {
403+
return
404+
}
405+
if !isVersionSupported(f, details.Client().ClientVersion()) {
406+
errs = append(errs, fmt.Sprintf(`"--%s" requires API version %s, but the Docker daemon API version is %s`, f.Name, getFlagAnnotation(f, "version"), details.Client().ClientVersion()))
407+
return
408+
}
409+
if !isOSTypeSupported(f, details.ServerInfo().OSType) {
410+
errs = append(errs, fmt.Sprintf(
411+
`"--%s" is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s`,
412+
f.Name,
413+
getFlagAnnotation(f, "ostype"), details.ServerInfo().OSType),
414+
)
415+
return
416+
}
417+
if _, ok := f.Annotations["experimental"]; ok && !details.ServerInfo().HasExperimental {
418+
errs = append(errs, fmt.Sprintf(`"--%s" is only supported on a Docker daemon with experimental features enabled`, f.Name))
421419
}
420+
if _, ok := f.Annotations["experimentalCLI"]; ok && !details.ClientInfo().HasExperimental {
421+
errs = append(errs, fmt.Sprintf(`"--%s" is only supported on a Docker cli with experimental cli features enabled`, f.Name))
422+
}
423+
// buildkit-specific flags are noop when buildkit is not enabled, so we do not add an error in that case
422424
})
423425
if len(errs) > 0 {
424426
return errors.New(strings.Join(errs, "\n"))
@@ -428,23 +430,18 @@ func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {
428430

429431
// Check recursively so that, e.g., `docker stack ls` returns the same output as `docker stack`
430432
func areSubcommandsSupported(cmd *cobra.Command, details versionDetails) error {
431-
clientVersion := details.Client().ClientVersion()
432-
osType := details.ServerInfo().OSType
433-
hasExperimental := details.ServerInfo().HasExperimental
434-
hasExperimentalCLI := details.ClientInfo().HasExperimental
435-
436433
// Check recursively so that, e.g., `docker stack ls` returns the same output as `docker stack`
437434
for curr := cmd; curr != nil; curr = curr.Parent() {
438-
if cmdVersion, ok := curr.Annotations["version"]; ok && versions.LessThan(clientVersion, cmdVersion) {
439-
return fmt.Errorf("%s requires API version %s, but the Docker daemon API version is %s", cmd.CommandPath(), cmdVersion, clientVersion)
435+
if cmdVersion, ok := curr.Annotations["version"]; ok && versions.LessThan(details.Client().ClientVersion(), cmdVersion) {
436+
return fmt.Errorf("%s requires API version %s, but the Docker daemon API version is %s", cmd.CommandPath(), cmdVersion, details.Client().ClientVersion())
440437
}
441-
if os, ok := curr.Annotations["ostype"]; ok && os != osType {
442-
return fmt.Errorf("%s is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s", cmd.CommandPath(), os, osType)
438+
if os, ok := curr.Annotations["ostype"]; ok && os != details.ServerInfo().OSType {
439+
return fmt.Errorf("%s is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s", cmd.CommandPath(), os, details.ServerInfo().OSType)
443440
}
444-
if _, ok := curr.Annotations["experimental"]; ok && !hasExperimental {
441+
if _, ok := curr.Annotations["experimental"]; ok && !details.ServerInfo().HasExperimental {
445442
return fmt.Errorf("%s is only supported on a Docker daemon with experimental features enabled", cmd.CommandPath())
446443
}
447-
if _, ok := curr.Annotations["experimentalCLI"]; ok && !hasExperimentalCLI {
444+
if _, ok := curr.Annotations["experimentalCLI"]; ok && !details.ClientInfo().HasExperimental {
448445
return fmt.Errorf("%s is only supported on a Docker cli with experimental cli features enabled", cmd.CommandPath())
449446
}
450447
}

0 commit comments

Comments
 (0)