add sandbox process operation relay API support by bergwolf · Pull Request #259 · kata-containers/runtime

bergwolf · 2018-04-25T09:50:01Z

As described in the API design, we need to support sandbox.WaitProcess(), sandbox.WinsizeProcess(), sandbox.SignalProcess() and sandbox.IOStream() operations.

sboeuf · 2018-04-25T15:53:13Z

Just adding do-not-merge for now, since we need to get #252 in first.

codecov · 2018-04-25T16:23:12Z

Codecov Report

❗ No coverage uploaded for pull request base (master@992c895). Click here to learn what that means.
The diff coverage is 56.47%.

@@            Coverage Diff            @@
##             master     #259   +/-   ##
=========================================
  Coverage          ?   65.26%           
=========================================
  Files             ?       76           
  Lines             ?     8185           
  Branches          ?        0           
=========================================
  Hits              ?     5342           
  Misses            ?     2269           
  Partials          ?      574

Impacted Files	Coverage Δ
virtcontainers/agent.go	`92.15% <ø> (ø)`
virtcontainers/pkg/vcmock/sandbox.go	`0% <0%> (ø)`
virtcontainers/noop_agent.go	`90% <100%> (ø)`
virtcontainers/iostream.go	`100% <100%> (ø)`
virtcontainers/sandbox.go	`69.41% <100%> (ø)`
virtcontainers/kata_agent.go	`29.84% <15.25%> (ø)`
virtcontainers/hyperstart_agent.go	`59.02% <7.69%> (ø)`
virtcontainers/container.go	`48.88% <77.77%> (ø)`

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 992c895...410e5e6. Read the comment docs.

sboeuf · 2018-05-01T17:04:25Z

#252 got merged, please rebase.

bergwolf · 2018-05-02T09:31:57Z

PR rebased. PTAL.

devimc

nice patch! lgtm

bergwolf · 2018-05-03T00:27:27Z

CI green. @sboeuf @egernst @amshinde PTAL.

It waits a process inside the container of a sandbox. Signed-off-by: Peng Tao <bergwolf@gmail.com>

It sends the signal to a process of a container, or all processes inside a container. Signed-off-by: Peng Tao <bergwolf@gmail.com>

It sends tty resize request to the agent to resize a process's tty window. Signed-off-by: Peng Tao <bergwolf@gmail.com>

It returns stdin, stdout and stderr stream of the specified process in the container. Fixes: kata-containers#258 Signed-off-by: Peng Tao <bergwolf@gmail.com>

egernst

Overall looks okay to me. @amshinde can PTAL?

A few minor nits (looks like comment copy/paste needs update), but otherwise looks good. Sorry for delay, and hope we can merge very soon. Thanks @bergwolf

egernst · 2018-05-04T15:43:23Z

virtcontainers/hyperstart_agent.go

 }
