8000
Skip to content

Preserve custom/c1 syntax during storage volume rename#14681

Merged
tomponline merged 2 commits intocanonical:mainfrom
MggMuggins:fix-storage-volume-rename
Dec 18, 2024
Merged

Preserve custom/c1 syntax during storage volume rename#14681
tomponline merged 2 commits intocanonical:mainfrom
MggMuggins:fix-storage-volume-rename

Conversation

@MggMuggins
Copy link
Copy Markdown
Contributor
@MggMuggins MggMuggins commented Dec 17, 2024

Follow-up to #14491; if a volume is specified as source: custom/c1, then a rename of c1->c2 should leave the device with source: custom/c2.

@MggMuggins
lxd: Preserve custom/c1 syntax in devices during volume rename
7f3eb7b 
`newVol.Type` should always be custom

Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
@MggMuggins
test: Rename should preserve custom/c1 syntax
687a275 
Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
@MggMuggins MggMuggins mentioned this pull request Dec 18, 2024
Merged
3 tasks
@tomponline tomponline merged commit 04d12df into canonical:main Dec 18, 2024
@tomponline
Copy link
Copy Markdown
Member
tomponline commented Dec 18, 2024

@MggMuggins TIL LXD does this.

@MggMuggins MggMuggins deleted the fix-storage-volume-rename branch December 18, 2024 14:50
@MggMuggins
Copy link
Copy Markdown
Contributor Author
MggMuggins commented Dec 18, 2024 •
edited
Loading

You and me both 😆

@MggMuggins
Copy link
Copy Markdown
Contributor Author
MggMuggins commented Dec 18, 2024 •
edited
Loading

@tomponline There's no bug when used with instance devices. Renaming devices attached to profiles fails, but with an unhelpful error message.

Instances

Assume the following configuration in a lxd cluster with cluster members c1, c2, and c3:

$ lxc ls
+------+---------+------+------+-----------------+-----------+----------+
| NAME |  STATE  | IPV4 | IPV6 |      TYPE       | SNAPSHOTS | LOCATION |
+------+---------+------+------+-----------------+-----------+----------+
| v1   | STOPPED |      |      | VIRTUAL-MACHINE | 0         | c1       |
+------+---------+------+------+-----------------+-----------+----------+
| v2   | STOPPED |      |      | VIRTUAL-MACHINE | 0         | c2       |
+------+---------+------+------+-----------------+-----------+----------+
$ lxc storage volume ls
+-------+-----------------+--------------------------------------------------+-------------+--------------+---------+----------+
| POOL  |      TYPE       |                       NAME                       | DESCRIPTION | CONTENT-TYPE | USED BY | LOCATION |
+-------+-----------------+--------------------------------------------------+-------------+--------------+---------+----------+
| local | custom          | blk1                                             |             | block        | 1       | c1       |
+-------+-----------------+--------------------------------------------------+-------------+--------------+---------+----------+
| local | custom          | blk1                                             |             | block        | 1       | c2       |
+-------+-----------------+--------------------------------------------------+-------------+--------------+---------+----------+
...
+-------+-----------------+--------------------------------------------------+-------------+--------------+---------+----------+
| local | virtual-machine | v1                                               |             | block        | 1       | c1       |
+-------+-----------------+--------------------------------------------------+-------------+--------------+---------+----------+
| local | virtual-machine | v2                                               |             | block        | 1       | c2       |
+-------+-----------------+--------------------------------------------------+-------------+--------------+---------+----------+
$ lxc config show v1
...
devices:
  blk1:
    pool: local
    source: blk1
    type: disk
ephemeral: false
profiles:
- default
- ...
$ lxc config show v2
...
devices:
  blk1:
    pool: local
    source: blk1
    type: disk
ephemeral: false
profiles:
- default
...

Renaming blk1 on c1 only modifies the disk device on v1:

$ lxc storage volume rename local blk1 blk2 --target c1
Renamed storage volume from "blk1" to "blk2"
$ lxc config show v1
...
devices:
  blk1:
    pool: local
    source: blk2
    type: disk
ephemeral: false
profiles:
- default
...
$ lxc config show v2
...
devices:
  blk1:
    pool: local
    source: blk1
    type: disk
ephemeral: false
profiles:
- default
...

Profiles

The block volume sharing rules aren't strictly correct; adding the same device to the default profile hits this:

$ lxc profile device add default blk1 disk pool=local source=blk1
Config parsing error: Device validation failed for "blk1": Cannot add custom storage block volume to profiles if security.shared is false or unset

Setting that aside for a moment, the rename fails here:

$ lxc storage volume set local blk1 security.shared=true --target c1
$ lxc storage volume set local blk1 security.shared=true --target c2
$ lxc storage volume rename local blk1 blk2 --target c1
Error: Device validation failed for "blk1": Failed loading custom volume: Storage volume not found

I tried a similar setup with filesystem volumes in profiles and get the same thing. I think the current implementation is sound, at least insofar as it validates changes after making them.

Looking at the code I think there's a corner case involving how revert happens (or not) when the rename fails like this, but I need a little more time to work it out properly.

cc @masnax

@MggMuggins MggMuggins mentioned this pull request Dec 20, 2024
Merged
tomponline added a commit that referenced this pull request Dec 20, 2024
@tomponline
Storage: Add reverter to disk device update & test (#14703)
5fbd3a7 
This makes `storagePoolVolumeUpdateUsers` safe to call without using a
reverter.

This also adds a few tests around renaming attached local custom storage
volumes in a clustered context. As we've discussed previously (#14681),
updating disk devices in profiles isn't necessarily sound. These tests
give me a little more confidence that updates behave sanely for local
storage volumes in a cluster.

We should consider eliminating this feature altogether and simply making
it a hard error to rename a custom storage volume while it is referred
to by any disk device.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomponline tomponline tomponline approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MggMuggins @tomponline
0