Conversation
|
Please add a PR comment, describing the highlights. |
|
This PR is introducing the feature called beacon-diff: Each Monitor client every 2 seconds sends the beacon with encapsulated GW data: list of subsystems, each subsystem contains list of namespaces and list of listeners.
|
|
@ronen-fr can you check if all of your comments were addressed? Thx. |
Only one nit: can you copy the 'This PR is introducing...' text to the PR description? (and, for next time, and as that comment is marked closed: BTW - '{:#x}' (adding the '#' sign) adds the '0x' automagically.) |
|
LGTM - but I do not know enough to approve NVME logic. |
|
@ronen-fr thx, we updated the description of the PR as you asked for. I believe that we addressed all of your comments. From the nvmeof logic side, I will approve it. From your side can you approve as well? |
|
jenkins test windows |
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
|
I just squashed all 6 commits to 1, added a tracker and re-based the PR |
athanatos
left a comment
There was a problem hiding this comment.
See above comment, I still need to review the rest of the PR.
gregsfortytwo
left a comment
There was a problem hiding this comment.
I didn't review the whole thing, but I have some concerns in the common core areas I looked at.
athanatos
left a comment
There was a problem hiding this comment.
Let me know if you need more information about how to encode subsystems in MNVMeofGwBeacon. It's going to be complex going forward as well, so it's important to understand it thoroughly.
Also, please update the commits as you want them in the final merge before requesting another review.
athanatos
left a comment
There was a problem hiding this comment.
LGTM, I'll approve after a qa run.
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
|
Observed a TEST_availablity_score failure in the RADOS QA run for this PR. |
|
Added the commit that should fix the monitor issue. Please run the test suite again @yuriw |
…ent.
-monclient encodes subsystems by beacon-diff rules if BEACON_DIFF
bit is enabled by quorum
-monitor processes beacons by beacon-diff new schema
-monitor detects sequence out of order(ooo) condition and handles it
-in case ooo detected monitor send ack to the gw with the expected correct sequence
-monitor skips failovers for some interval when ooo detected
-monitor ignores all becons with incorrect sequences until gw sends expected one
-coding upgrade rules
Signed-off-by: Leonid Chernin <leonidc@il.ibm.com>
Fixes: https://tracker.ceph.com/issues/72394
Signed-off-by: Samuel Just <sjust@redhat.com>
NOPE NOPE In order for the client to safely send BEACON_DIFF messages, it needs to be the case that the leader at the time of receipt will support BEACON_DIFF. Simply using the connection features for the MonClient's target mon is insufficient, because it might be a peon. If the peon supports BEACON_DIFF and the leader does not the leader will either crash or interpret it as a full BEACON. Neither outcome is acceptable. Instead, we need to wire up a feature bit to the MonMap mon_feature_t members and the CompatSet. Adding FEATURE_BEACON_DIFF to ceph::features::mon get_supported() and get_persistent() ensures that once all monitors in the quorum support it, MonMap::get_required_features() will include it. See Elector::propose_to_peers, Monitor::(win|lose)_election, MonmapMonitor::apply_mon_features. Once FEATURE_BEACON_DIFF is present in MonMap::get_required_features(): - Monitor::apply_monmap_to_compatset_features() will prevent downgrades of the monitors by updating the CompatSet to include CEPH_MON_FEATURE_INCOMPAT_NVMEOF_BEACON_DIFF - Monitor::calc_quorum_requirements() will set Monitor::required_features to require the NVMEOF_BEACON_DIFF for any monitor peers. - MonClient::get_monmap_required_features() will eventually include ceph::features::mon::FEATURE_NVMEOF_BEACON_DIFF. Signed-off-by: Samuel Just <sjust@redhat.com>
…d gws Fix race issue of map corruption when deleted gw sends beacons but this gw data was removed from pending map and still exists in map. Process beacons only if GW's data exists in both maps: main-map and pending-map, otherwise just ignore beacons. fixes: https://tracker.ceph.com/issues/74160 Signed-off-by: Leonid Chernin <leonidc@il.ibm.com>
|
jenkins test api |
|
This is an automated message by src/script/redmine-upkeep.py. I found one or more
The referenced tickets are: Those tickets do not reference this merged Pull Request. If this Pull Request merge resolves any of those tickets, please update the "Pull Request ID" field on each ticket. A future run of this script will appropriately update them. Update Log: https://github.com/ceph/ceph/actions/runs/20956912786 |
|
@leonidc: it looks the PR missed updating the |
|
Nice, another issue https://tracker.ceph.com/issues/74846 |
This PR is introducing the feature called beacon-diff:
Each Monitor client every 2 seconds sends the beacon with encapsulated GW data: list of subsystems, each subsystem contains list of namespaces and list of listeners.
In case configuration is big this can be pretty big overhead because in the stable site no changes happen - so same big portion of data is accepted by single monitor from many GW monitor clients.
In order to improve this behavior we designed the some kind of protocol:
In case Monitor client upon sending the beacon sees no difference on the subsystem, then this subsystem and its encapsulated "child's"(namespaces, listeners) are not present in the beacon
In case some subsystem changed from the previous beacon, it is added to the beacon alone with its child's, also added change-descriptor that describes the change (3 possible descriptor values ADDED, REMOVED, CHANGED)
Sequence number is added to each beacon. In case monitor gets beacon out of order, it ignores it and signals to the Monitor client to "return back" to the missing sequence number that monitor sends to it in the beacon ACK
When Monitor client receives ACK with indication of sequence out-of-order, it sends all subsystems in the next beacon(beacon diff is not calculated for the next beacon), it also sends the sequence number expected by the monitor.
see also
https://tracker.ceph.com/issues/72394
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 test classic perfJenkins Job | Jenkins Job Definitionjenkins test crimson perfJenkins Job | Jenkins Job Definitionjenkins test signedJenkins Job | Jenkins Job Definitionjenkins test make checkJenkins Job | Jenkins Job Definitionjenkins test make check arm64Jenkins Job | Jenkins Job Definitionjenkins test submodulesJenkins Job | Jenkins Job Definitionjenkins test dashboardJenkins Job | Jenkins Job Definitionjenkins test dashboard cephadmJenkins Job | Jenkins Job Definitionjenkins test apiJenkins Job | Jenkins Job Definitionjenkins test docsReadTheDocs | Github Workflow Definitionjenkins test ceph-volume allJenkins Jobs | Jenkins Jobs Definitionjenkins test windowsJenkins Job | Jenkins Job Definitionjenkins test rook e2eJenkins Job | Jenkins Job Definition