Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve custom/c1 syntax during storage volume rename #14681

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

MggMuggins
Copy link
Contributor

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.

`newVol.Type` should always be custom

Signed-off-by: Wesley Hershberger <[email protected]>
@tomponline tomponline merged commit 04d12df into canonical:main Dec 18, 2024
27 checks passed
@tomponline
Copy link
Member

@MggMuggins TIL LXD does this.

@MggMuggins MggMuggins deleted the fix-storage-volume-rename branch December 18, 2024 14:50
@MggMuggins
Copy link
Contributor Author

MggMuggins commented Dec 18, 2024

You and me both 😆

@MggMuggins
Copy link
Contributor Author

MggMuggins commented Dec 18, 2024

@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

tomponline added a commit that referenced this pull request Dec 20, 2024
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants