Skip to content

Commit

Permalink
cli: perform feature detection lazily
Browse files Browse the repository at this point in the history 
- 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 <github@gone.nl>
(cherry picked from commit b397391)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
  • Loading branch information
@thaJeztah
thaJeztah committed Apr 29, 2020
1 parent c44c18e commit cf663b5
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 74 deletions.
22 changes: 16 additions & 6 deletions cli/command/cli.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import (
"path/filepath"
"runtime"
"strconv"
"strings"
"sync"
"time"

"github.com/docker/cli/cli/config"
cliconfig "github.com/docker/cli/cli/config"
Expand Down Expand Up @@ -137,9 +140,12 @@ func (cli *DockerCli) loadConfigFile() {
cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err)
}

var fetchServerInfo sync.Once

// ServerInfo returns the server version details for the host this client is
// connected to
func (cli *DockerCli) ServerInfo() ServerInfo {
fetchServerInfo.Do(cli.initializeFromClient)
return cli.serverInfo
}

Expand Down Expand Up @@ -274,11 +280,6 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize
return err
}
}
err = cli.loadClientInfo()
if err != nil {
return err
}
cli.initializeFromClient()
return nil
}

Expand Down Expand Up @@ -369,7 +370,16 @@ func isEnabled(value string) (bool, error) {
}

