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.

Sign up for GitHub

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

Jump to bottom

LXC: Implement profile expansion on lxc copy #13118

Merged
merged 7 commits into from
Mar 27, 2024
Merged

LXC: Implement profile expansion on lxc copy #13118

merged 7 commits into from
Mar 27, 2024

Conversation

hamistao
Copy link
Contributor

Fixes #13063

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Mar 12, 2024
@hamistao hamistao force-pushed the issue13063/debug_copying_container_with_device branch from ba691f9 to 6dfabee Compare March 12, 2024 20:22
@github-actions github-actions bot removed Documentation Documentation needs updating API Changes to the REST API labels Mar 12, 2024
@hamistao hamistao force-pushed the issue13063/debug_copying_container_with_device branch from 6dfabee to b708308 Compare March 12, 2024 20:31
@hamistao
Copy link
Contributor Author

hamistao commented Mar 12, 2024 •
edited
Loading

@masnax @tomponline While working on this PR I was discussing with Max the usage of the --device flag on copy, should it indicate device creation as well?
For example, this operation below doesn't work, but maybe it should:

lxc profile device remove myremote:p1 root
lxc copy t1 myremote:t1 -d root,type=disk,pool=default,path=/
Error: Failed instance creation: Can't find a storage pool for the instance to use.

If you think this makes sense we can discuss and if we agree this is worth implementing I can create an issue on the subject and start thinking about how to implement it.

@hamistao hamistao mentioned this pull request Mar 12, 2024
Merged
@tomponline
Copy link
Member

@masnax @tomponline While working on this PR I was discussing with Max the usage of the --device flag on copy, should it indicate device creation as well? For example, this operation below doesn't work, but maybe it should: lxc profile device remove myremote:p1 root lxc copy t1 myremote:t1 -d root,type=disk,pool=default,path=/ Error: Failed instance creation: Can't find a storage pool for the instance to use. If you think whis makes sense we can discuss and if we agree this makes sense I can create an issue on the subject and start thinking about how to implement it.

Suggest for now we keep the functionality aligned with lxc init and lxc import and do not support adding new devices this way.

@hamistao hamistao changed the title LXC: Implement profile extension on lxc copy LXC: Implement profile expansion on lxc copy Mar 13, 2024
@hamistao hamistao force-pushed the issue13063/debug_copying_container_with_device branch from b708308 to fb59e36 Compare March 13, 2024 14:21
tomponline
tomponline requested changes Mar 14, 2024
View reviewed changes
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.

Please can you add some tests to check for regressions. Thanks

@hamistao
Copy link
Contributor Author

Please can you add some tests to check for regressions. Thanks

sorry, I am not sure I understand what you mean by regressions

@tomponline
Copy link
Member

sorry, I am not sure I understand what you mean by regressions

By adding tests for the functionality you're adding you will ensure any future regressions are caught early.

@hamistao hamistao force-pushed the issue13063/debug_copying_container_with_device branch from 8ba4bf2 to 835073e Compare March 14, 2024 19:58
tomponline
tomponline reviewed Mar 15, 2024
View reviewed changes
lxc/copy.go Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the issue13063/debug_copying_container_with_device branch 12 times, most recently from 7f58952 to 704dafc Compare March 18, 2024 19:48
@hamistao hamistao force-pushed the issue13063/debug_copying_container_with_device branch 5 times, most recently from 4a67641 to 50b2d5c Compare March 19, 2024 09:22
tomponline
tomponline reviewed Mar 19, 2024
View reviewed changes
lxc/copy.go Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the issue13063/debug_copying_container_with_device branch from 50b2d5c to 99a0447 Compare March 22, 2024 14:58
@hamistao hamistao marked this pull request as draft March 22, 2024 15:00
@hamistao hamistao force-pushed the issue13063/debug_copying_container_with_device branch from 99a0447 to e249594 Compare March 22, 2024 16:50
roosterfish
roosterfish requested changes Mar 22, 2024
View reviewed changes
shared/util.go Show resolved Hide resolved
shared/util.go Outdated Show resolved Hide resolved
shared/util.go Outdated Show resolved Hide resolved
lxc/utils.go Outdated Show resolved Hide resolved
lxd/api_internal.go Show resolved Hide resolved
lxc/copy.go Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the issue13063/debug_copying_container_with_device branch from f19c3f0 to 8193c82 Compare March 22, 2024 21:30
@hamistao hamistao mentioned this pull request Mar 22, 2024
Merged
@hamistao hamistao marked this pull request as ready for review March 25, 2024 14:26
roosterfish
roosterfish requested changes Mar 25, 2024
View reviewed changes
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Looking good, just a few nits.

lxc/utils.go Show resolved Hide resolved
lxc/init.go Show resolved Hide resolved
lxc/copy.go Show resolved Hide resolved
lxc/copy.go Outdated Show resolved Hide resolved
@roosterfish
Copy link
Contributor

And please make sure to remove the .go suffixes from the commit messages. See my comment from before #13118 (comment).

@hamistao
lxd/shared/util: create function for applying device overrides
c52248c 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxc/utils: create function for getting profile devices
b863bc5 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd/api_internal: eliminate duplicated code
f7b4713 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxc/init: eliminate duplicated code
5f7c9eb 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxc/copy: apply profile expansion on device override
94f37ea 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
test: add test for device overriding on copy
5ef71de 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
i18n: update translations
28163ad 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao hamistao force-pushed the issue13063/debug_copying_container_with_device branch from 8193c82 to 28163ad Compare March 25, 2024 18:20
roosterfish
roosterfish approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

LGTM!

tomponline
tomponline approved these changes Mar 27, 2024
View reviewed changes
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 c83a912 into canonical:main Mar 27, 2024
28 checks passed
@hamistao hamistao deleted the issue13063/debug_copying_container_with_device branch June 6, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roosterfish roosterfish roosterfish approved these changes

@tomponline tomponline tomponline approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lxc copy --device doesn't expand profile devices when overriding config
3 participants
@hamistao @tomponline @roosterfish