[lxc-devel] [lxd/master] Network: Bridge updates to use common driver functionality

tomponline on Github lxc-bot at linuxcontainers.org
Thu Jul 16 10:01:21 UTC 2020


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 411 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20200716/b670a7b9/attachment.bin>
-------------- next part --------------
From 5f847ad62238eb7b57c7e491f520c42d1347d719 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 16 Jul 2020 10:54:44 +0100
Subject: [PATCH 1/5] lxd/network/driver/common: Adds config different and db
 update common functions

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/network/driver_common.go | 82 ++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/lxd/network/driver_common.go b/lxd/network/driver_common.go
index 85d06a52d3..7889711489 100644
--- a/lxd/network/driver_common.go
+++ b/lxd/network/driver_common.go
@@ -7,9 +7,12 @@ import (
 
 	"github.com/pkg/errors"
 
+	lxd "github.com/lxc/lxd/client"
+	"github.com/lxc/lxd/lxd/cluster"
 	"github.com/lxc/lxd/lxd/instance"
 	"github.com/lxc/lxd/lxd/state"
 	"github.com/lxc/lxd/shared"
+	"github.com/lxc/lxd/shared/api"
 )
 
 // DHCPRange represents a range of IPs from start to end.
@@ -177,3 +180,82 @@ func (n *common) DHCPv6Ranges() []DHCPRange {
 
 	return dhcpRanges
 }
+
+// update applies network config to all nodes and database config (if notify true), and updates internal config.
+func (n *common) update(applyNetwork api.NetworkPut, notify bool) error {
+	// Notify all nodes and update the database.
+	if !notify {
+		// Notify all other nodes to update the network.
+		notifier, err := cluster.NewNotifier(n.state, n.state.Endpoints.NetworkCert(), cluster.NotifyAll)
+		if err != nil {
+			return err
+		}
+
+		err = notifier(func(client lxd.InstanceServer) error {
+			return client.UpdateNetwork(n.name, applyNetwork, "")
+		})
+		if err != nil {
+			return err
+		}
+
+		// Update the database.
+		err = n.state.Cluster.UpdateNetwork(n.name, applyNetwork.Description, applyNetwork.Config)
+		if err != nil {
+			return err
+		}
+	}
+
+	// Update internal config after database has been updated.
+	n.description = applyNetwork.Description
+	n.config = applyNetwork.Config
+
+	return nil
+}
+
+// configChanged compares supplied new config with existing config. Returns a boolean indicating if differences in
+// the config or description were found (and the database record needs updating), and a list of non-user config
+// keys that have changed, and a copy of the current internal network config that can be used to revert if needed.
+func (n *common) configChanged(newNetwork api.NetworkPut) (bool, []string, api.NetworkPut, error) {
+	// Backup the current state.
+	oldNetwork := api.NetworkPut{
+		Description: n.description,
+		Config:      map[string]string{},
+	}
+
+	err := shared.DeepCopy(&n.config, &oldNetwork.Config)
+	if err != nil {
+		return false, nil, oldNetwork, err
+	}
+
+	// Diff the configurations.
+	changedKeys := []string{}
+	dbUpdateNeeded := false
+
+	if newNetwork.Description != n.description {
+		dbUpdateNeeded = true
+	}
+
+	for k, v := range oldNetwork.Config {
+		if v != newNetwork.Config[k] {
+			dbUpdateNeeded = true
+
+			// Add non-user changed key to list of changed keys.
+			if !strings.HasPrefix(k, "user.") && !shared.StringInSlice(k, changedKeys) {
+				changedKeys = append(changedKeys, k)
+			}
+		}
+	}
+
+	for k, v := range newNetwork.Config {
+		if v != oldNetwork.Config[k] {
+			dbUpdateNeeded = true
+
+			// Add non-user changed key to list of changed keys.
+			if !strings.HasPrefix(k, "user.") && !shared.StringInSlice(k, changedKeys) {
+				changedKeys = append(changedKeys, k)
+			}
+		}
+	}
+
+	return dbUpdateNeeded, changedKeys, oldNetwork, nil
+}

