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.

Sign up for GitHub

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

Jump to bottom

Prevent custom block volume sharing #13183

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Prevent custom block volume sharing #13183

wants to merge 16 commits into from
+322 −70

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Mar 20, 2024 •
edited

Closes #12254.
Changes mostly based on lxc/incus#467 and lxc/incus#529.

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Mar 20, 2024
@hamistao hamistao force-pushed the issue12254/check_custom_block_storage_volume_sharing branch from 0488bfa to 087ec55 Compare March 20, 2024 14:11
@hamistao hamistao marked this pull request as draft March 20, 2024 14:17
@hamistao hamistao force-pushed the issue12254/check_custom_block_storage_volume_sharing branch 8 times, most recently from 28a7e15 to e7b3abb Compare March 21, 2024 11:45
@hamistao hamistao marked this pull request as ready for review March 21, 2024 13:42
@hamistao hamistao force-pushed the issue12254/check_custom_block_storage_volume_sharing branch from e7b3abb to ecf80be Compare March 21, 2024 20:42
@hamistao hamistao mentioned this pull request Mar 22, 2024
Merged
tomponline
tomponline reviewed Mar 27, 2024
View reviewed changes
lxd/device/disk.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Mar 27, 2024
View reviewed changes
lxd/device/disk.go Outdated Show resolved Hide resolved
tomponline
tomponline requested changes Mar 27, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just nits and small tweaks to address.

@github-actions GitHub Actions
Copy link

Heads up @ru-fu - the "Documentation" label was applied to this issue.

@hamistao hamistao force-pushed the issue12254/check_custom_block_storage_volume_sharing branch 5 times, most recently from e1dbfa5 to 1ab84a8 Compare March 28, 2024 10:39
tomponline
tomponline requested changes Mar 28, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you can create block volumes and attach them to profiles in the main test suite we should have a check for that in this PR too.

Also please can you add a check to https://github.com/canonical/lxd-ci/blob/main/tests/storage-volumes-vm for per-VM disk devices.

@hamistao hamistao force-pushed the issue12254/check_custom_block_storage_volume_sharing branch 3 times, most recently from 7400d31 to e8cc754 Compare May 2, 2024 23:03
@hamistao hamistao force-pushed the issue12254/check_custom_block_storage_volume_sharing branch 2 times, most recently from 1e48d69 to 80874f5 Compare June 3, 2024 18:55
roosterfish
roosterfish requested changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few small comments.

What I am wondering is what happens with volumes which are currently attached to multiple instances as soon as you update to "this" version of LXD? Will there be an error in validation at some point if you don't allow security.shared for this volume?

lxd/device/disk.go Outdated Show resolved Hide resolved
lxd/device/disk.go Outdated Show resolved Hide resolved
test/suites/storage_profiles.sh
lxc profile device add volumeSharingTest test-disk disk pool="lxdtest-$(basename "${LXD_DIR}")-pool1" source=block-vol

# Try to disable security.shared for a volume already added to a profile. That operation must fail.
! lxc storage volume set "lxdtest-$(basename "${LXD_DIR}")-pool1" block-vol security.shared false || false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be another check of this kind if the volume is directly attached to an instance? In the commits before I saw you have added code for this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I added these these on lxd-ci because block volumes can only be attached to VMs and security.shared is exclusive to block volumes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was if there is a test checking if security.shared cannot be unset if the volume is already attached to a profile, shouldn't there also be a test checking if security.shared cannot be unset if the volume is attached to multiple VMs?

I am talking about this part of the code https://github.com/canonical/lxd/pull/13183/files#diff-029fdd0e1afdacb0e66630c0d7e813772122db9a7d036060902838db315d9c4dR5625

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the check you are referring to. This is part of canonical/lxd-ci#146. Please let me know if I am understanfing something wrong

@hamistao hamistao force-pushed the issue12254/check_custom_block_storage_volume_sharing branch from 80874f5 to 74fa839 Compare June 4, 2024 14:27
@hamistao
Copy link
Contributor Author

hamistao commented Jun 4, 2024

@roosterfish Any volumes that were being shared before the update won't be affected until the one of the instances is started or the device is updated, at which point you could just set security.shared to true and then proceed. Also this is ready again in case you want to take another look.

@tomponline
Copy link
Member

What I am wondering is what happens with volumes which are currently attached to multiple instances as soon as you update to "this" version of LXD? Will there be an error in validation at some point if you don't allow security.shared for this volume?

@roosterfish Any volumes that were being shared before the update won't be affected until the one of the instances is started or the device is updated, at which point you could just set security.shared to true and then proceed. Also this is ready again in case you want to take another look.

This is a problem because if you restart your system after snap has updated to this version you'll end up with potentially unstartable VMs. Which will then require manual intervention and debugging to figure out what has changed.

That isn't good UX in my view.

We should have a DB patch that adds security.shared=true to appropriate existing disk devices so they continue to operate as they did previously (allowing shared usage).

What do you think?

@tomponline tomponline marked this pull request as draft June 11, 2024 08:52
@hamistao
Copy link
Contributor Author

hamistao commented Jun 14, 2024 •
edited

