Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow selection of IEC/JEDEC/non-SI units in podman stats memory output #8945

Closed
srcshelton opened this issue Jan 12, 2021 · 3 comments · Fixed by #8957
Closed

Allow selection of IEC/JEDEC/non-SI units in podman stats memory output #8945

srcshelton opened this issue Jan 12, 2021 · 3 comments · Fixed by #8957
Labels
Good First Issue This issue would be a good issue for a first time contributor to undertake. kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@srcshelton
Copy link
Contributor

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind feature

Description

The output of podman stats was updated to use "human-readable" SI units (KB/MB/GB) when displaying output. This makes sense in the case of storage where SI units are the default (... arguably for marketing reasons in this case, but then GbE is nominally 1,000,000,000bps rather than 1,073,741,824bps).

However, when it comes to memory sizes, this is not particularly helpful - especially when the display then doesn't match up with the interpretation of units used by the parameters of the various --memory* options.

For example, if I use --memory 8g then podman stats shows a MEM USAGE / LIMIT output of 8.59GB... which isn't wrong, but doesn't aid the human-readability of the output compared to simply reading 8GiB!

The best non-breaking way to fix this might be to add additional format options - there's already a .MemUsage template placeholder - could .MemUsageBin/.MemUsageIEC/.MemUsageJEDEC be added to select binary measurements?

Output of podman version:

Version:      2.2.1
API Version:  2.1.0
Go Version:   go1.15.5
Git Commit:   a0d478edea7f775b7ce32f8eb1a01e75374486cb
Built:        Wed Dec  9 11:33:11 2020
OS/Arch:      linux/amd64

Have you tested with the latest version of Podman and have you checked the Podman Troubleshooting Guide?

Yes

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 12, 2021
@mheon mheon added the Good First Issue This issue would be a good issue for a first time contributor to undertake. label Jan 12, 2021
@rhatdan
Copy link
Member

rhatdan commented Jan 12, 2021

Interested in opening a PR to add this functionality?

srcshelton added a commit to srcshelton/podman that referenced this issue Jan 12, 2021
Although storage is more human-readable when expressed in SI units,
IEC/JEDEC (Bytes) units are more pertinent for memory-related values
(and match the format of the --memory* command-line options).

(To prevent possible compatibility issues, the default SI display is
left unchanged)

See containers#8945

Signed-off-by: Stuart Shelton <stuart@shelton.me>
@srcshelton
Copy link
Contributor Author

Interested in opening a PR to add this functionality?

I've had a bash at this - please see #8957

I also notice that, even before I changed anything, if a custom format is specified for podman stats then table headings are no longer output - even if a prefix of table is added to the format string, meaning that the actual effect of table is to use tabs rather than spaces to separate columns.

Is this intended? is the thinking that, just as sorting can be handled by post-processing the output (which I believe was the decision the project settled on w.r.t. sorted output?), so can re-adding the (now missing) headers?

(The issue being that the column-widths are unknown and so output also has to be piped through column or similar in order to line up, and it's suddenly getting rather intensive to get updating sorted container status with explanatory headings which align!)

@rhatdan
Copy link
Member

rhatdan commented Jan 13, 2021

I have no idea if this was a concious decision. Thanks for the PR.

@github-actions 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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good First Issue This issue would be a good issue for a first time contributor to undertake. kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants