[lxc-devel] [lxd/master] Fix handling of shared network interfaces
stgraber on Github
lxc-bot at linuxcontainers.org
Thu Nov 7 20:06:59 UTC 2019
A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 301 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20191107/82107ab0/attachment.bin>
-------------- next part --------------
From 1212321a20284c6bce1529dc2fd03191a08c1957 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at 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 <stgraber at 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?= <stgraber at 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 <stgraber at 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
More information about the lxc-devel
mailing list