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

lxc: Use volume copy when moving to target project #12521

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

roosterfish
Copy link
Contributor

This fixes a regression introduced in #12348. When the target project flag is set don't use the function for renaming volumes.

Fixes #12518

This fixes a regression introduced in canonical#12348.
When the target project flag is set don't use the function for renaming volumes.

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
MusicDin
MusicDin previously approved these changes Nov 9, 2023
Copy link
Member

@MusicDin MusicDin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see all other tests use "lxdtest-$(basename "${LXD_DIR}")-${driver}1" as a pool name, but could we store that in a variable to make the tests easier to read?

proj="lxdtest-$(basename "${LXD_DIR}")-${driver}"
pool="lxdtest-$(basename "${LXD_DIR}")-${driver}1"

lxc project create "${proj}"
lxc storage colume move "${pool}/vol1" "${pool}/vol1" --project default --target-project "${proj}"
...

Just a suggestion, otherwise LGTM.

@roosterfish
Copy link
Contributor Author

@MusicDin changed to make it more readable.

MusicDin
MusicDin previously approved these changes Nov 14, 2023
@MusicDin
Copy link
Member

@roosterfish thanks, looks much better. Should we change that for an entire test, or is this out of scope? I think it would improving readability of the codebase. What are your thoughts?

@roosterfish
Copy link
Contributor Author

@roosterfish thanks, looks much better. Should we change that for an entire test, or is this out of scope? I think it would improving readability of the codebase. What are your thoughts?

@MusicDin I have now restructured the test suite. Let me know what you think.

@MusicDin
Copy link
Member

Looks good to me. However, do you have any idea why Ceph test is failing?

@roosterfish
Copy link
Contributor Author

Looks good to me. However, do you have any idea why Ceph test is failing?

Not really. I have triggered a rerun to be sure.

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

roosterfish commented Nov 15, 2023

Found the issue, I have replaced a wrong variable which let the test to not skip cephfs in the loop. This is fixed now, all tests are passing.

Copy link
Member

@MusicDin MusicDin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Looks much cleaner now :)

@tomponline tomponline merged commit 2e90731 into canonical:main Nov 15, 2023
26 checks passed
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.

Storage volume cannot be moved between projects
3 participants