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

Revert to filepath traversal if subvolume list fails #12565

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

markylaing
Copy link
Contributor

#12258 Added an optimisation to the btrfs storage driver to use btrfs subvolume list instead of traversing the file structure when listing subvolumes.

Unfortunately this may fail due to a permission error when using btrfs on btrfs. This commit reverts to using the previous method if btrfs subvolume list fails.

Closes #12555

@markylaing markylaing added the Bug Confirmed to be a bug label Nov 28, 2023
@markylaing markylaing self-assigned this Nov 28, 2023
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 @markylaing do you think it would be practical to add a test for this?


// Attempt to run `btrfs subvolume list`. This may fail inside a nested container, where the container does not have
Copy link
Member

Choose a reason for hiding this comment

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

@markylaing I wonder if it would be better to directly use the old mechanism if d.state.OS.RunningInUserNS is true, as this is what we've done in other places in the code that need to take into account nesting.

Rather than falling back on error and masking the btrfs command error.

@markylaing
Copy link
Contributor Author

thanks @markylaing do you think it would be practical to add a test for this?

I'll give it a go

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.

As discussed lets use d.state.OS.RunningInUserNS instead of falling back

@markylaing
Copy link
Contributor Author

As discussed lets use d.state.OS.RunningInUserNS instead of falling back

I've made this change. I'm finding testing it to be quite tricky, we need a set of utils for getting the LXD build running inside a container. The container itself can't be the busybox test image either. We don't seem to have any tests for nested containers as far as I can see. Shall I create a separate issue for it?

@tomponline
Copy link
Member

As discussed lets use d.state.OS.RunningInUserNS instead of falling back

I've made this change. I'm finding testing it to be quite tricky, we need a set of utils for getting the LXD build running inside a container. The container itself can't be the busybox test image either. We don't seem to have any tests for nested containers as far as I can see. Shall I create a separate issue for it?

I wonder if we should rename https://github.com/canonical/lxd-ci/blob/main/tests/docker to test for both docker and LXD container nesting functionality.

Because this is based on using snap you are then free to install the snap inside the guest and use a full ubuntu container.

See https://github.com/canonical/lxd-ci/blob/main/tests/vm_nesting.sh for something similar but for VMs

@markylaing markylaing force-pushed the btrfs-subvolume-list branch 2 times, most recently from ee8f8b9 to e967d4c Compare November 29, 2023 11:14
…nested container.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@tomponline tomponline merged commit 3293ffa into canonical:main Nov 29, 2023
26 checks passed
@tomponline
Copy link
Member

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed to be a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lxd 5.18/stable nested lxd cannot create containers
2 participants