cli: implement update command by devimc · Pull Request #270 · kata-containers/runtime

devimc · 2018-04-27T20:32:55Z

Update command is used to update container's resources at run time.
All constraints are applied inside the VM to each container cgroup.
By now only CPU constraints are fully supported, vCPU are hot added
or removed depending of the new constraint.

fixes #189

Signed-off-by: Julio Montes julio.montes@intel.com

devimc · 2018-05-02T16:27:06Z

@kata-containers/runtime please take a look

bergwolf · 2018-05-03T00:15:54Z

virtcontainers/api.go

+	}
+
+	// Fetch the container.
+	c, err := p.findContainer(containerID)


Please make UpdateContainer part of the VCSandbox interface and implement UpdateContainer() here as a wrapper of it. You can move p.findContainer() and c.update() to sandbox.UpdateContainer().

bergwolf · 2018-05-03T00:23:06Z

cli/update.go

+  },
+  "blockIO": {
+    "weight": 0
+  }


missing pids?

I don't think so, it's base on runC implementation

It's a bug in runc? --pids-limit and --l3-cache-schema were added after the initial runc update implementation and it seems the developer forgot to add them to the resources file.

ref opencontainers/runc/issues/1793

We do read pid limit at https://github.com/kata-containers/runtime/pull/270/files#diff-1cc31db47bce8b7af5fda9d984c048e6R162. So I think we should at least add pids here. l3-cache-schema may be left out since it is not part of specs.LinuxResources anyway.

good catch!

codecov · 2018-05-03T15:16:35Z

Codecov Report

Merging #270 into master will increase coverage by 1.31%.
The diff coverage is 70.49%.

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
+ Coverage    64.4%   65.71%   +1.31%     
==========================================
  Files          85       76       -9     
  Lines        8214     8197      -17     
==========================================
+ Hits         5290     5387      +97     
+ Misses       2361     2228     -133     
- Partials      563      582      +19

Impacted Files	Coverage Δ
cli/main.go	`88% <ø> (ø)`	⬆️
virtcontainers/agent.go	`92.15% <ø> (ø)`	⬆️
virtcontainers/implementation.go	`0% <0%> (ø)`	⬆️
virtcontainers/pkg/vcmock/sandbox.go	`0% <0%> (ø)`	⬆️
virtcontainers/pkg/vcmock/mock.go	`94.93% <0%> (-5.07%)`	⬇️
virtcontainers/hyperstart_agent.go	`60.42% <0%> (+1.39%)`	⬆️
virtcontainers/kata_agent.go	`32.43% <0%> (+2.59%)`	⬆️
virtcontainers/noop_agent.go	`86.66% <100%> (-3.34%)`	⬇️
virtcontainers/container.go	`45.51% <39.47%> (-3.37%)`	⬇️
virtcontainers/sandbox.go	`67.64% <60%> (+0.28%)`	⬆️
... and 31 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 fa848ba...2b2aeab. Read the comment docs.

devimc · 2018-05-03T15:31:10Z

@bergwolf changes applied, thanks

bergwolf

lgtm, thanks @devimc !

egernst · 2018-05-03T17:24:32Z

Gopkg.lock

  packages = ["pkg/cover"]
  revision = "b0c4c6d0583aae4e3b5d12b6ec47767670548cc4"

+[[projects]]


should this be part of the prior commit, @devimc ?

WeiZhang555 · 2018-05-04T07:56:32Z

virtcontainers/hyperstart_agent.go

 }

