From b39cb1d13a283322a489f9a746eeb8f99e173eda Mon Sep 17 00:00:00 2001 From: Samuel Ortiz Date: Fri, 25 Jan 2019 12:32:09 +0100 Subject: [PATCH 1/2] virtcontainers: Remove the network interface There's only one real implementer of the network interface and no real need to implement anything else. We can just go ahead and remove this abstraction. Fixes: #1179 Signed-off-by: Samuel Ortiz --- virtcontainers/README.md | 2 - virtcontainers/api_test.go | 15 +- virtcontainers/default_network.go | 108 -------------- virtcontainers/documentation/api/1.0/api.md | 16 -- virtcontainers/endpoint_test.go | 1 - virtcontainers/hack/virtc/main.go | 12 -- virtcontainers/iostream_test.go | 2 +- virtcontainers/monitor_test.go | 4 +- virtcontainers/network.go | 153 ++++++++++++-------- virtcontainers/network_test.go | 74 ---------- virtcontainers/noop_network.go | 31 ---- virtcontainers/pkg/oci/utils.go | 1 - virtcontainers/pkg/oci/utils_test.go | 1 - virtcontainers/sandbox.go | 19 +-- virtcontainers/sandbox_test.go | 44 +++--- 15 files changed, 129 insertions(+), 354 deletions(-) delete mode 100644 virtcontainers/default_network.go delete mode 100644 virtcontainers/noop_network.go diff --git a/virtcontainers/README.md b/virtcontainers/README.md index 491d56b3e..a2778971f 100644 --- a/virtcontainers/README.md +++ b/virtcontainers/README.md @@ -159,8 +159,6 @@ An example tool using the `virtcontainers` API is provided in the `hack/virtc` p Typically the former is the Docker default networking model while the later is used on Kubernetes deployments. -`virtcontainers` callers can select one or the other, on a per sandbox basis, by setting their `SandboxConfig`'s `NetworkModel` field properly. - [cnm]: https://github.com/docker/libnetwork/blob/master/docs/design.md [cni]: https://github.com/containernetworking/cni/ diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go index eed59f680..f7aebf720 100644 --- a/virtcontainers/api_test.go +++ b/virtcontainers/api_test.go @@ -155,7 +155,6 @@ func newTestSandboxConfigHyperstartAgentDefaultNetwork() SandboxConfig { AgentType: HyperstartAgent, AgentConfig: agentConfig, - NetworkModel: DefaultNetworkModel, NetworkConfig: netConfig, Containers: []ContainerConfig{container}, @@ -2020,7 +2019,7 @@ func TestProcessListContainer(t *testing.T) { * Benchmarks */ -func createNewSandboxConfig(hType HypervisorType, aType AgentType, aConfig interface{}, netModel NetworkModel) SandboxConfig { +func createNewSandboxConfig(hType HypervisorType, aType AgentType, aConfig interface{}) SandboxConfig { hypervisorConfig := HypervisorConfig{ KernelPath: "/usr/share/kata-containers/vmlinux.container", ImagePath: "/usr/share/kata-containers/kata-containers.img", @@ -2037,7 +2036,6 @@ func createNewSandboxConfig(hType HypervisorType, aType AgentType, aConfig inter AgentType: aType, AgentConfig: aConfig, - NetworkModel: netModel, NetworkConfig: netConfig, } } @@ -2187,7 +2185,7 @@ func createStartStopDeleteContainers(b *testing.B, sandboxConfig SandboxConfig, func BenchmarkCreateStartStopDeleteSandboxQemuHypervisorHyperstartAgentNetworkNoop(b *testing.B) { for i := 0; i < b.N; i++ { - sandboxConfig := createNewSandboxConfig(QemuHypervisor, HyperstartAgent, HyperConfig{}, NoopNetworkModel) + sandboxConfig := createNewSandboxConfig(QemuHypervisor, HyperstartAgent, HyperConfig{}) sockDir, err := testGenerateCCProxySockDir() if err != nil { @@ -2208,21 +2206,21 @@ func BenchmarkCreateStartStopDeleteSandboxQemuHypervisorHyperstartAgentNetworkNo func BenchmarkCreateStartStopDeleteSandboxQemuHypervisorNoopAgentNetworkNoop(b *testing.B) { for i := 0; i < b.N; i++ { - sandboxConfig := createNewSandboxConfig(QemuHypervisor, NoopAgentType, nil, NoopNetworkModel) + sandboxConfig := createNewSandboxConfig(QemuHypervisor, NoopAgentType, nil) createStartStopDeleteSandbox(b, sandboxConfig) } } func BenchmarkCreateStartStopDeleteSandboxMockHypervisorNoopAgentNetworkNoop(b *testing.B) { for i := 0; i < b.N; i++ { - sandboxConfig := createNewSandboxConfig(MockHypervisor, NoopAgentType, nil, NoopNetworkModel) + sandboxConfig := createNewSandboxConfig(MockHypervisor, NoopAgentType, nil) createStartStopDeleteSandbox(b, sandboxConfig) } } func BenchmarkStartStop1ContainerQemuHypervisorHyperstartAgentNetworkNoop(b *testing.B) { for i := 0; i < b.N; i++ { - sandboxConfig := createNewSandboxConfig(QemuHypervisor, HyperstartAgent, HyperConfig{}, NoopNetworkModel) + sandboxConfig := createNewSandboxConfig(QemuHypervisor, HyperstartAgent, HyperConfig{}) contConfigs := createNewContainerConfigs(1) sockDir, err := testGenerateCCProxySockDir() @@ -2244,7 +2242,7 @@ func BenchmarkStartStop1ContainerQemuHypervisorHyperstartAgentNetworkNoop(b *tes func BenchmarkStartStop10ContainerQemuHypervisorHyperstartAgentNetworkNoop(b *testing.B) { for i := 0; i < b.N; i++ { - sandboxConfig := createNewSandboxConfig(QemuHypervisor, HyperstartAgent, HyperConfig{}, NoopNetworkModel) + sandboxConfig := createNewSandboxConfig(QemuHypervisor, HyperstartAgent, HyperConfig{}) contConfigs := createNewContainerConfigs(10) sockDir, err := testGenerateCCProxySockDir() @@ -2448,7 +2446,6 @@ func TestNetworkOperation(t *testing.T) { defer deleteNetNS(netNSPath) config := newTestSandboxConfigNoop() - config.NetworkModel = DefaultNetworkModel config.NetworkConfig = NetworkConfig{ NetNSPath: netNSPath, } diff --git a/virtcontainers/default_network.go b/virtcontainers/default_network.go deleted file mode 100644 index 4b14c1254..000000000 --- a/virtcontainers/default_network.go +++ /dev/null @@ -1,108 +0,0 @@ -// Copyright (c) 2016 Intel Corporation -// -// SPDX-License-Identifier: Apache-2.0 -// - -package virtcontainers - -import ( - "context" - - "github.com/containernetworking/plugins/pkg/ns" - opentracing "github.com/opentracing/opentracing-go" - "github.com/sirupsen/logrus" -) - -type defNetwork struct { -} - -func (n *defNetwork) logger() *logrus.Entry { - return virtLog.WithField("subsystem", "default-network") -} - -func (n *defNetwork) trace(ctx context.Context, name string) (opentracing.Span, context.Context) { - span, ct := opentracing.StartSpanFromContext(ctx, name) - - span.SetTag("subsystem", "network") - span.SetTag("type", "default") - - return span, ct -} - -// run runs a callback in the specified network namespace. -func (n *defNetwork) run(networkNSPath string, cb func() error) error { - span, _ := n.trace(context.Background(), "run") - defer span.Finish() - - return doNetNS(networkNSPath, func(_ ns.NetNS) error { - return cb() - }) -} - -// add adds all needed interfaces inside the network namespace. -func (n *defNetwork) add(s *Sandbox, hotplug bool) error { - span, _ := n.trace(s.ctx, "add") - defer span.Finish() - - endpoints, err := createEndpointsFromScan(s.config.NetworkConfig.NetNSPath, s.config.NetworkConfig) - if err != nil { - return err - } - - s.networkNS.Endpoints = endpoints - - err = doNetNS(s.config.NetworkConfig.NetNSPath, func(_ ns.NetNS) error { - for _, endpoint := range s.networkNS.Endpoints { - n.logger().WithField("endpoint-type", endpoint.Type()).WithField("hotplug", hotplug).Info("Attaching endpoint") - if hotplug { - if err := endpoint.HotAttach(s.hypervisor); err != nil { - return err - } - } else { - if err := endpoint.Attach(s.hypervisor); err != nil { - return err - } - } - } - - return nil - }) - if err != nil { - return err - } - - n.logger().Debug("Network added") - - return nil -} - -// remove network endpoints in the network namespace. It also deletes the network -// namespace in case the namespace has been created by us. -func (n *defNetwork) remove(s *Sandbox, hotunplug bool) error { - span, _ := n.trace(s.ctx, "remove") - defer span.Finish() - - for _, endpoint := range s.networkNS.Endpoints { - // Detach for an endpoint should enter the network namespace - // if required. - n.logger().WithField("endpoint-type", endpoint.Type()).WithField("hotunplug", hotunplug).Info("Detaching endpoint") - if hotunplug { - if err := endpoint.HotDetach(s.hypervisor, s.networkNS.NetNsCreated, s.networkNS.NetNsPath); err != nil { - return err - } - } else { - if err := endpoint.Detach(s.networkNS.NetNsCreated, s.networkNS.NetNsPath); err != nil { - return err - } - } - } - - n.logger().Debug("Network removed") - - if s.networkNS.NetNsCreated { - n.logger().Infof("Network namespace %q deleted", s.networkNS.NetNsPath) - return deleteNetNS(s.networkNS.NetNsPath) - } - - return nil -} diff --git a/virtcontainers/documentation/api/1.0/api.md b/virtcontainers/documentation/api/1.0/api.md index c4d1150d3..d652fb291 100644 --- a/virtcontainers/documentation/api/1.0/api.md +++ b/virtcontainers/documentation/api/1.0/api.md @@ -40,7 +40,6 @@ sandbox lifecycle through the rest of the [sandbox API](#sandbox-functions). * [ProxyType](#proxytype) * [ProxyConfig](#proxyconfig) * [ShimType](#shimtype) - * [NetworkModel](#networkmodel) * [NetworkConfig](#networkconfig) * [NetInterworkingModel](#netinterworkingmodel) * [Volume](#volume) @@ -76,7 +75,6 @@ type SandboxConfig struct { ShimType ShimType ShimConfig interface{} - NetworkModel NetworkModel NetworkConfig NetworkConfig // Volumes is a list of shared volumes between the host and the Sandbox. @@ -257,20 +255,6 @@ const ( ) ``` -##### `NetworkModel` -```Go -// NetworkModel describes the type of network specification. -type NetworkModel string - -const ( - // NoopNetworkModel is the No-Op network. - NoopNetworkModel NetworkModel = "noop" - - // CNMNetworkModel is the CNM network. - CNMNetworkModel NetworkModel = "CNM" -) -``` - ##### `NetworkConfig` ```Go // NetworkConfig is the network configuration related to a network. diff --git a/virtcontainers/endpoint_test.go b/virtcontainers/endpoint_test.go index 251791fcb..e5473a96c 100644 --- a/virtcontainers/endpoint_test.go +++ b/virtcontainers/endpoint_test.go @@ -8,7 +8,6 @@ package virtcontainers import "testing" func testEndpointTypeSet(t *testing.T, value string, expected EndpointType) { - //var netModel NetworkModel var endpointType EndpointType err := endpointType.Set(value) diff --git a/virtcontainers/hack/virtc/main.go b/virtcontainers/hack/virtc/main.go index 7521d90ab..2c25d4303 100644 --- a/virtcontainers/hack/virtc/main.go +++ b/virtcontainers/hack/virtc/main.go @@ -52,12 +52,6 @@ var sandboxConfigFlags = []cli.Flag{ Usage: "hypervisor machine type", }, - cli.GenericFlag{ - Name: "network", - Value: new(vc.NetworkModel), - Usage: "the network model", - }, - cli.GenericFlag{ Name: "proxy", Value: new(vc.ProxyType), @@ -150,11 +144,6 @@ func buildSandboxConfig(context *cli.Context) (vc.SandboxConfig, error) { return vc.SandboxConfig{}, fmt.Errorf("Could not convert agent type") } - networkModel, ok := context.Generic("network").(*vc.NetworkModel) - if ok != true { - return vc.SandboxConfig{}, fmt.Errorf("Could not convert network model") - } - proxyType, ok := context.Generic("proxy").(*vc.ProxyType) if ok != true { return vc.SandboxConfig{}, fmt.Errorf("Could not convert proxy type") @@ -212,7 +201,6 @@ func buildSandboxConfig(context *cli.Context) (vc.SandboxConfig, error) { AgentType: *agentType, AgentConfig: agConfig, - NetworkModel: *networkModel, NetworkConfig: netConfig, ProxyType: *proxyType, diff --git a/virtcontainers/iostream_test.go b/virtcontainers/iostream_test.go index af1c423a9..152f0a611 100644 --- a/virtcontainers/iostream_test.go +++ b/virtcontainers/iostream_test.go @@ -13,7 +13,7 @@ import ( func TestIOStream(t *testing.T) { hConfig := newHypervisorConfig(nil, nil) - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NoopNetworkModel, NetworkConfig{}, []ContainerConfig{}, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, []ContainerConfig{}, nil) if err != nil { t.Fatal(err) } diff --git a/virtcontainers/monitor_test.go b/virtcontainers/monitor_test.go index 6b03b4a15..2d786acf3 100644 --- a/virtcontainers/monitor_test.go +++ b/virtcontainers/monitor_test.go @@ -18,7 +18,7 @@ func TestMonitorSuccess(t *testing.T) { hConfig := newHypervisorConfig(nil, nil) // create a sandbox - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NoopNetworkModel, NetworkConfig{}, []ContainerConfig{contConfig}, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, []ContainerConfig{contConfig}, nil) if err != nil { t.Fatal(err) } @@ -43,7 +43,7 @@ func TestMonitorClosedChannel(t *testing.T) { hConfig := newHypervisorConfig(nil, nil) // create a sandbox - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NoopNetworkModel, NetworkConfig{}, []ContainerConfig{contConfig}, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, []ContainerConfig{contConfig}, nil) if err != nil { t.Fatal(err) } diff --git a/virtcontainers/network.go b/virtcontainers/network.go index 777d12fec..94e961572 100644 --- a/virtcontainers/network.go +++ b/virtcontainers/network.go @@ -6,6 +6,7 @@ package virtcontainers import ( + "context" cryptoRand "crypto/rand" "encoding/json" "fmt" @@ -17,6 +18,7 @@ import ( "time" "github.com/containernetworking/plugins/pkg/ns" + opentracing "github.com/opentracing/opentracing-go" "github.com/sirupsen/logrus" "github.com/vishvananda/netlink" "github.com/vishvananda/netns" @@ -347,55 +349,6 @@ func (n *NetworkNamespace) UnmarshalJSON(b []byte) error { return nil } -// NetworkModel describes the type of network specification. -type NetworkModel string - -const ( - // NoopNetworkModel is the No-Op network. - NoopNetworkModel NetworkModel = "noop" - - // DefaultNetworkModel is the default network. - DefaultNetworkModel NetworkModel = "default" -) - -// Set sets a network type based on the input string. -func (networkType *NetworkModel) Set(value string) error { - switch value { - case "noop": - *networkType = NoopNetworkModel - return nil - case "default": - *networkType = DefaultNetworkModel - return nil - default: - return fmt.Errorf("Unknown network type %s", value) - } -} - -// String converts a network type to a string. -func (networkType *NetworkModel) String() string { - switch *networkType { - case NoopNetworkModel: - return string(NoopNetworkModel) - case DefaultNetworkModel: - return string(DefaultNetworkModel) - default: - return "" - } -} - -// newNetwork returns a network from a network type. -func newNetwork(networkType NetworkModel) network { - switch networkType { - case NoopNetworkModel: - return &noopNetwork{} - case DefaultNetworkModel: - return &defNetwork{} - default: - return &noopNetwork{} - } -} - func createLink(netHandle *netlink.Handle, name string, expectedLink netlink.Link, queues int) (netlink.Link, []*os.File, error) { var newLink netlink.Link var fds []*os.File @@ -1464,17 +1417,93 @@ func createEndpoint(netInfo NetworkInfo, idx int, model NetInterworkingModel) (E return endpoint, err } -// network is the virtcontainers network interface. -// Container network plugins are used to setup virtual network -// between VM netns and the host network physical interface. -type network interface { - // run runs a callback function in a specified network namespace. - run(networkNSPath string, cb func() error) error - - // add adds all needed interfaces inside the network namespace. - add(sandbox *Sandbox, hotplug bool) error - - // remove unbridges and deletes TAP interfaces. It also removes virtual network - // interfaces and deletes the network namespace. - remove(sandbox *Sandbox, hotunplug bool) error +// Network is the virtcontainer network structure +type Network struct { +} + +func (n *Network) trace(ctx context.Context, name string) (opentracing.Span, context.Context) { + span, ct := opentracing.StartSpanFromContext(ctx, name) + + span.SetTag("subsystem", "network") + span.SetTag("type", "default") + + return span, ct +} + +// Run runs a callback in the specified network namespace. +func (n *Network) Run(networkNSPath string, cb func() error) error { + span, _ := n.trace(context.Background(), "run") + defer span.Finish() + + return doNetNS(networkNSPath, func(_ ns.NetNS) error { + return cb() + }) +} + +// Add adds all needed interfaces inside the network namespace. +func (n *Network) Add(s *Sandbox, hotplug bool) error { + span, _ := n.trace(s.ctx, "add") + defer span.Finish() + + endpoints, err := createEndpointsFromScan(s.config.NetworkConfig.NetNSPath, s.config.NetworkConfig) + if err != nil { + return err + } + + s.networkNS.Endpoints = endpoints + + err = doNetNS(s.config.NetworkConfig.NetNSPath, func(_ ns.NetNS) error { + for _, endpoint := range s.networkNS.Endpoints { + networkLogger().WithField("endpoint-type", endpoint.Type()).WithField("hotplug", hotplug).Info("Attaching endpoint") + if hotplug { + if err := endpoint.HotAttach(s.hypervisor); err != nil { + return err + } + } else { + if err := endpoint.Attach(s.hypervisor); err != nil { + return err + } + } + } + + return nil + }) + if err != nil { + return err + } + + networkLogger().Debug("Network added") + + return nil +} + +// Remove network endpoints in the network namespace. It also deletes the network +// namespace in case the namespace has been created by us. +func (n *Network) Remove(s *Sandbox, hotunplug bool) error { + span, _ := n.trace(s.ctx, "remove") + defer span.Finish() + + for _, endpoint := range s.networkNS.Endpoints { + // Detach for an endpoint should enter the network namespace + // if required. + networkLogger().WithField("endpoint-type", endpoint.Type()).WithField("hotunplug", hotunplug).Info("Detaching endpoint") + if hotunplug { + if err := endpoint.HotDetach(s.hypervisor, s.networkNS.NetNsCreated, s.networkNS.NetNsPath); err != nil { + return err + } + } else { + if err := endpoint.Detach(s.networkNS.NetNsCreated, s.networkNS.NetNsPath); err != nil { + return err + } + } + } + + networkLogger().Debug("Network removed") + + if s.networkNS.NetNsCreated { + networkLogger().Infof("Network namespace %q deleted", s.networkNS.NetNsPath) + return deleteNetNS(s.networkNS.NetNsPath) + } + + return nil } diff --git a/virtcontainers/network_test.go b/virtcontainers/network_test.go index 601ab70b4..c1d4804f4 100644 --- a/virtcontainers/network_test.go +++ b/virtcontainers/network_test.go @@ -16,80 +16,6 @@ import ( "github.com/vishvananda/netlink" ) -func testNetworkModelSet(t *testing.T, value string, expected NetworkModel) { - var netModel NetworkModel - - err := netModel.Set(value) - if err != nil { - t.Fatal(err) - } - - if netModel != expected { - t.Fatal() - } -} - -func TestNoopNetworkModelSet(t *testing.T) { - testNetworkModelSet(t, "noop", NoopNetworkModel) -} - -func TestDefaultNetworkModelSet(t *testing.T) { - testNetworkModelSet(t, "default", DefaultNetworkModel) -} - -func TestNetworkModelSetFailure(t *testing.T) { - var netModel NetworkModel - - err := netModel.Set("wrong-value") - if err == nil { - t.Fatal(err) - } -} - -func testNetworkModelString(t *testing.T, netModel *NetworkModel, expected string) { - result := netModel.String() - - if result != expected { - t.Fatal() - } -} - -func TestNoopNetworkModelString(t *testing.T) { - netModel := NoopNetworkModel - testNetworkModelString(t, &netModel, string(NoopNetworkModel)) -} - -func TestDefaultNetworkModelString(t *testing.T) { - netModel := DefaultNetworkModel - testNetworkModelString(t, &netModel, string(DefaultNetworkModel)) -} - -func TestWrongNetworkModelString(t *testing.T) { - var netModel NetworkModel - testNetworkModelString(t, &netModel, "") -} - -func testNewNetworkFromNetworkModel(t *testing.T, netModel NetworkModel, expected interface{}) { - result := newNetwork(netModel) - - if reflect.DeepEqual(result, expected) == false { - t.Fatal() - } -} - -func TestNewNoopNetworkFromNetworkModel(t *testing.T) { - testNewNetworkFromNetworkModel(t, NoopNetworkModel, &noopNetwork{}) -} - -func TestNewDefaultNetworkFromNetworkModel(t *testing.T) { - testNewNetworkFromNetworkModel(t, DefaultNetworkModel, &defNetwork{}) -} - -func TestNewUnknownNetworkFromNetworkModel(t *testing.T) { - var netModel NetworkModel - testNewNetworkFromNetworkModel(t, netModel, &noopNetwork{}) -} - func TestCreateDeleteNetNS(t *testing.T) { if os.Geteuid() != 0 { t.Skip(testDisabledAsNonRoot) diff --git a/virtcontainers/noop_network.go b/virtcontainers/noop_network.go deleted file mode 100644 index 6553a96b1..000000000 --- a/virtcontainers/noop_network.go +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright (c) 2016 Intel Corporation -// -// SPDX-License-Identifier: Apache-2.0 -// - -package virtcontainers - -// noopNetwork a.k.a. NO-OP Network is an empty network implementation, for -// testing and mocking purposes. -type noopNetwork struct { -} - -// run runs a callback in the specified network namespace for -// the Noop network. -// It does nothing. -func (n *noopNetwork) run(networkNSPath string, cb func() error) error { - return cb() -} - -// add adds all needed interfaces inside the network namespace the Noop network. -// It does nothing. -func (n *noopNetwork) add(sandbox *Sandbox, hotattach bool) error { - return nil -} - -// remove unbridges and deletes TAP interfaces. It also removes virtual network -// interfaces and deletes the network namespace for the Noop network. -// It does nothing. -func (n *noopNetwork) remove(sandbox *Sandbox, hotdetach bool) error { - return nil -} diff --git a/virtcontainers/pkg/oci/utils.go b/virtcontainers/pkg/oci/utils.go index 02d86f7e9..4ffb4ec95 100644 --- a/virtcontainers/pkg/oci/utils.go +++ b/virtcontainers/pkg/oci/utils.go @@ -480,7 +480,6 @@ func SandboxConfig(ocispec CompatOCISpec, runtime RuntimeConfig, bundlePath, cid ShimType: runtime.ShimType, ShimConfig: runtime.ShimConfig, - NetworkModel: vc.DefaultNetworkModel, NetworkConfig: networkConfig, Containers: []vc.ContainerConfig{containerConfig}, diff --git a/virtcontainers/pkg/oci/utils_test.go b/virtcontainers/pkg/oci/utils_test.go index db7a33cb0..ff1daa9b6 100644 --- a/virtcontainers/pkg/oci/utils_test.go +++ b/virtcontainers/pkg/oci/utils_test.go @@ -226,7 +226,6 @@ func TestMinimalSandboxConfig(t *testing.T) { ProxyType: vc.CCProxyType, ShimType: vc.CCShimType, - NetworkModel: vc.DefaultNetworkModel, NetworkConfig: expectedNetworkConfig, Containers: []vc.ContainerConfig{expectedContainerConfig}, diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 004ad3dc0..60942d4ce 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -70,7 +70,6 @@ type SandboxConfig struct { ShimType ShimType ShimConfig interface{} - NetworkModel NetworkModel NetworkConfig NetworkConfig // Volumes is a list of shared volumes between the host and the Sandbox. @@ -206,7 +205,7 @@ type Sandbox struct { hypervisor hypervisor agent agent storage resourceStorage - network network + network Network monitor *monitor config *SandboxConfig @@ -553,15 +552,12 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor return nil, err } - network := newNetwork(sandboxConfig.NetworkModel) - s := &Sandbox{ id: sandboxConfig.ID, factory: factory, hypervisor: hypervisor, agent: agent, storage: &filesystem{}, - network: network, config: &sandboxConfig, volumes: sandboxConfig.Volumes, containers: map[string]*Container{}, @@ -771,7 +767,7 @@ func (s *Sandbox) startNetworkMonitor() error { sandboxID: s.id, } - return s.network.run(s.networkNS.NetNsPath, func() error { + return s.network.Run(s.networkNS.NetNsPath, func() error { pid, err := startNetmon(params) if err != nil { return err @@ -784,7 +780,8 @@ func (s *Sandbox) startNetworkMonitor() error { } func (s *Sandbox) createNetwork() error { - if s.config.NetworkConfig.DisableNewNetNs { + if s.config.NetworkConfig.DisableNewNetNs || + s.config.NetworkConfig.NetNSPath == "" { return nil } @@ -800,7 +797,7 @@ func (s *Sandbox) createNetwork() error { // after vm is started. if s.factory == nil { // Add the network - if err := s.network.add(s, false); err != nil { + if err := s.network.Add(s, false); err != nil { return err } @@ -825,7 +822,7 @@ func (s *Sandbox) removeNetwork() error { } } - return s.network.remove(s, s.factory != nil) + return s.network.Remove(s, s.factory != nil) } func (s *Sandbox) generateNetInfo(inf *vcTypes.Interface) (NetworkInfo, error) { @@ -929,7 +926,7 @@ func (s *Sandbox) startVM() error { s.Logger().Info("Starting VM") - if err := s.network.run(s.networkNS.NetNsPath, func() error { + if err := s.network.Run(s.networkNS.NetNsPath, func() error { if s.factory != nil { vm, err := s.factory.GetVM(ctx, VMConfig{ HypervisorType: s.config.HypervisorType, @@ -957,7 +954,7 @@ func (s *Sandbox) startVM() error { // In case of vm factory, network interfaces are hotplugged // after vm is started. if s.factory != nil { - if err := s.network.add(s, true); err != nil { + if err := s.network.Add(s, true); err != nil { return err } diff --git a/virtcontainers/sandbox_test.go b/virtcontainers/sandbox_test.go index 4951c7dff..39701332d 100644 --- a/virtcontainers/sandbox_test.go +++ b/virtcontainers/sandbox_test.go @@ -41,7 +41,7 @@ func newHypervisorConfig(kernelParams []Param, hParams []Param) HypervisorConfig func testCreateSandbox(t *testing.T, id string, htype HypervisorType, hconfig HypervisorConfig, atype AgentType, - nmodel NetworkModel, nconfig NetworkConfig, containers []ContainerConfig, + nconfig NetworkConfig, containers []ContainerConfig, volumes []types.Volume) (*Sandbox, error) { sconfig := SandboxConfig{ @@ -49,7 +49,6 @@ func testCreateSandbox(t *testing.T, id string, HypervisorType: htype, HypervisorConfig: hconfig, AgentType: atype, - NetworkModel: nmodel, NetworkConfig: nconfig, Volumes: volumes, Containers: containers, @@ -80,7 +79,7 @@ func testCreateSandbox(t *testing.T, id string, } func TestCreateEmptySandbox(t *testing.T) { - _, err := testCreateSandbox(t, testSandboxID, MockHypervisor, HypervisorConfig{}, NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + _, err := testCreateSandbox(t, testSandboxID, MockHypervisor, HypervisorConfig{}, NoopAgentType, NetworkConfig{}, nil, nil) if err == nil { t.Fatalf("VirtContainers should not allow empty sandboxes") } @@ -88,7 +87,7 @@ func TestCreateEmptySandbox(t *testing.T) { } func TestCreateEmptyHypervisorSandbox(t *testing.T) { - _, err := testCreateSandbox(t, testSandboxID, QemuHypervisor, HypervisorConfig{}, NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + _, err := testCreateSandbox(t, testSandboxID, QemuHypervisor, HypervisorConfig{}, NoopAgentType, NetworkConfig{}, nil, nil) if err == nil { t.Fatalf("VirtContainers should not allow sandboxes with empty hypervisors") } @@ -98,7 +97,7 @@ func TestCreateEmptyHypervisorSandbox(t *testing.T) { func TestCreateMockSandbox(t *testing.T) { hConfig := newHypervisorConfig(nil, nil) - _, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + _, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, nil, nil) if err != nil { t.Fatal(err) } @@ -108,7 +107,7 @@ func TestCreateMockSandbox(t *testing.T) { func TestCreateSandboxEmptyID(t *testing.T) { hConfig := newHypervisorConfig(nil, nil) - p, err := testCreateSandbox(t, "", MockHypervisor, hConfig, NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + p, err := testCreateSandbox(t, "", MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, nil, nil) if err == nil { t.Fatalf("Expected sandbox with empty ID to fail, but got sandbox %v", p) } @@ -118,7 +117,7 @@ func TestCreateSandboxEmptyID(t *testing.T) { func testSandboxStateTransition(t *testing.T, state types.StateString, newState types.StateString) error { hConfig := newHypervisorConfig(nil, nil) - p, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + p, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, nil, nil) if err != nil { return err } @@ -536,7 +535,7 @@ func TestSandboxSetSandboxAndContainerState(t *testing.T) { hConfig := newHypervisorConfig(nil, nil) // create a sandbox - p, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NoopNetworkModel, NetworkConfig{}, []ContainerConfig{contConfig}, nil) + p, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, []ContainerConfig{contConfig}, nil) if err != nil { t.Fatal(err) } @@ -896,7 +895,7 @@ func TestSandboxGetContainer(t *testing.T) { } hConfig := newHypervisorConfig(nil, nil) - p, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + p, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, nil, nil) if err != nil { t.Fatal(err) } @@ -942,7 +941,7 @@ func TestContainerSetStateBlockIndex(t *testing.T) { } hConfig := newHypervisorConfig(nil, nil) - sandbox, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NoopNetworkModel, NetworkConfig{}, containers, nil) + sandbox, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, containers, nil) if err != nil { t.Fatal(err) } @@ -1041,7 +1040,7 @@ func TestContainerStateSetFstype(t *testing.T) { } hConfig := newHypervisorConfig(nil, nil) - sandbox, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NoopNetworkModel, NetworkConfig{}, containers, nil) + sandbox, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, containers, nil) if err != nil { t.Fatal(err) } @@ -1315,7 +1314,7 @@ func TestRemoveContainerSuccess(t *testing.T) { } func TestCreateContainer(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1326,7 +1325,7 @@ func TestCreateContainer(t *testing.T) { } func TestDeleteContainer(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1343,7 +1342,7 @@ func TestDeleteContainer(t *testing.T) { } func TestStartContainer(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1363,7 +1362,7 @@ func TestStartContainer(t *testing.T) { } func TestStatusContainer(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1383,7 +1382,7 @@ func TestStatusContainer(t *testing.T) { } func TestStatusSandbox(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1391,7 +1390,7 @@ func TestStatusSandbox(t *testing.T) { } func TestEnterContainer(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1415,7 +1414,7 @@ func TestEnterContainer(t *testing.T) { } func TestMonitor(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1435,7 +1434,7 @@ func TestMonitor(t *testing.T) { } func TestWaitProcess(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1465,7 +1464,7 @@ func TestWaitProcess(t *testing.T) { } func TestSignalProcess(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1495,7 +1494,7 @@ func TestSignalProcess(t *testing.T) { } func TestWinsizeProcess(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1525,7 +1524,7 @@ func TestWinsizeProcess(t *testing.T) { } func TestContainerProcessIOStream(t *testing.T) { - s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NoopNetworkModel, NetworkConfig{}, nil, nil) + s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil) assert.Nil(t, err, "VirtContainers should not allow empty sandboxes") defer cleanUp() @@ -1749,7 +1748,6 @@ func TestStartNetworkMonitor(t *testing.T) { }, }, }, - network: &defNetwork{}, networkNS: NetworkNamespace{ NetNsPath: fmt.Sprintf("/proc/%d/task/%d/ns/net", os.Getpid(), unix.Gettid()), }, From 18dcd2c2f7a9e83af85e4c725126e5b45957cc0f Mon Sep 17 00:00:00 2001 From: Samuel Ortiz Date: Fri, 25 Jan 2019 14:45:26 +0100 Subject: [PATCH 2/2] virtcontainers: Decouple the network API from the sandbox one In order to fix #1059, we want to create a hypervisor package. Some of the hypervisor implementations (qemu) depend on the network and endpoint interfaces. We can not have a virtcontainers -> hypervisor -> network, endpoint -> virtcontainers cyclic dependency. So before creating the hypervisor package, we need to decouple the network API from the virtcontainers one. Fixes: #1180 Signed-off-by: Samuel Ortiz --- virtcontainers/network.go | 40 +++++++++++++++++++-------------------- virtcontainers/sandbox.go | 12 +++++++++--- 2 files changed, 28 insertions(+), 24 deletions(-) diff --git a/virtcontainers/network.go b/virtcontainers/network.go index 94e961572..01a666c8a 100644 --- a/virtcontainers/network.go +++ b/virtcontainers/network.go @@ -1300,7 +1300,7 @@ func networkInfoFromLink(handle *netlink.Handle, link netlink.Link) (NetworkInfo }, nil } -func createEndpointsFromScan(networkNSPath string, config NetworkConfig) ([]Endpoint, error) { +func createEndpointsFromScan(networkNSPath string, config *NetworkConfig) ([]Endpoint, error) { var endpoints []Endpoint netnsHandle, err := netns.GetFromPath(networkNSPath) @@ -1441,26 +1441,24 @@ func (n *Network) Run(networkNSPath string, cb func() error) error { } // Add adds all needed interfaces inside the network namespace. -func (n *Network) Add(s *Sandbox, hotplug bool) error { - span, _ := n.trace(s.ctx, "add") +func (n *Network) Add(ctx context.Context, config *NetworkConfig, hypervisor hypervisor, hotplug bool) ([]Endpoint, error) { + span, _ := n.trace(ctx, "add") defer span.Finish() - endpoints, err := createEndpointsFromScan(s.config.NetworkConfig.NetNSPath, s.config.NetworkConfig) + endpoints, err := createEndpointsFromScan(config.NetNSPath, config) if err != nil { - return err + return endpoints, err } - s.networkNS.Endpoints = endpoints - - err = doNetNS(s.config.NetworkConfig.NetNSPath, func(_ ns.NetNS) error { - for _, endpoint := range s.networkNS.Endpoints { + err = doNetNS(config.NetNSPath, func(_ ns.NetNS) error { + for _, endpoint := range endpoints { networkLogger().WithField("endpoint-type", endpoint.Type()).WithField("hotplug", hotplug).Info("Attaching endpoint") if hotplug { - if err := endpoint.HotAttach(s.hypervisor); err != nil { + if err := endpoint.HotAttach(hypervisor); err != nil { return err } } else { - if err := endpoint.Attach(s.hypervisor); err != nil { + if err := endpoint.Attach(hypervisor); err != nil { return err } } @@ -1469,30 +1467,30 @@ func (n *Network) Add(s *Sandbox, hotplug bool) error { return nil }) if err != nil { - return err + return []Endpoint{}, err } networkLogger().Debug("Network added") - return nil + return endpoints, nil } // Remove network endpoints in the network namespace. It also deletes the network // namespace in case the namespace has been created by us. -func (n *Network) Remove(s *Sandbox, hotunplug bool) error { - span, _ := n.trace(s.ctx, "remove") +func (n *Network) Remove(ctx context.Context, ns *NetworkNamespace, hypervisor hypervisor, hotunplug bool) error { + span, _ := n.trace(ctx, "remove") defer span.Finish() - for _, endpoint := range s.networkNS.Endpoints { + for _, endpoint := range ns.Endpoints { // Detach for an endpoint should enter the network namespace // if required. networkLogger().WithField("endpoint-type", endpoint.Type()).WithField("hotunplug", hotunplug).Info("Detaching endpoint") if hotunplug { - if err := endpoint.HotDetach(s.hypervisor, s.networkNS.NetNsCreated, s.networkNS.NetNsPath); err != nil { + if err := endpoint.HotDetach(hypervisor, ns.NetNsCreated, ns.NetNsPath); err != nil { return err } } else { - if err := endpoint.Detach(s.networkNS.NetNsCreated, s.networkNS.NetNsPath); err != nil { + if err := endpoint.Detach(ns.NetNsCreated, ns.NetNsPath); err != nil { return err } } @@ -1500,9 +1498,9 @@ func (n *Network) Remove(s *Sandbox, hotunplug bool) error { networkLogger().Debug("Network removed") - if s.networkNS.NetNsCreated { - networkLogger().Infof("Network namespace %q deleted", s.networkNS.NetNsPath) - return deleteNetNS(s.networkNS.NetNsPath) + if ns.NetNsCreated { + networkLogger().Infof("Network namespace %q deleted", ns.NetNsPath) + return deleteNetNS(ns.NetNsPath) } return nil diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 60942d4ce..1a9e715db 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -797,10 +797,13 @@ func (s *Sandbox) createNetwork() error { // after vm is started. if s.factory == nil { // Add the network - if err := s.network.Add(s, false); err != nil { + endpoints, err := s.network.Add(s.ctx, &s.config.NetworkConfig, s.hypervisor, false) + if err != nil { return err } + s.networkNS.Endpoints = endpoints + if s.config.NetworkConfig.NetmonConfig.Enable { if err := s.startNetworkMonitor(); err != nil { return err @@ -822,7 +825,7 @@ func (s *Sandbox) removeNetwork() error { } } - return s.network.Remove(s, s.factory != nil) + return s.network.Remove(s.ctx, &s.networkNS, s.hypervisor, s.factory != nil) } func (s *Sandbox) generateNetInfo(inf *vcTypes.Interface) (NetworkInfo, error) { @@ -954,10 +957,13 @@ func (s *Sandbox) startVM() error { // In case of vm factory, network interfaces are hotplugged // after vm is started. if s.factory != nil { - if err := s.network.Add(s, true); err != nil { + endpoints, err := s.network.Add(s.ctx, &s.config.NetworkConfig, s.hypervisor, true) + if err != nil { return err } + s.networkNS.Endpoints = endpoints + if s.config.NetworkConfig.NetmonConfig.Enable { if err := s.startNetworkMonitor(); err != nil { return err