The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/7660
This e-mail was sent by the LXC bot, direct replies will not reach the author
unless they happen to be subscribed to this list.
=== Description (from pull-request) ===
- Moving common functionality out of bridge driver and into common driver.
- Adds contextual logging support.
From 5f847ad62238eb7b57c7e491f520c42d1347d719 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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 {
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
