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

zpool.present: correctly handle "feature@" properties #62448

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

asomers
Copy link
Contributor

@asomers asomers commented Aug 9, 2022

Optional ZFS pool features can have three states: disabled, enabled, and
active. Enabled means that ZFS will use them if it needs them, but they
haven't changed the on-disk format yet, so they can still be switched
off. But active features have already changed the on-disk format, so
they can't be switched off. Disabled features may not be used.

When enabling such a feature via the zpool.present state, treat "active"
as identical to "enabled".

Fixes #62390

What does this PR do?

Treats zpool properties named "feature@XXX" as being "enabled" if they are really "active" during the zpool.present state.

What issues does this PR fix or reference?

Fixes: 62390

Previous Behavior

If a zpool.present state tried to set a feature to enabled, but that feature was already active on the pool, then Salt would redundantly execute zpool set again, to no effect.

New Behavior

Such a state won't result in any action being taken.

Merge requirements satisfied?

Commits signed with GPG?

Yes

@asomers asomers requested a review from a team as a code owner August 9, 2022 20:52
@asomers asomers requested review from twangboy and removed request for a team August 9, 2022 20:52
twangboy
twangboy previously approved these changes Oct 11, 2022
Optional ZFS pool features can have three states: disabled, enabled, and
active.  Enabled means that ZFS will use them if it needs them, but they
haven't changed the on-disk format yet, so they can still be switched
off.  But active features have already changed the on-disk format, so
they can't be switched off.  Disabled features may not be used.

When enabling such a feature via the zpool.present state, treat "active"
as identical to "enabled".

Fixes saltstack#62390
@Ch3LL
Copy link
Contributor

Ch3LL commented Oct 12, 2022

Can you help my understanding with this fix:

  • If you want the status to be active wouldn't you just set active in the state instead of "enabled"?
  • I'm worried this would change expected behavior because it is changing the status from what the user set in the state.

@asomers
Copy link
Contributor Author

asomers commented Oct 12, 2022

Can you help my understanding with this fix:

* If you want the status to be active wouldn't you just set `active` in the state instead of "enabled"?

No. "enabled" means that the pool feature is enabled but ZFS isn't using it yet. An "enabled" feature can be reset back to "disabled" if you want to. But an "active" feature means that ZFS is already using it, so you cannot disable it. For example, you might set the feature@edonr feature to "enabled". But it won't become active until you actually set checksum=edonr for some dataset. It isn't possible to explicitly set the feature to "active" with a command like zpool set feature@edonr=active tank.

* I'm worried this would change expected behavior because it is changing the status from what the user set in the state.

The original behavior was clearly a bug, because Salt would try to enable an active feature over and over, on every state application, to no effect.

@Ch3LL
Copy link
Contributor

Ch3LL commented Oct 12, 2022

Thanks for explaining. Makes sense now

@Ch3LL Ch3LL merged commit a808930 into saltstack:master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] zpool.present state tries to set "active" zpool properties to "enabled"
3 participants