From 4f6aa4af98f262c3988388835f03ced504f787f1 Mon Sep 17 00:00:00 2001 From: Lars Kellogg-Stedman Date: Thu, 23 Jan 2025 20:56:49 -0500 Subject: [PATCH] Make "esi mdc baremetal node list" work OpenStackConfig.get_all() will fail with a keystone exception if there are any OS_* variables set in the environment [1]. This is easy to trigger if, for example, you have set OS_BAREMETAL_API_VERSION in your environment to work around [2]. We can avoid the problem by removing the call to get_all(), and instead initializing the list of clouds using get_cloud_names(). This commit makes a number of additional changes to esiclient/v1/mdc/mdc_node_baremetal.py: - Several attribute names appear to have changed since this code was written (node.uuid is now node.id; node.instance_uuid is now node.instance_id). - Replace single-letter variable names with more meaningful names. - Add the '--ignore-invalid' command, which allows the command to continue in the event that it cannot successfully connect to one or more clouds. [1]: https://bugs.launchpad.net/openstacksdk/+bug/2096621 [2]: https://github.com/CCI-MOC/esi/issues/669 --- .../unit/v1/mdc/test_mdc_node_baremetal.py | 155 ++++++------------ esiclient/v1/mdc/mdc_node_baremetal.py | 47 +++--- 2 files changed, 72 insertions(+), 130 deletions(-) diff --git a/esiclient/tests/unit/v1/mdc/test_mdc_node_baremetal.py b/esiclient/tests/unit/v1/mdc/test_mdc_node_baremetal.py index 8499fb5..00e98da 100644 --- a/esiclient/tests/unit/v1/mdc/test_mdc_node_baremetal.py +++ b/esiclient/tests/unit/v1/mdc/test_mdc_node_baremetal.py @@ -11,89 +11,55 @@ # under the License. # -import mock -import munch +from unittest import mock +import openstack.baremetal.v1.node +import openstack.connection from esiclient.tests.unit import base from esiclient.v1.mdc import mdc_node_baremetal +@mock.patch("esiclient.v1.mdc.mdc_node_baremetal.openstack.connect") +@mock.patch("openstack.config.loader.OpenStackConfig.get_cloud_names") class TestMDCBaremetalNodeList(base.TestCommand): def setUp(self): - super(TestMDCBaremetalNodeList, self).setUp() + super().setUp() self.cmd = mdc_node_baremetal.MDCBaremetalNodeList(self.app, None) - - class FakeConfig(object): - def __init__(self, name, region): - self.name = name - self.config = {"region_name": region} - - self.cloud1 = FakeConfig("esi", "regionOne") - self.cloud2 = FakeConfig("esi", "regionTwo") - self.cloud3 = FakeConfig("esi2", "regionOne") - - self.node = munch.Munch( - uuid="node_uuid_1", - name="node1", - instance_uuid="instance_uuid_1", - power_state="off", - provision_state="active", - maintenance=False, - ) - - self.node2 = munch.Munch( - uuid="node_uuid_2", - name="node2", - instance_uuid="instance_uuid_2", - power_state="off", - provision_state="active", - maintenance=False, - ) - - self.node3 = munch.Munch( - uuid="node_uuid_3", - name="node3", - instance_uuid="instance_uuid_3", - power_state="off", - provision_state="active", - maintenance=False, - ) - - self.node4 = munch.Munch( - uuid="node_uuid_4", - name="node4", - instance_uuid="instance_uuid_4", - power_state="off", - provision_state="active", - maintenance=False, - ) - - @mock.patch("esiclient.v1.mdc.mdc_node_baremetal.openstack.connect") - @mock.patch( - "esiclient.v1.mdc.mdc_node_baremetal.openstack.config.loader." - "OpenStackConfig.get_all_clouds" - ) - def test_take_action_list(self, mock_config, mock_connect): - mock_cloud = mock.Mock() - mock_cloud.list_machines.side_effect = [ - [self.node], - [self.node2], - [self.node3, self.node4], + self.cloud_names = [f"esi{i}" for i in range(1, 4)] + self.connection = mock.Mock(openstack.connection.Connection, name="test-cloud") + + self.node = {} + for i in range(1, 5): + node = self.node[i] = mock.Mock( + openstack.baremetal.v1.node.Node, name=f"node{i}" + ) + node.configure_mock( + id=f"node_uuid_{i}", + name=f"node{i}", + instance_id=f"instance_uuid_{i}", + power_state="off", + provision_state="active", + is_maintenance=False, + ) + + def test_take_action_list_all_clouds(self, mock_get_cloud_names, mock_connect): + self.connection.list_machines.side_effect = [ + [self.node[1]], + [self.node[2]], + [self.node[3], self.node[4]], ] - mock_connect.return_value = mock_cloud - - mock_config.return_value = [self.cloud1, self.cloud2, self.cloud3] + mock_connect.return_value = self.connection + mock_get_cloud_names.return_value = self.cloud_names arglist = [] verifylist = [] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - results = self.cmd.take_action(parsed_args) + expected = ( [ "Cloud", - "Region", "UUID", "Name", "Instance UUID", @@ -103,8 +69,7 @@ def test_take_action_list(self, mock_config, mock_connect): ], [ [ - self.cloud1.name, - self.cloud1.config["region_name"], + "esi1", "node_uuid_1", "node1", "instance_uuid_1", @@ -113,8 +78,7 @@ def test_take_action_list(self, mock_config, mock_connect): False, ], [ - self.cloud2.name, - self.cloud2.config["region_name"], + "esi2", "node_uuid_2", "node2", "instance_uuid_2", @@ -123,8 +87,7 @@ def test_take_action_list(self, mock_config, mock_connect): False, ], [ - self.cloud3.name, - self.cloud3.config["region_name"], + "esi3", "node_uuid_3", "node3", "instance_uuid_3", @@ -133,8 +96,7 @@ def test_take_action_list(self, mock_config, mock_connect): False, ], [ - self.cloud3.name, - self.cloud3.config["region_name"], + "esi3", "node_uuid_4", "node4", "instance_uuid_4", @@ -145,26 +107,19 @@ def test_take_action_list(self, mock_config, mock_connect): ], ) self.assertEqual(expected, results) - mock_config.assert_called_once() - assert mock_cloud.list_machines.call_count == 3 - - @mock.patch("esiclient.v1.mdc.mdc_node_baremetal.openstack.connect") - @mock.patch( - "esiclient.v1.mdc.mdc_node_baremetal.openstack.config.loader." - "OpenStackConfig.get_all_clouds" - ) - def test_take_action_list_cloud(self, mock_config, mock_connect): - mock_cloud = mock.Mock() - mock_cloud.list_machines.side_effect = [ - [self.node], - [self.node2], - [self.node3, self.node4], + mock_get_cloud_names.assert_called_once() + assert self.connection.list_machines.call_count == 3 + + def test_take_action_list_specific_cloud(self, mock_get_cloud_names, mock_connect): + self.connection.list_machines.side_effect = [ + [self.node[1]], + [self.node[2]], + [self.node[3], self.node[4]], ] - mock_connect.return_value = mock_cloud + mock_connect.return_value = self.connection + mock_get_cloud_names.return_value = self.cloud_names - mock_config.return_value = [self.cloud1, self.cloud2, self.cloud3] - - arglist = ["--clouds", "esi"] + arglist = ["esi1"] verifylist = [] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -173,7 +128,6 @@ def test_take_action_list_cloud(self, mock_config, mock_connect): expected = ( [ "Cloud", - "Region", "UUID", "Name", "Instance UUID", @@ -183,8 +137,7 @@ def test_take_action_list_cloud(self, mock_config, mock_connect): ], [ [ - self.cloud1.name, - self.cloud1.config["region_name"], + "esi1", "node_uuid_1", "node1", "instance_uuid_1", @@ -192,18 +145,8 @@ def test_take_action_list_cloud(self, mock_config, mock_connect): "active", False, ], - [ - self.cloud2.name, - self.cloud2.config["region_name"], - "node_uuid_2", - "node2", - "instance_uuid_2", - "off", - "active", - False, - ], ], ) self.assertEqual(expected, results) - mock_config.assert_called_once() - assert mock_cloud.list_machines.call_count == 2 + mock_get_cloud_names.assert_called_once() + assert self.connection.list_machines.call_count == 1 diff --git a/esiclient/v1/mdc/mdc_node_baremetal.py b/esiclient/v1/mdc/mdc_node_baremetal.py index 64ee56d..ed3ecdb 100644 --- a/esiclient/v1/mdc/mdc_node_baremetal.py +++ b/esiclient/v1/mdc/mdc_node_baremetal.py @@ -24,13 +24,14 @@ class MDCBaremetalNodeList(command.Lister): auth_required = False def get_parser(self, prog_name): - parser = super(MDCBaremetalNodeList, self).get_parser(prog_name) + parser = super().get_parser(prog_name) + parser.add_argument("--ignore-invalid", "-i", action="store_true") parser.add_argument( - "--clouds", - dest="clouds", + "clouds", metavar="", - nargs="+", + nargs="*", help=_("Specify the cloud to use from clouds.yaml."), + default=openstack.config.OpenStackConfig().get_cloud_names(), ) return parser @@ -38,7 +39,6 @@ def get_parser(self, prog_name): def take_action(self, parsed_args): columns = [ "Cloud", - "Region", "UUID", "Name", "Instance UUID", @@ -48,27 +48,26 @@ def take_action(self, parsed_args): ] data = [] - cloud_regions = openstack.config.loader.OpenStackConfig().get_all_clouds() - if parsed_args.clouds: - cloud_regions = filter( - lambda c: c.name in parsed_args.clouds, cloud_regions - ) - for c in cloud_regions: - nodes = openstack.connect( - cloud=c.name, region=c.config["region_name"] - ).list_machines() - for n in nodes: - data.append( + for cloud in parsed_args.clouds: + try: + data.extend( [ - c.name, - c.config["region_name"], - n.uuid, - n.name, - n.instance_uuid, - n.power_state, - n.provision_state, - n.maintenance, + cloud, + node.id, + node.name, + node.instance_id, + node.power_state, + node.provision_state, + node.is_maintenance, ] + for node in openstack.connect(cloud=cloud).list_machines() ) + except Exception as err: + if parsed_args.ignore_invalid: + self.log.error( + "failed to retrieve information for cloud %s: %s", cloud, err + ) + continue + raise return columns, data