[lxc-devel] [lxd/master] Split auto and interactive logic

freeekanayaka on Github lxc-bot at linuxcontainers.org
Thu May 18 07:07:40 UTC 2017


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 1528 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20170518/d03780b3/attachment.bin>
-------------- next part --------------
From c3ee88c1307ca570037f16bd7179bec52f4ccdc6 Mon Sep 17 00:00:00 2001
From: Free Ekanayaka <free.ekanayaka at canonical.com>
Date: Thu, 4 May 2017 14:42:35 +0200
Subject: [PATCH 1/4] Extract validation of --auto args into a separate method

This just mechanically moves into a standalone method the code that
validates the various arguments that go with --auto. It makes the rest
of the code a bit shorter and easier to refactor in the follow-up
commit.

For good measure, I've also added a few unit tests covering some of
the invalid arguments combinations or values.

Signed-off-by: Free Ekanayaka <free.ekanayaka at canonical.com>
---
 lxd/main_init.go      | 128 ++++++++++++++++++++++++++++----------------------
 lxd/main_init_test.go |  50 ++++++++++++++++++++
 2 files changed, 123 insertions(+), 55 deletions(-)

diff --git a/lxd/main_init.go b/lxd/main_init.go
index be1510e..def9b3b 100644
--- a/lxd/main_init.go
+++ b/lxd/main_init.go
@@ -41,6 +41,10 @@ type CmdInit struct {
 
 // Run triggers the execution of the init command.
 func (cmd *CmdInit) Run() error {
+	// Figure what storage drivers among the supported ones are actually
+	// available on this system.
+	availableStoragePoolsDrivers := cmd.availableStoragePoolsDrivers()
+
 	// Check that command line arguments don't conflict with each other
 	err := cmd.validateArgs()
 	if err != nil {
@@ -57,7 +61,7 @@ func (cmd *CmdInit) Run() error {
 	if cmd.Args.Preseed {
 		err = cmd.runPreseed(client)
 	} else {
-		err = cmd.runAutoOrInteractive(client)
+		err = cmd.runAutoOrInteractive(client, availableStoragePoolsDrivers)
 	}
 
 	if err == nil {
@@ -74,7 +78,7 @@ func (cmd *CmdInit) Run() error {
 // runPreseed. The idea being that both runAuto and runInteractive
 // will end up populating the same low-level cmdInitData structure
 // passed to the common run() method.
-func (cmd *CmdInit) runAutoOrInteractive(c lxd.ContainerServer) error {
+func (cmd *CmdInit) runAutoOrInteractive(c lxd.ContainerServer, backendsAvailable []string) error {
 	var defaultPrivileged int // controls whether we set security.privileged=true
 	var storageSetup bool     // == supportedStoragePoolDrivers
 	var storageBackend string // == supportedStoragePoolDrivers
@@ -97,29 +101,6 @@ func (cmd *CmdInit) runAutoOrInteractive(c lxd.ContainerServer) error {
 	runningInUserns = cmd.RunningInUserns
 	imagesAutoUpdate = true
 
-	backendsAvailable := []string{"dir"}
-
-	// Check available backends
-	for _, driver := range supportedStoragePoolDrivers {
-		if driver == "dir" {
-			continue
-		}
-
-		// btrfs can work in user namespaces too. (If
-		// source=/some/path/on/btrfs is used.)
-		if runningInUserns && driver != "btrfs" {
-			continue
-		}
-
-		// Initialize a core storage interface for the given driver.
-		_, err := storageCoreInit(driver)
-		if err != nil {
-			continue
-		}
-
-		backendsAvailable = append(backendsAvailable, driver)
-	}
-
 	pools, err := c.GetStoragePoolNames()
 	if err != nil {
 		// We should consider this fatal since this means
@@ -132,32 +113,9 @@ func (cmd *CmdInit) runAutoOrInteractive(c lxd.ContainerServer) error {
 			cmd.Args.StorageBackend = "dir"
 		}
 
-		// Do a bunch of sanity checks
-		if !shared.StringInSlice(cmd.Args.StorageBackend, supportedStoragePoolDrivers) {
-			return fmt.Errorf("The requested backend '%s' isn't supported by lxd init.", cmd.Args.StorageBackend)
-		}
-
-		if !shared.StringInSlice(cmd.Args.StorageBackend, backendsAvailable) {
-			return fmt.Errorf("The requested backend '%s' isn't available on your system (missing tools).", cmd.Args.StorageBackend)
-		}
-
-		if cmd.Args.StorageBackend == "dir" {
-			if cmd.Args.StorageCreateLoop != -1 || cmd.Args.StorageCreateDevice != "" || cmd.Args.StorageDataset != "" {
-				return fmt.Errorf("None of --storage-pool, --storage-create-device or --storage-create-loop may be used with the 'dir' backend.")
-			}
-		} else {
-			if cmd.Args.StorageCreateLoop != -1 && cmd.Args.StorageCreateDevice != "" {
-				return fmt.Errorf("Only one of --storage-create-device or --storage-create-loop can be specified.")
-			}
-		}
-
-		if cmd.Args.NetworkAddress == "" {
-			if cmd.Args.NetworkPort != -1 {
-				return fmt.Errorf("--network-port cannot be used without --network-address.")
-			}
-			if cmd.Args.TrustPassword != "" {
-				return fmt.Errorf("--trust-password cannot be used without --network-address.")
-			}
+		err = cmd.validateArgsAuto(backendsAvailable)
+		if err != nil {
+			return err
 		}
 
 		storageBackend = cmd.Args.StorageBackend
@@ -175,10 +133,6 @@ func (cmd *CmdInit) runAutoOrInteractive(c lxd.ContainerServer) error {
 			storageSetup = true
 		}
 	} else {
-		if cmd.Args.StorageBackend != "" || cmd.Args.StorageCreateDevice != "" || cmd.Args.StorageCreateLoop != -1 || cmd.Args.StorageDataset != "" || cmd.Args.NetworkAddress != "" || cmd.Args.NetworkPort != -1 || cmd.Args.TrustPassword != "" {
-			return fmt.Errorf("Init configuration is only valid with --auto")
-		}
-
 		defaultStorage := "dir"
 		if shared.StringInSlice("zfs", backendsAvailable) {
 			defaultStorage = "zfs"
@@ -752,9 +706,73 @@ func (cmd *CmdInit) validateArgs() error {
 	if cmd.Args.Auto && cmd.Args.Preseed {
 		return fmt.Errorf("Non-interactive mode supported by only one of --auto or --preseed")
 	}
+	if !cmd.Args.Auto {
+		if cmd.Args.StorageBackend != "" || cmd.Args.StorageCreateDevice != "" || cmd.Args.StorageCreateLoop != -1 || cmd.Args.StorageDataset != "" || cmd.Args.NetworkAddress != "" || cmd.Args.NetworkPort != -1 || cmd.Args.TrustPassword != "" {
+			return fmt.Errorf("Init configuration is only valid with --auto")
+		}
+	}
+	return nil
+}
+
+// Check that the arguments passed along with --auto are valid and consistent.
+// and no invalid combination is provided.
+func (cmd *CmdInit) validateArgsAuto(availableStoragePoolsDrivers []string) error {
+	if !shared.StringInSlice(cmd.Args.StorageBackend, supportedStoragePoolDrivers) {
+		return fmt.Errorf("The requested backend '%s' isn't supported by lxd init.", cmd.Args.StorageBackend)
+	}
+	if !shared.StringInSlice(cmd.Args.StorageBackend, availableStoragePoolsDrivers) {
+		return fmt.Errorf("The requested backend '%s' isn't available on your system (missing tools).", cmd.Args.StorageBackend)
+	}
+
+	if cmd.Args.StorageBackend == "dir" {
+		if cmd.Args.StorageCreateLoop != -1 || cmd.Args.StorageCreateDevice != "" || cmd.Args.StorageDataset != "" {
+			return fmt.Errorf("None of --storage-pool, --storage-create-device or --storage-create-loop may be used with the 'dir' backend.")
+		}
+	} else {
+		if cmd.Args.StorageCreateLoop != -1 && cmd.Args.StorageCreateDevice != "" {
+			return fmt.Errorf("Only one of --storage-create-device or --storage-create-loop can be specified.")
+		}
+	}
+
+	if cmd.Args.NetworkAddress == "" {
+		if cmd.Args.NetworkPort != -1 {
+			return fmt.Errorf("--network-port cannot be used without --network-address.")
+		}
+		if cmd.Args.TrustPassword != "" {
+			return fmt.Errorf("--trust-password cannot be used without --network-address.")
+		}
+	}
+
 	return nil
 }
 
+// Return the available storage pools drivers (depending on installed tools).
+func (cmd *CmdInit) availableStoragePoolsDrivers() []string {
+	drivers := []string{"dir"}
+
+	// Check available backends
+	for _, driver := range supportedStoragePoolDrivers {
+		if driver == "dir" {
+			continue
+		}
+
+		// btrfs can work in user namespaces too. (If
+		// source=/some/path/on/btrfs is used.)
+		if cmd.RunningInUserns && driver != "btrfs" {
+			continue
+		}
+
+		// Initialize a core storage interface for the given driver.
+		_, err := storageCoreInit(driver)
+		if err != nil {
+			continue
+		}
+
+		drivers = append(drivers, driver)
+	}
+	return drivers
+}
+
 func (cmd *CmdInit) setServerConfig(c lxd.ContainerServer, key string, value string) error {
 	server, etag, err := c.GetServer()
 	if err != nil {
diff --git a/lxd/main_init_test.go b/lxd/main_init_test.go
index bb18b3c..b856598 100644
--- a/lxd/main_init_test.go
+++ b/lxd/main_init_test.go
@@ -54,6 +54,13 @@ func (suite *cmdInitTestSuite) TestCmdInit_AutoAndPreseedIncompatible() {
 	suite.Req.Equal("Non-interactive mode supported by only one of --auto or --preseed", err.Error())
 }
 
+// Some arguments can only be passed together with --auto.
+func (suite *cmdInitTestSuite) TestCmdInit_AutoSpecificArgs() {
+	suite.args.StorageBackend = "dir"
+	err := suite.command.Run()
+	suite.Req.Equal("Init configuration is only valid with --auto", err.Error())
+}
+
 // If the YAML preseed data is invalid, an error is returned.
 func (suite *cmdInitTestSuite) TestCmdInit_PreseedInvalidYAML() {
 	suite.args.Preseed = true
@@ -140,6 +147,49 @@ func (suite *cmdInitTestSuite) TestCmdInit_ImagesAutoUpdateNoOverwrite() {
 	suite.Req.Equal("10", key.Get())
 }
 
+// If an invalid backend type is passed with --storage-backend, an
+// error is returned.
+func (suite *cmdInitTestSuite) TestCmdInit_AutoWithInvalidBackendType() {
+	suite.args.Auto = true
+	suite.args.StorageBackend = "foo"
+
+	err := suite.command.Run()
+	suite.Req.Equal("The requested backend 'foo' isn't supported by lxd init.", err.Error())
+}
+
+// If an backend type that is not available on the system is passed
+// with --storage-backend, an error is returned.
+func (suite *cmdInitTestSuite) TestCmdInit_AutoWithUnavailableBackendType() {
+	suite.args.Auto = true
+	suite.args.StorageBackend = "zfs"
+	suite.command.RunningInUserns = true // This makes zfs unavailable
+
+	err := suite.command.Run()
+	suite.Req.Equal("The requested backend 'zfs' isn't available on your system (missing tools).", err.Error())
+}
+
+// If --storage-backend is set to "dir", --storage-create-device can't be passed.
+func (suite *cmdInitTestSuite) TestCmdInit_AutoWithDirStorageBackendAndCreateDevice() {
+	suite.args.Auto = true
+	suite.args.StorageBackend = "dir"
+	suite.args.StorageCreateDevice = "/dev/sda4"
+
+	err := suite.command.Run()
+	suite.Req.Equal("None of --storage-pool, --storage-create-device or --storage-create-loop may be used with the 'dir' backend.", err.Error())
+}
+
+// If --storage-backend is set to "dir", and both of --storage-create-device
+// or --storage-create-loop are given, an error is returned.
+func (suite *cmdInitTestSuite) TestCmdInit_AutoWithNonDirBackendAndNoDeviceOrLoop() {
+	suite.args.Auto = true
+	suite.args.StorageBackend = "zfs"
+	suite.args.StorageCreateDevice = "/dev/sda4"
+	suite.args.StorageCreateLoop = 1
+
+	err := suite.command.Run()
+	suite.Req.Equal("Only one of --storage-create-device or --storage-create-loop can be specified.", err.Error())
+}
+
 // If the user answers "no" to the images auto-update question, the value will
 // be set to 0.
 func (suite *cmdInitTestSuite) TestCmdInit_ImagesAutoUpdateAnswerNo() {

From c6f0e37beb955cd7b81f272c093930c72df1305a Mon Sep 17 00:00:00 2001
From: Free Ekanayaka <free.ekanayaka at canonical.com>
Date: Thu, 4 May 2017 17:54:19 +0200
Subject: [PATCH 2/4] Consolidate interactive/auto init logic with the preseed
 one

Change initAutoOrInteractive to use the same code path that
initPreseeds uses to create or update the entities using the API.

This is done by making initAutoOrInteractive fill a cmdInitData
structure that has the same schema as the preseed YAML.

The resulting code is slightly shorter and less redundant.

Signed-off-by: Free Ekanayaka <free.ekanayaka at canonical.com>
---
 lxd/main_init.go                | 194 ++++++++++++----------------------------
 test/main.sh                    |   1 +
 test/suites/init_auto.sh        |   1 +
 test/suites/init_interactive.sh |  37 ++++++++
 4 files changed, 98 insertions(+), 135 deletions(-)
 create mode 100644 test/suites/init_interactive.sh

diff --git a/lxd/main_init.go b/lxd/main_init.go
index def9b3b..68ebe78 100644
--- a/lxd/main_init.go
+++ b/lxd/main_init.go
@@ -293,14 +293,23 @@ they otherwise would.
 		}
 	}
 
+	server, _, err := c.GetServer()
+	if err != nil {
+		return err
+	}
+
+	data := &cmdInitData{}
+	data.ServerPut = server.Writable()
+
+	// If there's a default profile, and certain conditions are
+	// met we'll update its root disk device and/or eth0 network
+	// device, as well as its privileged mode (see below).
+	defaultProfile, _, getDefaultProfileErr := c.GetProfile("default")
+
 	if storageSetup {
 		// Unset core.https_address and core.trust_password
-		for _, key := range []string{"core.https_address", "core.trust_password"} {
-			err = cmd.setServerConfig(c, key, "")
-			if err != nil {
-				return err
-			}
-		}
+		data.Config["core.https_address"] = ""
+		data.Config["core.trust_password"] = ""
 
 		// Pool configuration
 		storagePoolConfig := map[string]string{}
@@ -329,31 +338,15 @@ they otherwise would.
 		}
 		storageStruct.Config = storagePoolConfig
 
-		err := c.CreateStoragePool(storageStruct)
-		if err != nil {
-			return err
-		}
+		data.Pools = []api.StoragePoolsPost{storageStruct}
 
 		// When lxd init is rerun and there are already storage pools
 		// configured, do not try to set a root disk device in the
 		// default profile again. Let the user figure this out.
 		if len(pools) == 0 {
-			// Check if there even is a default profile.
-			profiles, err := c.GetProfiles()
-			if err != nil {
-				return err
-			}
-
-			defaultProfileExists := false
-			for _, p := range profiles {
-				if p.Name != "default" {
-					continue
-				}
-
-				defaultProfileExists = true
-
+			if getDefaultProfileErr == nil {
 				foundRootDiskDevice := false
-				for k, v := range p.Devices {
+				for k, v := range defaultProfile.Devices {
 					if v["path"] == "/" && v["source"] == "" {
 						foundRootDiskDevice = true
 
@@ -362,63 +355,39 @@ they otherwise would.
 						// the default profile it must be empty otherwise it would
 						// not have been possible to delete the storage pool in
 						// the first place.
-						update := p.Writable()
-						update.Devices[k]["pool"] = storagePool
-
-						// Update profile devices.
-						err := c.UpdateProfile("default", update, "")
-						if err != nil {
-							return err
-						}
+						defaultProfile.Devices[k]["pool"] = storagePool
 						logger.Debugf("Set pool property of existing root disk device \"%s\" in profile \"default\" to \"%s\".", storagePool)
 
 						break
 					}
 				}
 
-				if foundRootDiskDevice {
-					break
-				}
-
-				props := map[string]string{
-					"type": "disk",
-					"path": "/",
-					"pool": storagePool,
-				}
+				if !foundRootDiskDevice {
+					err = cmd.profileDeviceAlreadyExists(defaultProfile, "root")
+					if err != nil {
+						return err
+					}
 
-				err = cmd.profileDeviceAdd(c, "default", "root", props)
-				if err != nil {
-					return err
+					defaultProfile.Devices["root"] = map[string]string{
+						"type": "disk",
+						"path": "/",
+						"pool": storagePool,
+					}
 				}
-
-				break
-			}
-
-			if !defaultProfileExists {
+			} else {
 				logger.Warnf("Did not find profile \"default\" so no default storage pool will be set. Manual intervention needed.")
 			}
 		}
 	}
 
-	if defaultPrivileged == 0 {
-		err = cmd.setProfileConfigItem(c, "default", "security.privileged", "")
-		if err != nil {
-			return err
-		}
+	if defaultPrivileged != -1 && getDefaultProfileErr != nil {
+		return getDefaultProfileErr
+	} else if defaultPrivileged == 0 {
+		defaultProfile.Config["security.privileged"] = ""
 	} else if defaultPrivileged == 1 {
-		err = cmd.setProfileConfigItem(c, "default", "security.privileged", "true")
-		if err != nil {
-		}
-	}
-
-	server, _, err := c.GetServer()
-	if err != nil {
-		return err
+		defaultProfile.Config["security.privileged"] = "true"
 	}
 
-	data := &cmdInitData{}
-	data.ServerPut = server.Writable()
-
 	if networkAddress != "" {
 		data.Config["core.https_address"] = fmt.Sprintf("%s:%d", networkAddress, networkPort)
 		if trustPassword != "" {
@@ -451,26 +420,36 @@ they otherwise would.
 			Name: bridgeName}
 		network.Config = bridgeConfig
 		data.Networks = []api.NetworksPost{network}
-	}
 
-	err = cmd.run(c, data)
-	if err != nil {
-		return nil
-	}
+		if getDefaultProfileErr != nil {
+			return getDefaultProfileErr
+		}
 
-	if bridgeName != "" {
+		err = cmd.profileDeviceAlreadyExists(defaultProfile, "eth0")
+		if err != nil {
+			return err
+		}
 
-		props := map[string]string{
+		defaultProfile.Devices["eth0"] = map[string]string{
 			"type":    "nic",
 			"nictype": "bridged",
 			"parent":  bridgeName,
 		}
 
-		err = cmd.profileDeviceAdd(c, "default", "eth0", props)
-		if err != nil {
-			return err
-		}
 	}
+
+	if getDefaultProfileErr == nil {
+		// Copy the default profile configuration (that we have
+		// possibly modified above).
+		data.Profiles = []api.ProfilesPost{{Name: "default"}}
+		data.Profiles[0].ProfilePut = defaultProfile.ProfilePut
+	}
+
+	err = cmd.run(c, data)
+	if err != nil {
+		return nil
+	}
+
 	return nil
 }
 
@@ -773,68 +752,13 @@ func (cmd *CmdInit) availableStoragePoolsDrivers() []string {
 	return drivers
 }
 
-func (cmd *CmdInit) setServerConfig(c lxd.ContainerServer, key string, value string) error {
-	server, etag, err := c.GetServer()
-	if err != nil {
-		return err
-	}
-
-	if server.Config == nil {
-		server.Config = map[string]interface{}{}
-	}
-
-	server.Config[key] = value
-
-	err = c.UpdateServer(server.Writable(), etag)
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
-func (cmd *CmdInit) profileDeviceAdd(c lxd.ContainerServer, profileName string, deviceName string, deviceConfig map[string]string) error {
-	profile, etag, err := c.GetProfile(profileName)
-	if err != nil {
-		return err
-	}
-
-	if profile.Devices == nil {
-		profile.Devices = map[string]map[string]string{}
-	}
-
+// Return an error if the given profile has already a device with the
+// given name.
+func (cmd *CmdInit) profileDeviceAlreadyExists(profile *api.Profile, deviceName string) error {
 	_, ok := profile.Devices[deviceName]
 	if ok {
 		return fmt.Errorf("Device already exists: %s", deviceName)
 	}
-
-	profile.Devices[deviceName] = deviceConfig
-
-	err = c.UpdateProfile(profileName, profile.Writable(), etag)
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
-func (cmd *CmdInit) setProfileConfigItem(c lxd.ContainerServer, profileName string, key string, value string) error {
-	profile, etag, err := c.GetProfile(profileName)
-	if err != nil {
-		return err
-	}
-
-	if profile.Config == nil {
-		profile.Config = map[string]string{}
-	}
-
-	profile.Config[key] = value
-
-	err = c.UpdateProfile(profileName, profile.Writable(), etag)
-	if err != nil {
-		return err
-	}
-
 	return nil
 }
 
diff --git a/test/main.sh b/test/main.sh
index 2bf6628..d16e870 100755
--- a/test/main.sh
+++ b/test/main.sh
@@ -622,6 +622,7 @@ run_test test_cpu_profiling "CPU profiling"
 run_test test_mem_profiling "memory profiling"
 run_test test_storage "storage"
 run_test test_init_auto "lxd init auto"
+run_test test_init_interactive "lxd init interactive"
 run_test test_init_preseed "lxd init preseed"
 run_test test_storage_profiles "storage profiles"
 run_test test_container_import "container import"
diff --git a/test/suites/init_auto.sh b/test/suites/init_auto.sh
index 2617dcc..993885d 100644
--- a/test/suites/init_auto.sh
+++ b/test/suites/init_auto.sh
@@ -16,6 +16,7 @@ test_init_auto() {
     # shellcheck disable=SC2154
     zpool create "lxdtest-$(basename "${LXD_DIR}")-pool1-existing-pool" "${loop_device_1}" -m none -O compression=on
     LXD_DIR=${LXD_INIT_DIR} lxd init --auto --storage-backend zfs --storage-pool "lxdtest-$(basename "${LXD_DIR}")-pool1-existing-pool"
+    LXD_DIR=${LXD_INIT_DIR} lxc profile show default | grep -q "pool: default"
 
     kill_lxd "${LXD_INIT_DIR}"
     sed -i "\|^${loop_device_1}|d" "${TEST_DIR}/loops"
diff --git a/test/suites/init_interactive.sh b/test/suites/init_interactive.sh
new file mode 100644
index 0000000..400c349
--- /dev/null
+++ b/test/suites/init_interactive.sh
@@ -0,0 +1,37 @@
+test_init_interactive() {
+  # - lxd init
+  LXD_INIT_DIR=$(mktemp -d -p "${TEST_DIR}" XXX)
+  chmod +x "${LXD_INIT_DIR}"
+  spawn_lxd "${LXD_INIT_DIR}" false
+
+  # XXX We need to remove the eth0 device from the default profile, which
+  #     is typically attached by spawn_lxd.
+  if LXD_DIR=${LXD_INIT_DIR} lxc profile show default | grep -q eth0; then
+      LXD_DIR=${LXD_INIT_DIR} lxc network detach-profile lxdbr0 default eth0
+  fi
+
+  LXD_DIR=${LXD_INIT_DIR} lxc profile show default
+  cat <<EOF | LXD_DIR=${LXD_INIT_DIR} lxd init
+yes
+my-storage-pool
+dir
+no
+no
+yes
+lxdt$$
+auto
+none
+EOF
+
+  LXD_DIR=${LXD_INIT_DIR} lxc info | grep -q 'images.auto_update_interval: "0"'
+  LXD_DIR=${LXD_INIT_DIR} lxc network list | grep -q "lxdt$$"
+  LXD_DIR=${LXD_INIT_DIR} lxc storage list | grep -q "my-storage-pool"
+  LXD_DIR=${LXD_INIT_DIR} lxc profile show default | grep -q "pool: my-storage-pool"
+  LXD_DIR=${LXD_INIT_DIR} lxc profile show default | grep -q "parent: lxdt$$"
+  LXD_DIR=${LXD_INIT_DIR} lxc profile delete default
+  LXD_DIR=${LXD_INIT_DIR} lxc network delete lxdt$$
+
+  kill_lxd "${LXD_INIT_DIR}"
+
+  return
+}

From 52bddabf0636e846769829af5aab9d2a0fa543d1 Mon Sep 17 00:00:00 2001
From: Free Ekanayaka <free.ekanayaka at canonical.com>
Date: Fri, 5 May 2017 07:40:02 +0200
Subject: [PATCH 3/4] Extract code asking init questions to individual methods

As a step towards making CmdInit.runAutoOrInteractive() shorter and
easier to split, I mechanically extracted the code that asks for the
various configuration choices into standalone methods.

This makes both the higher-level logic more readable (i.e. it's now
immediate to check which questions get asked and in which order), and
isolates the lower-level logic of individual questions, so it's easier
to see and have confidence of what are the moving parts and variables
involved (without interference between the various questions).

The logic has been moved around without any change, I've just added an
"XXX" comment in the new askStorage() method since there are a couple
of checks which seem redundant and could be dropped.

Signed-off-by: Free Ekanayaka <free.ekanayaka at canonical.com>
---
 lxd/main_init.go | 484 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 275 insertions(+), 209 deletions(-)

diff --git a/lxd/main_init.go b/lxd/main_init.go
index 68ebe78..ce47c27 100644
--- a/lxd/main_init.go
+++ b/lxd/main_init.go
@@ -80,25 +80,12 @@ func (cmd *CmdInit) Run() error {
 // passed to the common run() method.
 func (cmd *CmdInit) runAutoOrInteractive(c lxd.ContainerServer, backendsAvailable []string) error {
 	var defaultPrivileged int // controls whether we set security.privileged=true
-	var storageSetup bool     // == supportedStoragePoolDrivers
-	var storageBackend string // == supportedStoragePoolDrivers
-	var storageLoopSize int64 // Size in GB
-	var storageDevice string  // Path
-	var storagePool string    // pool name
-	var storageDataset string // existing ZFS pool name
-	var networkAddress string // Address
-	var networkPort int64     // Port
-	var trustPassword string  // Trust password
+	var storage *cmdInitStorageParams
+	var networking *cmdInitNetworkingParams
 	var imagesAutoUpdate bool // controls whether we set images.auto_update_interval to 0
-	var bridgeName string     // Bridge name
-	var bridgeIPv4 string     // IPv4 address
-	var bridgeIPv4Nat bool    // IPv4 address
-	var bridgeIPv6 string     // IPv6 address
-	var bridgeIPv6Nat bool    // IPv6 address
+	var bridge *cmdInitBridgeParams
 
-	// Detect userns
 	defaultPrivileged = -1
-	runningInUserns = cmd.RunningInUserns
 	imagesAutoUpdate = true
 
 	pools, err := c.GetStoragePoolNames()
@@ -118,179 +105,32 @@ func (cmd *CmdInit) runAutoOrInteractive(c lxd.ContainerServer, backendsAvailabl
 			return err
 		}
 
-		storageBackend = cmd.Args.StorageBackend
-		storageLoopSize = cmd.Args.StorageCreateLoop
-		storageDevice = cmd.Args.StorageCreateDevice
-		storageDataset = cmd.Args.StorageDataset
-		networkAddress = cmd.Args.NetworkAddress
-		networkPort = cmd.Args.NetworkPort
-		trustPassword = cmd.Args.TrustPassword
-		storagePool = "default"
+		networking = &cmdInitNetworkingParams{
+			Address:       cmd.Args.NetworkAddress,
+			Port:          cmd.Args.NetworkPort,
+			TrustPassword: cmd.Args.TrustPassword,
+		}
 
 		// FIXME: Allow to configure multiple storage pools on auto init
 		// run if explicit arguments to do so are passed.
 		if len(pools) == 0 {
-			storageSetup = true
-		}
-	} else {
-		defaultStorage := "dir"
-		if shared.StringInSlice("zfs", backendsAvailable) {
-			defaultStorage = "zfs"
-		}
-
-		// User chose an already existing storage pool name. Ask him
-		// again if he still wants to create one.
-	askForStorageAgain:
-		storageSetup = cmd.Context.AskBool("Do you want to configure a new storage pool (yes/no) [default=yes]? ", "yes")
-		if storageSetup {
-			storagePool = cmd.Context.AskString("Name of the new storage pool [default=default]: ", "default", nil)
-			if shared.StringInSlice(storagePool, pools) {
-				fmt.Printf("The requested storage pool \"%s\" already exists. Please choose another name.\n", storagePool)
-				// Ask the user again if hew wants to create a
-				// storage pool.
-				goto askForStorageAgain
-			}
-
-			storageBackend = cmd.Context.AskChoice(fmt.Sprintf("Name of the storage backend to use (%s) [default=%s]: ", strings.Join(backendsAvailable, ", "), defaultStorage), supportedStoragePoolDrivers, defaultStorage)
-
-			if !shared.StringInSlice(storageBackend, supportedStoragePoolDrivers) {
-				return fmt.Errorf("The requested backend '%s' isn't supported by lxd init.", storageBackend)
-			}
-
-			if !shared.StringInSlice(storageBackend, backendsAvailable) {
-				return fmt.Errorf("The requested backend '%s' isn't available on your system (missing tools).", storageBackend)
+			storage = &cmdInitStorageParams{
+				Backend:  cmd.Args.StorageBackend,
+				LoopSize: cmd.Args.StorageCreateLoop,
+				Device:   cmd.Args.StorageCreateDevice,
+				Dataset:  cmd.Args.StorageDataset,
+				Pool:     "default",
 			}
 		}
-
-		if storageSetup && storageBackend != "dir" {
-			storageLoopSize = -1
-			q := fmt.Sprintf("Create a new %s pool (yes/no) [default=yes]? ", strings.ToUpper(storageBackend))
-			if cmd.Context.AskBool(q, "yes") {
-				if cmd.Context.AskBool("Would you like to use an existing block device (yes/no) [default=no]? ", "no") {
-					deviceExists := func(path string) error {
-						if !shared.IsBlockdevPath(path) {
-							return fmt.Errorf("'%s' is not a block device", path)
-						}
-						return nil
-					}
-					storageDevice = cmd.Context.AskString("Path to the existing block device: ", "", deviceExists)
-				} else {
-					backingFs, err := filesystemDetect(shared.VarPath())
-					if err == nil && storageBackend == "btrfs" && backingFs == "btrfs" {
-						if cmd.Context.AskBool("Would you like to create a new subvolume for the BTRFS storage pool (yes/no) [default=yes]: ", "yes") {
-							storageDataset = shared.VarPath("storage-pools", storagePool)
-						}
-					} else {
-
-						st := syscall.Statfs_t{}
-						err := syscall.Statfs(shared.VarPath(), &st)
-						if err != nil {
-							return fmt.Errorf("couldn't statfs %s: %s", shared.VarPath(), err)
-						}
-
-						/* choose 15 GB < x < 100GB, where x is 20% of the disk size */
-						def := uint64(st.Frsize) * st.Blocks / (1024 * 1024 * 1024) / 5
-						if def > 100 {
-							def = 100
-						}
-						if def < 15 {
-							def = 15
-						}
-
-						q := fmt.Sprintf("Size in GB of the new loop device (1GB minimum) [default=%dGB]: ", def)
-						storageLoopSize = cmd.Context.AskInt(q, 1, -1, fmt.Sprintf("%d", def))
-					}
-				}
-			} else {
-				q := fmt.Sprintf("Name of the existing %s pool or dataset: ", strings.ToUpper(storageBackend))
-				storageDataset = cmd.Context.AskString(q, "", nil)
-			}
-		}
-
-		// Detect lack of uid/gid
-		needPrivileged := false
-		idmapset, err := shared.DefaultIdmapSet()
-		if err != nil || len(idmapset.Idmap) == 0 || idmapset.Usable() != nil {
-			needPrivileged = true
-		}
-
-		if runningInUserns && needPrivileged {
-			fmt.Printf(`
-We detected that you are running inside an unprivileged container.
-This means that unless you manually configured your host otherwise,
-you will not have enough uid and gid to allocate to your containers.
-
-LXD can re-use your container's own allocation to avoid the problem.
-Doing so makes your nested containers slightly less safe as they could
-in theory attack their parent container and gain more privileges than
-they otherwise would.
-
-`)
-			if cmd.Context.AskBool("Would you like to have your containers share their parent's allocation (yes/no) [default=yes]? ", "yes") {
-				defaultPrivileged = 1
-			} else {
-				defaultPrivileged = 0
-			}
-		}
-
-		if cmd.Context.AskBool("Would you like LXD to be available over the network (yes/no) [default=no]? ", "no") {
-			isIPAddress := func(s string) error {
-				if s != "all" && net.ParseIP(s) == nil {
-					return fmt.Errorf("'%s' is not an IP address", s)
-				}
-				return nil
-			}
-
-			networkAddress = cmd.Context.AskString("Address to bind LXD to (not including port) [default=all]: ", "all", isIPAddress)
-			if networkAddress == "all" {
-				networkAddress = "::"
-			}
-
-			if net.ParseIP(networkAddress).To4() == nil {
-				networkAddress = fmt.Sprintf("[%s]", networkAddress)
-			}
-			networkPort = cmd.Context.AskInt("Port to bind LXD to [default=8443]: ", 1, 65535, "8443")
-			trustPassword = cmd.Context.AskPassword("Trust password for new clients: ", cmd.PasswordReader)
-		}
-
-		if !cmd.Context.AskBool("Would you like stale cached images to be updated automatically (yes/no) [default=yes]? ", "yes") {
-			imagesAutoUpdate = false
-		}
-
-	askForNetworkAgain:
-		bridgeName = ""
-		if cmd.Context.AskBool("Would you like to create a new network bridge (yes/no) [default=yes]? ", "yes") {
-			bridgeName = cmd.Context.AskString("What should the new bridge be called [default=lxdbr0]? ", "lxdbr0", networkValidName)
-			_, _, err := c.GetNetwork(bridgeName)
-			if err == nil {
-				fmt.Printf("The requested network bridge \"%s\" already exists. Please choose another name.\n", bridgeName)
-				// Ask the user again if hew wants to create a
-				// storage pool.
-				goto askForNetworkAgain
-			}
-
-			bridgeIPv4 = cmd.Context.AskString("What IPv4 address should be used (CIDR subnet notation, “auto” or “none”) [default=auto]? ", "auto", func(value string) error {
-				if shared.StringInSlice(value, []string{"auto", "none"}) {
-					return nil
-				}
-				return networkValidAddressCIDRV4(value)
-			})
-
-			if !shared.StringInSlice(bridgeIPv4, []string{"auto", "none"}) {
-				bridgeIPv4Nat = cmd.Context.AskBool("Would you like LXD to NAT IPv4 traffic on your bridge? [default=yes]? ", "yes")
-			}
-
-			bridgeIPv6 = cmd.Context.AskString("What IPv6 address should be used (CIDR subnet notation, “auto” or “none”) [default=auto]? ", "auto", func(value string) error {
-				if shared.StringInSlice(value, []string{"auto", "none"}) {
-					return nil
-				}
-				return networkValidAddressCIDRV6(value)
-			})
-
-			if !shared.StringInSlice(bridgeIPv6, []string{"auto", "none"}) {
-				bridgeIPv6Nat = cmd.Context.AskBool("Would you like LXD to NAT IPv6 traffic on your bridge? [default=yes]? ", "yes")
-			}
+	} else {
+		storage, err = cmd.askStorage(c, pools, backendsAvailable)
+		if err != nil {
+			return err
 		}
+		defaultPrivileged = cmd.askDefaultPrivileged()
+		networking = cmd.askNetworking()
+		imagesAutoUpdate = cmd.askImages()
+		bridge = cmd.askBridge(c)
 	}
 
 	server, _, err := c.GetServer()
@@ -306,7 +146,7 @@ they otherwise would.
 	// device, as well as its privileged mode (see below).
 	defaultProfile, _, getDefaultProfileErr := c.GetProfile("default")
 
-	if storageSetup {
+	if storage != nil {
 		// Unset core.https_address and core.trust_password
 		data.Config["core.https_address"] = ""
 		data.Config["core.trust_password"] = ""
@@ -314,27 +154,27 @@ they otherwise would.
 		// Pool configuration
 		storagePoolConfig := map[string]string{}
 
-		if storageDevice != "" {
-			storagePoolConfig["source"] = storageDevice
-			if storageDataset != "" {
-				storagePool = storageDataset
+		if storage.Device != "" {
+			storagePoolConfig["source"] = storage.Device
+			if storage.Dataset != "" {
+				storage.Pool = storage.Dataset
 			}
-		} else if storageLoopSize != -1 {
-			if storageDataset != "" {
-				storagePool = storageDataset
+		} else if storage.LoopSize != -1 {
+			if storage.Dataset != "" {
+				storage.Pool = storage.Dataset
 			}
 		} else {
-			storagePoolConfig["source"] = storageDataset
+			storagePoolConfig["source"] = storage.Dataset
 		}
 
-		if storageLoopSize > 0 {
-			storagePoolConfig["size"] = strconv.FormatInt(storageLoopSize, 10) + "GB"
+		if storage.LoopSize > 0 {
+			storagePoolConfig["size"] = strconv.FormatInt(storage.LoopSize, 10) + "GB"
 		}
 
 		// Create the requested storage pool.
 		storageStruct := api.StoragePoolsPost{
-			Name:   storagePool,
-			Driver: storageBackend,
+			Name:   storage.Pool,
+			Driver: storage.Backend,
 		}
 		storageStruct.Config = storagePoolConfig
 
@@ -355,8 +195,8 @@ they otherwise would.
 						// the default profile it must be empty otherwise it would
 						// not have been possible to delete the storage pool in
 						// the first place.
-						defaultProfile.Devices[k]["pool"] = storagePool
-						logger.Debugf("Set pool property of existing root disk device \"%s\" in profile \"default\" to \"%s\".", storagePool)
+						defaultProfile.Devices[k]["pool"] = storage.Pool
+						logger.Debugf("Set pool property of existing root disk device \"%s\" in profile \"default\" to \"%s\".", storage.Pool)
 
 						break
 					}
@@ -371,7 +211,7 @@ they otherwise would.
 					defaultProfile.Devices["root"] = map[string]string{
 						"type": "disk",
 						"path": "/",
-						"pool": storagePool,
+						"pool": storage.Pool,
 					}
 				}
 			} else {
@@ -388,10 +228,10 @@ they otherwise would.
 		defaultProfile.Config["security.privileged"] = "true"
 	}
 
-	if networkAddress != "" {
-		data.Config["core.https_address"] = fmt.Sprintf("%s:%d", networkAddress, networkPort)
-		if trustPassword != "" {
-			data.Config["core.trust_password"] = trustPassword
+	if networking != nil {
+		data.Config["core.https_address"] = fmt.Sprintf("%s:%d", networking.Address, networking.Port)
+		if networking.TrustPassword != "" {
+			data.Config["core.trust_password"] = networking.TrustPassword
 		}
 	}
 
@@ -403,21 +243,21 @@ they otherwise would.
 		data.Config["images.auto_update_interval"] = "0"
 	}
 
-	if bridgeName != "" {
+	if bridge != nil {
 		bridgeConfig := map[string]string{}
-		bridgeConfig["ipv4.address"] = bridgeIPv4
-		bridgeConfig["ipv6.address"] = bridgeIPv6
+		bridgeConfig["ipv4.address"] = bridge.IPv4
+		bridgeConfig["ipv6.address"] = bridge.IPv6
 
-		if bridgeIPv4Nat {
+		if bridge.IPv4Nat {
 			bridgeConfig["ipv4.nat"] = "true"
 		}
 
-		if bridgeIPv6Nat {
+		if bridge.IPv6Nat {
 			bridgeConfig["ipv6.nat"] = "true"
 		}
 
 		network := api.NetworksPost{
-			Name: bridgeName}
+			Name: bridge.Name}
 		network.Config = bridgeConfig
 		data.Networks = []api.NetworksPost{network}
 
@@ -433,7 +273,7 @@ they otherwise would.
 		defaultProfile.Devices["eth0"] = map[string]string{
 			"type":    "nic",
 			"nictype": "bridged",
-			"parent":  bridgeName,
+			"parent":  bridge.Name,
 		}
 
 	}
@@ -762,6 +602,204 @@ func (cmd *CmdInit) profileDeviceAlreadyExists(profile *api.Profile, deviceName
 	return nil
 }
 
+// Ask if the user wants to create a new storage pool, and return
+// the relevant parameters if so.
+func (cmd *CmdInit) askStorage(client lxd.ContainerServer, existingPools []string, availableBackends []string) (*cmdInitStorageParams, error) {
+	if !cmd.Context.AskBool("Do you want to configure a new storage pool (yes/no) [default=yes]? ", "yes") {
+		return nil, nil
+	}
+	storage := &cmdInitStorageParams{}
+	defaultStorage := "dir"
+	if shared.StringInSlice("zfs", availableBackends) {
+		defaultStorage = "zfs"
+	}
+	for {
+		storage.Pool = cmd.Context.AskString("Name of the new storage pool [default=default]: ", "default", nil)
+		if shared.StringInSlice(storage.Pool, existingPools) {
+			fmt.Printf("The requested storage pool \"%s\" already exists. Please choose another name.\n", storage.Pool)
+			// Ask the user again if hew wants to create a
+			// storage pool.
+			continue
+		}
+
+		storage.Backend = cmd.Context.AskChoice(fmt.Sprintf("Name of the storage backend to use (%s) [default=%s]: ", strings.Join(availableBackends, ", "), defaultStorage), supportedStoragePoolDrivers, defaultStorage)
+
+		// XXX The following to checks don't make much sense, since
+		// AskChoice will always re-ask the question if the answer
+		// is not among supportedStoragePoolDrivers. It seems legacy
+		// code that we should drop?
+		if !shared.StringInSlice(storage.Backend, supportedStoragePoolDrivers) {
+			return nil, fmt.Errorf("The requested backend '%s' isn't supported by lxd init.", storage.Backend)
+		}
+
+		// XXX Instead of manually checking if the provided choice is
+		// among availableBackends, we could just pass to askChoice the
+		// availableBackends list instead of supportedStoragePoolDrivers.
+		if !shared.StringInSlice(storage.Backend, availableBackends) {
+			return nil, fmt.Errorf("The requested backend '%s' isn't available on your system (missing tools).", storage.Backend)
+		}
+
+		if storage.Backend == "dir" {
+			break
+		}
+
+		storage.LoopSize = -1
+		question := fmt.Sprintf("Create a new %s pool (yes/no) [default=yes]? ", strings.ToUpper(storage.Backend))
+		if cmd.Context.AskBool(question, "yes") {
+			if cmd.Context.AskBool("Would you like to use an existing block device (yes/no) [default=no]? ", "no") {
+				deviceExists := func(path string) error {
+					if !shared.IsBlockdevPath(path) {
+						return fmt.Errorf("'%s' is not a block device", path)
+					}
+					return nil
+				}
+				storage.Device = cmd.Context.AskString("Path to the existing block device: ", "", deviceExists)
+			} else {
+				backingFs, err := filesystemDetect(shared.VarPath())
+				if err == nil && storage.Backend == "btrfs" && backingFs == "btrfs" {
+					if cmd.Context.AskBool("Would you like to create a new subvolume for the BTRFS storage pool (yes/no) [default=yes]: ", "yes") {
+						storage.Dataset = shared.VarPath("storage-pools", storage.Pool)
+					}
+				} else {
+
+					st := syscall.Statfs_t{}
+					err := syscall.Statfs(shared.VarPath(), &st)
+					if err != nil {
+						return nil, fmt.Errorf("couldn't statfs %s: %s", shared.VarPath(), err)
+					}
+
+					/* choose 15 GB < x < 100GB, where x is 20% of the disk size */
+					defaultSize := uint64(st.Frsize) * st.Blocks / (1024 * 1024 * 1024) / 5
+					if defaultSize > 100 {
+						defaultSize = 100
+					}
+					if defaultSize < 15 {
+						defaultSize = 15
+					}
+
+					question := fmt.Sprintf("Size in GB of the new loop device (1GB minimum) [default=%dGB]: ", defaultSize)
+					storage.LoopSize = cmd.Context.AskInt(question, 1, -1, fmt.Sprintf("%d", defaultSize))
+				}
+			}
+		} else {
+			question := fmt.Sprintf("Name of the existing %s pool or dataset: ", strings.ToUpper(storage.Backend))
+			storage.Dataset = cmd.Context.AskString(question, "", nil)
+		}
+		break
+	}
+	return storage, nil
+}
+
+// If we detect that we are running inside an unprivileged container,
+// ask if the user wants to the default profile to be a privileged
+// one.
+func (cmd *CmdInit) askDefaultPrivileged() int {
+	// Detect lack of uid/gid
+	defaultPrivileged := -1
+	needPrivileged := false
+	idmapset, err := shared.DefaultIdmapSet()
+	if err != nil || len(idmapset.Idmap) == 0 || idmapset.Usable() != nil {
+		needPrivileged = true
+	}
+
+	if cmd.RunningInUserns && needPrivileged {
+		fmt.Printf(`
+We detected that you are running inside an unprivileged container.
+This means that unless you manually configured your host otherwise,
+you will not have enough uid and gid to allocate to your containers.
+
+LXD can re-use your container's own allocation to avoid the problem.
+Doing so makes your nested containers slightly less safe as they could
+in theory attack their parent container and gain more privileges than
+they otherwise would.
+
+`)
+
+		if cmd.Context.AskBool("Would you like to have your containers share their parent's allocation (yes/no) [default=yes]? ", "yes") {
+			defaultPrivileged = 1
+		} else {
+			defaultPrivileged = 0
+		}
+	}
+	return defaultPrivileged
+}
+
+// Ask if the user wants to expose LXD over the network, and collect
+// the relevant parameters if so.
+func (cmd *CmdInit) askNetworking() *cmdInitNetworkingParams {
+	if !cmd.Context.AskBool("Would you like LXD to be available over the network (yes/no) [default=no]? ", "no") {
+		return nil
+	}
+	networking := &cmdInitNetworkingParams{}
+
+	isIPAddress := func(s string) error {
+		if s != "all" && net.ParseIP(s) == nil {
+			return fmt.Errorf("'%s' is not an IP address", s)
+		}
+		return nil
+	}
+
+	networking.Address = cmd.Context.AskString("Address to bind LXD to (not including port) [default=all]: ", "all", isIPAddress)
+	if networking.Address == "all" {
+		networking.Address = "::"
+	}
+
+	if net.ParseIP(networking.Address).To4() == nil {
+		networking.Address = fmt.Sprintf("[%s]", networking.Address)
+	}
+	networking.Port = cmd.Context.AskInt("Port to bind LXD to [default=8443]: ", 1, 65535, "8443")
+	networking.TrustPassword = cmd.Context.AskPassword("Trust password for new clients: ", cmd.PasswordReader)
+
+	return networking
+}
+
+// Ask if the user wants images to be automatically refreshed.
+func (cmd *CmdInit) askImages() bool {
+	return cmd.Context.AskBool("Would you like stale cached images to be updated automatically (yes/no) [default=yes]? ", "yes")
+}
+
+// Ask if the user wants to create a new network bridge, and return
+// the relevant parameters if so.
+func (cmd *CmdInit) askBridge(client lxd.ContainerServer) *cmdInitBridgeParams {
+	if !cmd.Context.AskBool("Would you like to create a new network bridge (yes/no) [default=yes]? ", "yes") {
+		return nil
+	}
+	bridge := &cmdInitBridgeParams{}
+	for {
+		bridge.Name = cmd.Context.AskString("What should the new bridge be called [default=lxdbr0]? ", "lxdbr0", networkValidName)
+		_, _, err := client.GetNetwork(bridge.Name)
+		if err == nil {
+			fmt.Printf("The requested network bridge \"%s\" already exists. Please choose another name.\n", bridge.Name)
+			// Ask the user again if hew wants to create a
+			// storage pool.
+			continue
+		}
+		bridge.IPv4 = cmd.Context.AskString("What IPv4 address should be used (CIDR subnet notation, “auto” or “none”) [default=auto]? ", "auto", func(value string) error {
+			if shared.StringInSlice(value, []string{"auto", "none"}) {
+				return nil
+			}
+			return networkValidAddressCIDRV4(value)
+		})
+
+		if !shared.StringInSlice(bridge.IPv4, []string{"auto", "none"}) {
+			bridge.IPv4Nat = cmd.Context.AskBool("Would you like LXD to NAT IPv4 traffic on your bridge? [default=yes]? ", "yes")
+		}
+
+		bridge.IPv6 = cmd.Context.AskString("What IPv6 address should be used (CIDR subnet notation, “auto” or “none”) [default=auto]? ", "auto", func(value string) error {
+			if shared.StringInSlice(value, []string{"auto", "none"}) {
+				return nil
+			}
+			return networkValidAddressCIDRV6(value)
+		})
+
+		if !shared.StringInSlice(bridge.IPv6, []string{"auto", "none"}) {
+			bridge.IPv6Nat = cmd.Context.AskBool("Would you like LXD to NAT IPv6 traffic on your bridge? [default=yes]? ", "yes")
+		}
+		break
+	}
+	return bridge
+}
+
 // Defines the schema for all possible configuration knobs supported by the
 // lxd init command, either directly fed via --preseed or populated by
 // the auto/interactive modes.
@@ -772,6 +810,34 @@ type cmdInitData struct {
 	Profiles      []api.ProfilesPost
 }
 
+// Parameters needed when creating a storage pool in interactive or auto
+// mode.
+type cmdInitStorageParams struct {
+	Backend  string // == supportedStoragePoolDrivers
+	LoopSize int64  // Size in GB
+	Device   string // Path
+	Pool     string // pool name
+	Dataset  string // existing ZFS pool name
+}
+
+// Parameters needed when configuring the LXD server networking options in interactive
+// mode or auto mode.
+type cmdInitNetworkingParams struct {
+	Address       string // Address
+	Port          int64  // Port
+	TrustPassword string // Trust password
+}
+
+// Parameters needed when creating a bridge network device in interactive
+// mode.
+type cmdInitBridgeParams struct {
+	Name    string // Bridge name
+	IPv4    string // IPv4 address
+	IPv4Nat bool   // IPv4 address
+	IPv6    string // IPv6 address
+	IPv6Nat bool   // IPv6 address
+}
+
 // Shortcut for closure/anonymous functions that are meant to revert
 // some change, and that are passed around as parameters.
 type reverter func() error

From 3feddad613aa1ea0bc180f9c51e2da30c085fa42 Mon Sep 17 00:00:00 2001
From: Free Ekanayaka <free.ekanayaka at canonical.com>
Date: Fri, 5 May 2017 08:42:17 +0200
Subject: [PATCH 4/4] Extract logic to fill init data to standalone methods

This is a mechanical refactoring of the logic that fills in the
cmdInitData structure given the configuration parameters passed via
auto or collected in interactive mode.

It makes it a bit easier to both see in a blick what the high-level
logic workflow is and what individual configuration parameters
actually translate to low-level data to send via APIs.

Signed-off-by: Free Ekanayaka <free.ekanayaka at canonical.com>
---
 lxd/main_init.go | 435 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 250 insertions(+), 185 deletions(-)

diff --git a/lxd/main_init.go b/lxd/main_init.go
index ce47c27..0526d1c 100644
--- a/lxd/main_init.go
+++ b/lxd/main_init.go
@@ -41,10 +41,6 @@ type CmdInit struct {
 
 // Run triggers the execution of the init command.
 func (cmd *CmdInit) Run() error {
-	// Figure what storage drivers among the supported ones are actually
-	// available on this system.
-	availableStoragePoolsDrivers := cmd.availableStoragePoolsDrivers()
-
 	// Check that command line arguments don't conflict with each other
 	err := cmd.validateArgs()
 	if err != nil {
@@ -57,184 +53,269 @@ func (cmd *CmdInit) Run() error {
 		return fmt.Errorf("Unable to talk to LXD: %s", err)
 	}
 
-	// Kick off the appropriate run mode (either preseed, auto or interactive).
+	existingPools, err := client.GetStoragePoolNames()
+	if err != nil {
+		// We should consider this fatal since this means
+		// something's wrong with the daemon.
+		return err
+	}
+
+	data := &cmdInitData{}
+
+	// Kick off the appropriate way to fill the data (either
+	// preseed, auto or interactive).
 	if cmd.Args.Preseed {
-		err = cmd.runPreseed(client)
+		err = cmd.fillDataPreseed(data, client)
 	} else {
-		err = cmd.runAutoOrInteractive(client, availableStoragePoolsDrivers)
-	}
+		// Copy the data from the current default profile, if it exists.
+		cmd.fillDataWithCurrentServerConfig(data, client)
 
-	if err == nil {
-		cmd.Context.Output("LXD has been successfully configured.\n")
+		// Copy the data from the current server config.
+		cmd.fillDataWithCurrentDefaultProfile(data, client)
+
+		// Figure what storage drivers among the supported ones are actually
+		// available on this system.
+		backendsAvailable := cmd.availableStoragePoolsDrivers()
+
+		if cmd.Args.Auto {
+			err = cmd.fillDataAuto(data, client, backendsAvailable, existingPools)
+		} else {
+			err = cmd.fillDataInteractive(data, client, backendsAvailable, existingPools)
+		}
+	}
+	if err != nil {
+		return err
 	}
 
-	return err
-}
+	// Apply the desired configuration.
+	err = cmd.apply(client, data)
+	if err != nil {
+		return err
+	}
 
-// Run the logic for auto or interactive mode.
-//
-// XXX: this logic is going to be refactored into two separate runAuto
-// and runInteractive methods, sharing relevant logic with
-// runPreseed. The idea being that both runAuto and runInteractive
-// will end up populating the same low-level cmdInitData structure
-// passed to the common run() method.
-func (cmd *CmdInit) runAutoOrInteractive(c lxd.ContainerServer, backendsAvailable []string) error {
-	var defaultPrivileged int // controls whether we set security.privileged=true
-	var storage *cmdInitStorageParams
-	var networking *cmdInitNetworkingParams
-	var imagesAutoUpdate bool // controls whether we set images.auto_update_interval to 0
-	var bridge *cmdInitBridgeParams
+	cmd.Context.Output("LXD has been successfully configured.\n")
 
-	defaultPrivileged = -1
-	imagesAutoUpdate = true
+	return nil
+}
 
-	pools, err := c.GetStoragePoolNames()
+// Fill the given configuration data with parameters collected from
+// the --auto command line.
+func (cmd *CmdInit) fillDataAuto(data *cmdInitData, client lxd.ContainerServer, backendsAvailable []string, existingPools []string) error {
+	if cmd.Args.StorageBackend == "" {
+		cmd.Args.StorageBackend = "dir"
+	}
+	err := cmd.validateArgsAuto(backendsAvailable)
 	if err != nil {
-		// We should consider this fatal since this means
-		// something's wrong with the daemon.
 		return err
 	}
 
-	if cmd.Args.Auto {
-		if cmd.Args.StorageBackend == "" {
-			cmd.Args.StorageBackend = "dir"
-		}
+	networking := &cmdInitNetworkingParams{
+		Address:       cmd.Args.NetworkAddress,
+		Port:          cmd.Args.NetworkPort,
+		TrustPassword: cmd.Args.TrustPassword,
+	}
+	cmd.fillDataWithNetworking(data, networking)
 
-		err = cmd.validateArgsAuto(backendsAvailable)
+	// FIXME: Allow to configure multiple storage pools on auto init
+	// run if explicit arguments to do so are passed.
+	if len(existingPools) == 0 {
+		storage := &cmdInitStorageParams{
+			Backend:  cmd.Args.StorageBackend,
+			LoopSize: cmd.Args.StorageCreateLoop,
+			Device:   cmd.Args.StorageCreateDevice,
+			Dataset:  cmd.Args.StorageDataset,
+			Pool:     "default",
+		}
+		err = cmd.fillDataWithStorage(data, storage, existingPools)
 		if err != nil {
 			return err
 		}
+	}
+	return nil
+}
 
-		networking = &cmdInitNetworkingParams{
-			Address:       cmd.Args.NetworkAddress,
-			Port:          cmd.Args.NetworkPort,
-			TrustPassword: cmd.Args.TrustPassword,
-		}
+// Fill the given configuration data with parameters collected with
+// interactive questions.
+func (cmd *CmdInit) fillDataInteractive(data *cmdInitData, client lxd.ContainerServer, backendsAvailable []string, existingPools []string) error {
+	storage, err := cmd.askStorage(client, existingPools, backendsAvailable)
+	if err != nil {
+		return err
+	}
+	defaultPrivileged := cmd.askDefaultPrivileged()
+	networking := cmd.askNetworking()
+	imagesAutoUpdate := cmd.askImages()
+	bridge := cmd.askBridge(client)
 
-		// FIXME: Allow to configure multiple storage pools on auto init
-		// run if explicit arguments to do so are passed.
-		if len(pools) == 0 {
-			storage = &cmdInitStorageParams{
-				Backend:  cmd.Args.StorageBackend,
-				LoopSize: cmd.Args.StorageCreateLoop,
-				Device:   cmd.Args.StorageCreateDevice,
-				Dataset:  cmd.Args.StorageDataset,
-				Pool:     "default",
-			}
-		}
-	} else {
-		storage, err = cmd.askStorage(c, pools, backendsAvailable)
-		if err != nil {
-			return err
-		}
-		defaultPrivileged = cmd.askDefaultPrivileged()
-		networking = cmd.askNetworking()
-		imagesAutoUpdate = cmd.askImages()
-		bridge = cmd.askBridge(c)
+	err = cmd.fillDataWithStorage(data, storage, existingPools)
+	if err != nil {
+		return err
 	}
 
-	server, _, err := c.GetServer()
+	err = cmd.fillDataWithDefaultPrivileged(data, defaultPrivileged)
 	if err != nil {
 		return err
 	}
 
-	data := &cmdInitData{}
+	cmd.fillDataWithNetworking(data, networking)
+
+	cmd.fillDataWithImages(data, imagesAutoUpdate)
+
+	err = cmd.fillDataWithBridge(data, bridge)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// Fill the given configuration data from the preseed YAML text stream.
+func (cmd *CmdInit) fillDataPreseed(data *cmdInitData, client lxd.ContainerServer) error {
+	err := cmd.Context.InputYAML(data)
+	if err != nil {
+		return fmt.Errorf("Invalid preseed YAML content")
+	}
+
+	return nil
+}
+
+// Fill the given data with the current server configuration.
+func (cmd *CmdInit) fillDataWithCurrentServerConfig(data *cmdInitData, client lxd.ContainerServer) error {
+	server, _, err := client.GetServer()
+	if err != nil {
+		return err
+	}
 	data.ServerPut = server.Writable()
+	return nil
+}
 
-	// If there's a default profile, and certain conditions are
-	// met we'll update its root disk device and/or eth0 network
-	// device, as well as its privileged mode (see below).
-	defaultProfile, _, getDefaultProfileErr := c.GetProfile("default")
+// Fill the given data with the current default profile, if it exists.
+func (cmd *CmdInit) fillDataWithCurrentDefaultProfile(data *cmdInitData, client lxd.ContainerServer) {
+	defaultProfile, _, err := client.GetProfile("default")
+	if err == nil {
+		// Copy the default profile configuration (that we have
+		// possibly modified above).
+		data.Profiles = []api.ProfilesPost{{Name: "default"}}
+		data.Profiles[0].ProfilePut = defaultProfile.ProfilePut
+	}
+}
 
-	if storage != nil {
-		// Unset core.https_address and core.trust_password
-		data.Config["core.https_address"] = ""
-		data.Config["core.trust_password"] = ""
+// Fill the given init data with a new storage pool structure matching the
+// given storage parameters.
+func (cmd *CmdInit) fillDataWithStorage(data *cmdInitData, storage *cmdInitStorageParams, existingPools []string) error {
 
-		// Pool configuration
-		storagePoolConfig := map[string]string{}
+	if storage == nil {
+		return nil
+	}
 
-		if storage.Device != "" {
-			storagePoolConfig["source"] = storage.Device
-			if storage.Dataset != "" {
-				storage.Pool = storage.Dataset
-			}
-		} else if storage.LoopSize != -1 {
-			if storage.Dataset != "" {
-				storage.Pool = storage.Dataset
-			}
-		} else {
-			storagePoolConfig["source"] = storage.Dataset
-		}
+	// Unset core.https_address and core.trust_password
+	data.Config["core.https_address"] = ""
+	data.Config["core.trust_password"] = ""
 
-		if storage.LoopSize > 0 {
-			storagePoolConfig["size"] = strconv.FormatInt(storage.LoopSize, 10) + "GB"
-		}
+	// Pool configuration
+	storagePoolConfig := map[string]string{}
 
-		// Create the requested storage pool.
-		storageStruct := api.StoragePoolsPost{
-			Name:   storage.Pool,
-			Driver: storage.Backend,
+	if storage.Device != "" {
+		storagePoolConfig["source"] = storage.Device
+		if storage.Dataset != "" {
+			storage.Pool = storage.Dataset
 		}
-		storageStruct.Config = storagePoolConfig
-
-		data.Pools = []api.StoragePoolsPost{storageStruct}
-
-		// When lxd init is rerun and there are already storage pools
-		// configured, do not try to set a root disk device in the
-		// default profile again. Let the user figure this out.
-		if len(pools) == 0 {
-			if getDefaultProfileErr == nil {
-				foundRootDiskDevice := false
-				for k, v := range defaultProfile.Devices {
-					if v["path"] == "/" && v["source"] == "" {
-						foundRootDiskDevice = true
-
-						// Unconditionally overwrite because if the user ends up
-						// with a clean LXD but with a pool property key existing in
-						// the default profile it must be empty otherwise it would
-						// not have been possible to delete the storage pool in
-						// the first place.
-						defaultProfile.Devices[k]["pool"] = storage.Pool
-						logger.Debugf("Set pool property of existing root disk device \"%s\" in profile \"default\" to \"%s\".", storage.Pool)
-
-						break
-					}
+	} else if storage.LoopSize != -1 {
+		if storage.Dataset != "" {
+			storage.Pool = storage.Dataset
+		}
+	} else {
+		storagePoolConfig["source"] = storage.Dataset
+	}
+
+	if storage.LoopSize > 0 {
+		storagePoolConfig["size"] = strconv.FormatInt(storage.LoopSize, 10) + "GB"
+	}
+
+	// Create the requested storage pool.
+	storageStruct := api.StoragePoolsPost{
+		Name:   storage.Pool,
+		Driver: storage.Backend,
+	}
+	storageStruct.Config = storagePoolConfig
+
+	data.Pools = []api.StoragePoolsPost{storageStruct}
+
+	// When lxd init is rerun and there are already storage pools
+	// configured, do not try to set a root disk device in the
+	// default profile again. Let the user figure this out.
+	if len(existingPools) == 0 {
+		if len(data.Profiles) != 0 {
+			defaultProfile := data.Profiles[0]
+			foundRootDiskDevice := false
+			for k, v := range defaultProfile.Devices {
+				if v["path"] == "/" && v["source"] == "" {
+					foundRootDiskDevice = true
+
+					// Unconditionally overwrite because if the user ends up
+					// with a clean LXD but with a pool property key existing in
+					// the default profile it must be empty otherwise it would
+					// not have been possible to delete the storage pool in
+					// the first place.
+					defaultProfile.Devices[k]["pool"] = storage.Pool
+					logger.Debugf("Set pool property of existing root disk device \"%s\" in profile \"default\" to \"%s\".", storage.Pool)
+
+					break
 				}
+			}
 
-				if !foundRootDiskDevice {
-					err = cmd.profileDeviceAlreadyExists(defaultProfile, "root")
-					if err != nil {
-						return err
-					}
+			if !foundRootDiskDevice {
+				err := cmd.profileDeviceAlreadyExists(&defaultProfile, "root")
+				if err != nil {
+					return err
+				}
 
-					defaultProfile.Devices["root"] = map[string]string{
-						"type": "disk",
-						"path": "/",
-						"pool": storage.Pool,
-					}
+				defaultProfile.Devices["root"] = map[string]string{
+					"type": "disk",
+					"path": "/",
+					"pool": storage.Pool,
 				}
-			} else {
-				logger.Warnf("Did not find profile \"default\" so no default storage pool will be set. Manual intervention needed.")
 			}
+		} else {
+			logger.Warnf("Did not find profile \"default\" so no default storage pool will be set. Manual intervention needed.")
 		}
 	}
 
-	if defaultPrivileged != -1 && getDefaultProfileErr != nil {
-		return getDefaultProfileErr
-	} else if defaultPrivileged == 0 {
+	return nil
+}
+
+// Fill the default profile in the given init data with options about whether
+// to run in privileged mode.
+func (cmd *CmdInit) fillDataWithDefaultPrivileged(data *cmdInitData, defaultPrivileged int) error {
+	if defaultPrivileged == -1 {
+		return nil
+	}
+	if len(data.Profiles) == 0 {
+		return fmt.Errorf("error: profile 'default' profile not found")
+	}
+	defaultProfile := data.Profiles[0]
+	if defaultPrivileged == 0 {
 		defaultProfile.Config["security.privileged"] = ""
 	} else if defaultPrivileged == 1 {
 		defaultProfile.Config["security.privileged"] = "true"
 	}
+	return nil
+}
 
-	if networking != nil {
-		data.Config["core.https_address"] = fmt.Sprintf("%s:%d", networking.Address, networking.Port)
-		if networking.TrustPassword != "" {
-			data.Config["core.trust_password"] = networking.TrustPassword
-		}
+// Fill the given init data with server config details matching the
+// given networking parameters.
+func (cmd *CmdInit) fillDataWithNetworking(data *cmdInitData, networking *cmdInitNetworkingParams) {
+	if networking == nil {
+		return
+	}
+	data.Config["core.https_address"] = fmt.Sprintf("%s:%d", networking.Address, networking.Port)
+	if networking.TrustPassword != "" {
+		data.Config["core.trust_password"] = networking.TrustPassword
 	}
+}
 
+// Fill the given init data with server config details matching the
+// given images auto update choice.
+func (cmd *CmdInit) fillDataWithImages(data *cmdInitData, imagesAutoUpdate bool) {
 	if imagesAutoUpdate {
 		if val, ok := data.Config["images.auto_update_interval"]; ok && val == "0" {
 			data.Config["images.auto_update_interval"] = ""
@@ -242,71 +323,55 @@ func (cmd *CmdInit) runAutoOrInteractive(c lxd.ContainerServer, backendsAvailabl
 	} else {
 		data.Config["images.auto_update_interval"] = "0"
 	}
+}
 
-	if bridge != nil {
-		bridgeConfig := map[string]string{}
-		bridgeConfig["ipv4.address"] = bridge.IPv4
-		bridgeConfig["ipv6.address"] = bridge.IPv6
-
-		if bridge.IPv4Nat {
-			bridgeConfig["ipv4.nat"] = "true"
-		}
-
-		if bridge.IPv6Nat {
-			bridgeConfig["ipv6.nat"] = "true"
-		}
-
-		network := api.NetworksPost{
-			Name: bridge.Name}
-		network.Config = bridgeConfig
-		data.Networks = []api.NetworksPost{network}
+// Fill the given init data with a new bridge network device structure
+// matching the given storage parameters.
+func (cmd *CmdInit) fillDataWithBridge(data *cmdInitData, bridge *cmdInitBridgeParams) error {
+	if bridge == nil {
+		return nil
+	}
 
-		if getDefaultProfileErr != nil {
-			return getDefaultProfileErr
-		}
+	bridgeConfig := map[string]string{}
+	bridgeConfig["ipv4.address"] = bridge.IPv4
+	bridgeConfig["ipv6.address"] = bridge.IPv6
 
-		err = cmd.profileDeviceAlreadyExists(defaultProfile, "eth0")
-		if err != nil {
-			return err
-		}
-
-		defaultProfile.Devices["eth0"] = map[string]string{
-			"type":    "nic",
-			"nictype": "bridged",
-			"parent":  bridge.Name,
-		}
+	if bridge.IPv4Nat {
+		bridgeConfig["ipv4.nat"] = "true"
+	}
 
+	if bridge.IPv6Nat {
+		bridgeConfig["ipv6.nat"] = "true"
 	}
 
-	if getDefaultProfileErr == nil {
-		// Copy the default profile configuration (that we have
-		// possibly modified above).
-		data.Profiles = []api.ProfilesPost{{Name: "default"}}
-		data.Profiles[0].ProfilePut = defaultProfile.ProfilePut
+	network := api.NetworksPost{
+		Name: bridge.Name}
+	network.Config = bridgeConfig
+	data.Networks = []api.NetworksPost{network}
+
+	if len(data.Profiles) == 0 {
+		return fmt.Errorf("error: profile 'default' profile not found")
 	}
 
-	err = cmd.run(c, data)
+	// Attach the bridge as eth0 device of the default profile, if such
+	// device doesn't exists yet.
+	defaultProfile := data.Profiles[0]
+	err := cmd.profileDeviceAlreadyExists(&defaultProfile, "eth0")
 	if err != nil {
-		return nil
+		return err
+	}
+	defaultProfile.Devices["eth0"] = map[string]string{
+		"type":    "nic",
+		"nictype": "bridged",
+		"parent":  bridge.Name,
 	}
 
 	return nil
-}
-
-// Run the logic for preseed mode
-func (cmd *CmdInit) runPreseed(client lxd.ContainerServer) error {
-	data := &cmdInitData{}
-
-	err := cmd.Context.InputYAML(data)
-	if err != nil {
-		return fmt.Errorf("Invalid preseed YAML content")
-	}
 
-	return cmd.run(client, data)
 }
 
 // Apply the configuration specified in the given init data.
-func (cmd *CmdInit) run(client lxd.ContainerServer, data *cmdInitData) error {
+func (cmd *CmdInit) apply(client lxd.ContainerServer, data *cmdInitData) error {
 	// Functions that should be invoked to revert back to initial
 	// state any change that was successfully applied, in case
 	// anything goes wrong after that change.
@@ -594,7 +659,7 @@ func (cmd *CmdInit) availableStoragePoolsDrivers() []string {
 
 // Return an error if the given profile has already a device with the
 // given name.
-func (cmd *CmdInit) profileDeviceAlreadyExists(profile *api.Profile, deviceName string) error {
+func (cmd *CmdInit) profileDeviceAlreadyExists(profile *api.ProfilesPost, deviceName string) error {
 	_, ok := profile.Devices[deviceName]
 	if ok {
 		return fmt.Errorf("Device already exists: %s", deviceName)


More information about the lxc-devel mailing list