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

Document grub2 compatibility updates and test for all feature sets #15505

Closed
wants to merge 2 commits into from

Conversation

usaleem-ix
Copy link
Contributor

@usaleem-ix usaleem-ix commented Nov 8, 2023

Motivation and Context

grub2 compatibility list has been updated. zpool-features.7 should be updated accordingly.

Adding a new feature to compatibility list also enables all its dependencies. It may potentially
enable any feature which is not present in compatibility list. We should test for all know feature
sets and make sure only those features are enabled that are present in compatibility list.

Description

zpool-features.7 is updated to reflect updates in grub2 compatibility list.

zpool_create_features_007.ksh tests for compat-2020 feature set, that it only enables the
features that are listed in compat-2020. zpool_create_features_007.ksh is updated to test
for all known feature sets.

There was a fallout after updating the test, openzfsonosx-1.8.1 lists encryption to be enabled,
encryption depends on bookmark_v2 feature which is also enabled, but not listed in
openzfsonosx-1.8.1 list. So, encryption has been removed from openzfsonosx-1.8.1.

How Has This Been Tested?

A successful ZTS run after the updates.

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:

This commit updates zpool-features.7 man page to add newly added
zpool features to grub2 compatibility list.

Signed-off-by: Umer Saleem <usaleem@ixsystems.com>
@rincebrain
Copy link
Contributor

That change might be incorrect.

bookmark_v2 was added as a dependency after yet another way encryption was broken was found, so it's possible that OpenZFS on OSX 1.8.1 doesn't support bookmark_v2, and consequently, trying to use that compatibility mode to enable that would break using that version, and the more correct choice would be to just drop encryption as an allowed feature there.

@usaleem-ix
Copy link
Contributor Author

Thank you @rincebrain, I was not exactly sure if adding bookmark_v2 is the right thing to do here.

@lundman would you please suggest if removing encryption from the feature list for OpenZFS on OSX 1.8.1 would be alright?

@rincebrain
Copy link
Contributor

rincebrain commented Nov 8, 2023

This might be also bad historically even more than there - for example, 0.8 got bookmark_v2 in 0.8.4 (if I recall), so do we want the compatibility setting to not work except for 0.8.4-6?

Hm, I'm not actually sure if that's correct any more now. Maybe it went in before 0.8 final release, but I thought there was some caveat about hard requiring it coming later that meant I kept seeing people at the time complaining about the errata 4 message when they didn't get it before...

zpool_create_features_007_pos only tested for compat-2020 feature
set. It would be useful to test for all known features sets. If
any additional feature is found enabled that is not present in
compatibility list or feature set, it should be caught and
reported earlier.

This commit also removes encryption from openzfsonosx-1.8.1
compatibility list. Encryption enables bookmark_v2, since it is
a dependency of encryption, but not listed in openzfsonoxx-1.8.1
compatibility list.

Signed-off-by: Umer Saleem <usaleem@ixsystems.com>
@usaleem-ix
Copy link
Contributor Author

Instead of adding bookmark_v2, I have removed encryption from the feature set for OpenZFS on OSX 1.8.1. Let's see what other people think about it.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Nov 8, 2023
@behlendorf
Copy link
Contributor

Instead of adding bookmark_v2, I have removed encryption from the feature set for OpenZFS on OSX 1.8.1. Let's see what other people think about it.

This seems like the safest thing to do. However, according to these release notes the bookmark_v2 feature was included in 1.8.1.

@behlendorf behlendorf closed this in 15a8fa7 Nov 9, 2023
behlendorf pushed a commit that referenced this pull request Nov 9, 2023
zpool_create_features_007_pos only tested for compat-2020 feature
set. It would be useful to test for all known features sets. If
any additional feature is found enabled that is not present in
compatibility list or feature set, it should be caught and
reported earlier.

This commit also removes encryption from openzfsonosx-1.8.1
compatibility list. Encryption enables bookmark_v2, since it is
a dependency of encryption, but not listed in openzfsonoxx-1.8.1
compatibility list.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Umer Saleem <usaleem@ixsystems.com>
Closes #15505
@usaleem-ix usaleem-ix deleted the NAS-124771-1 branch November 10, 2023 05:01
ixhamza pushed a commit to truenas/zfs that referenced this pull request Nov 20, 2023
This commit updates zpool-features.7 man page to add newly added
zpool features to grub2 compatibility list.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Umer Saleem <usaleem@ixsystems.com>
Closes openzfs#15505
ixhamza pushed a commit to truenas/zfs that referenced this pull request Nov 20, 2023
zpool_create_features_007_pos only tested for compat-2020 feature
set. It would be useful to test for all known features sets. If
any additional feature is found enabled that is not present in
compatibility list or feature set, it should be caught and
reported earlier.

This commit also removes encryption from openzfsonosx-1.8.1
compatibility list. Encryption enables bookmark_v2, since it is
a dependency of encryption, but not listed in openzfsonoxx-1.8.1
compatibility list.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Umer Saleem <usaleem@ixsystems.com>
Closes openzfs#15505
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
This commit updates zpool-features.7 man page to add newly added
zpool features to grub2 compatibility list.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Umer Saleem <usaleem@ixsystems.com>
Closes openzfs#15505
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
zpool_create_features_007_pos only tested for compat-2020 feature
set. It would be useful to test for all known features sets. If
any additional feature is found enabled that is not present in
compatibility list or feature set, it should be caught and
reported earlier.

This commit also removes encryption from openzfsonosx-1.8.1
compatibility list. Encryption enables bookmark_v2, since it is
a dependency of encryption, but not listed in openzfsonoxx-1.8.1
compatibility list.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Umer Saleem <usaleem@ixsystems.com>
Closes openzfs#15505
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.

4 participants