@tomponline I agree that this isn't good UX but I also think it is risky to set it for the user since the purpose of setting security.shared is stating that the user is aware of the risk of data loss on those shared volumes.
Maybe a good approach would be having the DB patch but somehow also showing a warning on update alerting for the risk of data loss on those volumes. What do you think about this?

@hamistao hamistao force-pushed the issue12254/check_custom_block_storage_volume_sharing branch from 74fa839 to 6884431 Compare June 15, 2024 06:19
monstermunchkin and others added 16 commits June 15, 2024 04:11
@monstermunchkin @hamistao
api: Add shared_custom_block_volumes API extension
885e503 
Signed-off-by: Thomas Hipp <thomashipp@gmail.com>
(cherry picked from commit b8d3d735dffe6c587ed0b8965288122f63dfc108)
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
License: Apache-2.0
@monstermunchkin @hamistao
lxd/device/config: Fix return values of Update function
002382b 
The `Update` function returns `map[string]Device` values despite there
being a `Devices` type which is the same thing.

This commit changes the return types to be `Devices` instead.

Signed-off-by: Thomas Hipp <thomashipp@gmail.com>
(cherry picked from commit 6fff607f8462237843a590751d7afcb38335e3f4)
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
License: Apache-2.0
@hamistao
lxd: Rename ErrInstanceListStop to ErrListStop
f3da248 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@monstermunchkin @hamistao
lxd/device: Validate shared block devices
5ab741e 
This validates shared custom block volumes. Such volumes can be attached
to profiles only if `security.shared` is `true`. Also, they can only be
attached to more than one instance if `security.shared` is `true`.

Signed-off-by: Thomas Hipp <thomashipp@gmail.com>
(cherry picked from commit 78315b5d6d8b1783604b482dec5c210767597098)
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
License: Apache-2.0
@monstermunchkin @hamistao
lxd/storage: Handle security.shared update
372a72d 
This handles the `security.shared` config key update. If a shared custom
storage block volume is attached to a profile, this volume cannot be
un-shared. Furthermore, if such a volume is attached to more than one
instance, it also cannot be un-shared.

Signed-off-by: Thomas Hipp <thomashipp@gmail.com>
(cherry picked from commit 29b45bb029d3a447e80a3e6f20e19658183ffb36)
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
License: Apache-2.0
@monstermunchkin @hamistao
lxd/storage: Allow security.shared key for custom block volumes
7dec08e 
Signed-off-by: Thomas Hipp <thomashipp@gmail.com>
(cherry picked from commit 6e4e6b6b2061ad719ec576ee143e20f1b208b411)
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
License: Apache-2.0
@monstermunchkin @hamistao
lxd/storage/drivers: Handle security.shared in fillVolumeConfig
35fd9ad 
Signed-off-by: Thomas Hipp <thomashipp@gmail.com>
(cherry picked from commit 606b833d1a67a3d8bc3e7cec032889c372229242)
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
License: Apache-2.0
@stgraber @hamistao
lxd/device/disk: Fix incorrect block volume usage
986c098 
Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
(cherry picked from commit 32a4beecbf8098fdbb15ef5f36088956922630f7)
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
License: Apache-2.0
@hamistao
lxd/device: Update confusing error message
0c79a3d 
This check is not for when the path property is missing, but to check if
a device that is not a root disk (and therefore its path is different
from '/') has a source property defined

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd/device: Include path definition checks when validating profile
9925f62 
This performs checks for mount path definition on devices that are being
added to profiles. This will also make it easier to simplify
`validateConfig`.

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd/device: Create checkBlockVolSharing
c86d225 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd/device: Simplify conditions on validateConfig
49f2130 
`validateConfig` was unnecessarily complicated, this commit proposes a
rearrangement of the conditions at play to make the function simpler and
more readable without changing its behavior. The biggest change was to
port (most) non-profile checks to a separate if and then avoid code duplication on creating `dbVolume`.

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd/storage: Add comments for metadata generation
0b3cb1b 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
metadata: Add references to security.shared
3ccb98f 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
test: Add test for adding block volumes to profile
22fabe7 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
doc: Update block volume usage
6f5707b 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao hamistao force-pushed the issue12254/check_custom_block_storage_volume_sharing branch from 6884431 to 6f5707b Compare June 15, 2024 07:11
@tomponline
Copy link
Member

tomponline commented Jun 17, 2024 •
edited

@tomponline I agree that this isn't good UX but I also think it is risky to set it for the user since the purpose of setting security.shared is stating that the user is aware of the risk of data loss on those shared volumes. Maybe a good approach would be having the DB patch but somehow also showing a warning on update alerting for the risk of data loss on those volumes. What do you think about this?

Please could you update the PR with a description of the behaviour change in validation ready for review.

Will existing VMs only be not startable if they are have a volume that is attached to multiple instances?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline requested changes

@roosterfish roosterfish roosterfish requested changes

@ru-fu ru-fu ru-fu left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prevent custom storage volumes to be used more than once
6 participants
@hamistao @tomponline @roosterfish @ru-fu @monstermunchkin @stgraber