Stretched cluster by leonidc · Pull Request #66624 · ceph/ceph

leonidc · 2025-12-14T07:23:11Z

The NVMe-oF GWs are currently unaware of the data center location. When all of the GWs in the group are collocated in the same location, there is no issue. But if GWs (within the same GW group) are located in different locations within the same Ceph cluster, the failover target might be wrong. Choosing the wrong failover target will bring into a situation where the compute is on one site but storage is on another site. This is causing a significant impact to the I/O performance.
Also when disaster cluster recovered failback of ANA groups to it should be done only by monitor command.

Last update
Introduce explicit commands to set and clear a disaster state for a location:
nvme-gw pool group disaster Set
nvme-gw pool group disaster Clear
These commands explicitly inform the monitor of the disaster status of a given location.

remove the command:
nvme-gw pool group start-failback

Expected Behavior
With this mechanism in place:

Controlled failbacks
Failbacks from other locations are allowed for all gateways except those belonging to a location marked as being in a disaster state.
Stable ANA ownership during disasters
Gateways in a disaster location, even if partially recovered, will not acquire ANA group ownership during failovers caused by gateway failures in other locations.
Recovery handling
When a location is marked as recovered using the “disaster Clear” command, the system triggers the same workflow previously initiated by the start-failback command, forcing failbacks to the gateways in the previously affected location.

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 x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins test classic perf Jenkins Job | Jenkins Job Definition
jenkins test crimson perf Jenkins Job | Jenkins Job Definition
jenkins test signed Jenkins Job | Jenkins Job Definition
jenkins test make check Jenkins Job | Jenkins Job Definition
jenkins test make check arm64 Jenkins Job | Jenkins Job Definition
jenkins test submodules Jenkins Job | Jenkins Job Definition
jenkins test dashboard Jenkins Job | Jenkins Job Definition
jenkins test dashboard cephadm Jenkins Job | Jenkins Job Definition
jenkins test api Jenkins Job | Jenkins Job Definition
jenkins test docs ReadTheDocs | Github Workflow Definition
jenkins test ceph-volume all Jenkins Jobs | Jenkins Jobs Definition
jenkins test windows Jenkins Job | Jenkins Job Definition
jenkins test rook e2e Jenkins Job | Jenkins Job Definition

You must only issue one Jenkins command per-comment. Jenkins does not understand
comments with more than one command.

caroav · 2026-01-07T08:25:36Z

@athanatos can you review it? thx.

athanatos · 2026-01-07T18:37:38Z

I will try to get to this this week.

athanatos · 2026-01-09T21:27:32Z

Not going to get to it today, I'll review it Monday morning.

athanatos

I commented on some things, but I don't really feel that I understand the intent here. Perhaps you could explain in a bit more detail how the additions in this patch work?

athanatos · 2026-01-12T22:32:08Z

src/mon/NVMeofGwMap.cc

