From 376ba349bfadc47a938084ccb3c2d112c92f09ca Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 23 Sep 2020 13:32:58 +0200 Subject: stats refactor Refactor the entities' stats API to simplify using it and reduce the risk of running into concurrency issues at the call sites. Further simplify the stats code by de-spaghetti-ing the logic and reducing duplicate code. `ContainerStats` now returns a data channel and an error. If the error is nil, callers can read from the channel. Signed-off-by: Valentin Rothberg --- cmd/podman/containers/stats.go | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) (limited to 'cmd') diff --git a/cmd/podman/containers/stats.go b/cmd/podman/containers/stats.go index ddb5f32ef..1a4adb376 100644 --- a/cmd/podman/containers/stats.go +++ b/cmd/podman/containers/stats.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "strings" - "sync" "text/tabwriter" "text/template" @@ -107,32 +106,31 @@ func stats(cmd *cobra.Command, args []string) error { return errors.New("stats is not supported in rootless mode without cgroups v2") } } - statsOptions.StatChan = make(chan []*define.ContainerStats, 1) - wg := sync.WaitGroup{} - wg.Add(1) - go func() { - for reports := range statsOptions.StatChan { - if err := outputStats(reports); err != nil { - logrus.Error(err) - } - } - wg.Done() - }() - err := registry.ContainerEngine().ContainerStats(registry.Context(), args, statsOptions) - wg.Wait() - return err + statsChan, err := registry.ContainerEngine().ContainerStats(registry.Context(), args, statsOptions) + if err != nil { + return err + } + for report := range statsChan { + if report.Error != nil { + return report.Error + } + if err := outputStats(report.Stats); err != nil { + logrus.Error(err) + } + } + return nil } -func outputStats(reports []*define.ContainerStats) error { +func outputStats(reports []define.ContainerStats) error { if len(statsOptions.Format) < 1 && !statsOptions.NoReset { tm.Clear() tm.MoveCursor(1, 1) tm.Flush() } - stats := make([]*containerStats, 0, len(reports)) + stats := make([]containerStats, 0, len(reports)) for _, r := range reports { - stats = append(stats, &containerStats{r}) + stats = append(stats, containerStats{r}) } if statsOptions.Format == "json" { return outputJSON(stats) @@ -163,7 +161,7 @@ func outputStats(reports []*define.ContainerStats) error { } type containerStats struct { - *define.ContainerStats + define.ContainerStats } func (s *containerStats) ID() string { @@ -213,7 +211,7 @@ func combineHumanValues(a, b uint64) string { return fmt.Sprintf("%s / %s", units.HumanSize(float64(a)), units.HumanSize(float64(b))) } -func outputJSON(stats []*containerStats) error { +func outputJSON(stats []containerStats) error { type jstat struct { Id string `json:"id"` //nolint Name string `json:"name"` -- cgit v1.2.3-54-g00ecf From 19b955f0999f7fe9e187e94d60327e4d6ee891c0 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 24 Sep 2020 14:28:10 +0200 Subject: stats: break out CLI options Have a clear separation of concerns for the CLI-only options (and their logic) from the backend. The backend logic is now easier to understand (e.g., `stream` instead of `noStream`). Signed-off-by: Valentin Rothberg --- cmd/podman/containers/stats.go | 20 ++++++++++++++++++-- pkg/api/handlers/libpod/containers_stats.go | 3 +-- pkg/domain/entities/containers.go | 12 +++++++----- pkg/domain/infra/abi/containers.go | 9 ++++++--- pkg/domain/infra/tunnel/containers.go | 6 ++++-- 5 files changed, 36 insertions(+), 14 deletions(-) (limited to 'cmd') diff --git a/cmd/podman/containers/stats.go b/cmd/podman/containers/stats.go index 1a4adb376..bbd389bbf 100644 --- a/cmd/podman/containers/stats.go +++ b/cmd/podman/containers/stats.go @@ -47,8 +47,18 @@ var ( } ) +// statsOptionsCLI is used for storing CLI arguments. Some fields are later +// used in the backend. +type statsOptionsCLI struct { + All bool + Format string + Latest bool + NoReset bool + NoStream bool +} + var ( - statsOptions entities.ContainerStatsOptions + statsOptions statsOptionsCLI defaultStatsRow = "{{.ID}}\t{{.Name}}\t{{.CPUPerc}}\t{{.MemUsage}}\t{{.MemPerc}}\t{{.NetIO}}\t{{.BlockIO}}\t{{.PIDS}}\n" defaultStatsHeader = "ID\tNAME\tCPU %\tMEM USAGE / LIMIT\tMEM %\tNET IO\tBLOCK IO\tPIDS\n" ) @@ -107,7 +117,13 @@ func stats(cmd *cobra.Command, args []string) error { } } - statsChan, err := registry.ContainerEngine().ContainerStats(registry.Context(), args, statsOptions) + // Convert to the entities options. We should not leak CLI-only + // options into the backend and separate concerns. + opts := entities.ContainerStatsOptions{ + Latest: statsOptions.Latest, + Stream: !statsOptions.NoStream, + } + statsChan, err := registry.ContainerEngine().ContainerStats(registry.Context(), args, opts) if err != nil { return err } diff --git a/pkg/api/handlers/libpod/containers_stats.go b/pkg/api/handlers/libpod/containers_stats.go index 3066d5ebc..4d5abe118 100644 --- a/pkg/api/handlers/libpod/containers_stats.go +++ b/pkg/api/handlers/libpod/containers_stats.go @@ -36,8 +36,7 @@ func StatsContainer(w http.ResponseWriter, r *http.Request) { containerEngine := abi.ContainerEngine{Libpod: runtime} statsOptions := entities.ContainerStatsOptions{ - All: len(query.Containers) == 0, // no containers -> query all of them - NoStream: !query.Stream, + Stream: query.Stream, } // Stats will stop if the connection is closed. diff --git a/pkg/domain/entities/containers.go b/pkg/domain/entities/containers.go index b6f86785f..7b272f01e 100644 --- a/pkg/domain/entities/containers.go +++ b/pkg/domain/entities/containers.go @@ -411,15 +411,17 @@ type ContainerCpReport struct { // ContainerStatsOptions describes input options for getting // stats on containers type ContainerStatsOptions struct { - All bool - Format string - Latest bool - NoReset bool - NoStream bool + // Operate on the latest known container. Only supported for local + // clients. + Latest bool + // Stream stats. + Stream bool } // ContainerStatsReport is used for streaming container stats. type ContainerStatsReport struct { + // Error from reading stats. Error error + // Results, set when there is no error. Stats []define.ContainerStats } diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index c77d27cb2..8b0d53940 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -1146,6 +1146,7 @@ func (ic *ContainerEngine) ContainerStats(ctx context.Context, namesOrIds []stri statsChan = make(chan entities.ContainerStatsReport, 1) containerFunc := ic.Libpod.GetRunningContainers + queryAll := false switch { case options.Latest: containerFunc = func() ([]*libpod.Container, error) { @@ -1157,7 +1158,9 @@ func (ic *ContainerEngine) ContainerStats(ctx context.Context, namesOrIds []stri } case len(namesOrIds) > 0: containerFunc = func() ([]*libpod.Container, error) { return ic.Libpod.GetContainersByList(namesOrIds) } - case options.All: + default: + // No containers, no latest -> query all! + queryAll = true containerFunc = ic.Libpod.GetAllContainers } @@ -1197,7 +1200,7 @@ func (ic *ContainerEngine) ContainerStats(ctx context.Context, namesOrIds []stri stats, err := ctr.GetContainerStats(prev) if err != nil { cause := errors.Cause(err) - if options.All && (cause == define.ErrCtrRemoved || cause == define.ErrNoSuchCtr || cause == define.ErrCtrStateInvalid) { + if queryAll && (cause == define.ErrCtrRemoved || cause == define.ErrNoSuchCtr || cause == define.ErrCtrStateInvalid) { continue } if cause == cgroups.ErrCgroupV1Rootless { @@ -1216,7 +1219,7 @@ func (ic *ContainerEngine) ContainerStats(ctx context.Context, namesOrIds []stri report.Stats, report.Error = computeStats() statsChan <- report - if report.Error != nil || options.NoStream { + if report.Error != nil || !options.Stream { return } diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go index 41fb62ab6..31b94ea4b 100644 --- a/pkg/domain/infra/tunnel/containers.go +++ b/pkg/domain/infra/tunnel/containers.go @@ -683,6 +683,8 @@ func (ic *ContainerEngine) Shutdown(_ context.Context) { } func (ic *ContainerEngine) ContainerStats(ctx context.Context, namesOrIds []string, options entities.ContainerStatsOptions) (statsChan chan entities.ContainerStatsReport, err error) { - stream := !options.NoStream - return containers.Stats(ic.ClientCxt, namesOrIds, &stream) + if options.Latest { + return nil, errors.New("latest is not supported for the remote client") + } + return containers.Stats(ic.ClientCxt, namesOrIds, &options.Stream) } -- cgit v1.2.3-54-g00ecf