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

lxd/storage/drivers/zfs: Fix content type detection for custom block volumes #11991

Merged
merged 2 commits into from
Jul 12, 2023
Merged

lxd/storage/drivers/zfs: Fix content type detection for custom block volumes #11991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f555ce
lxd/storage/drivers/zfs: Fix content type detection for custom block …
monstermunchkin Jul 11, 2023
c4f1d67
test: Test recovering custom block volumes
monstermunchkin Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions lxd/storage/drivers/driver_zfs_volumes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1842,16 +1842,19 @@ func (d *zfs) ListVolumes() ([]Volume, error) {
continue // Ignore unrecognised volume.
}

// Detect if a volume is block content type using both the defined suffix and the dataset type.
isBlock := strings.HasSuffix(volName, zfsBlockVolSuffix) && zfsContentType == "volume"
// Detect if a volume is block content type using only the dataset type.
isBlock := zfsContentType == "volume"
Copy link
Member

Choose a reason for hiding this comment

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

@monstermunchkin did custom block volumes not use the .block suffix ever? or did that change recently?

Copy link
Member

Choose a reason for hiding this comment

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

Does this function support detecting filesystem volumes with zfs.block_mode=true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They never have. That's only for VMs as they have two datasets/volumes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this function support detecting filesystem volumes with zfs.block_mode=true?

No, this function only takes the output of zfs list. zfs.block_mode is a LXD config key and is therefore unknown by this function. There's also no need for this config key as it wouldn't change anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What this function does not support is listing custom ISO volumes. That is because they follow the same naming schema as custom block volumes. We might want to change this in the future, by suffixing ISOs with .iso or something.

Copy link
Member

Choose a reason for hiding this comment

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

No, this function only takes the output of zfs list. zfs.block_mode is a LXD config key and is therefore unknown by this function. There's also no need for this config key as it wouldn't change anything.

But those filesystem block volumes would be unusable after recovery though right?

Copy link
Member

Choose a reason for hiding this comment

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

What this function does not support is listing custom ISO volumes. That is because they follow the same naming schema as custom block volumes. We might want to change this in the future, by suffixing ISOs with .iso or something.

hrm yeah can you open an issue and assign that to you to sort out please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But those filesystem block volumes would be unusable after recovery though right?

Oh, I see what you mean, and you're right. As a workaround, we could do lxc storage volume set zfs vol1 zfs.block_mode=true, right?

Copy link
Member

Choose a reason for hiding this comment

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

I dont think you can do that on a volume once it created - if you can thats a bug.

Copy link
Member

Choose a reason for hiding this comment

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

We probably need to have a content type suffix for those volumes too


if volType == VolumeTypeVM && !isBlock {
continue // Ignore VM filesystem volumes as we will just return the VM's block volume.
}

contentType := ContentTypeFS
if volType == VolumeTypeVM || isBlock {
if isBlock {
contentType = ContentTypeBlock
}

if volType == VolumeTypeVM || isBlock {
volName = strings.TrimSuffix(volName, zfsBlockVolSuffix)
}

Expand All @@ -1863,7 +1866,7 @@ func (d *zfs) ListVolumes() ([]Volume, error) {
if !foundExisting || (existingVol.Type() == VolumeTypeImage && existingVol.ContentType() == ContentTypeFS) {
v := NewVolume(d, d.name, volType, contentType, volName, make(map[string]string), d.config)

if zfsContentType == "volume" {
if isBlock {
v.SetMountFilesystemProbe(true)
}

Expand Down
1 change: 1 addition & 0 deletions test/main.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,7 @@ if [ "${1:-"all"}" != "cluster" ]; then
run_test test_filtering "API filtering"
run_test test_warnings "Warnings"
run_test test_metrics "Metrics"
run_test test_storage_volume_recover "Recover storage volumes"
fi

# shellcheck disable=SC2034
Expand Down
31 changes: 31 additions & 0 deletions test/suites/backup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,34 @@
test_storage_volume_recover() {
LXD_IMPORT_DIR=$(mktemp -d -p "${TEST_DIR}" XXX)
chmod +x "${LXD_IMPORT_DIR}"
spawn_lxd "${LXD_IMPORT_DIR}" true

poolName=$(lxc profile device get default root pool)

# Create custom block volume.
lxc storage volume create "${poolName}" vol1 --type=block

# Delete database entry of the created custom block volume.
lxd sql global "PRAGMA foreign_keys=ON; DELETE FROM storage_volumes WHERE name='vol1'"

# Ensure the custom block volume is no longer listed.
! lxc storage volume show "${poolName}" vol1 || false

# Recover custom block volume.
cat <<EOF | lxd recover
no
yes
yes
EOF

# Ensure custom storage volume has been recovered.
lxc storage volume show "${poolName}" vol1 | grep -q 'content_type: block'

# Cleanup
lxc storage volume delete "${poolName}" vol1
shutdown_lxd "${LXD_DIR}"
}

test_container_recover() {
LXD_IMPORT_DIR=$(mktemp -d -p "${TEST_DIR}" XXX)
chmod +x "${LXD_IMPORT_DIR}"
Expand Down