squid: osd: fix require_min_compat_client handling for msr rules#59492
Merged
ljflores merged 5 commits intoceph:squidfrom Sep 5, 2024
Merged
squid: osd: fix require_min_compat_client handling for msr rules#59492ljflores merged 5 commits intoceph:squidfrom
ljflores merged 5 commits intoceph:squidfrom
Conversation
Unit testing ------------ ``` [rzarzynski@o06 build]$ bin/unittest_features ... [ RUN ] features.release_features 1 argonaut features 0x40000 looks like argonaut 2 bobtail features 0x40000 looks like argonaut 3 cuttlefish features 0x40000 looks like argonaut 4 dumpling features 0x42040000 looks like dumpling 5 emperor features 0x42040000 looks like dumpling 6 firefly features 0x20842040000 looks like firefly 7 giant features 0x20842040000 looks like firefly 8 hammer features 0x1020842040000 looks like hammer 9 infernalis features 0x1020842040000 looks like hammer 10 jewel features 0x401020842040000 looks like jewel 11 kraken features 0xc01020842040000 looks like kraken 12 luminous features 0xe01020842240000 looks like luminous 13 mimic features 0xe01020842240000 looks like luminous 14 nautilus features 0xe01020842240000 looks like luminous 15 octopus features 0xe01020842240000 looks like luminous 16 pacific features 0xe01020842240000 looks like luminous 17 quincy features 0xe01020842240000 looks like luminous 18 reef features 0xe010208d2240000 looks like reef 19 squid features 0xe010248d2240000 looks like squid [ OK ] features.release_features (0 ms) ``` Manual testing -------------- \### `reef` client present in `squid` cluster ``` [rzarzynski@o06 build]$ bin/ceph daemon mon.a sessions | jq -jr '.[] | .name, "\t", .con_features, "\t", .con_features_hex, "\n"' | grep client client.? 4540701547738038271 3f03cffffffdffff client.? 4540138322906710015 3f01cfbffffdffff [rzarzynski@o06 build]$ bin/ceph osd get-require-min-compat-client luminous [rzarzynski@o06 build]$ bin/ceph osd set-require-min-compat-client squid Error EPERM: cannot set require_min_compat_client to squid: 1 connected client(s) look like reef (missing 0x4000000000); add --yes-i-really-mean-it to do it anyway ``` \### only `squid` clients and `squid` cluster ``` [rzarzynski@o06 build]$ bin/ceph daemon mon.a sessions | jq -jr '.[] | .name, "\t", .con_features, "\t", .con_features_hex, "\n"' | grep client client.? 4540701547738038271 3f03cffffffdffff client.? 4540701547738038271 3f03cffffffdffff [rzarzynski@o06 build]$ bin/ceph osd set-require-min-compat-client squid set require_min_compat_client to squid ``` Fixes: https://tracker.ceph.com/issues/66297 Signed-off-by: Radoslaw Zarzynski <[email protected]> (cherry picked from commit 4b54b07)
…pool OSDMap::get_features is used by OSDMonitor::validate_crush_against_features via OSDMap::get_min_compat_client() to check whether changes to the crushmap will require newer features than the existing require_min_compat_client field. Monitor commands which create rules from ec profiles may result in msr rules. While it might be harmless to allow msr rules to exist as long as there aren't any pools actually using the rule, it's probably simpler to disallow their creation in the first place until require_min_compat_client is updated. Signed-off-by: Samuel Just <[email protected]> (cherry picked from commit 2130115)
This was simply a mistake: uint64_t features = 0; features |= CEPH_FEATURE_CRUSH_MSR; ceph_assert(HAVE_FEATURE(features, CRUSH_MSR)); will fail the assert as CEPH_FEATURE_CRUSH_MSR lacks the mask. In basically all cases, CEPH_FEATUREMASK_* is the correct choice. Signed-off-by: Samuel Just <[email protected]> (cherry picked from commit cb157b4)
Also adjusts validate_crush_against_features to use an ostream& rather than a stringstream&. Fixes: https://tracker.ceph.com/issues/67755 Signed-off-by: Samuel Just <[email protected]> (cherry picked from commit 1d6b4d4)
ljflores
added
needs-upgrade-testing
core
common
mon
and removed
core
common
mon
labels
athanatos
approved these changes
Aug 28, 2024
Contributor
athanatos
left a comment
athanatos
left a comment
There was a problem hiding this comment.
LGTM once tested and the original merges
… profiles Signed-off-by: Samuel Just <[email protected]> (cherry picked from commit 4f9289e)
Member
Author
|
jenkins retest this please |
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #59474
Backport of 4b54b07
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windowsjenkins test rook e2e