[lxc-devel] [lxd/master] Network: OVN use DB transactions when allocating external IP on parent network

tomponline on Github lxc-bot at linuxcontainers.org
Mon Aug 17 13:51:26 UTC 2020


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 829 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20200817/8a2254ab/attachment.bin>
-------------- next part --------------
From 3d8cf6fdae11c5c9b3fda5c7b553a4e0aea4a0df Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Mon, 17 Aug 2020 14:43:37 +0100
Subject: [PATCH 1/5] lxd/db/networks: Separates network type and status
 conversion into separate functions

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/db/networks.go | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/lxd/db/networks.go b/lxd/db/networks.go
index 88a3e175fb..9e60c995c8 100644
--- a/lxd/db/networks.go
+++ b/lxd/db/networks.go
@@ -356,6 +356,20 @@ func (c *Cluster) getNetwork(name string, onlyCreated bool) (int64, *api.Network
 	network.Description = description.String
 	network.Config = config
 
+	// Populate Status and Type fields by converting from DB values.
+	networkFillStatus(&network, state)
+	networkFillType(&network, netType)
+
+	nodes, err := c.networkNodes(id)
+	if err != nil {
+		return -1, nil, err
+	}
+	network.Locations = nodes
+
+	return id, &network, nil
+}
+
+func networkFillStatus(network *api.Network, state int) {
 	switch state {
 	case networkPending:
 		network.Status = api.NetworkStatusPending
@@ -366,7 +380,9 @@ func (c *Cluster) getNetwork(name string, onlyCreated bool) (int64, *api.Network
 	default:
 		network.Status = api.NetworkStatusUnknown
 	}
+}
 
+func networkFillType(network *api.Network, netType NetworkType) {
 	switch netType {
 	case NetworkTypeBridge:
 		network.Type = "bridge"
@@ -379,14 +395,6 @@ func (c *Cluster) getNetwork(name string, onlyCreated bool) (int64, *api.Network
 	default:
 		network.Type = "" // Unknown
 	}
-
-	nodes, err := c.networkNodes(id)
-	if err != nil {
-		return -1, nil, err
-	}
-	network.Locations = nodes
-
-	return id, &network, nil
 }
 
 // Return the names of the nodes the given network is defined on.

From 984b076140a02a12dad74a520a3c69a4ac040082 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Mon, 17 Aug 2020 14:44:53 +0100
Subject: [PATCH 2/5] lxd/db/networks: Adds ClusterTx.GetNonPendingNetworks
 function

Returns non-pending networks and their configs using a transaction connection.

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

diff --git a/lxd/db/networks.go b/lxd/db/networks.go
index 9e60c995c8..c1bb3f82af 100644
--- a/lxd/db/networks.go
+++ b/lxd/db/networks.go
@@ -66,6 +66,60 @@ func (c *ClusterTx) GetNonPendingNetworkIDs() (map[string]int64, error) {
 	return ids, nil
 }
 
+// GetNonPendingNetworks returns a map of api.Network associated to network ID.
+//
+// Pending networks are skipped.
+func (c *ClusterTx) GetNonPendingNetworks() (map[int64]api.Network, error) {
+	stmt, err := c.tx.Prepare("SELECT id, name, description, type, state FROM networks WHERE state != ?")
+	if err != nil {
+		return nil, err
+	}
+	defer stmt.Close()
+
+	rows, err := stmt.Query(networkPending)
+	if err != nil {
+		return nil, err
+	}
+	defer rows.Close()
+
+	networks := make(map[int64]api.Network)
+
+	for i := 0; rows.Next(); i++ {
+		var networkID int64
+		var networkType NetworkType
+		var networkState int
+		var network api.Network
+
+		err := rows.Scan(&networkID, &network.Name, &network.Description, &networkType, &networkState)
+		if err != nil {
+			return nil, err
+		}
+
+		// Populate Status and Type fields by converting from DB values.
+		networkFillStatus(&network, networkState)
+		networkFillType(&network, networkType)
+
+		networks[networkID] = network
+	}
+	err = rows.Err()
+	if err != nil {
+		return nil, err
+	}
+
+	// Populate config.
+	for networkID, network := range networks {
+		networkConfig, err := query.SelectConfig(c.tx, "networks_config", "network_id=? AND (node_id=? OR node_id IS NULL)", networkID, c.nodeID)
+		if err != nil {
+			return nil, err
+		}
+
+		network.Config = networkConfig
+		networks[networkID] = network
+	}
+
+	return networks, nil
+}
+
 // GetNetworkID returns the ID of the network with the given name.
 func (c *ClusterTx) GetNetworkID(name string) (int64, error) {
 	stmt := "SELECT id FROM networks WHERE name=?"

From 304a344deff1eeeac1f7e150e7fec45616b9e732 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Mon, 17 Aug 2020 14:45:53 +0100
Subject: [PATCH 3/5] lxd/db/networks: Adds ClusterTx.UpdateNetwork function

And updates existing UpdateNetwork function to use it.

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/db/networks.go | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/lxd/db/networks.go b/lxd/db/networks.go
index c1bb3f82af..2755eb35d9 100644
--- a/lxd/db/networks.go
+++ b/lxd/db/networks.go
@@ -312,6 +312,26 @@ func (c *ClusterTx) networkState(name string, state int) error {
 	return nil
 }
 
+// UpdateNetwork updates the network with the given ID.
+func (c *ClusterTx) UpdateNetwork(id int64, description string, config map[string]string) error {
+	err := updateNetworkDescription(c.tx, id, description)
+	if err != nil {
+		return err
+	}
+
+	err = clearNetworkConfig(c.tx, id, c.nodeID)
+	if err != nil {
+		return err
+	}
+
+	err = networkConfigAdd(c.tx, id, c.nodeID, config)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
 // GetNetworks returns the names of existing networks.
 func (c *Cluster) GetNetworks() ([]string, error) {
 	return c.networks("")
@@ -603,17 +623,7 @@ func (c *Cluster) UpdateNetwork(name, description string, config map[string]stri
 	}
 
 	err = c.Transaction(func(tx *ClusterTx) error {
-		err = updateNetworkDescription(tx.tx, id, description)
-		if err != nil {
-			return err
-		}
-
-		err = clearNetworkConfig(tx.tx, id, c.nodeID)
-		if err != nil {
-			return err
-		}
-
-		err = networkConfigAdd(tx.tx, id, c.nodeID, config)
+		err = tx.UpdateNetwork(id, description, config)
 		if err != nil {
 			return err
 		}

From 05af94244e81f3c8b2312515f53e819529f078a6 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Mon, 17 Aug 2020 14:46:54 +0100
Subject: [PATCH 4/5] lxd/network/driver/ovn: Use DB transactions to safely
 allocate OVN external IPs on parent network

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

diff --git a/lxd/network/driver_ovn.go b/lxd/network/driver_ovn.go
index 9d0cca9a26..fc49a5ce5a 100644
--- a/lxd/network/driver_ovn.go
+++ b/lxd/network/driver_ovn.go
@@ -15,6 +15,7 @@ import (
 	"github.com/pkg/errors"
 
 	"github.com/lxc/lxd/lxd/cluster"
+	"github.com/lxc/lxd/lxd/db"
 	"github.com/lxc/lxd/lxd/dnsmasq"
 	"github.com/lxc/lxd/lxd/locking"
 	"github.com/lxc/lxd/lxd/network/openvswitch"
@@ -288,58 +289,74 @@ func (n *ovn) setupParentPortBridge(parentNet Network, routerMAC net.HardwareAdd
 		v.routerExtGwIPv6 = parentIPv6
 	}
 
-	// Retrieve and parse existing allocated IPs for this network on the parent network.
+	// Parse existing allocated IPs for this network on the parent network (if not set yet, will be nil).
 	routerExtPortIPv4 := net.ParseIP(n.config[ovnVolatileParentIPv4])
 	routerExtPortIPv6 := net.ParseIP(n.config[ovnVolatileParentIPv6])
 
 	// Decide whether we need to allocate new IP(s) and go to the expense of retrieving all allocated IPs.
 	if (parentIPv4Net != nil && routerExtPortIPv4 == nil) || (parentIPv6Net != nil && routerExtPortIPv6 == nil) {
-		allAllocatedIPv4, allAllocatedIPv6, err := n.parentAllAllocatedIPs(parentNet.Name())
-		if err != nil {
-			return nil, errors.Wrapf(err, "Failed to get all allocated IPs for parent")
-		}
-
-		if parentIPv4Net != nil && routerExtPortIPv4 == nil {
-			ipRanges, err := parseIPRanges(parentNetConf["ipv4.ovn.ranges"], parentNet.DHCPv4Subnet())
-			if err != nil {
-				return nil, errors.Wrapf(err, "Failed to parse parent IPv4 OVN ranges")
-			}
-
-			routerExtPortIPv4, err = n.parentAllocateIP(ipRanges, allAllocatedIPv4)
+		err := n.state.Cluster.Transaction(func(tx *db.ClusterTx) error {
+			allAllocatedIPv4, allAllocatedIPv6, err := n.parentAllAllocatedIPs(tx, parentNet.Name())
 			if err != nil {
-				return nil, errors.Wrapf(err, "Failed to allocate parent IPv4 address")
+				return errors.Wrapf(err, "Failed to get all allocated IPs for parent")
 			}
 
-			n.config[ovnVolatileParentIPv4] = routerExtPortIPv4.String()
-		}
+			if parentIPv4Net != nil && routerExtPortIPv4 == nil {
+				if parentNetConf["ipv4.ovn.ranges"] == "" {
+					return fmt.Errorf(`Missing required "ipv4.ovn.ranges" config key on parent network`)
+				}
 
-		if parentIPv6Net != nil && routerExtPortIPv6 == nil {
-			// If IPv6 OVN ranges are specified by the parent, allocate from them.
-			if parentNetConf["ipv6.ovn.ranges"] != "" {
-				ipRanges, err := parseIPRanges(parentNetConf["ipv6.ovn.ranges"], parentNet.DHCPv6Subnet())
+				ipRanges, err := parseIPRanges(parentNetConf["ipv4.ovn.ranges"], parentNet.DHCPv4Subnet())
 				if err != nil {
-					return nil, errors.Wrapf(err, "Failed to parse parent IPv6 OVN ranges")
+					return errors.Wrapf(err, "Failed to parse parent IPv4 OVN ranges")
 				}
 
-				routerExtPortIPv6, err = n.parentAllocateIP(ipRanges, allAllocatedIPv6)
+				routerExtPortIPv4, err = n.parentAllocateIP(ipRanges, allAllocatedIPv4)
 				if err != nil {
-					return nil, errors.Wrapf(err, "Failed to allocate parent IPv6 address")
+					return errors.Wrapf(err, "Failed to allocate parent IPv4 address")
 				}
 
-			} else {
-				// Otherwise use EUI64 derived from MAC address.
-				routerExtPortIPv6, err = eui64.ParseMAC(parentIPv6Net.IP, routerMAC)
-				if err != nil {
-					return nil, err
+				n.config[ovnVolatileParentIPv4] = routerExtPortIPv4.String()
+			}
+
+			if parentIPv6Net != nil && routerExtPortIPv6 == nil {
+				// If IPv6 OVN ranges are specified by the parent, allocate from them.
+				if parentNetConf["ipv6.ovn.ranges"] != "" {
+					ipRanges, err := parseIPRanges(parentNetConf["ipv6.ovn.ranges"], parentNet.DHCPv6Subnet())
+					if err != nil {
+						return errors.Wrapf(err, "Failed to parse parent IPv6 OVN ranges")
+					}
+
+					routerExtPortIPv6, err = n.parentAllocateIP(ipRanges, allAllocatedIPv6)
+					if err != nil {
+						return errors.Wrapf(err, "Failed to allocate parent IPv6 address")
+					}
+
+				} else {
+					// Otherwise use EUI64 derived from MAC address.
+					routerExtPortIPv6, err = eui64.ParseMAC(parentIPv6Net.IP, routerMAC)
+					if err != nil {
+						return err
+					}
 				}
+
+				n.config[ovnVolatileParentIPv6] = routerExtPortIPv6.String()
 			}
 
-			n.config[ovnVolatileParentIPv6] = routerExtPortIPv6.String()
-		}
+			networkID, err := tx.GetNetworkID(n.name)
+			if err != nil {
+				return errors.Wrapf(err, "Failed to get network ID for network %q", n.name)
+			}
 
-		err = n.state.Cluster.UpdateNetwork(n.name, n.description, n.config)
+			err = tx.UpdateNetwork(networkID, n.description, n.config)
+			if err != nil {
+				return errors.Wrapf(err, "Failed saving allocated parent network IPs")
+			}
+
+			return nil
+		})
 		if err != nil {
-			return nil, errors.Wrapf(err, "Failed saving allocated parent network IPs")
+			return nil, err
 		}
 	}
 
@@ -364,9 +381,9 @@ func (n *ovn) setupParentPortBridge(parentNet Network, routerMAC net.HardwareAdd
 }
 
 // parentAllAllocatedIPs gets a list of all IPv4 and IPv6 addresses allocated to OVN networks connected to parent.
-func (n *ovn) parentAllAllocatedIPs(parentNetName string) ([]net.IP, []net.IP, error) {
-	// Get a list of managed networks.
-	networks, err := n.state.Cluster.GetNonPendingNetworks()
+func (n *ovn) parentAllAllocatedIPs(tx *db.ClusterTx, parentNetName string) ([]net.IP, []net.IP, error) {
+	// Get all managed networks.
+	networks, err := tx.GetNonPendingNetworks()
 	if err != nil {
 		return nil, nil, err
 	}
@@ -374,12 +391,7 @@ func (n *ovn) parentAllAllocatedIPs(parentNetName string) ([]net.IP, []net.IP, e
 	v4IPs := make([]net.IP, 0)
 	v6IPs := make([]net.IP, 0)
 
-	for _, name := range networks {
-		_, netInfo, err := n.state.Cluster.GetNetworkInAnyState(name)
-		if err != nil {
-			return nil, nil, err
-		}
-
+	for _, netInfo := range networks {
 		if netInfo.Type != "ovn" || netInfo.Config["parent"] != parentNetName {
 			continue
 		}

From 245443955eabede3bf8a5c91e20c4514363bf116 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Mon, 17 Aug 2020 14:47:23 +0100
Subject: [PATCH 5/5] lxd/network/driver/ovn: Include last IP in OVN range for
 allocatable IPs

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/network/driver_ovn.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lxd/network/driver_ovn.go b/lxd/network/driver_ovn.go
index fc49a5ce5a..cabe01f2f8 100644
--- a/lxd/network/driver_ovn.go
+++ b/lxd/network/driver_ovn.go
@@ -436,7 +436,7 @@ func (n *ovn) parentAllocateIP(ipRanges []*shared.IPRange, allAllocated []net.IP
 
 		// Iterate through IPs in range, return the first unallocated one found.
 		for {
-			if startBig.Cmp(endBig) >= 0 {
+			if startBig.Cmp(endBig) > 0 {
 				break
 			}
 


More information about the lxc-devel mailing list