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

setup stats command #73

Merged
merged 1 commit into from
Nov 16, 2021
Merged

Conversation

fahedouch
Copy link
Member

Signed-off-by: fahed dorgaa [email protected]

@fahedouch fahedouch marked this pull request as draft February 22, 2021 19:12
@fahedouch fahedouch marked this pull request as ready for review March 4, 2021 20:59
stats.go Outdated Show resolved Hide resolved
stats.go Outdated Show resolved Hide resolved
formatter_stats.go Outdated Show resolved Hide resolved
formatter_stats.go Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda added the enhancement New feature or request label Mar 10, 2021
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
stats.go Outdated Show resolved Hide resolved
stats.go Outdated Show resolved Hide resolved
stats.go Outdated Show resolved Hide resolved
stats.go Outdated Show resolved Hide resolved
@AkihiroSuda
Copy link
Member

Thanks for working on this, but needs cleaning up the code, squashing the commits, and rebasing with the master.

@AkihiroSuda AkihiroSuda marked this pull request as draft April 2, 2021 10:04
@AkihiroSuda
Copy link
Member

@fahedouch Are you still planning to work on this?

@fahedouch
Copy link
Member Author

@AkihiroSuda yes, if you see if it's a priority I can prioritize it over other tasks

@fahedouch fahedouch self-assigned this Aug 11, 2021
@nehaldattani
Copy link

nehaldattani commented Oct 20, 2021

This enhancement is pending for quite a while. Would this PR be accepted anytime soon ? @fahedouch @AkihiroSuda

@fahedouch
Copy link
Member Author

@nehaldattani i will revisit this PR by the end of the week ;)

@fahedouch fahedouch force-pushed the setup-stats branch 5 times, most recently from 9371e08 to e89233b Compare November 3, 2021 11:39
@fahedouch fahedouch marked this pull request as ready for review November 3, 2021 11:40
@fahedouch fahedouch requested a review from AkihiroSuda November 3, 2021 11:40
}

if showAll {
// TODO: watches for container creation and removal (dynamic stats)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add this limitation to the --help text and README.md

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And open an issue ticket

@AkihiroSuda
Copy link
Member

CI failing for Windows and FreeBSD

@fahedouch
Copy link
Member Author

@AkihiroSuda I added the support for dynamic container creation/deletion. I will fix the CI error this night or tomorrow. Sorry for the delay I am traveling abroad.

@fahedouch fahedouch force-pushed the setup-stats branch 2 times, most recently from 0b149e4 to fb54465 Compare November 14, 2021 19:29
func (w *eventHandler) Watch(c <-chan *events.Envelope) {
for e := range c {
w.mu.Lock()
fmt.Println(e.Topic)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

@AkihiroSuda
Copy link
Member

@AkihiroSuda I added the support for dynamic container creation/deletion. I will fix the CI error this night or tomorrow. Sorry for the delay I am traveling abroad.

Thanks, but creation doesn't seem handled yet

mem = calculateCgroup2MemUsage(metrics)
memLimit = float64(metrics.Memory.UsageLimit)
memPercent = calculateMemPercent(memLimit, mem)
pidsStatsCurrent = metrics.Pids.Current
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NetworkTx/Rx missing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But docker stats work with cgroup v2.
We can revisit this in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda I'm assuming that docker doesn't consume the cgroup api from containerd https://github.com/containerd/cgroups.
do you think it is a lack in the containerd cgroup api and that I have to open a ticket at https://github.com/containerd/cgroups

Copy link
Member

@AkihiroSuda AkihiroSuda Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda thank you for checking this I will fix it.

@fahedouch
Copy link
Member Author

fahedouch commented Nov 15, 2021

@AkihiroSuda I added the support for dynamic container creation/deletion. I will fix the CI error this night or tomorrow. Sorry for the delay I am traveling abroad.

Thanks, but creation doesn't seem handled yet

@AkihiroSuda creation is handled only with --all flag, have you tested with with --all flag ?

@AkihiroSuda
Copy link
Member

@AkihiroSuda I added the support for dynamic container creation/deletion. I will fix the CI error this night or tomorrow. Sorry for the delay I am traveling abroad.

Thanks, but creation doesn't seem handled yet

@AkihiroSuda creation is handled only with --all flag, have you tested with with --all flag ?

Can we watch creation events by default?
docker stats doesn't need --all to watch creation events.

Signed-off-by: fahed dorgaa <[email protected]>
@fahedouch
Copy link
Member Author

@AkihiroSuda I added the support for dynamic container creation/deletion. I will fix the CI error this night or tomorrow. Sorry for the delay I am traveling abroad.

Thanks, but creation doesn't seem handled yet

@AkihiroSuda creation is handled only with --all flag, have you tested with with --all flag ?

Can we watch creation events by default? docker stats doesn't need --all to watch creation events.

fixed @AkihiroSuda

Display a live stream of container(s) resource usage statistics.


Usage: `nerdctl stats [flags]`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next time please add flags too

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@AkihiroSuda AkihiroSuda merged commit f316402 into containerd:master Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants