From 7255340ae5b343fc7a53150d3fdbb30d97d1ed54 Mon Sep 17 00:00:00 2001 From: Alexander Presnyakov Date: Fri, 18 Apr 2025 02:07:15 +0000 Subject: [PATCH] Add dbroots of other nodes to the read-only node --- cmapi/cmapi_server/managers/transaction.py | 4 +- cmapi/cmapi_server/node_manipulation.py | 81 ++++++++++++++++++---- cmapi/cmapi_server/test/test_cluster.py | 4 -- cmapi/cmapi_server/test/test_node_manip.py | 67 ++++++++++++++++-- 4 files changed, 133 insertions(+), 23 deletions(-) diff --git a/cmapi/cmapi_server/managers/transaction.py b/cmapi/cmapi_server/managers/transaction.py index 68bb7bc77..98de50524 100644 --- a/cmapi/cmapi_server/managers/transaction.py +++ b/cmapi/cmapi_server/managers/transaction.py @@ -106,10 +106,10 @@ class TransactionManager(ContextDecorator): try: rollback_transaction(self.txn_id, nodes=nodes) self.active_transaction = False - logging.debug(f'Success rollback of transaction "{self.txn_id}".') + logging.debug(f'Successfull rollback of transaction "{self.txn_id}".') except Exception: logging.error( - f'Error while rollback transaction "{self.txn_id}"', + f'Error while rolling back transaction "{self.txn_id}"', exc_info=True ) diff --git a/cmapi/cmapi_server/node_manipulation.py b/cmapi/cmapi_server/node_manipulation.py index 975197891..0a3f4c4c7 100644 --- a/cmapi/cmapi_server/node_manipulation.py +++ b/cmapi/cmapi_server/node_manipulation.py @@ -112,9 +112,7 @@ def add_node( _rebalance_dbroots(c_root) _move_primary_node(c_root) else: - logging.debug( - 'Node is read-only, skipping dbroots rebalancing.' - ) + add_dbroots_of_other_nodes(c_root, pm_num) except Exception: logging.error( 'Caught exception while adding node, config file is unchanged', @@ -186,6 +184,9 @@ def remove_node( if use_rebalance_dbroots and not is_read_only: _rebalance_dbroots(c_root) _move_primary_node(c_root) + + if is_read_only: + remove_dbroots_of_node(c_root, pm_num) else: # TODO: # - IMO undefined behaviour here. Removing one single node @@ -548,6 +549,19 @@ def unassign_dbroot1(root): i += 1 +def _get_existing_db_roots(root: etree.Element) -> list[int]: + '''Get all the existing dbroot IDs from the config file''' + # There can be holes in the dbroot numbering, so can't just scan from [1-dbroot_count] + # Going to scan from 1-99 instead + sysconf_node = root.find("./SystemConfig") + existing_dbroots = [] + for num in range(1, 100): + node = sysconf_node.find(f"./DBRoot{num}") + if node is not None: + existing_dbroots.append(num) + return existing_dbroots + + def _rebalance_dbroots(root, test_mode=False): # TODO: add code to detect whether we are using shared storage or not. If not, exit # without doing anything. @@ -591,14 +605,7 @@ def _rebalance_dbroots(root, test_mode=False): current_mapping = get_current_dbroot_mapping(root) sysconf_node = root.find("./SystemConfig") - - # There can be holes in the dbroot numbering, so can't just scan from [1-dbroot_count] - # Going to scan from 1-99 instead. - existing_dbroots = [] - for num in range(1, 100): - node = sysconf_node.find(f"./DBRoot{num}") - if node is not None: - existing_dbroots.append(num) + existing_dbroots = _get_existing_db_roots(root) # assign the unassigned dbroots unassigned_dbroots = set(existing_dbroots) - set(current_mapping[0]) @@ -650,7 +657,7 @@ def _rebalance_dbroots(root, test_mode=False): # timed out # possible node is not ready, leave retry as-is pass - except Exception as e: + except Exception: retry = False if not found_master: @@ -986,7 +993,7 @@ def _add_Module_entries(root, node): logging.info(f"_add_Module_entries(): hostname doesn't match, updating address to {new_ip_addr}") smc_node.find(f"ModuleHostName{i}-1-3").text = new_ip_addr else: - logging.info(f"_add_Module_entries(): no update is necessary") + logging.info("_add_Module_entries(): no update is necessary") return # if we find a matching hostname, update the ip addr @@ -1174,3 +1181,51 @@ def _replace_localhost(root: etree.Element, node: str) -> bool: # New Exception types class NodeNotFoundException(Exception): pass + + +def add_dbroots_of_other_nodes(root: etree.Element, module_num: int) -> None: + """Adds all the dbroots listed in the config to this (read-only) node""" + existing_dbroots = _get_existing_db_roots(root) + sysconf_node = root.find("./SystemModuleConfig") + + # Write node's dbroot count + dbroot_count_node = sysconf_node.find(f"./ModuleDBRootCount{module_num}-3") + if dbroot_count_node is not None: + sysconf_node.remove(dbroot_count_node) + dbroot_count_node = etree.SubElement( + sysconf_node, f"ModuleDBRootCount{module_num}-3" + ) + dbroot_count_node.text = str(len(existing_dbroots)) + + # Remove existing dbroot IDs of this module if present + for i in range(1, 100): + dbroot_id_node = sysconf_node.find(f"./ModuleDBRootID{module_num}-{i}-3") + if dbroot_id_node is not None: + sysconf_node.remove(dbroot_id_node) + + # Write new dbroot IDs to the module mapping + for i, dbroot_id in enumerate(existing_dbroots, start=1): + dbroot_id_node = etree.SubElement( + sysconf_node, f"ModuleDBRootID{module_num}-{i}-3" + ) + dbroot_id_node.text = str(dbroot_id) + + logging.info("Added %d dbroots to read-only node %d: %s", len(existing_dbroots), module_num, sorted(existing_dbroots)) + + +def remove_dbroots_of_node(root: etree.Element, module_num: int) -> None: + """Removes all the dbroots listed in the config from this (read-only) node""" + sysconf_node = root.find("./SystemModuleConfig") + dbroot_count_node = sysconf_node.find(f"./ModuleDBRootCount{module_num}-3") + if dbroot_count_node is not None: + sysconf_node.remove(dbroot_count_node) + else: + logging.error( + f"ModuleDBRootCount{module_num}-3 not found in SystemModuleConfig" + ) + + # Remove existing dbroot IDs + for i in range(1, 100): + dbroot_id_node = sysconf_node.find(f"./ModuleDBRootID{module_num}-{i}-3") + if dbroot_id_node is not None: + sysconf_node.remove(dbroot_id_node) \ No newline at end of file diff --git a/cmapi/cmapi_server/test/test_cluster.py b/cmapi/cmapi_server/test/test_cluster.py index 903ee4e1e..a3f1980fb 100644 --- a/cmapi/cmapi_server/test/test_cluster.py +++ b/cmapi/cmapi_server/test/test_cluster.py @@ -203,10 +203,6 @@ class ClusterAddNodeTestCase(BaseClusterTestCase): ['pgrep', MCSProgs.CONTROLLER_NODE.value]) self.assertIsNotNone(controllernode) - # Check that WriteEngineServer was started - wes = subprocess.check_output(['pgrep', MCSProgs.WRITE_ENGINE_SERVER.value]) - self.assertIsNotNone(wes) - class ClusterRemoveNodeTestCase(BaseClusterTestCase): URL = ClusterAddNodeTestCase.URL diff --git a/cmapi/cmapi_server/test/test_node_manip.py b/cmapi/cmapi_server/test/test_node_manip.py index bfdf12262..d52953ed5 100644 --- a/cmapi/cmapi_server/test/test_node_manip.py +++ b/cmapi/cmapi_server/test/test_node_manip.py @@ -1,12 +1,13 @@ import logging import socket - +import unittest from unittest.mock import patch from lxml import etree from cmapi_server import node_manipulation from cmapi_server.constants import MCS_DATA_PATH from cmapi_server.helpers import get_read_only_nodes +from cmapi_server.node_manipulation import add_dbroots_of_other_nodes, remove_dbroots_of_node from cmapi_server.test.unittest_global import ( tmp_mcs_config_filename, BaseNodeManipTestCase ) @@ -68,7 +69,9 @@ class NodeManipTester(BaseNodeManipTestCase): # Mock _rebalance_dbroots and _move_primary_node (only after the first node is added) with patch('cmapi_server.node_manipulation._rebalance_dbroots') as mock_rebalance_dbroots, \ - patch('cmapi_server.node_manipulation._move_primary_node') as mock_move_primary_node: + patch('cmapi_server.node_manipulation._move_primary_node') as mock_move_primary_node, \ + patch('cmapi_server.node_manipulation.add_dbroots_of_other_nodes') as mock_add_dbroots_of_other_nodes, \ + patch('cmapi_server.node_manipulation.remove_dbroots_of_node') as mock_remove_dbroots_of_node: # Add a read-only node node_manipulation.add_node( @@ -92,9 +95,9 @@ class NodeManipTester(BaseNodeManipTestCase): wes_node = root.find('./pm2_WriteEngineServer') self.assertIsNone(wes_node) - # Check that the dbroot related methods were not called mock_rebalance_dbroots.assert_not_called() mock_move_primary_node.assert_not_called() + mock_add_dbroots_of_other_nodes.assert_called_once_with(root, 2) # Test read-only node removal node_manipulation.remove_node( @@ -106,9 +109,9 @@ class NodeManipTester(BaseNodeManipTestCase): read_only_nodes = get_read_only_nodes(root) self.assertEqual(len(read_only_nodes), 0) - # Check that dbroot related methods were not called mock_rebalance_dbroots.assert_not_called() mock_move_primary_node.assert_not_called() + mock_remove_dbroots_of_node.assert_called_once_with(root, 2) def test_add_dbroots_nodes_rebalance(self): @@ -268,3 +271,59 @@ class NodeManipTester(BaseNodeManipTestCase): caught_it = True self.assertTrue(caught_it) + + +class TestReadOnlyNodeDBRootsManip(unittest.TestCase): + our_module_idx = 2 + + def setUp(self): + # Mock initial XML structure (add two dbroots) + self.root = etree.Element('Columnstore') + etree.SubElement(self.root, 'SystemModuleConfig') + system_config = etree.SubElement(self.root, 'SystemConfig') + dbroot_count = etree.SubElement(system_config, 'DBRootCount') + dbroot_count.text = '2' + dbroot1 = etree.SubElement(system_config, 'DBRoot1') + dbroot1.text = '/data/dbroot1' + dbroot2 = etree.SubElement(system_config, 'DBRoot2') + dbroot2.text = '/data/dbroot2' + + def test_add_dbroots_of_other_nodes(self): + '''add_dbroots_of_other_nodes must add dbroots of other nodes into mapping of the node.''' + add_dbroots_of_other_nodes(self.root, self.our_module_idx) + + # Check that ModuleDBRootCount of the module was updated + module_count = self.root.find(f'./SystemModuleConfig/ModuleDBRootCount{self.our_module_idx}-3') + self.assertIsNotNone(module_count) + self.assertEqual(module_count.text, '2') + + # Check that dbroots were added to ModuleDBRootID{module_num}-{i}-3 + dbroot1 = self.root.find(f'./SystemModuleConfig/ModuleDBRootID{self.our_module_idx}-1-3') + dbroot2 = self.root.find(f'./SystemModuleConfig/ModuleDBRootID{self.our_module_idx}-2-3') + self.assertIsNotNone(dbroot1) + self.assertIsNotNone(dbroot2) + self.assertEqual(dbroot1.text, '1') + self.assertEqual(dbroot2.text, '2') + + def test_remove_dbroots_of_node(self): + '''Test that remove_dbroots_of_node correctly removes dbroots from the node's mapping''' + # Add dbroot association to the node + smc = self.root.find('./SystemModuleConfig') + dbroot1 = etree.SubElement(smc, f'ModuleDBRootID{self.our_module_idx}-1-3') + dbroot1.text = '1' + dbroot2 = etree.SubElement(smc, f'ModuleDBRootID{self.our_module_idx}-2-3') + dbroot2.text = '2' + # Add ModuleDBRootCount to the node + module_count = etree.SubElement(smc, f'ModuleDBRootCount{self.our_module_idx}-3') + module_count.text = '2' + + remove_dbroots_of_node(self.root, self.our_module_idx) + + # Check that ModuleDBRootCount was removed + module_count = self.root.find(f'./SystemModuleConfig/ModuleDBRootCount{self.our_module_idx}-3') + self.assertIsNone(module_count) + # Check that dbroot mappings of the module were removed + dbroot1 = self.root.find(f'./SystemModuleConfig/ModuleDBRootID{self.our_module_idx}-1-3') + dbroot2 = self.root.find(f'./SystemModuleConfig/ModuleDBRootID{self.our_module_idx}-2-3') + self.assertIsNone(dbroot1) + self.assertIsNone(dbroot2)