From bced4852c7a92959f5457360dd91eb397af4d270 Mon Sep 17 00:00:00 2001 From: Pedro Martins Date: Sat, 31 Aug 2019 20:10:47 -0300 Subject: [PATCH 001/586] Add port ranges on floating ip portforwardings cli This patch is one of a series of patches to implement floating ip port forwarding with port ranges. The specification is defined in: https://github.com/openstack/neutron-specs/blob/master/specs/wallaby/port-forwarding-port-ranges.rst Change-Id: If9679c87fd8b770fcd960048e091ee8d59205285 Implements: blueprint floatingips-portforwarding-ranges Related-Bug: #1885921 --- .../network/v2/floating_ip_port_forwarding.py | 102 +++++--- .../tests/unit/network/v2/fakes.py | 38 ++- .../v2/test_floating_ip_port_forwarding.py | 231 ++++++++++++++++-- ...t-forwarding-command-8c6ee05cf625578a.yaml | 4 + 4 files changed, 318 insertions(+), 57 deletions(-) create mode 100644 releasenotes/notes/add-port-ranges-in-port-forwarding-command-8c6ee05cf625578a.yaml diff --git a/openstackclient/network/v2/floating_ip_port_forwarding.py b/openstackclient/network/v2/floating_ip_port_forwarding.py index b33633d34..cd71c05a9 100644 --- a/openstackclient/network/v2/floating_ip_port_forwarding.py +++ b/openstackclient/network/v2/floating_ip_port_forwarding.py @@ -25,6 +25,61 @@ LOG = logging.getLogger(__name__) +def validate_ports_diff(ports): + if len(ports) == 0: + return 0 + + ports_diff = ports[-1] - ports[0] + if ports_diff < 0: + msg = _("The last number in port range must be" + " greater or equal to the first") + raise exceptions.CommandError(msg) + return ports_diff + + +def validate_ports_match(internal_ports, external_ports): + internal_ports_diff = validate_ports_diff(internal_ports) + external_ports_diff = validate_ports_diff(external_ports) + + if internal_ports_diff != 0 and internal_ports_diff != external_ports_diff: + msg = _("The relation between internal and external ports does not " + "match the pattern 1:N and N:N") + raise exceptions.CommandError(msg) + + +def validate_and_assign_port_ranges(parsed_args, attrs): + internal_port_range = parsed_args.internal_protocol_port + external_port_range = parsed_args.external_protocol_port + external_ports = internal_ports = [] + if external_port_range: + external_ports = list(map(int, str(external_port_range).split(':'))) + if internal_port_range: + internal_ports = list(map(int, str(internal_port_range).split(':'))) + + validate_ports_match(internal_ports, external_ports) + + for port in external_ports + internal_ports: + validate_port(port) + + if internal_port_range: + if ':' in internal_port_range: + attrs['internal_port_range'] = internal_port_range + else: + attrs['internal_port'] = int(internal_port_range) + + if external_port_range: + if ':' in external_port_range: + attrs['external_port_range'] = external_port_range + else: + attrs['external_port'] = int(external_port_range) + + +def validate_port(port): + if port <= 0 or port > 65535: + msg = _("The port number range is <1-65535>") + raise exceptions.CommandError(msg) + + def _get_columns(item): column_map = {} hidden_columns = ['location'] @@ -58,7 +113,6 @@ def get_parser(self, prog_name): ) parser.add_argument( '--internal-protocol-port', - type=int, metavar='', required=True, help=_("The protocol port number " @@ -67,7 +121,6 @@ def get_parser(self, prog_name): ) parser.add_argument( '--external-protocol-port', - type=int, metavar='', required=True, help=_("The protocol port number of " @@ -92,6 +145,7 @@ def get_parser(self, prog_name): help=_("Floating IP that the port forwarding belongs to " "(IP address or ID)") ) + return parser def take_action(self, parsed_args): @@ -102,19 +156,7 @@ def take_action(self, parsed_args): ignore_missing=False, ) - if parsed_args.internal_protocol_port is not None: - if (parsed_args.internal_protocol_port <= 0 or - parsed_args.internal_protocol_port > 65535): - msg = _("The port number range is <1-65535>") - raise exceptions.CommandError(msg) - attrs['internal_port'] = parsed_args.internal_protocol_port - - if parsed_args.external_protocol_port is not None: - if (parsed_args.external_protocol_port <= 0 or - parsed_args.external_protocol_port > 65535): - msg = _("The port number range is <1-65535>") - raise exceptions.CommandError(msg) - attrs['external_port'] = parsed_args.external_protocol_port + validate_and_assign_port_ranges(parsed_args, attrs) if parsed_args.port: port = client.find_port(parsed_args.port, @@ -226,7 +268,9 @@ def take_action(self, parsed_args): 'internal_port_id', 'internal_ip_address', 'internal_port', + 'internal_port_range', 'external_port', + 'external_port_range', 'protocol', 'description', ) @@ -235,7 +279,9 @@ def take_action(self, parsed_args): 'Internal Port ID', 'Internal IP Address', 'Internal Port', + 'Internal Port Range', 'External Port', + 'External Port Range', 'Protocol', 'Description', ) @@ -246,8 +292,13 @@ def take_action(self, parsed_args): port = client.find_port(parsed_args.port, ignore_missing=False) query['internal_port_id'] = port.id - if parsed_args.external_protocol_port is not None: - query['external_port'] = parsed_args.external_protocol_port + external_port = parsed_args.external_protocol_port + if external_port: + if ':' in external_port: + query['external_port_range'] = external_port + else: + query['external_port'] = int( + parsed_args.external_protocol_port) if parsed_args.protocol is not None: query['protocol'] = parsed_args.protocol @@ -297,14 +348,12 @@ def get_parser(self, prog_name): parser.add_argument( '--internal-protocol-port', metavar='', - type=int, help=_("The TCP/UDP/other protocol port number of the " "network port fixed IPv4 address associated to " "the floating IP port forwarding") ) parser.add_argument( '--external-protocol-port', - type=int, metavar='', help=_("The TCP/UDP/other protocol port number of the " "port forwarding's floating IP address") @@ -339,19 +388,8 @@ def take_action(self, parsed_args): if parsed_args.internal_ip_address: attrs['internal_ip_address'] = parsed_args.internal_ip_address - if parsed_args.internal_protocol_port is not None: - if (parsed_args.internal_protocol_port <= 0 or - parsed_args.internal_protocol_port > 65535): - msg = _("The port number range is <1-65535>") - raise exceptions.CommandError(msg) - attrs['internal_port'] = parsed_args.internal_protocol_port - - if parsed_args.external_protocol_port is not None: - if (parsed_args.external_protocol_port <= 0 or - parsed_args.external_protocol_port > 65535): - msg = _("The port number range is <1-65535>") - raise exceptions.CommandError(msg) - attrs['external_port'] = parsed_args.external_protocol_port + + validate_and_assign_port_ranges(parsed_args, attrs) if parsed_args.protocol: attrs['protocol'] = parsed_args.protocol diff --git a/openstackclient/tests/unit/network/v2/fakes.py b/openstackclient/tests/unit/network/v2/fakes.py index 912f451fe..bb113d3c9 100644 --- a/openstackclient/tests/unit/network/v2/fakes.py +++ b/openstackclient/tests/unit/network/v2/fakes.py @@ -1346,11 +1346,13 @@ class FakeFloatingIPPortForwarding(object): """"Fake one or more Port forwarding""" @staticmethod - def create_one_port_forwarding(attrs=None): + def create_one_port_forwarding(attrs=None, use_range=False): """Create a fake Port Forwarding. :param Dictionary attrs: A dictionary with all attributes + :param Boolean use_range: + A boolean which defines if we will use ranges or not :return: A FakeResource object with name, id, etc. """ @@ -1364,13 +1366,29 @@ def create_one_port_forwarding(attrs=None): 'floatingip_id': floatingip_id, 'internal_port_id': 'internal-port-id-' + uuid.uuid4().hex, 'internal_ip_address': '192.168.1.2', - 'internal_port': randint(1, 65535), - 'external_port': randint(1, 65535), 'protocol': 'tcp', 'description': 'some description', 'location': 'MUNCHMUNCHMUNCH', } + if use_range: + port_range = randint(0, 100) + internal_start = randint(1, 65535 - port_range) + internal_end = internal_start + port_range + internal_range = ':'.join(map(str, [internal_start, internal_end])) + external_start = randint(1, 65535 - port_range) + external_end = external_start + port_range + external_range = ':'.join(map(str, [external_start, external_end])) + port_forwarding_attrs['internal_port_range'] = internal_range + port_forwarding_attrs['external_port_range'] = external_range + port_forwarding_attrs['internal_port'] = None + port_forwarding_attrs['external_port'] = None + else: + port_forwarding_attrs['internal_port'] = randint(1, 65535) + port_forwarding_attrs['external_port'] = randint(1, 65535) + port_forwarding_attrs['internal_port_range'] = '' + port_forwarding_attrs['external_port_range'] = '' + # Overwrite default attributes. port_forwarding_attrs.update(attrs) @@ -1381,25 +1399,28 @@ def create_one_port_forwarding(attrs=None): return port_forwarding @staticmethod - def create_port_forwardings(attrs=None, count=2): + def create_port_forwardings(attrs=None, count=2, use_range=False): """Create multiple fake Port Forwarding. :param Dictionary attrs: A dictionary with all attributes :param int count: The number of Port Forwarding rule to fake + :param Boolean use_range: + A boolean which defines if we will use ranges or not :return: A list of FakeResource objects faking the Port Forwardings """ port_forwardings = [] for i in range(0, count): port_forwardings.append( - FakeFloatingIPPortForwarding.create_one_port_forwarding(attrs) + FakeFloatingIPPortForwarding.create_one_port_forwarding( + attrs, use_range=use_range) ) return port_forwardings @staticmethod - def get_port_forwardings(port_forwardings=None, count=2): + def get_port_forwardings(port_forwardings=None, count=2, use_range=False): """Get a list of faked Port Forwardings. If port forwardings list is provided, then initialize the Mock object @@ -1409,13 +1430,16 @@ def get_port_forwardings(port_forwardings=None, count=2): A list of FakeResource objects faking port forwardings :param int count: The number of Port Forwardings to fake + :param Boolean use_range: + A boolean which defines if we will use ranges or not :return: An iterable Mock object with side_effect set to a list of faked Port Forwardings """ if port_forwardings is None: port_forwardings = ( - FakeFloatingIPPortForwarding.create_port_forwardings(count) + FakeFloatingIPPortForwarding.create_port_forwardings( + count, use_range=use_range) ) return mock.Mock(side_effect=port_forwardings) diff --git a/openstackclient/tests/unit/network/v2/test_floating_ip_port_forwarding.py b/openstackclient/tests/unit/network/v2/test_floating_ip_port_forwarding.py index 7b9e3aa6e..4c82fd17b 100644 --- a/openstackclient/tests/unit/network/v2/test_floating_ip_port_forwarding.py +++ b/openstackclient/tests/unit/network/v2/test_floating_ip_port_forwarding.py @@ -49,6 +49,18 @@ def setUp(self): } ) ) + + self.new_port_forwarding_with_ranges = ( + network_fakes.FakeFloatingIPPortForwarding. + create_one_port_forwarding( + use_range=True, + attrs={ + 'internal_port_id': self.port.id, + 'floatingip_id': self.floating_ip.id, + } + ) + ) + self.network.create_floating_ip_port_forwarding = mock.Mock( return_value=self.new_port_forwarding) @@ -63,22 +75,26 @@ def setUp(self): self.columns = ( 'description', 'external_port', + 'external_port_range', 'floatingip_id', 'id', 'internal_ip_address', 'internal_port', 'internal_port_id', + 'internal_port_range', 'protocol' ) self.data = ( self.new_port_forwarding.description, self.new_port_forwarding.external_port, + self.new_port_forwarding.external_port_range, self.new_port_forwarding.floatingip_id, self.new_port_forwarding.id, self.new_port_forwarding.internal_ip_address, self.new_port_forwarding.internal_port, self.new_port_forwarding.internal_port_id, + self.new_port_forwarding.internal_port_range, self.new_port_forwarding.protocol, ) @@ -90,6 +106,160 @@ def test_create_no_options(self): self.assertRaises(tests_utils.ParserException, self.check_parser, self.cmd, arglist, verifylist) + def test_create_all_options_with_range(self): + arglist = [ + '--port', self.new_port_forwarding_with_ranges.internal_port_id, + '--internal-protocol-port', + self.new_port_forwarding_with_ranges.internal_port_range, + '--external-protocol-port', + self.new_port_forwarding_with_ranges.external_port_range, + '--protocol', self.new_port_forwarding_with_ranges.protocol, + self.new_port_forwarding_with_ranges.floatingip_id, + '--internal-ip-address', + self.new_port_forwarding_with_ranges.internal_ip_address, + '--description', + self.new_port_forwarding_with_ranges.description, + ] + verifylist = [ + ('port', self.new_port_forwarding_with_ranges.internal_port_id), + ('internal_protocol_port', + self.new_port_forwarding_with_ranges.internal_port_range), + ('external_protocol_port', + self.new_port_forwarding_with_ranges.external_port_range), + ('protocol', self.new_port_forwarding_with_ranges.protocol), + ('floating_ip', + self.new_port_forwarding_with_ranges.floatingip_id), + ('internal_ip_address', self.new_port_forwarding_with_ranges. + internal_ip_address), + ('description', self.new_port_forwarding_with_ranges.description), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + columns, data = self.cmd.take_action(parsed_args) + + self.network.create_floating_ip_port_forwarding.\ + assert_called_once_with( + self.new_port_forwarding.floatingip_id, + **{ + 'external_port_range': + self.new_port_forwarding_with_ranges. + external_port_range, + 'internal_ip_address': + self.new_port_forwarding_with_ranges. + internal_ip_address, + 'internal_port_range': + self.new_port_forwarding_with_ranges. + internal_port_range, + 'internal_port_id': + self.new_port_forwarding_with_ranges.internal_port_id, + 'protocol': self.new_port_forwarding_with_ranges.protocol, + 'description': + self.new_port_forwarding_with_ranges.description, + }) + self.assertEqual(self.columns, columns) + self.assertEqual(self.data, data) + + def test_create_all_options_with_range_invalid_port_exception(self): + invalid_port_range = '999999:999999' + arglist = [ + '--port', self.new_port_forwarding_with_ranges.internal_port_id, + '--internal-protocol-port', invalid_port_range, + '--external-protocol-port', invalid_port_range, + '--protocol', self.new_port_forwarding_with_ranges.protocol, + self.new_port_forwarding_with_ranges.floatingip_id, + '--internal-ip-address', + self.new_port_forwarding_with_ranges.internal_ip_address, + '--description', + self.new_port_forwarding_with_ranges.description, + ] + verifylist = [ + ('port', self.new_port_forwarding_with_ranges.internal_port_id), + ('internal_protocol_port', invalid_port_range), + ('external_protocol_port', invalid_port_range), + ('protocol', self.new_port_forwarding_with_ranges.protocol), + ('floating_ip', + self.new_port_forwarding_with_ranges.floatingip_id), + ('internal_ip_address', self.new_port_forwarding_with_ranges. + internal_ip_address), + ('description', self.new_port_forwarding_with_ranges.description), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + msg = 'The port number range is <1-65535>' + try: + self.cmd.take_action(parsed_args) + self.fail('CommandError should be raised.') + except exceptions.CommandError as e: + self.assertEqual(msg, str(e)) + self.network.create_floating_ip_port_forwarding.assert_not_called() + + def test_create_all_options_with_invalid_range_exception(self): + invalid_port_range = '80:70' + arglist = [ + '--port', self.new_port_forwarding_with_ranges.internal_port_id, + '--internal-protocol-port', invalid_port_range, + '--external-protocol-port', invalid_port_range, + '--protocol', self.new_port_forwarding_with_ranges.protocol, + self.new_port_forwarding_with_ranges.floatingip_id, + '--internal-ip-address', + self.new_port_forwarding_with_ranges.internal_ip_address, + '--description', + self.new_port_forwarding_with_ranges.description, + ] + verifylist = [ + ('port', self.new_port_forwarding_with_ranges.internal_port_id), + ('internal_protocol_port', invalid_port_range), + ('external_protocol_port', invalid_port_range), + ('protocol', self.new_port_forwarding_with_ranges.protocol), + ('floating_ip', + self.new_port_forwarding_with_ranges.floatingip_id), + ('internal_ip_address', self.new_port_forwarding_with_ranges. + internal_ip_address), + ('description', self.new_port_forwarding_with_ranges.description), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + msg = 'The last number in port range must be greater or equal to ' \ + 'the first' + try: + self.cmd.take_action(parsed_args) + self.fail('CommandError should be raised.') + except exceptions.CommandError as e: + self.assertEqual(msg, str(e)) + self.network.create_floating_ip_port_forwarding.assert_not_called() + + def test_create_all_options_with_unmatch_ranges_exception(self): + internal_range = '80:90' + external_range = '8080:8100' + arglist = [ + '--port', self.new_port_forwarding_with_ranges.internal_port_id, + '--internal-protocol-port', internal_range, + '--external-protocol-port', external_range, + '--protocol', self.new_port_forwarding_with_ranges.protocol, + self.new_port_forwarding_with_ranges.floatingip_id, + '--internal-ip-address', + self.new_port_forwarding_with_ranges.internal_ip_address, + '--description', + self.new_port_forwarding_with_ranges.description, + ] + verifylist = [ + ('port', self.new_port_forwarding_with_ranges.internal_port_id), + ('internal_protocol_port', internal_range), + ('external_protocol_port', external_range), + ('protocol', self.new_port_forwarding_with_ranges.protocol), + ('floating_ip', + self.new_port_forwarding_with_ranges.floatingip_id), + ('internal_ip_address', self.new_port_forwarding_with_ranges. + internal_ip_address), + ('description', self.new_port_forwarding_with_ranges.description), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + msg = "The relation between internal and external ports does not " \ + "match the pattern 1:N and N:N" + try: + self.cmd.take_action(parsed_args) + self.fail('CommandError should be raised.') + except exceptions.CommandError as e: + self.assertEqual(msg, str(e)) + self.network.create_floating_ip_port_forwarding.assert_not_called() + def test_create_all_options(self): arglist = [ '--port', self.new_port_forwarding.internal_port_id, @@ -106,8 +276,10 @@ def test_create_all_options(self): ] verifylist = [ ('port', self.new_port_forwarding.internal_port_id), - ('internal_protocol_port', self.new_port_forwarding.internal_port), - ('external_protocol_port', self.new_port_forwarding.external_port), + ('internal_protocol_port', + str(self.new_port_forwarding.internal_port)), + ('external_protocol_port', + str(self.new_port_forwarding.external_port)), ('protocol', self.new_port_forwarding.protocol), ('floating_ip', self.new_port_forwarding.floatingip_id), ('internal_ip_address', self.new_port_forwarding. @@ -253,7 +425,9 @@ class TestListFloatingIPPortForwarding(TestFloatingIPPortForwarding): 'Internal Port ID', 'Internal IP Address', 'Internal Port', + 'Internal Port Range', 'External Port', + 'External Port Range', 'Protocol', 'Description', ) @@ -275,7 +449,9 @@ def setUp(self): port_forwarding.internal_port_id, port_forwarding.internal_ip_address, port_forwarding.internal_port, + port_forwarding.internal_port_range, port_forwarding.external_port, + port_forwarding.external_port_range, port_forwarding.protocol, port_forwarding.description, )) @@ -330,7 +506,7 @@ def test_port_forwarding_list_all_options(self): query = { 'internal_port_id': self.port_forwardings[0].internal_port_id, - 'external_port': str(self.port_forwardings[0].external_port), + 'external_port': self.port_forwardings[0].external_port, 'protocol': self.port_forwardings[0].protocol, } @@ -392,7 +568,7 @@ def test_set_nothing(self): self.assertIsNone(result) def test_set_all_thing(self): - arglist = [ + arglist_single = [ '--port', self.port.id, '--internal-ip-address', 'new_internal_ip_address', '--internal-protocol-port', '100', @@ -402,21 +578,23 @@ def test_set_all_thing(self): self._port_forwarding.floatingip_id, self._port_forwarding.id, ] - verifylist = [ + arglist_range = list(arglist_single) + arglist_range[5] = '100:110' + arglist_range[7] = '200:210' + verifylist_single = [ ('port', self.port.id), ('internal_ip_address', 'new_internal_ip_address'), - ('internal_protocol_port', 100), - ('external_protocol_port', 200), + ('internal_protocol_port', '100'), + ('external_protocol_port', '200'), ('protocol', 'tcp'), ('description', 'some description'), ('floating_ip', self._port_forwarding.floatingip_id), ('port_forwarding_id', self._port_forwarding.id), ] - - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - result = self.cmd.take_action(parsed_args) - attrs = { + verifylist_range = list(verifylist_single) + verifylist_range[2] = ('internal_protocol_port', '100:110') + verifylist_range[3] = ('external_protocol_port', '200:210') + attrs_single = { 'internal_port_id': self.port.id, 'internal_ip_address': 'new_internal_ip_address', 'internal_port': 100, @@ -424,12 +602,25 @@ def test_set_all_thing(self): 'protocol': 'tcp', 'description': 'some description', } - self.network.update_floating_ip_port_forwarding.assert_called_with( - self._port_forwarding.floatingip_id, - self._port_forwarding.id, - **attrs - ) - self.assertIsNone(result) + attrs_range = dict(attrs_single, internal_port_range='100:110', + external_port_range='200:210') + attrs_range.pop('internal_port') + attrs_range.pop('external_port') + + def run_and_validate(arglist, verifylist, attrs): + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + self.network.update_floating_ip_port_forwarding.assert_called_with( + self._port_forwarding.floatingip_id, + self._port_forwarding.id, + **attrs + ) + self.assertIsNone(result) + + run_and_validate(arglist_single, verifylist_single, attrs_single) + run_and_validate(arglist_range, verifylist_range, attrs_range) class TestShowFloatingIPPortForwarding(TestFloatingIPPortForwarding): @@ -438,11 +629,13 @@ class TestShowFloatingIPPortForwarding(TestFloatingIPPortForwarding): columns = ( 'description', 'external_port', + 'external_port_range', 'floatingip_id', 'id', 'internal_ip_address', 'internal_port', 'internal_port_id', + 'internal_port_range', 'protocol', ) @@ -459,11 +652,13 @@ def setUp(self): self.data = ( self._port_forwarding.description, self._port_forwarding.external_port, + self._port_forwarding.external_port_range, self._port_forwarding.floatingip_id, self._port_forwarding.id, self._port_forwarding.internal_ip_address, self._port_forwarding.internal_port, self._port_forwarding.internal_port_id, + self._port_forwarding.internal_port_range, self._port_forwarding.protocol, ) self.network.find_floating_ip_port_forwarding = mock.Mock( diff --git a/releasenotes/notes/add-port-ranges-in-port-forwarding-command-8c6ee05cf625578a.yaml b/releasenotes/notes/add-port-ranges-in-port-forwarding-command-8c6ee05cf625578a.yaml new file mode 100644 index 000000000..80e4445e3 --- /dev/null +++ b/releasenotes/notes/add-port-ranges-in-port-forwarding-command-8c6ee05cf625578a.yaml @@ -0,0 +1,4 @@ +--- +features: + - | + Add port ranges support to the ``floating ip port forwarding`` commands. From 28ac0141b56b8c0943fb29733c1fd6c000856b17 Mon Sep 17 00:00:00 2001 From: "Dr. Jens Harbott" Date: Tue, 20 Sep 2022 20:20:11 +0200 Subject: [PATCH 002/586] Run swift in -tips job Since there is only py3 left, swift has learned to live with it, so we might as well test it. Change-Id: Iab5232858e4a67e356680d169a885875d574c3cc --- .zuul.yaml | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/.zuul.yaml b/.zuul.yaml index d27ae3dae..dc5ce44ce 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -149,22 +149,6 @@ vars: devstack_localrc: LIBS_FROM_GIT: python-openstackclient,openstacksdk,osc-lib,cliff - # This is insufficient, but leaving it here as a reminder of what may - # someday be all we need to make this work - # disable_python3_package swift - DISABLED_PYTHON3_PACKAGES: swift - devstack_services: - # Swift is not ready for python3 yet: At a minimum keystonemiddleware needs - # to be installed in the py2 env, there are probably other things too... - s-account: false - s-container: false - s-object: false - s-proxy: false - # As swift is not available for this job, c-bak service won't be functional. - # The backup related tests can be handled by other jobs having swift enabled. - # The backup service along with swift services can be enabled once swift is - # compatible with py3 - c-bak: false tox_envlist: functional tox_install_siblings: true From ce4cbeab67f6dbf87b0f15dabcff43488eca0050 Mon Sep 17 00:00:00 2001 From: Daniel Wilson Date: Tue, 18 Oct 2022 00:30:55 -0400 Subject: [PATCH 003/586] Use the compute SDK in usage commands Update usage list and usage show to use the compute component of the OpenStack SDK instead of directly using the nova interface. Change-Id: I1c4d2247c9c1a577ed9efad7e8332e7c9b974ad5 --- openstackclient/compute/v2/usage.py | 69 ++++++++----------- .../tests/unit/compute/v2/test_usage.py | 45 +++++------- 2 files changed, 48 insertions(+), 66 deletions(-) diff --git a/openstackclient/compute/v2/usage.py b/openstackclient/compute/v2/usage.py index 69fa04e8b..86f538a7d 100644 --- a/openstackclient/compute/v2/usage.py +++ b/openstackclient/compute/v2/usage.py @@ -15,12 +15,10 @@ """Usage action implementations""" -import collections import datetime import functools from cliff import columns as cliff_columns -from novaclient import api_versions from osc_lib.command import command from osc_lib import utils @@ -58,7 +56,7 @@ def human_readable(self): class CountColumn(cliff_columns.FormattableColumn): def human_readable(self): - return len(self._value) + return len(self._value) if self._value is not None else None class FloatColumn(cliff_columns.FormattableColumn): @@ -69,7 +67,7 @@ def human_readable(self): def _formatters(project_cache): return { - 'tenant_id': functools.partial( + 'project_id': functools.partial( ProjectColumn, project_cache=project_cache), 'server_usages': CountColumn, 'total_memory_mb_usage': FloatColumn, @@ -102,10 +100,10 @@ def _merge_usage(usage, next_usage): def _merge_usage_list(usages, next_usage_list): for next_usage in next_usage_list: - if next_usage.tenant_id in usages: - _merge_usage(usages[next_usage.tenant_id], next_usage) + if next_usage.project_id in usages: + _merge_usage(usages[next_usage.project_id], next_usage) else: - usages[next_usage.tenant_id] = next_usage + usages[next_usage.project_id] = next_usage class ListUsage(command.Lister): @@ -138,9 +136,9 @@ def _format_project(project): else: return project - compute_client = self.app.client_manager.compute + compute_client = self.app.client_manager.sdk_connection.compute columns = ( - "tenant_id", + "project_id", "server_usages", "total_memory_mb_usage", "total_vcpus_usage", @@ -154,36 +152,25 @@ def _format_project(project): "Disk GB-Hours" ) - dateformat = "%Y-%m-%d" + date_cli_format = "%Y-%m-%d" + date_api_format = "%Y-%m-%dT%H:%M:%S" now = datetime.datetime.utcnow() if parsed_args.start: - start = datetime.datetime.strptime(parsed_args.start, dateformat) + start = datetime.datetime.strptime( + parsed_args.start, date_cli_format) else: start = now - datetime.timedelta(weeks=4) if parsed_args.end: - end = datetime.datetime.strptime(parsed_args.end, dateformat) + end = datetime.datetime.strptime(parsed_args.end, date_cli_format) else: end = now + datetime.timedelta(days=1) - if compute_client.api_version < api_versions.APIVersion("2.40"): - usage_list = compute_client.usage.list(start, end, detailed=True) - else: - # If the number of instances used to calculate the usage is greater - # than CONF.api.max_limit, the usage will be split across multiple - # requests and the responses will need to be merged back together. - usages = collections.OrderedDict() - usage_list = compute_client.usage.list(start, end, detailed=True) - _merge_usage_list(usages, usage_list) - marker = _get_usage_list_marker(usage_list) - while marker: - next_usage_list = compute_client.usage.list( - start, end, detailed=True, marker=marker) - marker = _get_usage_list_marker(next_usage_list) - if marker: - _merge_usage_list(usages, next_usage_list) - usage_list = list(usages.values()) + usage_list = list(compute_client.usages( + start=start.strftime(date_api_format), + end=end.strftime(date_api_format), + detailed=True)) # Cache the project list project_cache = {} @@ -196,8 +183,8 @@ def _format_project(project): if parsed_args.formatter == 'table' and len(usage_list) > 0: self.app.stdout.write(_("Usage from %(start)s to %(end)s: \n") % { - "start": start.strftime(dateformat), - "end": end.strftime(dateformat), + "start": start.strftime(date_cli_format), + "end": end.strftime(date_cli_format), }) return ( @@ -239,17 +226,19 @@ def get_parser(self, prog_name): def take_action(self, parsed_args): identity_client = self.app.client_manager.identity - compute_client = self.app.client_manager.compute - dateformat = "%Y-%m-%d" + compute_client = self.app.client_manager.sdk_connection.compute + date_cli_format = "%Y-%m-%d" + date_api_format = "%Y-%m-%dT%H:%M:%S" now = datetime.datetime.utcnow() if parsed_args.start: - start = datetime.datetime.strptime(parsed_args.start, dateformat) + start = datetime.datetime.strptime( + parsed_args.start, date_cli_format) else: start = now - datetime.timedelta(weeks=4) if parsed_args.end: - end = datetime.datetime.strptime(parsed_args.end, dateformat) + end = datetime.datetime.strptime(parsed_args.end, date_cli_format) else: end = now + datetime.timedelta(days=1) @@ -262,19 +251,21 @@ def take_action(self, parsed_args): # Get the project from the current auth project = self.app.client_manager.auth_ref.project_id - usage = compute_client.usage.get(project, start, end) + usage = compute_client.get_usage( + project=project, start=start.strftime(date_api_format), + end=end.strftime(date_api_format)) if parsed_args.formatter == 'table': self.app.stdout.write(_( "Usage from %(start)s to %(end)s on project %(project)s: \n" ) % { - "start": start.strftime(dateformat), - "end": end.strftime(dateformat), + "start": start.strftime(date_cli_format), + "end": end.strftime(date_cli_format), "project": project, }) columns = ( - "tenant_id", + "project_id", "server_usages", "total_memory_mb_usage", "total_vcpus_usage", diff --git a/openstackclient/tests/unit/compute/v2/test_usage.py b/openstackclient/tests/unit/compute/v2/test_usage.py index bbccb9bdc..85b45e1b2 100644 --- a/openstackclient/tests/unit/compute/v2/test_usage.py +++ b/openstackclient/tests/unit/compute/v2/test_usage.py @@ -11,11 +11,8 @@ # under the License. # -import datetime from unittest import mock -from novaclient import api_versions - from openstackclient.compute.v2 import usage as usage_cmds from openstackclient.tests.unit.compute.v2 import fakes as compute_fakes from openstackclient.tests.unit.identity.v3 import fakes as identity_fakes @@ -26,8 +23,9 @@ class TestUsage(compute_fakes.TestComputev2): def setUp(self): super(TestUsage, self).setUp() - self.usage_mock = self.app.client_manager.compute.usage - self.usage_mock.reset_mock() + self.app.client_manager.sdk_connection = mock.Mock() + self.app.client_manager.sdk_connection.compute = mock.Mock() + self.sdk_client = self.app.client_manager.sdk_connection.compute self.projects_mock = self.app.client_manager.identity.projects self.projects_mock.reset_mock() @@ -38,7 +36,7 @@ class TestUsageList(TestUsage): project = identity_fakes.FakeProject.create_one_project() # Return value of self.usage_mock.list(). usages = compute_fakes.FakeUsage.create_usages( - attrs={'tenant_id': project.name}, count=1) + attrs={'project_id': project.name}, count=1) columns = ( "Project", @@ -49,7 +47,7 @@ class TestUsageList(TestUsage): ) data = [( - usage_cmds.ProjectColumn(usages[0].tenant_id), + usage_cmds.ProjectColumn(usages[0].project_id), usage_cmds.CountColumn(usages[0].server_usages), usage_cmds.FloatColumn(usages[0].total_memory_mb_usage), usage_cmds.FloatColumn(usages[0].total_vcpus_usage), @@ -59,7 +57,7 @@ class TestUsageList(TestUsage): def setUp(self): super(TestUsageList, self).setUp() - self.usage_mock.list.return_value = self.usages + self.sdk_client.usages.return_value = self.usages self.projects_mock.list.return_value = [self.project] # Get the command object to test @@ -97,9 +95,9 @@ def test_usage_list_with_options(self): columns, data = self.cmd.take_action(parsed_args) self.projects_mock.list.assert_called_with() - self.usage_mock.list.assert_called_with( - datetime.datetime(2016, 11, 11, 0, 0), - datetime.datetime(2016, 12, 20, 0, 0), + self.sdk_client.usages.assert_called_with( + start='2016-11-11T00:00:00', + end='2016-12-20T00:00:00', detailed=True) self.assertCountEqual(self.columns, columns) @@ -112,20 +110,13 @@ def test_usage_list_with_pagination(self): ('end', None), ] - self.app.client_manager.compute.api_version = api_versions.APIVersion( - '2.40') - self.usage_mock.list.reset_mock() - self.usage_mock.list.side_effect = [self.usages, []] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) columns, data = self.cmd.take_action(parsed_args) self.projects_mock.list.assert_called_with() - self.usage_mock.list.assert_has_calls([ - mock.call(mock.ANY, mock.ANY, detailed=True), - mock.call(mock.ANY, mock.ANY, detailed=True, - marker=self.usages[0]['server_usages'][0]['instance_id']) + self.sdk_client.usages.assert_has_calls([ + mock.call(start=mock.ANY, end=mock.ANY, detailed=True) ]) self.assertCountEqual(self.columns, columns) self.assertCountEqual(tuple(self.data), tuple(data)) @@ -136,7 +127,7 @@ class TestUsageShow(TestUsage): project = identity_fakes.FakeProject.create_one_project() # Return value of self.usage_mock.list(). usage = compute_fakes.FakeUsage.create_one_usage( - attrs={'tenant_id': project.name}) + attrs={'project_id': project.name}) columns = ( 'Project', @@ -147,7 +138,7 @@ class TestUsageShow(TestUsage): ) data = ( - usage_cmds.ProjectColumn(usage.tenant_id), + usage_cmds.ProjectColumn(usage.project_id), usage_cmds.CountColumn(usage.server_usages), usage_cmds.FloatColumn(usage.total_memory_mb_usage), usage_cmds.FloatColumn(usage.total_vcpus_usage), @@ -157,7 +148,7 @@ class TestUsageShow(TestUsage): def setUp(self): super(TestUsageShow, self).setUp() - self.usage_mock.get.return_value = self.usage + self.sdk_client.get_usage.return_value = self.usage self.projects_mock.get.return_value = self.project # Get the command object to test @@ -199,10 +190,10 @@ def test_usage_show_with_options(self): columns, data = self.cmd.take_action(parsed_args) - self.usage_mock.get.assert_called_with( - self.project.id, - datetime.datetime(2016, 11, 11, 0, 0), - datetime.datetime(2016, 12, 20, 0, 0)) + self.sdk_client.get_usage.assert_called_with( + project=self.project.id, + start='2016-11-11T00:00:00', + end='2016-12-20T00:00:00') self.assertEqual(self.columns, columns) self.assertEqual(self.data, data) From e76609650fd322cca6eb899aa6125c0450727b7c Mon Sep 17 00:00:00 2001 From: Artom Lifshitz Date: Wed, 26 Oct 2022 09:49:55 -0400 Subject: [PATCH 004/586] Improve `server dump create` helptext The `server dump create` command instructs Nova to trigger a crash dump in the guest OS. Assuming the guest supports this, the resulting dump file will be located in the guest, in a location dependent on the guest OS. Explain all that in the helptext. Story: 2010384 Change-Id: If940ed5cce6c5ab4193ab1494738149370da9aad --- openstackclient/compute/v2/server.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 1d0c03a27..01559b428 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -1756,8 +1756,9 @@ class CreateServerDump(command.Command): Trigger crash dump in server(s) with features like kdump in Linux. It will create a dump file in the server(s) dumping the server(s)' - memory, and also crash the server(s). OSC sees the dump file - (server dump) as a kind of resource. + memory, and also crash the server(s). This is contingent on guest operating + system support, and the location of the dump file inside the guest will + depend on the exact guest operating system. This command requires ``--os-compute-api-version`` 2.17 or greater. """ From a7975c42003d7df2af91154007435cd5f8560f24 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 26 Oct 2022 13:24:30 +0300 Subject: [PATCH 005/586] compute: Add '--no-network', '--auto-network' flags These are aliases for '--nic none' and '--nic auto', respectively. Change-Id: I7b4f7e5c3769a813bd8b2b9cd6090c6fe501e13d Signed-off-by: Stephen Finucane --- openstackclient/compute/v2/server.py | 110 ++++++++++++++---- .../tests/unit/compute/v2/test_server.py | 58 ++++++--- ...o-no-network-options-f4ddb2bb7544d2f5.yaml | 6 + 3 files changed, 136 insertions(+), 38 deletions(-) create mode 100644 releasenotes/notes/auto-no-network-options-f4ddb2bb7544d2f5.yaml diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 1d0c03a27..39b2bdc81 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -409,8 +409,8 @@ def get_parser(self, prog_name): '--tag', metavar='', help=_( - "Tag for the attached interface. " - "(Supported by API versions '2.49' - '2.latest')" + 'Tag for the attached interface ' + '(supported by --os-compute-api-version 2.49 or later)' ) ) return parser @@ -652,29 +652,68 @@ def take_action(self, parsed_args): ) -# TODO(stephenfin): Replace with 'MultiKeyValueAction' when we no longer -# support '--nic=auto' and '--nic=none' +class NoneNICAction(argparse.Action): + + def __init__(self, option_strings, dest, help=None): + super().__init__( + option_strings=option_strings, + dest=dest, + nargs=0, + default=[], + required=False, + help=help, + ) + + def __call__(self, parser, namespace, values, option_string=None): + # Make sure we have an empty dict rather than None + if getattr(namespace, self.dest, None) is None: + setattr(namespace, self.dest, []) + + getattr(namespace, self.dest).append('none') + + +class AutoNICAction(argparse.Action): + + def __init__(self, option_strings, dest, help=None): + super().__init__( + option_strings=option_strings, + dest=dest, + nargs=0, + default=[], + required=False, + help=help, + ) + + def __call__(self, parser, namespace, values, option_string=None): + # Make sure we have an empty dict rather than None + if getattr(namespace, self.dest, None) is None: + setattr(namespace, self.dest, []) + + getattr(namespace, self.dest).append('auto') + + class NICAction(argparse.Action): def __init__( self, option_strings, dest, - nargs=None, - const=None, - default=None, - type=None, - choices=None, - required=False, help=None, metavar=None, key=None, ): self.key = key super().__init__( - option_strings=option_strings, dest=dest, nargs=nargs, const=const, - default=default, type=type, choices=choices, required=required, - help=help, metavar=metavar, + option_strings=option_strings, + dest=dest, + nargs=None, + const=None, + default=[], + type=None, + choices=None, + required=False, + help=help, + metavar=metavar, ) def __call__(self, parser, namespace, values, option_string=None): @@ -707,7 +746,7 @@ def __call__(self, parser, namespace, values, option_string=None): } for kv_str in values.split(','): - k, sep, v = kv_str.partition("=") + k, sep, v = kv_str.partition('=') if k not in list(info) + ['tag'] or not v: msg = _( @@ -998,28 +1037,23 @@ def get_parser(self, prog_name): ) parser.add_argument( '--network', - metavar="", + metavar='', dest='nics', - default=[], action=NICAction, key='net-id', - # NOTE(RuiChen): Add '\n' to the end of line to improve formatting; - # see cliff's _SmartHelpFormatter for more details. help=_( "Create a NIC on the server and connect it to network. " "Specify option multiple times to create multiple NICs. " "This is a wrapper for the '--nic net-id=' " "parameter that provides simple syntax for the standard " "use case of connecting a new server to a given network. " - "For more advanced use cases, refer to the '--nic' " - "parameter." + "For more advanced use cases, refer to the '--nic' parameter." ), ) parser.add_argument( '--port', - metavar="", + metavar='', dest='nics', - default=[], action=NICAction, key='port-id', help=_( @@ -1031,13 +1065,41 @@ def get_parser(self, prog_name): "more advanced use cases, refer to the '--nic' parameter." ), ) + parser.add_argument( + '--no-network', + dest='nics', + action=NoneNICAction, + help=_( + "Do not attach a network to the server. " + "This is a wrapper for the '--nic none' option that provides " + "a simple syntax for disabling network connectivity for a new " + "server. " + "For more advanced use cases, refer to the '--nic' parameter. " + "(supported by --os-compute-api-version 2.37 or above)" + ), + ) + parser.add_argument( + '--auto-network', + dest='nics', + action=AutoNICAction, + help=_( + "Automatically allocate a network to the server. " + "This is the default network allocation policy. " + "This is a wrapper for the '--nic auto' option that provides " + "a simple syntax for enabling automatic configuration of " + "network connectivity for a new server. " + "For more advanced use cases, refer to the '--nic' parameter. " + "(supported by --os-compute-api-version 2.37 or above)" + ), + ) parser.add_argument( '--nic', metavar="", - action=NICAction, dest='nics', - default=[], + action=NICAction, + # NOTE(RuiChen): Add '\n' to the end of line to improve formatting; + # see cliff's _SmartHelpFormatter for more details. help=_( "Create a NIC on the server.\n" "NIC in the format:\n" diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 2e64e071e..f4fd44b17 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -1900,13 +1900,7 @@ def test_server_create_with_network_tag_pre_v243(self): self.assertRaises( exceptions.CommandError, self.cmd.take_action, parsed_args) - def test_server_create_with_auto_network(self): - arglist = [ - '--image', 'image1', - '--flavor', 'flavor1', - '--nic', 'auto', - self.new_server.name, - ] + def _test_server_create_with_auto_network(self, arglist): verifylist = [ ('image', 'image1'), ('flavor', 'flavor1'), @@ -1946,6 +1940,27 @@ def test_server_create_with_auto_network(self): self.assertEqual(self.columns, columns) self.assertEqual(self.datalist(), data) + # NOTE(stephenfin): '--auto-network' is an alias for '--nic auto' so the + # tests are nearly identical + + def test_server_create_with_auto_network_legacy(self): + arglist = [ + '--image', 'image1', + '--flavor', 'flavor1', + '--nic', 'auto', + self.new_server.name, + ] + self._test_server_create_with_auto_network(arglist) + + def test_server_create_with_auto_network(self): + arglist = [ + '--image', 'image1', + '--flavor', 'flavor1', + '--auto-network', + self.new_server.name, + ] + self._test_server_create_with_auto_network(arglist) + def test_server_create_with_auto_network_default_v2_37(self): """Tests creating a server without specifying --nic using 2.37.""" arglist = [ @@ -1996,13 +2011,7 @@ def test_server_create_with_auto_network_default_v2_37(self): self.assertEqual(self.columns, columns) self.assertEqual(self.datalist(), data) - def test_server_create_with_none_network(self): - arglist = [ - '--image', 'image1', - '--flavor', 'flavor1', - '--nic', 'none', - self.new_server.name, - ] + def _test_server_create_with_none_network(self, arglist): verifylist = [ ('image', 'image1'), ('flavor', 'flavor1'), @@ -2042,6 +2051,27 @@ def test_server_create_with_none_network(self): self.assertEqual(self.columns, columns) self.assertEqual(self.datalist(), data) + # NOTE(stephenfin): '--no-network' is an alias for '--nic none' so the + # tests are nearly identical + + def test_server_create_with_none_network_legacy(self): + arglist = [ + '--image', 'image1', + '--flavor', 'flavor1', + '--nic', 'none', + self.new_server.name, + ] + self._test_server_create_with_none_network(arglist) + + def test_server_create_with_none_network(self): + arglist = [ + '--image', 'image1', + '--flavor', 'flavor1', + '--no-network', + self.new_server.name, + ] + self._test_server_create_with_none_network(arglist) + def test_server_create_with_conflict_network_options(self): arglist = [ '--image', 'image1', diff --git a/releasenotes/notes/auto-no-network-options-f4ddb2bb7544d2f5.yaml b/releasenotes/notes/auto-no-network-options-f4ddb2bb7544d2f5.yaml new file mode 100644 index 000000000..3123d775a --- /dev/null +++ b/releasenotes/notes/auto-no-network-options-f4ddb2bb7544d2f5.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + The ``server create`` command now accepts two new options, ``--no-network`` + and ``--auto-network``. These are aliases for ``--nic none`` and + ``--nic auto``, respectively. From d7aa53b9a2bb1969abee90b9d0a547230886aa0f Mon Sep 17 00:00:00 2001 From: melanie witt Date: Mon, 7 Nov 2022 22:54:55 +0000 Subject: [PATCH 006/586] Add note about microversion 2.87 in server rescue help The ability to rescue a volume-backed server was added in compute microversion 2.87 [1]. This adds a note to the command help to improve user experience. [1] https://docs.openstack.org/nova/latest/user/rescue.html Change-Id: I5f40c3ca28e13bd1f979bc5f8c337302a3b9a5be --- openstackclient/compute/v2/server.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 1d0c03a27..97084e5a3 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -3645,7 +3645,11 @@ def take_action(self, parsed_args): class RescueServer(command.Command): - _description = _("Put server in rescue mode") + _description = _( + "Put server in rescue mode. " + "Specify ``--os-compute-api-version 2.87`` or higher to rescue a " + "server booted from a volume." + ) def get_parser(self, prog_name): parser = super(RescueServer, self).get_parser(prog_name) From 1d71479a4cea2a90eb2f244864fbcc665ed9484f Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 7 Nov 2022 16:35:56 +0000 Subject: [PATCH 007/586] zuul: Remove nova-network tests nova-network has been removed from nova for a very long time now and we've no way to test it in CI save for installing old versions of OpenStack. We don't care about this enough to do that, so just remove the thing. In the vein of things that have been removed, we also remove configuration that was supposed to enable cinder's v1 API but doesn't since the related knob was removed over 5 years ago [1]. [1] https://github.com/openstack/cinder/commit/3e91de956e1947a7014709010b99df380242ac74 Change-Id: I76efeccec04937c3a68108e2654872e00fadcec4 Signed-off-by: Stephen Finucane --- .zuul.yaml | 34 ---------------------------------- 1 file changed, 34 deletions(-) diff --git a/.zuul.yaml b/.zuul.yaml index 7883a4b19..95ab34aba 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -54,12 +54,6 @@ LIBS_FROM_GIT: python-openstackclient # NOTE(dtroyer): Functional tests need a bit more volume headroom VOLUME_BACKING_FILE_SIZE: 20G - devstack_local_conf: - post-config: - $CINDER_CONF: - DEFAULT: - # NOTE(dtroyer): OSC needs to support Volume v1 for a while yet so re-enable - enable_v1_api: true devstack_services: ceilometer-acentral: false ceilometer-acompute: false @@ -114,28 +108,6 @@ Q_ML2_PLUGIN_MECHANISM_DRIVERS: openvswitch tox_envlist: functional -- job: - name: osc-functional-devstack-n-net - parent: osc-functional-devstack-base - timeout: 7800 - vars: - devstack_localrc: - FLAT_INTERFACE: br_flat - PUBLIC_INTERFACE: br_pub - devstack_services: - n-cell: true - n-net: true - neutron: false - neutron-segments: false - q-agt: false - q-dhcp: false - q-l3: false - q-meta: false - q-metering: false - q-qos: false - q-svc: false - tox_envlist: functional - - job: name: osc-functional-devstack-tips parent: osc-functional-devstack @@ -232,12 +204,6 @@ jobs: - osc-build-image - osc-functional-devstack - # - osc-functional-devstack-n-net: - # voting: false - # # The job testing nova-network no longer works before Pike, and - # # should be disabled until the New Way of testing against old clouds - # # is ready and backported - # branches: ^(?!stable/(newton|ocata)).*$ - osc-functional-devstack-tips: # The functional-tips job only tests the latest and shouldn't be run # on the stable branches From ed0d568b948a04e893270d297c538773d058b73e Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 26 Oct 2022 18:17:38 +0300 Subject: [PATCH 008/586] compute: Fix '--network none/auto' handling This should lookup a network called 'none' or 'auto', not do the equivalent on '--nic none' or '--nic auto'. Correct this. Change-Id: I3c5acc49bfe8162d8fb6110603da56d56090b78f Signed-off-by: Stephen Finucane Story: 2010385 Task: 46658 --- openstackclient/compute/v2/server.py | 11 +++-- .../tests/unit/compute/v2/test_server.py | 49 ++++++++++++++----- 2 files changed, 43 insertions(+), 17 deletions(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 39b2bdc81..1d0724636 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -721,11 +721,6 @@ def __call__(self, parser, namespace, values, option_string=None): if getattr(namespace, self.dest, None) is None: setattr(namespace, self.dest, []) - # Handle the special auto/none cases - if values in ('auto', 'none'): - getattr(namespace, self.dest).append(values) - return - if self.key: if ',' in values or '=' in values: msg = _( @@ -735,6 +730,12 @@ def __call__(self, parser, namespace, values, option_string=None): raise argparse.ArgumentTypeError(msg % values) values = '='.join([self.key, values]) + else: + # Handle the special auto/none cases but only when a key isn't set + # (otherwise those could be valid values for the key) + if values in ('auto', 'none'): + getattr(namespace, self.dest).append(values) + return # We don't include 'tag' here by default since that requires a # particular microversion diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 858c9a2a6..48a36172b 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -1675,6 +1675,7 @@ def test_server_create_with_network(self): '--nic', 'net-id=net1,v4-fixed-ip=10.0.0.2', '--port', 'port1', '--network', 'net1', + '--network', 'auto', # this is a network called 'auto' '--nic', 'port-id=port2', self.new_server.name, ] @@ -1683,24 +1684,40 @@ def test_server_create_with_network(self): ('flavor', 'flavor1'), ('nics', [ { - 'net-id': 'net1', 'port-id': '', - 'v4-fixed-ip': '', 'v6-fixed-ip': '', + 'net-id': 'net1', + 'port-id': '', + 'v4-fixed-ip': '', + 'v6-fixed-ip': '', }, { - 'net-id': 'net1', 'port-id': '', - 'v4-fixed-ip': '10.0.0.2', 'v6-fixed-ip': '', + 'net-id': 'net1', + 'port-id': '', + 'v4-fixed-ip': '10.0.0.2', + 'v6-fixed-ip': '', }, { - 'net-id': '', 'port-id': 'port1', - 'v4-fixed-ip': '', 'v6-fixed-ip': '', + 'net-id': '', + 'port-id': 'port1', + 'v4-fixed-ip': '', + 'v6-fixed-ip': '', }, { - 'net-id': 'net1', 'port-id': '', - 'v4-fixed-ip': '', 'v6-fixed-ip': '', + 'net-id': 'net1', + 'port-id': '', + 'v4-fixed-ip': '', + 'v6-fixed-ip': '', }, { - 'net-id': '', 'port-id': 'port2', - 'v4-fixed-ip': '', 'v6-fixed-ip': '', + 'net-id': 'auto', + 'port-id': '', + 'v4-fixed-ip': '', + 'v6-fixed-ip': '', + }, + { + 'net-id': '', + 'port-id': 'port2', + 'v4-fixed-ip': '', + 'v6-fixed-ip': '', }, ]), ('config_drive', False), @@ -1729,12 +1746,16 @@ def test_server_create_with_network(self): "port2": port2_resource}[port_id]) # Mock sdk APIs. - _network = mock.Mock(id='net1_uuid') + _network_1 = mock.Mock(id='net1_uuid') + _network_auto = mock.Mock(id='auto_uuid') _port1 = mock.Mock(id='port1_uuid') _port2 = mock.Mock(id='port2_uuid') find_network = mock.Mock() find_port = mock.Mock() - find_network.return_value = _network + find_network.side_effect = lambda net_id, ignore_missing: { + "net1": _network_1, + "auto": _network_auto, + }[net_id] find_port.side_effect = (lambda port_id, ignore_missing: {"port1": _port1, "port2": _port2}[port_id]) @@ -1775,6 +1796,10 @@ def test_server_create_with_network(self): 'v4-fixed-ip': '', 'v6-fixed-ip': '', 'port-id': ''}, + {'net-id': 'auto_uuid', + 'v4-fixed-ip': '', + 'v6-fixed-ip': '', + 'port-id': ''}, {'net-id': '', 'v4-fixed-ip': '', 'v6-fixed-ip': '', From ffb69116b30cfbf7b981666528a3e417d502f93b Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 26 Oct 2022 18:18:50 +0300 Subject: [PATCH 009/586] compute: Add missing microversion check for networks The 'auto' and 'none' network allocation policies are only supported on compute API microversion 2.37 or later. Enforce this in the code. Change-Id: I90f8fb1e61ead4bd406ea76bbeb731b913805b13 Signed-off-by: Stephen Finucane Story: 2010385 Task: 46657 --- openstackclient/compute/v2/server.py | 8 ++ .../tests/unit/compute/v2/test_server.py | 100 +++++++++++++++--- 2 files changed, 91 insertions(+), 17 deletions(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 1d0724636..609faf5aa 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -1615,6 +1615,14 @@ def _match_image(image_api, wanted_properties): ) raise exceptions.CommandError(msg) + if compute_client.api_version < api_versions.APIVersion('2.37'): + msg = _( + '--os-compute-api-version 2.37 or greater is ' + 'required to support explicit auto-allocation of a ' + 'network or to disable network allocation' + ) + raise exceptions.CommandError(msg) + nics = nics[0] else: for nic in nics: diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 48a36172b..f59c954a3 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -1926,6 +1926,10 @@ def test_server_create_with_network_tag_pre_v243(self): exceptions.CommandError, self.cmd.take_action, parsed_args) def _test_server_create_with_auto_network(self, arglist): + # requires API microversion 2.37 or later + self.app.client_manager.compute.api_version = api_versions.APIVersion( + '2.37') + verifylist = [ ('image', 'image1'), ('flavor', 'flavor1'), @@ -1986,8 +1990,45 @@ def test_server_create_with_auto_network(self): ] self._test_server_create_with_auto_network(arglist) + def test_server_create_with_auto_network_pre_v237(self): + # use an API microversion that's too old + self.app.client_manager.compute.api_version = api_versions.APIVersion( + '2.36') + + arglist = [ + '--image', 'image1', + '--flavor', 'flavor1', + '--nic', 'auto', + self.new_server.name, + ] + verifylist = [ + ('image', 'image1'), + ('flavor', 'flavor1'), + ('nics', ['auto']), + ('config_drive', False), + ('server_name', self.new_server.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + exc = self.assertRaises( + exceptions.CommandError, + self.cmd.take_action, + parsed_args, + ) + self.assertIn( + '--os-compute-api-version 2.37 or greater is required to support ' + 'explicit auto-allocation of a network or to disable network ' + 'allocation', + str(exc), + ) + self.assertNotCalled(self.servers_mock.create) + def test_server_create_with_auto_network_default_v2_37(self): """Tests creating a server without specifying --nic using 2.37.""" + # requires API microversion 2.37 or later + self.app.client_manager.compute.api_version = api_versions.APIVersion( + '2.37') + arglist = [ '--image', 'image1', '--flavor', 'flavor1', @@ -2001,12 +2042,7 @@ def test_server_create_with_auto_network_default_v2_37(self): ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - # Since check_parser doesn't handle compute global options like - # --os-compute-api-version, we have to mock the construction of - # the novaclient client object with our own APIVersion. - with mock.patch.object(self.app.client_manager.compute, 'api_version', - api_versions.APIVersion('2.37')): - columns, data = self.cmd.take_action(parsed_args) + columns, data = self.cmd.take_action(parsed_args) # Set expected values kwargs = dict( @@ -2037,6 +2073,10 @@ def test_server_create_with_auto_network_default_v2_37(self): self.assertEqual(self.datalist(), data) def _test_server_create_with_none_network(self, arglist): + # requires API microversion 2.37 or later + self.app.client_manager.compute.api_version = api_versions.APIVersion( + '2.37') + verifylist = [ ('image', 'image1'), ('flavor', 'flavor1'), @@ -2097,6 +2137,40 @@ def test_server_create_with_none_network(self): ] self._test_server_create_with_none_network(arglist) + def test_server_create_with_none_network_pre_v237(self): + # use an API microversion that's too old + self.app.client_manager.compute.api_version = api_versions.APIVersion( + '2.36') + + arglist = [ + '--image', 'image1', + '--flavor', 'flavor1', + '--nic', 'none', + self.new_server.name, + ] + + verifylist = [ + ('image', 'image1'), + ('flavor', 'flavor1'), + ('nics', ['none']), + ('config_drive', False), + ('server_name', self.new_server.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + exc = self.assertRaises( + exceptions.CommandError, + self.cmd.take_action, + parsed_args, + ) + self.assertIn( + '--os-compute-api-version 2.37 or greater is required to support ' + 'explicit auto-allocation of a network or to disable network ' + 'allocation', + str(exc), + ) + self.assertNotCalled(self.servers_mock.create) + def test_server_create_with_conflict_network_options(self): arglist = [ '--image', 'image1', @@ -3227,13 +3301,11 @@ def test_server_create_image_property(self): arglist = [ '--image-property', 'hypervisor_type=qemu', '--flavor', 'flavor1', - '--nic', 'none', self.new_server.name, ] verifylist = [ ('image_properties', {'hypervisor_type': 'qemu'}), ('flavor', 'flavor1'), - ('nics', ['none']), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -3261,7 +3333,7 @@ def test_server_create_image_property(self): availability_zone=None, admin_pass=None, block_device_mapping_v2=[], - nics='none', + nics=[], meta=None, scheduler_hints={}, config_drive=None, @@ -3282,14 +3354,12 @@ def test_server_create_image_property_multi(self): '--image-property', 'hypervisor_type=qemu', '--image-property', 'hw_disk_bus=ide', '--flavor', 'flavor1', - '--nic', 'none', self.new_server.name, ] verifylist = [ ('image_properties', {'hypervisor_type': 'qemu', 'hw_disk_bus': 'ide'}), ('flavor', 'flavor1'), - ('nics', ['none']), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -3317,7 +3387,7 @@ def test_server_create_image_property_multi(self): availability_zone=None, admin_pass=None, block_device_mapping_v2=[], - nics='none', + nics=[], meta=None, scheduler_hints={}, config_drive=None, @@ -3338,14 +3408,12 @@ def test_server_create_image_property_missed(self): '--image-property', 'hypervisor_type=qemu', '--image-property', 'hw_disk_bus=virtio', '--flavor', 'flavor1', - '--nic', 'none', self.new_server.name, ] verifylist = [ ('image_properties', {'hypervisor_type': 'qemu', 'hw_disk_bus': 'virtio'}), ('flavor', 'flavor1'), - ('nics', ['none']), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -3369,7 +3437,6 @@ def test_server_create_image_property_with_image_list(self): '--image-property', 'owner_specified.openstack.object=image/cirros', '--flavor', 'flavor1', - '--nic', 'none', self.new_server.name, ] @@ -3377,7 +3444,6 @@ def test_server_create_image_property_with_image_list(self): ('image_properties', {'owner_specified.openstack.object': 'image/cirros'}), ('flavor', 'flavor1'), - ('nics', ['none']), ('server_name', self.new_server.name), ] # create a image_info as the side_effect of the fake image_list() @@ -3407,7 +3473,7 @@ def test_server_create_image_property_with_image_list(self): availability_zone=None, admin_pass=None, block_device_mapping_v2=[], - nics='none', + nics=[], meta=None, scheduler_hints={}, config_drive=None, From a244bb84e07617dad12dee91bbe63bdca3357b1e Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 7 Nov 2022 17:02:58 +0000 Subject: [PATCH 010/586] tests: Move json decoding to base test class We do this everywhere. Add a simple knob to simplify the pattern. Only one use is migrated initially. The rest will be done separately. Change-Id: Ic3b8958bd4fb1459a8ac3adaff216c2a26628491 Signed-off-by: Stephen Finucane --- openstackclient/tests/functional/base.py | 62 ++++++++++++------- .../tests/functional/common/test_module.py | 16 +++-- 2 files changed, 48 insertions(+), 30 deletions(-) diff --git a/openstackclient/tests/functional/base.py b/openstackclient/tests/functional/base.py index b6867f819..0c430267b 100644 --- a/openstackclient/tests/functional/base.py +++ b/openstackclient/tests/functional/base.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +import json import logging import os import shlex @@ -48,33 +49,52 @@ def execute(cmd, fail_ok=False, merge_stderr=False): class TestCase(testtools.TestCase): @classmethod - def openstack(cls, cmd, cloud=ADMIN_CLOUD, fail_ok=False): + def openstack( + cls, + cmd, + *, + cloud=ADMIN_CLOUD, + fail_ok=False, + parse_output=False, + ): """Executes openstackclient command for the given action - NOTE(dtroyer): There is a subtle distinction between passing - cloud=None and cloud='': for compatibility reasons passing - cloud=None continues to include the option '--os-auth-type none' - in the command while passing cloud='' omits the '--os-auth-type' - option completely to let the default handlers be invoked. + :param cmd: A string representation of the command to execute. + :param cloud: The cloud to execute against. This can be a string, empty + string, or None. A string results in '--os-auth-type $cloud', an + empty string results in the '--os-auth-type' option being + omitted, and None resuts in '--os-auth-type none' for legacy + reasons. + :param fail_ok: If failure is permitted. If False (default), a command + failure will result in `~tempest.lib.exceptions.CommandFailed` + being raised. + :param parse_output: If true, pass the '-f json' parameter and decode + the output. + :returns: The output from the command. + :raises: `~tempest.lib.exceptions.CommandFailed` if the command failed + and ``fail_ok`` was ``False``. """ + auth_args = [] if cloud is None: # Execute command with no auth - return execute( - 'openstack --os-auth-type none ' + cmd, - fail_ok=fail_ok - ) - elif cloud == '': - # Execute command with no auth options at all - return execute( - 'openstack ' + cmd, - fail_ok=fail_ok - ) - else: + auth_args.append('--os-auth-type none') + elif cloud != '': # Execute command with an explicit cloud specified - return execute( - 'openstack --os-cloud=' + cloud + ' ' + cmd, - fail_ok=fail_ok - ) + auth_args.append(f'--os-cloud {cloud}') + + format_args = [] + if parse_output: + format_args.append('-f json') + + output = execute( + ' '.join(['openstack'] + auth_args + [cmd] + format_args), + fail_ok=fail_ok, + ) + + if parse_output: + return json.loads(output) + else: + return output @classmethod def is_service_enabled(cls, service, version=None): diff --git a/openstackclient/tests/functional/common/test_module.py b/openstackclient/tests/functional/common/test_module.py index 41aabb7f8..967d3b498 100644 --- a/openstackclient/tests/functional/common/test_module.py +++ b/openstackclient/tests/functional/common/test_module.py @@ -11,9 +11,6 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. -# - -import json from openstackclient.tests.functional import base @@ -31,14 +28,14 @@ class ModuleTest(base.TestCase): def test_module_list(self): # Test module list - cmd_output = json.loads(self.openstack('module list -f json')) + cmd_output = self.openstack('module list', parse_output=True) for one_module in self.CLIENTS: self.assertIn(one_module, cmd_output.keys()) for one_module in self.LIBS: self.assertNotIn(one_module, cmd_output.keys()) # Test module list --all - cmd_output = json.loads(self.openstack('module list --all -f json')) + cmd_output = self.openstack('module list --all', parse_output=True) for one_module in self.CLIENTS + self.LIBS: self.assertIn(one_module, cmd_output.keys()) @@ -56,7 +53,7 @@ class CommandTest(base.TestCase): ] def test_command_list_no_option(self): - cmd_output = json.loads(self.openstack('command list -f json')) + cmd_output = self.openstack('command list', parse_output=True) group_names = [each.get('Command Group') for each in cmd_output] for one_group in self.GROUPS: self.assertIn(one_group, group_names) @@ -70,9 +67,10 @@ def test_command_list_with_group(self): 'compute.v2' ] for each_input in input_groups: - cmd_output = json.loads(self.openstack( - 'command list --group %s -f json' % each_input - )) + cmd_output = self.openstack( + 'command list --group %s' % each_input, + parse_output=True, + ) group_names = [each.get('Command Group') for each in cmd_output] for each_name in group_names: self.assertIn(each_input, each_name) From 38e39b6dc14fd88318541728cb34fd8442d59e8a Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 7 Nov 2022 17:21:12 +0000 Subject: [PATCH 011/586] tests: Convert more functional tests to use 'parse_output' Change-Id: I1d968181eb196c6df4583c772c67ed58bc7ba585 Signed-off-by: Stephen Finucane --- .../tests/functional/common/test_args.py | 25 ++--- .../common/test_availability_zone.py | 8 +- .../functional/common/test_configuration.py | 24 +++-- .../tests/functional/common/test_extension.py | 56 ++++------- .../tests/functional/common/test_quota.py | 99 +++++++++++-------- .../tests/functional/common/test_versions.py | 6 +- 6 files changed, 105 insertions(+), 113 deletions(-) diff --git a/openstackclient/tests/functional/common/test_args.py b/openstackclient/tests/functional/common/test_args.py index 02cad6c16..1f5ecc1cf 100644 --- a/openstackclient/tests/functional/common/test_args.py +++ b/openstackclient/tests/functional/common/test_args.py @@ -10,8 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import json - from tempest.lib import exceptions as tempest_exc from openstackclient.tests.functional import base @@ -21,10 +19,11 @@ class ArgumentTests(base.TestCase): """Functional tests for command line arguments""" def test_default_auth_type(self): - cmd_output = json.loads(self.openstack( - 'configuration show -f json', + cmd_output = self.openstack( + 'configuration show', cloud='', - )) + parse_output=True, + ) self.assertIsNotNone(cmd_output) self.assertIn( 'auth_type', @@ -36,10 +35,11 @@ def test_default_auth_type(self): ) def test_auth_type_none(self): - cmd_output = json.loads(self.openstack( - 'configuration show -f json', + cmd_output = self.openstack( + 'configuration show', cloud=None, - )) + parse_output=True, + ) self.assertIsNotNone(cmd_output) self.assertIn( 'auth_type', @@ -54,7 +54,7 @@ def test_auth_type_token_endpoint_opt(self): # Make sure token_endpoint is really gone try: self.openstack( - 'configuration show -f json --os-auth-type token_endpoint', + 'configuration show --os-auth-type token_endpoint', cloud=None, ) except tempest_exc.CommandFailed as e: @@ -64,10 +64,11 @@ def test_auth_type_token_endpoint_opt(self): self.fail('CommandFailed should be raised') def test_auth_type_password_opt(self): - cmd_output = json.loads(self.openstack( - 'configuration show -f json --os-auth-type password', + cmd_output = self.openstack( + 'configuration show --os-auth-type password', cloud=None, - )) + parse_output=True, + ) self.assertIsNotNone(cmd_output) self.assertIn( 'auth_type', diff --git a/openstackclient/tests/functional/common/test_availability_zone.py b/openstackclient/tests/functional/common/test_availability_zone.py index 025da95c2..f319ffc5e 100644 --- a/openstackclient/tests/functional/common/test_availability_zone.py +++ b/openstackclient/tests/functional/common/test_availability_zone.py @@ -10,8 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import json - from openstackclient.tests.functional import base @@ -19,8 +17,10 @@ class AvailabilityZoneTests(base.TestCase): """Functional tests for availability zone. """ def test_availability_zone_list(self): - cmd_output = json.loads(self.openstack( - 'availability zone list -f json')) + cmd_output = self.openstack( + 'availability zone list', + parse_output=True, + ) zones = [x['Zone Name'] for x in cmd_output] self.assertIn( 'internal', diff --git a/openstackclient/tests/functional/common/test_configuration.py b/openstackclient/tests/functional/common/test_configuration.py index 17e0f45d1..614b3e46e 100644 --- a/openstackclient/tests/functional/common/test_configuration.py +++ b/openstackclient/tests/functional/common/test_configuration.py @@ -10,7 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import json import os from openstackclient.common import configuration @@ -30,9 +29,7 @@ def test_configuration_show(self): items = self.parse_listing(raw_output) self.assert_table_structure(items, BASIC_CONFIG_HEADERS) - cmd_output = json.loads(self.openstack( - 'configuration show -f json' - )) + cmd_output = self.openstack('configuration show', parse_output=True) self.assertEqual( configuration.REDACTED, cmd_output['auth.password'] @@ -43,18 +40,18 @@ def test_configuration_show(self): ) # Test show --mask - cmd_output = json.loads(self.openstack( - 'configuration show --mask -f json' - )) + cmd_output = self.openstack( + 'configuration show --mask', parse_output=True, + ) self.assertEqual( configuration.REDACTED, cmd_output['auth.password'] ) # Test show --unmask - cmd_output = json.loads(self.openstack( - 'configuration show --unmask -f json' - )) + cmd_output = self.openstack( + 'configuration show --unmask', parse_output=True, + ) # If we are using os-client-config, this will not be set. Rather than # parse clouds.yaml to get the right value, just make sure # we are not getting redacted. @@ -84,10 +81,11 @@ def test_configuration_show(self): items = self.parse_listing(raw_output) self.assert_table_structure(items, BASIC_CONFIG_HEADERS) - cmd_output = json.loads(self.openstack( - 'configuration show -f json', + cmd_output = self.openstack( + 'configuration show', cloud=None, - )) + parse_output=True, + ) self.assertNotIn( 'auth.password', cmd_output, diff --git a/openstackclient/tests/functional/common/test_extension.py b/openstackclient/tests/functional/common/test_extension.py index 92efabefe..8784c55b1 100644 --- a/openstackclient/tests/functional/common/test_extension.py +++ b/openstackclient/tests/functional/common/test_extension.py @@ -13,8 +13,6 @@ # License for the specific language governing permissions and limitations # under the License. -import json - from tempest.lib import exceptions as tempest_exc from openstackclient.tests.functional import base @@ -30,11 +28,11 @@ def setUpClass(cls): def test_extension_list_compute(self): """Test compute extension list""" - json_output = json.loads(self.openstack( - 'extension list -f json ' + - '--compute' - )) - name_list = [item.get('Name') for item in json_output] + output = self.openstack( + 'extension list --compute', + parse_output=True, + ) + name_list = [item.get('Name') for item in output] self.assertIn( 'ImageSize', name_list, @@ -42,11 +40,11 @@ def test_extension_list_compute(self): def test_extension_list_volume(self): """Test volume extension list""" - json_output = json.loads(self.openstack( - 'extension list -f json ' + - '--volume' - )) - name_list = [item.get('Name') for item in json_output] + output = self.openstack( + 'extension list --volume', + parse_output=True, + ) + name_list = [item.get('Name') for item in output] self.assertIn( 'TypesManage', name_list, @@ -57,43 +55,29 @@ def test_extension_list_network(self): if not self.haz_network: self.skipTest("No Network service present") - json_output = json.loads(self.openstack( - 'extension list -f json ' + - '--network' - )) - name_list = [item.get('Name') for item in json_output] + output = self.openstack( + 'extension list --network', + parse_output=True, + ) + name_list = [item.get('Name') for item in output] self.assertIn( 'Default Subnetpools', name_list, ) - # NOTE(dtroyer): Only network extensions are currently supported but - # I am going to leave this here anyway as a reminder - # fix that. - # def test_extension_show_compute(self): - # """Test compute extension show""" - # json_output = json.loads(self.openstack( - # 'extension show -f json ' + - # 'ImageSize' - # )) - # self.assertEqual( - # 'OS-EXT-IMG-SIZE', - # json_output.get('Alias'), - # ) - def test_extension_show_network(self): """Test network extension show""" if not self.haz_network: self.skipTest("No Network service present") name = 'agent' - json_output = json.loads(self.openstack( - 'extension show -f json ' + - name - )) + output = self.openstack( + 'extension show ' + name, + parse_output=True, + ) self.assertEqual( name, - json_output.get('alias'), + output.get('alias'), ) def test_extension_show_not_exist(self): diff --git a/openstackclient/tests/functional/common/test_quota.py b/openstackclient/tests/functional/common/test_quota.py index 9089cba5c..6e48df1d7 100644 --- a/openstackclient/tests/functional/common/test_quota.py +++ b/openstackclient/tests/functional/common/test_quota.py @@ -10,7 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import json import uuid from tempest.lib import exceptions @@ -36,9 +35,10 @@ def setUpClass(cls): def test_quota_list_details_compute(self): expected_headers = ["Resource", "In Use", "Reserved", "Limit"] - cmd_output = json.loads(self.openstack( - 'quota list -f json --detail --compute' - )) + cmd_output = self.openstack( + 'quota list --detail --compute', + parse_output=True, + ) self.assertIsNotNone(cmd_output) resources = [] for row in cmd_output: @@ -52,9 +52,10 @@ def test_quota_list_details_compute(self): def test_quota_list_details_network(self): expected_headers = ["Resource", "In Use", "Reserved", "Limit"] - cmd_output = json.loads(self.openstack( - 'quota list -f json --detail --network' - )) + cmd_output = self.openstack( + 'quota list --detail --network', + parse_output=True, + ) self.assertIsNotNone(cmd_output) resources = [] for row in cmd_output: @@ -70,9 +71,10 @@ def test_quota_list_network_option(self): if not self.haz_network: self.skipTest("No Network service present") self.openstack('quota set --networks 40 ' + self.PROJECT_NAME) - cmd_output = json.loads(self.openstack( - 'quota list -f json --network' - )) + cmd_output = self.openstack( + 'quota list --network', + parse_output=True, + ) self.assertIsNotNone(cmd_output) self.assertEqual( 40, @@ -81,9 +83,10 @@ def test_quota_list_network_option(self): def test_quota_list_compute_option(self): self.openstack('quota set --instances 30 ' + self.PROJECT_NAME) - cmd_output = json.loads(self.openstack( - 'quota list -f json --compute' - )) + cmd_output = self.openstack( + 'quota list --compute', + parse_output=True, + ) self.assertIsNotNone(cmd_output) self.assertEqual( 30, @@ -92,9 +95,10 @@ def test_quota_list_compute_option(self): def test_quota_list_volume_option(self): self.openstack('quota set --volumes 20 ' + self.PROJECT_NAME) - cmd_output = json.loads(self.openstack( - 'quota list -f json --volume' - )) + cmd_output = self.openstack( + 'quota list --volume', + parse_output=True, + ) self.assertIsNotNone(cmd_output) self.assertEqual( 20, @@ -111,9 +115,10 @@ def test_quota_set_project(self): network_option + self.PROJECT_NAME ) - cmd_output = json.loads(self.openstack( - 'quota show -f json ' + self.PROJECT_NAME - )) + cmd_output = self.openstack( + 'quota show ' + self.PROJECT_NAME, + parse_output=True, + ) cmd_output = {x['Resource']: x['Limit'] for x in cmd_output} self.assertIsNotNone(cmd_output) self.assertEqual( @@ -131,9 +136,10 @@ def test_quota_set_project(self): ) # Check default quotas - cmd_output = json.loads(self.openstack( - 'quota show -f json --default' - )) + cmd_output = self.openstack( + 'quota show --default', + parse_output=True, + ) self.assertIsNotNone(cmd_output) # We don't necessarily know the default quotas, we're checking the # returned attributes @@ -148,9 +154,10 @@ def test_quota_set_class(self): 'quota set --key-pairs 33 --snapshots 43 ' + '--class default' ) - cmd_output = json.loads(self.openstack( - 'quota show -f json --class default' - )) + cmd_output = self.openstack( + 'quota show --class default', + parse_output=True, + ) self.assertIsNotNone(cmd_output) cmd_output = {x['Resource']: x['Limit'] for x in cmd_output} self.assertEqual( @@ -163,9 +170,10 @@ def test_quota_set_class(self): ) # Check default quota class - cmd_output = json.loads(self.openstack( - 'quota show -f json --class' - )) + cmd_output = self.openstack( + 'quota show --class', + parse_output=True, + ) self.assertIsNotNone(cmd_output) # We don't necessarily know the default quotas, we're checking the # returned attributes @@ -182,16 +190,18 @@ def test_quota_network_set_with_no_force(self): if not self.is_extension_enabled('quota-check-limit'): self.skipTest('No "quota-check-limit" extension present') - cmd_output = json.loads(self.openstack( - 'quota list -f json --network' - )) + cmd_output = self.openstack( + 'quota list --network', + parse_output=True, + ) self.addCleanup(self._restore_quota_limit, 'network', cmd_output[0]['Networks'], self.PROJECT_NAME) self.openstack('quota set --networks 40 ' + self.PROJECT_NAME) - cmd_output = json.loads(self.openstack( - 'quota list -f json --network' - )) + cmd_output = self.openstack( + 'quota list --network', + parse_output=True, + ) self.assertIsNotNone(cmd_output) self.assertEqual(40, cmd_output[0]['Networks']) @@ -218,16 +228,18 @@ def test_quota_network_set_with_force(self): if not self.is_extension_enabled('quota-check-limit'): self.skipTest('No "quota-check-limit" extension present') - cmd_output = json.loads(self.openstack( - 'quota list -f json --network' - )) + cmd_output = self.openstack( + 'quota list --network', + parse_output=True, + ) self.addCleanup(self._restore_quota_limit, 'network', cmd_output[0]['Networks'], self.PROJECT_NAME) self.openstack('quota set --networks 40 ' + self.PROJECT_NAME) - cmd_output = json.loads(self.openstack( - 'quota list -f json --network' - )) + cmd_output = self.openstack( + 'quota list --network', + parse_output=True, + ) self.assertIsNotNone(cmd_output) self.assertEqual(40, cmd_output[0]['Networks']) @@ -237,8 +249,9 @@ def test_quota_network_set_with_force(self): (self.PROJECT_NAME, uuid.uuid4().hex)) self.openstack('quota set --networks 1 --force ' + self.PROJECT_NAME) - cmd_output = json.loads(self.openstack( - 'quota list -f json --network' - )) + cmd_output = self.openstack( + 'quota list --network', + parse_output=True, + ) self.assertIsNotNone(cmd_output) self.assertEqual(1, cmd_output[0]['Networks']) diff --git a/openstackclient/tests/functional/common/test_versions.py b/openstackclient/tests/functional/common/test_versions.py index adc74ebc6..6575671aa 100644 --- a/openstackclient/tests/functional/common/test_versions.py +++ b/openstackclient/tests/functional/common/test_versions.py @@ -10,8 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import json - from openstackclient.tests.functional import base @@ -21,9 +19,7 @@ class VersionsTests(base.TestCase): def test_versions_show(self): # TODO(mordred) Make this better. The trick is knowing what in the # payload to test for. - cmd_output = json.loads(self.openstack( - 'versions show -f json' - )) + cmd_output = self.openstack('versions show', parse_output=True) self.assertIsNotNone(cmd_output) self.assertIn( "Region Name", From b7d01833d02260006e6d4c6e0a065748ecc8b913 Mon Sep 17 00:00:00 2001 From: Maksim Malchuk Date: Wed, 9 Nov 2022 17:28:28 +0300 Subject: [PATCH 012/586] Add baremetal agent type list filtering This change adds and ability to filter out the baremetal nodes in 'network agent list' command. Related-Story: 2008590 Related-Task: 41746 Related-Bug: #1658964 Change-Id: I01ffbd82662abbc1c2f56eb8f1e700f392bc063c Signed-off-by: Maksim Malchuk --- openstackclient/network/v2/network_agent.py | 7 ++++--- .../notes/add-baremetal-agent-type-7c46365e8d457ac8.yaml | 5 +++++ 2 files changed, 9 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/add-baremetal-agent-type-7c46365e8d457ac8.yaml diff --git a/openstackclient/network/v2/network_agent.py b/openstackclient/network/v2/network_agent.py index d963b3bf3..f67f67bd6 100644 --- a/openstackclient/network/v2/network_agent.py +++ b/openstackclient/network/v2/network_agent.py @@ -168,11 +168,11 @@ def get_parser(self, prog_name): metavar='', choices=["bgp", "dhcp", "open-vswitch", "linux-bridge", "ofa", "l3", "loadbalancer", "metering", "metadata", "macvtap", - "nic"], + "nic", "baremetal"], help=_("List only agents with the specified agent type. " "The supported agent types are: bgp, dhcp, open-vswitch, " "linux-bridge, ofa, l3, loadbalancer, metering, " - "metadata, macvtap, nic.") + "metadata, macvtap, nic, baremetal.") ) parser.add_argument( '--host', @@ -231,7 +231,8 @@ def take_action(self, parsed_args): 'metering': 'Metering agent', 'metadata': 'Metadata agent', 'macvtap': 'Macvtap agent', - 'nic': 'NIC Switch agent' + 'nic': 'NIC Switch agent', + 'baremetal': 'Baremetal Node' } filters = {} diff --git a/releasenotes/notes/add-baremetal-agent-type-7c46365e8d457ac8.yaml b/releasenotes/notes/add-baremetal-agent-type-7c46365e8d457ac8.yaml new file mode 100644 index 000000000..a9a3a0dfd --- /dev/null +++ b/releasenotes/notes/add-baremetal-agent-type-7c46365e8d457ac8.yaml @@ -0,0 +1,5 @@ +--- +features: + - | + Add ``baremetal`` agent type to ``--agent-type`` option for + ``network agent list`` command. From bafece762a5d0b03e28f9d81c98ad46777f56a34 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 21 Oct 2022 16:40:36 +0100 Subject: [PATCH 013/586] image: Ignore '--progress' if providing image data from stdin You can provide data via stdin when creating an image. Using this with '--progress' makes no sense and causes an error currently. Fix this. Change-Id: I3c2d658b72a7c62931b779b0d19bb97f60a0c655 Signed-off-by: Stephen Finucane --- openstackclient/image/v2/image.py | 4 ++- .../tests/unit/image/v2/test_image.py | 31 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/openstackclient/image/v2/image.py b/openstackclient/image/v2/image.py index 2342fd3e2..1ff8ad3ae 100644 --- a/openstackclient/image/v2/image.py +++ b/openstackclient/image/v2/image.py @@ -476,7 +476,9 @@ def _take_action_image(self, parsed_args): LOG.warning(_("Failed to get an image file.")) return {}, {} - if fp is not None and parsed_args.progress: + if parsed_args.progress and parsed_args.file: + # NOTE(stephenfin): we only show a progress bar if the user + # requested it *and* we're reading from a file (not stdin) filesize = os.path.getsize(fname) if filesize is not None: kwargs['validate_checksum'] = False diff --git a/openstackclient/tests/unit/image/v2/test_image.py b/openstackclient/tests/unit/image/v2/test_image.py index f2c113649..e17363a50 100644 --- a/openstackclient/tests/unit/image/v2/test_image.py +++ b/openstackclient/tests/unit/image/v2/test_image.py @@ -252,6 +252,37 @@ def test_image_create_file(self): self.expected_data, data) + @mock.patch('openstackclient.image.v2.image.get_data_file') + def test_image_create__progress_ignore_with_stdin( + self, mock_get_data_file, + ): + fake_stdin = io.StringIO('fake-image-data') + mock_get_data_file.return_value = (fake_stdin, None) + + arglist = [ + '--progress', + self.new_image.name, + ] + verifylist = [ + ('progress', True), + ('name', self.new_image.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + columns, data = self.cmd.take_action(parsed_args) + + self.client.create_image.assert_called_with( + name=self.new_image.name, + allow_duplicates=True, + container_format=image.DEFAULT_CONTAINER_FORMAT, + disk_format=image.DEFAULT_DISK_FORMAT, + data=fake_stdin, + validate_checksum=False, + ) + + self.assertEqual(self.expected_columns, columns) + self.assertCountEqual(self.expected_data, data) + def test_image_create_dead_options(self): arglist = [ From 3d9a9df935af1f76a33d008fe76975475c77a268 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 21 Oct 2022 16:40:55 +0100 Subject: [PATCH 014/586] image: Simplify handling of data provided via stdin This was unnecessarily complex. Change-Id: I8289d5ce7356d8bc89425590a7f71bca91a6d396 Signed-off-by: Stephen Finucane --- openstackclient/image/v2/image.py | 91 ++++++++++-------- .../tests/unit/image/v2/test_image.py | 94 ++++++------------- 2 files changed, 82 insertions(+), 103 deletions(-) diff --git a/openstackclient/image/v2/image.py b/openstackclient/image/v2/image.py index 1ff8ad3ae..53cfadede 100644 --- a/openstackclient/image/v2/image.py +++ b/openstackclient/image/v2/image.py @@ -134,34 +134,32 @@ def _get_member_columns(item): ) -def get_data_file(args): - if args.file: - return (open(args.file, 'rb'), args.file) +def get_data_from_stdin(): + # distinguish cases where: + # (1) stdin is not valid (as in cron jobs): + # openstack ... <&- + # (2) image data is provided through stdin: + # openstack ... < /tmp/file + # (3) no image data provided + # openstack ... + try: + os.fstat(0) + except OSError: + # (1) stdin is not valid + return None + + if not sys.stdin.isatty(): + # (2) image data is provided through stdin + image = sys.stdin + if hasattr(sys.stdin, 'buffer'): + image = sys.stdin.buffer + if msvcrt: + msvcrt.setmode(sys.stdin.fileno(), os.O_BINARY) + + return image else: - # distinguish cases where: - # (1) stdin is not valid (as in cron jobs): - # openstack ... <&- - # (2) image data is provided through stdin: - # openstack ... < /tmp/file - # (3) no image data provided - # openstack ... - try: - os.fstat(0) - except OSError: - # (1) stdin is not valid - return (None, None) - if not sys.stdin.isatty(): - # (2) image data is provided through stdin - image = sys.stdin - if hasattr(sys.stdin, 'buffer'): - image = sys.stdin.buffer - if msvcrt: - msvcrt.setmode(sys.stdin.fileno(), os.O_BINARY) - - return (image, None) - else: - # (3) - return (None, None) + # (3) + return None class AddProjectToImage(command.ShowOne): @@ -277,6 +275,7 @@ def get_parser(self, prog_name): source_group = parser.add_mutually_exclusive_group() source_group.add_argument( "--file", + dest="filename", metavar="", help=_("Upload image from local file"), ) @@ -299,7 +298,10 @@ def get_parser(self, prog_name): "--progress", action="store_true", default=False, - help=_("Show upload progress bar."), + help=_( + "Show upload progress bar " + "(ignored if passing data via stdin)" + ), ) parser.add_argument( '--sign-key-path', @@ -463,7 +465,15 @@ def _take_action_image(self, parsed_args): # open the file first to ensure any failures are handled before the # image is created. Get the file name (if it is file, and not stdin) # for easier further handling. - fp, fname = get_data_file(parsed_args) + if parsed_args.filename: + try: + fp = open(parsed_args.filename, 'rb') + except FileNotFoundError: + raise exceptions.CommandError( + '%r is not a valid file' % parsed_args.filename, + ) + else: + fp = get_data_from_stdin() if fp is not None and parsed_args.volume: msg = _( @@ -472,26 +482,24 @@ def _take_action_image(self, parsed_args): ) raise exceptions.CommandError(msg) - if fp is None and parsed_args.file: - LOG.warning(_("Failed to get an image file.")) - return {}, {} - - if parsed_args.progress and parsed_args.file: + if parsed_args.progress and parsed_args.filename: # NOTE(stephenfin): we only show a progress bar if the user # requested it *and* we're reading from a file (not stdin) - filesize = os.path.getsize(fname) + filesize = os.path.getsize(parsed_args.filename) if filesize is not None: kwargs['validate_checksum'] = False kwargs['data'] = progressbar.VerboseFileWrapper(fp, filesize) - elif fname: - kwargs['filename'] = fname + else: + kwargs['data'] = fp + elif parsed_args.filename: + kwargs['filename'] = parsed_args.filename elif fp: kwargs['validate_checksum'] = False kwargs['data'] = fp # sign an image using a given local private key file if parsed_args.sign_key_path or parsed_args.sign_cert_id: - if not parsed_args.file: + if not parsed_args.filename: msg = _( "signing an image requires the --file option, " "passing files via stdin when signing is not " @@ -546,6 +554,10 @@ def _take_action_image(self, parsed_args): kwargs['img_signature_key_type'] = signer.padding_method image = image_client.create_image(**kwargs) + + if parsed_args.filename: + fp.close() + return _format_image(image) def _take_action_volume(self, parsed_args): @@ -980,6 +992,7 @@ def get_parser(self, prog_name): parser.add_argument( "--file", metavar="", + dest="filename", help=_("Downloaded image save filename (default: stdout)"), ) parser.add_argument( @@ -993,7 +1006,7 @@ def take_action(self, parsed_args): image_client = self.app.client_manager.image image = image_client.find_image(parsed_args.image) - output_file = parsed_args.file + output_file = parsed_args.filename if output_file is None: output_file = getattr(sys.stdout, "buffer", sys.stdout) diff --git a/openstackclient/tests/unit/image/v2/test_image.py b/openstackclient/tests/unit/image/v2/test_image.py index e17363a50..ac9ddae6e 100644 --- a/openstackclient/tests/unit/image/v2/test_image.py +++ b/openstackclient/tests/unit/image/v2/test_image.py @@ -14,7 +14,6 @@ import copy import io -import os import tempfile from unittest import mock @@ -104,15 +103,8 @@ def test_image_reserve_no_options(self, raw_input): disk_format=image.DEFAULT_DISK_FORMAT, ) - # Verify update() was not called, if it was show the args - self.assertEqual(self.client.update_image.call_args_list, []) - - self.assertEqual( - self.expected_columns, - columns) - self.assertCountEqual( - self.expected_data, - data) + self.assertEqual(self.expected_columns, columns) + self.assertCountEqual(self.expected_data, data) @mock.patch('sys.stdin', side_effect=[None]) def test_image_reserve_options(self, raw_input): @@ -121,10 +113,11 @@ def test_image_reserve_options(self, raw_input): '--disk-format', 'ami', '--min-disk', '10', '--min-ram', '4', - ('--protected' - if self.new_image.is_protected else '--unprotected'), - ('--private' - if self.new_image.visibility == 'private' else '--public'), + '--protected' if self.new_image.is_protected else '--unprotected', + ( + '--private' + if self.new_image.visibility == 'private' else '--public' + ), '--project', self.new_image.owner_id, '--project-domain', self.domain.id, self.new_image.name, @@ -160,12 +153,8 @@ def test_image_reserve_options(self, raw_input): visibility=self.new_image.visibility, ) - self.assertEqual( - self.expected_columns, - columns) - self.assertCountEqual( - self.expected_data, - data) + self.assertEqual(self.expected_columns, columns) + self.assertCountEqual(self.expected_data, data) def test_image_create_with_unexist_project(self): self.project_mock.get.side_effect = exceptions.NotFound(None) @@ -217,7 +206,7 @@ def test_image_create_file(self): self.new_image.name, ] verifylist = [ - ('file', imagefile.name), + ('filename', imagefile.name), ('is_protected', self.new_image.is_protected), ('visibility', self.new_image.visibility), ('properties', {'Alpha': '1', 'Beta': '2'}), @@ -252,12 +241,12 @@ def test_image_create_file(self): self.expected_data, data) - @mock.patch('openstackclient.image.v2.image.get_data_file') + @mock.patch('openstackclient.image.v2.image.get_data_from_stdin') def test_image_create__progress_ignore_with_stdin( - self, mock_get_data_file, + self, mock_get_data_from_stdin, ): fake_stdin = io.StringIO('fake-image-data') - mock_get_data_file.return_value = (fake_stdin, None) + mock_get_data_from_stdin.return_value = fake_stdin arglist = [ '--progress', @@ -322,11 +311,11 @@ def test_image_create_import(self, raw_input): ) @mock.patch('osc_lib.utils.find_resource') - @mock.patch('openstackclient.image.v2.image.get_data_file') + @mock.patch('openstackclient.image.v2.image.get_data_from_stdin') def test_image_create_from_volume(self, mock_get_data_f, mock_get_vol): fake_vol_id = 'fake-volume-id' - mock_get_data_f.return_value = (None, None) + mock_get_data_f.return_value = None class FakeVolume: id = fake_vol_id @@ -353,12 +342,12 @@ class FakeVolume: ) @mock.patch('osc_lib.utils.find_resource') - @mock.patch('openstackclient.image.v2.image.get_data_file') + @mock.patch('openstackclient.image.v2.image.get_data_from_stdin') def test_image_create_from_volume_fail(self, mock_get_data_f, mock_get_vol): fake_vol_id = 'fake-volume-id' - mock_get_data_f.return_value = (None, None) + mock_get_data_f.return_value = None class FakeVolume: id = fake_vol_id @@ -379,7 +368,7 @@ class FakeVolume: parsed_args) @mock.patch('osc_lib.utils.find_resource') - @mock.patch('openstackclient.image.v2.image.get_data_file') + @mock.patch('openstackclient.image.v2.image.get_data_from_stdin') def test_image_create_from_volume_v31(self, mock_get_data_f, mock_get_vol): @@ -387,7 +376,7 @@ def test_image_create_from_volume_v31(self, mock_get_data_f, api_versions.APIVersion('3.1')) fake_vol_id = 'fake-volume-id' - mock_get_data_f.return_value = (None, None) + mock_get_data_f.return_value = None class FakeVolume: id = fake_vol_id @@ -1798,7 +1787,7 @@ def test_save_data(self): arglist = ['--file', '/path/to/file', self.image.id] verifylist = [ - ('file', '/path/to/file'), + ('filename', '/path/to/file'), ('image', self.image.id), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -1813,49 +1802,26 @@ def test_save_data(self): class TestImageGetData(TestImage): - def setUp(self): - super().setUp() - self.args = mock.Mock() - - def test_get_data_file_file(self): - (fd, fname) = tempfile.mkstemp(prefix='osc_test_image') - self.args.file = fname - - (test_fd, test_name) = image.get_data_file(self.args) - - self.assertEqual(fname, test_name) - test_fd.close() - - os.unlink(fname) - - def test_get_data_file_2(self): - - self.args.file = None - - f = io.BytesIO(b"some initial binary data: \x00\x01") + def test_get_data_from_stdin(self): + fd = io.BytesIO(b"some initial binary data: \x00\x01") with mock.patch('sys.stdin') as stdin: - stdin.return_value = f + stdin.return_value = fd stdin.isatty.return_value = False - stdin.buffer = f + stdin.buffer = fd - (test_fd, test_name) = image.get_data_file(self.args) + test_fd = image.get_data_from_stdin() # Ensure data written to temp file is correct - self.assertEqual(f, test_fd) - self.assertIsNone(test_name) - - def test_get_data_file_3(self): - - self.args.file = None + self.assertEqual(fd, test_fd) - f = io.BytesIO(b"some initial binary data: \x00\x01") + def test_get_data_from_stdin__interactive(self): + fd = io.BytesIO(b"some initial binary data: \x00\x01") with mock.patch('sys.stdin') as stdin: # There is stdin, but interactive - stdin.return_value = f + stdin.return_value = fd - (test_fd, test_fname) = image.get_data_file(self.args) + test_fd = image.get_data_from_stdin() self.assertIsNone(test_fd) - self.assertIsNone(test_fname) From 1fb8d1f48b256a2bad78e7d5633ea53c6537907c Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 19 Oct 2022 18:07:41 +0100 Subject: [PATCH 015/586] image: Add 'image stage' command This is the equivalent of the 'image-stage' glanceclient command. Change-Id: I10b01ef145740a2f7ffe5a8c7ce0296df0ece0bd Signed-off-by: Stephen Finucane --- doc/source/cli/data/glance.csv | 2 +- openstackclient/image/v2/image.py | 77 +++++++++++++ openstackclient/tests/unit/image/v2/fakes.py | 1 + .../tests/unit/image/v2/test_image.py | 104 +++++++++++++----- .../notes/image-stage-ac19c47e6a52ffeb.yaml | 5 + setup.cfg | 1 + 6 files changed, 164 insertions(+), 26 deletions(-) create mode 100644 releasenotes/notes/image-stage-ac19c47e6a52ffeb.yaml diff --git a/doc/source/cli/data/glance.csv b/doc/source/cli/data/glance.csv index 12b6851d1..26f720cdb 100644 --- a/doc/source/cli/data/glance.csv +++ b/doc/source/cli/data/glance.csv @@ -8,7 +8,7 @@ image-import,,Initiate the image import taskflow. image-list,image list,List images you can access. image-reactivate,image set --activate,Reactivate specified image. image-show,image show,Describe a specific image. -image-stage,,Upload data for a specific image to staging. +image-stage,image stage,Upload data for a specific image to staging. image-tag-delete,image unset --tag ,Delete the tag associated with the given image. image-tag-update,image set --tag ,Update an image with the given tag. image-update,image set,Update an existing image. diff --git a/openstackclient/image/v2/image.py b/openstackclient/image/v2/image.py index 53cfadede..039f1d2dc 100644 --- a/openstackclient/image/v2/image.py +++ b/openstackclient/image/v2/image.py @@ -1484,3 +1484,80 @@ def take_action(self, parsed_args): "Failed to unset %(propret)s of %(proptotal)s" " properties." ) % {'propret': propret, 'proptotal': proptotal} raise exceptions.CommandError(msg) + + +class StageImage(command.Command): + _description = _( + "Upload data for a specific image to staging.\n" + "This requires support for the interoperable image import process, " + "which was first introduced in Image API version 2.6 " + "(Glance 16.0.0 (Queens))" + ) + + def get_parser(self, prog_name): + parser = super().get_parser(prog_name) + + parser.add_argument( + '--file', + metavar='', + dest='filename', + help=_( + 'Local file that contains disk image to be uploaded. ' + 'Alternatively, images can be passed via stdin.' + ), + ) + # NOTE(stephenfin): glanceclient had a --size argument but it didn't do + # anything so we have chosen not to port this + parser.add_argument( + '--progress', + action='store_true', + default=False, + help=_( + 'Show upload progress bar ' + '(ignored if passing data via stdin)' + ), + ) + parser.add_argument( + 'image', + metavar='', + help=_('Image to upload data for (name or ID)'), + ) + + return parser + + def take_action(self, parsed_args): + image_client = self.app.client_manager.image + + image = image_client.find_image( + parsed_args.image, + ignore_missing=False, + ) + # open the file first to ensure any failures are handled before the + # image is created. Get the file name (if it is file, and not stdin) + # for easier further handling. + if parsed_args.filename: + try: + fp = open(parsed_args.filename, 'rb') + except FileNotFoundError: + raise exceptions.CommandError( + '%r is not a valid file' % parsed_args.filename, + ) + else: + fp = get_data_from_stdin() + + kwargs = {} + + if parsed_args.progress and parsed_args.filename: + # NOTE(stephenfin): we only show a progress bar if the user + # requested it *and* we're reading from a file (not stdin) + filesize = os.path.getsize(parsed_args.filename) + if filesize is not None: + kwargs['data'] = progressbar.VerboseFileWrapper(fp, filesize) + else: + kwargs['data'] = fp + elif parsed_args.filename: + kwargs['filename'] = parsed_args.filename + elif fp: + kwargs['data'] = fp + + image_client.stage_image(image, **kwargs) diff --git a/openstackclient/tests/unit/image/v2/fakes.py b/openstackclient/tests/unit/image/v2/fakes.py index cf09df778..8ce2a7d55 100644 --- a/openstackclient/tests/unit/image/v2/fakes.py +++ b/openstackclient/tests/unit/image/v2/fakes.py @@ -38,6 +38,7 @@ def __init__(self, **kwargs): self.download_image = mock.Mock() self.reactivate_image = mock.Mock() self.deactivate_image = mock.Mock() + self.stage_image = mock.Mock() self.members = mock.Mock() self.add_member = mock.Mock() diff --git a/openstackclient/tests/unit/image/v2/test_image.py b/openstackclient/tests/unit/image/v2/test_image.py index ac9ddae6e..8dea7f05a 100644 --- a/openstackclient/tests/unit/image/v2/test_image.py +++ b/openstackclient/tests/unit/image/v2/test_image.py @@ -22,7 +22,7 @@ from osc_lib.cli import format_columns from osc_lib import exceptions -from openstackclient.image.v2 import image +from openstackclient.image.v2 import image as _image from openstackclient.tests.unit.identity.v3 import fakes as identity_fakes from openstackclient.tests.unit.image.v2 import fakes as image_fakes from openstackclient.tests.unit.volume.v3 import fakes as volume_fakes @@ -73,10 +73,10 @@ def setUp(self): self.client.update_image.return_value = self.new_image (self.expected_columns, self.expected_data) = zip( - *sorted(image._format_image(self.new_image).items())) + *sorted(_image._format_image(self.new_image).items())) # Get the command object to test - self.cmd = image.CreateImage(self.app, None) + self.cmd = _image.CreateImage(self.app, None) @mock.patch("sys.stdin", side_effect=[None]) def test_image_reserve_no_options(self, raw_input): @@ -84,8 +84,8 @@ def test_image_reserve_no_options(self, raw_input): self.new_image.name ] verifylist = [ - ('container_format', image.DEFAULT_CONTAINER_FORMAT), - ('disk_format', image.DEFAULT_DISK_FORMAT), + ('container_format', _image.DEFAULT_CONTAINER_FORMAT), + ('disk_format', _image.DEFAULT_DISK_FORMAT), ('name', self.new_image.name), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -99,8 +99,8 @@ def test_image_reserve_no_options(self, raw_input): self.client.create_image.assert_called_with( name=self.new_image.name, allow_duplicates=True, - container_format=image.DEFAULT_CONTAINER_FORMAT, - disk_format=image.DEFAULT_DISK_FORMAT, + container_format=_image.DEFAULT_CONTAINER_FORMAT, + disk_format=_image.DEFAULT_DISK_FORMAT, ) self.assertEqual(self.expected_columns, columns) @@ -224,8 +224,8 @@ def test_image_create_file(self): self.client.create_image.assert_called_with( name=self.new_image.name, allow_duplicates=True, - container_format=image.DEFAULT_CONTAINER_FORMAT, - disk_format=image.DEFAULT_DISK_FORMAT, + container_format=_image.DEFAULT_CONTAINER_FORMAT, + disk_format=_image.DEFAULT_DISK_FORMAT, is_protected=self.new_image.is_protected, visibility=self.new_image.visibility, Alpha='1', @@ -245,7 +245,7 @@ def test_image_create_file(self): def test_image_create__progress_ignore_with_stdin( self, mock_get_data_from_stdin, ): - fake_stdin = io.StringIO('fake-image-data') + fake_stdin = io.BytesIO(b'some fake data') mock_get_data_from_stdin.return_value = fake_stdin arglist = [ @@ -263,8 +263,8 @@ def test_image_create__progress_ignore_with_stdin( self.client.create_image.assert_called_with( name=self.new_image.name, allow_duplicates=True, - container_format=image.DEFAULT_CONTAINER_FORMAT, - disk_format=image.DEFAULT_DISK_FORMAT, + container_format=_image.DEFAULT_CONTAINER_FORMAT, + disk_format=_image.DEFAULT_DISK_FORMAT, data=fake_stdin, validate_checksum=False, ) @@ -305,8 +305,8 @@ def test_image_create_import(self, raw_input): self.client.create_image.assert_called_with( name=self.new_image.name, allow_duplicates=True, - container_format=image.DEFAULT_CONTAINER_FORMAT, - disk_format=image.DEFAULT_DISK_FORMAT, + container_format=_image.DEFAULT_CONTAINER_FORMAT, + disk_format=_image.DEFAULT_DISK_FORMAT, use_import=True ) @@ -445,7 +445,7 @@ def setUp(self): self.project_mock.get.return_value = self.project self.domain_mock.get.return_value = self.domain # Get the command object to test - self.cmd = image.AddProjectToImage(self.app, None) + self.cmd = _image.AddProjectToImage(self.app, None) def test_add_project_to_image_no_option(self): arglist = [ @@ -504,7 +504,7 @@ def setUp(self): self.client.delete_image.return_value = None # Get the command object to test - self.cmd = image.DeleteImage(self.app, None) + self.cmd = _image.DeleteImage(self.app, None) def test_image_delete_no_options(self): images = self.setup_images_mock(count=1) @@ -595,7 +595,7 @@ def setUp(self): self.client.images.side_effect = [[self._image], []] # Get the command object to test - self.cmd = image.ListImage(self.app, None) + self.cmd = _image.ListImage(self.app, None) def test_image_list_no_options(self): arglist = [] @@ -993,7 +993,7 @@ def setUp(self): self.client.find_image.return_value = self._image self.client.members.return_value = [self.member] - self.cmd = image.ListImageProjects(self.app, None) + self.cmd = _image.ListImageProjects(self.app, None) def test_image_member_list(self): arglist = [ @@ -1028,7 +1028,7 @@ def setUp(self): self.domain_mock.get.return_value = self.domain self.client.remove_member.return_value = None # Get the command object to test - self.cmd = image.RemoveProjectImage(self.app, None) + self.cmd = _image.RemoveProjectImage(self.app, None) def test_remove_project_image_no_options(self): arglist = [ @@ -1095,7 +1095,7 @@ def setUp(self): ) # Get the command object to test - self.cmd = image.SetImage(self.app, None) + self.cmd = _image.SetImage(self.app, None) def test_image_set_no_options(self): arglist = [ @@ -1624,7 +1624,7 @@ def setUp(self): self.client.find_image = mock.Mock(return_value=self._data) # Get the command object to test - self.cmd = image.ShowImage(self.app, None) + self.cmd = _image.ShowImage(self.app, None) def test_image_show(self): arglist = [ @@ -1689,7 +1689,7 @@ def setUp(self): self.client.update_image.return_value = self.image # Get the command object to test - self.cmd = image.UnsetImage(self.app, None) + self.cmd = _image.UnsetImage(self.app, None) def test_image_unset_no_options(self): arglist = [ @@ -1769,6 +1769,60 @@ def test_image_unset_mixed_option(self): self.assertIsNone(result) +class TestImageStage(TestImage): + + image = image_fakes.create_one_image({}) + + def setUp(self): + super().setUp() + + self.client.find_image.return_value = self.image + + self.cmd = _image.StageImage(self.app, None) + + def test_stage_image__from_file(self): + imagefile = tempfile.NamedTemporaryFile(delete=False) + imagefile.write(b'\0') + imagefile.close() + + arglist = [ + '--file', imagefile.name, + self.image.name, + ] + verifylist = [ + ('filename', imagefile.name), + ('image', self.image.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.cmd.take_action(parsed_args) + + self.client.stage_image.assert_called_once_with( + self.image, + filename=imagefile.name, + ) + + @mock.patch('openstackclient.image.v2.image.get_data_from_stdin') + def test_stage_image__from_stdin(self, mock_get_data_from_stdin): + fake_stdin = io.BytesIO(b"some initial binary data: \x00\x01") + mock_get_data_from_stdin.return_value = fake_stdin + + arglist = [ + self.image.name, + ] + verifylist = [ + ('image', self.image.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.cmd.take_action(parsed_args) + + self.client.stage_image.assert_called_once_with( + self.image, + data=fake_stdin, + ) + + class TestImageSave(TestImage): image = image_fakes.create_one_image({}) @@ -1780,7 +1834,7 @@ def setUp(self): self.client.download_image.return_value = self.image # Get the command object to test - self.cmd = image.SaveImage(self.app, None) + self.cmd = _image.SaveImage(self.app, None) def test_save_data(self): @@ -1810,7 +1864,7 @@ def test_get_data_from_stdin(self): stdin.isatty.return_value = False stdin.buffer = fd - test_fd = image.get_data_from_stdin() + test_fd = _image.get_data_from_stdin() # Ensure data written to temp file is correct self.assertEqual(fd, test_fd) @@ -1822,6 +1876,6 @@ def test_get_data_from_stdin__interactive(self): # There is stdin, but interactive stdin.return_value = fd - test_fd = image.get_data_from_stdin() + test_fd = _image.get_data_from_stdin() self.assertIsNone(test_fd) diff --git a/releasenotes/notes/image-stage-ac19c47e6a52ffeb.yaml b/releasenotes/notes/image-stage-ac19c47e6a52ffeb.yaml new file mode 100644 index 000000000..10bd0497e --- /dev/null +++ b/releasenotes/notes/image-stage-ac19c47e6a52ffeb.yaml @@ -0,0 +1,5 @@ +--- +features: + - | + Added a new command, ``image stage``, that will allow users to upload data + for an image to staging. diff --git a/setup.cfg b/setup.cfg index f8d0dffce..7900bbe21 100644 --- a/setup.cfg +++ b/setup.cfg @@ -383,6 +383,7 @@ openstack.image.v2 = image_show = openstackclient.image.v2.image:ShowImage image_set = openstackclient.image.v2.image:SetImage image_unset = openstackclient.image.v2.image:UnsetImage + image_stage = openstackclient.image.v2.image:StageImage image_task_show = openstackclient.image.v2.task:ShowTask image_task_list = openstackclient.image.v2.task:ListTask image_metadef_namespace_list = openstackclient.image.v2.metadef_namespaces:ListMetadefNameSpaces From 4eea3408dc492e948671b625ffc4379212b5857c Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 20 Oct 2022 13:49:17 +0100 Subject: [PATCH 016/586] image: Add 'image import' command Note that we require some additional functionality in SDK for this to work properly, but it's a start. Change-Id: I87f94db6cced67f36f71685e791416f9eed16bd0 Signed-off-by: Stephen Finucane --- doc/source/cli/data/glance.csv | 2 +- openstackclient/image/v2/image.py | 243 ++++++++++++++++++ openstackclient/tests/unit/image/v2/fakes.py | 37 ++- .../tests/unit/image/v2/test_image.py | 191 ++++++++++++++ .../notes/image-import-d5da3e5ce8733fb0.yaml | 6 + 5 files changed, 473 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/image-import-d5da3e5ce8733fb0.yaml diff --git a/doc/source/cli/data/glance.csv b/doc/source/cli/data/glance.csv index 26f720cdb..d5c65f2dc 100644 --- a/doc/source/cli/data/glance.csv +++ b/doc/source/cli/data/glance.csv @@ -4,7 +4,7 @@ image-create-via-import,,EXPERIMENTAL: Create a new image via image import. image-deactivate,image set --deactivate,Deactivate specified image. image-delete,image delete,Delete specified image. image-download,image save,Download a specific image. -image-import,,Initiate the image import taskflow. +image-import,image import,Initiate the image import taskflow. image-list,image list,List images you can access. image-reactivate,image set --activate,Reactivate specified image. image-show,image show,Describe a specific image. diff --git a/openstackclient/image/v2/image.py b/openstackclient/image/v2/image.py index 039f1d2dc..c2c0fe394 100644 --- a/openstackclient/image/v2/image.py +++ b/openstackclient/image/v2/image.py @@ -22,6 +22,7 @@ import sys from cinderclient import api_versions +from openstack import exceptions as sdk_exceptions from openstack.image import image_signer from osc_lib.api import utils as api_utils from osc_lib.cli import format_columns @@ -1561,3 +1562,245 @@ def take_action(self, parsed_args): kwargs['data'] = fp image_client.stage_image(image, **kwargs) + + +class ImportImage(command.ShowOne): + _description = _( + "Initiate the image import process.\n" + "This requires support for the interoperable image import process, " + "which was first introduced in Image API version 2.6 " + "(Glance 16.0.0 (Queens))" + ) + + def get_parser(self, prog_name): + parser = super().get_parser(prog_name) + + parser.add_argument( + 'image', + metavar='', + help=_('Image to initiate import process for (name or ID)'), + ) + # TODO(stephenfin): Uncomment help text when we have this command + # implemented + parser.add_argument( + '--method', + metavar='', + default='glance-direct', + dest='import_method', + choices=[ + 'glance-direct', + 'web-download', + 'glance-download', + 'copy-image', + ], + help=_( + "Import method used for image import process. " + "Not all deployments will support all methods. " + # "Valid values can be retrieved with the 'image import " + # "methods' command. " + "The 'glance-direct' method (default) requires images be " + "first staged using the 'image-stage' command." + ), + ) + parser.add_argument( + '--uri', + metavar='', + help=_( + "URI to download the external image " + "(only valid with the 'web-download' import method)" + ), + ) + parser.add_argument( + '--remote-image', + metavar='', + help=_( + "The image of remote glance (ID only) to be imported " + "(only valid with the 'glance-download' import method)" + ), + ) + parser.add_argument( + '--remote-region', + metavar='', + help=_( + "The remote Glance region to download the image from " + "(only valid with the 'glance-download' import method)" + ), + ) + parser.add_argument( + '--remote-service-interface', + metavar='', + help=_( + "The remote Glance service interface to use when importing " + "images " + "(only valid with the 'glance-download' import method)" + ), + ) + stores_group = parser.add_mutually_exclusive_group() + stores_group.add_argument( + '--store', + metavar='', + dest='stores', + nargs='*', + help=_( + "Backend store to upload image to " + "(specify multiple times to upload to multiple stores) " + "(either '--store' or '--all-stores' required with the " + "'copy-image' import method)" + ), + ) + stores_group.add_argument( + '--all-stores', + help=_( + "Make image available to all stores " + "(either '--store' or '--all-stores' required with the " + "'copy-image' import method)" + ), + ) + parser.add_argument( + '--allow-failure', + action='store_true', + dest='allow_failure', + default=True, + help=_( + 'When uploading to multiple stores, indicate that the import ' + 'should be continue should any of the uploads fail. ' + 'Only usable with --stores or --all-stores' + ), + ) + parser.add_argument( + '--disallow-failure', + action='store_true', + dest='allow_failure', + default=True, + help=_( + 'When uploading to multiple stores, indicate that the import ' + 'should be reverted should any of the uploads fail. ' + 'Only usable with --stores or --all-stores' + ), + ) + parser.add_argument( + '--wait', + action='store_true', + help=_('Wait for operation to complete'), + ) + return parser + + def take_action(self, parsed_args): + image_client = self.app.client_manager.image + + try: + import_info = image_client.get_import_info() + except sdk_exceptions.ResourceNotFound: + msg = _( + 'The Image Import feature is not supported by this deployment' + ) + raise exceptions.CommandError(msg) + + import_methods = import_info.import_methods['value'] + + if parsed_args.import_method not in import_methods: + msg = _( + "The '%s' import method is not supported by this deployment. " + "Supported: %s" + ) + raise exceptions.CommandError( + msg % (parsed_args.import_method, ', '.join(import_methods)), + ) + + if parsed_args.import_method == 'web-download': + if not parsed_args.uri: + msg = _( + "The '--uri' option is required when using " + "'--method=web-download'" + ) + raise exceptions.CommandError(msg) + else: + if parsed_args.uri: + msg = _( + "The '--uri' option is only supported when using " + "'--method=web-download'" + ) + raise exceptions.CommandError(msg) + + if parsed_args.import_method == 'glance-download': + if not (parsed_args.remote_region and parsed_args.remote_image): + msg = _( + "The '--remote-region' and '--remote-image' options are " + "required when using '--method=web-download'" + ) + raise exceptions.CommandError(msg) + else: + if parsed_args.remote_region: + msg = _( + "The '--remote-region' option is only supported when " + "using '--method=glance-download'" + ) + raise exceptions.CommandError(msg) + + if parsed_args.remote_image: + msg = _( + "The '--remote-image' option is only supported when using " + "'--method=glance-download'" + ) + raise exceptions.CommandError(msg) + + if parsed_args.remote_service_interface: + msg = _( + "The '--remote-service-interface' option is only " + "supported when using '--method=glance-download'" + ) + raise exceptions.CommandError(msg) + + if parsed_args.import_method == 'copy-image': + if not (parsed_args.stores or parsed_args.all_stores): + msg = _( + "The '--stores' or '--all-stores' options are required " + "when using '--method=copy-image'" + ) + raise exceptions.CommandError(msg) + + image = image_client.find_image(parsed_args.image) + + if not image.container_format and not image.disk_format: + msg = _( + "The 'container_format' and 'disk_format' properties " + "must be set on an image before it can be imported" + ) + raise exceptions.CommandError(msg) + + if parsed_args.import_method == 'glance-direct': + if image.status != 'uploading': + msg = _( + "The 'glance-direct' import method can only be used with " + "an image in status 'uploading'" + ) + raise exceptions.CommandError(msg) + elif parsed_args.import_method == 'web-download': + if image.status != 'queued': + msg = _( + "The 'web-download' import method can only be used with " + "an image in status 'queued'" + ) + raise exceptions.CommandError(msg) + elif parsed_args.import_method == 'copy-image': + if image.status != 'active': + msg = _( + "The 'copy-image' import method can only be used with " + "an image in status 'active'" + ) + raise exceptions.CommandError(msg) + + image_client.import_image( + image, + method=parsed_args.import_method, + # uri=parsed_args.uri, + # remote_region=parsed_args.remote_region, + # remote_image=parsed_args.remote_image, + # remote_service_interface=parsed_args.remote_service_interface, + stores=parsed_args.stores, + all_stores=parsed_args.all_stores, + all_stores_must_succeed=not parsed_args.allow_failure, + ) + + info = _format_image(image) + return zip(*sorted(info.items())) diff --git a/openstackclient/tests/unit/image/v2/fakes.py b/openstackclient/tests/unit/image/v2/fakes.py index 8ce2a7d55..ded9ff313 100644 --- a/openstackclient/tests/unit/image/v2/fakes.py +++ b/openstackclient/tests/unit/image/v2/fakes.py @@ -19,6 +19,7 @@ from openstack.image.v2 import image from openstack.image.v2 import member from openstack.image.v2 import metadef_namespace +from openstack.image.v2 import service_info as _service_info from openstack.image.v2 import task from openstackclient.tests.unit import fakes @@ -39,6 +40,7 @@ def __init__(self, **kwargs): self.reactivate_image = mock.Mock() self.deactivate_image = mock.Mock() self.stage_image = mock.Mock() + self.import_image = mock.Mock() self.members = mock.Mock() self.add_member = mock.Mock() @@ -49,17 +51,15 @@ def __init__(self, **kwargs): self.metadef_namespaces = mock.Mock() self.tasks = mock.Mock() + self.tasks.resource_class = fakes.FakeResource(None, {}) self.get_task = mock.Mock() + self.get_import_info = mock.Mock() + self.auth_token = kwargs['token'] self.management_url = kwargs['endpoint'] self.version = 2.0 - self.tasks = mock.Mock() - self.tasks.resource_class = fakes.FakeResource(None, {}) - - self.metadef_namespaces = mock.Mock() - class TestImagev2(utils.TestCommand): @@ -143,6 +143,33 @@ def create_one_image_member(attrs=None): return member.Member(**image_member_info) +def create_one_import_info(attrs=None): + """Create a fake import info. + + :param attrs: A dictionary with all attributes of import info + :type attrs: dict + :return: A fake Import object. + :rtype: `openstack.image.v2.service_info.Import` + """ + attrs = attrs or {} + + import_info = { + 'import-methods': { + 'description': 'Import methods available.', + 'type': 'array', + 'value': [ + 'glance-direct', + 'web-download', + 'glance-download', + 'copy-image', + ] + } + } + import_info.update(attrs) + + return _service_info.Import(**import_info) + + def create_one_task(attrs=None): """Create a fake task. diff --git a/openstackclient/tests/unit/image/v2/test_image.py b/openstackclient/tests/unit/image/v2/test_image.py index 8dea7f05a..010c4a9d3 100644 --- a/openstackclient/tests/unit/image/v2/test_image.py +++ b/openstackclient/tests/unit/image/v2/test_image.py @@ -1823,6 +1823,197 @@ def test_stage_image__from_stdin(self, mock_get_data_from_stdin): ) +class TestImageImport(TestImage): + + image = image_fakes.create_one_image( + { + 'container_format': 'bare', + 'disk_format': 'qcow2', + } + ) + import_info = image_fakes.create_one_import_info() + + def setUp(self): + super().setUp() + + self.client.find_image.return_value = self.image + self.client.get_import_info.return_value = self.import_info + + self.cmd = _image.ImportImage(self.app, None) + + def test_import_image__glance_direct(self): + self.image.status = 'uploading' + arglist = [ + self.image.name, + ] + verifylist = [ + ('image', self.image.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.cmd.take_action(parsed_args) + + self.client.import_image.assert_called_once_with( + self.image, + method='glance-direct', + stores=None, + all_stores=None, + all_stores_must_succeed=False, + ) + + def test_import_image__web_download(self): + self.image.status = 'queued' + arglist = [ + self.image.name, + '--method', 'web-download', + '--uri', 'https://example.com/', + ] + verifylist = [ + ('image', self.image.name), + ('import_method', 'web-download'), + ('uri', 'https://example.com/'), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.cmd.take_action(parsed_args) + + self.client.import_image.assert_called_once_with( + self.image, + method='web-download', + # uri='https://example.com/', + stores=None, + all_stores=None, + all_stores_must_succeed=False, + ) + + # NOTE(stephenfin): We don't do this for all combinations since that would + # be tedious af. You get the idea... + def test_import_image__web_download_missing_options(self): + arglist = [ + self.image.name, + '--method', 'web-download', + ] + verifylist = [ + ('image', self.image.name), + ('import_method', 'web-download'), + ('uri', None), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + exc = self.assertRaises( + exceptions.CommandError, + self.cmd.take_action, + parsed_args, + ) + self.assertIn("The '--uri' option is required ", str(exc)) + + self.client.import_image.assert_not_called() + + # NOTE(stephenfin): Ditto + def test_import_image__web_download_invalid_options(self): + arglist = [ + self.image.name, + '--method', 'glance-direct', # != web-download + '--uri', 'https://example.com/', + ] + verifylist = [ + ('image', self.image.name), + ('import_method', 'glance-direct'), + ('uri', 'https://example.com/'), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + exc = self.assertRaises( + exceptions.CommandError, + self.cmd.take_action, + parsed_args, + ) + self.assertIn("The '--uri' option is only supported ", str(exc)) + + self.client.import_image.assert_not_called() + + def test_import_image__web_download_invalid_image_state(self): + self.image.status = 'uploading' # != 'queued' + arglist = [ + self.image.name, + '--method', 'web-download', + '--uri', 'https://example.com/', + ] + verifylist = [ + ('image', self.image.name), + ('import_method', 'web-download'), + ('uri', 'https://example.com/'), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + exc = self.assertRaises( + exceptions.CommandError, + self.cmd.take_action, + parsed_args, + ) + self.assertIn( + "The 'web-download' import method can only be used with " + "an image in status 'queued'", + str(exc), + ) + + self.client.import_image.assert_not_called() + + def test_import_image__copy_image(self): + self.image.status = 'active' + arglist = [ + self.image.name, + '--method', 'copy-image', + '--store', 'fast', + ] + verifylist = [ + ('image', self.image.name), + ('import_method', 'copy-image'), + ('stores', ['fast']), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.cmd.take_action(parsed_args) + + self.client.import_image.assert_called_once_with( + self.image, + method='copy-image', + stores=['fast'], + all_stores=None, + all_stores_must_succeed=False, + ) + + def test_import_image__glance_download(self): + arglist = [ + self.image.name, + '--method', 'glance-download', + '--remote-region', 'eu/dublin', + '--remote-image', 'remote-image-id', + '--remote-service-interface', 'private', + ] + verifylist = [ + ('image', self.image.name), + ('import_method', 'glance-download'), + ('remote_region', 'eu/dublin'), + ('remote_image', 'remote-image-id'), + ('remote_service_interface', 'private'), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.cmd.take_action(parsed_args) + + self.client.import_image.assert_called_once_with( + self.image, + method='glance-download', + # remote_region='eu/dublin', + # remote_image='remote-image-id', + # remote_service_interface='private', + stores=None, + all_stores=None, + all_stores_must_succeed=False, + ) + + class TestImageSave(TestImage): image = image_fakes.create_one_image({}) diff --git a/releasenotes/notes/image-import-d5da3e5ce8733fb0.yaml b/releasenotes/notes/image-import-d5da3e5ce8733fb0.yaml new file mode 100644 index 000000000..0c394c82d --- /dev/null +++ b/releasenotes/notes/image-import-d5da3e5ce8733fb0.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + Add ``image import`` command, allowing users to take advantage of the + interoperable image import functionality first introduced in Glance 16.0.0 + (Queens). From 006e35509d3cea66dc13fe2238dac92f47fe3c5c Mon Sep 17 00:00:00 2001 From: Violet Kurtz Date: Mon, 15 Aug 2022 23:52:55 +0000 Subject: [PATCH 017/586] Moved hypervisor to the SDK Change-Id: Ie955fb4d27c30e044626732a1f3e0f141cb85aa5 --- openstackclient/compute/v2/hypervisor.py | 137 ++++++++--- .../tests/unit/compute/v2/fakes.py | 134 +++++----- .../tests/unit/compute/v2/test_hypervisor.py | 230 +++++++++++------- ...ch-hypervisor-to-sdk-2e90b26a14ffcef3.yaml | 3 + 4 files changed, 303 insertions(+), 201 deletions(-) create mode 100644 releasenotes/notes/switch-hypervisor-to-sdk-2e90b26a14ffcef3.yaml diff --git a/openstackclient/compute/v2/hypervisor.py b/openstackclient/compute/v2/hypervisor.py index 5f7497b5b..d4b4003bf 100644 --- a/openstackclient/compute/v2/hypervisor.py +++ b/openstackclient/compute/v2/hypervisor.py @@ -18,8 +18,8 @@ import json import re -from novaclient import api_versions from novaclient import exceptions as nova_exceptions +from openstack import utils as sdk_utils from osc_lib.cli import format_columns from osc_lib.command import command from osc_lib import exceptions @@ -28,11 +28,44 @@ from openstackclient.i18n import _ +def _get_hypervisor_columns(item, client): + column_map = {'name': 'hypervisor_hostname'} + hidden_columns = ['location', 'servers'] + + if sdk_utils.supports_microversion(client, '2.88'): + hidden_columns.extend([ + 'current_workload', + 'disk_available', + 'local_disk_free', + 'local_disk_size', + 'local_disk_used', + 'memory_free', + 'memory_size', + 'memory_used', + 'running_vms', + 'vcpus_used', + 'vcpus', + ]) + else: + column_map.update({ + 'disk_available': 'disk_available_least', + 'local_disk_free': 'free_disk_gb', + 'local_disk_size': 'local_gb', + 'local_disk_used': 'local_gb_used', + 'memory_free': 'free_ram_mb', + 'memory_used': 'memory_mb_used', + 'memory_size': 'memory_mb', + }) + + return utils.get_osc_show_columns_for_sdk_resource( + item, column_map, hidden_columns) + + class ListHypervisor(command.Lister): _description = _("List hypervisors") def get_parser(self, prog_name): - parser = super(ListHypervisor, self).get_parser(prog_name) + parser = super().get_parser(prog_name) parser.add_argument( '--matching', metavar='', @@ -67,7 +100,7 @@ def get_parser(self, prog_name): return parser def take_action(self, parsed_args): - compute_client = self.app.client_manager.compute + compute_client = self.app.client_manager.sdk_connection.compute list_opts = {} @@ -78,7 +111,7 @@ def take_action(self, parsed_args): raise exceptions.CommandError(msg) if parsed_args.marker: - if compute_client.api_version < api_versions.APIVersion('2.33'): + if not sdk_utils.supports_microversion(compute_client, '2.33'): msg = _( '--os-compute-api-version 2.33 or greater is required to ' 'support the --marker option' @@ -87,7 +120,7 @@ def take_action(self, parsed_args): list_opts['marker'] = parsed_args.marker if parsed_args.limit: - if compute_client.api_version < api_versions.APIVersion('2.33'): + if not sdk_utils.supports_microversion(compute_client, '2.33'): msg = _( '--os-compute-api-version 2.33 or greater is required to ' 'support the --limit option' @@ -95,23 +128,43 @@ def take_action(self, parsed_args): raise exceptions.CommandError(msg) list_opts['limit'] = parsed_args.limit - columns = ( + column_headers = ( "ID", "Hypervisor Hostname", "Hypervisor Type", "Host IP", "State" ) + columns = ( + 'id', + 'name', + 'hypervisor_type', + 'host_ip', + 'state' + ) if parsed_args.long: - columns += ("vCPUs Used", "vCPUs", "Memory MB Used", "Memory MB") + if not sdk_utils.supports_microversion(compute_client, '2.88'): + column_headers += ( + 'vCPUs Used', + 'vCPUs', + 'Memory MB Used', + 'Memory MB' + ) + columns += ( + 'vcpus_used', + 'vcpus', + 'memory_used', + 'memory_size' + ) if parsed_args.matching: - data = compute_client.hypervisors.search(parsed_args.matching) + data = compute_client.find_hypervisor( + parsed_args.matching, ignore_missing=False) else: - data = compute_client.hypervisors.list(**list_opts) + data = compute_client.hypervisors(**list_opts, details=True) return ( - columns, + column_headers, (utils.get_item_properties(s, columns) for s in data), ) @@ -120,7 +173,7 @@ class ShowHypervisor(command.ShowOne): _description = _("Display hypervisor details") def get_parser(self, prog_name): - parser = super(ShowHypervisor, self).get_parser(prog_name) + parser = super().get_parser(prog_name) parser.add_argument( "hypervisor", metavar="", @@ -129,20 +182,25 @@ def get_parser(self, prog_name): return parser def take_action(self, parsed_args): - compute_client = self.app.client_manager.compute - hypervisor = utils.find_resource(compute_client.hypervisors, - parsed_args.hypervisor)._info.copy() + compute_client = self.app.client_manager.sdk_connection.compute + hypervisor = compute_client.find_hypervisor( + parsed_args.hypervisor, ignore_missing=False).copy() + + # Some of the properties in the hypervisor object need to be processed + # before they get reported to the user. We spend this section + # extracting the relevant details to be reported by modifying our + # copy of the hypervisor object. + aggregates = compute_client.aggregates() + hypervisor['aggregates'] = list() + service_details = hypervisor['service_details'] - aggregates = compute_client.aggregates.list() - hypervisor["aggregates"] = list() if aggregates: # Hypervisors in nova cells are prefixed by "@" - if "@" in hypervisor['service']['host']: - cell, service_host = hypervisor['service']['host'].split( - '@', 1) + if "@" in service_details['host']: + cell, service_host = service_details['host'].split('@', 1) else: cell = None - service_host = hypervisor['service']['host'] + service_host = service_details['host'] if cell: # The host aggregates are also prefixed by "@" @@ -154,42 +212,45 @@ def take_action(self, parsed_args): member_of = [aggregate.name for aggregate in aggregates if service_host in aggregate.hosts] - hypervisor["aggregates"] = member_of + hypervisor['aggregates'] = member_of try: - uptime = compute_client.hypervisors.uptime(hypervisor['id'])._info + if sdk_utils.supports_microversion(compute_client, '2.88'): + uptime = hypervisor['uptime'] or '' + del hypervisor['uptime'] + else: + del hypervisor['uptime'] + uptime = compute_client.get_hypervisor_uptime( + hypervisor['id'])['uptime'] # Extract data from uptime value # format: 0 up 0, 0 users, load average: 0, 0, 0 # example: 17:37:14 up 2:33, 3 users, # load average: 0.33, 0.36, 0.34 m = re.match( r"\s*(.+)\sup\s+(.+),\s+(.+)\susers?,\s+load average:\s(.+)", - uptime['uptime']) + uptime) if m: - hypervisor["host_time"] = m.group(1) - hypervisor["uptime"] = m.group(2) - hypervisor["users"] = m.group(3) - hypervisor["load_average"] = m.group(4) + hypervisor['host_time'] = m.group(1) + hypervisor['uptime'] = m.group(2) + hypervisor['users'] = m.group(3) + hypervisor['load_average'] = m.group(4) except nova_exceptions.HTTPNotImplemented: pass - hypervisor["service_id"] = hypervisor["service"]["id"] - hypervisor["service_host"] = hypervisor["service"]["host"] - del hypervisor["service"] + hypervisor['service_id'] = service_details['id'] + hypervisor['service_host'] = service_details['host'] + del hypervisor['service_details'] - if compute_client.api_version < api_versions.APIVersion('2.28'): + if not sdk_utils.supports_microversion(compute_client, '2.28'): # microversion 2.28 transformed this to a JSON blob rather than a # string; on earlier fields, do this manually - if hypervisor['cpu_info']: - hypervisor['cpu_info'] = json.loads(hypervisor['cpu_info']) - else: - hypervisor['cpu_info'] = {} - - columns = tuple(sorted(hypervisor)) + hypervisor['cpu_info'] = json.loads(hypervisor['cpu_info'] or '{}') + display_columns, columns = _get_hypervisor_columns( + hypervisor, compute_client) data = utils.get_dict_properties( hypervisor, columns, formatters={ 'cpu_info': format_columns.DictColumn, }) - return (columns, data) + return display_columns, data diff --git a/openstackclient/tests/unit/compute/v2/fakes.py b/openstackclient/tests/unit/compute/v2/fakes.py index d77797abd..7d8c29ad7 100644 --- a/openstackclient/tests/unit/compute/v2/fakes.py +++ b/openstackclient/tests/unit/compute/v2/fakes.py @@ -20,6 +20,7 @@ from novaclient import api_versions from openstack.compute.v2 import flavor as _flavor +from openstack.compute.v2 import hypervisor as _hypervisor from openstack.compute.v2 import server from openstack.compute.v2 import server_group as _server_group from openstack.compute.v2 import server_interface as _server_interface @@ -340,75 +341,6 @@ def create_one_extension(attrs=None): return extension -class FakeHypervisor(object): - """Fake one or more hypervisor.""" - - @staticmethod - def create_one_hypervisor(attrs=None): - """Create a fake hypervisor. - - :param dict attrs: - A dictionary with all attributes - :return: - A FakeResource object, with id, hypervisor_hostname, and so on - """ - attrs = attrs or {} - - # Set default attributes. - hypervisor_info = { - 'id': 'hypervisor-id-' + uuid.uuid4().hex, - 'hypervisor_hostname': 'hypervisor-hostname-' + uuid.uuid4().hex, - 'status': 'enabled', - 'host_ip': '192.168.0.10', - 'cpu_info': { - 'aaa': 'aaa', - }, - 'free_disk_gb': 50, - 'hypervisor_version': 2004001, - 'disk_available_least': 50, - 'local_gb': 50, - 'free_ram_mb': 1024, - 'service': { - 'host': 'aaa', - 'disabled_reason': None, - 'id': 1, - }, - 'vcpus_used': 0, - 'hypervisor_type': 'QEMU', - 'local_gb_used': 0, - 'vcpus': 4, - 'memory_mb_used': 512, - 'memory_mb': 1024, - 'current_workload': 0, - 'state': 'up', - 'running_vms': 0, - } - - # Overwrite default attributes. - hypervisor_info.update(attrs) - - hypervisor = fakes.FakeResource(info=copy.deepcopy(hypervisor_info), - loaded=True) - return hypervisor - - @staticmethod - def create_hypervisors(attrs=None, count=2): - """Create multiple fake hypervisors. - - :param dict attrs: - A dictionary with all attributes - :param int count: - The number of hypervisors to fake - :return: - A list of FakeResource objects faking the hypervisors - """ - hypervisors = [] - for i in range(0, count): - hypervisors.append(FakeHypervisor.create_one_hypervisor(attrs)) - - return hypervisors - - class FakeHypervisorStats(object): """Fake one or more hypervisor stats.""" @@ -1795,6 +1727,70 @@ def create_sdk_volume_attachments(attrs=None, methods=None, count=2): return volume_attachments +def create_one_hypervisor(attrs=None): + """Create a fake hypervisor. + + :param dict attrs: + A dictionary with all attributes + :return: + A FakeResource object, with id, hypervisor_hostname, and so on + """ + attrs = attrs or {} + + # Set default attributes. + hypervisor_info = { + 'id': 'hypervisor-id-' + uuid.uuid4().hex, + 'hypervisor_hostname': 'hypervisor-hostname-' + uuid.uuid4().hex, + 'status': 'enabled', + 'host_ip': '192.168.0.10', + 'cpu_info': { + 'aaa': 'aaa', + }, + 'free_disk_gb': 50, + 'hypervisor_version': 2004001, + 'disk_available_least': 50, + 'local_gb': 50, + 'free_ram_mb': 1024, + 'service': { + 'host': 'aaa', + 'disabled_reason': None, + 'id': 1, + }, + 'vcpus_used': 0, + 'hypervisor_type': 'QEMU', + 'local_gb_used': 0, + 'vcpus': 4, + 'memory_mb_used': 512, + 'memory_mb': 1024, + 'current_workload': 0, + 'state': 'up', + 'running_vms': 0, + } + + # Overwrite default attributes. + hypervisor_info.update(attrs) + + hypervisor = _hypervisor.Hypervisor(**hypervisor_info, loaded=True) + return hypervisor + + +def create_hypervisors(attrs=None, count=2): + """Create multiple fake hypervisors. + + :param dict attrs: + A dictionary with all attributes + :param int count: + The number of hypervisors to fake + :return: + A list of FakeResource objects faking the hypervisors + """ + hypervisors = [] + for i in range(0, count): + hypervisors.append(create_one_hypervisor(attrs)) + + return hypervisors + + def create_one_server_group(attrs=None): """Create a fake server group diff --git a/openstackclient/tests/unit/compute/v2/test_hypervisor.py b/openstackclient/tests/unit/compute/v2/test_hypervisor.py index 7dbd6e198..e5804665c 100644 --- a/openstackclient/tests/unit/compute/v2/test_hypervisor.py +++ b/openstackclient/tests/unit/compute/v2/test_hypervisor.py @@ -13,41 +13,37 @@ # under the License. # -import copy import json +from unittest import mock -from novaclient import api_versions from novaclient import exceptions as nova_exceptions +from openstack import utils as sdk_utils from osc_lib.cli import format_columns from osc_lib import exceptions from openstackclient.compute.v2 import hypervisor from openstackclient.tests.unit.compute.v2 import fakes as compute_fakes -from openstackclient.tests.unit import fakes class TestHypervisor(compute_fakes.TestComputev2): def setUp(self): - super(TestHypervisor, self).setUp() + super().setUp() - # Get a shortcut to the compute client hypervisors mock - self.hypervisors_mock = self.app.client_manager.compute.hypervisors - self.hypervisors_mock.reset_mock() - - # Get a shortcut to the compute client aggregates mock - self.aggregates_mock = self.app.client_manager.compute.aggregates - self.aggregates_mock.reset_mock() + # Create and get a shortcut to the compute client mock + self.app.client_manager.sdk_connection = mock.Mock() + self.sdk_client = self.app.client_manager.sdk_connection.compute + self.sdk_client.reset_mock() class TestHypervisorList(TestHypervisor): def setUp(self): - super(TestHypervisorList, self).setUp() + super().setUp() # Fake hypervisors to be listed up - self.hypervisors = compute_fakes.FakeHypervisor.create_hypervisors() - self.hypervisors_mock.list.return_value = self.hypervisors + self.hypervisors = compute_fakes.create_hypervisors() + self.sdk_client.hypervisors.return_value = self.hypervisors self.columns = ( "ID", @@ -70,14 +66,14 @@ def setUp(self): self.data = ( ( self.hypervisors[0].id, - self.hypervisors[0].hypervisor_hostname, + self.hypervisors[0].name, self.hypervisors[0].hypervisor_type, self.hypervisors[0].host_ip, self.hypervisors[0].state ), ( self.hypervisors[1].id, - self.hypervisors[1].hypervisor_hostname, + self.hypervisors[1].name, self.hypervisors[1].hypervisor_type, self.hypervisors[1].host_ip, self.hypervisors[1].state @@ -87,25 +83,25 @@ def setUp(self): self.data_long = ( ( self.hypervisors[0].id, - self.hypervisors[0].hypervisor_hostname, + self.hypervisors[0].name, self.hypervisors[0].hypervisor_type, self.hypervisors[0].host_ip, self.hypervisors[0].state, self.hypervisors[0].vcpus_used, self.hypervisors[0].vcpus, - self.hypervisors[0].memory_mb_used, - self.hypervisors[0].memory_mb + self.hypervisors[0].memory_used, + self.hypervisors[0].memory_size ), ( self.hypervisors[1].id, - self.hypervisors[1].hypervisor_hostname, + self.hypervisors[1].name, self.hypervisors[1].hypervisor_type, self.hypervisors[1].host_ip, self.hypervisors[1].state, self.hypervisors[1].vcpus_used, self.hypervisors[1].vcpus, - self.hypervisors[1].memory_mb_used, - self.hypervisors[1].memory_mb + self.hypervisors[1].memory_used, + self.hypervisors[1].memory_size ), ) # Get the command object to test @@ -121,25 +117,25 @@ def test_hypervisor_list_no_option(self): # containing the data to be listed. columns, data = self.cmd.take_action(parsed_args) - self.hypervisors_mock.list.assert_called_with() + self.sdk_client.hypervisors.assert_called_with(details=True) self.assertEqual(self.columns, columns) self.assertEqual(self.data, tuple(data)) def test_hypervisor_list_matching_option_found(self): arglist = [ - '--matching', self.hypervisors[0].hypervisor_hostname, + '--matching', self.hypervisors[0].name, ] verifylist = [ - ('matching', self.hypervisors[0].hypervisor_hostname), + ('matching', self.hypervisors[0].name), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) # Fake the return value of search() - self.hypervisors_mock.search.return_value = [self.hypervisors[0]] + self.sdk_client.find_hypervisor.return_value = [self.hypervisors[0]] self.data = ( ( self.hypervisors[0].id, - self.hypervisors[0].hypervisor_hostname, + self.hypervisors[0].name, self.hypervisors[1].hypervisor_type, self.hypervisors[1].host_ip, self.hypervisors[1].state, @@ -151,8 +147,9 @@ def test_hypervisor_list_matching_option_found(self): # containing the data to be listed. columns, data = self.cmd.take_action(parsed_args) - self.hypervisors_mock.search.assert_called_with( - self.hypervisors[0].hypervisor_hostname + self.sdk_client.find_hypervisor.assert_called_with( + self.hypervisors[0].name, + ignore_missing=False ) self.assertEqual(self.columns, columns) self.assertEqual(self.data, tuple(data)) @@ -167,25 +164,25 @@ def test_hypervisor_list_matching_option_not_found(self): parsed_args = self.check_parser(self.cmd, arglist, verifylist) # Fake exception raised from search() - self.hypervisors_mock.search.side_effect = exceptions.NotFound(None) + self.sdk_client.find_hypervisor.side_effect = \ + exceptions.NotFound(None) self.assertRaises(exceptions.NotFound, self.cmd.take_action, parsed_args) - def test_hypervisor_list_with_matching_and_pagination_options(self): - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.32') - + @mock.patch.object(sdk_utils, 'supports_microversion', return_value=False) + def test_hypervisor_list_with_matching_and_pagination_options( + self, sm_mock): arglist = [ - '--matching', self.hypervisors[0].hypervisor_hostname, + '--matching', self.hypervisors[0].name, '--limit', '1', - '--marker', self.hypervisors[0].hypervisor_hostname, + '--marker', self.hypervisors[0].name, ] verifylist = [ - ('matching', self.hypervisors[0].hypervisor_hostname), + ('matching', self.hypervisors[0].name), ('limit', 1), - ('marker', self.hypervisors[0].hypervisor_hostname), + ('marker', self.hypervisors[0].name), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -197,7 +194,8 @@ def test_hypervisor_list_with_matching_and_pagination_options(self): self.assertIn( '--matching is not compatible with --marker or --limit', str(ex)) - def test_hypervisor_list_long_option(self): + @mock.patch.object(sdk_utils, 'supports_microversion', return_value=False) + def test_hypervisor_list_long_option(self, sm_mock): arglist = [ '--long', ] @@ -211,14 +209,12 @@ def test_hypervisor_list_long_option(self): # containing the data to be listed. columns, data = self.cmd.take_action(parsed_args) - self.hypervisors_mock.list.assert_called_with() + self.sdk_client.hypervisors.assert_called_with(details=True) self.assertEqual(self.columns_long, columns) self.assertEqual(self.data_long, tuple(data)) - def test_hypervisor_list_with_limit(self): - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.33') - + @mock.patch.object(sdk_utils, 'supports_microversion', return_value=True) + def test_hypervisor_list_with_limit(self, sm_mock): arglist = [ '--limit', '1', ] @@ -229,12 +225,10 @@ def test_hypervisor_list_with_limit(self): parsed_args = self.check_parser(self.cmd, arglist, verifylist) self.cmd.take_action(parsed_args) - self.hypervisors_mock.list.assert_called_with(limit=1) - - def test_hypervisor_list_with_limit_pre_v233(self): - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.32') + self.sdk_client.hypervisors.assert_called_with(limit=1, details=True) + @mock.patch.object(sdk_utils, 'supports_microversion', return_value=False) + def test_hypervisor_list_with_limit_pre_v233(self, sm_mock): arglist = [ '--limit', '1', ] @@ -251,10 +245,8 @@ def test_hypervisor_list_with_limit_pre_v233(self): self.assertIn( '--os-compute-api-version 2.33 or greater is required', str(ex)) - def test_hypervisor_list_with_marker(self): - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.33') - + @mock.patch.object(sdk_utils, 'supports_microversion', return_value=True) + def test_hypervisor_list_with_marker(self, sm_mock): arglist = [ '--marker', 'test_hyp', ] @@ -265,12 +257,11 @@ def test_hypervisor_list_with_marker(self): parsed_args = self.check_parser(self.cmd, arglist, verifylist) self.cmd.take_action(parsed_args) - self.hypervisors_mock.list.assert_called_with(marker='test_hyp') - - def test_hypervisor_list_with_marker_pre_v233(self): - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.32') + self.sdk_client.hypervisors.assert_called_with( + marker='test_hyp', details=True) + @mock.patch.object(sdk_utils, 'supports_microversion', return_value=False) + def test_hypervisor_list_with_marker_pre_v233(self, sm_mock): arglist = [ '--marker', 'test_hyp', ] @@ -291,29 +282,66 @@ def test_hypervisor_list_with_marker_pre_v233(self): class TestHypervisorShow(TestHypervisor): def setUp(self): - super(TestHypervisorShow, self).setUp() + super().setUp() + + uptime_string = (' 01:28:24 up 3 days, 11:15, 1 user, ' + ' load average: 0.94, 0.62, 0.50\n') # Fake hypervisors to be listed up - self.hypervisor = compute_fakes.FakeHypervisor.create_one_hypervisor() + self.hypervisor = compute_fakes.create_one_hypervisor(attrs={ + 'uptime': uptime_string, + }) - # Return value of utils.find_resource() - self.hypervisors_mock.get.return_value = self.hypervisor + # Return value of compute_client.find_hypervisor + self.sdk_client.find_hypervisor.return_value = self.hypervisor - # Return value of compute_client.aggregates.list() - self.aggregates_mock.list.return_value = [] + # Return value of compute_client.aggregates() + self.sdk_client.aggregates.return_value = [] - # Return value of compute_client.hypervisors.uptime() + # Return value of compute_client.get_hypervisor_uptime() uptime_info = { 'status': self.hypervisor.status, 'state': self.hypervisor.state, 'id': self.hypervisor.id, - 'hypervisor_hostname': self.hypervisor.hypervisor_hostname, - 'uptime': ' 01:28:24 up 3 days, 11:15, 1 user, ' - ' load average: 0.94, 0.62, 0.50\n', + 'hypervisor_hostname': self.hypervisor.name, + 'uptime': uptime_string, } - self.hypervisors_mock.uptime.return_value = fakes.FakeResource( - info=copy.deepcopy(uptime_info), - loaded=True + self.sdk_client.get_hypervisor_uptime.return_value = uptime_info + + self.columns_v288 = ( + 'aggregates', + 'cpu_info', + 'host_ip', + 'host_time', + 'hypervisor_hostname', + 'hypervisor_type', + 'hypervisor_version', + 'id', + 'load_average', + 'service_host', + 'service_id', + 'state', + 'status', + 'uptime', + 'users', + ) + + self.data_v288 = ( + [], + format_columns.DictColumn({'aaa': 'aaa'}), + '192.168.0.10', + '01:28:24', + self.hypervisor.name, + 'QEMU', + 2004001, + self.hypervisor.id, + '0.94, 0.62, 0.50', + 'aaa', + 1, + 'up', + 'enabled', + '3 days, 11:15', + '1', ) self.columns = ( @@ -353,7 +381,7 @@ def setUp(self): 1024, '192.168.0.10', '01:28:24', - self.hypervisor.hypervisor_hostname, + self.hypervisor.name, 'QEMU', 2004001, self.hypervisor.id, @@ -376,15 +404,32 @@ def setUp(self): # Get the command object to test self.cmd = hypervisor.ShowHypervisor(self.app, None) - def test_hypervisor_show(self): - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.28') + @mock.patch.object(sdk_utils, 'supports_microversion', return_value=True) + def test_hypervisor_show(self, sm_mock): + arglist = [ + self.hypervisor.name, + ] + verifylist = [ + ('hypervisor', self.hypervisor.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + # In base command class ShowOne in cliff, abstract method take_action() + # returns a two-part tuple with a tuple of column names and a tuple of + # data to be shown. + columns, data = self.cmd.take_action(parsed_args) + + self.assertEqual(self.columns_v288, columns) + self.assertCountEqual(self.data_v288, data) + + @mock.patch.object(sdk_utils, 'supports_microversion', + side_effect=[False, True, False]) + def test_hypervisor_show_pre_v288(self, sm_mock): arglist = [ - self.hypervisor.hypervisor_hostname, + self.hypervisor.name, ] verifylist = [ - ('hypervisor', self.hypervisor.hypervisor_hostname), + ('hypervisor', self.hypervisor.name), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -396,21 +441,19 @@ def test_hypervisor_show(self): self.assertEqual(self.columns, columns) self.assertCountEqual(self.data, data) - def test_hypervisor_show_pre_v228(self): - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.27') - + @mock.patch.object(sdk_utils, 'supports_microversion', return_value=False) + def test_hypervisor_show_pre_v228(self, sm_mock): # before microversion 2.28, nova returned a stringified version of this # field - self.hypervisor._info['cpu_info'] = json.dumps( - self.hypervisor._info['cpu_info']) - self.hypervisors_mock.get.return_value = self.hypervisor + self.hypervisor.cpu_info = json.dumps( + self.hypervisor.cpu_info) + self.sdk_client.find_hypervisor.return_value = self.hypervisor arglist = [ - self.hypervisor.hypervisor_hostname, + self.hypervisor.name, ] verifylist = [ - ('hypervisor', self.hypervisor.hypervisor_hostname), + ('hypervisor', self.hypervisor.name), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -422,19 +465,18 @@ def test_hypervisor_show_pre_v228(self): self.assertEqual(self.columns, columns) self.assertCountEqual(self.data, data) - def test_hypervisor_show_uptime_not_implemented(self): - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.28') - + @mock.patch.object(sdk_utils, 'supports_microversion', + side_effect=[False, True, False]) + def test_hypervisor_show_uptime_not_implemented(self, sm_mock): arglist = [ - self.hypervisor.hypervisor_hostname, + self.hypervisor.name, ] verifylist = [ - ('hypervisor', self.hypervisor.hypervisor_hostname), + ('hypervisor', self.hypervisor.name), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - self.hypervisors_mock.uptime.side_effect = ( + self.sdk_client.get_hypervisor_uptime.side_effect = ( nova_exceptions.HTTPNotImplemented(501)) # In base command class ShowOne in cliff, abstract method take_action() @@ -474,7 +516,7 @@ def test_hypervisor_show_uptime_not_implemented(self): 50, 1024, '192.168.0.10', - self.hypervisor.hypervisor_hostname, + self.hypervisor.name, 'QEMU', 2004001, self.hypervisor.id, diff --git a/releasenotes/notes/switch-hypervisor-to-sdk-2e90b26a14ffcef3.yaml b/releasenotes/notes/switch-hypervisor-to-sdk-2e90b26a14ffcef3.yaml new file mode 100644 index 000000000..c5167929e --- /dev/null +++ b/releasenotes/notes/switch-hypervisor-to-sdk-2e90b26a14ffcef3.yaml @@ -0,0 +1,3 @@ +--- +features: + - Switch hypervisor to the OpenStackSDK From 348eb796321c8475af73b727a310c3a09f519ffa Mon Sep 17 00:00:00 2001 From: Jadon Naas Date: Thu, 10 Nov 2022 11:51:18 -0500 Subject: [PATCH 018/586] Docstring fix for CreateVolumeAttachment class The command "volume attachment create" has a typo in the docstring. The docstring says to use "server add volume", but the command is actually "server volume add". This change fixes the typo in the docstring. Task: 46781 Story: 2010401 Change-Id: Ie19a24ead100dd9177669653a7a9997772ef4538 --- openstackclient/volume/v3/volume_attachment.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openstackclient/volume/v3/volume_attachment.py b/openstackclient/volume/v3/volume_attachment.py index c74018379..57a6da734 100644 --- a/openstackclient/volume/v3/volume_attachment.py +++ b/openstackclient/volume/v3/volume_attachment.py @@ -82,7 +82,7 @@ class CreateVolumeAttachment(command.ShowOne): the volume to the server at the hypervisor level. As a result, it should typically only be used for troubleshooting issues with an existing server in combination with other tooling. For all other use cases, the 'server - volume add' command should be preferred. + add volume' command should be preferred. """ def get_parser(self, prog_name): From bd0727c4f897289722ba639930c9e979cfee534a Mon Sep 17 00:00:00 2001 From: whoami-rajat Date: Thu, 17 Nov 2022 18:35:01 +0530 Subject: [PATCH 019/586] Add option to create volume from backup Support for creating a volume from backup was added in microversio 3.47. This patch adds a --backup option to the volume create command to add that support. Change-Id: Ib26d2d335475d9aacbf77c0fd7b7cda2ba743943 --- .../tests/unit/volume/v2/test_volume.py | 73 +++++++++++++++++++ openstackclient/volume/v2/volume.py | 29 +++++++- ...option-to-create-vol-fc36c2c745ebcff5.yaml | 4 + 3 files changed, 103 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/add-backup-option-to-create-vol-fc36c2c745ebcff5.yaml diff --git a/openstackclient/tests/unit/volume/v2/test_volume.py b/openstackclient/tests/unit/volume/v2/test_volume.py index f802f637f..ef9c2fab5 100644 --- a/openstackclient/tests/unit/volume/v2/test_volume.py +++ b/openstackclient/tests/unit/volume/v2/test_volume.py @@ -16,6 +16,7 @@ from unittest import mock from unittest.mock import call +from cinderclient import api_versions from osc_lib.cli import format_columns from osc_lib import exceptions from osc_lib import utils @@ -47,6 +48,9 @@ def setUp(self): self.snapshots_mock = self.app.client_manager.volume.volume_snapshots self.snapshots_mock.reset_mock() + self.backups_mock = self.app.client_manager.volume.backups + self.backups_mock.reset_mock() + self.types_mock = self.app.client_manager.volume.volume_types self.types_mock.reset_mock() @@ -129,6 +133,7 @@ def test_volume_create_min_options(self): source_volid=None, consistencygroup_id=None, scheduler_hints=None, + backup_id=None, ) self.assertEqual(self.columns, columns) @@ -174,6 +179,7 @@ def test_volume_create_options(self): source_volid=None, consistencygroup_id=consistency_group.id, scheduler_hints={'k': 'v'}, + backup_id=None, ) self.assertEqual(self.columns, columns) @@ -210,6 +216,7 @@ def test_volume_create_properties(self): source_volid=None, consistencygroup_id=None, scheduler_hints=None, + backup_id=None, ) self.assertEqual(self.columns, columns) @@ -248,6 +255,7 @@ def test_volume_create_image_id(self): source_volid=None, consistencygroup_id=None, scheduler_hints=None, + backup_id=None, ) self.assertEqual(self.columns, columns) @@ -286,6 +294,7 @@ def test_volume_create_image_name(self): source_volid=None, consistencygroup_id=None, scheduler_hints=None, + backup_id=None, ) self.assertEqual(self.columns, columns) @@ -323,11 +332,72 @@ def test_volume_create_with_snapshot(self): source_volid=None, consistencygroup_id=None, scheduler_hints=None, + backup_id=None, + ) + + self.assertEqual(self.columns, columns) + self.assertCountEqual(self.datalist, data) + + def test_volume_create_with_backup(self): + backup = volume_fakes.create_one_backup() + self.new_volume.backup_id = backup.id + arglist = [ + '--backup', self.new_volume.backup_id, + self.new_volume.name, + ] + verifylist = [ + ('backup', self.new_volume.backup_id), + ('name', self.new_volume.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.backups_mock.get.return_value = backup + + self.app.client_manager.volume.api_version = \ + api_versions.APIVersion('3.47') + + # In base command class ShowOne in cliff, abstract method take_action() + # returns a two-part tuple with a tuple of column names and a tuple of + # data to be shown. + columns, data = self.cmd.take_action(parsed_args) + + self.volumes_mock.create.assert_called_once_with( + size=backup.size, + snapshot_id=None, + name=self.new_volume.name, + description=None, + volume_type=None, + availability_zone=None, + metadata=None, + imageRef=None, + source_volid=None, + consistencygroup_id=None, + scheduler_hints=None, + backup_id=backup.id, ) self.assertEqual(self.columns, columns) self.assertCountEqual(self.datalist, data) + def test_volume_create_with_backup_pre_347(self): + backup = volume_fakes.create_one_backup() + self.new_volume.backup_id = backup.id + arglist = [ + '--backup', self.new_volume.backup_id, + self.new_volume.name, + ] + verifylist = [ + ('backup', self.new_volume.backup_id), + ('name', self.new_volume.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.backups_mock.get.return_value = backup + + exc = self.assertRaises(exceptions.CommandError, self.cmd.take_action, + parsed_args) + self.assertIn("--os-volume-api-version 3.47 or greater", str(exc)) + def test_volume_create_with_bootable_and_readonly(self): arglist = [ '--bootable', @@ -361,6 +431,7 @@ def test_volume_create_with_bootable_and_readonly(self): source_volid=None, consistencygroup_id=None, scheduler_hints=None, + backup_id=None, ) self.assertEqual(self.columns, columns) @@ -403,6 +474,7 @@ def test_volume_create_with_nonbootable_and_readwrite(self): source_volid=None, consistencygroup_id=None, scheduler_hints=None, + backup_id=None, ) self.assertEqual(self.columns, columns) @@ -454,6 +526,7 @@ def test_volume_create_with_bootable_and_readonly_fail( source_volid=None, consistencygroup_id=None, scheduler_hints=None, + backup_id=None, ) self.assertEqual(2, mock_error.call_count) diff --git a/openstackclient/volume/v2/volume.py b/openstackclient/volume/v2/volume.py index 1e1fde922..53f6e643d 100644 --- a/openstackclient/volume/v2/volume.py +++ b/openstackclient/volume/v2/volume.py @@ -71,10 +71,10 @@ def _check_size_arg(args): volume is not specified. """ - if ((args.snapshot or args.source) + if ((args.snapshot or args.source or args.backup) is None and args.size is None): - msg = _("--size is a required option if snapshot " - "or source volume is not specified.") + msg = _("--size is a required option if snapshot, backup " + "or source volume are not specified.") raise exceptions.CommandError(msg) @@ -117,6 +117,12 @@ def get_parser(self, prog_name): metavar="", help=_("Volume to clone (name or ID)"), ) + source_group.add_argument( + "--backup", + metavar="", + help=_("Restore backup to a volume (name or ID) " + "(supported by --os-volume-api-version 3.47 or later)"), + ) source_group.add_argument( "--source-replicated", metavar="", @@ -177,9 +183,16 @@ def get_parser(self, prog_name): def take_action(self, parsed_args): _check_size_arg(parsed_args) + volume_client = self.app.client_manager.volume image_client = self.app.client_manager.image + if parsed_args.backup and not ( + volume_client.api_version.matches('3.47')): + msg = _("--os-volume-api-version 3.47 or greater is required " + "to create a volume from backup.") + raise exceptions.CommandError(msg) + source_volume = None if parsed_args.source: source_volume = utils.find_resource( @@ -213,6 +226,15 @@ def take_action(self, parsed_args): # snapshot size. size = max(size or 0, snapshot_obj.size) + backup = None + if parsed_args.backup: + backup_obj = utils.find_resource( + volume_client.backups, + parsed_args.backup) + backup = backup_obj.id + # As above + size = max(size or 0, backup_obj.size) + volume = volume_client.volumes.create( size=size, snapshot_id=snapshot, @@ -225,6 +247,7 @@ def take_action(self, parsed_args): source_volid=source_volume, consistencygroup_id=consistency_group, scheduler_hints=parsed_args.hint, + backup_id=backup, ) if parsed_args.bootable or parsed_args.non_bootable: diff --git a/releasenotes/notes/add-backup-option-to-create-vol-fc36c2c745ebcff5.yaml b/releasenotes/notes/add-backup-option-to-create-vol-fc36c2c745ebcff5.yaml new file mode 100644 index 000000000..081ebcea7 --- /dev/null +++ b/releasenotes/notes/add-backup-option-to-create-vol-fc36c2c745ebcff5.yaml @@ -0,0 +1,4 @@ +--- +features: + - | + Added ``--backup`` option to the ``volume create`` command. From 96162c24eaba5ba70a6d8f61da815d2d6ffea7ed Mon Sep 17 00:00:00 2001 From: whoami-rajat Date: Tue, 22 Nov 2022 20:59:13 +0530 Subject: [PATCH 020/586] Change --size helptext to include backup Followup from [1]. Modifying help text of --size argument to include --backup option. [1] https://review.opendev.org/c/openstack/python-openstackclient/+/864893 Change-Id: I12cf60079ebcfe1cd059602fbfc1a13c8fe86803 --- openstackclient/volume/v2/volume.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openstackclient/volume/v2/volume.py b/openstackclient/volume/v2/volume.py index 53f6e643d..ffcbd573f 100644 --- a/openstackclient/volume/v2/volume.py +++ b/openstackclient/volume/v2/volume.py @@ -93,8 +93,8 @@ def get_parser(self, prog_name): "--size", metavar="", type=int, - help=_("Volume size in GB (Required unless --snapshot or " - "--source is specified)"), + help=_("Volume size in GB (required unless --snapshot, " + "--source or --backup is specified)"), ) parser.add_argument( "--type", From 4710cbeca6638cc78880f2c4fc22d74aa7bb7c41 Mon Sep 17 00:00:00 2001 From: whoami-rajat Date: Tue, 22 Nov 2022 19:25:39 +0530 Subject: [PATCH 021/586] Add test for creating volume from source This patch adds a test to create a new volume from source. We also include code changes to pass the right size i.e. either size passed by the user via --size argument or the source volume size. This case is already handled at the API layer[1] but it helps being consistent with passing the right size value as in case of creating a volume from snapshot or backup. [1] https://github.com/openstack/cinder/blob/7c1a5ce7b11964da4537fd6a7d157ede646b9e94/cinder/api/v3/volumes.py#L381-L382 Change-Id: Idc71636dad6bb678fe24f19b0836d2e9bd92d7d2 --- .../tests/unit/volume/v2/test_volume.py | 37 +++++++++++++++++++ openstackclient/volume/v2/volume.py | 13 +++++-- 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/openstackclient/tests/unit/volume/v2/test_volume.py b/openstackclient/tests/unit/volume/v2/test_volume.py index ef9c2fab5..c930002f3 100644 --- a/openstackclient/tests/unit/volume/v2/test_volume.py +++ b/openstackclient/tests/unit/volume/v2/test_volume.py @@ -398,6 +398,43 @@ def test_volume_create_with_backup_pre_347(self): parsed_args) self.assertIn("--os-volume-api-version 3.47 or greater", str(exc)) + def test_volume_create_with_source_volume(self): + source_vol = "source_vol" + arglist = [ + '--source', self.new_volume.id, + source_vol, + ] + verifylist = [ + ('source', self.new_volume.id), + ('name', source_vol), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.volumes_mock.get.return_value = self.new_volume + + # In base command class ShowOne in cliff, abstract method take_action() + # returns a two-part tuple with a tuple of column names and a tuple of + # data to be shown. + columns, data = self.cmd.take_action(parsed_args) + + self.volumes_mock.create.assert_called_once_with( + size=self.new_volume.size, + snapshot_id=None, + name=source_vol, + description=None, + volume_type=None, + availability_zone=None, + metadata=None, + imageRef=None, + source_volid=self.new_volume.id, + consistencygroup_id=None, + scheduler_hints=None, + backup_id=None, + ) + + self.assertEqual(self.columns, columns) + self.assertCountEqual(self.datalist, data) + def test_volume_create_with_bootable_and_readonly(self): arglist = [ '--bootable', diff --git a/openstackclient/volume/v2/volume.py b/openstackclient/volume/v2/volume.py index ffcbd573f..7905e0971 100644 --- a/openstackclient/volume/v2/volume.py +++ b/openstackclient/volume/v2/volume.py @@ -183,6 +183,11 @@ def get_parser(self, prog_name): def take_action(self, parsed_args): _check_size_arg(parsed_args) + # size is validated in the above call to + # _check_size_arg where we check that size + # should be passed if we are not creating a + # volume from snapshot, backup or source volume + size = parsed_args.size volume_client = self.app.client_manager.volume image_client = self.app.client_manager.image @@ -195,9 +200,11 @@ def take_action(self, parsed_args): source_volume = None if parsed_args.source: - source_volume = utils.find_resource( + source_volume_obj = utils.find_resource( volume_client.volumes, - parsed_args.source).id + parsed_args.source) + source_volume = source_volume_obj.id + size = max(size or 0, source_volume_obj.size) consistency_group = None if parsed_args.consistency_group: @@ -210,8 +217,6 @@ def take_action(self, parsed_args): image = image_client.find_image(parsed_args.image, ignore_missing=False).id - size = parsed_args.size - snapshot = None if parsed_args.snapshot: snapshot_obj = utils.find_resource( From abf1a7cc4b1caada799aa4549f9753b76146e243 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 24 Nov 2022 12:59:33 +0000 Subject: [PATCH 022/586] docs: Small cleanup of human interface guide Before we add more content. Change-Id: I6cf28bdd217326db991466a21221b685124d4b99 Signed-off-by: Stephen Finucane --- .../contributor/humaninterfaceguide.rst | 80 ++++++++++--------- 1 file changed, 43 insertions(+), 37 deletions(-) diff --git a/doc/source/contributor/humaninterfaceguide.rst b/doc/source/contributor/humaninterfaceguide.rst index a7db38005..29fba40e0 100644 --- a/doc/source/contributor/humaninterfaceguide.rst +++ b/doc/source/contributor/humaninterfaceguide.rst @@ -117,14 +117,14 @@ interface to the user, not the user to the interface. Commands should be discoverable via the interface itself. -To determine a list of available commands, use the :code:`-h` or -:code:`--help` options: +To determine a list of available commands, use the ``-h`` or +``--help`` options: .. code-block:: bash $ openstack --help -For help with an individual command, use the :code:`help` command: +For help with an individual command, use the ``help`` command: .. code-block:: bash @@ -167,7 +167,7 @@ Command Structure OpenStackClient has a consistent and predictable format for all of its commands. -* The top level command name is :code:`openstack` +* The top level command name is ``openstack`` * Sub-commands take the form: .. code-block:: bash @@ -193,50 +193,56 @@ invocation regardless of action to be performed. They include authentication credentials and API version selection. Most global options have a corresponding environment variable that may also be used to set the value. If both are present, the command-line option takes priority. The environment variable names are derived -from the option name by dropping the leading dashes ('--'), converting each embedded -dash ('-') to an underscore ('_'), and converting to upper case. +from the option name by dropping the leading dashes (``--``), converting each embedded +dash (``-``) to an underscore (``_``), and converting to upper case. * Global options shall always have a long option name, certain common options may also have short names. Short names should be reserved for global options to limit the potential for duplication and multiple meanings between commands given the limited set of available short names. -* All long options names shall begin with two dashes ('--') and use a single dash - ('-') internally between words (:code:`--like-this`). Underscores ('_') shall not + +* All long options names shall begin with two dashes (``--``) and use a single dash + (``-`` internally between words (``--like-this``). Underscores (``_``) shall not be used in option names. + * Authentication options conform to the common CLI authentication guidelines in :ref:`authentication`. -For example, :code:`--os-username` can be set from the environment via -:code:`OS_USERNAME`. +For example, ``--os-username`` can be set from the environment via +``OS_USERNAME``. ---help -++++++ +``--help`` +++++++++++ -The standard :code:`--help` global option displays the documentation for invoking +The standard ``--help`` global option displays the documentation for invoking the program and a list of the available commands on standard output. All other options and commands are ignored when this is present. The traditional short -form help option (:code:`-h`) is also available. +form help option (``-h``) is also available. ---version -+++++++++ +``--version`` ++++++++++++++ -The standard :code:`--version` option displays the name and version on standard +The standard ``--version`` option displays the name and version on standard output. All other options and commands are ignored when this is present. Command Object(s) and Action ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Commands consist of an object described by one or more words followed by an action. Commands that require two objects have the primary object ahead of the action and the secondary object after the action. Any positional arguments identifying the objects shall appear in the same order as the objects. In badly formed English it is expressed as "(Take) object1 (and perform) action (using) object2 (to it)." +Commands consist of an object described by one or more words followed by an +action. Commands that require two objects have the primary object ahead of the +action and the secondary object after the action. Any positional arguments +identifying the objects shall appear in the same order as the objects. In +badly formed English it is expressed as "(Take) object-1 (and perform) action +(using) object-2 (to it).":: [] Examples: -* :code:`group add user ` -* :code:`volume type list` # Note that :code:`volume type` is a two-word - single object +* ``group add user `` +* ``volume type list`` (note that ``volume type`` is a two-word single object) -The :code:`help` command is unique as it appears in front of a normal command +The ``help`` command is unique as it appears in front of a normal command and displays the help text for that command rather than execute it. Object names are always specified in command in their singular form. This is @@ -256,21 +262,21 @@ meaning across multiple commands. Option Forms ++++++++++++ -* **boolean**: boolean options shall use a form of :code:`--|--` - (preferred) or :code:`--