API: support sandbox monitor operation by bergwolf · Pull Request #252 · kata-containers/runtime

bergwolf · 2018-04-24T01:05:35Z

As described in the API design, we need to support Sandbox.Monitor() operation that returns an error channel for the caller to watch on sandbox fetal errors.

Fixes #251

bergwolf · 2018-04-25T01:37:43Z

@sboeuf @amshinde @jodh-intel @laijs PTAL.

sboeuf · 2018-04-25T06:04:24Z

@bergwolf I'll take a look at this tomorrow, could you please fix the CI.

codecov · 2018-04-25T08:21:12Z

Codecov Report

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

@@            Coverage Diff            @@
##             master     #252   +/-   ##
=========================================
  Coverage          ?   65.64%           
=========================================
  Files             ?       75           
  Lines             ?     8008           
  Branches          ?        0           
=========================================
  Hits              ?     5257           
  Misses            ?     2181           
  Partials          ?      570

Impacted Files	Coverage Δ
virtcontainers/agent.go	`92.15% <ø> (ø)`
virtcontainers/hyperstart_agent.go	`60.71% <0%> (ø)`
virtcontainers/noop_agent.go	`85.71% <0%> (ø)`
virtcontainers/pkg/vcmock/sandbox.go	`0% <0%> (ø)`
virtcontainers/kata_agent.go	`32.87% <65.78%> (ø)`
virtcontainers/sandbox.go	`67.72% <66.66%> (ø)`
virtcontainers/monitor.go	`81.48% <81.48%> (ø)`

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 70b3c77...9d1311d. Read the comment docs.

amshinde · 2018-04-25T18:39:39Z

lgtm

devimc · 2018-04-25T18:40:24Z

virtcontainers/sandbox.go

+// Monitor returns a error channel for watcher to watch at
+func (s *Sandbox) Monitor() (chan error, error) {
+	if s.state.State != StateRunning {
+		return nil, fmt.Errorf("Sandbox not running")


Sandbox is not running, might be?

good point!

devimc · 2018-04-25T18:43:22Z

virtcontainers/monitor.go

+		m.running = true
+
+		// create and start agent watcher
+		go func() {


I wonder if you should add this routine to a wait group ?

Good point! I'll let stop() wait on it.

sboeuf · 2018-04-25T21:05:43Z

virtcontainers/sandbox.go

+
+	if s.monitor == nil {
+		s.Lock()
+		if s.monitor == nil {


Second time you check if s.monitor is nil, I think that's a bit too much 😃

It ensures we only create and set the monitor once. Otherwise concurrent callers might race in and we end up setting monitor twice.

You can simply do the lock and test the monitor value. This is really cheap here.

Fair enough, I'll change it.

sboeuf · 2018-04-25T21:44:12Z

virtcontainers/agent.go

+	startContainer(sandbox *Sandbox, c *Container) error

 	// stopContainer will tell the agent to stop a container related to a Sandbox.
-	stopContainer(sandbox Sandbox, c Container) error


The rationale behind using pointers versus copies was to make sure the sandbox could not get modified by some functions that obviously didn't need it.
Now I understand your concern about memory being overused because we're generating a lot of copies and I think I pretty okay with changing things into pointers. But please submit this into a separate PR as it is an important change and this way we can discuss properly about it. I don't want this to be hidden through the monitor PR.

Without it, I got CI errors about copying mutex.

Ok, but please submit it as a separate PR.

Then this PR cannot pass CI.

That's fine, we'll merge the new PR, so that you can rebase this one.

ok, here it is #263

sboeuf · 2018-04-25T21:46:11Z

virtcontainers/kata_agent.go

+type reqFunc func(context.Context, interface{}, ...golangGrpc.CallOption) (interface{}, error)
+
+func (k *kataAgent) installReqFunc(c *kataclient.AgentClient) {
+	k.reqHandlers = make(map[string]reqFunc)


Yes, in this case, I think the map is the only way to avoid the gocyclo complexity going over 15.
Any reason you didn't use a static map here ?

Because the agent client is dynamically allocated. I also wanted to use a static map but it turns out there is no static grpc request handler.

sboeuf · 2018-04-25T21:58:21Z

virtcontainers/sandbox.go

+		s.Unlock()
+	}
+
+	return s.monitor.newWatcher()


I am a bit concerned about the way this is handled here. The caller can basically call a million times into Monitor(), this will result into a million go routines being spawned here. If think we should try to handle things better, by returning the existing channel from newWatcher() if it already exists. This means we would always have only one channel, because I don't see the usage for having several channels referring to the same thing. Maybe I am missing something.

No, the monitor makes sure there is only one watcher goroutine per monitor. We just add more watch channels to it.

Oh I see. So we'll have one go routine with an infinite number of channels. Sounds good.

sboeuf · 2018-05-01T14:37:52Z

@bergwolf please rebase (and make sure it removes the last commit that we've already merged in a previous PR)

It monitors the sandbox status and returns an error channel to let caller watch it. Fixes: kata-containers#251 Signed-off-by: Peng Tao <bergwolf@gmail.com>

CI complains about cyclomatic complexity in sendReq. warning: cyclomatic complexity 16 of function (*kataAgent).sendReq() is high (> 15) (gocyclo) Refactor it a bit to avoid such error. I'm not a big fan of the new code but it is done so because golang does not support generics. Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf · 2018-05-01T14:43:14Z

PR rebased.

egernst · 2018-05-01T16:29:16Z

@sboeuf - PTAL and merge when you're happy with the changes.

Allow frozen containers receive signals in order to avoid runtime waits for shim to finish. fixes kata-containers#252 Signed-off-by: Julio Montes <julio.montes@intel.com>

amshinde added the review label Apr 24, 2018

bergwolf force-pushed the sandbox_api_1 branch 2 times, most recently from 2693f6c to 7329554 Compare April 25, 2018 01:28

bergwolf force-pushed the sandbox_api_1 branch from 7329554 to 14a889c Compare April 25, 2018 07:09

bergwolf mentioned this pull request Apr 25, 2018

add sandbox process operation relay API support #259

Merged

devimc reviewed Apr 25, 2018

View reviewed changes

sboeuf suggested changes Apr 25, 2018

View reviewed changes

bergwolf force-pushed the sandbox_api_1 branch from 14a889c to 8afff5f Compare April 26, 2018 02:27

sboeuf mentioned this pull request Apr 30, 2018

virtcontainers: always pass sandbox as a pointer #263

Merged

bergwolf added 2 commits May 1, 2018 22:42

api: add sandbox Monitor API

35ebadc

It monitors the sandbox status and returns an error channel to let caller watch it. Fixes: kata-containers#251 Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf force-pushed the sandbox_api_1 branch from 8afff5f to 9d1311d Compare May 1, 2018 14:42

egernst approved these changes May 1, 2018

View reviewed changes

sboeuf approved these changes May 1, 2018

View reviewed changes

sboeuf merged commit 87aa1d7 into kata-containers:master May 1, 2018

sboeuf removed the review label May 1, 2018

bergwolf mentioned this pull request May 9, 2018

virtcontainers: define and implement pod based external API #49

Closed

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

Conversation

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

Uh oh!

Uh oh!

bergwolf commented Apr 25, 2018

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!

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

Uh oh!

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!

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!

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!

sboeuf commented May 1, 2018

Uh oh!

bergwolf commented May 1, 2018

Uh oh!

egernst commented May 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bergwolf commented Apr 24, 2018 •

edited

Loading

codecov bot commented Apr 25, 2018 •

edited

Loading

amshinde commented Apr 25, 2018 •

edited

Loading