Skip to content

Volume disk device source-type #14886

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
Merged
tomponline merged 8 commits into from
Feb 4, 2025
Merged

Volume disk device source-type #14886

tomponline merged 8 commits into from
Feb 4, 2025

Conversation

@MggMuggins
Copy link
Contributor

@MggMuggins MggMuggins commented Jan 30, 2025 •
edited
Loading

I'm torn between the volumeType/volumeName and source-type syntaxes for specifying other types of storage volumes in disk devices. A few factors here, some I knew when I wrote my spec, but a few I didn't:

The current syntax for specifying volumes as disk devices is:

devices:
  cosmic-dust-analyzer-data:
	pool: default
	source: custom/cosmic-dust-analyzer-data
	type: disk

This PR uses this style instead:

devices:
  cosmic-dust-analyzer-data:
	pool: default
	source: cosmic-dust-analyzer-data
        source-type: custom
	type: disk

source-type is custom by default so it can be omitted for custom volumes.

@MggMuggins MggMuggins force-pushed the volume-disk-device-sourcetype branch from e45b418 to 5e55eff Compare January 30, 2025 15:11
@MggMuggins MggMuggins marked this pull request as ready for review January 30, 2025 16:02
@MggMuggins MggMuggins force-pushed the volume-disk-device-sourcetype branch from 5e55eff to b56f737 Compare January 30, 2025 23:45
@tomponline
Copy link
Member

tomponline commented Jan 31, 2025

sourcetype: custom

I think source-type would be more readable here.

Would having a source-type of "instance" require a lot of extra DB lookups in order to figure out the instance type btw?

@MggMuggins
Copy link
Contributor Author

MggMuggins commented Feb 3, 2025

I went with sourcetype to be consistent with nictype but I totally agree; will change.

Yes, If the volume type is not derivable from the disk device we'd need to look it up to know which Mount{CustomVolume,CustomVolumeSnapshot,Instance,InstanceSnapshot} function to call. Each of those functions also load the volume from the DB, so you'd end up doing basically the same query twice 🫤

@tomponline
Copy link
Member

tomponline commented Feb 3, 2025

I went with sourcetype to be consistent with nictype but I totally agree; will change.

Yes, If the volume type is not derivable from the disk device we'd need to look it up to know which Mount{CustomVolume,CustomVolumeSnapshot,Instance,InstanceSnapshot} function to call. Each of those functions also load the volume from the DB, so you'd end up doing basically the same query twice 🫤

Looks like we are doing this already anyway?

https://github.com/canonical/lxd/pull/14886/files#diff-30511c7ce3bcabd6a4ddc8624541cc2f5240a13b5e3b19aac8c787fdbd0e4720R1138

@MggMuggins MggMuggins force-pushed the volume-disk-device-sourcetype branch from b56f737 to bd48de3 Compare February 3, 2025 17:38
@MggMuggins
Copy link
Contributor Author

MggMuggins commented Feb 3, 2025 •
edited
Loading

Looks like a second query isn't done for 50% of the times we parse that field. 1 2 3

I think we've discussed this before, but I don't love instance from a consistency point of view (lxc storage volume ls <pool> TYPE column). I'm willing to implement it that way but I think it muddies the waters between the source-type field and the volumeType/volumeName syntax that's still being used in lxc.

I was able to cut out an extra query for each usage by using the already-loaded project.

@github-actions github-actions bot added the Documentation Documentation needs updating label Feb 3, 2025
@MggMuggins MggMuggins force-pushed the volume-disk-device-sourcetype branch 2 times, most recently from 904bd91 to f535991 Compare February 3, 2025 18:04
@tomponline
Copy link
Member

tomponline commented Feb 4, 2025

Looks like a second query isn't done for 50% of the times we parse that field. 1 2 3

I think we've discussed this before, but I don't love instance from a consistency point of view (lxc storage volume ls <pool> TYPE column). I'm willing to implement it that way but I think it muddies the waters between the source-type field and the volumeType/volumeName syntax that's still being used in lxc.

I was able to cut out an extra query for each usage by using the already-loaded project.

OK makes sense, lets use the existing "container" and "virtual-machine" prefixes then. Ta

tomponline
tomponline reviewed Feb 4, 2025
View reviewed changes
tomponline
tomponline reviewed Feb 4, 2025
View reviewed changes
tomponline
tomponline reviewed Feb 4, 2025
View reviewed changes
tomponline
tomponline reviewed Feb 4, 2025
View reviewed changes
tomponline
tomponline requested changes Feb 4, 2025
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.

I think a rebase is required as it looks like you've inadvertently changed a bunch of unrelated sprintf removals back.

@MggMuggins MggMuggins force-pushed the volume-disk-device-sourcetype branch from e36e002 to 036d600 Compare February 4, 2025 14:51
@MggMuggins
lxd/device: Add source-type disk device key
5375b48 
Replaces parsing of the source property as `volumeType/volumeName` with
a separate key `source-type`.

Signed-off-by: Wesley Hershberger <[email protected]>
@MggMuggins
lxd/storage: Replace DiskVolumeSourceParse with source-type key
569c552 
Signed-off-by: Wesley Hershberger <[email protected]>
@MggMuggins
lxc/storage_volume: Use source-type key when add/rm disk devices
08e58de 
Signed-off-by: Wesley Hershberger <[email protected]>
@MggMuggins
lxd: Revert "Preserve custom/c1 syntax in devices during volume ren…
eb1b45f 
…ame"

This reverts commit 7f3eb7b.

Signed-off-by: Wesley Hershberger <[email protected]>
@MggMuggins
test: Revert "Rename should preserve custom/c1 syntax"
20cce89 
This reverts commit 687a275.

Signed-off-by: Wesley Hershberger <[email protected]>
@MggMuggins
test/storage: Revert "custom/volume syntax"
8509453 
This reverts commit af7c79d.

Signed-off-by: Wesley Hershberger <[email protected]>
@MggMuggins
lxd/device: Eliminate uneeded queries
f230989 
Signed-off-by: Wesley Hershberger <[email protected]>
@MggMuggins
doc: Update metadata
63dc1c3 
Signed-off-by: Wesley Hershberger <[email protected]>
@MggMuggins MggMuggins force-pushed the volume-disk-device-sourcetype branch from 036d600 to 63dc1c3 Compare February 4, 2025 14:52
@MggMuggins
Copy link
Contributor Author

MggMuggins commented Feb 4, 2025

Yes, sad rebase. Totally unsure what happened there. Fixed now.

@MggMuggins MggMuggins changed the title Volume disk device sourcetype Volume disk device source-type Feb 4, 2025
tomponline
tomponline approved these changes Feb 4, 2025
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.

LGTM!

@tomponline tomponline merged commit cbe1b51 into canonical:main Feb 4, 2025
26 checks passed
@MggMuggins MggMuggins deleted the volume-disk-device-sourcetype branch February 4, 2025 15:56
@tomponline tomponline mentioned this pull request Feb 5, 2025
Merged
@MggMuggins MggMuggins mentioned this pull request Dec 1, 2025
Closed
7 tasks
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

Documentation Documentation needs updating

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MggMuggins @tomponline