Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

[DNM] network: Move to network hotplug #600

Closed
wants to merge 12 commits into from

Conversation

sboeuf
Copy link

@sboeuf sboeuf commented Aug 17, 2018

This patchset introduces all the changes needed to move to a fully hotplugged network instead of the legacy coldplug that was in place.
Also, going along with those changes, this patchset moves the handling of the OCI hooks out of virtcontainers, allowing for an OCI agnostic Kata API.

Fixes #598
Fixes #599

@sboeuf
Copy link
Author

sboeuf commented Aug 17, 2018

This PR is blocked by the fact that we cannot move to a fully hotplugged model until we get both VFIO network and vhost-user hotplug support.

@sboeuf
Copy link
Author

sboeuf commented Aug 17, 2018

@sboeuf sboeuf changed the title network: Move to network hotplug [DNM] network: Move to network hotplug Aug 17, 2018
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 167161 KB
Proxy: 6350 KB
Shim: 8882 KB

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

@opendev-zuul
Copy link

opendev-zuul bot commented Aug 17, 2018

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

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 164958 KB
Proxy: 4159 KB
Shim: 8794 KB

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

@egernst
Copy link
Member

egernst commented Aug 17, 2018

will want to make sure we test this with all CNI/CNM plugins (incl vhost-user based and SRIOV/VFIO based).

@egernst
Copy link
Member

egernst commented Aug 20, 2018

@sboeuf - are there changes in the gRPC protocol which this is reliant on since 1.2?

@sboeuf
Copy link
Author

sboeuf commented Aug 20, 2018

@egernst no, nothing related to GRPC changes here.

Sebastien Boeuf added 12 commits August 20, 2018 13:21
Some cnmLogger() where called from the common network handling file
network.go. This patch replaces those unproper calls with the one
related to network.go, that is networLogger().

Signed-off-by: Sebastien Boeuf <[email protected]>
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]>
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]>
This patch slightly modifies the network interface in order to
facilitate the transition to a fully hotplugged model.

Signed-off-by: Sebastien Boeuf <[email protected]>
The QMP shutdown is taken care of by the sandbox release, through a
call to hypervisor.disconnect(). By shutting down the QMP at the qemu
level directly, we are creating some unrecoverable errors by trying to
close an already closed channel.

This patch simply removes the faulty code, following the same design
other hotplug functions are designed.

Signed-off-by: Sebastien Boeuf <[email protected]>
The specific agent implementation kata_agent was defining a very
useful generic function that is now moved to the global file
network.go

Signed-off-by: Sebastien Boeuf <[email protected]>
This noop implementation of resourceStorage will allow for easier
unit testing of some sandbox functions.

Signed-off-by: Sebastien Boeuf <[email protected]>
Because we want to move the OCI hooks out of virtcontainers, we have
to make sure we are actually able to hotplug the network as a first
step. This patch is an in-between state where we hotplug the network
even if we don't strictly need to.

Fixes kata-containers#598

Signed-off-by: Sebastien Boeuf <[email protected]>
Because at some point we will need from the caller of virtcontainers
to be able to split the sandbox creation from the network creation,
we have to introduce a new exported function to allow this level of
granularity.

Signed-off-by: Sebastien Boeuf <[email protected]>
Now that Kata API has an external function to setup the network
anytime after the sandbox has been created, we can call this from
the CLI, and remove it from virtcontainers.

The purpose of this commit is to anticipate the OCI hooks handling
to be moved from virtcontainers to the CLI.

Signed-off-by: Sebastien Boeuf <[email protected]>
First of all, RunSandbox() was never used since an OCI "run" was
actually relying on CreateSandbox() + StartSandbox(), meaning there
was no consumer of this function.

Second, and that's the main reason here, with the refactoring of the
network, by moving part of the network creation up to the CLI, a big
function like RunSandbox() cannot exist. The granularity is not thin
enough, which would end up setting up the network after this function
returns, which would be wrong as the sandbox and its containers would
be running already.

Signed-off-by: Sebastien Boeuf <[email protected]>
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]>
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 166978 KB
Proxy: 4236 KB
Shim: 8848 KB

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

@opendev-zuul
Copy link

opendev-zuul bot commented Aug 20, 2018

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 00m 34s

@codecov
Copy link

codecov bot commented Aug 20, 2018

Codecov Report

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

@@            Coverage Diff            @@
##             master     #600   +/-   ##
=========================================
  Coverage          ?   63.26%           
=========================================
  Files             ?       85           
  Lines             ?     9585           
  Branches          ?        0           
=========================================
  Hits              ?     6064           
  Misses            ?     2889           
  Partials          ?      632

@egernst
Copy link
Member

egernst commented Aug 23, 2018

I mentioned this in side discussions, but for clarity here: I'd like to see a higher level design description with respect to kata-runtime for this feature, and show why this is helpful (ie, improved boot time, etc).

@sboeuf sboeuf added enhancement Improvement to an existing feature feature New functionality labels Sep 12, 2018
@raravena80
Copy link
Member

Hi, @sboeuf any updates on this? why is it marked do not merge? Thx!

@sboeuf
Copy link
Author

sboeuf commented Sep 24, 2018

@raravena80 this PR can be dropped as I don't think we'll move to a fully hotplugged model by default. We already have Kata able to hotplug new network.

See #598 for reference about the discussion we already had on the topic.

@sboeuf sboeuf closed this Sep 24, 2018
egernst pushed a commit to egernst/runtime that referenced this pull request Feb 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvement to an existing feature feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants