-
Notifications
You must be signed in to change notification settings - Fork 931
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
Volumes: Add remote copy support for snapshots #12045
Conversation
81f4777
to
7c5dd7f
Compare
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
7c5dd7f
to
45fcba3
Compare
I've pinned curl in main now so you can rebase. |
45fcba3
to
33d521d
Compare
LGTM but code tests are failing. |
deb646b
to
a974583
Compare
b0cc4e3
to
ee5537a
Compare
Thanks, cleared. Also the swagger doc update is now added. |
ee5537a
to
370ed59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, I like the thorough testing added.
There's a couple of minor changes needed, as this introduces an API change so we need to make an api extension and associated commit.
client/lxd_storage_volumes.go
Outdated
Target: volume.Target, | ||
} | ||
|
||
path = fmt.Sprintf("/storage-pools/%s/volumes/custom/%s/snapshots/%s", url.PathEscape(pool), srcVolParentName, srcVolSnapName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use url.PathEscape() on all path parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched to using api.NewURL().Path().String()
to remove the burden of having to call url.PathEscape()
countless times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is preferred way of doing it indeed.
client/lxd_storage_volumes.go
Outdated
path = fmt.Sprintf("/storage-pools/%s/volumes/custom/%s/snapshots/%s", url.PathEscape(pool), srcVolParentName, srcVolSnapName) | ||
} else { | ||
req = volume | ||
path = fmt.Sprintf("/storage-pools/%s/volumes/custom/%s", url.PathEscape(pool), volume.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use url.PathEscape() on all path parameters.
// Initiate volume snapshot migration | ||
// Example: false | ||
// | ||
// API extension: storage_api_remote_volume_handling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isnt the API extension that added this field. Instead it should be the name of the new API extension you add in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking a bit here but the concept of those extensions wasn't clear to me yet. Thanks. Just saw the client now also needs a check for r.HasExtension
. Will add this in now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use r.CheckExtension. Please could you add a comment to r.HasExtension that it is considered deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
|
||
// Migration target (for push mode) | ||
// | ||
// API extension: storage_api_remote_volume_handling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be the new API extension name.
172549f
to
eaeca34
Compare
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
vol.Snapshots() returns an error if the volume is a snapshot. 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>
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
The extension needs to be present when trying to copy volume snapshots between remotes 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>
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
eaeca34
to
1e5e307
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
Adds support for copying volume snapshots to and from a remote.
Includes tests for local and remote volume snapshot copy scenarios.
Fixes #11112