network: Move OCI hooks and network namespace creation out of virtcontainers by sboeuf · Pull Request #623 · kata-containers/runtime

sboeuf · 2018-08-22T07:47:45Z

Before we decide to move to a fully hotpluggable network model, we want to go step by step by making sure we can still support the network being coldplugged in case of kata-runtime, but also letting the possibility to consumer of the API (such as frakti) to hotplug the network interfaces, without having virtcontainers doing some prework there.

This PR introduces a few rework and fixes, but the most important part is about the creation of the network namespace and the execution of the OCI hooks being handled directly from the CLI. This allows the Kata API to be OCI agnostic.

Fixes #599

sboeuf · 2018-08-22T07:51:54Z

@egernst @WeiZhang555 @bergwolf @amshinde @caoruidong PTAL

FYI, this PR does not supersede #600, but shares part of the commits. The approach here is less aggressive as we're not dropping the network coldplug, but we let the possibility for the network not to be created if we don't want to (frakti case with the factory).

We need to discuss if we want eventually to move to a fully hopluggable model, but this is beyond this PR scope.

katacontainersbot · 2018-08-22T08:18:15Z

PSS Measurement:
Qemu: 169182 KB
Proxy: 4151 KB
Shim: 8884 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003588 KB

opendev-zuul · 2018-08-22T09:10:11Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : TIMED_OUT in 1h 01m 13s

jodh-intel · 2018-08-22T09:41:44Z

This is going to conflict with #617, so please could folk take a look at that one :)

jodh-intel · 2018-08-22T14:42:39Z

cli/create.go

+	}
+
+	// Execute prestart OCI hooks
+	if err := enterNetNS(sandboxConfig.NetworkConfig.NetNSPath, func() error {


This is a bit of a mind-bender :) I think it would be clearer if you assigned to err and then checked err on a separate line.

Ok fair enough ;)

jodh-intel · 2018-08-22T14:44:45Z

virtcontainers/noop_resource_storage.go

+	"github.com/kata-containers/runtime/virtcontainers/device/api"
+)
+
+type noopResourceStorage struct{}


Nice. Since you've added this though, how about splitting filesystem.go into:

resource_storage.go (type resourceStorage interface only).

resource_storage_filesystem.go

And renaming filesystem_test.go to resource_storage_filesystem_test.go ?

Yes, this makes sense, let me take care of this.

addressed in other PR (already merged)

jodh-intel · 2018-08-22T14:52:27Z

cli/hook.go

