Merge pull request #4129 from crazy-max/fix-perf-reg-2

improve and load plugin command stubs when required

cli-plugins/manager/cobra.go

@@ -3,6 +3,7 @@ package manager
 import (
 	"fmt"
 	"os"
+	"sync"
 
 	"github.com/docker/cli/cli/command"
 	"github.com/spf13/cobra"
@@ -31,64 +32,69 @@ const (
 	CommandAnnotationPluginInvalid = "com.docker.cli.plugin-invalid"
 )
 
+var pluginCommandStubsOnce sync.Once
+
 // AddPluginCommandStubs adds a stub cobra.Commands for each valid and invalid
 // plugin. The command stubs will have several annotations added, see
 // `CommandAnnotationPlugin*`.
-func AddPluginCommandStubs(dockerCli command.Cli, rootCmd *cobra.Command) error {
+func AddPluginCommandStubs(dockerCli command.Cli, rootCmd *cobra.Command) (err error) {
-	plugins, err := ListPlugins(dockerCli, rootCmd)
+	pluginCommandStubsOnce.Do(func() {
-	if err != nil {
+		var plugins []Plugin
-		return err
+		plugins, err = ListPlugins(dockerCli, rootCmd)
-	}
+		if err != nil {
-	for _, p := range plugins {
+			return
-		p := p
-		vendor := p.Vendor
-		if vendor == "" {
-			vendor = "unknown"
 		}
-		annotations := map[string]string{
+		for _, p := range plugins {
-			CommandAnnotationPlugin:        "true",
+			p := p
-			CommandAnnotationPluginVendor:  vendor,
+			vendor := p.Vendor
-			CommandAnnotationPluginVersion: p.Version,
+			if vendor == "" {
-		}
+				vendor = "unknown"
-		if p.Err != nil {
+			}
-			annotations[CommandAnnotationPluginInvalid] = p.Err.Error()
+			annotations := map[string]string{
-		}
+				CommandAnnotationPlugin:        "true",
-		rootCmd.AddCommand(&cobra.Command{
+				CommandAnnotationPluginVendor:  vendor,
-			Use:                p.Name,
+				CommandAnnotationPluginVersion: p.Version,
-			Short:              p.ShortDescription,
+			}
-			Run:                func(_ *cobra.Command, _ []string) {},
+			if p.Err != nil {
-			Annotations:        annotations,
+				annotations[CommandAnnotationPluginInvalid] = p.Err.Error()
-			DisableFlagParsing: true,
+			}
-			RunE: func(cmd *cobra.Command, args []string) error {
+			rootCmd.AddCommand(&cobra.Command{
-				flags := rootCmd.PersistentFlags()
+				Use:                p.Name,
-				flags.SetOutput(nil)
+				Short:              p.ShortDescription,
-				err := flags.Parse(args)
+				Run:                func(_ *cobra.Command, _ []string) {},
-				if err != nil {
+				Annotations:        annotations,
-					return err
+				DisableFlagParsing: true,
-				}
+				RunE: func(cmd *cobra.Command, args []string) error {
-				if flags.Changed("help") {
+					flags := rootCmd.PersistentFlags()
-					cmd.HelpFunc()(rootCmd, args)
+					flags.SetOutput(nil)
-					return nil
+					perr := flags.Parse(args)
-				}
+					if perr != nil {
-				return fmt.Errorf("docker: '%s' is not a docker command.\nSee 'docker --help'", cmd.Name())
+						return err
-			},
+					}
-			ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
+					if flags.Changed("help") {
-				// Delegate completion to plugin
+						cmd.HelpFunc()(rootCmd, args)
-				cargs := []string{p.Path, cobra.ShellCompRequestCmd, p.Name}
+						return nil
-				cargs = append(cargs, args...)
+					}
-				cargs = append(cargs, toComplete)
+					return fmt.Errorf("docker: '%s' is not a docker command.\nSee 'docker --help'", cmd.Name())
-				os.Args = cargs
+				},
-				runCommand, err := PluginRunCommand(dockerCli, p.Name, cmd)
+				ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
-				if err != nil {
+					// Delegate completion to plugin
+					cargs := []string{p.Path, cobra.ShellCompRequestCmd, p.Name}
+					cargs = append(cargs, args...)
+					cargs = append(cargs, toComplete)
+					os.Args = cargs
+					runCommand, runErr := PluginRunCommand(dockerCli, p.Name, cmd)
+					if runErr != nil {
+						return nil, cobra.ShellCompDirectiveError
+					}
+					runErr = runCommand.Run()
+					if runErr == nil {
+						os.Exit(0) // plugin already rendered complete data
+					}
 					return nil, cobra.ShellCompDirectiveError
-				}
+				},
-				err = runCommand.Run()
+			})
-				if err == nil {
+		}
-					os.Exit(0) // plugin already rendered complete data
+	})
-				}
+	return err
-				return nil, cobra.ShellCompDirectiveError
-			},
-		})
-	}
-	return nil
 }

cli-plugins/manager/manager.go

@@ -1,15 +1,18 @@
 package manager
 
 import (
+	"context"
 	"os"
 	"path/filepath"
 	"sort"
 	"strings"
+	"sync"
 
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/config"
 	"github.com/fvbommel/sortorder"
 	"github.com/spf13/cobra"
+	"golang.org/x/sync/errgroup"
 	exec "golang.org/x/sys/execabs"
 )
 
@@ -146,19 +149,31 @@ func ListPlugins(dockerCli command.Cli, rootcmd *cobra.Command) ([]Plugin, error
 	}
 
 	var plugins []Plugin
+	var mu sync.Mutex
+	eg, _ := errgroup.WithContext(context.TODO())
 	for _, paths := range candidates {
-		if len(paths) == 0 {
+		func(paths []string) {
-			continue
+			eg.Go(func() error {
-		}
+				if len(paths) == 0 {
-		c := &candidate{paths[0]}
+					return nil
-		p, err := newPlugin(c, rootcmd)
+				}
-		if err != nil {
+				c := &candidate{paths[0]}
-			return nil, err
+				p, err := newPlugin(c, rootcmd)
-		}
+				if err != nil {
-		if !IsNotFound(p.Err) {
+					return err
-			p.ShadowedPaths = paths[1:]
+				}
-			plugins = append(plugins, p)
+				if !IsNotFound(p.Err) {
-		}
+					p.ShadowedPaths = paths[1:]
+					mu.Lock()
+					defer mu.Unlock()
+					plugins = append(plugins, p)
+				}
+				return nil
+			})
+		}(paths)
+	}
+	if err := eg.Wait(); err != nil {
+		return nil, err
 	}
 
 	sort.Slice(plugins, func(i, j int) bool {

cli/cobra.go

@@ -204,6 +204,16 @@ func DisableFlagsInUseLine(cmd *cobra.Command) {
 	})
 }
 
+// HasCompletionArg returns true if a cobra completion arg request is found.
+func HasCompletionArg(args []string) bool {
+	for _, arg := range args {
+		if arg == cobra.ShellCompRequestCmd || arg == cobra.ShellCompNoDescRequestCmd {
+			return true
+		}
+	}
+	return false
+}
+
 var helpCommand = &cobra.Command{
 	Use:               "help [command]",
 	Short:             "Help about the command",

cmd/docker/docker.go

@@ -133,13 +133,20 @@ func tryRunPluginHelp(dockerCli command.Cli, ccmd *cobra.Command, cargs []string
 func setHelpFunc(dockerCli command.Cli, cmd *cobra.Command) {
 	defaultHelpFunc := cmd.HelpFunc()
 	cmd.SetHelpFunc(func(ccmd *cobra.Command, args []string) {
-		if pluginmanager.IsPluginCommand(ccmd) {
+		if err := pluginmanager.AddPluginCommandStubs(dockerCli, ccmd.Root()); err != nil {
+			ccmd.Println(err)
+			return
+		}
+
+		if len(args) >= 1 {
 			err := tryRunPluginHelp(dockerCli, ccmd, args)
+			if err == nil {
+				return
+			}
 			if !pluginmanager.IsNotFound(err) {
 				ccmd.Println(err)
+				return
 			}
-			cmd.PrintErrf("unknown help topic: %v\n", ccmd.Name())
-			return
 		}
 
 		if err := isSupported(ccmd, dockerCli); err != nil {
@@ -227,9 +234,14 @@ func runDocker(dockerCli *command.DockerCli) error {
 		return err
 	}
 
-	err = pluginmanager.AddPluginCommandStubs(dockerCli, cmd)
+	if cli.HasCompletionArg(args) {
-	if err != nil {
+		// We add plugin command stubs early only for completion. We don't
-		return err
+		// want to add them for normal command execution as it would cause
+		// a significant performance hit.
+		err = pluginmanager.AddPluginCommandStubs(dockerCli, cmd)
+		if err != nil {
+			return err
+		}
 	}
 
 	if len(args) > 0 {

e2e/cli-plugins/run_test.go

@@ -83,7 +83,7 @@ func TestHelpBad(t *testing.T) {
 
 	res := icmd.RunCmd(run("help", "badmeta"))
 	res.Assert(t, icmd.Expected{
-		ExitCode: 0,
+		ExitCode: 1,
 		Out:      icmd.None,
 	})
 	golden.Assert(t, res.Stderr(), "docker-help-badmeta-err.golden")
@@ -110,8 +110,8 @@ func TestBadHelp(t *testing.T) {
 	res.Assert(t, icmd.Expected{
 		ExitCode: 0,
 		// This should be identical to the --help case above
-		Out: usage,
+		Out: shortHFlagDeprecated + usage,
-		Err: shortHFlagDeprecated,
+		Err: icmd.None,
 	})
 }
 

vendor.mod

@@ -37,6 +37,7 @@ require (
 	github.com/theupdateframework/notary v0.7.1-0.20210315103452-bf96a202a09a
 	github.com/tonistiigi/go-rosetta v0.0.0-20200727161949-f79598599c5d
 	github.com/xeipuuv/gojsonschema v1.2.0
+	golang.org/x/sync v0.1.0
 	golang.org/x/sys v0.5.0
 	golang.org/x/term v0.5.0
 	golang.org/x/text v0.7.0

vendor.sum

@@ -524,6 +524,8 @@ golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a/go.mod h1:RxMgew5VJxzue5/jJ
 golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
+golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o=
+golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=

vendor/golang.org/x/sync/LICENSE

@@ -0,0 +1,27 @@
+Copyright (c) 2009 The Go Authors. All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are
+met:
+
+   * Redistributions of source code must retain the above copyright
+notice, this list of conditions and the following disclaimer.
+   * Redistributions in binary form must reproduce the above
+copyright notice, this list of conditions and the following disclaimer
+in the documentation and/or other materials provided with the
+distribution.
+   * Neither the name of Google Inc. nor the names of its
+contributors may be used to endorse or promote products derived from
+this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

vendor/golang.org/x/sync/PATENTS

@@ -0,0 +1,22 @@
+Additional IP Rights Grant (Patents)
+
+"This implementation" means the copyrightable works distributed by
+Google as part of the Go project.
+
+Google hereby grants to You a perpetual, worldwide, non-exclusive,
+no-charge, royalty-free, irrevocable (except as stated in this section)
+patent license to make, have made, use, offer to sell, sell, import,
+transfer and otherwise run, modify and propagate the contents of this
+implementation of Go, where such license applies only to those patent
+claims, both currently owned or controlled by Google and acquired in
+the future, licensable by Google that are necessarily infringed by this
+implementation of Go.  This grant does not include claims that would be
+infringed only as a consequence of further modification of this
+implementation.  If you or your agent or exclusive licensee institute or
+order or agree to the institution of patent litigation against any
+entity (including a cross-claim or counterclaim in a lawsuit) alleging
+that this implementation of Go or any code incorporated within this
+implementation of Go constitutes direct or contributory patent
+infringement, or inducement of patent infringement, then any patent
+rights granted to you under this License for this implementation of Go
+shall terminate as of the date such litigation is filed.

vendor/golang.org/x/sync/errgroup/errgroup.go

@@ -0,0 +1,132 @@
+// Copyright 2016 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package errgroup provides synchronization, error propagation, and Context
+// cancelation for groups of goroutines working on subtasks of a common task.
+package errgroup
+
+import (
+	"context"
+	"fmt"
+	"sync"
+)
+
+type token struct{}
+
+// A Group is a collection of goroutines working on subtasks that are part of
+// the same overall task.
+//
+// A zero Group is valid, has no limit on the number of active goroutines,
+// and does not cancel on error.
+type Group struct {
+	cancel func()
+
+	wg sync.WaitGroup
+
+	sem chan token
+
+	errOnce sync.Once
+	err     error
+}
+
+func (g *Group) done() {
+	if g.sem != nil {
+		<-g.sem
+	}
+	g.wg.Done()
+}
+
+// WithContext returns a new Group and an associated Context derived from ctx.
+//
+// The derived Context is canceled the first time a function passed to Go
+// returns a non-nil error or the first time Wait returns, whichever occurs
+// first.
+func WithContext(ctx context.Context) (*Group, context.Context) {
+	ctx, cancel := context.WithCancel(ctx)
+	return &Group{cancel: cancel}, ctx
+}
+
+// Wait blocks until all function calls from the Go method have returned, then
+// returns the first non-nil error (if any) from them.
+func (g *Group) Wait() error {
+	g.wg.Wait()
+	if g.cancel != nil {
+		g.cancel()
+	}
+	return g.err
+}
+
+// Go calls the given function in a new goroutine.
+// It blocks until the new goroutine can be added without the number of
+// active goroutines in the group exceeding the configured limit.
+//
+// The first call to return a non-nil error cancels the group's context, if the
+// group was created by calling WithContext. The error will be returned by Wait.
+func (g *Group) Go(f func() error) {
+	if g.sem != nil {
+		g.sem <- token{}
+	}
+
+	g.wg.Add(1)
+	go func() {
+		defer g.done()
+
+		if err := f(); err != nil {
+			g.errOnce.Do(func() {
+				g.err = err
+				if g.cancel != nil {
+					g.cancel()
+				}
+			})
+		}
+	}()
+}
+
+// TryGo calls the given function in a new goroutine only if the number of
+// active goroutines in the group is currently below the configured limit.
+//
+// The return value reports whether the goroutine was started.
+func (g *Group) TryGo(f func() error) bool {
+	if g.sem != nil {
+		select {
+		case g.sem <- token{}:
+			// Note: this allows barging iff channels in general allow barging.
+		default:
+			return false
+		}
+	}
+
+	g.wg.Add(1)
+	go func() {
+		defer g.done()
+
+		if err := f(); err != nil {
+			g.errOnce.Do(func() {
+				g.err = err
+				if g.cancel != nil {
+					g.cancel()
+				}
+			})
+		}
+	}()
+	return true
+}
+
+// SetLimit limits the number of active goroutines in this group to at most n.
+// A negative value indicates no limit.
+//
+// Any subsequent call to the Go method will block until it can add an active
+// goroutine without exceeding the configured limit.
+//
+// The limit must not be modified while any goroutines in the group are active.
+func (g *Group) SetLimit(n int) {
+	if n < 0 {
+		g.sem = nil
+		return
+	}
+	if len(g.sem) != 0 {
+		panic(fmt.Errorf("errgroup: modify limit while %v goroutines in the group are still active", len(g.sem)))
+	}
+	g.sem = make(chan token, n)
+}

vendor/modules.txt

@@ -276,6 +276,9 @@ golang.org/x/net/internal/socks
 golang.org/x/net/internal/timeseries
 golang.org/x/net/proxy
 golang.org/x/net/trace
+# golang.org/x/sync v0.1.0
+## explicit
+golang.org/x/sync/errgroup
 # golang.org/x/sys v0.5.0
 ## explicit; go 1.17
 golang.org/x/sys/execabs