The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/6420
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) ===
From 1212321a20284c6bce1529dc2fd03191a08c1957 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgra...@ubuntu.com>
Date: Thu, 7 Nov 2019 11:03:39 -0500
Subject: [PATCH 1/2] lxd/device/nic: Fix race in vlan creation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Stéphane Graber <stgra...@ubuntu.com>
---
lxd/device/device_utils_network.go | 4 ++++
lxd/device/nic_ipvlan.go | 4 ++++
lxd/device/nic_macvlan.go | 4 ++++
lxd/device/nic_physical.go | 4 ++++
4 files changed, 16 insertions(+)
diff --git a/lxd/device/device_utils_network.go
b/lxd/device/device_utils_network.go
index e2eeed970b..6462d4c493 100644
--- a/lxd/device/device_utils_network.go
+++ b/lxd/device/device_utils_network.go
@@ -11,6 +11,7 @@ import (
"regexp"
"strconv"
"strings"
+ "sync"
deviceConfig "github.com/lxc/lxd/lxd/device/config"
"github.com/lxc/lxd/shared"
@@ -18,6 +19,9 @@ import (
"github.com/lxc/lxd/shared/units"
)
+// Instances can be started in parallel, so lock the creation of VLANs.
+var networkCreateSharedDeviceLock sync.Mutex
+
// NetworkSysctlGet retrieves the value of a sysctl file in /proc/sys/net.
func NetworkSysctlGet(path string) (string, error) {
// Read the current content
diff --git a/lxd/device/nic_ipvlan.go b/lxd/device/nic_ipvlan.go
index 226d8aa7d3..82f698bfe7 100644
--- a/lxd/device/nic_ipvlan.go
+++ b/lxd/device/nic_ipvlan.go
@@ -113,6 +113,10 @@ func (d *nicIPVLAN) Start() (*RunConfig, error) {
return nil, err
}
+ // Lock to avoid issues with containers starting in parallel.
+ networkCreateSharedDeviceLock.Lock()
+ defer networkCreateSharedDeviceLock.Unlock()
+
saveData := make(map[string]string)
// Decide which parent we should use based on VLAN setting.
diff --git a/lxd/device/nic_macvlan.go b/lxd/device/nic_macvlan.go
index e5f92565aa..0b8fe8077a 100644
--- a/lxd/device/nic_macvlan.go
+++ b/lxd/device/nic_macvlan.go
@@ -47,6 +47,10 @@ func (d *nicMACVLAN) Start() (*RunConfig, error) {
return nil, err
}
+ // Lock to avoid issues with containers starting in parallel.
+ networkCreateSharedDeviceLock.Lock()
+ defer networkCreateSharedDeviceLock.Unlock()
+
saveData := make(map[string]string)
// Decide which parent we should use based on VLAN setting.
diff --git a/lxd/device/nic_physical.go b/lxd/device/nic_physical.go
index 1a935518cc..7989049ea5 100644
--- a/lxd/device/nic_physical.go
+++ b/lxd/device/nic_physical.go
@@ -54,6 +54,10 @@ func (d *nicPhysical) Start() (*RunConfig, error) {
return nil, err
}
+ // Lock to avoid issues with containers starting in parallel.
+ networkCreateSharedDeviceLock.Lock()
+ defer networkCreateSharedDeviceLock.Unlock()
+
saveData := make(map[string]string)
// Record the host_name device used for restoration later.
From f59a5d927890357fec9feaf5a6345be63ac10d62 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgra...@ubuntu.com>
Date: Thu, 7 Nov 2019 15:05:13 -0500
Subject: [PATCH 2/2] lxd/device/nic: Fix handling of shared vlans
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Closes #6416
Signed-off-by: Stéphane Graber <stgra...@ubuntu.com>
---
lxd/device/device_utils_network.go | 62 ++++++++++++++++++++++++++----
lxd/device/nic_ipvlan.go | 8 ++--
lxd/device/nic_macvlan.go | 12 +++---
lxd/device/nic_physical.go | 8 ++--
4 files changed, 70 insertions(+), 20 deletions(-)
diff --git a/lxd/device/device_utils_network.go
b/lxd/device/device_utils_network.go
index 6462d4c493..67cd1fbf42 100644
--- a/lxd/device/device_utils_network.go
+++ b/lxd/device/device_utils_network.go
@@ -14,6 +14,7 @@ import (
"sync"
deviceConfig "github.com/lxc/lxd/lxd/device/config"
+ "github.com/lxc/lxd/lxd/state"
"github.com/lxc/lxd/shared"
"github.com/lxc/lxd/shared/logger"
"github.com/lxc/lxd/shared/units"
@@ -155,31 +156,78 @@ func NetworkRemoveInterface(nic string) error {
return err
}
+// NetworkRemoveInterface removes a network interface by name but only if no
other instance is using it.
+func NetworkRemoveInterfaceIfNeeded(state *state.State, nic string, current
Instance, parent string, vlanID string) error {
+ // Check if it's used by another instance.
+ instances, err := InstanceLoadNodeAll(state)
+ if err != nil {
+ return err
+ }
+
+ for _, inst := range instances {
+ if inst.Name() == current.Name() && inst.Project() ==
current.Project() {
+ continue
+ }
+
+ for devName, dev := range inst.ExpandedDevices() {
+ if dev["type"] != "nic" || dev["vlan"] != vlanID ||
dev["parent"] != parent {
+ continue
+ }
+
+ // Check if another running instance created the
device, if so, don't touch it.
+ if
shared.IsTrue(inst.ExpandedConfig()[fmt.Sprintf("volatile.%s.last_state.created",
devName)]) {
+ return nil
+ }
+ }
+ }
+
+ return NetworkRemoveInterface(nic)
+}
+
// NetworkCreateVlanDeviceIfNeeded creates a VLAN device if doesn't already
exist.
-func NetworkCreateVlanDeviceIfNeeded(parent string, vlanDevice string, vlanID
string) (bool, error) {
+func NetworkCreateVlanDeviceIfNeeded(state *state.State, parent string,
vlanDevice string, vlanID string) (string, error) {
if vlanID != "" {
if !shared.PathExists(fmt.Sprintf("/sys/class/net/%s",
vlanDevice)) {
// Bring the parent interface up so we can add a vlan
to it.
_, err := shared.RunCommand("ip", "link", "set", "dev",
parent, "up")
if err != nil {
- return false, fmt.Errorf("Failed to bring up
parent %s: %v", parent, err)
+ return "", fmt.Errorf("Failed to bring up
parent %s: %v", parent, err)
}
// Add VLAN interface on top of parent.
_, err = shared.RunCommand("ip", "link", "add", "link",
parent, "name", vlanDevice, "up", "type", "vlan", "id", vlanID)
if err != nil {
- return false, err
+ return "", err
}
- // Attempt to disable IPv6 router advertisement
acceptance
+ // Attempt to disable IPv6 router advertisement
acceptance.
NetworkSysctlSet(fmt.Sprintf("ipv6/conf/%s/accept_ra",
vlanDevice), "0")
- // We created a new vlan interface, return true
- return true, nil
+ // We created a new vlan interface, return true.
+ return "created", nil
+ } else {
+ // Check if it was created for another running instance.
+ instances, err := InstanceLoadNodeAll(state)
+ if err != nil {
+ return "", err
+ }
+
+ for _, inst := range instances {
+ for devName, dev := range
inst.ExpandedDevices() {
+ if dev["type"] != "nic" || dev["vlan"]
!= vlanID || dev["parent"] != parent {
+ continue
+ }
+
+ // Check if another running instance
created the device, if so, mark it as created.
+ if
shared.IsTrue(inst.ExpandedConfig()[fmt.Sprintf("volatile.%s.last_state.created",
devName)]) {
+ return "reused", nil
+ }
+ }
+ }
}
}
- return false, nil
+ return "existing", nil
}
// networkSnapshotPhysicalNic records properties of the NIC to volatile so
they can be restored later.
diff --git a/lxd/device/nic_ipvlan.go b/lxd/device/nic_ipvlan.go
index 82f698bfe7..0308e90c5e 100644
--- a/lxd/device/nic_ipvlan.go
+++ b/lxd/device/nic_ipvlan.go
@@ -122,16 +122,16 @@ func (d *nicIPVLAN) Start() (*RunConfig, error) {
// Decide which parent we should use based on VLAN setting.
parentName := NetworkGetHostDevice(d.config["parent"], d.config["vlan"])
- createdDev, err := NetworkCreateVlanDeviceIfNeeded(d.config["parent"],
parentName, d.config["vlan"])
+ statusDev, err := NetworkCreateVlanDeviceIfNeeded(d.state,
d.config["parent"], parentName, d.config["vlan"])
if err != nil {
return nil, err
}
// Record whether we created this device or not so it can be removed on
stop.
- saveData["last_state.created"] = fmt.Sprintf("%t", createdDev)
+ saveData["last_state.created"] = fmt.Sprintf("%t", statusDev !=
"existing")
// If we created a VLAN interface, we need to setup the sysctls on that
interface.
- if createdDev {
+ if statusDev == "created" {
err := d.setupParentSysctls(parentName)
if err != nil {
return nil, err
@@ -231,7 +231,7 @@ func (d *nicIPVLAN) postStop() error {
// This will delete the parent interface if we created it for VLAN
parent.
if shared.IsTrue(v["last_state.created"]) {
parentName := NetworkGetHostDevice(d.config["parent"],
d.config["vlan"])
- err := NetworkRemoveInterface(parentName)
+ err := NetworkRemoveInterfaceIfNeeded(d.state, parentName,
d.instance, d.config["parent"], d.config["vlan"])
if err != nil {
return err
}
diff --git a/lxd/device/nic_macvlan.go b/lxd/device/nic_macvlan.go
index 0b8fe8077a..f02ee4a38f 100644
--- a/lxd/device/nic_macvlan.go
+++ b/lxd/device/nic_macvlan.go
@@ -60,13 +60,13 @@ func (d *nicMACVLAN) Start() (*RunConfig, error) {
saveData["host_name"] = NetworkRandomDevName("mac")
// Create VLAN parent device if needed.
- createdDev, err := NetworkCreateVlanDeviceIfNeeded(d.config["parent"],
parentName, d.config["vlan"])
+ statusDev, err := NetworkCreateVlanDeviceIfNeeded(d.state,
d.config["parent"], parentName, d.config["vlan"])
if err != nil {
return nil, err
}
// Record whether we created the parent device or not so it can be
removed on stop.
- saveData["last_state.created"] = fmt.Sprintf("%t", createdDev)
+ saveData["last_state.created"] = fmt.Sprintf("%t", statusDev !=
"existing")
// Create MACVLAN interface.
_, err = shared.RunCommand("ip", "link", "add", "dev",
saveData["host_name"], "link", parentName, "type", "macvlan", "mode", "bridge")
@@ -78,9 +78,10 @@ func (d *nicMACVLAN) Start() (*RunConfig, error) {
if d.config["hwaddr"] != "" {
_, err := shared.RunCommand("ip", "link", "set", "dev",
saveData["host_name"], "address", d.config["hwaddr"])
if err != nil {
- if createdDev {
+ if statusDev == "created" {
NetworkRemoveInterface(saveData["host_name"])
}
+
return nil, fmt.Errorf("Failed to set the MAC address:
%s", err)
}
}
@@ -89,9 +90,10 @@ func (d *nicMACVLAN) Start() (*RunConfig, error) {
if d.config["mtu"] != "" {
_, err := shared.RunCommand("ip", "link", "set", "dev",
saveData["host_name"], "mtu", d.config["mtu"])
if err != nil {
- if createdDev {
+ if statusDev == "created" {
NetworkRemoveInterface(saveData["host_name"])
}
+
return nil, fmt.Errorf("Failed to set the MTU: %s", err)
}
}
@@ -148,7 +150,7 @@ func (d *nicMACVLAN) postStop() error {
// This will delete the parent interface if we created it for VLAN
parent.
if shared.IsTrue(v["last_state.created"]) {
parentName := NetworkGetHostDevice(d.config["parent"],
d.config["vlan"])
- err := NetworkRemoveInterface(parentName)
+ err := NetworkRemoveInterfaceIfNeeded(d.state, parentName,
d.instance, d.config["parent"], d.config["vlan"])
if err != nil {
errs = append(errs, err)
}
diff --git a/lxd/device/nic_physical.go b/lxd/device/nic_physical.go
index 7989049ea5..13c0653dfc 100644
--- a/lxd/device/nic_physical.go
+++ b/lxd/device/nic_physical.go
@@ -62,24 +62,24 @@ func (d *nicPhysical) Start() (*RunConfig, error) {
// Record the host_name device used for restoration later.
saveData["host_name"] = NetworkGetHostDevice(d.config["parent"],
d.config["vlan"])
- createdDev, err := NetworkCreateVlanDeviceIfNeeded(d.config["parent"],
saveData["host_name"], d.config["vlan"])
+ statusDev, err := NetworkCreateVlanDeviceIfNeeded(d.state,
d.config["parent"], saveData["host_name"], d.config["vlan"])
if err != nil {
return nil, err
}
// Record whether we created this device or not so it can be removed on
stop.
- saveData["last_state.created"] = fmt.Sprintf("%t", createdDev)
+ saveData["last_state.created"] = fmt.Sprintf("%t", statusDev !=
"existing")
// If we return from this function with an error, ensure we clean up
created device.
defer func() {
- if err != nil && createdDev {
+ if err != nil && statusDev == "created" {
NetworkRemoveInterface(saveData["host_name"])
}
}()
// If we didn't create the device we should track various properties so
we can
// restore them when the instance is stopped or the device is detached.
- if createdDev == false {
+ if statusDev == "existing" {
err = networkSnapshotPhysicalNic(saveData["host_name"],
saveData)
if err != nil {
return nil, err
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
