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

Storage: Set the volume UUIDs consistently #12904

Merged
merged 4 commits into from
Feb 20, 2024

Conversation

roosterfish
Copy link
Contributor

@roosterfish roosterfish commented Feb 16, 2024

As a follow up on #12871 and #12840, these changes make the creation of volume UUIDs more consistent by adding a new function GetNewVolume that instantiates new volumes that don't yet exist on the database.
This allows assigning new UUIDs at a single place without requiring to duplicate the UUID generation at several places in the code base.
Additionally this removes the assignment of the UUID from the volume's DB entry creation.

@roosterfish roosterfish force-pushed the backend_use_vol_uuids branch 5 times, most recently from 5fafd82 to d22c9ad Compare February 16, 2024 14:08
@roosterfish roosterfish marked this pull request as ready for review February 16, 2024 14:17
@roosterfish roosterfish force-pushed the backend_use_vol_uuids branch 2 times, most recently from db8085d to eca015a Compare February 16, 2024 16:10
lxd/api_internal.go Outdated Show resolved Hide resolved
lxd/api_internal.go Outdated Show resolved Hide resolved
lxd/instances_post.go Outdated Show resolved Hide resolved
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'm not sure how much of the remaining PR remains relevant after my suggestion earlier.

@tomponline
Copy link
Member

tomponline commented Feb 16, 2024

As these changes are changing very core functionality of the storage subsystem i'd prefer to see a PR per logical isolated change rather than bundling several changes together to help reason about their impacts in isolation.

For example if there are fixes needed for backup restoration that should be a change in its own PR.

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.

Lets chat about this in our 1:1 im getting a bit lost with why these changes are needed.

@tomponline
Copy link
Member

@roosterfish as discussed in our 1:1, lets explore moving the volatile.uuid out of the VolumeDBCreate function and into a new b.GetNewVolume() function so it can be available earlier and make clear that by calling this new function we are modelling a volume that does not yet exist in the storage layer.

@roosterfish roosterfish force-pushed the backend_use_vol_uuids branch 2 times, most recently from c2dd918 to b04d927 Compare February 20, 2024 09:36
@roosterfish roosterfish changed the title Storage: Use the volume UUIDs Storage: Set the volume UUIDs consistently Feb 20, 2024
@roosterfish roosterfish force-pushed the backend_use_vol_uuids branch from b04d927 to 92e0ff3 Compare February 20, 2024 11:01
…eration

When used to instantiate a new volume, it deep copies the given config
and assigns a new UUID for later usage.

Signed-off-by: Julian Pelizäus <[email protected]>
@roosterfish roosterfish force-pushed the backend_use_vol_uuids branch from 92e0ff3 to 21c6ec8 Compare February 20, 2024 12:06
@roosterfish roosterfish marked this pull request as ready for review February 20, 2024 12:12
@roosterfish
Copy link
Contributor Author

@tomponline ready for review.

The backend is now using the new GetNewVolume() which is the only place were we actually assign a new UUID to volumes using = uuid.New().String(). The UUID isn't anymore created when inserting a volume into the DB.

Based on this I was able to get rid of all the other places were we required an exception to delete or set a new UUID.

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! thanks

@tomponline tomponline merged commit 972756d into canonical:main Feb 20, 2024
26 checks passed
@roosterfish roosterfish deleted the backend_use_vol_uuids branch February 20, 2024 13:03
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