-
Notifications
You must be signed in to change notification settings - Fork 460
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
support live monitoring critool stats #467
support live monitoring critool stats #467
Conversation
ping @feiskyer |
cmd/crictl/display.go
Outdated
namespace = "NAMESPACE" | ||
size = "SIZE" | ||
tag = "TAG" | ||
digest = "DIGEST" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a prefix for those const names?
cmd/crictl/image.go
Outdated
"github.com/sirupsen/logrus" | ||
"github.com/urfave/cli" | ||
"golang.org/x/net/context" | ||
pb "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" | ||
"sort" | ||
"strings" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move those two to the first group
cmd/crictl/stats.go
Outdated
if duration == 0 { | ||
return fmt.Errorf("cpu stat is not updated during sample") | ||
display := newTableDisplay(20, 1, 3, ' ', 0) | ||
for range time.Tick(500 * time.Millisecond) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a new option for this, e.g. --watch
or -w
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think live monitoring is enabled by default, like docker stats
. So do we still need this watch option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT? @feiskyer Do we really need this option? What is the use case of non live monitoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just like kubectl top, it only show the output for one time. We can enable it by default, but it could be disabled by an option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your reply, I have added this -w option to enable live monitoring, PTAL.
Signed-off-by: yeya24 <yb532204897@gmail.com> fix gometalinter Signed-off-by: yeya24 <yb532204897@gmail.com> add option watch Signed-off-by: yeya24 <yb532204897@gmail.com>
f24ad23
to
7cf1e74
Compare
thanks. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, yeya24 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:
Approvers can indicate their approval by writing |
Close and re-open to trigger the failed travis CI. |
Signed-off-by: yeya24 yb532204897@gmail.com
Add support for live critool stats;
Also refactor the table display