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

Storage: Fix redundant settings of zfs properties #13218

Merged
merged 2 commits into from
Apr 25, 2024
Merged

Storage: Fix redundant settings of zfs properties #13218

merged 2 commits into from
Apr 25, 2024

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Mar 26, 2024 •
edited
Loading

Fixes #12278 by making It possible for LXD to get ZFS dataset properties before setting them and avoid resetting those that already have the desired value. Performs this operation on ensureInitialDatasets.

@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch from b7ee6ae to 573a8fb Compare March 26, 2024 19:19
@hamistao hamistao mentioned this pull request Mar 26, 2024
Closed
@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch from 573a8fb to 65094ba Compare March 26, 2024 19:21
@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch 7 times, most recently from d54e8aa to f9bd384 Compare April 2, 2024 14:16
@tomponline tomponline changed the title Fix redundant settings of zfs properties Storage: Fix redundant settings of zfs properties Apr 5, 2024
@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch from f9bd384 to 97da373 Compare April 9, 2024 02:14
roosterfish
roosterfish requested changes Apr 9, 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.

I am leaving #13218 (comment) up to you.

lxd/storage/drivers/driver_zfs.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Apr 9, 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.

The existing implementation of ensureInitialDatasets is going a call to d.datasetExists already and altering its call to a setDatasetProperties rather than createDataset call.

The reason for this is so that the dataset policies are enforced when the pool is mounted.

I don't quite see what this PR is aiming to achieve?

@hamistao
Copy link
Contributor Author

hamistao commented Apr 9, 2024

@tomponline this implementation avoids creating duplicate events on zfs by setting dataset properties multiple times unnecessarily. I am still discussing with @roosterfish to agree on the best way to implement this.

@tomponline
Copy link
Member

@tomponline this implementation avoids creating duplicate events on zfs by setting dataset properties multiple times unnecessarily. I am still discussing with @roosterfish to agree on the best way to implement this.

Yes, but it does this by removing the mount time data set policy application, which is undesirable.

@hamistao
Copy link
Contributor Author

hamistao commented Apr 9, 2024

Yes, I agree. This is why I haven't opened this PR for review yet. I will try to find a better solution as soon as possible.

@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch 4 times, most recently from 58c673c to 01aebdf Compare April 18, 2024 03:53
@hamistao
Copy link
Contributor Author

@tomponline @roosterfish What do you think of this approach?

@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch from 01aebdf to 9ade999 Compare April 18, 2024 04:29
@hamistao hamistao marked this pull request as ready for review April 18, 2024 05:40
roosterfish
roosterfish reviewed Apr 18, 2024
View reviewed changes
test/suites/storage_driver_zfs.sh Show resolved Hide resolved
lxd/storage/drivers/driver_zfs_utils.go Outdated Show resolved Hide resolved
lxd/storage/drivers/driver_zfs_utils.go Outdated Show resolved Hide resolved
lxd/storage/drivers/driver_zfs_utils.go Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch 3 times, most recently from 7dbc233 to 4685896 Compare April 18, 2024 15:35
@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch from dcad5e5 to 0a213e0 Compare April 19, 2024 19:14
roosterfish
roosterfish reviewed Apr 22, 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 one more comment from my side :)

lxd/storage/drivers/driver_zfs.go Show resolved Hide resolved
@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch 2 times, most recently from 8bfcd93 to d5cb5b2 Compare April 22, 2024 17:45
roosterfish
roosterfish previously approved these changes Apr 23, 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 this one seems ready.

tomponline
tomponline reviewed Apr 23, 2024
View reviewed changes
lxd/storage/drivers/driver_zfs.go Outdated
@@ -34,7 +34,7 @@ var zfsDefaultSettings = map[string]string{
"setuid": "on",
"exec": "on",
"devices": "on",
"acltype": "posixacl",
"acltype": "posix", // acltype "posix" is equivalent to "posixacl". Set as "posix" to avoid setting it multiple times on ensureInitialDatasets.
Copy link
Member

Choose a reason for hiding this comment

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

Is posix newer than posixacl and if so, when did it get introduced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

posixacl is older but was kept as an alias when posix was added in September 16th, 2020

Copy link
Member

Choose a reason for hiding this comment

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

which version was it added in?

Copy link
Member

Choose a reason for hiding this comment

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

Can you check this doesn't break LXD 5.21/stable with this binary sideloaded on a ZFS 0.8 system (bionic with hwe kernel) please.

Copy link
Contributor Author

@hamistao hamistao Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately it breaks. As can be seen here, support for posix wasn't added until later. I am pushing a new version that solves this problem.

@tomponline
Copy link
Member

@hamistao please can you update the PR description to explain what this PR changes.

@hamistao
Copy link
Contributor Author

@tomponline Done!

tomponline
tomponline requested changes Apr 23, 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.

We need to ensure the initial datasets are created at pool creation time and fail pool creation if not.

@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch from d5cb5b2 to 92fdb3d Compare April 23, 2024 12:32
@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch 3 times, most recently from 611bda3 to 590eec0 Compare April 25, 2024 08:28
@tomponline
Copy link
Member

@hamistao is this ready for review (please do ping me each time you are ready for another review).

@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch 2 times, most recently from 03c0358 to d4cfecd Compare April 25, 2024 08:53
@hamistao
Copy link
Contributor Author

@tomponline yes, it is. I will ping you from now on, sorry for the confusion.

@hamistao
test: Fix setting block.* tests
1742dac 
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
lxd/storage/drivers/zfs: filter redundant options on `ensureInitialDa…
1f506b0 
…tasets`

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao hamistao force-pushed the issue12278/fix_redundant_setting_of_zfs_properties branch from d4cfecd to 1f506b0 Compare April 25, 2024 12:40
@tomponline tomponline merged commit 04e0483 into canonical:main Apr 25, 2024
29 checks passed
@hamistao hamistao deleted the issue12278/fix_redundant_setting_of_zfs_properties 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 left review comments

@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.

Some zfs properties are set multiple times
3 participants
@hamistao @tomponline @roosterfish