func (cli *DockerCli) initializeFromClient() {
ping, err := cli.client.Ping(context.Background())
ctx := context.Background()
if strings.HasPrefix(cli.DockerEndpoint().Host, "tcp://") {
// @FIXME context.WithTimeout doesn't work with connhelper / ssh connections
// time="2020-04-10T10:16:26Z" level=warning msg="commandConn.CloseWrite: commandconn: failed to wait: signal: killed"
var cancel func()
ctx, cancel = context.WithTimeout(ctx, 2*time.Second)
defer cancel()
}

ping, err := cli.client.Ping(ctx)
if err != nil {
// Default to true if we fail to connect to daemon
cli.serverInfo = ServerInfo{HasExperimental: true}
Expand Down
133 changes: 65 additions & 68 deletions cmd/docker/docker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,63 +312,65 @@ type versionDetails interface {
ServerInfo() command.ServerInfo
}

func hideFeatureFlag(f *pflag.Flag, hasFeature bool, annotation string) {
if hasFeature {
func hideFlagIf(f *pflag.Flag, condition func(string) bool, annotation string) {
if f.Hidden {
return
}
if _, ok := f.Annotations[annotation]; ok {
f.Hidden = true
if values, ok := f.Annotations[annotation]; ok && len(values) > 0 {
if condition(values[0]) {
f.Hidden = true
}
}
}

func hideFeatureSubCommand(subcmd *cobra.Command, hasFeature bool, annotation string) {
if hasFeature {
func hideSubcommandIf(subcmd *cobra.Command, condition func(string) bool, annotation string) {
if subcmd.Hidden {
return
}
if _, ok := subcmd.Annotations[annotation]; ok {
subcmd.Hidden = true
if v, ok := subcmd.Annotations[annotation]; ok {
if condition(v) {
subcmd.Hidden = true
}
}
}

func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) error {
clientVersion := details.Client().ClientVersion()
osType := details.ServerInfo().OSType
hasExperimental := details.ServerInfo().HasExperimental
hasExperimentalCLI := details.ClientInfo().HasExperimental
hasBuildKit, err := command.BuildKitEnabled(details.ServerInfo())
if err != nil {
return err
}
var (
buildKitDisabled = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return !v }
buildKitEnabled = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return v }
notExperimental = func(_ string) bool { return !details.ServerInfo().HasExperimental }
notExperimentalCLI = func(_ string) bool { return !details.ClientInfo().HasExperimental }
notOSType = func(v string) bool { return v != details.ServerInfo().OSType }
versionOlderThan = func(v string) bool { return versions.LessThan(details.Client().ClientVersion(), v) }
)

cmd.Flags().VisitAll(func(f *pflag.Flag) {
hideFeatureFlag(f, hasExperimental, "experimental")
hideFeatureFlag(f, hasExperimentalCLI, "experimentalCLI")
hideFeatureFlag(f, hasBuildKit, "buildkit")
hideFeatureFlag(f, !hasBuildKit, "no-buildkit")
// hide flags not supported by the server
if !isOSTypeSupported(f, osType) || !isVersionSupported(f, clientVersion) {
f.Hidden = true
}
// root command shows all top-level flags
if cmd.Parent() != nil {
if commands, ok := f.Annotations["top-level"]; ok {
f.Hidden = !findCommand(cmd, commands)
if cmds, ok := f.Annotations["top-level"]; ok {
f.Hidden = !findCommand(cmd, cmds)
}
if f.Hidden {
return
}
}

hideFlagIf(f, buildKitDisabled, "buildkit")
hideFlagIf(f, buildKitEnabled, "no-buildkit")
hideFlagIf(f, notExperimental, "experimental")
hideFlagIf(f, notExperimentalCLI, "experimentalCLI")
hideFlagIf(f, notOSType, "ostype")
hideFlagIf(f, versionOlderThan, "version")
})

for _, subcmd := range cmd.Commands() {
hideFeatureSubCommand(subcmd, hasExperimental, "experimental")
hideFeatureSubCommand(subcmd, hasExperimentalCLI, "experimentalCLI")
hideFeatureSubCommand(subcmd, hasBuildKit, "buildkit")
hideFeatureSubCommand(subcmd, !hasBuildKit, "no-buildkit")
// hide subcommands not supported by the server
if subcmdVersion, ok := subcmd.Annotations["version"]; ok && versions.LessThan(clientVersion, subcmdVersion) {
subcmd.Hidden = true
}
if v, ok := subcmd.Annotations["ostype"]; ok && v != osType {
subcmd.Hidden = true
}
hideSubcommandIf(subcmd, buildKitDisabled, "buildkit")
hideSubcommandIf(subcmd, buildKitEnabled, "no-buildkit")
hideSubcommandIf(subcmd, notExperimental, "experimental")
hideSubcommandIf(subcmd, notExperimentalCLI, "experimentalCLI")
hideSubcommandIf(subcmd, notOSType, "ostype")
hideSubcommandIf(subcmd, versionOlderThan, "version")
}
return nil
}
Expand All @@ -394,31 +396,31 @@ func isSupported(cmd *cobra.Command, details versionDetails) error {
}

func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {
clientVersion := details.Client().ClientVersion()
osType := details.ServerInfo().OSType
hasExperimental := details.ServerInfo().HasExperimental
hasExperimentalCLI := details.ClientInfo().HasExperimental

errs := []string{}

cmd.Flags().VisitAll(func(f *pflag.Flag) {
if f.Changed {
if !isVersionSupported(f, clientVersion) {
errs = append(errs, fmt.Sprintf("\"--%s\" requires API version %s, but the Docker daemon API version is %s", f.Name, getFlagAnnotation(f, "version"), clientVersion))
return
}
if !isOSTypeSupported(f, osType) {
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))
return
}
if _, ok := f.Annotations["experimental"]; ok && !hasExperimental {
errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker daemon with experimental features enabled", f.Name))
}
if _, ok := f.Annotations["experimentalCLI"]; ok && !hasExperimentalCLI {
errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker cli with experimental cli features enabled", f.Name))
}
// buildkit-specific flags are noop when buildkit is not enabled, so we do not add an error in that case
if !f.Changed {
return
}
if !isVersionSupported(f, details.Client().ClientVersion()) {
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()))
return
}
if !isOSTypeSupported(f, details.ServerInfo().OSType) {
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"), details.ServerInfo().OSType),
)
return
}
if _, ok := f.Annotations["experimental"]; ok && !details.ServerInfo().HasExperimental {
errs = append(errs, fmt.Sprintf(`"--%s" is only supported on a Docker daemon with experimental features enabled`, f.Name))
}
if _, ok := f.Annotations["experimentalCLI"]; ok && !details.ClientInfo().HasExperimental {
errs = append(errs, fmt.Sprintf(`"--%s" is only supported on a Docker cli with experimental cli features enabled`, f.Name))
}
// buildkit-specific flags are noop when buildkit is not enabled, so we do not add an error in that case
})
if len(errs) > 0 {
return errors.New(strings.Join(errs, "\n"))
Expand All @@ -428,23 +430,18 @@ func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {

// Check recursively so that, e.g., `docker stack ls` returns the same output as `docker stack`
func areSubcommandsSupported(cmd *cobra.Command, details versionDetails) error {
clientVersion := details.Client().ClientVersion()
osType := details.ServerInfo().OSType
hasExperimental := details.ServerInfo().HasExperimental
hasExperimentalCLI := details.ClientInfo().HasExperimental

// Check recursively so that, e.g., `docker stack ls` returns the same output as `docker stack`
for curr := cmd; curr != nil; curr = curr.Parent() {
if cmdVersion, ok := curr.Annotations["version"]; ok && versions.LessThan(clientVersion, cmdVersion) {
return fmt.Errorf("%s requires API version %s, but the Docker daemon API version is %s", cmd.CommandPath(), cmdVersion, clientVersion)
if cmdVersion, ok := curr.Annotations["version"]; ok && versions.LessThan(details.Client().ClientVersion(), cmdVersion) {
return fmt.Errorf("%s requires API version %s, but the Docker daemon API version is %s", cmd.CommandPath(), cmdVersion, details.Client().ClientVersion())
}
if os, ok := curr.Annotations["ostype"]; ok && os != osType {
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)
if os, ok := curr.Annotations["ostype"]; ok && os != details.ServerInfo().OSType {
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)
}
if _, ok := curr.Annotations["experimental"]; ok && !hasExperimental {
if _, ok := curr.Annotations["experimental"]; ok && !details.ServerInfo().HasExperimental {
return fmt.Errorf("%s is only supported on a Docker daemon with experimental features enabled", cmd.CommandPath())
}
if _, ok := curr.Annotations["experimentalCLI"]; ok && !hasExperimentalCLI {
if _, ok := curr.Annotations["experimentalCLI"]; ok && !details.ClientInfo().HasExperimental {
return fmt.Errorf("%s is only supported on a Docker cli with experimental cli features enabled", cmd.CommandPath())
}
}
Expand Down

0 comments on commit cf663b5

Please sign in to comment.