+	return nil
+}
+
+func postStartHooks(spec oci.CompatOCISpec, cid, bundlePath string) error {


This function and postStopHooks() are almost identical to preStartHooks(), so how about having them all call a generic function. Something like:

func runHooks(cid, bundlePath string, hooks []Hook, name string) error { } func preStartHooks(spec oci.CompatOCISpec, cid, bundlePath string) error { if spec.Hooks == nil { return nil } return runHooks(cid, bundlePath, spec.Hooks.Prestart, "pre-start") }

Okay I can work on this

jodh-intel · 2018-08-22T14:53:24Z

cli/start.go

+
+		if err := enterNetNS(s.GetNetNs(), func() error {
+			return postStartHooks(ociSpec, sandboxID, status.Annotations[vcAnnot.BundlePathKey])
+		}); err != nil {


Same comment as above - this is compact, but rather difficult to understand ;)

jodh-intel · 2018-08-22T14:55:18Z

virtcontainers/default_network.go

+type defNetwork struct {
+}
+
+func defNetworkLogger() *logrus.Entry {


Why not?:

func (n *defNetwork) logger() *logrus.Entry

sure, that works !

jodh-intel · 2018-08-22T14:57:04Z

virtcontainers/network.go

 	return endpoint, nil
 }

+func generateInterfacesAndRoutes(networkNS NetworkNamespace) ([]*grpc.Interface, []*grpc.Route, error) {


I'm pretty surprised the linters don't complain about the length/complexity of this function ;)

codecov · 2018-08-22T17:30:11Z

Codecov Report

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

@@            Coverage Diff            @@
##             master     #623   +/-   ##
=========================================
  Coverage          ?   64.92%           
=========================================
  Files             ?       85           
  Lines             ?     9589           
  Branches          ?        0           
=========================================
  Hits              ?     6226           
  Misses            ?     2717           
  Partials          ?      646

katacontainersbot · 2018-08-22T17:46:29Z

PSS Measurement:
Qemu: 165401 KB
Proxy: 4238 KB
Shim: 8920 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003564 KB

opendev-zuul · 2018-08-22T18:01:32Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : TIMED_OUT in 1h 01m 06s

egernst · 2018-08-22T22:24:02Z

virtcontainers/network.go

-	// CNMNetworkModel is the CNM network.
-	CNMNetworkModel NetworkModel = "CNM"
+	// DefaultNetworkModel is the default network.
+	DefaultNetworkModel NetworkModel = "default"


Having a defaultNetworkModel and just a Noop model seems a bit awkward now? Really we're either having a network or not at this point, from what I can see? Maybe just a better name than 'default?'

Yeah tell me which name I could use, but I agree "default" is definitely not great...
A bit more background here, initially I wanted to remove the interface since it was not really needed as we now have only one real implementation, but I realized that our unit testing would be way more complicated/impossible? if we decide to remove the noop implementation.

@egernst I think naming the network to Default is a bit awkward, lets rework this in a separate PR if required.

egernst · 2018-08-22T22:27:57Z

This is a big PR - nice work. I feel like some of the commits would be better off in a seperate PR? In particular:
virtcontainers: qemu: Don't shutdown QMP from hotplug

And probably:
virtcontainers: kata_agent: Move out a generic function
virtcontainers: storage: Add a noop version of filesystem

caoruidong · 2018-08-23T01:51:47Z

Most of changes are code movement, generally good

bergwolf

While I see this is mostly moving codes from virtcontainers to cli, there are a couple of issues that we should address IMHO.

bergwolf · 2018-08-23T03:34:08Z

cli/create.go

+
+	// Execute prestart OCI hooks
+	if err := enterNetNS(sandboxConfig.NetworkConfig.NetNSPath, func() error {
+		return preStartHooks(ociSpec, containerID, bundlePath)


This assumes that only the very first container of a sandbox has prestart hooks. However, OCI hooks are part of the OCI spec that every container can include. So IMO we should call these hooks for every container instead of just on the first container of a sandbox.

This makes sense, even if I don't expect any hook for containers following the sandbox container.

bergwolf · 2018-08-23T03:35:50Z

cli/delete.go


 	switch containerType {
 	case vc.PodSandbox:
+		if err := postStopHooks(ociSpec, sandboxID, status.Annotations[vcAnnot.BundlePathKey]); err != nil {


Same here. It only runs the post stop hooks of the last container of a sandbox.

Also post stop hooks are supposed to run after a container is stopped. However, here we might have called them before the container is stopped.

Oh yes good point, I have to move this slightly further in the code.

bergwolf · 2018-08-23T03:46:58Z

cli/hook.go

+
+	var stdout, stderr bytes.Buffer
+	cmd := &exec.Cmd{
+		Path:   hook.Path,


I think we must validate the hook commands (e.g., with a configurable command binary whitelist) before really executing any of them. For example, imagine a public container cloud provider that runs kata containers inside, and then here comes a user who creates a container with rm -rf / in the OCI spec hook. BOOM!

It looks like runc does not handle it either. But kata is supposed to be more secure than runc. :)

I am trying not to introduce too much new code, simply doing a clean move from virtcontainers to the CLI. I think your proposal should go through a follow up PR.

@sboeuf Please open an issue to track this.

amshinde · 2018-08-23T22:34:13Z

virtcontainers/network.go

 	return endpoint, nil
 }

+func generateInterfacesAndRoutes(networkNS NetworkNamespace) ([]*grpc.Interface, []*grpc.Route, error) {


This code is very Kata agent specific since it deals with grpc and not applicable for other agents. So I think it should be it in kata agent.

I understand, but if you look at the Kata API, we're using both grpc.Interface and grpc.Route structures as the generic structures to add interfaces or routes.
I think moving this function out of kata_agent.go is the first step, and the second logical step would be to move those very generic structure definitions out of kata-containers/agent/protocols/grpc. It'd might be better to have them in a separate package so that they don't get associated with the kata-agent.

amshinde · 2018-08-23T22:50:20Z

cli/hook.go

+
+	var stdout, stderr bytes.Buffer
+	cmd := &exec.Cmd{
+		Path:   hook.Path,


@sboeuf Please open an issue to track this.

sboeuf · 2018-08-24T19:54:13Z

Rebased! I'll address the comments now.

Since we removed the CNI implementation and that we agreed the network should only be handled in a single way from virtcontainers, this patch logically replace the "CNM" naming with "Default". Signed-off-by: Sebastien Boeuf <[email protected]>

This commit moves the network namespace creation out of virtcontainers in order to anticipate the move of the OCI hooks to the CLI through a follow up commit. Signed-off-by: Sebastien Boeuf <[email protected]>

As we want to call the OCI hook from the CLI, we need a way for the CLI to figure out what is the network namespace used by the sandbox. This is needed particularly because virtcontainers creates the netns if none was provided. Signed-off-by: Sebastien Boeuf <[email protected]>

sboeuf · 2018-08-24T21:31:51Z

@jodh-intel all your comments addressed. The rebase on top of the opentracing was trickier than I thought it'd be :)

The CLI being the implementation of the OCI specification, and the hooks being OCI specific, it makes sense to move the handling of any OCI hooks to the CLI level. This changes allows the Kata API to become OCI agnostic. Fixes kata-containers#599 Signed-off-by: Sebastien Boeuf <[email protected]>

If the sandbox has been initialized with a factory, this means the caller should be in charge of adding any network to the VM, and virtcontainers library cannot make any assumptions about adding the default underlying network. Signed-off-by: Sebastien Boeuf <[email protected]>

sboeuf · 2018-08-24T22:26:34Z

@bergwolf I have addressed your comments, PTAL

opendev-zuul · 2018-08-24T23:10:36Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : TIMED_OUT in 1h 01m 06s

katacontainersbot · 2018-08-25T00:45:30Z

PSS Measurement:
Qemu: 169727 KB
Proxy: 4267 KB
Shim: 8816 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003564 KB

bergwolf

Looks good overall. Just have one comment about the place to call prestart hooks.

bergwolf · 2018-08-28T02:55:44Z

cli/create.go

+
+	// Run pre-start OCI hooks.
+	err = enterNetNS(sandboxConfig.NetworkConfig.NetNSPath, func() error {
+		return preStartHooks(ctx, ociSpec, containerID, bundlePath)


While it can work, I wonder why we are not calling it in start.go? After all, they are preStart hooks not preCreate hooks.

@bergwolf This is what the OCI spec calls them. We have simply chosen to call it in create as we need the network hook to be called. @sboeuf and I had a discussion about moving to a hotplug model, but lets address that in a separate PR.

I also believe we need to evaluate if there are certain hooks that need to be run inside the VM.

I think it's worth pointing out that both the current code, and this PR are somewhat abusing the standard:

https://github.com/opencontainers/runtime-spec/blob/master/config.md#prestart

@amshinde @sboeuf I think we will need to move it to start.go otherwise vm factory will still be lacking network configurations. But I agree with you that it can be handled in a following up PR.

amshinde

lgtm

amshinde · 2018-08-28T17:21:48Z

@egernst Can you take a look at this? I would like to get this merged if it looks good to you.

egernst · 2018-08-29T21:02:17Z

change is fine with me, I'll want to get @bergwolf 's ACK. I'd prefer a less awkward naming scheme (defaultNetwork when we only have one network is a bit odd), but don't want to delay this too much longer when I'm not providing a better alternative...

amshinde · 2018-08-31T06:30:40Z

@bergwolf Can you take another look at this? Are you ok with merging this as is and address the hooks issue in a separate PR. Else we can wait for @sboeuf when he is back next week.

bergwolf · 2018-08-31T07:41:49Z

cli/create.go

+
+	// Run pre-start OCI hooks.
+	err = enterNetNS(sandboxConfig.NetworkConfig.NetNSPath, func() error {
+		return preStartHooks(ctx, ociSpec, containerID, bundlePath)


@amshinde @sboeuf I think we will need to move it to start.go otherwise vm factory will still be lacking network configurations. But I agree with you that it can be handled in a following up PR.

bergwolf · 2018-08-31T07:45:51Z

Well, it looks like pullapprove really dislikes my approval and just keeps ignoring it. I'm going to ignore it as well, again.

sboeuf · 2018-09-04T16:25:47Z

Thanks for merging this ;)

Routes with proto kernel are automatically added by the kernel when an interface is added with ip assigned as a subnet address. With github.com/kata-containers#1936, these routes will no longer be sent from the runtime. Depends-on: github.com/kata-containers#1936 Fixes: kata-containers#623 Signed-off-by: Archana Shinde <[email protected]>

egernst added the review label Aug 22, 2018

sboeuf requested a review from amshinde August 22, 2018 07:48

sboeuf force-pushed the move_hooks branch from 649347c to e7fea79 Compare August 22, 2018 07:53

jodh-intel reviewed Aug 22, 2018

View reviewed changes

sboeuf force-pushed the move_hooks branch from e7fea79 to fd4c6fe Compare August 22, 2018 16:22

egernst reviewed Aug 22, 2018

View reviewed changes

bergwolf reviewed Aug 23, 2018

View reviewed changes

sboeuf mentioned this pull request Aug 23, 2018

virtcontainers: network: Add VFIO hotplug support #620

Closed

amshinde reviewed Aug 23, 2018

View reviewed changes

This was referenced Aug 23, 2018

virtcontainers: qemu: Don't shutdown QMP from hotplug #628

Merged

virtcontainers: kata_agent: Move out a generic function #630

Merged

virtcontainers: storage: Rework resource storage #634

Merged

sboeuf force-pushed the move_hooks branch from fd4c6fe to 0b85113 Compare August 24, 2018 19:52

sboeuf force-pushed the move_hooks branch from 0b85113 to 74be4f4 Compare August 24, 2018 20:49

Sebastien Boeuf added 3 commits August 24, 2018 14:12

network: Create network namespace from the CLI

cb351dc

This commit moves the network namespace creation out of virtcontainers in order to anticipate the move of the OCI hooks to the CLI through a follow up commit. Signed-off-by: Sebastien Boeuf <[email protected]>

sboeuf force-pushed the move_hooks branch 2 times, most recently from 03d731a to 39cfbba Compare August 24, 2018 21:30

Sebastien Boeuf added 2 commits August 24, 2018 15:07

sboeuf force-pushed the move_hooks branch from 39cfbba to 97d280e Compare August 24, 2018 22:07

bergwolf reviewed Aug 28, 2018

View reviewed changes

amshinde approved these changes Aug 28, 2018

View reviewed changes

amshinde mentioned this pull request Aug 28, 2018

network: Add support for macvlan and macvtap driver #506

Merged

bergwolf approved these changes Aug 31, 2018

View reviewed changes

bergwolf merged commit b982373 into kata-containers:master Aug 31, 2018

egernst removed the review label Aug 31, 2018

sboeuf added the enhancement Improvement to an existing feature label Sep 12, 2018

WeiZhang555 mentioned this pull request Sep 17, 2018

namespaces: How to enter them in a foolproof way #148

Closed

Conversation

sboeuf commented Aug 22, 2018

Uh oh!

sboeuf commented Aug 22, 2018

Uh oh!

katacontainersbot commented Aug 22, 2018

Uh oh!

opendev-zuul bot commented Aug 22, 2018

Uh oh!

jodh-intel commented Aug 22, 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!

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 Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

katacontainersbot commented Aug 22, 2018

Uh oh!

opendev-zuul bot commented Aug 22, 2018

Uh oh!

egernst Aug 22, 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!

egernst commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caoruidong commented Aug 23, 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!

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

codecov bot commented Aug 22, 2018 •

edited

Loading

egernst Aug 22, 2018 •

edited

Loading

egernst commented Aug 22, 2018 •

edited

Loading