+
+bool NVMeofGwMap::validate_number_locations(int num_gws, int num_locations)
+{
+  return true; // This function  may have not empty body in the nex commit


There is no next commit. Is this for a future PR? Perhaps something like // TODO: add validation

athanatos · 2026-01-12T22:51:23Z

src/mon/NVMeofGwMap.cc

+
+int NVMeofGwMap::cfg_set_location(const NvmeGwId &gw_id,
+    const NvmeGroupKey& group_key,
+    std::string &location, bool &propose_pending, bool test) {


test is not used.

removed, thanks

src/mon/NVMeofGwMap.cc

athanatos · 2026-01-13T02:20:35Z

src/mon/NVMeofGwMap.cc

+  */
+#define MIN_NUM_ANA_GROUPS 0xFFF
+  uint32_t min_num_ana_groups_in_gw = MIN_NUM_ANA_GROUPS;
+  NvmeGwId min_gw_id = "empty";


What's the intent here -- is empty not a valid gw id?

@athanatos thanks, agree , will remove this assignment

src/mon/NVMeofGwMap.cc

athanatos · 2026-01-13T02:29:21Z

src/mon/NVMeofGwMap.cc

+  for (auto& found_gw_state: gws_states) {
+    auto st = found_gw_state.second;
+    if (st.location == location) {
+      if(st.availability == gw_availability_t::GW_AVAILABLE &&


athanatos · 2026-01-13T02:32:22Z

src/mon/NVMeofGwMap.cc

+   * for ana-grp in list make relocation.
+   * if all ana-grps in location active remove location from the map failbacks_in_progress
+  */
+  FailbackLocation location = "";


Why declare this here with a placeholder value? Seems like you could just declare it when set below at

location = failbacks_in_progress[group_key];

athanatos · 2026-01-13T02:36:01Z

src/mon/NVMeofGwMap.cc

+   * function tries to found the minimum loaded gw in location
+   * if found, relocate ana grp to it using regular failback FSM
+  */
+#define MIN_NUM_ANA_GROUPS 0xFFF


So if any particular gw has more than MIN_NUM_ANA_GROUPS you won't migrate to it? Seems like a bad name.

@athanatos , the problem is name of value ? what do you propose?

Well, it seems like a max? MAX_NUM_ANA_GROUPS_FOR_RELOCATE? Also, don't use #defines for things like this, use a constexpr.

athanatos · 2026-01-13T02:37:13Z

src/mon/NVMeofGwMap.cc

  bool do_failback = false;
-  dout(10) << "Find failback GW for GW " << gw_id << dendl;
+  bool force_inter_location = false;
+  FailbackLocation location = "";


Once again, why initialize this with a dummy value? It only seems to be used in the if block immediately below, just declare it when assigned to.

athanatos · 2026-01-13T02:37:37Z

src/mon/NVMeofGwMap.cc

+    for (auto& found_gw_state: gws_states) {
+      auto st = found_gw_state.second;
+      if (st.availability == gw_availability_t::GW_AVAILABLE)
+    	    if (location == "" || st.location == location) {


you are right , this is a bad style because "" is also valid location, thought the current code is also working,
will add parameter to the function "ignore_location"

github-actions · 2026-01-13T12:48:31Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

athanatos · 2026-01-23T00:51:58Z

I'm still basically just not sure how these commands are supposed to work. Is there documentation to tell users when and how to actually make use of them?

leonidc · 2026-01-25T09:26:37Z

@athanatos , I added a description to the PR and motivations for the update.
If it is not sufficient will add more info

caroav · 2026-02-08T06:45:26Z

@athanatos will you be able to continue the review soon? thx.

athanatos

I don't see any serious problems. I still don't entirely follow the nvme specific logic -- perhaps someone from the nvme team could do a review?

src/mon/NVMeofGwMap.cc

athanatos · 2026-02-10T22:48:10Z

src/mon/NVMeofGwMap.cc

+}
+
+bool NVMeofGwMap::is_location_in_disaster(const NvmeGroupKey& group_key,
+           NvmeLocation& location, bool &cleanup_in_process) {


If the goal isn't to be able to call this multiple times with the same cleanup_in_progress param, it would be a lot better to set it in every exit path.

agree , will change

@athanatos @baum reviewed and approved. Will you be able to approve.

src/mon/NVMeofGwMap.h

src/mon/NVMeofGwSerialize.h

src/mon/NVMeofGwMon.cc

src/mon/NVMeofGwMap.cc

src/mon/NVMeofGwSerialize.h

baum

lgtm

GW commands added : set location and set admin state enable/disable added start-failback <location> command. failover logic is impacted by GW location implement GW admin commands enable/disable added map for location-failback-in-progress failback between locations happens only by monitor command implemented new ana-group relocation process used when inter-location failback command sent added upgrade rules fixes: https://tracker.ceph.com/issues/74210 Signed-off-by: Leonid Chernin <leonidc@il.ibm.com>

changes in behavior of failover/failback, additional validations added Signed-off-by: Leonid Chernin <leonidc@il.ibm.com>

Signed-off-by: Leonid Chernin <leonidc@il.ibm.com>

athanatos

This should go through rados teuthology prior to merge.

remove invalid code from NVMeofGwSerialize.h put encode/decode at the file NVmeofGwMap.h Signed-off-by: Leonid Chernin <leonidc@il.ibm.com>

yuriw · 2026-03-09T17:22:13Z

jenkins test this please

@athanatos

This reverts commit 6dddf54, reversing changes made to 07ec509. Backport 6dddf54 introduced a new connection feature bit NVMEOF_BEACON_DIFF but there are plans (ceph#66624) to make further enhancements on that feature bit. This would cause the mons to crash during upgrades. However, this connection feature bit should not have been added to begin with. The correct way to do this is extend e55ad7b by @athanatos to require `CEPH_MON_FEATURE_INCOMPAT_NVMEOF_BEACON_DIFF` if all mons support it. This should be done by having mons add/update their supported features the MonMap via an update from `MMonJoin` (see for instance `crush_loc` which was recently added to `mon_info_t`). Once the supported features indicated for each mon in the `MonMap` show they understand the new NVMEOF_BEACON_DIFF, then it should be turned on globally in the `MonMap` as a required feature (added to the incompat set). Conflicts: src/mon/NVMeofGwMon.h: conflicts with header change from 19c9be2 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

@athanatos

This reverts commit 6dddf54, reversing changes made to 07ec509. Backport 6dddf54 introduced a new connection feature bit NVMEOF_BEACON_DIFF but there are plans (ceph#66624) to make further enhancements on that feature bit. This would cause the mons to crash during upgrades. However, this connection feature bit should not have been added to begin with. The correct way to do this is extend e55ad7b by @athanatos to require `CEPH_MON_FEATURE_INCOMPAT_NVMEOF_BEACON_DIFF` if all mons support it. This should be done by having mons add/update their supported features the MonMap via an update from `MMonJoin` (see for instance `crush_loc` which was recently added to `mon_info_t`). Once the supported features indicated for each mon in the `MonMap` show they understand the new NVMEOF_BEACON_DIFF, then it should be turned on globally in the `MonMap` as a required feature (added to the incompat set). Conflicts: src/mon/NVMeofGwMon.h: conflicts with header change from 19c9be2 fix missing header change in ceph#66584 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

JonBailey1993 · 2026-03-20T14:25:23Z

Rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues75119

leonidc requested a review from a team as a code owner December 14, 2025 07:23

github-actions bot added build/ops core mon nvmeof tests labels Dec 14, 2025

leonidc requested a review from athanatos December 15, 2025 05:07

leonidc force-pushed the stretched-cluster branch from 662ae2d to 42e81b1 Compare January 7, 2026 08:26

athanatos requested changes Jan 13, 2026

View reviewed changes

leonidc force-pushed the stretched-cluster branch 2 times, most recently from ea9fb24 to b561d27 Compare January 13, 2026 12:44

github-actions bot added the needs-rebase label Jan 13, 2026

leonidc force-pushed the stretched-cluster branch from b561d27 to 60ebc15 Compare January 14, 2026 07:55

leonidc force-pushed the stretched-cluster branch from 60ebc15 to 25845c7 Compare January 22, 2026 16:43

leonidc force-pushed the stretched-cluster branch from 25845c7 to 08afd86 Compare January 23, 2026 08:36

leonidc force-pushed the stretched-cluster branch from 08afd86 to 3458e78 Compare January 25, 2026 09:30

github-actions bot removed the needs-rebase label Jan 25, 2026

athanatos requested changes Feb 10, 2026

View reviewed changes

leonidc force-pushed the stretched-cluster branch from 3458e78 to a0d57de Compare February 11, 2026 13:17

leonidc requested a review from baum February 11, 2026 13:51

baum reviewed Feb 11, 2026

View reviewed changes

leonidc force-pushed the stretched-cluster branch from a0d57de to 9dac29a Compare February 12, 2026 08:03

baum approved these changes Feb 17, 2026

View reviewed changes

leonidc added 3 commits February 18, 2026 10:15

nvmeofgw: disaster set/clear command, introduced disaster-locations map,

67f3a7f

changes in behavior of failover/failback, additional validations added Signed-off-by: Leonid Chernin <leonidc@il.ibm.com>

code review changes

380ebf0

Signed-off-by: Leonid Chernin <leonidc@il.ibm.com>

leonidc force-pushed the stretched-cluster branch from 9dac29a to 380ebf0 Compare February 18, 2026 08:15

athanatos self-requested a review February 23, 2026 17:03

athanatos approved these changes Feb 23, 2026

View reviewed changes

athanatos added the needs-qa label Feb 23, 2026

yuriw added the wip-yuri8-testing label Feb 23, 2026

fix upgrade

c319e6a

remove invalid code from NVMeofGwSerialize.h put encode/decode at the file NVmeofGwMap.h Signed-off-by: Leonid Chernin <leonidc@il.ibm.com>

batrick mentioned this pull request Mar 12, 2026

tentacle: Revert "Merge pull request #66958 from Hezko/wip-74413-tentacle" #67750

Merged

14 tasks

yuriw merged commit c47f5b4 into ceph:main Mar 20, 2026
13 checks passed

Conversation

leonidc commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

caroav commented Jan 7, 2026

Uh oh!

athanatos commented Jan 7, 2026

Uh oh!

athanatos commented Jan 9, 2026

Uh oh!

athanatos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 13, 2026

Uh oh!

athanatos commented Jan 23, 2026

Uh oh!

leonidc commented Jan 25, 2026

Uh oh!

caroav commented Feb 8, 2026

Uh oh!

athanatos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

athanatos Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leonidc commented Dec 14, 2025 •

edited

Loading

athanatos Feb 10, 2026 •

edited

Loading