The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/7781
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) ===
- Aims to prevent concurrent OVN network creation from allocating the same IP on the parent network by using transactions.
- Relies on the sqlite snapshot behaviour in WAL mode to detect if a the IP info of other networks has changed when writing the allocated IP back to the database.
- Adds `GetNonPendingNetworks` and `UpdateNetwork` to `ClusterTx` type so that all network related queries can be done inside a transaction.
- Updates `setupParentPortBridge` to start a transaction and pass it to its associated functions.
From 3d8cf6fdae11c5c9b3fda5c7b553a4e0aea4a0df Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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.parr...@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
}
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
