Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

cli :Implement events command#299

Merged
bergwolf merged 2 commits intokata-containers:masterfrom
jshachm:implement-events-command
May 18, 2018
Merged

cli :Implement events command#299
bergwolf merged 2 commits intokata-containers:masterfrom
jshachm:implement-events-command

Conversation

@jshachm
Copy link
Copy Markdown
Member

@jshachm jshachm commented May 11, 2018

Events cli display container events such as cpu,memory, and IO usage statistics.

By now OOM notifications are not fully supproted.

Fixes : #186

@jshachm
vendor: update agent
f1f534c 
This new version of kata-agent brings support for
stats of a certain container

Short logs:
077e6f9 grpc : Add the StatsContainer api for `events cli`
d29bf53 block: Get rid of device prediction for Storage as well
3b565ad block: Use PCI address to determine block device name

Signed-off-by: Haomin <caihaomin@huawei.com>
@jshachm jshachm changed the title cli : [WIP] Implement events command [WIP] cli :Implement events command May 11, 2018
@jodh-intel
Copy link
Copy Markdown

jodh-intel commented May 14, 2018 •
edited by amshinde
Loading

Could you raise issues for OOM support (and Intel RDT ;) and add the limitation label for both please?

The CI is failing due to a few lint errors which need fixing. Once done,

lgtm

Approved with PullApprove

@jshachm
Copy link
Copy Markdown
Member Author

jshachm commented May 14, 2018

@jodh-intel thx. Working on fix lint error. Issues will be raised later ~

This was referenced May 15, 2018
Closed
Closed
@jshachm
Copy link
Copy Markdown
Member Author

jshachm commented May 15, 2018

@jodh-intel
issues are raised and lint errors have been fixed.

But I think I still need to add tests file like events_test.go and some other.

Will update these later~

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented May 15, 2018

Thanks @jshachm!

@codecov
Copy link
Copy Markdown

codecov bot commented May 15, 2018 •
edited
Loading

Codecov Report

Merging #299 into master will decrease coverage by 0.47%.
The diff coverage is 31.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
- Coverage   64.35%   63.87%   -0.48%     
==========================================
  Files          86       87       +1     
  Lines        8469     8620     +151     
==========================================
+ Hits         5450     5506      +56     
- Misses       2441     2530      +89     
- Partials      578      584       +6
Impacted Files Coverage Δ
cli/main.go 88% <ø> (ø) ⬆️
virtcontainers/agent.go 92.15% <ø> (ø) ⬆️
virtcontainers/implementation.go 0% <0%> (ø) ⬆️
virtcontainers/hyperstart_agent.go 59.12% <0%> (-0.28%) ⬇️
virtcontainers/pkg/vcmock/mock.go 95.18% <100%> (+0.24%) ⬆️
virtcontainers/noop_agent.go 90.9% <100%> (+0.43%) ⬆️
cli/events.go 29.03% <29.03%> (ø)
virtcontainers/api.go 63.12% <50%> (+1.56%) ⬆️
virtcontainers/container.go 47.52% <50%> (+0.02%) ⬆️
virtcontainers/kata_agent.go 28.61% <9.52%> (-0.69%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 207ceab...1205e34. Read the comment docs.

@jshachm
Copy link
Copy Markdown
Member Author

jshachm commented May 16, 2018

working on fixing codecov and add some other tests.Once get it done, I think it should be ready to review.

@jshachm jshachm changed the title [WIP] cli :Implement events command cli :Implement events command May 16, 2018
@jshachm
Copy link
Copy Markdown
Member Author

jshachm commented May 16, 2018

@bergwolf @sboeuf @jodh-intel

PTAL

tests have been added~

bergwolf
bergwolf reviewed May 16, 2018
View reviewed changes
virtcontainers/interfaces.go
KillContainer(sandboxID, containerID string, signal syscall.Signal, all bool) error
StartContainer(sandboxID, containerID string) (VCContainer, error)
StatusContainer(sandboxID, containerID string) (ContainerStatus, error)
StatsContainer(sandboxID, containerID string) (ContainerStats, error)
Copy link
Copy Markdown
Member

@bergwolf bergwolf May 16, 2018

Choose a reason for hiding this comment

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

Can you please make this part of the VCSandbox interface and let VC.StatsContainer be a wrapper of it, like we did for the other sandbox APIs?

Copy link
Copy Markdown
Member Author

@jshachm jshachm May 16, 2018

Choose a reason for hiding this comment

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

@bergwolf Got it~ On the way to fix it

@jshachm
Copy link
Copy Markdown
Member Author

jshachm commented May 17, 2018

@bergwolf
Interface changes done.
PTAL
But CI for ubuntu-17.10 failes randomly. So strange....

@bergwolf
Copy link
Copy Markdown
Member

bergwolf commented May 18, 2018 •
edited by amshinde
Loading

LGTM. Thanks @jshachm!

Approved with PullApprove

@jshachm
Copy link
Copy Markdown
Member Author

jshachm commented May 18, 2018

@bergwolf thanks
OK, let's begin fighting with the strange CI....

@jshachm
cli: implement events command
1205e34 
Events cli display container events such as cpu,
memory, and IO usage statistics.

By now OOM notifications and intel RDT are not fully supproted.

Fixes: #186

Signed-off-by: Haomin <caihaomin@huawei.com>
@jshachm
Copy link
Copy Markdown
Member Author

jshachm commented May 18, 2018

@jodh-intel @bergwolf CI is happy now. Ready to merge~ thx

@bergwolf bergwolf merged commit be82c7f into kata-containers:master May 18, 2018
@jshachm jshachm deleted the implement-events-command branch May 18, 2018 07:37
zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019
@bergwolf
Merge pull request kata-containers#299 from devimc/topic/vsocks
9140c9c 
channel: support communication channel hotplug
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@bergwolf bergwolf bergwolf left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jshachm @jodh-intel @bergwolf