diff --git a/doc/source/command-objects/server.rst b/doc/source/command-objects/server.rst index dc78080b6..26bb77ca0 100644 --- a/doc/source/command-objects/server.rst +++ b/doc/source/command-objects/server.rst @@ -193,7 +193,23 @@ Create a new server .. option:: --block-device-mapping - Map block devices; map is ::: (optional extension) + Create a block device on the server. + + Block device mapping in the format + + =::: + + : block device name, like: vdb, xvdc (required) + + : UUID of the volume or snapshot (required) + + : volume or snapshot; default: volume (optional) + + : volume size if create from snapshot (optional) + + : true or false; default: false (optional) + + (optional extension) .. option:: --nic diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 60dc605ca..66bdeae85 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -446,10 +446,22 @@ class CreateServer(command.ShowOne): parser.add_argument( '--block-device-mapping', metavar='', - action='append', - default=[], - help=_('Map block devices; map is ' - '::: ' + action=parseractions.KeyValueAction, + default={}, + # NOTE(RuiChen): Add '\n' at the end of line to put each item in + # the separated line, avoid the help message looks + # messy, see _SmartHelpFormatter in cliff. + help=_('Create a block device on the server.\n' + 'Block device mapping in the format\n' + '=:::\n' + ': block device name, like: vdb, xvdc ' + '(required)\n' + ': UUID of the volume or snapshot (required)\n' + ': volume or snapshot; default: volume (optional)\n' + ': volume size if create from snapshot ' + '(optional)\n' + ': true or false; default: false ' + '(optional)\n' '(optional extension)'), ) parser.add_argument( @@ -593,33 +605,39 @@ class CreateServer(command.ShowOne): 'source_type': 'volume', 'destination_type': 'volume' }] - for dev_map in parsed_args.block_device_mapping: - dev_name, dev_map = dev_map.split('=', 1) - if dev_map: - dev_map = dev_map.split(':') - if len(dev_map) > 0: - mapping = { - 'device_name': dev_name, - 'uuid': utils.find_resource( - volume_client.volumes, - dev_map[0], - ).id} - # Block device mapping v1 compatibility - if len(dev_map) > 1 and \ - dev_map[1] in ('volume', 'snapshot'): - mapping['source_type'] = dev_map[1] - else: - mapping['source_type'] = 'volume' - mapping['destination_type'] = 'volume' - if len(dev_map) > 2 and dev_map[2]: - mapping['volume_size'] = dev_map[2] - if len(dev_map) > 3: - mapping['delete_on_termination'] = dev_map[3] + # Handle block device by device name order, like: vdb -> vdc -> vdd + for dev_name in sorted(six.iterkeys(parsed_args.block_device_mapping)): + dev_map = parsed_args.block_device_mapping[dev_name] + dev_map = dev_map.split(':') + if dev_map[0]: + mapping = {'device_name': dev_name} + # 1. decide source and destination type + if (len(dev_map) > 1 and + dev_map[1] in ('volume', 'snapshot')): + mapping['source_type'] = dev_map[1] else: - msg = _("Volume name or ID must be specified if " - "--block-device-mapping is specified") - raise exceptions.CommandError(msg) - block_device_mapping_v2.append(mapping) + mapping['source_type'] = 'volume' + mapping['destination_type'] = 'volume' + # 2. check target exist, update target uuid according by + # source type + if mapping['source_type'] == 'volume': + volume_id = utils.find_resource( + volume_client.volumes, dev_map[0]).id + mapping['uuid'] = volume_id + elif mapping['source_type'] == 'snapshot': + snapshot_id = utils.find_resource( + volume_client.volume_snapshots, dev_map[0]).id + mapping['uuid'] = snapshot_id + # 3. append size and delete_on_termination if exist + if len(dev_map) > 2 and dev_map[2]: + mapping['volume_size'] = dev_map[2] + if len(dev_map) > 3 and dev_map[3]: + mapping['delete_on_termination'] = dev_map[3] + else: + msg = _("Volume or snapshot (name or ID) must be specified if " + "--block-device-mapping is specified") + raise exceptions.CommandError(msg) + block_device_mapping_v2.append(mapping) nics = [] auto_or_none = False diff --git a/openstackclient/tests/functional/compute/v2/test_server.py b/openstackclient/tests/functional/compute/v2/test_server.py index 7d54b6a6a..9f542e291 100644 --- a/openstackclient/tests/functional/compute/v2/test_server.py +++ b/openstackclient/tests/functional/compute/v2/test_server.py @@ -388,6 +388,113 @@ class ServerTests(common.ComputeTestCase): cmd_output['status'], ) + def test_server_boot_with_bdm_snapshot(self): + """Test server create from image with bdm snapshot, server delete""" + # get volume status wait function + volume_wait_for = test_volume.VolumeTests( + methodName='wait_for', + ).wait_for + + # create source empty volume + empty_volume_name = uuid.uuid4().hex + cmd_output = json.loads(self.openstack( + 'volume create -f json ' + + '--size 1 ' + + empty_volume_name + )) + self.assertIsNotNone(cmd_output["id"]) + self.addCleanup(self.openstack, + 'volume delete ' + empty_volume_name) + self.assertEqual( + empty_volume_name, + cmd_output['name'], + ) + volume_wait_for("volume", empty_volume_name, "available") + + # create snapshot of source empty volume + empty_snapshot_name = uuid.uuid4().hex + cmd_output = json.loads(self.openstack( + 'volume snapshot create -f json ' + + '--volume ' + empty_volume_name + ' ' + + empty_snapshot_name + )) + self.assertIsNotNone(cmd_output["id"]) + self.assertEqual( + empty_snapshot_name, + cmd_output['name'], + ) + volume_wait_for("volume snapshot", empty_snapshot_name, "available") + + # create server with bdm snapshot + server_name = uuid.uuid4().hex + server = json.loads(self.openstack( + 'server create -f json ' + + '--flavor ' + self.flavor_name + ' ' + + '--image ' + self.image_name + ' ' + + '--block-device-mapping ' + 'vdb=' + empty_snapshot_name + ':snapshot:1:true ' + + self.network_arg + ' ' + + '--wait ' + + server_name + )) + self.assertIsNotNone(server["id"]) + self.assertEqual( + server_name, + server['name'], + ) + self.wait_for_status(server_name, 'ACTIVE') + + # check server volumes_attached, format is + # {"volumes_attached": "id='2518bc76-bf0b-476e-ad6b-571973745bb5'",} + cmd_output = json.loads(self.openstack( + 'server show -f json ' + + server_name + )) + volumes_attached = cmd_output['volumes_attached'] + self.assertTrue(volumes_attached.startswith('id=')) + attached_volume_id = volumes_attached.replace('id=', '') + + # check the volume that attached on server + cmd_output = json.loads(self.openstack( + 'volume show -f json ' + + attached_volume_id + )) + attachments = cmd_output['attachments'] + self.assertEqual( + 1, + len(attachments), + ) + self.assertEqual( + server['id'], + attachments[0]['server_id'], + ) + self.assertEqual( + "in-use", + cmd_output['status'], + ) + + # delete server, then check the attached volume had been deleted, + # =true + self.openstack('server delete --wait ' + server_name) + cmd_output = json.loads(self.openstack( + 'volume list -f json' + )) + target_volume = [each_volume + for each_volume in cmd_output + if each_volume['ID'] == attached_volume_id] + if target_volume: + # check the attached volume is 'deleting' status + self.assertEqual('deleting', target_volume[0]['Status']) + else: + # the attached volume had been deleted + pass + + # clean up volume snapshot manually, make sure the snapshot and volume + # can be deleted sequentially, self.addCleanup so fast, that cause + # volume service API 400 error and the volume is left over at the end. + self.openstack('volume snapshot delete ' + empty_snapshot_name) + volume_wait_for('volume snapshot', empty_snapshot_name, 'disappear') + def test_server_create_with_none_network(self): """Test server create with none network option.""" server_name = uuid.uuid4().hex diff --git a/openstackclient/tests/functional/volume/v2/test_volume.py b/openstackclient/tests/functional/volume/v2/test_volume.py index ce98236f8..94ac792f9 100644 --- a/openstackclient/tests/functional/volume/v2/test_volume.py +++ b/openstackclient/tests/functional/volume/v2/test_volume.py @@ -14,6 +14,8 @@ import json import time import uuid +from tempest.lib import exceptions + from openstackclient.tests.functional.volume.v2 import common @@ -253,7 +255,13 @@ class VolumeTests(common.BaseVolumeTests): total_sleep = 0 opts = self.get_opts(['status']) while total_sleep < wait: - status = self.openstack(check_type + ' show ' + check_name + opts) + try: + status = self.openstack( + check_type + ' show ' + check_name + opts + ) + except exceptions.CommandFailed: + # Show command raise Exception when object had been deleted + status = 'disappear' status = status.rstrip() print('Checking {} {} Waiting for {} current status: {}' .format(check_type, check_name, desired_status, status)) diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 9370bf6bd..d25cabbb4 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. # +import argparse import collections import getpass import mock @@ -49,6 +50,10 @@ class TestServer(compute_fakes.TestComputev2): self.volumes_mock = self.app.client_manager.volume.volumes self.volumes_mock.reset_mock() + # Get a shortcut to the volume client VolumeManager Mock + self.snapshots_mock = self.app.client_manager.volume.volume_snapshots + self.snapshots_mock.reset_mock() + # Set object attributes to be tested. Could be overwritten in subclass. self.attrs = {} @@ -326,7 +331,9 @@ class TestServerCreate(TestServer): self.volume = volume_fakes.FakeVolume.create_one_volume() self.volumes_mock.get.return_value = self.volume - self.block_device_mapping = 'vda=' + self.volume.name + ':::0' + + self.snapshot = volume_fakes.FakeSnapshot.create_one_snapshot() + self.snapshots_mock.get.return_value = self.snapshot # Get the command object to test self.cmd = server.CreateServer(self.app, None) @@ -852,13 +859,13 @@ class TestServerCreate(TestServer): arglist = [ '--image', 'image1', '--flavor', self.flavor.id, - '--block-device-mapping', self.block_device_mapping, + '--block-device-mapping', 'vda=' + self.volume.name + ':::false', self.new_server.name, ] verifylist = [ ('image', 'image1'), ('flavor', self.flavor.id), - ('block_device_mapping', [self.block_device_mapping]), + ('block_device_mapping', {'vda': self.volume.name + ':::false'}), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -867,11 +874,6 @@ class TestServerCreate(TestServer): # CreateServer.take_action() returns two tuples columns, data = self.cmd.take_action(parsed_args) - real_volume_mapping = ( - (self.block_device_mapping.split('=', 1)[1]).replace( - self.volume.name, - self.volume.id)) - # Set expected values kwargs = dict( meta=None, @@ -885,10 +887,10 @@ class TestServerCreate(TestServer): availability_zone=None, block_device_mapping_v2=[{ 'device_name': 'vda', - 'uuid': real_volume_mapping.split(':', 1)[0], + 'uuid': self.volume.id, 'destination_type': 'volume', 'source_type': 'volume', - 'delete_on_termination': '0' + 'delete_on_termination': 'false', }], nics=[], scheduler_hints={}, @@ -905,6 +907,323 @@ class TestServerCreate(TestServer): self.assertEqual(self.columns, columns) self.assertEqual(self.datalist(), data) + def test_server_create_with_block_device_mapping_min_input(self): + arglist = [ + '--image', 'image1', + '--flavor', self.flavor.id, + '--block-device-mapping', 'vdf=' + self.volume.name, + self.new_server.name, + ] + verifylist = [ + ('image', 'image1'), + ('flavor', self.flavor.id), + ('block_device_mapping', {'vdf': self.volume.name}), + ('config_drive', False), + ('server_name', self.new_server.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # CreateServer.take_action() returns two tuples + columns, data = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = dict( + meta=None, + files={}, + reservation_id=None, + min_count=1, + max_count=1, + security_groups=[], + userdata=None, + key_name=None, + availability_zone=None, + block_device_mapping_v2=[{ + 'device_name': 'vdf', + 'uuid': self.volume.id, + 'destination_type': 'volume', + 'source_type': 'volume', + }], + nics=[], + scheduler_hints={}, + config_drive=None, + ) + # ServerManager.create(name, image, flavor, **kwargs) + self.servers_mock.create.assert_called_with( + self.new_server.name, + self.image, + self.flavor, + **kwargs + ) + + self.assertEqual(self.columns, columns) + self.assertEqual(self.datalist(), data) + + def test_server_create_with_block_device_mapping_default_input(self): + arglist = [ + '--image', 'image1', + '--flavor', self.flavor.id, + '--block-device-mapping', 'vdf=' + self.volume.name + ':::', + self.new_server.name, + ] + verifylist = [ + ('image', 'image1'), + ('flavor', self.flavor.id), + ('block_device_mapping', {'vdf': self.volume.name + ':::'}), + ('config_drive', False), + ('server_name', self.new_server.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # CreateServer.take_action() returns two tuples + columns, data = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = dict( + meta=None, + files={}, + reservation_id=None, + min_count=1, + max_count=1, + security_groups=[], + userdata=None, + key_name=None, + availability_zone=None, + block_device_mapping_v2=[{ + 'device_name': 'vdf', + 'uuid': self.volume.id, + 'destination_type': 'volume', + 'source_type': 'volume', + }], + nics=[], + scheduler_hints={}, + config_drive=None, + ) + # ServerManager.create(name, image, flavor, **kwargs) + self.servers_mock.create.assert_called_with( + self.new_server.name, + self.image, + self.flavor, + **kwargs + ) + + self.assertEqual(self.columns, columns) + self.assertEqual(self.datalist(), data) + + def test_server_create_with_block_device_mapping_full_input(self): + arglist = [ + '--image', 'image1', + '--flavor', self.flavor.id, + '--block-device-mapping', + 'vde=' + self.volume.name + ':volume:3:true', + self.new_server.name, + ] + verifylist = [ + ('image', 'image1'), + ('flavor', self.flavor.id), + ('block_device_mapping', + {'vde': self.volume.name + ':volume:3:true'}), + ('config_drive', False), + ('server_name', self.new_server.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # CreateServer.take_action() returns two tuples + columns, data = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = dict( + meta=None, + files={}, + reservation_id=None, + min_count=1, + max_count=1, + security_groups=[], + userdata=None, + key_name=None, + availability_zone=None, + block_device_mapping_v2=[{ + 'device_name': 'vde', + 'uuid': self.volume.id, + 'destination_type': 'volume', + 'source_type': 'volume', + 'delete_on_termination': 'true', + 'volume_size': '3' + }], + nics=[], + scheduler_hints={}, + config_drive=None, + ) + # ServerManager.create(name, image, flavor, **kwargs) + self.servers_mock.create.assert_called_with( + self.new_server.name, + self.image, + self.flavor, + **kwargs + ) + + self.assertEqual(self.columns, columns) + self.assertEqual(self.datalist(), data) + + def test_server_create_with_block_device_mapping_snapshot(self): + arglist = [ + '--image', 'image1', + '--flavor', self.flavor.id, + '--block-device-mapping', + 'vds=' + self.volume.name + ':snapshot:5:true', + self.new_server.name, + ] + verifylist = [ + ('image', 'image1'), + ('flavor', self.flavor.id), + ('block_device_mapping', + {'vds': self.volume.name + ':snapshot:5:true'}), + ('config_drive', False), + ('server_name', self.new_server.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # CreateServer.take_action() returns two tuples + columns, data = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = dict( + meta=None, + files={}, + reservation_id=None, + min_count=1, + max_count=1, + security_groups=[], + userdata=None, + key_name=None, + availability_zone=None, + block_device_mapping_v2=[{ + 'device_name': 'vds', + 'uuid': self.snapshot.id, + 'destination_type': 'volume', + 'source_type': 'snapshot', + 'delete_on_termination': 'true', + 'volume_size': '5' + }], + nics=[], + scheduler_hints={}, + config_drive=None, + ) + # ServerManager.create(name, image, flavor, **kwargs) + self.servers_mock.create.assert_called_with( + self.new_server.name, + self.image, + self.flavor, + **kwargs + ) + + self.assertEqual(self.columns, columns) + self.assertEqual(self.datalist(), data) + + def test_server_create_with_block_device_mapping_multiple(self): + arglist = [ + '--image', 'image1', + '--flavor', self.flavor.id, + '--block-device-mapping', 'vdb=' + self.volume.name + ':::false', + '--block-device-mapping', 'vdc=' + self.volume.name + ':::true', + self.new_server.name, + ] + verifylist = [ + ('image', 'image1'), + ('flavor', self.flavor.id), + ('block_device_mapping', {'vdb': self.volume.name + ':::false', + 'vdc': self.volume.name + ':::true'}), + ('config_drive', False), + ('server_name', self.new_server.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # CreateServer.take_action() returns two tuples + columns, data = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = dict( + meta=None, + files={}, + reservation_id=None, + min_count=1, + max_count=1, + security_groups=[], + userdata=None, + key_name=None, + availability_zone=None, + block_device_mapping_v2=[ + { + 'device_name': 'vdb', + 'uuid': self.volume.id, + 'destination_type': 'volume', + 'source_type': 'volume', + 'delete_on_termination': 'false', + }, + { + 'device_name': 'vdc', + 'uuid': self.volume.id, + 'destination_type': 'volume', + 'source_type': 'volume', + 'delete_on_termination': 'true', + } + ], + nics=[], + scheduler_hints={}, + config_drive=None, + ) + # ServerManager.create(name, image, flavor, **kwargs) + self.servers_mock.create.assert_called_with( + self.new_server.name, + self.image, + self.flavor, + **kwargs + ) + + self.assertEqual(self.columns, columns) + self.assertEqual(self.datalist(), data) + + def test_server_create_with_block_device_mapping_invalid_format(self): + # 1. block device mapping don't contain equal sign "=" + arglist = [ + '--image', 'image1', + '--flavor', self.flavor.id, + '--block-device-mapping', 'not_contain_equal_sign', + self.new_server.name, + ] + self.assertRaises(argparse.ArgumentTypeError, + self.check_parser, + self.cmd, arglist, []) + # 2. block device mapping don't contain device name "=uuid:::true" + arglist = [ + '--image', 'image1', + '--flavor', self.flavor.id, + '--block-device-mapping', '=uuid:::true', + self.new_server.name, + ] + self.assertRaises(argparse.ArgumentTypeError, + self.check_parser, + self.cmd, arglist, []) + + def test_server_create_with_block_device_mapping_no_uuid(self): + arglist = [ + '--image', 'image1', + '--flavor', self.flavor.id, + '--block-device-mapping', 'vdb=', + self.new_server.name, + ] + verifylist = [ + ('image', 'image1'), + ('flavor', self.flavor.id), + ('block_device_mapping', {'vdb': ''}), + ('config_drive', False), + ('server_name', self.new_server.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.assertRaises(exceptions.CommandError, + self.cmd.take_action, + parsed_args) + class TestServerDelete(TestServer): diff --git a/releasenotes/notes/bug-1667266-6497727abc2af9a5.yaml b/releasenotes/notes/bug-1667266-6497727abc2af9a5.yaml new file mode 100644 index 000000000..bedfa67f9 --- /dev/null +++ b/releasenotes/notes/bug-1667266-6497727abc2af9a5.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + Make ``block-device-mapping`` option of ``server create`` command more + stable and clear. Fix ValueError when input block device mapping option in + wrong format. Support to create block device from snapshot. Add details in + help message about block-device-mapping option format and regular value of + each item. + [Bug `1667266 `_]