lxd: Fix target profile on snapshot copy #15155
Conversation
lxd/instance.go
Outdated
| @@ -397,7 +397,7 @@ func instanceCreateAsCopy(s *state.State, opts instanceCreateAsCopyOpts, op *ope | |||
| Description: srcSnap.Description(), | |||
| Ephemeral: srcSnap.IsEphemeral(), | |||
| Name: newSnapName, | |||
| Profiles: srcSnap.Profiles(), | |||
| Profiles: opts.targetInstance.Profiles, | |||
There was a problem hiding this comment.
Thanks @skozina this indeed looks like the culprit.
I'm slightly concerned that this is going to change the default behaviour where previously the historic profiles were retained within each snapshot, but now we are going to update each copied snapshot to use the same profiles as request by the client (or default if not specified at all as per line 1158 in instances_post.go).
This is a classic example of existing historical behaviour not being well defined, and so the "correct thing" to do is not always clear. Some users may be expecting the existing behaviour, as we aim to not alter snapshots where possible.
The question here I suppose can be more concisely stated as:
Do the
--profileand--no-profilesflags refer to just the main instance or the main instance and its snapshots?
I've run out of time now as have a meeting, but we can discuss more later on.
There was a problem hiding this comment.
Thanks for quick review!
Yes, I'm not sure what's the best way to proceed. I suppose the best "fix" would be to only fix the behavior if some explicit flags are specified by the user. Ie. apply to target instance profiles only if the --no-profiles was specified. I'm not sure if we can detect such case as the arguments are not passed to lxd, and I'm not sure if the "empty profiles list" is a definitive indication if the --no-profiles was specified?
Lets talk later. Thanks!
There was a problem hiding this comment.
Yes, its made more difficult by the fact that the lxc CLI is responsible for retrieving the current instance's profiles and then populating that list for the new instance, so the lxd server doesn't know if that list has been explicitly set by the user or is coming from the source instance.
There was a problem hiding this comment.
A similar issue exists for snapshot config:
https://github.com/canonical/lxd/pull/15155/files#diff-272a0e97fc7cdf4bb7a22457a8bf507889f75f3b6df9689074820b8c1168e4feL393
https://github.com/canonical/lxd/pull/15155/files#diff-272a0e97fc7cdf4bb7a22457a8bf507889f75f3b6df9689074820b8c1168e4feL396
api.InstanceSource (which is used in api.InstancesPost as the Source field when copying/migrating instances) has the InstanceOnly field indicating to copy only the main volume and not snapshots from the source.
I wonder if we could add a field to api.InstanceSource with an API extension to alter how copies/migrations treat snapshot config (profiles, local config and local devices).
There was a problem hiding this comment.
Got another meeting to attend now, but this is where im at currently...
|
converted to draft until we figure out approach. |
github-actions
bot
added
Documentation
skozina
force-pushed
the
ersin-issue15078
branch
2 times, most recently
from
d11069c to
88c2549
Compare
The value is used to indicate if the copied instance snapshots should get the instance profiles
skozina
force-pushed
the
ersin-issue15078
branch
from
88c2549 to
1e5ec17
Compare
Fixes #15078