+func (h *hyper) updateContainer(sandbox *Sandbox, c Container, resources specs.LinuxResources) error {
+	// cc-agent does not support update


I think this should be hyperstart_agent ?

I think cc-agent is correct, see https://github.com/kata-containers/runtime/blob/master/virtcontainers/hyperstart_agent.go#L800-L808

Hmmm, so weird, this file is written for hyperstart agent but the comment is saying cc-agent.

I'm confused a bit, is this correct? @bergwolf

Oh, I believe this is a typo now. Check: https://github.com/kata-containers/runtime/pull/259/files#diff-d4339baeea8e93444b3ddc3e97b53c87R806

It's already been fixed in #259 now

WeiZhang555 · 2018-05-04T07:58:21Z

virtcontainers/interfaces.go

 	StatusContainer(sandboxID, containerID string) (ContainerStatus, error)
 	StopContainer(sandboxID, containerID string) (VCContainer, error)
 	ProcessListContainer(sandboxID, containerID string, options ProcessListOptions) (ProcessList, error)
+	UpdateContainer(sandboxID, containerID string, resources specs.LinuxResources) error


Why do we need this API? I think sandbox.UpdateContainer is enough for us.

we need it to write unit tests

see cli/update_test.go

It's OK for now. I think we have lots of exported functions just used for test, hope we can find a way to remove them and keep the test cases at the same time later.
It's another topic though. This part looks good for now.

devimc · 2018-05-04T15:35:02Z

@egernst changes applied, thanks

egernst

Looks good to me, thanks @devimc.

@WeiZhang555 - can you please take a look @ the update and merge if you are okay with the changes?

WeiZhang555 · 2018-05-07T03:57:38Z

Codes LGTM. Only a typo left s/cc-agent/hyperstart-agent/

So anyone please feel free to merge this after the typo is fixed.

devimc · 2018-05-07T13:48:57Z

@WeiZhang555 changes applied, thanks

egernst · 2018-05-08T04:10:49Z

@devimc please rebase and lets move forward to merge after...

go-units package is used to convert integers to memory units, for example 1024 -> 1G Signed-off-by: Julio Montes <julio.montes@intel.com>

Update command is used to update container's resources at run time. All constraints are applied inside the VM to each container cgroup. By now only CPU constraints are fully supported, vCPU are hot added or removed depending of the new constraint. fixes kata-containers#189 Signed-off-by: Julio Montes <julio.montes@intel.com>

This new version of kata-agent brings support for updating resources and cpuset cgroups Shortlog: 28cf91a grpc: implement update command d96b8e1 grpc: update cpuset cgroup 4bcacdc network: Don't remove network routes or DNS when destroying sandbox 1f5cf20 network: Don't store the network info as pointers if slices used 8f828bb uevent: Fix netlink error while assigning pid in netlink client 093f61b agent: add grpc tracer UT 33bd601 agent: add server interceptor to log grpc requests 134d5d5 test: add start/stop grpc server UT 7e94246 agent: track grpc server 9fb8024 UT: add tests for channel bea6183 agent: wait serial channel to be ready before reading f8c8c4c agent: accept grpc connections multiple times Signed-off-by: Julio Montes <julio.montes@intel.com>

Convert all non-string types into strings prior to logging to avoid attempting to display non-string types with the `%q` format verb. Fixes kata-containers#270. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

devimc added do-not-merge labels Apr 27, 2018

devimc force-pushed the command/update branch 2 times, most recently from 1e0059a to 0d15561 Compare May 2, 2018 15:10

devimc removed the do-not-merge label May 2, 2018

devimc changed the title ~~[WIP] implement update command~~ implement update command May 2, 2018

devimc changed the title ~~implement update command~~ cli: implement update command May 2, 2018

devimc force-pushed the command/update branch 3 times, most recently from 4267c9f to bee9560 Compare May 2, 2018 17:34

bergwolf suggested changes May 3, 2018

View reviewed changes

devimc force-pushed the command/update branch from bee9560 to 18d05d4 Compare May 3, 2018 15:16

devimc force-pushed the command/update branch from 18d05d4 to 56a2299 Compare May 3, 2018 15:30

devimc force-pushed the command/update branch from 56a2299 to 9a934be Compare May 3, 2018 15:32

bergwolf approved these changes May 3, 2018

View reviewed changes

egernst reviewed May 3, 2018

View reviewed changes

WeiZhang555 reviewed May 4, 2018

View reviewed changes

devimc force-pushed the command/update branch from 9a934be to e4c888b Compare May 4, 2018 15:29

egernst approved these changes May 6, 2018

View reviewed changes

devimc force-pushed the command/update branch from e4c888b to f7c35e7 Compare May 7, 2018 13:48

vendor: add docker/go-units

0cea861

go-units package is used to convert integers to memory units, for example 1024 -> 1G Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc force-pushed the command/update branch from f7c35e7 to 0993a47 Compare May 8, 2018 12:22

devimc force-pushed the command/update branch from 0993a47 to 2b2aeab Compare May 8, 2018 12:29

egernst merged commit f6544a3 into kata-containers:master May 8, 2018

devimc deleted the command/update branch May 9, 2018 15:38

Conversation

devimc commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devimc commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devimc commented May 3, 2018

Uh oh!

bergwolf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeiZhang555 May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devimc commented May 4, 2018

Uh oh!

egernst left a comment

Choose a reason for hiding this comment

Uh oh!

WeiZhang555 commented May 7, 2018

Uh oh!

devimc commented May 7, 2018

Uh oh!

egernst commented May 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

devimc commented Apr 27, 2018 •

edited

Loading

codecov bot commented May 3, 2018 •

edited

Loading

WeiZhang555 May 7, 2018 •

edited

Loading