Podman Stats additional features #10696

cdoern · 2021-06-16T18:08:07Z

Added Avg CPU calculation and CPU up time to podman stats. Adding different feature sets in different PRs, CPU first.

The CPU track array is used to keep track of all CPU usage values to tally up. AvgCPU contains the average CPU percent usage which is calculated using cgroupStats.CPU.Usage.Total. Lastly, the Start and UpTime stats keep track of when the CPU tracking was started and how long the CPU has been active for since that tracking began.

resolves #9258

Signed-off-by: cdoern [email protected]

mheon · 2021-06-16T18:11:23Z

cmd/podman/containers/stats.go

@@ -146,7 +146,9 @@ func stats(cmd *cobra.Command, args []string) error {
 func outputStats(reports []define.ContainerStats) error {
 	headers := report.Headers(define.ContainerStats{}, map[string]string{
 		"ID":            "ID",
+		"UpTime":        "CPU TIME (S)",


Is UpTime really correect? I've always heard that as the total duration a system has been running for, not cumulative CPU time?

This is the result of podman stats

ID NAME CPU TIME (S) CPU % AVG CPU % MEM USAGE / LIMIT MEM % NET IO BLOCK IO PIDS f10ae061aa6b thirsty_colden 14m1.261643561s 1.28% 6.60% 729.4MB / 16.02GB 4.55% -- / -- -- / -- 117

I made it so that the CPU time pulls the container creation time if the variable is time's zero value. I assumed that CPU time just basically meant up time of the container? If I was wrong I can go back and adjust accordingly.

No, CPU time is the number of seconds or CPU time the container has actively used, not seconds it has been alive.

@mheon I think most recent commit fixed this. I didn't realize cgroupStats.CPU.Usage.Total was a duration in ns of how much up time the group had. I switched around some things with avg CPU % given this information as well.

giuseppe · 2021-06-17T12:41:58Z

libpod/stats.go

 	stats.CPU = calculateCPUPercent(cgroupStats, previousCPU, now, previousStats.SystemNano)
+	stats.AvgCPU = calculateAvgCPU(stats.CPU, previousStats.CPUTrack)
+	stats.CPUTrack = append(previousStats.CPUTrack, float64(cgroupStats.CPU.Usage.Total))


any risk that this can grow indefinitely or use outdated info (or should the average computed using all the available data points)?

If we need all data points, could we just store the current average and number of data points to calculate the incremental average?

Good idea, i will store number of datapoints and the current average I am pretty sure I can do ((oldAvg * numDataPoints) + newData) / (numDataPoints + 1)

giuseppe

LGTM

rhatdan · 2021-06-18T08:44:36Z

libpod/stats.go

+
+// calculateAvgCPU calculates the avg CPU percentage given the previous average and the number of data points.
+func calculateAvgCPU(statsCPU float64, prevAvg float64, prevData int64) float64 {
+	total := prevAvg


Why add a temporary variable? Why not just use prevAvg?

rhatdan · 2021-06-18T08:45:11Z

test/e2e/stats_test.go

+		Expect(session.ExitCode()).To(Equal(0))
+		session = podmanTest.Podman([]string{"stats", "--all", "--no-stream", "--format", "\"{{.ID}} {{.UpTime}} {{.AVGCPU}}\""})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))


Can you look for the header in the the output?

@rhatdan do you mean something like

session = podmanTest.Podman([]string{"stats", "--all", "--no-stream", "--format", "\"{{.ID}} {{.UpTime}} {{.AVGCPU}}\""})
out := session.OutputToStringArray()
Expect(len(out)).To(BeNumerically("==", 3))

or would that not work? It seems that none of the stats tests actually validate the output too much.

No I just want you to make sure the headings you expect show up in the output.
Expect(session.LineInOutputContains("UpTime").To(BeTrue())
Expect(session.LineInOutputContains("AVGCPU").To(BeTrue())

openshift-ci · 2021-06-18T08:45:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, giuseppe, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2021-06-18T10:00:44Z

cmd/podman/containers/stats.go

@@ -166,7 +168,7 @@ func outputStats(reports []define.ContainerStats) error {
 	if report.IsJSON(statsOptions.Format) {
 		return outputJSON(stats)
 	}
-	format := "{{.ID}}\t{{.Name}}\t{{.CPUPerc}}\t{{.MemUsage}}\t{{.MemPerc}}\t{{.NetIO}}\t{{.BlockIO}}\t{{.PIDS}}\n"
+	format := "{{.ID}}\t{{.Name}}\t{{.UpTime}}\t{{.CPUPerc}}\t{{.AVGCPU}}\t{{.MemUsage}}\t{{.MemPerc}}\t{{.NetIO}}\t{{.BlockIO}}\t{{.PIDS}}\n"


Changing the default format would break docker compat. Not sure if this is desired. @mheon @rhatdan WDYT?

I am not sure I would classify this as a breaking change to compat, but it would probbaly be best if we added the new fields to the end of the output, that way, if someone built a script for docker that expected the second field to be CPUPerc, we would not break it.

Do you want me to switch around the ordering? I feel like that would make the output look strange though to have CPU data all scattered throughout.

Unfortunately, I think we need to @cdoern

Maybe add an --alpha flag which would then show the stats in alphabetical order left to right? Or if you really want to go off the rails, make it all selectable by the caller.

@TomSweeneyRedHat working on an alpha flag right now, should it simply just be an if/else around format:=... with two different options?

@TomSweeneyRedHat I don't see the value for an extra alpha flag, the user can always change the output format with --format

Tend to agree - I don't see a strong case for this until and unless we get a user request to add it.

Ok, so we are not going to move the new data to the end of stats? And just tell users if they want a specific format to use --format. I guess I am good with this. Make the default look good for the user.

@Luap99 The value in my eye is just typing --alpha rather than working it out in the --format option. YMMV. I think it would be a good addition, but not a biggy.

rhatdan · 2021-06-22T13:41:05Z

cmd/podman/containers/stats.go

@@ -75,6 +76,7 @@ func statFlags(cmd *cobra.Command) {

 	flags.BoolVar(&statsOptions.NoReset, "no-reset", false, "Disable resetting the screen between intervals")
 	flags.BoolVar(&statsOptions.NoStream, "no-stream", false, "Disable streaming stats and only pull the first result, default setting is false")
+	flags.BoolVar(&statsOptions.Alpha, "alpha", false, "Decides whether or not to alphabetize the output")


I thought we decided to not add the --alpha flag?

Yep, sorry. Just removed it.

baude · 2021-06-22T14:25:02Z

lgtm

cmd/podman/containers/stats.go

added Avg Cpu calculation and CPU up time to podman stats. Adding different feature sets in different PRs, CPU first. resolves containers#9258 Signed-off-by: cdoern <[email protected]>

rhatdan · 2021-06-23T17:06:42Z

/lgtm
/hold

rhatdan · 2021-06-23T18:43:00Z

/hold cancel

srcshelton

I've patched this on top of podman-3.2.2, and I'm always getting exactly the same value for .CPUPerc and the new .AVGCPU at all times - is this expected to be fully hooked-up at this point (or dependent on another patch on top of 3.2.2 to function), or have I missed something?

(Also, if the manpages/docs aren't auto-generated from the code, could a doc update for these additions please also be added?)

cdoern · 2021-06-29T14:32:04Z

@srcshelton running now and I can't seem to recreate this. Can I see how you're running this/your output?

srcshelton · 2021-06-29T17:28:27Z

I've patched this on top of podman-3.2.2, and I'm always getting exactly the same value for .CPUPerc and the new .AVGCPU at all times - is this expected to be fully hooked-up at this point (or dependent on another patch on top of 3.2.2 to function), or have I missed something?

I've realised that this is likely because I'm running podman stats with the --no-stream option in a loop, to integrate other data into the eventual output.

I'd suggest that the '{{.AVGCPU}}' option must gather data persistently beyond the podman invocation which outputs the current counters!

mheon reviewed Jun 16, 2021

View reviewed changes

cdoern force-pushed the libpodStats branch 3 times, most recently from 2e63a56 to a45ae91 Compare June 16, 2021 20:11

giuseppe reviewed Jun 17, 2021

View reviewed changes

cdoern force-pushed the libpodStats branch 2 times, most recently from e2bee36 to 6878875 Compare June 17, 2021 13:37

giuseppe approved these changes Jun 18, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2021

rhatdan approved these changes Jun 18, 2021

View reviewed changes

Luap99 reviewed Jun 18, 2021

View reviewed changes

cdoern force-pushed the libpodStats branch 5 times, most recently from 82c21c3 to 79a7aa6 Compare June 21, 2021 17:38

rhatdan reviewed Jun 22, 2021

View reviewed changes

cdoern force-pushed the libpodStats branch from 79a7aa6 to 2467933 Compare June 22, 2021 14:20

jwhonce reviewed Jun 22, 2021

View reviewed changes

cmd/podman/containers/stats.go Outdated Show resolved Hide resolved

cmd/podman/containers/stats.go Outdated Show resolved Hide resolved

cmd/podman/containers/stats.go Outdated Show resolved Hide resolved

cdoern force-pushed the libpodStats branch from 2467933 to 3062c5c Compare June 23, 2021 13:22

Podman Stats additional features

f26fa53

added Avg Cpu calculation and CPU up time to podman stats. Adding different feature sets in different PRs, CPU first. resolves containers#9258 Signed-off-by: cdoern <[email protected]>

cdoern force-pushed the libpodStats branch from 3062c5c to f26fa53 Compare June 23, 2021 13:23

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2021

openshift-ci bot assigned rhatdan Jun 23, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2021

openshift-merge-robot merged commit 2b850ef into containers:master Jun 23, 2021

srcshelton reviewed Jun 29, 2021

View reviewed changes

srcshelton mentioned this pull request Jun 29, 2021

Additional podman stats metrics #9258

Closed

edsantiago mentioned this pull request Jun 30, 2021

flake: ubuntu 2010: systemd: Failed to create ... control group, EPERM #10386

Closed

TomSweeneyRedHat mentioned this pull request May 27, 2023

[v3.0.1-rhel] podman stats: calc CPU percentage correctly #18710

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Podman Stats additional features #10696

Podman Stats additional features #10696

cdoern commented Jun 16, 2021

mheon Jun 16, 2021

cdoern Jun 16, 2021

mheon Jun 16, 2021

cdoern Jun 16, 2021

giuseppe Jun 17, 2021

cdoern Jun 17, 2021

giuseppe left a comment

rhatdan Jun 18, 2021

rhatdan Jun 18, 2021

cdoern Jun 18, 2021

rhatdan Jun 18, 2021

openshift-ci bot commented Jun 18, 2021

Luap99 Jun 18, 2021

rhatdan Jun 18, 2021

cdoern Jun 18, 2021

TomSweeneyRedHat Jun 18, 2021

TomSweeneyRedHat Jun 18, 2021

cdoern Jun 21, 2021

Luap99 Jun 21, 2021

mheon Jun 21, 2021

rhatdan Jun 22, 2021

TomSweeneyRedHat Jun 22, 2021

rhatdan Jun 22, 2021

cdoern Jun 22, 2021

baude commented Jun 22, 2021

rhatdan commented Jun 23, 2021

rhatdan commented Jun 23, 2021

srcshelton left a comment •

edited

Loading

cdoern commented Jun 29, 2021

srcshelton commented Jun 29, 2021

Podman Stats additional features #10696

Podman Stats additional features #10696

Conversation

cdoern commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baude commented Jun 22, 2021

rhatdan commented Jun 23, 2021

rhatdan commented Jun 23, 2021

srcshelton left a comment • edited Loading

Choose a reason for hiding this comment

cdoern commented Jun 29, 2021

srcshelton commented Jun 29, 2021

srcshelton left a comment •

edited

Loading