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: Use reverter for Ceph RBD volume refresh #13198

Merged
merged 13 commits into from
Mar 25, 2024

Conversation

roosterfish
Copy link
Contributor

This ensures snapshots are cleaned up in case the refresh fails. Otherwise this leads to error messages for subsequent refreshes as some of the snapshots already exist.

@roosterfish roosterfish requested a review from tomponline as a code owner March 22, 2024 09:13
tomponline
tomponline previously approved these changes Mar 22, 2024
@roosterfish
Copy link
Contributor Author

Mhh, I want to check one more thing before we merge.
If the refresh of one of the snap block vols fails, and the snap fs vols are already refreshed, it will only reset the snap fs vols up until the failing snap block vol.

The generic methods probably should return a cleanup to integrate this properly.

@roosterfish roosterfish marked this pull request as draft March 22, 2024 09:51
…Volume

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
This adds a new refreshVolume function which allows returning cleanup hooks.
This is useful as a VMs filesystem volume gets refreshed using the generic approach
whilst block volumes use the optimized way of refreshing RBD backed volumes.

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
…olume

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
…teVolumeFromMigration

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
…umeFromMigration

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
…eFromMigration

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
…eFromMigration

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
…eVolumeFromMigration

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
…eFromMigration

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
@roosterfish roosterfish marked this pull request as ready for review March 22, 2024 17:30
@roosterfish
Copy link
Contributor Author

Ok so this is now ready for review again.

In order to properly cleanup the volumes that get refreshed using the generic approach, the respective functions now return the revert.Hooks so that the actual driver function can cleanup those and the ones of the actual block volume refresh.

I have moved most of the logic into unexported equivalents of the driver functions so that those can be called twice but return the cleanup hooks for each of their invocations. The actual drivers function can now collect the cleanups and react appropriately in case of error.

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.

Thanks!

@tomponline tomponline merged commit 24fb895 into canonical:main Mar 25, 2024
28 checks passed
@roosterfish roosterfish deleted the rbd_refresh_revert branch March 25, 2024 11:36
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