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: Rework volume volatile.uuid patch #13162

Merged
merged 3 commits into from
Mar 15, 2024

Conversation

roosterfish
Copy link
Contributor

This reworks the patch added in #12840 so that it always applies the new volatile.uuid in any case without needing to rely on the LXD clusters leader election.
I tried to reuse the same select query for all three cases, not sure if it's worth adding this complexity but it looks cleaner in my opinion.

Additionally I have fixed a bug in which new image volumes don't get a new volatile.uuid. This was missed whilst introducing the new GetNewVolume() function which now also adds a volatile.uuid to the volume's config.

lxd/patches.go Outdated Show resolved Hide resolved
@roosterfish
Copy link
Contributor Author

I just saw I could also make the INSERT as a function so that it can be called three times instead of having three separate queries. I'll change this.

This patch supersedes patchStorageSetVolumeUUID as it isn't affected by the leader election.

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
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.

Should we do all of this inside a single transaction?

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.

but this is looking a lot cleaner

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
@roosterfish
Copy link
Contributor Author

Should we do all of this inside a single transaction?

Absolutely, I haven't really thought about using one. Would you see any impact by not using a transaction?

@tomponline
Copy link
Member

Should we do all of this inside a single transaction?

Absolutely, I haven't really thought about using one. Would you see any impact by not using a transaction?

If one fails it'll fallback. But im not sure we want this in this case. Happy to leave as is.

@tomponline tomponline merged commit 822a4d4 into canonical:main Mar 15, 2024
27 checks passed
@roosterfish roosterfish deleted the rework_uuid_patch branch March 15, 2024 13:59
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