runtime: add builtin proxy and shim capability by bergwolf · Pull Request #173 · kata-containers/runtime

bergwolf · 2018-04-03T06:48:16Z

Add a new proxy type KataProxyBuiltinType and a new shim type KataBuiltinShimType. When they are specified, new proxy or shim processes will not be created. OTOH, the proxy and shim capabilities are provided by the kata runtime library directly.

For proxy, a yamux grpc dialer is used to create connections to the kata agent. And a goroutine is created to watch guest's console.

For shim, sandbox relay APIs will be provided. They are left to future PRs though.

Unfortunately due to limitation of the yamux library (at most one connection between client and server), this cannot be used with the kata runtime cli. So no cli change is included.

Depends-on: kata-containers/agent/pull/201

sboeuf

@bergwolf This PR looks very good to me semantically speaking. I have a bunch of comments about syntax mostly.

sboeuf · 2018-04-03T17:19:38Z

virtcontainers/hypervisor.go


+const (
+	CONSOLE_PROTO_UNIX = "unix"
+	CONSOLE_PROTO_PTY  = "pty"


Those const should be part of proxy.go

sboeuf · 2018-04-03T17:22:24Z

virtcontainers/proxy.go

 	}
 }

+func isProxyBuiltin(pType ProxyType) bool {


s/isProxyBuiltin/isProxyBuiltIn/

sboeuf · 2018-04-03T17:28:26Z

virtcontainers/kata_builtin_proxy.go

+// This is a kata builtin proxy implementation of the proxy interface. Kata proxy
+// functionality is implemented inside the virtcontainers library.
+type kataBuiltinProxy struct {
+	podId string


s/podId/podID/

sboeuf · 2018-04-03T17:29:01Z

virtcontainers/kata_builtin_proxy.go

+
+// This is a kata builtin proxy implementation of the proxy interface. Kata proxy
+// functionality is implemented inside the virtcontainers library.
+type kataBuiltinProxy struct {


s/kataBuiltinProxy/kataBuiltInProxy/

sboeuf · 2018-04-03T17:29:47Z

virtcontainers/kata_builtin_proxy.go

+	conn  net.Conn
+}
+
+// register is the proxy start implementation for kata builtin proxy.


// start is the proxy start ...

sboeuf · 2018-04-03T18:09:29Z

virtcontainers/podlist.go

+// globalPodlist tracks pods globally
+var globalPodlist = podlist{pods: make(map[string]*Pod)}
+
+func (pl *podlist) addPod(pod *Pod) (err error) {


func (p *podList) addPod(pod *Pod) (err error) {

sboeuf · 2018-04-03T18:10:28Z

virtcontainers/podlist.go

+	return err
+}
+
+func (pl *podlist) removePod(id string) error {


func (p *podList) removePod(podID string) error {

sboeuf · 2018-04-03T18:10:59Z

virtcontainers/podlist.go

+	return nil
+}
+
+func (pl *podlist) lookupPod(id string) (pod *Pod, err error) {


func (p *podList) lookupPod(podID string) (pod *Pod, err error) {

sboeuf · 2018-04-03T18:12:57Z

virtcontainers/podlist.go

+	return pod, err
+}
+
+func (pl *podlist) foreachPod(f func(*Pod) error) error {


Not used from your code here. Might be worth adding it only when it's gonna be called from some real code.

sboeuf · 2018-04-03T18:16:08Z

virtcontainers/podlist.go

+
+func foreachPod(f func(*Pod) error) error {
+	return globalPodlist.foreachPod(f)
+}


I would remove those:

func addPod(pod *Pod) error { return globalPodlist.addPod(pod) } func removePod(id string) error { return globalPodlist.removePod(id) } func lookupPod(id string) (*Pod, error) { return globalPodlist.lookupPod(id) } func foreachPod(f func(*Pod) error) error { return globalPodlist.foreachPod(f) }

I think they add more confusion where we can call directly from pod.go:

globalPodList.addPod(p) ...

bergwolf · 2018-04-04T08:25:18Z

@sboeuf thanks for reviewing. changes applied and UT added. PTAL again.

bergwolf · 2018-04-04T08:48:35Z

add do not merge label since it depends on kata-containers/agent#201

jodh-intel · 2018-04-04T08:50:26Z

virtcontainers/kata_builtin_proxy.go

+		if err != nil {
+			return err
+		}
+	// TODO: add pty console support for kvmtools


Is there an open issue for this so we don't forget?

This should be part of the work that introduces kvmtools hypervisor support.

jodh-intel · 2018-04-04T08:53:52Z

virtcontainers/kata_builtin_proxy.go

+	go func() {
+		scanner = bufio.NewScanner(conn)
+		for scanner.Scan() {
+			fmt.Printf("[SB-%s] vmconsole: %s\n", p.podID, scanner.Text())


SB == "scanner buffer"? Could that prefix be [pod %s] for clarity.

SB == Sandbox

The problem is that almost all other components use pod / pod-id I think?

I thought we want to switch from pod to sandbox as agreed in the api design. But it can be done later.

Yes I think the sandbox naming should be done in a separate PR, meaning that it makes more sense to stick with [POD-%s] for now.

OK. I'll keep pod notion here and submit another PR to do the conversion.

jodh-intel · 2018-04-04T08:56:14Z

virtcontainers/kata_builtin_proxy.go

+			if err == io.EOF {
+				logger.Info("console wather quits")
+			} else {
+				logger.Errorf("Failed reading agent logs from socket: %v", err)


How about:

logger.WithError(err).WithFields(logrus.Fields{ "console-protocol" : proto, "console-socket" : console, }).Error("Failed to read agent logs")

Sure, I'll add it. Thanks!

bergwolf · 2018-04-05T00:18:22Z

Dropping do not merge label as kata-containers/agent#201 is merged. @sboeuf @jodh-intel PTAL.

sboeuf

A few more comments !

sboeuf · 2018-04-05T07:16:30Z

virtcontainers/kata_builtin_proxy.go

+	go func() {
+		scanner = bufio.NewScanner(conn)
+		for scanner.Scan() {
+			fmt.Printf("[SB-%s] vmconsole: %s\n", p.podID, scanner.Text())


Yes I think the sandbox naming should be done in a separate PR, meaning that it makes more sense to stick with [POD-%s] for now.

sboeuf · 2018-04-05T07:28:48Z

virtcontainers/kata_agent.go

 	if err := k.connect(); err != nil {
 		return nil, err
 	}
-	defer k.disconnect()


Yes you're right but this should not be part of this PR. I think we should go first with what I proposed so that it properly covers the case of the builtin proxy, and later we can submit another PR covering the case of a long running runtime. This should be covered more neatly, by introducing a notion of short running vs long running IMO, and allowing the current use case to still use disconnect(). I don't like that we implicitly handle the disconnection because the process ends.

sboeuf · 2018-04-05T07:33:34Z

virtcontainers/podlist.go

+	p.lock.Lock()
+	defer p.lock.Unlock()
+	delete(p.pods, id)
+	return nil


No reason to return an error. Let's have this function returning nothing then.

sboeuf · 2018-04-05T07:35:29Z

Gopkg.toml

 [[constraint]]
  name = "github.com/kata-containers/agent"
-  revision = "a93071539feee29bfa22b6184380d3fd7c156ef7"
+  revision = "c8199f60759a1d52932df37a1af710cdeada8c81"


Please add the shortlog diff since the last vendoring of this dependency into your commit message. It helps tracking which patches are pulled through this vendoring.

Good point! I'll add it. Thanks!

Hey, sorry to be picky here, but we don't really need the merge commits. If you run the following command:

git log --no-merges --abbrev-commit --pretty=oneline a93071539feee29bfa22b6184380d3fd7c156ef7..origin/master

You should only get the real commits.

Taking notes and commit message updated. Thanks @sboeuf!

devimc · 2018-04-05T12:45:19Z

virtcontainers/podlist.go

+func (p *podList) lookupPod(id string) (pod *Pod, err error) {
+	p.lock.RLock()
+	defer p.lock.RUnlock()
+	if p.pods[id] != nil {


I think this looks better

if p.pods[id] != nil { return p.pods[id], nil } return nil, fmt.Errorf("pod %s does not exist", id)

Yup, I'll change it. Thanks!

To include the grpc yamux dialer. Included kata agent git shortlog: Archana Shinde (1): mount: Correct error message with mount failure. Eric Ernst (2): Merge pull request kata-containers#196 from sboeuf/fix_rollback Merge pull request kata-containers#201 from bergwolf/yamux_client Graham whaley (1): ci: lib: allow override of tests_repo James O. D. Hunt (16): mount: Log params and validate device: Add validation and debug tests: Move helper function tests: Use root skip function tests: Skip more tests if non-root CI: Require pullapprove ack for protocol changes agent: Add announce function announce: Add standard fields when running as PID 1 announce: Add device and storage handlers announce: Add total memory Merge pull request kata-containers#180 from jodh-intel/add-announce-log-call github: Add issue template Merge pull request kata-containers#189 from grahamwhaley/20180322_tests_repo Merge pull request kata-containers#187 from jodh-intel/github-issue-template Merge pull request kata-containers#192 from sboeuf/fix_device Merge pull request kata-containers#197 from amshinde/correct-err-msg Lai Jiangshan (1): Merge pull request kata-containers#176 from jodh-intel/pullapprove-for-grpc-changes Peng Tao (3): Merge pull request kata-containers#171 from jodh-intel/validate-mount-params Merge pull request kata-containers#206 from sboeuf/fix_rollback protocol: client: enable builtin yamux client support Sebastien Boeuf (7): Merge pull request kata-containers#178 from jodh-intel/skip-more-tests-if-not-root Merge pull request kata-containers#173 from jodh-intel/add-device-debug device: VmPath can be empty if an Id is provided agent: Rollback properly when container creation fails vendor: Update libcontainer vendoring Merge pull request kata-containers#203 from sboeuf/revendor_libcontainer agent: Fix container creation rollback Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf · 2018-04-07T04:46:35Z

@devimc @jodh-intel @sboeuf PR updated. PTAL.

bergwolf · 2018-04-08T08:36:18Z

Jenkins failed on #193

sboeuf

One comment about the shortlog from the vendoring commit, but other than that LGTM !

sboeuf · 2018-04-09T13:58:58Z

Gopkg.toml

 [[constraint]]
  name = "github.com/kata-containers/agent"
-  revision = "a93071539feee29bfa22b6184380d3fd7c156ef7"
+  revision = "c8199f60759a1d52932df37a1af710cdeada8c81"


Hey, sorry to be picky here, but we don't really need the merge commits. If you run the following command:

git log --no-merges --abbrev-commit --pretty=oneline a93071539feee29bfa22b6184380d3fd7c156ef7..origin/master

You should only get the real commits.

sboeuf · 2018-04-09T14:08:47Z

Thx @bergwolf
LGTM

bergwolf · 2018-04-10T02:11:37Z

Jenkins failed on kata-containers/tests/issues/225

sboeuf · 2018-04-10T05:12:20Z

@bergwolf have you rebased on latest master version ? Just want to make sure you have #193 included.

When specified, it does not spawn a new process to proxy kata grpc connections. Instead, the yamux multiplexing functionality is builtin in the kata agent dialer. Signed-off-by: Peng Tao <bergwolf@gmail.com>

To include the grpc yamux dialer. Included kata agent git log: e37feac protocol: client: enable builtin yamux client support a862fea agent: Fix container creation rollback 9602e11 vendor: Update libcontainer vendoring 92f87a1 agent: Rollback properly when container creation fails 128f87d mount: Correct error message with mount failure. 7a182a4 device: VmPath can be empty if an Id is provided 0275654 ci: lib: allow override of tests_repo 205a4d7 github: Add issue template 103aacd announce: Add total memory e277ec6 announce: Add device and storage handlers 5d7463f announce: Add standard fields when running as PID 1 4655950 agent: Add announce function 5e6c385 CI: Require pullapprove ack for protocol changes 5d40027 tests: Skip more tests if non-root 4ba8499 tests: Use root skip function 9a2da30 tests: Move helper function ae2be84 device: Add validation and debug 9e7b27c mount: Log params and validate Signed-off-by: Peng Tao <bergwolf@gmail.com>

It tracks all existing pods in the current runtime. If the runtime calls multiple APIs, it can reuse existing pod data structure instead of re-construct it in every API call. Signed-off-by: Peng Tao <bergwolf@gmail.com>

When set, the kata shim will not be created. Fixes: kata-containers#172 Signed-off-by: Peng Tao <bergwolf@gmail.com>

To test built-in proxy and shim types. Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf · 2018-04-10T05:18:57Z

@sboeuf rebased now.

Otherwise there might be cached pod alive even if we remove all the config dirs etc. Signed-off-by: Peng Tao <bergwolf@gmail.com>

devimc

lgtm

bergwolf · 2018-04-10T15:07:27Z

ping @kata-containers/runtime

device: Add validation and debug

amshinde added the review label Apr 3, 2018

sboeuf suggested changes Apr 3, 2018

View reviewed changes

bergwolf force-pushed the proxy branch from d1c50fb to 5d4eca1 Compare April 4, 2018 08:16

bergwolf force-pushed the proxy branch from d21294b to b2f458f Compare April 4, 2018 08:44

bergwolf added the do-not-merge label Apr 4, 2018

bergwolf force-pushed the proxy branch from b2f458f to 97e649a Compare April 4, 2018 08:51

jodh-intel reviewed Apr 4, 2018

View reviewed changes

bergwolf force-pushed the proxy branch 3 times, most recently from dcb1287 to 60f7391 Compare April 5, 2018 00:16

bergwolf removed the do-not-merge label Apr 5, 2018

bergwolf force-pushed the proxy branch 3 times, most recently from d3c920e to dc11182 Compare April 5, 2018 01:04

sboeuf reviewed Apr 5, 2018

View reviewed changes

devimc reviewed Apr 5, 2018

View reviewed changes

bergwolf force-pushed the proxy branch from dc11182 to 1036256 Compare April 7, 2018 04:43

bergwolf force-pushed the proxy branch from 1036256 to 5e430dd Compare April 7, 2018 04:45

bergwolf force-pushed the proxy branch 2 times, most recently from 14b8d21 to f3c070b Compare April 8, 2018 05:37

sboeuf approved these changes Apr 9, 2018

View reviewed changes

bergwolf force-pushed the proxy branch from f3c070b to 1b8ecca Compare April 9, 2018 14:05

bergwolf force-pushed the proxy branch from 1b8ecca to 5c8140b Compare April 10, 2018 01:26

bergwolf added 5 commits April 10, 2018 13:17

proxy: add kataProxyBuiltin

5a9ff3c

When specified, it does not spawn a new process to proxy kata grpc connections. Instead, the yamux multiplexing functionality is builtin in the kata agent dialer. Signed-off-by: Peng Tao <bergwolf@gmail.com>

pod: add a in-memory pod list

1d291fc

It tracks all existing pods in the current runtime. If the runtime calls multiple APIs, it can reuse existing pod data structure instead of re-construct it in every API call. Signed-off-by: Peng Tao <bergwolf@gmail.com>

shim: add kata builtin shim type

f483d6f

When set, the kata shim will not be created. Fixes: kata-containers#172 Signed-off-by: Peng Tao <bergwolf@gmail.com>

proxy/shim: add unit tests

81bb561

To test built-in proxy and shim types. Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf force-pushed the proxy branch from 5c8140b to 3975101 Compare April 10, 2018 05:18

test: delete pod in test cleanup

b353e36

Otherwise there might be cached pod alive even if we remove all the config dirs etc. Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf force-pushed the proxy branch from 3975101 to b353e36 Compare April 10, 2018 09:30

devimc approved these changes Apr 10, 2018

View reviewed changes

devimc mentioned this pull request Apr 10, 2018

virtcontainers: agent: fix CPU hot plug race condition #91

Merged

egernst approved these changes Apr 10, 2018

View reviewed changes

egernst merged commit be151cb into kata-containers:master Apr 10, 2018

egernst removed the review label Apr 10, 2018

bergwolf deleted the proxy branch April 11, 2018 08:38

bergwolf mentioned this pull request May 9, 2018

virtcontainers: define and implement pod based external API #49

Closed

zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019

Merge pull request kata-containers#173 from jodh-intel/add-device-debug

48a37cf

device: Add validation and debug

Conversation

bergwolf commented Apr 3, 2018

Uh oh!

sboeuf 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!

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!

bergwolf commented Apr 4, 2018

Uh oh!

bergwolf commented Apr 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergwolf commented Apr 5, 2018

Uh oh!

sboeuf 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!

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!

bergwolf commented Apr 7, 2018

Uh oh!

bergwolf commented Apr 8, 2018

Uh oh!

sboeuf left a comment

Choose a reason for hiding this comment

Uh oh!