Merge pull request #12942 from tomponline/tp-backup-import-devices

Instance: Fix device override issues during import

.github/workflows/tests.yml

@@ -320,6 +320,7 @@ jobs:
           sudo umount /mnt
 
           sudo snap install microceph --edge
+          sudo snap connect microceph:mount-observe
           sudo apt-get install --no-install-recommends -y ceph-common
           sudo microceph cluster bootstrap
           sudo microceph.ceph config set global osd_pool_default_size 1

lxd/api_internal.go

@@ -590,7 +590,8 @@ func internalSQLExec(tx *sql.Tx, query string, result *internalSQLResult) error
 
 // internalImportFromBackup creates instance, storage pool and volume DB records from an instance's backup file.
 // It expects the instance volume to be mounted so that the backup.yaml file is readable.
-func internalImportFromBackup(s *state.State, projectName string, instName string, allowNameOverride bool, devices map[string]map[string]string) error {
+// Also accepts an optional map of device overrides.
+func internalImportFromBackup(s *state.State, projectName string, instName string, allowNameOverride bool, deviceOverrides map[string]map[string]string) error {
 	if instName == "" {
 		return fmt.Errorf("The name of the instance is required")
 	}
@@ -663,28 +664,6 @@ func internalImportFromBackup(s *state.State, projectName string, instName strin
 		return err
 	}
 
-	// Override instance devices.
-	if len(devices) > 0 {
-		if backupConf.Container.Devices == nil {
-			backupConf.Container.Devices = map[string]map[string]string{}
-		}
-
-		for devName, devConfig := range devices {
-			if backupConf.Container.Devices[devName] == nil {
-				backupConf.Container.Devices[devName] = map[string]string{}
-			}
-
-			for k, v := range devConfig {
-				backupConf.Container.Devices[devName][k] = v
-			}
-		}
-
-		err = backup.OverrideConfigYamlFile(backupYamlPath, backupConf)
-		if err != nil {
-			return err
-		}
-	}
-
 	if allowNameOverride && instName != "" {
 		backupConf.Container.Name = instName
 	}
@@ -695,7 +674,7 @@ func internalImportFromBackup(s *state.State, projectName string, instName strin
 
 	if backupConf.Pool == nil {
 		// We don't know what kind of storage type the pool is.
-		return fmt.Errorf(`No storage pool struct in the backup file found. The storage pool needs to be recovered manually`)
+		return fmt.Errorf("No storage pool struct in the backup file found. The storage pool needs to be recovered manually")
 	}
 
 	// Try to retrieve the storage pool the instance supposedly lives on.
@@ -794,7 +773,7 @@ func internalImportFromBackup(s *state.State, projectName string, instName strin
 		return fmt.Errorf("Failed loading profiles for instance: %w", err)
 	}
 
-	// Add root device if needed.
+	// Initialise the devices maps.
 	if backupConf.Container.Devices == nil {
 		backupConf.Container.Devices = make(map[string]map[string]string, 0)
 	}
@@ -803,6 +782,33 @@ func internalImportFromBackup(s *state.State, projectName string, instName strin
 		backupConf.Container.ExpandedDevices = make(map[string]map[string]string, 0)
 	}
 
+	// Apply device overrides.
+	// Do this before calling internalImportRootDevicePopulate so that device overrides are taken into account.
+	for deviceName := range deviceOverrides {
+		_, isLocalDevice := backupConf.Container.Devices[deviceName]
+		if isLocalDevice {
+			// Apply overrides to local device.
+			for k, v := range deviceOverrides[deviceName] {
+				backupConf.Container.Devices[deviceName][k] = v
+			}
+		} else {
+			// Check device exists in expanded profile devices.
+			profileDeviceConfig, found := backupConf.Container.ExpandedDevices[deviceName]
+			if !found {
+				return fmt.Errorf("Cannot override config for device %q: Device not found in profile devices", deviceName)
+			}
+
+			for k, v := range deviceOverrides[deviceName] {
+				profileDeviceConfig[k] = v
+			}
+
+			// Add device to local devices.
+			backupConf.Container.Devices[deviceName] = profileDeviceConfig
+		}
+	}
+
+	// Add root device if needed.
+	// And ensure root device is associated with same pool as instance has been imported to.
 	internalImportRootDevicePopulate(instancePoolName, backupConf.Container.Devices, backupConf.Container.ExpandedDevices, profiles)
 
 	revert := revert.New()
@@ -972,7 +978,7 @@ func internalImportRootDevicePopulate(instancePoolName string, localDevices map[
 	expandedRootName, expandedRootConfig, _ := instancetype.GetRootDiskDevice(expandedDevices)
 
 	// Extract root disk from expanded profile devices.
-	profileExpandedDevices := db.ExpandInstanceDevices(deviceConfig.NewDevices(localDevices), profiles)
+	profileExpandedDevices := instancetype.ExpandInstanceDevices(deviceConfig.NewDevices(localDevices), profiles)
 	profileExpandedRootName, profileExpandedRootConfig, _ := instancetype.GetRootDiskDevice(profileExpandedDevices.CloneNative())
 
 	// Record whether we need to add a new local disk device.

lxd/backup/backup_config_utils.go

@@ -90,33 +90,6 @@ func ParseConfigYamlFile(path string) (*config.Config, error) {
 	return &backupConf, nil
 }
 
-// OverrideConfigYamlFile overrides the YAML file.
-func OverrideConfigYamlFile(path string, backupConf *config.Config) error {
-	f, err := os.Create(path)
-	if err != nil {
-		return err
-	}
-
-	defer f.Close()
-
-	data, err := yaml.Marshal(backupConf)
-	if err != nil {
-		return err
-	}
-
-	err = f.Chmod(0400)
-	if err != nil {
-		return err
-	}
-
-	_, err = f.Write(data)
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
 // updateRootDevicePool updates the root disk device in the supplied list of devices to the pool
 // specified. Returns true if a root disk device has been found and updated otherwise false.
 func updateRootDevicePool(devices map[string]map[string]string, poolName string) bool {

lxd/db/cluster/devices.go

@@ -114,7 +114,7 @@ func NewDeviceType(t string) (DeviceType, error) {
 	case "pci":
 		return TypePCI, nil
 	default:
-		return -1, fmt.Errorf("Invalid device type %s", t)
+		return -1, fmt.Errorf("Invalid device type %q", t)
 	}
 }
 

lxd/db/cluster/instances.go

@@ -101,14 +101,14 @@ func (i *Instance) ToAPI(ctx context.Context, tx *sql.Tx) (*api.Instance, error)
 	}
 
 	apiDevices := DevicesToAPI(devices)
-	expandedDevices := ExpandInstanceDevices(config.NewDevices(apiDevices), apiProfiles)
+	expandedDevices := instancetype.ExpandInstanceDevices(config.NewDevices(apiDevices), apiProfiles)
 
 	config, err := GetInstanceConfig(ctx, tx, i.ID)
 	if err != nil {
 		return nil, err
 	}
 
-	expandedConfig := ExpandInstanceConfig(config, apiProfiles)
+	expandedConfig := instancetype.ExpandInstanceConfig(config, apiProfiles)
 
 	archName, err := osarch.ArchitectureName(i.Architecture)
 	if err != nil {

lxd/db/cluster/profiles.go

@@ -6,7 +6,6 @@ import (
 	"context"
 	"database/sql"
 
-	"github.com/canonical/lxd/lxd/device/config"
 	"github.com/canonical/lxd/shared/api"
 )
 
@@ -99,53 +98,3 @@ func GetProfilesIfEnabled(ctx context.Context, tx *sql.Tx, projectName string, n
 
 	return profiles, nil
 }
-
-// ExpandInstanceConfig expands the given instance config with the config
-// values of the given profiles.
-func ExpandInstanceConfig(config map[string]string, profiles []api.Profile) map[string]string {
-	expandedConfig := map[string]string{}
-
-	// Apply all the profiles
-	profileConfigs := make([]map[string]string, len(profiles))
-	for i, profile := range profiles {
-		profileConfigs[i] = profile.Config
-	}
-
-	for i := range profileConfigs {
-		for k, v := range profileConfigs[i] {
-			expandedConfig[k] = v
-		}
-	}
-
-	// Stick the given config on top
-	for k, v := range config {
-		expandedConfig[k] = v
-	}
-
-	return expandedConfig
-}
-
-// ExpandInstanceDevices expands the given instance devices with the devices
-// defined in the given profiles.
-func ExpandInstanceDevices(devices config.Devices, profiles []api.Profile) config.Devices {
-	expandedDevices := config.Devices{}
-
-	// Apply all the profiles
-	profileDevices := make([]config.Devices, len(profiles))
-	for i, profile := range profiles {
-		profileDevices[i] = config.NewDevices(profile.Devices)
-	}
-
-	for i := range profileDevices {
-		for k, v := range profileDevices[i] {
-			expandedDevices[k] = v
-		}
-	}
-
-	// Stick the given devices on top
-	for k, v := range devices {
-		expandedDevices[k] = v
-	}
-
-	return expandedDevices
-}

lxd/db/instances_test.go

@@ -267,8 +267,8 @@ func TestInstanceList(t *testing.T) {
 
 	err = c.Transaction(context.TODO(), func(ctx context.Context, tx *db.ClusterTx) error {
 		return tx.InstanceList(ctx, func(dbInst db.InstanceArgs, p api.Project) error {
-			dbInst.Config = db.ExpandInstanceConfig(dbInst.Config, dbInst.Profiles)
-			dbInst.Devices = db.ExpandInstanceDevices(dbInst.Devices, dbInst.Profiles)
+			dbInst.Config = instancetype.ExpandInstanceConfig(dbInst.Config, dbInst.Profiles)
+			dbInst.Devices = instancetype.ExpandInstanceDevices(dbInst.Devices, dbInst.Profiles)
 
 			instances = append(instances, dbInst)
 

lxd/db/profiles.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 
 	"github.com/canonical/lxd/lxd/db/cluster"
-	deviceConfig "github.com/canonical/lxd/lxd/device/config"
 	"github.com/canonical/lxd/shared/api"
 )
 
@@ -145,53 +144,3 @@ DELETE FROM profiles_devices_config WHERE profile_device_id NOT IN (SELECT id FR
 
 	return err
 }
-
-// ExpandInstanceConfig expands the given instance config with the config
-// values of the given profiles.
-func ExpandInstanceConfig(config map[string]string, profiles []api.Profile) map[string]string {
-	expandedConfig := map[string]string{}
-
-	// Apply all the profiles
-	profileConfigs := make([]map[string]string, len(profiles))
-	for i, profile := range profiles {
-		profileConfigs[i] = profile.Config
-	}
-
-	for i := range profileConfigs {
-		for k, v := range profileConfigs[i] {
-			expandedConfig[k] = v
-		}
-	}
-
-	// Stick the given config on top
-	for k, v := range config {
-		expandedConfig[k] = v
-	}
-
-	return expandedConfig
-}
-
-// ExpandInstanceDevices expands the given instance devices with the devices
-// defined in the given profiles.
-func ExpandInstanceDevices(devices deviceConfig.Devices, profiles []api.Profile) deviceConfig.Devices {
-	expandedDevices := deviceConfig.Devices{}
-
-	// Apply all the profiles
-	profileDevices := make([]deviceConfig.Devices, len(profiles))
-	for i, profile := range profiles {
-		profileDevices[i] = deviceConfig.NewDevices(profile.Devices)
-	}
-
-	for i := range profileDevices {
-		for k, v := range profileDevices[i] {
-			expandedDevices[k] = v
-		}
-	}
-
-	// Stick the given devices on top
-	for k, v := range devices {
-		expandedDevices[k] = v
-	}
-
-	return expandedDevices
-}

lxd/endpoints/network_util_test.go

@@ -41,25 +41,25 @@ func Test_networkServerErrorLogWriter_shouldDiscard(t *testing.T) {
 		{
 			name:    "ipv4 trusted proxy (read)",
 			proxies: []net.IP{net.ParseIP("10.24.0.32")},
-			log:     []byte("Sep 17 04:58:30 abydos incus.daemon[21884]: 2021/09/17 04:58:30 http: TLS handshake error from 10.24.0.32:55672: read tcp 10.24.0.22:8443->10.24.0.32:55672: read: connection reset by peer\n"),
+			log:     []byte("Sep 17 04:58:30 abydos lxd.daemon[21884]: 2021/09/17 04:58:30 http: TLS handshake error from 10.24.0.32:55672: read tcp 10.24.0.22:8443->10.24.0.32:55672: read: connection reset by peer\n"),
 			want:    "",
 		},
 		{
 			name:    "ipv4 non-trusted proxy (read)",
 			proxies: []net.IP{net.ParseIP("10.24.0.33")},
-			log:     []byte("Sep 17 04:58:30 abydos incus.daemon[21884]: 2021/09/17 04:58:30 http: TLS handshake error from 10.24.0.32:55672: read tcp 10.24.0.22:8443->10.24.0.32:55672: read: connection reset by peer\n"),
+			log:     []byte("Sep 17 04:58:30 abydos lxd.daemon[21884]: 2021/09/17 04:58:30 http: TLS handshake error from 10.24.0.32:55672: read tcp 10.24.0.22:8443->10.24.0.32:55672: read: connection reset by peer\n"),
 			want:    "http: TLS handshake error from 10.24.0.32:55672: read tcp 10.24.0.22:8443->10.24.0.32:55672: read: connection reset by peer",
 		},
 		{
 			name:    "ipv6 trusted proxy (read)",
 			proxies: []net.IP{net.ParseIP("2602:fd23:8:1003:216:3eff:fefa:7670")},
-			log:     []byte("Sep 17 04:58:30 abydos incus.daemon[21884]: 2021/09/17 04:58:30 http: TLS handshake error from [2602:fd23:8:1003:216:3eff:fefa:7670]:55672: read tcp [2602:fd23:8:101::100]:8443->[2602:fd23:8:1003:216:3eff:fefa:7670]:55672: read: connection reset by peer\n"),
+			log:     []byte("Sep 17 04:58:30 abydos lxd.daemon[21884]: 2021/09/17 04:58:30 http: TLS handshake error from [2602:fd23:8:1003:216:3eff:fefa:7670]:55672: read tcp [2602:fd23:8:101::100]:8443->[2602:fd23:8:1003:216:3eff:fefa:7670]:55672: read: connection reset by peer\n"),
 			want:    "",
 		},
 		{
 			name:    "ipv6 non-trusted proxy (read)",
 			proxies: []net.IP{net.ParseIP("2602:fd23:8:1003:216:3eff:fefa:7671")},
-			log:     []byte("Sep 17 04:58:30 abydos incus.daemon[21884]: 2021/09/17 04:58:30 http: TLS handshake error from [2602:fd23:8:1003:216:3eff:fefa:7670]:55672: read tcp [2602:fd23:8:101::100]:8443->[2602:fd23:8:1003:216:3eff:fefa:7670]:55672: read: connection reset by peer\n"),
+			log:     []byte("Sep 17 04:58:30 abydos lxd.daemon[21884]: 2021/09/17 04:58:30 http: TLS handshake error from [2602:fd23:8:1003:216:3eff:fefa:7670]:55672: read tcp [2602:fd23:8:101::100]:8443->[2602:fd23:8:1003:216:3eff:fefa:7670]:55672: read: connection reset by peer\n"),
 			want:    "http: TLS handshake error from [2602:fd23:8:1003:216:3eff:fefa:7670]:55672: read tcp [2602:fd23:8:101::100]:8443->[2602:fd23:8:1003:216:3eff:fefa:7670]:55672: read: connection reset by peer",
 		},
 

lxd/instance/drivers/driver_common.go

@@ -509,8 +509,8 @@ func (d *common) deviceVolatileSetFunc(devName string) func(save map[string]stri
 
 // expandConfig applies the config of each profile in order, followed by the local config.
 func (d *common) expandConfig() error {
-	d.expandedConfig = db.ExpandInstanceConfig(d.localConfig, d.profiles)
-	d.expandedDevices = db.ExpandInstanceDevices(d.localDevices, d.profiles)
+	d.expandedConfig = instancetype.ExpandInstanceConfig(d.localConfig, d.profiles)
+	d.expandedDevices = instancetype.ExpandInstanceDevices(d.localDevices, d.profiles)
 
 	return nil
 }

lxd/instance/drivers/driver_qemu.go

@@ -823,12 +823,16 @@ func (*qemu) fwPath(filename string) string {
 func (d *qemu) killQemuProcess(pid int) error {
 	proc, err := os.FindProcess(pid)
 	if err != nil {
+		if err == os.ErrProcessDone {
+			return nil
+		}
+
 		return err
 	}
 
 	err = proc.Kill()
 	if err != nil {
-		if strings.Contains(err.Error(), "process already finished") {
+		if err == os.ErrProcessDone {
 			return nil
 		}
 

lxd/instance/drivers/driver_qemu_templates.go

@@ -484,8 +484,16 @@ func qemuCPU(opts *qemuCPUOpts, pinning bool) []cfgSection {
 			return nil
 		}
 
+		// Cap the max number of CPUs to 64 unless directly assigned more.
+		max := 64
+		if int(cpu.Total) < max {
+			max = int(cpu.Total)
+		} else if opts.cpuCount > max {
+			max = opts.cpuCount
+		}
+
 		entries = append(entries, cfgEntry{
-			key: "maxcpus", value: fmt.Sprintf("%d", cpu.Total),
+			key: "maxcpus", value: fmt.Sprintf("%d", max),
 		})
 	}