dp: Expose link based resources by default (#11)

* Fix formatting

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

* DP: fix and improve documentation of OnLinkEvent

Wrong break statement was preventing the subsciption to be properly
terminated.

Improved function doc to better explain behavior on error.

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

* DP: make structs private for cleanliness

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

* DP: work on main process namespace

Allocate, ListAndWatch and the netlink subsciption callbacks are
executed on their own goroutines. As documented here [1], go thread
model may relocate goroutines among different OS threads that might be
running on different namespaces. This patch makes sure that network
operations always run on initial main thread namespace. Otherwise the
device plugin may end up operating in different namespaces than the one
intended.

[1] https://github.com/containernetworking/plugins/blob/master/pkg/ns/README.md

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

* DP: added explicit config tests

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

* DP: expose link based resources by default

When no explicit configuration is provided, the device plugin will expose
default resources corresponding to links present on the node. So, for
every link considered suitable as a macvtap parent, a resource
to consume macvtap interfaces would be setup as
macvtap.network.kubevirt.io/<link>

Right now only physical and bond interfaces are considered suitable
macvtap parents. These resources will be operating in bridge mode
and capacity of 100.

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

* DP: exit on Discover initial error

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

README.md

@@ -25,7 +25,7 @@ to be made available:
 * `capacity` (uint, optional, default=100) the capacity of the resource
 
 In the default deployment, this configuration shall be provided through a
-config map, for [example](examples/macvtap-deviceplugin-config.yaml):
+config map, for [example](examples/macvtap-deviceplugin-config-explicit.yaml):
 
 ```yaml
 kind: ConfigMap
@@ -51,6 +51,13 @@ This configuration will result in up to 50 macvtap interfaces being offered for
 consumption, using eth0 as the lower device, in bridge mode, and under
 resource name `macvtap.network.kubevirt.io/dataplane`.
 
+A configuration consisting of an empty json array, as proposed in the default
+[example](examples/macvtap-deviceplugin-config-default.yaml), causes the device
+plugin to expose one resource for every physical link or bond on each node. For
+example, if a node has a physical link called eth0, a resourced named
+`macvtap.network.kubevirt.io/eth0` would be made available to use macvtap
+interfaces with eth0 as the lower device
+
 The macvtap CNI can be deployed using the proposed
 [daemon set](manifests/macvtap.yaml):
 

cluster/sync.sh

@@ -19,5 +19,5 @@ DESTINATION=$destination IMAGE_REGISTRY=registry:5000 make manifests
 ./cluster/kubectl.sh delete --ignore-not-found configmap macvtap-deviceplugin-config
 ./cluster/kubectl.sh delete --ignore-not-found ds macvtap-cni
 
-./cluster/kubectl.sh create -f examples/macvtap-deviceplugin-config.yaml
+./cluster/kubectl.sh create -f examples/macvtap-deviceplugin-config-default.yaml
 ./cluster/kubectl.sh create -f _out/manifests/macvtap.yaml

cmd/deviceplugin/macvtap.go

@@ -7,16 +7,24 @@ import (
 	"github.com/golang/glog"
 	"github.com/kubevirt/device-plugin-manager/pkg/dpm"
 	macvtap "github.com/kubevirt/macvtap-cni/pkg/deviceplugin"
+	"github.com/kubevirt/macvtap-cni/pkg/util"
 )
 
 func main() {
 	flag.Parse()
+	// Device plugin operates with several goroutines that might be
+	// relocated among different OS threads with different namespaces.
+	// We capture the main namespace here and make sure that we do any
+	// network operation on that namespace.
+	// See
+	// https://github.com/containernetworking/plugins/blob/master/pkg/ns/README.md
+	mainNsPath := util.GetMainThreadNetNsPath()
 
 	_, configDefined := os.LookupEnv(macvtap.ConfigEnvironmentVariable)
 	if !configDefined {
 		glog.Exitf("%s environment variable must be set", macvtap.ConfigEnvironmentVariable)
 	}
 
-	manager := dpm.NewManager(macvtap.MacvtapLister{})
+	manager := dpm.NewManager(macvtap.NewMacvtapLister(mainNsPath))
 	manager.Run()
 }

examples/macvtap-deviceplugin-config-default.yaml

@@ -0,0 +1,6 @@
+kind: ConfigMap
+apiVersion: v1
+metadata:
+  name: macvtap-deviceplugin-config
+data:
+  DP_MACVTAP_CONF: "[]"

go.mod

@@ -15,6 +15,7 @@ require (
 	github.com/onsi/gomega v1.8.1
 	github.com/tomnomnom/linkheader v0.0.0-20180905144013-02ca5825eb80 // indirect
 	github.com/vishvananda/netlink v1.1.0
+	github.com/vishvananda/netns v0.0.0-20191106174202-0a2b9b5464df
 	github.com/voxelbrain/goptions v0.0.0-20180630082107-58cddc247ea2 // indirect
 	golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3 // indirect
 	golang.org/x/net v0.0.0-20190812203447-cdfb69ac37fc

pkg/deviceplugin/lister.go

@@ -4,32 +4,43 @@ import (
 	"encoding/json"
 	"os"
 
+	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/golang/glog"
 	"github.com/kubevirt/device-plugin-manager/pkg/dpm"
+	"github.com/kubevirt/macvtap-cni/pkg/util"
 )
 
 const (
 	resourceNamespace         = "macvtap.network.kubevirt.io"
 	ConfigEnvironmentVariable = "DP_MACVTAP_CONF"
 )
 
-type MacvtapConfig struct {
+type macvtapConfig struct {
 	Name     string `json:"name"`
 	Master   string `json:"master"`
 	Mode     string `json:"mode"`
 	Capacity int    `json:"capacity"`
 }
 
-type MacvtapLister struct {
+type macvtapLister struct {
+	Config map[string]macvtapConfig
+	// NetNsPath is the path to the network namespace the lister operates in.
+	NetNsPath string
 }
 
-func (ml MacvtapLister) GetResourceNamespace() string {
+func NewMacvtapLister(netNsPath string) *macvtapLister {
+	return &macvtapLister{
+		NetNsPath: netNsPath,
+	}
+}
+
+func (ml macvtapLister) GetResourceNamespace() string {
 	return resourceNamespace
 }
 
-func readConfig() (map[string]MacvtapConfig, error) {
-	var config []MacvtapConfig
-	configMap := make(map[string]MacvtapConfig)
+func readConfig() (map[string]macvtapConfig, error) {
+	var config []macvtapConfig
+	configMap := make(map[string]macvtapConfig)
 
 	configEnv := os.Getenv(ConfigEnvironmentVariable)
 	err := json.Unmarshal([]byte(configEnv), &config)
@@ -44,13 +55,13 @@ func readConfig() (map[string]MacvtapConfig, error) {
 	return configMap, nil
 }
 
-func (ml MacvtapLister) Discover(pluginListCh chan dpm.PluginNameList) {
+func discoverByConfig(pluginListCh chan dpm.PluginNameList) (map[string]macvtapConfig, error) {
 	var plugins = make(dpm.PluginNameList, 0)
 
 	config, err := readConfig()
 	if err != nil {
 		glog.Errorf("Error reading config: %v", err)
-		return
+		return nil, err
 	}
 
 	glog.V(3).Infof("Read configuration %+v", config)
@@ -59,16 +70,100 @@ func (ml MacvtapLister) Discover(pluginListCh chan dpm.PluginNameList) {
 		plugins = append(plugins, macvtapConfig.Name)
 	}
 
-	pluginListCh <- plugins
+	if len(plugins) > 0 {
+		pluginListCh <- plugins
+	}
+	return config, nil
+}
+
+func discoverByLinks(pluginListCh chan dpm.PluginNameList, netNsPath string) error {
+	// To know when the manager is stoping, we need to read from pluginListCh.
+	// We avoid reading our own updates by using a middle channel.
+	// We buffer up to one msg because of the initial call to sendSuitableParents.
+	parentListCh := make(chan []string, 1)
+	defer close(parentListCh)
+
+	sendSuitableParents := func() error {
+		var linkNames []string
+		err := ns.WithNetNSPath(netNsPath, func(_ ns.NetNS) error {
+			var err error
+			linkNames, err = util.FindSuitableMacvtapParents()
+			return err
+		})
+
+		if err != nil {
+			glog.Errorf("Error while finding links: %v", err)
+			return err
+		}
+
+		parentListCh <- linkNames
+		return nil
+	}
+
+	// Do an initial search to catch early permanent runtime problems
+	err := sendSuitableParents()
+	if err != nil {
+		return err
+	}
+
+	// Keep updating on changes for suitable parents.
+	stop := make(chan struct{})
+	defer close(stop)
+	go util.OnSuitableMacvtapParentEvent(
+		netNsPath,
+		// Wrapper to ignore error
+		func() {
+			sendSuitableParents()
+		},
+		stop,
+		func(err error) {
+			glog.Error(err)
+		})
+
+	// Keep forwarding updates to the manager until it closes down
+	for {
+		select {
+		case parentNames := <-parentListCh:
+			pluginListCh <- parentNames
+		case _, open := <-pluginListCh:
+			if !open {
+				return nil
+			}
+		}
+	}
 }
 
-func (ml MacvtapLister) NewPlugin(name string) dpm.PluginInterface {
-	config, _ := readConfig()
-	glog.V(3).Infof("Creating device plugin with config %+v", config[name])
-	return NewMacvtapDevicePlugin(
-		config[name].Name,
-		config[name].Master,
-		config[name].Mode,
-		config[name].Capacity,
-	)
+func (ml *macvtapLister) Discover(pluginListCh chan dpm.PluginNameList) {
+	config, err := discoverByConfig(pluginListCh)
+	if err != nil {
+		os.Exit(1)
+	}
+
+	// Configuration is static and we don't need to do anything else
+	ml.Config = config
+	if len(config) > 0 {
+		return
+	}
+
+	// If there was no configuration, we setup resources based on the existing
+	// links of the host.
+	err = discoverByLinks(pluginListCh, ml.NetNsPath)
+	if err != nil {
+		os.Exit(1)
+	}
+}
+
+func (ml *macvtapLister) NewPlugin(name string) dpm.PluginInterface {
+	c, ok := ml.Config[name]
+	if !ok {
+		c = macvtapConfig{
+			Name:     name,
+			Master:   name,
+			Mode:     DefaultMode,
+			Capacity: DefaultCapacity,
+		}
+	}
+
+	glog.V(3).Infof("Creating device plugin with config %+v", c)
+	return NewMacvtapDevicePlugin(c.Name, c.Master, c.Mode, c.Capacity, ml.NetNsPath)
 }

pkg/deviceplugin/plugin.go

@@ -3,6 +3,7 @@ package deviceplugin
 import (
 	"fmt"
 
+	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/golang/glog"
 	"golang.org/x/net/context"
 	pluginapi "k8s.io/kubernetes/pkg/kubelet/apis/deviceplugin/v1beta1"
@@ -13,34 +14,40 @@ import (
 const (
 	tapPath = "/dev/tap"
 	// Interfaces will be named as <Name><suffix>[0-<Capacity>]
-	suffix          = "Mvp"
-	defaultCapacity = 100
+	suffix = "Mvp"
+	// DefaultCapacity is the default when no capacity is provided
+	DefaultCapacity = 100
+	// DefaultMode is the default when no mode is provided
+	DefaultMode = "bridge"
 )
 
-type MacvtapDevicePlugin struct {
-	Name        string
-	Master      string
-	Mode        string
-	Capacity    int
+type macvtapDevicePlugin struct {
+	Name     string
+	Master   string
+	Mode     string
+	Capacity int
+	// NetNsPath is the path to the network namespace the plugin operates in.
+	NetNsPath   string
 	stopWatcher chan struct{}
 }
 
-func NewMacvtapDevicePlugin(name string, master string, mode string, capacity int) *MacvtapDevicePlugin {
-	return &MacvtapDevicePlugin{
+func NewMacvtapDevicePlugin(name string, master string, mode string, capacity int, netNsPath string) *macvtapDevicePlugin {
+	return &macvtapDevicePlugin{
 		Name:        name,
 		Master:      master,
 		Mode:        mode,
 		Capacity:    capacity,
+		NetNsPath:   netNsPath,
 		stopWatcher: make(chan struct{}),
 	}
 }
 
-func (mdp *MacvtapDevicePlugin) generateMacvtapDevices() []*pluginapi.Device {
+func (mdp *macvtapDevicePlugin) generateMacvtapDevices() []*pluginapi.Device {
 	var macvtapDevs []*pluginapi.Device
 
 	var capacity = mdp.Capacity
 	if capacity <= 0 {
-		capacity = defaultCapacity
+		capacity = DefaultCapacity
 	}
 
 	for i := 0; i < capacity; i++ {
@@ -54,7 +61,7 @@ func (mdp *MacvtapDevicePlugin) generateMacvtapDevices() []*pluginapi.Device {
 	return macvtapDevs
 }
 
-func (mdp *MacvtapDevicePlugin) ListAndWatch(e *pluginapi.Empty, s pluginapi.DevicePlugin_ListAndWatchServer) error {
+func (mdp *macvtapDevicePlugin) ListAndWatch(e *pluginapi.Empty, s pluginapi.DevicePlugin_ListAndWatchServer) error {
 	// Initialize two arrays, one for devices offered when master exists,
 	// and no devices if master does not exist.
 	allocatableDevs := mdp.generateMacvtapDevices()
@@ -72,22 +79,29 @@ func (mdp *MacvtapDevicePlugin) ListAndWatch(e *pluginapi.Empty, s pluginapi.Dev
 
 	didMasterExist := false
 	onMasterEvent := func() {
-		doesMasterExist, err := util.LinkExists(mdp.Master)
+		var doesMasterExist bool
+		err := ns.WithNetNSPath(mdp.NetNsPath, func(_ ns.NetNS) error {
+			var err error
+			doesMasterExist, err = util.LinkExists(mdp.Master)
+			return err
+		})
 		if err != nil {
 			glog.Warningf("Error while checking on master %s: %v", mdp.Master, err)
 			return
 		}
+
 		if didMasterExist != doesMasterExist {
 			emitResponse(doesMasterExist)
 			didMasterExist = doesMasterExist
 		}
 	}
 
 	// Listen for events of master interface. On any, check if master a
-	// interface exists. If it does, offer up to capacity mactvtap devices. Do
+	// interface exists. If it does, offer up to capacity macvtap devices. Do
 	// not offer any otherwise.
 	util.OnLinkEvent(
 		mdp.Master,
+		mdp.NetNsPath,
 		onMasterEvent,
 		mdp.stopWatcher,
 		func(err error) {
@@ -97,7 +111,7 @@ func (mdp *MacvtapDevicePlugin) ListAndWatch(e *pluginapi.Empty, s pluginapi.Dev
 	return nil
 }
 
-func (mdp *MacvtapDevicePlugin) Allocate(ctx context.Context, r *pluginapi.AllocateRequest) (*pluginapi.AllocateResponse, error) {
+func (mdp *macvtapDevicePlugin) Allocate(ctx context.Context, r *pluginapi.AllocateRequest) (*pluginapi.AllocateResponse, error) {
 	var response pluginapi.AllocateResponse
 
 	for _, req := range r.ContainerRequests {
@@ -113,7 +127,12 @@ func (mdp *MacvtapDevicePlugin) Allocate(ctx context.Context, r *pluginapi.Alloc
 			// no de-allocate flow to clean up. So we attempt to delete a
 			// possibly existing existing interface before creating it to reset
 			// its state.
-			index, err := util.RecreateMacvtap(name, mdp.Master, mdp.Mode)
+			var index int
+			err := ns.WithNetNSPath(mdp.NetNsPath, func(_ ns.NetNS) error {
+				var err error
+				index, err = util.RecreateMacvtap(name, mdp.Master, mdp.Mode)
+				return err
+			})
 			if err != nil {
 				return nil, err
 			}
@@ -133,15 +152,15 @@ func (mdp *MacvtapDevicePlugin) Allocate(ctx context.Context, r *pluginapi.Alloc
 	return &response, nil
 }
 
-func (mdp *MacvtapDevicePlugin) PreStartContainer(context.Context, *pluginapi.PreStartContainerRequest) (*pluginapi.PreStartContainerResponse, error) {
+func (mdp *macvtapDevicePlugin) PreStartContainer(context.Context, *pluginapi.PreStartContainerRequest) (*pluginapi.PreStartContainerResponse, error) {
 	return nil, nil
 }
 
-func (mdp *MacvtapDevicePlugin) GetDevicePluginOptions(context.Context, *pluginapi.Empty) (*pluginapi.DevicePluginOptions, error) {
+func (mdp *macvtapDevicePlugin) GetDevicePluginOptions(context.Context, *pluginapi.Empty) (*pluginapi.DevicePluginOptions, error) {
 	return nil, nil
 }
 
-func (mdp *MacvtapDevicePlugin) Stop() error {
+func (mdp *macvtapDevicePlugin) Stop() error {
 	close(mdp.stopWatcher)
 	return nil
 }