+
+func (h *hyper) waitProcess(c *Container, processID string) (int32, error) {
+	// cc-agent does not support wait process


s/cc-agent/hyperstart-agent?

egernst · 2018-05-04T15:43:42Z

virtcontainers/hyperstart_agent.go

+}
+
+func (h *hyper) readProcessStderr(c *Container, processID string, data []byte) (int, error) {
+	// cc-agent does not support stderr read request


s/cc-agent/hyperstart-agent?
same for all the other comments above...

amshinde · 2018-05-04T16:13:23Z

virtcontainers/container.go

 	if err := waitForShim(c.process.Pid); err != nil {
 		// Force the container to be killed.
-		if err := c.sandbox.agent.killContainer(c.sandbox, *c, syscall.SIGKILL, true); err != nil {
+		if err := c.kill(syscall.SIGKILL, true); err != nil {


Why not just call into c.signalProcess here. c.kill api is not really required.

It's a helper function for cases like this when we want to send signals to the container process itself. And it hides the detail that we use c.process.Token as process ID of the container process, useful because it's also called by KillContainer() in api.go.

amshinde

I have just one comment here, overall looks good to me.

@egernst

As @egernst pointed out, it should be hyperstart_agent instead of cc-agent. Signed-off-by: Peng Tao <bergwolf@gmail.com>

egernst · 2018-05-06T17:50:26Z

@amshinde - PTAL at the update and merge if you're okay with the explanation provided?

WeiZhang555 · 2018-05-07T03:43:47Z

virtcontainers/iostream.go

+)
+
+type iostream struct {
+	sandbox   *Sandbox


I think an iostream is only bounded to a container, so we don't need the sandbox here, and sandbox can be fetched from container.sandbox if needed.
We have lots of redundant struct fields in many places, maybe we can rework them later.

WeiZhang555 · 2018-05-07T03:44:49Z

LGTM

laijs · 2018-05-07T09:43:58Z

LGTM

I found your comment has been resolved (reasonable answered, actually, IMO)

amshinde

LGTM

If new vCPUs have been added, container cpuset cgroup *parents* MUST BE updated, doesn't matter if the container already have a cgroup defined. Updating cpuset cgroup *parents* won't affect to the container cpuset cgroup. fixes kata-containers#259 Signed-off-by: Julio Montes <julio.montes@intel.com>

amshinde added the review label Apr 25, 2018

bergwolf force-pushed the sandbox_api_2 branch from 8dcf934 to 69bf034 Compare April 25, 2018 14:22

sboeuf added the do-not-merge label Apr 25, 2018

bergwolf force-pushed the sandbox_api_2 branch 4 times, most recently from 9b4d63e to 057b2cc Compare May 2, 2018 09:07

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

devimc approved these changes May 2, 2018

View reviewed changes

bergwolf added 4 commits May 4, 2018 15:38

API: add sandbox waitprocess api

45970ba

It waits a process inside the container of a sandbox. Signed-off-by: Peng Tao <bergwolf@gmail.com>

API: add sandbox signalprocess api

55dc0b2

It sends the signal to a process of a container, or all processes inside a container. Signed-off-by: Peng Tao <bergwolf@gmail.com>

API: add sandbox winsizeprocess api

bf4ef43

It sends tty resize request to the agent to resize a process's tty window. Signed-off-by: Peng Tao <bergwolf@gmail.com>

api: add sandbox iostream API

1bb6ab9

It returns stdin, stdout and stderr stream of the specified process in the container. Fixes: kata-containers#258 Signed-off-by: Peng Tao <bergwolf@gmail.com>

egernst suggested changes May 4, 2018

View reviewed changes

amshinde reviewed May 4, 2018

View reviewed changes

amshinde previously requested changes May 4, 2018

View reviewed changes

hyperstart_agent: fix comments

410e5e6

As @egernst pointed out, it should be hyperstart_agent instead of cc-agent. Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf force-pushed the sandbox_api_2 branch from 057b2cc to 410e5e6 Compare May 5, 2018 01:30

egernst approved these changes May 5, 2018

View reviewed changes

WeiZhang555 mentioned this pull request May 7, 2018

cli: implement update command #270

Merged

WeiZhang555 reviewed May 7, 2018

View reviewed changes

amshinde approved these changes May 7, 2018

View reviewed changes

amshinde merged commit 81503d7 into kata-containers:master May 7, 2018

bergwolf mentioned this pull request May 9, 2018

virtcontainers: define and implement pod based external API #49

Closed

bergwolf deleted the sandbox_api_2 branch September 13, 2018 03:27

Conversation

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

Uh oh!

Uh oh!

sboeuf commented Apr 25, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

sboeuf commented May 1, 2018

Uh oh!

bergwolf commented May 2, 2018

Uh oh!

devimc left a comment

Choose a reason for hiding this comment

Uh oh!

bergwolf commented May 3, 2018

Uh oh!

egernst left a comment • 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!

egernst May 4, 2018

Choose a reason for hiding this comment

Uh oh!

bergwolf May 5, 2018

Choose a reason for hiding this comment

Uh oh!

egernst May 4, 2018

Choose a reason for hiding this comment

Uh oh!

bergwolf May 5, 2018

Choose a reason for hiding this comment

Uh oh!

amshinde May 4, 2018

Choose a reason for hiding this comment

Uh oh!

bergwolf May 5, 2018

Choose a reason for hiding this comment

Uh oh!

amshinde left a comment

Choose a reason for hiding this comment

Uh oh!

egernst commented May 6, 2018

Uh oh!

WeiZhang555 May 7, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

amshinde left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bergwolf commented Apr 25, 2018 •

edited

Loading

codecov bot commented Apr 25, 2018 •

edited

Loading

egernst left a comment •

edited

Loading

WeiZhang555 commented May 7, 2018 •

edited by amshinde

Loading

laijs commented May 7, 2018 •

edited by amshinde

Loading