From 2389768fb7c6738cceab5ac50dcf6cc8ec7c2c33 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 20 Dec 2023 11:23:05 +0100 Subject: [PATCH 1/3] cli/command/container: runStats(): minor cleanup and fixes - memoize the API-client in a local variable. - use struct-literals in some places. - rename some variables for clarity and to prevent colliding with imports. - make use of the event-constants (events.ContainerEventType). - fix some grammar - fix some minor linting warnings Signed-off-by: Sebastiaan van Stijn --- cli/command/container/stats.go | 92 +++++++++++++++++----------------- 1 file changed, 45 insertions(+), 47 deletions(-) diff --git a/cli/command/container/stats.go b/cli/command/container/stats.go index 1e20445a5d..a972c41225 100644 --- a/cli/command/container/stats.go +++ b/cli/command/container/stats.go @@ -29,29 +29,29 @@ type statsOptions struct { containers []string } -// NewStatsCommand creates a new cobra.Command for `docker stats` -func NewStatsCommand(dockerCli command.Cli) *cobra.Command { - var opts statsOptions +// NewStatsCommand creates a new [cobra.Command] for "docker stats". +func NewStatsCommand(dockerCLI command.Cli) *cobra.Command { + var options statsOptions cmd := &cobra.Command{ Use: "stats [OPTIONS] [CONTAINER...]", Short: "Display a live stream of container(s) resource usage statistics", Args: cli.RequiresMinArgs(0), RunE: func(cmd *cobra.Command, args []string) error { - opts.containers = args - return runStats(cmd.Context(), dockerCli, &opts) + options.containers = args + return runStats(cmd.Context(), dockerCLI, &options) }, Annotations: map[string]string{ "aliases": "docker container stats, docker stats", }, - ValidArgsFunction: completion.ContainerNames(dockerCli, false), + ValidArgsFunction: completion.ContainerNames(dockerCLI, false), } flags := cmd.Flags() - flags.BoolVarP(&opts.all, "all", "a", false, "Show all containers (default shows just running)") - flags.BoolVar(&opts.noStream, "no-stream", false, "Disable streaming stats and only pull the first result") - flags.BoolVar(&opts.noTrunc, "no-trunc", false, "Do not truncate output") - flags.StringVar(&opts.format, "format", "", flagsHelper.FormatHelp) + flags.BoolVarP(&options.all, "all", "a", false, "Show all containers (default shows just running)") + flags.BoolVar(&options.noStream, "no-stream", false, "Disable streaming stats and only pull the first result") + flags.BoolVar(&options.noTrunc, "no-trunc", false, "Do not truncate output") + flags.StringVar(&options.format, "format", "", flagsHelper.FormatHelp) return cmd } @@ -59,22 +59,21 @@ func NewStatsCommand(dockerCli command.Cli) *cobra.Command { // This shows real-time information on CPU usage, memory usage, and network I/O. // //nolint:gocyclo -func runStats(ctx context.Context, dockerCli command.Cli, opts *statsOptions) error { - showAll := len(opts.containers) == 0 +func runStats(ctx context.Context, dockerCLI command.Cli, options *statsOptions) error { + showAll := len(options.containers) == 0 closeChan := make(chan error) + apiClient := dockerCLI.Client() // monitorContainerEvents watches for container creation and removal (only // used when calling `docker stats` without arguments). monitorContainerEvents := func(started chan<- struct{}, c chan events.Message, stopped <-chan struct{}) { f := filters.NewArgs() - f.Add("type", "container") - options := types.EventsOptions{ + f.Add("type", string(events.ContainerEventType)) + eventChan, errChan := apiClient.Events(ctx, types.EventsOptions{ Filters: f, - } + }) - eventq, errq := dockerCli.Client().Events(ctx, options) - - // Whether we successfully subscribed to eventq or not, we can now + // Whether we successfully subscribed to eventChan or not, we can now // unblock the main goroutine. close(started) defer close(c) @@ -83,9 +82,9 @@ func runStats(ctx context.Context, dockerCli command.Cli, opts *statsOptions) er select { case <-stopped: return - case event := <-eventq: + case event := <-eventChan: c <- event - case err := <-errq: + case err := <-errChan: closeChan <- err return } @@ -94,7 +93,7 @@ func runStats(ctx context.Context, dockerCli command.Cli, opts *statsOptions) er // Get the daemonOSType if not set already if daemonOSType == "" { - sv, err := dockerCli.Client().ServerVersion(ctx) + sv, err := apiClient.ServerVersion(ctx) if err != nil { return err } @@ -108,10 +107,9 @@ func runStats(ctx context.Context, dockerCli command.Cli, opts *statsOptions) er // getContainerList simulates creation event for all previously existing // containers (only used when calling `docker stats` without arguments). getContainerList := func() { - options := container.ListOptions{ - All: opts.all, - } - cs, err := dockerCli.Client().ContainerList(ctx, options) + cs, err := apiClient.ContainerList(ctx, container.ListOptions{ + All: options.all, + }) if err != nil { closeChan <- err } @@ -119,24 +117,24 @@ func runStats(ctx context.Context, dockerCli command.Cli, opts *statsOptions) er s := NewStats(ctr.ID[:12]) if cStats.add(s) { waitFirst.Add(1) - go collect(ctx, s, dockerCli.Client(), !opts.noStream, waitFirst) + go collect(ctx, s, apiClient, !options.noStream, waitFirst) } } } if showAll { - // If no names were specified, start a long running goroutine which + // If no names were specified, start a long-running goroutine which // monitors container events. We make sure we're subscribed before // retrieving the list of running containers to avoid a race where we // would "miss" a creation. started := make(chan struct{}) eh := command.InitEventHandler() eh.Handle(events.ActionCreate, func(e events.Message) { - if opts.all { + if options.all { s := NewStats(e.Actor.ID[:12]) if cStats.add(s) { waitFirst.Add(1) - go collect(ctx, s, dockerCli.Client(), !opts.noStream, waitFirst) + go collect(ctx, s, apiClient, !options.noStream, waitFirst) } } }) @@ -145,12 +143,12 @@ func runStats(ctx context.Context, dockerCli command.Cli, opts *statsOptions) er s := NewStats(e.Actor.ID[:12]) if cStats.add(s) { waitFirst.Add(1) - go collect(ctx, s, dockerCli.Client(), !opts.noStream, waitFirst) + go collect(ctx, s, apiClient, !options.noStream, waitFirst) } }) eh.Handle(events.ActionDie, func(e events.Message) { - if !opts.all { + if !options.all { cStats.remove(e.Actor.ID[:12]) } }) @@ -171,11 +169,11 @@ func runStats(ctx context.Context, dockerCli command.Cli, opts *statsOptions) er } else { // Artificially send creation events for the containers we were asked to // monitor (same code path than we use when monitoring all containers). - for _, name := range opts.containers { + for _, name := range options.containers { s := NewStats(name) if cStats.add(s) { waitFirst.Add(1) - go collect(ctx, s, dockerCli.Client(), !opts.noStream, waitFirst) + go collect(ctx, s, apiClient, !options.noStream, waitFirst) } } @@ -198,22 +196,22 @@ func runStats(ctx context.Context, dockerCli command.Cli, opts *statsOptions) er } } - format := opts.format + format := options.format if len(format) == 0 { - if len(dockerCli.ConfigFile().StatsFormat) > 0 { - format = dockerCli.ConfigFile().StatsFormat + if len(dockerCLI.ConfigFile().StatsFormat) > 0 { + format = dockerCLI.ConfigFile().StatsFormat } else { format = formatter.TableFormatKey } } statsCtx := formatter.Context{ - Output: dockerCli.Out(), + Output: dockerCLI.Out(), Format: NewStatsFormat(format, daemonOSType), } cleanScreen := func() { - if !opts.noStream { - fmt.Fprint(dockerCli.Out(), "\033[2J") - fmt.Fprint(dockerCli.Out(), "\033[H") + if !options.noStream { + _, _ = fmt.Fprint(dockerCLI.Out(), "\033[2J") + _, _ = fmt.Fprint(dockerCLI.Out(), "\033[H") } } @@ -222,28 +220,28 @@ func runStats(ctx context.Context, dockerCli command.Cli, opts *statsOptions) er defer ticker.Stop() for range ticker.C { cleanScreen() - ccstats := []StatsEntry{} + var ccStats []StatsEntry cStats.mu.RLock() for _, c := range cStats.cs { - ccstats = append(ccstats, c.GetStatistics()) + ccStats = append(ccStats, c.GetStatistics()) } cStats.mu.RUnlock() - if err = statsFormatWrite(statsCtx, ccstats, daemonOSType, !opts.noTrunc); err != nil { + if err = statsFormatWrite(statsCtx, ccStats, daemonOSType, !options.noTrunc); err != nil { break } if len(cStats.cs) == 0 && !showAll { break } - if opts.noStream { + if options.noStream { break } select { case err, ok := <-closeChan: if ok { if err != nil { - // this is suppressing "unexpected EOF" in the cli when the - // daemon restarts so it shutdowns cleanly - if err == io.ErrUnexpectedEOF { + // Suppress "unexpected EOF" errors in the CLI so that + // it shuts down cleanly when the daemon restarts. + if errors.Is(err, io.ErrUnexpectedEOF) { return nil } return err From 5ed6c128e8d89bea0e37d42414118a047d086196 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 20 Dec 2023 11:34:34 +0100 Subject: [PATCH 2/3] cli/command/container: runStats(): don't register unused event handlers We were unconditionally registering event-handlers for these events, but the handler itself would ignore the event depending on the "all" option. This patch skips registering the event handlers, so that we're not handling them (saving some resources). Signed-off-by: Sebastiaan van Stijn --- cli/command/container/stats.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/cli/command/container/stats.go b/cli/command/container/stats.go index a972c41225..d84126593e 100644 --- a/cli/command/container/stats.go +++ b/cli/command/container/stats.go @@ -129,15 +129,15 @@ func runStats(ctx context.Context, dockerCLI command.Cli, options *statsOptions) // would "miss" a creation. started := make(chan struct{}) eh := command.InitEventHandler() - eh.Handle(events.ActionCreate, func(e events.Message) { - if options.all { + if options.all { + eh.Handle(events.ActionCreate, func(e events.Message) { s := NewStats(e.Actor.ID[:12]) if cStats.add(s) { waitFirst.Add(1) go collect(ctx, s, apiClient, !options.noStream, waitFirst) } - } - }) + }) + } eh.Handle(events.ActionStart, func(e events.Message) { s := NewStats(e.Actor.ID[:12]) @@ -147,11 +147,11 @@ func runStats(ctx context.Context, dockerCLI command.Cli, options *statsOptions) } }) - eh.Handle(events.ActionDie, func(e events.Message) { - if !options.all { + if !options.all { + eh.Handle(events.ActionDie, func(e events.Message) { cStats.remove(e.Actor.ID[:12]) - } - }) + }) + } eventChan := make(chan events.Message) go eh.Watch(eventChan) From 633ba88c26350674ce6f291a829ea37a0914e45c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 20 Dec 2023 11:54:12 +0100 Subject: [PATCH 3/3] cli/command/container: runStats(): move code to where it's used The monitorContainerEvents and getContainerList closures where only used when collecting "all" containers, so let's define them in that branch of the code. Also move some of the other variables closer to where they're used. Signed-off-by: Sebastiaan van Stijn --- cli/command/container/stats.go | 94 +++++++++++++++++----------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/cli/command/container/stats.go b/cli/command/container/stats.go index d84126593e..6141ba1380 100644 --- a/cli/command/container/stats.go +++ b/cli/command/container/stats.go @@ -60,37 +60,8 @@ func NewStatsCommand(dockerCLI command.Cli) *cobra.Command { // //nolint:gocyclo func runStats(ctx context.Context, dockerCLI command.Cli, options *statsOptions) error { - showAll := len(options.containers) == 0 - closeChan := make(chan error) apiClient := dockerCLI.Client() - // monitorContainerEvents watches for container creation and removal (only - // used when calling `docker stats` without arguments). - monitorContainerEvents := func(started chan<- struct{}, c chan events.Message, stopped <-chan struct{}) { - f := filters.NewArgs() - f.Add("type", string(events.ContainerEventType)) - eventChan, errChan := apiClient.Events(ctx, types.EventsOptions{ - Filters: f, - }) - - // Whether we successfully subscribed to eventChan or not, we can now - // unblock the main goroutine. - close(started) - defer close(c) - - for { - select { - case <-stopped: - return - case event := <-eventChan: - c <- event - case err := <-errChan: - closeChan <- err - return - } - } - } - // Get the daemonOSType if not set already if daemonOSType == "" { sv, err := apiClient.ServerVersion(ctx) @@ -102,26 +73,10 @@ func runStats(ctx context.Context, dockerCLI command.Cli, options *statsOptions) // waitFirst is a WaitGroup to wait first stat data's reach for each container waitFirst := &sync.WaitGroup{} - + closeChan := make(chan error) cStats := stats{} - // getContainerList simulates creation event for all previously existing - // containers (only used when calling `docker stats` without arguments). - getContainerList := func() { - cs, err := apiClient.ContainerList(ctx, container.ListOptions{ - All: options.all, - }) - if err != nil { - closeChan <- err - } - for _, ctr := range cs { - s := NewStats(ctr.ID[:12]) - if cStats.add(s) { - waitFirst.Add(1) - go collect(ctx, s, apiClient, !options.noStream, waitFirst) - } - } - } + showAll := len(options.containers) == 0 if showAll { // If no names were specified, start a long-running goroutine which // monitors container events. We make sure we're subscribed before @@ -153,6 +108,51 @@ func runStats(ctx context.Context, dockerCLI command.Cli, options *statsOptions) }) } + // monitorContainerEvents watches for container creation and removal (only + // used when calling `docker stats` without arguments). + monitorContainerEvents := func(started chan<- struct{}, c chan events.Message, stopped <-chan struct{}) { + f := filters.NewArgs() + f.Add("type", string(events.ContainerEventType)) + eventChan, errChan := apiClient.Events(ctx, types.EventsOptions{ + Filters: f, + }) + + // Whether we successfully subscribed to eventChan or not, we can now + // unblock the main goroutine. + close(started) + defer close(c) + + for { + select { + case <-stopped: + return + case event := <-eventChan: + c <- event + case err := <-errChan: + closeChan <- err + return + } + } + } + + // getContainerList simulates creation event for all previously existing + // containers (only used when calling `docker stats` without arguments). + getContainerList := func() { + cs, err := apiClient.ContainerList(ctx, container.ListOptions{ + All: options.all, + }) + if err != nil { + closeChan <- err + } + for _, ctr := range cs { + s := NewStats(ctr.ID[:12]) + if cStats.add(s) { + waitFirst.Add(1) + go collect(ctx, s, apiClient, !options.noStream, waitFirst) + } + } + } + eventChan := make(chan events.Message) go eh.Watch(eventChan) stopped := make(chan struct{})