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 | 149 ++++++++++++-------- 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, 127 insertions(+), 352 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 491d56b3e5..a2778971f6 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 eed59f6801..f7aebf7205 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 4b14c1254f..0000000000 --- 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 c4d1150d30..d652fb291e 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 251791fcb9..e5473a96c7 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 7521d90ab2..2c25d43031 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 af1c423a92..152f0a6118 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 6b03b4a151..2d786acf37 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 777d12fec2..94e961572f 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 +// 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() - // add adds all needed interfaces inside the network namespace. - add(sandbox *Sandbox, hotplug bool) error + 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 + } - // remove unbridges and deletes TAP interfaces. It also removes virtual network - // interfaces and deletes the network namespace. - remove(sandbox *Sandbox, hotunplug bool) error + 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 601ab70b4b..c1d4804f45 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 6553a96b14..0000000000 --- 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 02d86f7e98..4ffb4ec957 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 db7a33cb0b..ff1daa9b64 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 004ad3dc08..60942d4ce3 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 4951c7dff2..39701332d3 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 94e961572f..01a666c8a0 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 60942d4ce3..1a9e715db7 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