The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/7190
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 updates all calls to Update() to properly set userRequested to true
if the config change comes through a user request.
If it doesn't, then ignore validation issues as reverting will be just
as broken and there's nothing the user can do at that point anyway.
This also removes the logic checking if `volatile.` keys have been
modified since the check never actually applied to PUT/PATCH requests
due to us having to use that API when handling migrations.
So in practice userRequested was never set to true when a change was
actually requested by the user, making the check useless.
Closes #6661
Signed-off-by: Stéphane Graber <stgra...@ubuntu.com>
From b330cdf8ea0d99582cf67fd761ed60833fd4d282 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgra...@ubuntu.com>
Date: Tue, 14 Apr 2020 20:53:29 -0400
Subject: [PATCH] lxd/instances: Better use userRequested on Update
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This updates all calls to Update() to properly set userRequested to true
if the config change comes through a user request.
If it doesn't, then ignore validation issues as reverting will be just
as broken and there's nothing the user can do at that point anyway.
This also removes the logic checking if `volatile.` keys have been
modified since the check never actually applied to PUT/PATCH requests
due to us having to use that API when handling migrations.
So in practice userRequested was never set to true when a change was
actually requested by the user, making the check useless.
Closes #6661
Signed-off-by: Stéphane Graber <stgra...@ubuntu.com>
---
lxd/instance/drivers/driver_lxc.go | 85 ++++++++++++-----------------
lxd/instance/drivers/driver_qemu.go | 66 ++++++++--------------
lxd/instance_patch.go | 2 +-
lxd/instance_put.go | 3 +-
lxd/instance_state.go | 2 +-
lxd/patches.go | 1 +
6 files changed, 63 insertions(+), 96 deletions(-)
diff --git a/lxd/instance/drivers/driver_lxc.go
b/lxd/instance/drivers/driver_lxc.go
index 41baeb283a..38e7265942 100644
--- a/lxd/instance/drivers/driver_lxc.go
+++ b/lxd/instance/drivers/driver_lxc.go
@@ -3289,7 +3289,7 @@ func (c *lxc) Restore(sourceContainer instance.Instance,
stateful bool) error {
// On function return, set the flag back on.
defer func() {
args.Ephemeral = ephemeral
- c.Update(args, true)
+ c.Update(args, false)
}()
}
@@ -3338,6 +3338,7 @@ func (c *lxc) Restore(sourceContainer instance.Instance,
stateful bool) error {
Snapshot: sourceContainer.IsSnapshot(),
}
+ // Don't pass as user-requested as there's no way to fix a bad config.
err = c.Update(args, false)
if err != nil {
logger.Error("Failed restoring container configuration", ctxMap)
@@ -3777,16 +3778,18 @@ func (c *lxc) Update(args db.InstanceArgs,
userRequested bool) error {
args.Profiles = []string{}
}
- // Validate the new config
- err := instance.ValidConfig(c.state.OS, args.Config, false, false)
- if err != nil {
- return errors.Wrap(err, "Invalid config")
- }
+ if userRequested {
+ // Validate the new config
+ err := instance.ValidConfig(c.state.OS, args.Config, false,
false)
+ if err != nil {
+ return errors.Wrap(err, "Invalid config")
+ }
- // Validate the new devices without using expanded devices validation
(expensive checks disabled).
- err = instance.ValidDevices(c.state, c.state.Cluster, c.Type(),
args.Devices, false)
- if err != nil {
- return errors.Wrap(err, "Invalid devices")
+ // Validate the new devices without using expanded devices
validation (expensive checks disabled).
+ err = instance.ValidDevices(c.state, c.state.Cluster, c.Type(),
args.Devices, false)
+ if err != nil {
+ return errors.Wrap(err, "Invalid devices")
+ }
}
// Validate the new profiles
@@ -3816,29 +3819,6 @@ func (c *lxc) Update(args db.InstanceArgs, userRequested
bool) error {
}
}
- // Check that volatile and image keys weren't modified
- if userRequested {
- for k, v := range args.Config {
- if strings.HasPrefix(k, "volatile.") &&
c.localConfig[k] != v {
- return fmt.Errorf("Volatile keys are read-only")
- }
-
- if strings.HasPrefix(k, "image.") && c.localConfig[k]
!= v {
- return fmt.Errorf("Image keys are read-only")
- }
- }
-
- for k, v := range c.localConfig {
- if strings.HasPrefix(k, "volatile.") && args.Config[k]
!= v {
- return fmt.Errorf("Volatile keys are read-only")
- }
-
- if strings.HasPrefix(k, "image.") && args.Config[k] !=
v {
- return fmt.Errorf("Image keys are read-only")
- }
- }
- }
-
// Get a copy of the old configuration
oldDescription := c.Description()
oldArchitecture := 0
@@ -3968,27 +3948,32 @@ func (c *lxc) Update(args db.InstanceArgs,
userRequested bool) error {
return updateFields
})
- // Do some validation of the config diff
- err = instance.ValidConfig(c.state.OS, c.expandedConfig, false, true)
- if err != nil {
- return errors.Wrap(err, "Invalid expanded config")
- }
+ if userRequested {
+ // Do some validation of the config diff
+ err = instance.ValidConfig(c.state.OS, c.expandedConfig, false,
true)
+ if err != nil {
+ return errors.Wrap(err, "Invalid expanded config")
+ }
- // Do full expanded validation of the devices diff.
- err = instance.ValidDevices(c.state, c.state.Cluster, c.Type(),
c.expandedDevices, true)
- if err != nil {
- return errors.Wrap(err, "Invalid expanded devices")
+ // Do full expanded validation of the devices diff.
+ err = instance.ValidDevices(c.state, c.state.Cluster, c.Type(),
c.expandedDevices, true)
+ if err != nil {
+ return errors.Wrap(err, "Invalid expanded devices")
+ }
}
// Run through initLXC to catch anything we missed
- if c.c != nil {
- c.c.Release()
- c.c = nil
- }
- c.cConfig = false
- err = c.initLXC(true)
- if err != nil {
- return errors.Wrap(err, "Initialize LXC")
+ if userRequested {
+ if c.c != nil {
+ c.c.Release()
+ c.c = nil
+ }
+
+ c.cConfig = false
+ err = c.initLXC(true)
+ if err != nil {
+ return errors.Wrap(err, "Initialize LXC")
+ }
}
cg, err := c.cgroup(nil)
diff --git a/lxd/instance/drivers/driver_qemu.go
b/lxd/instance/drivers/driver_qemu.go
index 58835eea4d..0b83a30380 100644
--- a/lxd/instance/drivers/driver_qemu.go
+++ b/lxd/instance/drivers/driver_qemu.go
@@ -2047,7 +2047,7 @@ func (vm *qemu) Restore(source instance.Instance,
stateful bool) error {
// On function return, set the flag back on.
defer func() {
args.Ephemeral = ephemeral
- vm.Update(args, true)
+ vm.Update(args, false)
}()
}
@@ -2087,6 +2087,7 @@ func (vm *qemu) Restore(source instance.Instance,
stateful bool) error {
Snapshot: source.IsSnapshot(),
}
+ // Don't pass as user-requested as there's no way to fix a bad config.
err = vm.Update(args, false)
if err != nil {
logger.Error("Failed restoring instance configuration", ctxMap)
@@ -2328,16 +2329,18 @@ func (vm *qemu) Update(args db.InstanceArgs,
userRequested bool) error {
args.Profiles = []string{}
}
- // Validate the new config.
- err := instance.ValidConfig(vm.state.OS, args.Config, false, false)
- if err != nil {
- return errors.Wrap(err, "Invalid config")
- }
+ if userRequested {
+ // Validate the new config.
+ err := instance.ValidConfig(vm.state.OS, args.Config, false,
false)
+ if err != nil {
+ return errors.Wrap(err, "Invalid config")
+ }
- // Validate the new devices without using expanded devices validation
(expensive checks disabled).
- err = instance.ValidDevices(vm.state, vm.state.Cluster, vm.Type(),
args.Devices, false)
- if err != nil {
- return errors.Wrap(err, "Invalid devices")
+ // Validate the new devices without using expanded devices
validation (expensive checks disabled).
+ err = instance.ValidDevices(vm.state, vm.state.Cluster,
vm.Type(), args.Devices, false)
+ if err != nil {
+ return errors.Wrap(err, "Invalid devices")
+ }
}
// Validate the new profiles.
@@ -2367,29 +2370,6 @@ func (vm *qemu) Update(args db.InstanceArgs,
userRequested bool) error {
}
}
- // Check that volatile and image keys weren't modified.
- if userRequested {
- for k, v := range args.Config {
- if strings.HasPrefix(k, "volatile.") &&
vm.localConfig[k] != v {
- return fmt.Errorf("Volatile keys are read-only")
- }
-
- if strings.HasPrefix(k, "image.") && vm.localConfig[k]
!= v {
- return fmt.Errorf("Image keys are read-only")
- }
- }
-
- for k, v := range vm.localConfig {
- if strings.HasPrefix(k, "volatile.") && args.Config[k]
!= v {
- return fmt.Errorf("Volatile keys are read-only")
- }
-
- if strings.HasPrefix(k, "image.") && args.Config[k] !=
v {
- return fmt.Errorf("Image keys are read-only")
- }
- }
- }
-
// Get a copy of the old configuration.
oldDescription := vm.Description()
oldArchitecture := 0
@@ -2512,16 +2492,18 @@ func (vm *qemu) Update(args db.InstanceArgs,
userRequested bool) error {
return updateFields
})
- // Do some validation of the config diff.
- err = instance.ValidConfig(vm.state.OS, vm.expandedConfig, false, true)
- if err != nil {
- return errors.Wrap(err, "Invalid expanded config")
- }
+ if userRequested {
+ // Do some validation of the config diff.
+ err = instance.ValidConfig(vm.state.OS, vm.expandedConfig,
false, true)
+ if err != nil {
+ return errors.Wrap(err, "Invalid expanded config")
+ }
- // Do full expanded validation of the devices diff.
- err = instance.ValidDevices(vm.state, vm.state.Cluster, vm.Type(),
vm.expandedDevices, true)
- if err != nil {
- return errors.Wrap(err, "Invalid expanded devices")
+ // Do full expanded validation of the devices diff.
+ err = instance.ValidDevices(vm.state, vm.state.Cluster,
vm.Type(), vm.expandedDevices, true)
+ if err != nil {
+ return errors.Wrap(err, "Invalid expanded devices")
+ }
}
// Use the device interface to apply update changes.
diff --git a/lxd/instance_patch.go b/lxd/instance_patch.go
index 13a856f127..af4cacf31a 100644
--- a/lxd/instance_patch.go
+++ b/lxd/instance_patch.go
@@ -140,7 +140,7 @@ func containerPatch(d *Daemon, r *http.Request)
response.Response {
Project: project,
}
- err = c.Update(args, false)
+ err = c.Update(args, true)
if err != nil {
return response.SmartError(err)
}
diff --git a/lxd/instance_put.go b/lxd/instance_put.go
index df8b9a813a..5390756615 100644
--- a/lxd/instance_put.go
+++ b/lxd/instance_put.go
@@ -89,8 +89,7 @@ func containerPut(d *Daemon, r *http.Request)
response.Response {
Project: project,
}
- // FIXME: should set to true when not migrating
- err = c.Update(args, false)
+ err = c.Update(args, true)
if err != nil {
return err
}
diff --git a/lxd/instance_state.go b/lxd/instance_state.go
index e1b55e9754..b0fab52b0b 100644
--- a/lxd/instance_state.go
+++ b/lxd/instance_state.go
@@ -163,7 +163,7 @@ func containerStatePut(d *Daemon, r *http.Request)
response.Response {
// On function return, set the flag back on
defer func() {
args.Ephemeral = ephemeral
- c.Update(args, true)
+ c.Update(args, false)
}()
}
diff --git a/lxd/patches.go b/lxd/patches.go
index 52f26a9e11..64a8663bef 100644
--- a/lxd/patches.go
+++ b/lxd/patches.go
@@ -2038,6 +2038,7 @@ func updatePoolPropertyForAllObjects(d *Daemon, poolName
string, allcontainers [
err = c.Update(args, false)
if err != nil {
+ logger.Warnf("Unable to add pool name to '%s': %v",
c.Name(), err)
continue
}
}
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
