StateStorage self heal fix bugs (#47336)

Co-authored-by: Copilot Autofix powered by AI <[email protected]>

3 files changed, 196 insertions, 0 deletions

diff --git a/ydb/core/blobstorage/nodewarden/distconf_generate.cpp b/ydb/core/blobstorage/nodewarden/distconf_generate.cpp
index 96e83a946b7..dc9d3817de5 100644
--- a/ydb/core/blobstorage/nodewarden/distconf_generate.cpp
+++ b/ydb/core/blobstorage/nodewarden/distconf_generate.cpp
@@ -626,6 +626,23 @@ namespace NKikimr::NStorage {
         ) {
         if (!automaticManagement) {
             ss->CopyFrom(oldConfig);
+
+            const auto collectNodes = [&](const auto& self, const auto& ring) -> void {
+                for (ui32 nodeId : ring.GetNode()) {
+                    usedNodes.insert(nodeId);
+                }
+                for (const auto& subRing : ring.GetRing()) {
+                    self(self, subRing);
+                }
+            };
+
+            if (oldConfig.HasRing()) {
+                collectNodes(collectNodes, oldConfig.GetRing());
+            }
+            for (const auto& rg : oldConfig.GetRingGroups()) {
+                collectNodes(collectNodes, rg);
+            }
+
             return true;
         }
         std::map<TBridgePileId, THashMap<TString, std::vector<std::tuple<ui32, TNodeLocation>>>> nodes;
@@ -643,6 +660,13 @@ namespace NKikimr::NStorage {
             generator.AddRingGroup(ss);
             goodConfig &= generator.IsGoodConfig();
         }
+        if (ss->RingGroupsSize() == 0) {
+            // nodesToUse was non-empty, but none of the specified node IDs exist in baseConfig
+            // (e.g. StateStorageSelfHealAllowedNodes referencing decommissioned nodes); avoid
+            // returning a bogus empty state storage config that would trigger a destructive
+            // full reconfiguration.
+            return false;
+        }
         return goodConfig;
     }
 
diff --git a/ydb/core/blobstorage/nodewarden/distconf_ut.cpp b/ydb/core/blobstorage/nodewarden/distconf_ut.cpp
index cbb9d93aa45..5803954ef31 100644
--- a/ydb/core/blobstorage/nodewarden/distconf_ut.cpp
+++ b/ydb/core/blobstorage/nodewarden/distconf_ut.cpp
@@ -246,6 +246,28 @@ Y_UNIT_TEST_SUITE(TDistconfGenerateConfigTest) {
         CheckStateStorage(GenerateSimpleStateStorage(3, {}, 0, 0, 200, {1, 2, 3, 9999}), 3, {1, 2, 3});
     }
 
+    Y_UNIT_TEST(NodesToUseAllUnknownYieldsBadConfigNotEmptySS) {
+        // Regression test: if nodesToUse is non-empty, but *none* of the specified node ids
+        // exist in baseConfig's AllNodes (e.g. StateStorageSelfHealAllowedNodes referencing
+        // decommissioned nodes), the generator must not silently produce an empty (but "good")
+        // state storage config, since that would trigger a destructive full reconfiguration
+        // to an empty state storage in the self-heal path. Instead, GenerateStateStorageConfig
+        // must report the config as bad.
+        bool goodConfig = true;
+        auto ss = GenerateSimpleStateStorage(3, {}, 0, 0, 200, {9998, 9999}, &goodConfig);
+        UNIT_ASSERT(!goodConfig);
+        UNIT_ASSERT_EQUAL(ss.RingGroupsSize(), 0);
+    }
+
+    Y_UNIT_TEST(NodesToUseAllUnknownInDCTopologyYieldsBadConfig) {
+        // Same regression, but exercised through the multi-DC/rack topology generator, since
+        // that is the code path used in practice by the self-heal state storage logic.
+        bool goodConfig = true;
+        auto ss = GenerateDCStateStorage(3, 3, 3, {}, {}, {}, 9, 0, 0, 1000, /*nodesToUse=*/{9998, 9999}, &goodConfig);
+        UNIT_ASSERT(!goodConfig);
+        UNIT_ASSERT_EQUAL(ss.RingGroupsSize(), 0);
+    }
+
     Y_UNIT_TEST(NodesToUseForcesAlternateNodeSelectionInDC) {
         // Without restriction, the default pick within each 3-node rack is its
         // first node: {1, 4, 7, 10, 13, 16, 19, 22, 25}.
@@ -370,6 +392,105 @@ Y_UNIT_TEST_SUITE(TDistconfGenerateConfigTest) {
             200, /*nodesToUse=*/{1, 2, 3}, &goodConfig, oldSS, /*automaticManagement=*/false);
         UNIT_ASSERT(goodConfig);
     }
+
+    Y_UNIT_TEST(AutomaticManagementDisabledPopulatesUsedNodes) {
+        // Regression test: when automaticManagement is false, the generator must still populate
+        // usedNodes with the node IDs taken from oldConfig, because usedNodes is shared across
+        // the StateStorage / StateStorageBoard / SchemeBoard generator invocations, and subsequent
+        // subsystem generators rely on it to avoid co-locating replicas on the same nodes.
+        NKikimrConfig::TDomainsConfig::TStateStorage oldSS;
+        {
+            auto* rg = oldSS.AddRingGroups();
+            rg->SetNToSelect(2);
+            auto* ring1 = rg->AddRing();
+            ring1->AddNode(3);
+            auto* ring2 = rg->AddRing();
+            ring2->AddNode(5);
+        }
+
+        std::unordered_set<ui32> usedNodes;
+        bool goodConfig = false;
+        GenerateSimpleStateStorage(8, usedNodes, 0, 0, 200, {}, &goodConfig, oldSS,
+            /*automaticManagement=*/false);
+        UNIT_ASSERT(goodConfig);
+
+        // usedNodes is passed by value into GenerateSimpleStateStorage's helper, so re-derive it
+        // directly via the keeper API to check that the reference-passed set was actually filled.
+        NKikimr::NStorage::TDistributedConfigKeeper keeper(nullptr, nullptr, true);
+        NKikimrConfig::TDomainsConfig::TStateStorage ssOut;
+        NKikimrBlobStorage::TStorageConfig config;
+        for (ui32 i : xrange(8)) {
+            auto *node = config.AddAllNodes();
+            node->SetNodeId(i + 1);
+        }
+        std::unordered_set<ui32> refUsedNodes;
+        bool refGoodConfig = keeper.GenerateStateStorageConfig(&ssOut, config, refUsedNodes, {}, oldSS,
+            /*automaticManagement=*/false, 0, 0, 200);
+        UNIT_ASSERT(refGoodConfig);
+        UNIT_ASSERT(refUsedNodes.contains(3));
+        UNIT_ASSERT(refUsedNodes.contains(5));
+        UNIT_ASSERT_EQUAL(refUsedNodes.size(), 2u);
+    }
+
+    Y_UNIT_TEST(AutomaticManagementDisabledUsedNodesAvoidedBySubsequentGenerator) {
+        // Simulate the real-world scenario from the bug report: StateStorage has automatic
+        // management disabled (its nodes are kept as-is), while StateStorageBoard has it enabled.
+        // The board generator must avoid nodes already occupied by StateStorage replicas, which
+        // are threaded through the shared usedNodes set.
+        NKikimrConfig::TDomainsConfig::TStateStorage oldStateStorage;
+        {
+            auto* rg = oldStateStorage.AddRingGroups();
+            rg->SetNToSelect(3);
+            for (ui32 node : {1, 2, 3}) {
+                auto* ring = rg->AddRing();
+                ring->AddNode(node);
+            }
+        }
+
+        // Use a node pool large enough that plenty of spare nodes remain even after 3 are marked
+        // as used by StateStorage, so the board generator has enough room to build its own config.
+        NKikimrBlobStorage::TStorageConfig config;
+        for (ui32 i : xrange(20)) {
+            auto *node = config.AddAllNodes();
+            node->SetNodeId(i + 1);
+        }
+
+        NKikimr::NStorage::TDistributedConfigKeeper keeper(nullptr, nullptr, true);
+        // Explicitly mark all nodes as GOOD; otherwise nodes absent from SelfHealNodesState are
+        // treated as UNKNOWN, which would make IsGoodConfig() always return false.
+        for (ui32 i : xrange(20)) {
+            keeper.SelfHealNodesState[i + 1] = 0;
+        }
+        std::unordered_set<ui32> usedNodes;
+
+        // Step 1: StateStorage generation with automatic management disabled.
+        NKikimrConfig::TDomainsConfig::TStateStorage stateStorageOut;
+        bool goodConfig1 = keeper.GenerateStateStorageConfig(&stateStorageOut, config, usedNodes, {},
+            oldStateStorage, /*automaticManagement=*/false, 0, 0, 200);
+        UNIT_ASSERT(goodConfig1);
+        UNIT_ASSERT(usedNodes.contains(1));
+        UNIT_ASSERT(usedNodes.contains(2));
+        UNIT_ASSERT(usedNodes.contains(3));
+
+        // Step 2: StateStorageBoard generation with automatic management enabled, sharing usedNodes.
+        NKikimrConfig::TDomainsConfig::TStateStorage boardOut;
+        NKikimrConfig::TDomainsConfig::TStateStorage emptyOldBoard;
+        bool goodConfig2 = keeper.GenerateStateStorageConfig(&boardOut, config, usedNodes, {},
+            emptyOldBoard, /*automaticManagement=*/true, 0, 0, 200);
+        UNIT_ASSERT(goodConfig2);
+
+        // None of the nodes selected for the board must overlap with the nodes already used by
+        // StateStorage, since those were marked as used via usedNodes.
+        for (const auto& rg : boardOut.GetRingGroups()) {
+            for (const auto& ring : rg.GetRing()) {
+                for (ui32 node : ring.GetNode()) {
+                    UNIT_ASSERT_C(!(node == 1 || node == 2 || node == 3),
+                        "StateStorageBoard replica placed on node " << node
+                            << " which is already occupied by StateStorage");
+                }
+            }
+        }
+    }
 }
 
 Y_UNIT_TEST_SUITE(TDeriveStorageConfigCleanupTest) {
diff --git a/ydb/tests/functional/config/test_distconf_self_heal.py b/ydb/tests/functional/config/test_distconf_self_heal.py
index bd14fa0c77e..375e1094ff3 100644
--- a/ydb/tests/functional/config/test_distconf_self_heal.py
+++ b/ydb/tests/functional/config/test_distconf_self_heal.py
@@ -334,3 +334,54 @@ class TestKiKiMRDistConfSelfHealAutomaticManagementDisabled(KiKiMRDistConfSelfHe
         # automatic management for this subsystem is disabled.
         unchanged_config = get_ring_group(self.do_request_config(), configName)
         assert_eq(unchanged_config, bad_config)
+
+
+class TestKiKiMRDistConfSelfHealMixedAutomaticManagement(KiKiMRDistConfSelfHealTest):
+    # Verifies the mixed scenario: automatic management is disabled for one subsystem
+    # (StateStorage) while enabled for the other two (StateStorageBoard, SchemeBoard). Self-heal
+    # must heal the enabled subsystems while leaving the disabled one untouched, exercising the
+    # interaction with the shared usedNodes/nodesToReplace state in SelfHealStateStorage.
+    erasure = Erasure.MIRROR_3_DC
+    nodes_count = 9
+    rgOffset = 1
+
+    disabled_config_name = "StateStorage"
+
+    self_management_extra_options = {
+        "automatic_state_storage_management": False,
+        "automatic_state_storage_board_management": True,
+        "automatic_scheme_board_management": True,
+    }
+
+    def check_failed(self, req, message):
+        resp = self.do_request(req)
+        assert_that(resp.get("ErrorReason", "").startswith(message), {"Response": resp, "Expected": message})
+
+    def do_test(self, configName):
+        self.do_bad_config(configName)
+        bad_config = get_ring_group(self.do_request_config(), configName)
+
+        logger.info(
+            "Start SelfHeal with mixed automatic management (disabled for %s) for %s",
+            self.disabled_config_name, configName)
+
+        if configName == self.disabled_config_name:
+            self.check_failed(
+                {"SelfHealStateStorage": {"WaitForConfigStep": 1, "ForceHeal": True}},
+                "Current configuration is recommended. Nothing to self-heal.")
+            time.sleep(5)
+
+            # The disabled subsystem's config must remain completely untouched, even though the
+            # other subsystems are actively being healed around it.
+            unchanged_config = get_ring_group(self.do_request_config(), configName)
+            assert_eq(unchanged_config, bad_config)
+        else:
+            disabled_before = get_ring_group(self.do_request_config(), self.disabled_config_name)
+            logger.info(self.do_request({"SelfHealStateStorage": {"WaitForConfigStep": 1, "ForceHeal": True}}))
+            time.sleep(10)
+
+            disabled_after = get_ring_group(self.do_request_config(), self.disabled_config_name)
+            assert_eq(disabled_after, disabled_before)
+            rg = get_ring_group(self.do_request_config(), configName)
+            assert_eq(rg["NToSelect"], 9)
+            assert_eq(len(rg["Ring"]), 9)