From 9cb0b59f653bcaf2ac25b126121980ee9670f00c Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 16 Jul 2020 10:55:52 +0100
Subject: [PATCH 2/5] lxd/network/driver/common: Adds contextual logger

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/network/driver_common.go | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lxd/network/driver_common.go b/lxd/network/driver_common.go
index 7889711489..63fe54812b 100644
--- a/lxd/network/driver_common.go
+++ b/lxd/network/driver_common.go
@@ -13,6 +13,9 @@ import (
 	"github.com/lxc/lxd/lxd/state"
 	"github.com/lxc/lxd/shared"
 	"github.com/lxc/lxd/shared/api"
+	log "github.com/lxc/lxd/shared/log15"
+	"github.com/lxc/lxd/shared/logger"
+	"github.com/lxc/lxd/shared/logging"
 )
 
 // DHCPRange represents a range of IPs from start to end.
@@ -23,19 +26,18 @@ type DHCPRange struct {
 
 // common represents a generic LXD network.
 type common struct {
-	// Properties
+	logger      logger.Logger
 	state       *state.State
 	id          int64
 	name        string
 	netType     string
 	description string
-
-	// config
-	config map[string]string
+	config      map[string]string
 }
 
 // init initialise internal variables.
 func (n *common) init(state *state.State, id int64, name string, netType string, description string, config map[string]string) {
+	n.logger = logging.AddContext(logger.Log, log.Ctx{"driver": netType, "network": name})
 	n.id = id
 	n.name = name
 	n.netType = netType

From 9846dc72faf7ce652957e8f6b2c24aa2848abb19 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 16 Jul 2020 10:56:57 +0100
Subject: [PATCH 3/5] lxd/network/driver/bridge: Simplifies Update function to
 use common update functions

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/network/driver_bridge.go | 144 ++++++++++-------------------------
 1 file changed, 41 insertions(+), 103 deletions(-)

diff --git a/lxd/network/driver_bridge.go b/lxd/network/driver_bridge.go
index 2d9088fc4e..0600d6fe9d 100644
--- a/lxd/network/driver_bridge.go
+++ b/lxd/network/driver_bridge.go
@@ -15,11 +15,11 @@ import (
 
 	"github.com/pkg/errors"
 
-	lxd "github.com/lxc/lxd/client"
 	"github.com/lxc/lxd/lxd/cluster"
 	"github.com/lxc/lxd/lxd/daemon"
 	"github.com/lxc/lxd/lxd/dnsmasq"
 	"github.com/lxc/lxd/lxd/node"
+	"github.com/lxc/lxd/lxd/revert"
 	"github.com/lxc/lxd/lxd/util"
 	"github.com/lxc/lxd/shared"
 	"github.com/lxc/lxd/shared/api"
@@ -1292,136 +1292,74 @@ func (n *bridge) Update(newNetwork api.NetworkPut, notify bool) error {
 	if err != nil {
 		return err
 	}
-	newConfig := newNetwork.Config
 
-	// Backup the current state
-	oldConfig := map[string]string{}
-	oldDescription := n.description
-	err = shared.DeepCopy(&n.config, &oldConfig)
+	dbUpdateNeeeded, changedKeys, oldNetwork, err := n.common.configChanged(newNetwork)
 	if err != nil {
 		return err
 	}
 
-	// Define a function which reverts everything.  Defer this function
-	// so that it doesn't need to be explicitly called in every failing
-	// return path.  Track whether or not we want to undo the changes
-	// using a closure.
-	undoChanges := true
-	defer func() {
-		if undoChanges {
-			// Revert changes to the struct
-			n.config = oldConfig
-			n.description = oldDescription
+	if !dbUpdateNeeeded {
+		return nil // Nothing changed.
+	}
 
-			// Update the database
-			n.state.Cluster.UpdateNetwork(n.name, n.description, n.config)
+	revert := revert.New()
+	defer revert.Fail()
 
-			// Reset any change that was made to the bridge
-			n.setup(newConfig)
-		}
-	}()
+	// Define a function which reverts everything.
+	revert.Add(func() {
+		// Reset changes to all nodes and database.
+		n.common.update(oldNetwork, notify)
 
-	// Diff the configurations
-	changedConfig := []string{}
-	userOnly := true
-	for key := range oldConfig {
-		if oldConfig[key] != newConfig[key] {
-			if !strings.HasPrefix(key, "user.") {
-				userOnly = false
-			}
+		// Reset any change that was made to local bridge.
+		n.setup(newNetwork.Config)
+	})
 
-			if !shared.StringInSlice(key, changedConfig) {
-				changedConfig = append(changedConfig, key)
-			}
-		}
-	}
-
-	for key := range newConfig {
-		if oldConfig[key] != newConfig[key] {
-			if !strings.HasPrefix(key, "user.") {
-				userOnly = false
-			}
-
-			if !shared.StringInSlice(key, changedConfig) {
-				changedConfig = append(changedConfig, key)
-			}
+	// Bring the bridge down entirely if the driver has changed.
+	if shared.StringInSlice("bridge.driver", changedKeys) && n.isRunning() {
+		err = n.Stop()
+		if err != nil {
+			return err
 		}
 	}
 
-	// Skip on no change
-	if len(changedConfig) == 0 && newNetwork.Description == n.description {
-		return nil
-	}
-
-	// Update the network
-	if !userOnly {
-		if shared.StringInSlice("bridge.driver", changedConfig) && n.isRunning() {
-			err = n.Stop()
-			if err != nil {
-				return err
-			}
+	// Detach any external interfaces should no longer be attached.
+	if shared.StringInSlice("bridge.external_interfaces", changedKeys) && n.isRunning() {
+		devices := []string{}
+		for _, dev := range strings.Split(newNetwork.Config["bridge.external_interfaces"], ",") {
+			dev = strings.TrimSpace(dev)
+			devices = append(devices, dev)
 		}
 
-		if shared.StringInSlice("bridge.external_interfaces", changedConfig) && n.isRunning() {
-			devices := []string{}
-			for _, dev := range strings.Split(newConfig["bridge.external_interfaces"], ",") {
-				dev = strings.TrimSpace(dev)
-				devices = append(devices, dev)
+		for _, dev := range strings.Split(oldNetwork.Config["bridge.external_interfaces"], ",") {
+			dev = strings.TrimSpace(dev)
+			if dev == "" {
+				continue
 			}
 
-			for _, dev := range strings.Split(oldConfig["bridge.external_interfaces"], ",") {
-				dev = strings.TrimSpace(dev)
-				if dev == "" {
-					continue
-				}
-
-				if !shared.StringInSlice(dev, devices) && shared.PathExists(fmt.Sprintf("/sys/class/net/%s", dev)) {
-					err = DetachInterface(n.name, dev)
-					if err != nil {
-						return err
-					}
+			if !shared.StringInSlice(dev, devices) && shared.PathExists(fmt.Sprintf("/sys/class/net/%s", dev)) {
+				err = DetachInterface(n.name, dev)
+				if err != nil {
+					return err
 				}
 			}
 		}
 	}
 
-	// Apply changes
-	n.config = newConfig
-	n.description = newNetwork.Description
-
-	// Update the database
-	if !notify {
-		// Notify all other nodes to update the network.
-		notifier, err := cluster.NewNotifier(n.state, n.state.Endpoints.NetworkCert(), cluster.NotifyAll)
-		if err != nil {
-			return err
-		}
-
-		err = notifier(func(client lxd.InstanceServer) error {
-			return client.UpdateNetwork(n.name, newNetwork, "")
-		})
-		if err != nil {
-			return err
-		}
-
-		// Update the database.
-		err = n.state.Cluster.UpdateNetwork(n.name, n.description, n.config)
-		if err != nil {
-			return err
-		}
+	// Apply changes to database.
+	err = n.common.update(newNetwork, notify)
+	if err != nil {
+		return err
 	}
 
-	// Restart the network
-	if !userOnly {
-		err = n.setup(oldConfig)
+	// Restart the network if needed.
+	if len(changedKeys) > 0 {
+		err = n.setup(oldNetwork.Config)
 		if err != nil {
 			return err
 		}
 	}
 
-	// Success, update the closure to mark that the changes should be kept.
-	undoChanges = false
-
+	revert.Success()
 	return nil
 }
 

From ea06a613dba726eecd0d746ca2e0416820b3775a Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 16 Jul 2020 10:57:24 +0100
Subject: [PATCH 4/5] lxd/network/driver/bridge: Updates to use contextual
 logger

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/network/driver_bridge.go | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lxd/network/driver_bridge.go b/lxd/network/driver_bridge.go
index 0600d6fe9d..5052ed645e 100644
--- a/lxd/network/driver_bridge.go
+++ b/lxd/network/driver_bridge.go
@@ -24,7 +24,6 @@ import (
 	"github.com/lxc/lxd/shared"
 	"github.com/lxc/lxd/shared/api"
 	log "github.com/lxc/lxd/shared/log15"
-	"github.com/lxc/lxd/shared/logger"
 	"github.com/lxc/lxd/shared/subprocess"
 	"github.com/lxc/lxd/shared/version"
 )
@@ -328,6 +327,8 @@ func (n *bridge) setup(oldConfig map[string]string) error {
 		return nil
 	}
 
+	n.logger.Debug("Setting up network")
+
 	// Create directory
 	if !shared.PathExists(shared.VarPath("networks", n.name)) {
 		err := os.MkdirAll(shared.VarPath("networks", n.name), 0711)
@@ -439,13 +440,13 @@ func (n *bridge) setup(oldConfig map[string]string) error {
 	if n.config["bridge.driver"] != "openvswitch" {
 		err = BridgeVLANFilterSetStatus(n.name, "1")
 		if err != nil {
-			logger.Warnf("%v", err)
+			n.logger.Warn(fmt.Sprintf("%v", err))
 		}
 
 		// Set the default PVID for new ports to 1.
 		err = BridgeVLANSetDefaultPVID(n.name, "1")
 		if err != nil {
-			logger.Warnf("%v", err)
+			n.logger.Warn(fmt.Sprintf("%v", err))
 		}
 	}
 
@@ -461,6 +462,7 @@ func (n *bridge) setup(oldConfig map[string]string) error {
 			entry = strings.TrimSpace(entry)
 			iface, err := net.InterfaceByName(entry)
 			if err != nil {
+				n.logger.Warn("Skipping attaching missing external interface", log.Ctx{"interface": entry})
 				continue
 			}
 
@@ -711,7 +713,7 @@ func (n *bridge) setup(oldConfig map[string]string) error {
 		subnetSize, _ := subnet.Mask.Size()
 
 		if subnetSize > 64 {
-			logger.Warn("IPv6 networks with a prefix larger than 64 aren't properly supported by dnsmasq", log.Ctx{"network": n.name})
+			n.logger.Warn("IPv6 networks with a prefix larger than 64 aren't properly supported by dnsmasq")
 		}
 
 		// Update the dnsmasq config
@@ -1412,7 +1414,7 @@ func (n *bridge) HandleHeartbeat(heartbeatData *cluster.APIHeartbeat) error {
 		return err
 	}
 
-	logger.Infof("Refreshing forkdns peers for %v", n.name)
+	n.logger.Info("Refreshing forkdns peers")
 
 	cert := n.state.Endpoints.NetworkCert()
 	for _, node := range heartbeatData.Members {
@@ -1446,7 +1448,7 @@ func (n *bridge) HandleHeartbeat(heartbeatData *cluster.APIHeartbeat) error {
 	curList, err := ForkdnsServersList(n.name)
 	if err != nil {
 		// Only warn here, but continue on to regenerate the servers list from cluster info.
-		logger.Warnf("Failed to load existing forkdns server list: %v", err)
+		n.logger.Warn("Failed to load existing forkdns server list", log.Ctx{"err": err})
 	}
 
 	// If current list is same as cluster list, nothing to do.
@@ -1459,7 +1461,7 @@ func (n *bridge) HandleHeartbeat(heartbeatData *cluster.APIHeartbeat) error {
 		return err
 	}
 
-	logger.Infof("Updated forkdns server list for '%s': %v", n.name, addresses)
+	n.logger.Info("Updated forkdns server list", log.Ctx{"nodes": addresses})
 	return nil
 }
 

From e8f9c948bbb35f3ddb77e75d6f333350dead64c4 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Thu, 16 Jul 2020 10:58:37 +0100
Subject: [PATCH 5/5] lxd/network/driver/common: Removes stuttering on "common"
 in validation rules function

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/network/driver_common.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lxd/network/driver_common.go b/lxd/network/driver_common.go
index 63fe54812b..9e8762ca58 100644
--- a/lxd/network/driver_common.go
+++ b/lxd/network/driver_common.go
@@ -46,8 +46,8 @@ func (n *common) init(state *state.State, id int64, name string, netType string,
 	n.description = description
 }
 
-// commonRules returns a map of config rules common to all drivers.
-func (n *common) commonRules() map[string]func(string) error {
+// validationRules returns a map of config rules common to all drivers.
+func (n *common) validationRules() map[string]func(string) error {
 	return map[string]func(string) error{}
 }
 
@@ -56,7 +56,7 @@ func (n *common) validate(config map[string]string, driverRules map[string]func(
 	checkedFields := map[string]struct{}{}
 
 	// Get rules common for all drivers.
-	rules := n.commonRules()
+	rules := n.validationRules()
 
 	// Merge driver specific rules into common rules.
 	for field, validator := range driverRules {


More information about the lxc-devel mailing list