The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/6043

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) ===
This modifies the `device` interface to allow the `Stop()` function to return a `RunConfig` struct, similar to the `Start()` function. 

This allows the `Stop()` function to return a partially populated `RunConfig` struct to communicate with LXD which devices it should detach from the container and how to do it (such as what interface name to use when detaching a NIC back onto the host).

It also allows a slice of functions to be returned in the `PostHooks` variable, which LXD will then execute after the device has been detached.

This also changes when the `Stop()` function is called, it is now executed *before* any devices are detached from a running container, however the `PostHooks` are called after the detaching has been completed.

It also has a nice side-effect of mirroring the behaviour of the `Start()` function.

The reason for this change is as follows:

1. Allows NIC devices to indicate to LXD that it does not need to detach an interface (such as bridge and p2p veth devices), as the PostHooks will delete the parent device which will delete the veth peer.
2. Allows NIC devices to specify the detach hostname directly, without having to use volatile, which pushes more detach logic down into the device package.
3. Allows future unix devices to figure out which paths need to be detached inside the device package, without having to put device-type specific logic into LXD. 

This PR also adds some more tests to check for volatile key clean up.


From 4d08285c9e54704756a4005534c0398c145f4e0d Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:03:11 +0100
Subject: [PATCH 01/11] container/lxc: Restructures deviceStop to support post
 stop hooks

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 lxd/container_lxc.go | 147 +++++++++++++++++++++++--------------------
 1 file changed, 80 insertions(+), 67 deletions(-)

diff --git a/lxd/container_lxc.go b/lxd/container_lxc.go
index 26e60b9fbc..8e1723e5e2 100644
--- a/lxd/container_lxc.go
+++ b/lxd/container_lxc.go
@@ -1821,8 +1821,8 @@ func (c *containerLXC) initLXC(config bool) error {
        return nil
 }
 
