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

userprops: make clearing pool & vdev userprops work the same #16887

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

robn
Copy link
Member

@robn robn commented Dec 19, 2024

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

Someone told me recently that it wasn't possible to clear pool userprops once set. I looked at it today.

I found that setting them to the empty-string actually does set them, that is, their source stays "local". vdev props on the other hand, really do delete the property (remove from the ZAP).

On the other side, if you request a userprop that doesn't exist, pool userprops will show an empty, default-source value, which vdev props simply won't be displayed.

Description

This makes empty/clearing pool and vdev userprops behave the same way:

  • setting a pool userprop to the empty string actually deletes it
  • requesting a vdev userprop that doesn't exist will still display it

How Has This Been Tested?

Test added to make sure these work and do the same thing.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@robn robn force-pushed the clear-pool-vdev-userprops branch from 9d98a92 to 17421a6 Compare December 19, 2024 10:26
@robn robn requested a review from allanjude December 19, 2024 11:09
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

The change looks good to me, assuming no code depends on the current behavior.

Meanwhile the test added uncovered existing problem (at least) on FreeBSD: #16890 . Would be good to test them together.

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Dec 19, 2024
amotin and others added 4 commits December 20, 2024 09:51
VDEV_PROP_USERPROP is equal do VDEV_PROP_INVAL and so is not a real
property.  That's why vdev_prop_readonly() does not work right for
it.  In particular it may declare all vdev user properties readonly
on FreeBSD.

Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
If a vdev userprop is not found, present it as value '-', default
source, so it matches the output from pool userprops.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <rob.norris@klarasystems.com>
People have noted there's no way to remove a pool userprop, only zero
it. Turns vdev userprops had a method, by setting empty-string. So this
makes pool userprops follow the same behaviour.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <rob.norris@klarasystems.com>
Confirming that clearing pool and vdev userprops produce the same
result: an empty value, with default source.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <rob.norris@klarasystems.com>
@robn robn force-pushed the clear-pool-vdev-userprops branch from 17421a6 to d67914e Compare December 19, 2024 22:52
@robn
Copy link
Member Author

robn commented Dec 19, 2024

Last push includes 6965636 (#16890) to see how the tests fall out. If they succeed, that should be merged first.

@amotin
Copy link
Member

amotin commented Dec 20, 2024

Seems to be happy now. The FreeBSD 15 failures seem unrelated and should be fixed by #16892 .

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants