From 740aded32ad394b9499483da236e9adfb21b3ce5 Mon Sep 17 00:00:00 2001 From: nicholasyang Date: Fri, 26 Jul 2024 17:49:39 +0800 Subject: [PATCH] Dev: ui_corosync: refine the error messages for missing or duplicated nodes (jsc#PED-8083) --- crmsh/corosync.py | 23 ++++++++++++++----- crmsh/ui_corosync.py | 39 ++++++++++++++++++++++++--------- test/unittests/test_corosync.py | 12 +++++----- 3 files changed, 53 insertions(+), 21 deletions(-) diff --git a/crmsh/corosync.py b/crmsh/corosync.py index 526065632..09d70eed1 100644 --- a/crmsh/corosync.py +++ b/crmsh/corosync.py @@ -5,6 +5,7 @@ configuration file, and also the corosync-* utilities. ''' import dataclasses +import ipaddress import itertools import os import re @@ -582,6 +583,19 @@ def __load_option(self, data: dict[str, str], field: dataclasses.Field): class LinkManager: + class LinkManageException(Exception): + pass + + @dataclasses.dataclass + class MissingNodesException(LinkManageException): + nodeids: list[int] + + @dataclasses.dataclass + class DuplicatedNodeAddressException(LinkManageException): + address: str + node1: int + node2: int + LINK_OPTIONS_UPDATABLE = { field.name for field in dataclasses.fields(Link) @@ -754,7 +768,7 @@ def __upsert_node_addr_impl( # need to change uniqueness existing = existing_addr_node_map.get(canonical_addr, None) if existing is not None: - raise ValueError(f'Duplicated node address: {addr}: already used by node {existing}') + raise LinkManager.DuplicatedNodeAddressException(addr, nodeid, existing) found.addr = addr existing_addr_node_map[canonical_addr] = found.nodeid nodes = config['nodelist']['node'] @@ -769,10 +783,9 @@ def add_link(self, node_addresses: typing.Mapping[int, str], options: dict[str, links = self.links() if len(links) >= KNET_LINK_NUM_LIMIT: raise ValueError(f'Cannot add a new link. The maximum number of links supported is {KNET_LINK_NUM_LIMIT}.') - node_ids = {node.nodeid for node in links[0].nodes} - for nodeid in node_ids: - if nodeid not in node_addresses: - raise ValueError(f'The address of node {nodeid} is not specified.') + unspecified_nodes = [node.nodeid for node in links[0].nodes if node.nodeid not in node_addresses] + if unspecified_nodes: + raise self.MissingNodesException(unspecified_nodes) next_linknumber = len(links) links.append(Link(next_linknumber, [dataclasses.replace(node, addr='') for node in links[0].nodes])) self.__upsert_node_addr_impl(self._config, links, next_linknumber, node_addresses) diff --git a/crmsh/ui_corosync.py b/crmsh/ui_corosync.py index 8358bd1c9..ef85669ac 100644 --- a/crmsh/ui_corosync.py +++ b/crmsh/ui_corosync.py @@ -162,11 +162,17 @@ def do_update(self, context, *argv): if not args.nodes and not args.options: logger.info("Nothing is updated.") else: - self.__save_changed_config( - lm, - lm.update_node_addr(args.linknumber, node_addresses), - reload=not args.nodes, # changes to node addresses are not hot reloadable - ) + try: + self.__save_changed_config( + lm, + lm.update_node_addr(args.linknumber, node_addresses), + reload=not args.nodes, # changes to node addresses are not hot reloadable + ) + except corosync.LinkManager.DuplicatedNodeAddressException as e: + node1 = next(x.name for x in nodes if x.nodeid == e.node1) + node2 = next(x.name for x in nodes if x.nodeid == e.node2) + logger.error('Duplicated ip address %s: used for both %s and %s.', e.address, e.node1, e.node2) + return False def do_add(self, context, *argv): lm = self.__load_and_validate_links() @@ -184,11 +190,24 @@ def do_add(self, context, *argv): if nodeid == -1: logger.error(f'Unknown node {name}.') node_addresses[nodeid] = addr - self.__save_changed_config( - lm, - lm.add_link(node_addresses, args.options), - reload=True, - ) + try: + self.__save_changed_config( + lm, + lm.add_link(node_addresses, args.options), + reload=True, + ) + except corosync.LinkManager.MissingNodesException as e: + for nodeid in e.nodeids: + name = next(x.name for x in nodes if x.nodeid == nodeid) + logger.error('The address of node %s is not specified.', name) + logger.error('The address of all nodes must be specified when adding a new link.') + return False + except corosync.LinkManager.DuplicatedNodeAddressException as e: + node1 = next(x.name for x in nodes if x.nodeid == e.node1) + node2 = next(x.name for x in nodes if x.nodeid == e.node2) + logger.error('Duplicated ip address %s=%s: already used for %s=%s.', e.node1, e.address, e.node2, e.address) + return False + @command.completer(completers.call(lambda: [ str(link.linknumber) diff --git a/test/unittests/test_corosync.py b/test/unittests/test_corosync.py index 5a4f0927d..1ad786f1c 100644 --- a/test/unittests/test_corosync.py +++ b/test/unittests/test_corosync.py @@ -512,13 +512,13 @@ def test_update_unchanged_addr(self): self.assertListEqual(self.ORIGINAL['nodelist']['node'], self.lm._config['nodelist']['node']) def test_update_duplicated_addr(self): - with self.assertRaises(ValueError): + with self.assertRaises(corosync.LinkManager.DuplicatedNodeAddressException): self.lm.update_node_addr(1, {1: "192.0.2.1"}) - with self.assertRaises(ValueError): + with self.assertRaises(corosync.LinkManager.DuplicatedNodeAddressException): self.lm.update_node_addr(1, {1: "192.0.2.2"}) - with self.assertRaises(ValueError): + with self.assertRaises(corosync.LinkManager.DuplicatedNodeAddressException): self.lm.update_node_addr(1, {1: "192.0.2.102"}) - with self.assertRaises(ValueError): + with self.assertRaises(corosync.LinkManager.DuplicatedNodeAddressException): self.lm.update_node_addr( 1, { @@ -527,7 +527,7 @@ def test_update_duplicated_addr(self): 3: "192.0.2.65", } ) - with self.assertRaises(ValueError): + with self.assertRaises(corosync.LinkManager.DuplicatedNodeAddressException): self.lm.update_node_addr( 1, { @@ -638,7 +638,7 @@ def test_unspecified_node(self, mock_links, mock_upsert_node, mock_update_link): corosync.LinkNode(2, 'node2', '192.0.2.102'), ])] lm = corosync.LinkManager(dict()) - with self.assertRaises(ValueError): + with self.assertRaises(corosync.LinkManager.MissingNodesException): lm.add_link({1: '192.0.2.201'}, dict()) mock_upsert_node.assert_not_called() mock_update_link.assert_not_called()