-// runPostStartHooks executes the callback functions after container has 
started.
-func (c *containerLXC) runPostStartHooks(hooks []func() error) error {
+// runHooks executes the callback functions returned from a function.
+func (c *containerLXC) runHooks(hooks []func() error) error {
        // Run any post start hooks.
        if len(hooks) > 0 {
                for _, hook := range hooks {
@@ -1893,13 +1893,13 @@ func (c *containerLXC) deviceStart(deviceName string, 
rawConfig map[string]strin
        if isRunning && runConfig != nil {
                // If network interface settings returned, then attach NIC to 
container.
                if len(runConfig.NetworkInterface) > 0 {
-                       err = c.deviceAttachNIC(configCopy, runConfig)
+                       err = c.deviceAttachNIC(configCopy, 
runConfig.NetworkInterface)
                        if err != nil {
                                return nil, err
                        }
                }
 
-               err = c.runPostStartHooks(runConfig.PostStartHooks)
+               err = c.runHooks(runConfig.PostHooks)
                if err != nil {
                        return nil, err
                }
@@ -1909,9 +1909,9 @@ func (c *containerLXC) deviceStart(deviceName string, 
rawConfig map[string]strin
 }
 
 // deviceAttachNIC live attaches a NIC device to a container.
-func (c *containerLXC) deviceAttachNIC(configCopy map[string]string, runConfig 
*device.RunConfig) error {
+func (c *containerLXC) deviceAttachNIC(configCopy map[string]string, netIF 
[]device.RunConfigItem) error {
        devName := ""
-       for _, dev := range runConfig.NetworkInterface {
+       for _, dev := range netIF {
                if dev.Key == "link" {
                        devName = dev.Value
                        break
@@ -1959,76 +1959,89 @@ func (c *containerLXC) deviceStop(deviceName string, 
rawConfig map[string]string
                return err
        }
 
+       // An empty netns path means we haven't been called from the LXC stop 
hook, so are running.
        if canHotPlug, _ := d.CanHotPlug(); stopHookNetnsPath == "" && 
!canHotPlug {
                return fmt.Errorf("Device cannot be stopped when container is 
running")
        }
 
-       // Ensure nic and infiniband devices are detached.
-       if shared.StringInSlice(rawConfig["type"], []string{"nic", 
"infiniband"}) {
-               hostName := c.localConfig[fmt.Sprintf("volatile.%s.host_name", 
deviceName)]
-               hostNameExists := hostName != "" && 
shared.PathExists(fmt.Sprintf("/sys/class/net/%s", hostName))
-
-               // If container is running, perform live detach of interface 
back to host.
-               if stopHookNetnsPath == "" {
-                       // For some reason, having network config confuses 
detach, so get our own go-lxc struct.
-                       cname := project.Prefix(c.Project(), c.Name())
-                       cc, err := lxc.NewContainer(cname, c.state.OS.LxcPath)
+       runConfig, err := d.Stop()
+       if err != nil {
+               return err
+       }
+
+       if runConfig != nil {
+               // If network interface settings returned, then detach NIC from 
container.
+               if len(runConfig.NetworkInterface) > 0 {
+                       err = c.deviceDetachNIC(configCopy, 
runConfig.NetworkInterface, stopHookNetnsPath)
                        if err != nil {
                                return err
                        }
-                       defer cc.Release()
+               }
 
-                       // Get interfaces inside container.
-                       ifaces, err := cc.Interfaces()
-                       if err != nil {
-                               return fmt.Errorf("Failed to list network 
interfaces: %v", err)
-                       }
+               err = c.runHooks(runConfig.PostHooks)
+               if err != nil {
+                       return err
+               }
+       }
 
-                       // Remove the interface from the container if it exists 
inside container.
-                       if shared.StringInSlice(configCopy["name"], ifaces) {
-                               detachHostName := hostName
+       return nil
+}
 
-                               // If the host_name is empty or already exists, 
we need to detach to a
-                               // random device name, so generate one here.
-                               if hostName == "" || hostNameExists {
-                                       detachHostName = 
device.NetworkRandomDevName("lxd")
-                               }
+// deviceDetachNIC detaches a NIC device from a container.
+func (c *containerLXC) deviceDetachNIC(configCopy map[string]string, netIF 
[]device.RunConfigItem, stopHookNetnsPath string) error {
+       // Get requested device name to detach interface back to on the host.
+       devName := ""
+       for _, dev := range netIF {
+               if dev.Key == "link" {
+                       devName = dev.Value
+                       break
+               }
+       }
 
-                               err = 
cc.DetachInterfaceRename(configCopy["name"], detachHostName)
-                               if err != nil {
-                                       return errors.Wrapf(err, "Failed to 
detach interface: %s to %s", configCopy["name"], detachHostName)
-                               }
+       if devName == "" {
+               return fmt.Errorf("Device didn't provide a link property to 
use")
+       }
 
-                               // If we have detached the device to a random 
host_name it is our
-                               // responsibility to delete the device as there 
is no other record of this.
-                               if hostName == "" || hostNameExists {
-                                       // Attempt to remove device, but don't 
return on failure as Stop()
-                                       // still needs to be called.
-                                       err := 
device.NetworkRemoveInterface(detachHostName)
-                                       if err != nil {
-                                               logger.Errorf("Error removing 
interface: %s: %v", detachHostName, err)
-                                       }
-                               }
-                       }
-               } else {
-                       // Currently liblxc does not move devices back to the 
host on stop that were added
-                       // after the the container was started. For this reason 
we utilise the lxc.hook.stop
-                       // hook so that we can capture the netns path, enter 
the namespace and move the nics
-                       // back to the host and rename them if liblxc hasn't 
already done it.
-                       // We can only move back devices that have an expected 
host_name record and where
-                       // that device doesn't already exist on the host.
-                       if hostName != "" && !hostNameExists {
-                               err := 
c.detachInterfaceRename(stopHookNetnsPath, configCopy["name"], hostName)
-                               if err != nil {
-                                       return errors.Wrapf(err, "Failed to 
detach interface: %s to %s", configCopy["name"], hostName)
-                               }
-                       }
+       // If container is running, perform live detach of interface back to 
host.
+       if stopHookNetnsPath == "" {
+               // For some reason, having network config confuses detach, so 
get our own go-lxc struct.
+               cname := project.Prefix(c.Project(), c.Name())
+               cc, err := lxc.NewContainer(cname, c.state.OS.LxcPath)
+               if err != nil {
+                       return err
                }
-       }
+               defer cc.Release()
 
-       err = d.Stop()
-       if err != nil {
-               return err
+               // Get interfaces inside container.
+               ifaces, err := cc.Interfaces()
+               if err != nil {
+                       return fmt.Errorf("Failed to list network interfaces: 
%v", err)
+               }
+
+               // If interface doesn't exist inside container, cannot proceed.
+               if !shared.StringInSlice(configCopy["name"], ifaces) {
+                       return nil
+               }
+
+               err = cc.DetachInterfaceRename(configCopy["name"], devName)
+               if err != nil {
+                       return errors.Wrapf(err, "Failed to detach interface: 
%s to %s", configCopy["name"], devName)
+               }
+       } else {
+               // Currently liblxc does not move devices back to the host on 
stop that were added
+               // after the the container was started. For this reason we 
utilise the lxc.hook.stop
+               // hook so that we can capture the netns path, enter the 
namespace and move the nics
+               // back to the host and rename them if liblxc hasn't already 
done it.
+               // We can only move back devices that have an expected 
host_name record and where
+               // that device doesn't already exist on the host as if a device 
exists on the host
+               // we can't know whether that is because liblxc has moved it 
back already or whether
+               // it is a conflicting device.
+               if !shared.PathExists(fmt.Sprintf("/sys/class/net/%s", 
devName)) {
+                       err := c.detachInterfaceRename(stopHookNetnsPath, 
configCopy["name"], devName)
+                       if err != nil {
+                               return errors.Wrapf(err, "Failed to detach 
interface: %s to %s", configCopy["name"], devName)
+                       }
+               }
        }
 
        return nil
@@ -2537,8 +2550,8 @@ func (c *containerLXC) startCommon() (string, []func() 
error, error) {
                                }
 
                                // Add any post start hooks.
-                               if len(runConfig.PostStartHooks) > 0 {
-                                       postStartHooks = append(postStartHooks, 
runConfig.PostStartHooks...)
+                               if len(runConfig.PostHooks) > 0 {
+                                       postStartHooks = append(postStartHooks, 
runConfig.PostHooks...)
                                }
 
                                continue
@@ -2732,7 +2745,7 @@ func (c *containerLXC) Start(stateful bool) error {
                }
 
                // Run any post start hooks.
-               err = c.runPostStartHooks(postStartHooks)
+               err = c.runHooks(postStartHooks)
                if err != nil {
                        // Attempt to stop container.
                        op.Done(err)
@@ -2800,7 +2813,7 @@ func (c *containerLXC) Start(stateful bool) error {
        }
 
        // Run any post start hooks.
-       err = c.runPostStartHooks(postStartHooks)
+       err = c.runHooks(postStartHooks)
        if err != nil {
                // Attempt to stop container.
                op.Done(err)
@@ -5711,7 +5724,7 @@ func (c *containerLXC) Migrate(args *CriuMigrationArgs) 
error {
 
                if migrateErr == nil {
                        // Run any post start hooks.
-                       err := c.runPostStartHooks(postStartHooks)
+                       err := c.runHooks(postStartHooks)
                        if err != nil {
                                // Attempt to stop container.
                                c.Stop(false)

From 9e3e7273247e76f071a83f9be8d5833035310543 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:03:40 +0100
Subject: [PATCH 02/11] device: Updates interface for Stop() to return
 RunConfig

For supporting post stop hooks.

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 lxd/device/device.go | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lxd/device/device.go b/lxd/device/device.go
index 4281bdcd3e..bd481de53e 100644
--- a/lxd/device/device.go
+++ b/lxd/device/device.go
@@ -46,7 +46,8 @@ type Device interface {
 
        // Stop performs any host-side cleanup required when a device is 
removed from an instance,
        // either due to unplugging it from a running instance or instance is 
being shutdown.
-       Stop() error
+       // Returns run-time configuration needed for detaching the device from 
the instance.
+       Stop() (*RunConfig, error)
 
        // Remove performs any host-side cleanup when an instance is removed 
from an instance.
        Remove() error

From 342526cee818f329c2d76c6e445389d833d359e8 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:04:12 +0100
Subject: [PATCH 03/11] device/device/runconfig: Renames PostStartHooks to
 PostHooks

So it can be used with both Start() and Stop() device functions.

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 lxd/device/device_runconfig.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lxd/device/device_runconfig.go b/lxd/device/device_runconfig.go
index 831dcef688..9bf5633ff3 100644
--- a/lxd/device/device_runconfig.go
+++ b/lxd/device/device_runconfig.go
@@ -6,8 +6,8 @@ type RunConfigItem struct {
        Value string
 }
 
-// RunConfig represents LXD defined run-time config used for device setup.
+// RunConfig represents LXD defined run-time config used for device 
setup/cleanup.
 type RunConfig struct {
-       NetworkInterface []RunConfigItem
-       PostStartHooks   []func() error
+       NetworkInterface []RunConfigItem // Network interface configuration 
settings.
+       PostHooks        []func() error  // Functions to be run after device 
attach/detach.
 }

From 9f6b910d425ba42b10f984ea9f893a1114504e3c Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:04:49 +0100
Subject: [PATCH 04/11] device/nic/bridged: Updates for post stop hooks

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 lxd/device/nic_bridged.go | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/lxd/device/nic_bridged.go b/lxd/device/nic_bridged.go
index 1dcd37713b..b2fe08a569 100644
--- a/lxd/device/nic_bridged.go
+++ b/lxd/device/nic_bridged.go
@@ -229,7 +229,16 @@ func (d *nicBridged) Update(oldConfig config.Device, 
isRunning bool) error {
 }
 
 // Stop is run when the device is removed from the instance.
-func (d *nicBridged) Stop() error {
+func (d *nicBridged) Stop() (*RunConfig, error) {
+       runConfig := RunConfig{
+               PostHooks: []func() error{d.postStop},
+       }
+
+       return &runConfig, nil
+}
+
+// postStop is run after the device is removed from the instance.
+func (d *nicBridged) postStop() error {
        defer d.volatileSet(map[string]string{
                "host_name": "",
        })

From 2ce181faf7f2ac8de9f6f1a6d31848be1766ddb2 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:05:19 +0100
Subject: [PATCH 05/11] device/nic/p2p: Updates for post stop hooks

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 lxd/device/nic_p2p.go | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lxd/device/nic_p2p.go b/lxd/device/nic_p2p.go
index ea6909c816..7f37af4a16 100644
--- a/lxd/device/nic_p2p.go
+++ b/lxd/device/nic_p2p.go
@@ -116,8 +116,17 @@ func (d *nicP2P) Update(oldConfig config.Device, isRunning 
bool) error {
        return nil
 }
 
-// Stop is run when the device is removed from the container.
-func (d *nicP2P) Stop() error {
+// Stop is run when the device is removed from the instance.
+func (d *nicP2P) Stop() (*RunConfig, error) {
+       runConfig := RunConfig{
+               PostHooks: []func() error{d.postStop},
+       }
+
+       return &runConfig, nil
+}
+
+// postStop is run after the device is removed from the instance.
+func (d *nicP2P) postStop() error {
        defer d.volatileSet(map[string]string{
                "host_name": "",
        })

From a8de4b7115540c5acdfd42460119f53695aac12f Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:56:31 +0100
Subject: [PATCH 06/11] device/nic/vlan: Updates for post stop hooks

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 lxd/device/nic_ipvlan.go | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lxd/device/nic_ipvlan.go b/lxd/device/nic_ipvlan.go
index c380d549d3..e67565d7af 100644
--- a/lxd/device/nic_ipvlan.go
+++ b/lxd/device/nic_ipvlan.go
@@ -208,8 +208,17 @@ func (d *nicIPVLAN) setupParentSysctls(parentName string) 
error {
        return nil
 }
 
-// Stop is run when the device is removed from the container.
-func (d *nicIPVLAN) Stop() error {
+// Stop is run when the device is removed from the instance.
+func (d *nicIPVLAN) Stop() (*RunConfig, error) {
+       runConfig := RunConfig{
+               PostHooks: []func() error{d.postStop},
+       }
+
+       return &runConfig, nil
+}
+
+// postStop is run after the device is removed from the instance.
+func (d *nicIPVLAN) postStop() error {
        defer d.volatileSet(map[string]string{
                "last_state.created": "",
        })

From ab55b694bf6297d4e1a39ed47ef982eba95d8c59 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:56:45 +0100
Subject: [PATCH 07/11] device/nic/macvlan: Updates for post stop hooks

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 lxd/device/nic_macvlan.go | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/lxd/device/nic_macvlan.go b/lxd/device/nic_macvlan.go
index 1aabbeb660..2f75467374 100644
--- a/lxd/device/nic_macvlan.go
+++ b/lxd/device/nic_macvlan.go
@@ -109,8 +109,21 @@ func (d *nicMACVLAN) Start() (*RunConfig, error) {
        return &runConf, nil
 }
 
-// Stop is run when the device is removed from the container.
-func (d *nicMACVLAN) Stop() error {
+// Stop is run when the device is removed from the instance.
+func (d *nicMACVLAN) Stop() (*RunConfig, error) {
+       v := d.volatileGet()
+       runConfig := RunConfig{
+               PostHooks: []func() error{d.postStop},
+               NetworkInterface: []RunConfigItem{
+                       {Key: "link", Value: v["host_name"]},
+               },
+       }
+
+       return &runConfig, nil
+}
+
+// postStop is run after the device is removed from the instance.
+func (d *nicMACVLAN) postStop() error {
        defer d.volatileSet(map[string]string{
                "host_name":          "",
                "last_state.hwaddr":  "",

From 55414948cd940e0c0ed1b5719f95d67cd11370cc Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:56:57 +0100
Subject: [PATCH 08/11] device/nic/physical: Updates for post stop hooks

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 lxd/device/nic_physical.go | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/lxd/device/nic_physical.go b/lxd/device/nic_physical.go
index 5faa6a2ec4..092ae01e8c 100644
--- a/lxd/device/nic_physical.go
+++ b/lxd/device/nic_physical.go
@@ -115,8 +115,21 @@ func (d *nicPhysical) Start() (*RunConfig, error) {
        return &runConf, nil
 }
 
-// Stop is run when the device is removed from the container.
-func (d *nicPhysical) Stop() error {
+// Stop is run when the device is removed from the instance.
+func (d *nicPhysical) Stop() (*RunConfig, error) {
+       v := d.volatileGet()
+       runConfig := RunConfig{
+               PostHooks: []func() error{d.postStop},
+               NetworkInterface: []RunConfigItem{
+                       {Key: "link", Value: v["host_name"]},
+               },
+       }
+
+       return &runConfig, nil
+}
+
+// postStop is run after the device is removed from the instance.
+func (d *nicPhysical) postStop() error {
        defer d.volatileSet(map[string]string{
                "host_name":          "",
                "last_state.hwaddr":  "",

From 6cdb29e2dd6f7e7f7df3528017058c171fe5e226 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:57:10 +0100
Subject: [PATCH 09/11] device/nic/sriov: Updates for post stop hooks

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 lxd/device/nic_sriov.go | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/lxd/device/nic_sriov.go b/lxd/device/nic_sriov.go
index ebad700af6..37cb536917 100644
--- a/lxd/device/nic_sriov.go
+++ b/lxd/device/nic_sriov.go
@@ -141,8 +141,21 @@ func (d *nicSRIOV) Start() (*RunConfig, error) {
        return &runConf, nil
 }
 
-// Stop is run when the device is removed from the container.
-func (d *nicSRIOV) Stop() error {
+// Stop is run when the device is removed from the instance.
+func (d *nicSRIOV) Stop() (*RunConfig, error) {
+       v := d.volatileGet()
+       runConfig := RunConfig{
+               PostHooks: []func() error{d.postStop},
+               NetworkInterface: []RunConfigItem{
+                       {Key: "link", Value: v["host_name"]},
+               },
+       }
+
+       return &runConfig, nil
+}
+
+// postStop is run after the device is removed from the instance.
+func (d *nicSRIOV) postStop() error {
        defer d.volatileSet(map[string]string{
                "host_name":                "",
                "last_state.hwaddr":        "",

From 409743eb7fe64a67cd0ff7634510bb47cae55da5 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:57:25 +0100
Subject: [PATCH 10/11] device/proxy: Updates for post stop hooks

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 lxd/device/proxy.go | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lxd/device/proxy.go b/lxd/device/proxy.go
index dbb3ad7fed..2d0c3cef88 100644
--- a/lxd/device/proxy.go
+++ b/lxd/device/proxy.go
@@ -140,7 +140,7 @@ func (d *proxy) Start() (*RunConfig, error) {
 
        // Proxy devices have to be setup once the container is running.
        runConf := RunConfig{}
-       runConf.PostStartHooks = []func() error{
+       runConf.PostHooks = []func() error{
                func() error {
                        if shared.IsTrue(d.config["nat"]) {
                                return d.setupNAT()
@@ -226,8 +226,8 @@ func (d *proxy) checkProcStarted(logPath string) (bool, 
error) {
        return false, nil
 }
 
-// Stop is run when the device is removed from the container.
-func (d *proxy) Stop() error {
+// Stop is run when the device is removed from the instance.
+func (d *proxy) Stop() (*RunConfig, error) {
        // Remove possible iptables entries
        iptables.ContainerClear("ipv4", fmt.Sprintf("%s (%s)", 
d.instance.Name(), d.name), "nat")
        iptables.ContainerClear("ipv6", fmt.Sprintf("%s (%s)", 
d.instance.Name(), d.name), "nat")
@@ -237,15 +237,15 @@ func (d *proxy) Stop() error {
 
        if !shared.PathExists(devPath) {
                // There's no proxy process if NAT is enabled
-               return nil
+               return nil, nil
        }
 
        err := d.killProxyProc(devPath)
        if err != nil {
-               return err
+               return nil, err
        }
 
-       return nil
+       return nil, nil
 }
 
 func (d *proxy) setupNAT() error {

From daeffea60e3fae5fb329ffe62a01d624dec5fedd Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Mon, 5 Aug 2019 11:57:45 +0100
Subject: [PATCH 11/11] test: Updates NIC tests to check for volatile key
 cleanup

Also makes variable naming more consistent.

Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
 test/suites/container_devices_nic_bridged.sh  |  7 ++
 ...container_devices_nic_bridged_filtering.sh |  6 ++
 test/suites/container_devices_nic_ipvlan.sh   | 65 ++++++++++--------
 test/suites/container_devices_nic_macvlan.sh  | 68 +++++++++++--------
 test/suites/container_devices_nic_p2p.sh      |  8 ++-
 test/suites/container_devices_nic_physical.sh |  6 +-
 test/suites/container_devices_nic_sriov.sh    |  5 ++
 7 files changed, 103 insertions(+), 62 deletions(-)

diff --git a/test/suites/container_devices_nic_bridged.sh 
b/test/suites/container_devices_nic_bridged.sh
index 88ec2d6cf5..2a6fdee16b 100644
--- a/test/suites/container_devices_nic_bridged.sh
+++ b/test/suites/container_devices_nic_bridged.sh
@@ -283,6 +283,13 @@ test_container_devices_nic_bridged() {
     false
   fi
 
+  # Check volatile cleanup on stop.
+  lxc stop -f "${ctName}"
+  if lxc config show "${ctName}" | grep volatile.eth0 ; then
+    echo "unexpected volatile key remains"
+    false
+  fi
+
   # Test DHCP lease clearance.
   lxc delete "${ctName}" -f
   lxc launch testimage "${ctName}" -p "${ctName}"
diff --git a/test/suites/container_devices_nic_bridged_filtering.sh 
b/test/suites/container_devices_nic_bridged_filtering.sh
index 23ace8c2b7..997b74b0b9 100644
--- a/test/suites/container_devices_nic_bridged_filtering.sh
+++ b/test/suites/container_devices_nic_bridged_filtering.sh
@@ -274,6 +274,12 @@ test_container_devices_nic_bridged_filtering() {
       false
   fi
 
+  # Check volatile cleanup on stop.
+  if lxc config show "${ctPrefix}A" | grep volatile.eth0 | grep -v 
volatile.eth0.hwaddr ; then
+    echo "unexpected volatile key remains"
+    false
+  fi
+
   # Set static MAC so that SLAAC address is derived predictably and check it 
is applied to static config.
   lxc config device unset "${ctPrefix}A" eth0 ipv6.address
   lxc config device set "${ctPrefix}A" eth0 hwaddr 00:16:3e:92:f3:c1
diff --git a/test/suites/container_devices_nic_ipvlan.sh 
b/test/suites/container_devices_nic_ipvlan.sh
index 811d2122f4..c77e32165d 100644
--- a/test/suites/container_devices_nic_ipvlan.sh
+++ b/test/suites/container_devices_nic_ipvlan.sh
@@ -7,67 +7,72 @@ test_container_devices_nic_ipvlan() {
     return
   fi
 
-  ct_name="nt$$"
+  ctName="nt$$"
   ipRand=$(shuf -i 0-9 -n 1)
 
   # Test ipvlan support to offline container (hot plugging not supported).
-  ip link add "${ct_name}" type dummy
+  ip link add "${ctName}" type dummy
 
   # Record how many nics we started with.
   startNicCount=$(find /sys/class/net | wc -l)
 
   # Check that starting IPVLAN container.
-  sysctl net.ipv6.conf."${ct_name}".proxy_ndp=1
-  sysctl net.ipv6.conf."${ct_name}".forwarding=1
-  sysctl net.ipv4.conf."${ct_name}".forwarding=1
-  lxc init testimage "${ct_name}"
-  lxc config device add "${ct_name}" eth0 nic \
+  sysctl net.ipv6.conf."${ctName}".proxy_ndp=1
+  sysctl net.ipv6.conf."${ctName}".forwarding=1
+  sysctl net.ipv4.conf."${ctName}".forwarding=1
+  lxc init testimage "${ctName}"
+  lxc config device add "${ctName}" eth0 nic \
     nictype=ipvlan \
-    parent=${ct_name} \
+    parent=${ctName} \
     ipv4.address="192.0.2.1${ipRand}" \
     ipv6.address="2001:db8::1${ipRand}" \
     mtu=1400
-  lxc start "${ct_name}"
+  lxc start "${ctName}"
 
   # Check custom MTU is applied.
-  if ! lxc exec "${ct_name}" -- ip link show eth0 | grep "mtu 1400" ; then
+  if ! lxc exec "${ctName}" -- ip link show eth0 | grep "mtu 1400" ; then
     echo "mtu invalid"
     false
   fi
 
   #Spin up another container with multiple IPs.
-  lxc init testimage "${ct_name}2"
-  lxc config device add "${ct_name}2" eth0 nic \
+  lxc init testimage "${ctName}2"
+  lxc config device add "${ctName}2" eth0 nic \
     nictype=ipvlan \
-    parent=${ct_name} \
+    parent=${ctName} \
     ipv4.address="192.0.2.2${ipRand}, 192.0.2.3${ipRand}" \
     ipv6.address="2001:db8::2${ipRand}, 2001:db8::3${ipRand}"
-  lxc start "${ct_name}2"
+  lxc start "${ctName}2"
 
   # Check comms between containers.
-  lxc exec "${ct_name}" -- ping -c2 -W1 "192.0.2.2${ipRand}"
-  lxc exec "${ct_name}" -- ping -c2 -W1 "192.0.2.3${ipRand}"
-  lxc exec "${ct_name}" -- ping6 -c2 -W1 "2001:db8::2${ipRand}"
-  lxc exec "${ct_name}" -- ping6 -c2 -W1 "2001:db8::3${ipRand}"
-  lxc exec "${ct_name}2" -- ping -c2 -W1 "192.0.2.1${ipRand}"
-  lxc exec "${ct_name}2" -- ping6 -c2 -W1 "2001:db8::1${ipRand}"
-  lxc stop -f "${ct_name}2"
+  lxc exec "${ctName}" -- ping -c2 -W1 "192.0.2.2${ipRand}"
+  lxc exec "${ctName}" -- ping -c2 -W1 "192.0.2.3${ipRand}"
+  lxc exec "${ctName}" -- ping6 -c2 -W1 "2001:db8::2${ipRand}"
+  lxc exec "${ctName}" -- ping6 -c2 -W1 "2001:db8::3${ipRand}"
+  lxc exec "${ctName}2" -- ping -c2 -W1 "192.0.2.1${ipRand}"
+  lxc exec "${ctName}2" -- ping6 -c2 -W1 "2001:db8::1${ipRand}"
+  lxc stop -f "${ctName}2"
 
   # Check IPVLAN ontop of VLAN parent.
-  lxc stop -f "${ct_name}"
-  lxc config device set "${ct_name}" eth0 vlan 1234
-  lxc start "${ct_name}"
+  lxc stop -f "${ctName}"
+  lxc config device set "${ctName}" eth0 vlan 1234
+  lxc start "${ctName}"
 
   # Check VLAN interface created
-  if ! grep "1" "/sys/class/net/${ct_name}.1234/carrier" ; then
+  if ! grep "1" "/sys/class/net/${ctName}.1234/carrier" ; then
     echo "vlan interface not created"
     false
   fi
 
-  lxc stop -f "${ct_name}"
+  # Check volatile cleanup on stop.
+  lxc stop -f "${ctName}"
+  if lxc config show "${ctName}" | grep volatile.eth0 | grep -v 
volatile.eth0.hwaddr | grep -v volatile.eth0.name ; then
+    echo "unexpected volatile key remains"
+    false
+  fi
 
   # Check parent device is still up.
-  if ! grep "1" "/sys/class/net/${ct_name}/carrier" ; then
+  if ! grep "1" "/sys/class/net/${ctName}/carrier" ; then
     echo "parent is down"
     false
   fi
@@ -80,7 +85,7 @@ test_container_devices_nic_ipvlan() {
   fi
 
   # Cleanup ipvlan checks
-  lxc delete "${ct_name}" -f
-  lxc delete "${ct_name}2" -f
-  ip link delete "${ct_name}" type dummy
+  lxc delete "${ctName}" -f
+  lxc delete "${ctName}2" -f
+  ip link delete "${ctName}" type dummy
 }
diff --git a/test/suites/container_devices_nic_macvlan.sh 
b/test/suites/container_devices_nic_macvlan.sh
index 3530e311e5..1070be7bbb 100644
--- a/test/suites/container_devices_nic_macvlan.sh
+++ b/test/suites/container_devices_nic_macvlan.sh
@@ -2,86 +2,94 @@ test_container_devices_nic_macvlan() {
   ensure_import_testimage
   ensure_has_localhost_remote "${LXD_ADDR}"
 
-  ct_name="nt$$"
+  ctName="nt$$"
   ipRand=$(shuf -i 0-9 -n 1)
 
   # Create dummy interface for use as parent.
-  ip link add "${ct_name}" type dummy
-  ip link set "${ct_name}" up
+  ip link add "${ctName}" type dummy
+  ip link set "${ctName}" up
 
   # Record how many nics we started with.
   startNicCount=$(find /sys/class/net | wc -l)
 
   # Test pre-launch profile config is applied at launch.
-  lxc profile copy default "${ct_name}"
+  lxc profile copy default "${ctName}"
 
   # Modifiy profile nictype and parent in atomic operation to ensure 
validation passes.
-  lxc profile show "${ct_name}" | sed  "s/nictype: p2p/nictype: macvlan\n    
parent: ${ct_name}/" | lxc profile edit "${ct_name}"
-  lxc profile device set "${ct_name}" eth0 mtu "1400"
+  lxc profile show "${ctName}" | sed  "s/nictype: p2p/nictype: macvlan\n    
parent: ${ctName}/" | lxc profile edit "${ctName}"
+  lxc profile device set "${ctName}" eth0 mtu "1400"
 
-  lxc launch testimage "${ct_name}" -p "${ct_name}"
-  lxc exec "${ct_name}" -- ip addr add "192.0.2.1${ipRand}/24" dev eth0
-  lxc exec "${ct_name}" -- ip addr add "2001:db8::1${ipRand}/64" dev eth0
+  lxc launch testimage "${ctName}" -p "${ctName}"
+  lxc exec "${ctName}" -- ip addr add "192.0.2.1${ipRand}/24" dev eth0
+  lxc exec "${ctName}" -- ip addr add "2001:db8::1${ipRand}/64" dev eth0
 
   # Check custom MTU is applied if feature available in LXD.
   if lxc info | grep 'network_phys_macvlan_mtu: "true"' ; then
-    if ! lxc exec "${ct_name}" -- ip link show eth0 | grep "mtu 1400" ; then
+    if ! lxc exec "${ctName}" -- ip link show eth0 | grep "mtu 1400" ; then
       echo "mtu invalid"
       false
     fi
   fi
 
   #Spin up another container with multiple IPs.
-  lxc launch testimage "${ct_name}2" -p "${ct_name}"
-  lxc exec "${ct_name}2" -- ip addr add "192.0.2.2${ipRand}/24" dev eth0
-  lxc exec "${ct_name}2" -- ip addr add "2001:db8::2${ipRand}/64" dev eth0
+  lxc launch testimage "${ctName}2" -p "${ctName}"
+  lxc exec "${ctName}2" -- ip addr add "192.0.2.2${ipRand}/24" dev eth0
+  lxc exec "${ctName}2" -- ip addr add "2001:db8::2${ipRand}/64" dev eth0
 
   # Check comms between containers.
-  lxc exec "${ct_name}" -- ping -c2 -W1 "192.0.2.2${ipRand}"
-  lxc exec "${ct_name}" -- ping6 -c2 -W1 "2001:db8::2${ipRand}"
-  lxc exec "${ct_name}2" -- ping -c2 -W1 "192.0.2.1${ipRand}"
-  lxc exec "${ct_name}2" -- ping6 -c2 -W1 "2001:db8::1${ipRand}"
+  lxc exec "${ctName}" -- ping -c2 -W1 "192.0.2.2${ipRand}"
+  lxc exec "${ctName}" -- ping6 -c2 -W1 "2001:db8::2${ipRand}"
+  lxc exec "${ctName}2" -- ping -c2 -W1 "192.0.2.1${ipRand}"
+  lxc exec "${ctName}2" -- ping6 -c2 -W1 "2001:db8::1${ipRand}"
 
   # Test hot plugging a container nic with different settings to profile with 
the same name.
-  lxc config device add "${ct_name}" eth0 nic \
+  lxc config device add "${ctName}" eth0 nic \
     nictype=macvlan \
     name=eth0 \
-    parent="${ct_name}" \
+    parent="${ctName}" \
     mtu=1401
 
   # Check custom MTU is applied on hot-plug.
-  if ! lxc exec "${ct_name}" -- ip link show eth0 | grep "mtu 1401" ; then
+  if ! lxc exec "${ctName}" -- ip link show eth0 | grep "mtu 1401" ; then
     echo "mtu invalid"
     false
   fi
 
-  lxc config device remove "${ct_name}" eth0
+  # Check volatile cleanup on stop.
+  lxc stop -f "${ctName}"
+  if lxc config show "${ctName}" | grep volatile.eth0 | grep -v 
volatile.eth0.hwaddr ; then
+    echo "unexpected volatile key remains"
+    false
+  fi
+
+  lxc start "${ctName}"
+  lxc config device remove "${ctName}" eth0
 
   # Test hot plugging macvlan device based on vlan parent.
-  lxc config device add "${ct_name}" eth0 nic \
+  lxc config device add "${ctName}" eth0 nic \
     nictype=macvlan \
-    parent="${ct_name}" \
+    parent="${ctName}" \
     name=eth0 \
     vlan=10 \
     mtu=1402
 
   # Check custom MTU is applied.
-  if ! lxc exec "${ct_name}" -- ip link show eth0 | grep "mtu 1402" ; then
+  if ! lxc exec "${ctName}" -- ip link show eth0 | grep "mtu 1402" ; then
     echo "mtu invalid"
     false
   fi
 
   # Check VLAN interface created
-  if ! grep "1" "/sys/class/net/${ct_name}.10/carrier" ; then
+  if ! grep "1" "/sys/class/net/${ctName}.10/carrier" ; then
     echo "vlan interface not created"
     false
   fi
 
   # Remove device from container, this should also remove created VLAN parent 
device.
-  lxc config device remove "${ct_name}" eth0
+  lxc config device remove "${ctName}" eth0
 
   # Check parent device is still up.
-  if ! grep "1" "/sys/class/net/${ct_name}/carrier" ; then
+  if ! grep "1" "/sys/class/net/${ctName}/carrier" ; then
     echo "parent is down"
     false
   fi
@@ -94,7 +102,7 @@ test_container_devices_nic_macvlan() {
   fi
 
   # Cleanup.
-  lxc delete "${ct_name}" -f
-  lxc delete "${ct_name}2" -f
-  ip link delete "${ct_name}" type dummy
+  lxc delete "${ctName}" -f
+  lxc delete "${ctName}2" -f
+  ip link delete "${ctName}" type dummy
 }
diff --git a/test/suites/container_devices_nic_p2p.sh 
b/test/suites/container_devices_nic_p2p.sh
index a6db37b026..01bbae94a3 100644
--- a/test/suites/container_devices_nic_p2p.sh
+++ b/test/suites/container_devices_nic_p2p.sh
@@ -244,8 +244,14 @@ test_container_devices_nic_p2p() {
     false
   fi
 
-  # Now add a nic to a stopped container with routes.
+  # Check volatile cleanup on stop.
   lxc stop -f "${ctName}"
+  if lxc config show "${ctName}" | grep volatile.eth0 | grep -v 
volatile.eth0.hwaddr ; then
+    echo "unexpected volatile key remains"
+    false
+  fi
+
+  # Now add a nic to a stopped container with routes.
   lxc config device add "${ctName}" eth0 nic \
     nictype=p2p \
     ipv4.routes="192.0.2.2${ipRand}/32" \
diff --git a/test/suites/container_devices_nic_physical.sh 
b/test/suites/container_devices_nic_physical.sh
index ab05624b2b..a027dc392b 100644
--- a/test/suites/container_devices_nic_physical.sh
+++ b/test/suites/container_devices_nic_physical.sh
@@ -40,8 +40,12 @@ test_container_devices_nic_physical() {
     false
   fi
 
-  # Stop container and check MTU is restored.
+  # Check volatile cleanup on stop.
   lxc stop -f "${ctName}"
+  if lxc config show "${ctName}" | grep volatile.eth0 ; then
+    echo "unexpected volatile key remains"
+    false
+  fi
 
   # Check original MTU is restored on physical device.
   if lxc info | grep 'network_phys_macvlan_mtu: "true"' ; then
diff --git a/test/suites/container_devices_nic_sriov.sh 
b/test/suites/container_devices_nic_sriov.sh
index 8c3ccc9cc5..8ae6429d99 100644
--- a/test/suites/container_devices_nic_sriov.sh
+++ b/test/suites/container_devices_nic_sriov.sh
@@ -72,7 +72,12 @@ test_container_devices_nic_sriov() {
     fi
   fi
 
+  # Check volatile cleanup on stop.
   lxc stop -f "${ctName}"
+  if lxc config show "${ctName}" | grep volatile.eth0 | grep -v 
volatile.eth0.hwaddr | grep -v volatile.eth0.name ; then
+    echo "unexpected volatile key remains"
+    false
+  fi
 
   # Set custom MAC
   lxc config device set "${ctName}" eth0 hwaddr "${ctMAC1}"
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Reply via email to