From 70480fa86236f7de583c7b098cc53f0acedfd91d Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 21 Jan 2021 12:26:13 +0000 Subject: [PATCH] compute: Remove deprecated 'server migrate --live' option It's been long enough. Time to remove this. Change-Id: I37ef09eca0db9286544a4b0bb33f845311baa9b2 Signed-off-by: Stephen Finucane --- openstackclient/compute/v2/server.py | 75 ++----- .../tests/unit/compute/v2/test_server.py | 203 ++++-------------- ...-migrate-live-option-28ec43ee210124dc.yaml | 8 + 3 files changed, 67 insertions(+), 219 deletions(-) create mode 100644 releasenotes/notes/remove-deprecated-server-migrate-live-option-28ec43ee210124dc.yaml diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 48f5b7cf1..9838ed548 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -2473,27 +2473,7 @@ revert to release the new server and restart the old one.""") 'validated by the scheduler' ), ) - # The --live and --host options are mutually exclusive ways of asking - # for a target host during a live migration. - host_group = parser.add_mutually_exclusive_group() - # TODO(mriedem): Remove --live in the next major version bump after - # the Train release. - host_group.add_argument( - '--live', - metavar='', - help=_( - '**Deprecated** This option is problematic in that it ' - 'requires a host and prior to compute API version 2.30, ' - 'specifying a host during live migration will bypass ' - 'validation by the scheduler which could result in ' - 'failures to actually migrate the server to the specified ' - 'host or over-subscribe the host. Use the ' - '``--live-migration`` option instead. If both this option ' - 'and ``--live-migration`` are used, ``--live-migration`` ' - 'takes priority.' - ), - ) - host_group.add_argument( + parser.add_argument( '--host', metavar='', help=_( @@ -2551,15 +2531,6 @@ revert to release the new server and restart the old one.""") ) return parser - def _log_warning_for_live(self, parsed_args): - if parsed_args.live: - # NOTE(mriedem): The --live option requires a host and if - # --os-compute-api-version is less than 2.30 it will forcefully - # bypass the scheduler which is dangerous. - self.log.warning(_( - 'The --live option has been deprecated. Please use the ' - '--live-migration option instead.')) - def take_action(self, parsed_args): def _show_progress(progress): @@ -2573,11 +2544,8 @@ revert to release the new server and restart the old one.""") compute_client.servers, parsed_args.server, ) - # Check for live migration. - if parsed_args.live or parsed_args.live_migration: - # Always log a warning if --live is used. - self._log_warning_for_live(parsed_args) + if parsed_args.live_migration: kwargs = {} block_migration = parsed_args.block_migration @@ -2592,28 +2560,23 @@ revert to release the new server and restart the old one.""") kwargs['block_migration'] = block_migration - # Prefer --live-migration over --live if both are specified. - if parsed_args.live_migration: - # Technically we could pass a non-None host with - # --os-compute-api-version < 2.30 but that is the same thing - # as the --live option bypassing the scheduler which we don't - # want to support, so if the user is using --live-migration - # and --host, we want to enforce that they are using version - # 2.30 or greater. - if ( - parsed_args.host and - compute_client.api_version < - api_versions.APIVersion('2.30') - ): - raise exceptions.CommandError( - '--os-compute-api-version 2.30 or greater is required ' - 'when using --host' - ) + # Technically we could pass a non-None host with + # --os-compute-api-version < 2.30 but that is the same thing + # as the --live option bypassing the scheduler which we don't + # want to support, so if the user is using --live-migration + # and --host, we want to enforce that they are using version + # 2.30 or greater. + if ( + parsed_args.host and + compute_client.api_version < api_versions.APIVersion('2.30') + ): + raise exceptions.CommandError( + '--os-compute-api-version 2.30 or greater is required ' + 'when using --host' + ) - # The host parameter is required in the API even if None. - kwargs['host'] = parsed_args.host - else: - kwargs['host'] = parsed_args.live + # The host parameter is required in the API even if None. + kwargs['host'] = parsed_args.host if compute_client.api_version < api_versions.APIVersion('2.25'): kwargs['disk_over_commit'] = parsed_args.disk_overcommit @@ -2628,7 +2591,7 @@ revert to release the new server and restart the old one.""") self.log.warning(msg) server.live_migrate(**kwargs) - else: + else: # cold migration if parsed_args.block_migration or parsed_args.disk_overcommit: raise exceptions.CommandError( "--live-migration must be specified if " diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 9afc4cebe..ced5d458c 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -4523,7 +4523,7 @@ class TestServerMigrate(TestServer): self.server.id, ] verifylist = [ - ('live', None), + ('live_migration', False), ('block_migration', None), ('disk_overcommit', False), ('wait', False), @@ -4544,7 +4544,6 @@ class TestServerMigrate(TestServer): '--host', 'fakehost', self.server.id, ] verifylist = [ - ('live', None), ('live_migration', False), ('host', 'fakehost'), ('block_migration', None), @@ -4568,7 +4567,7 @@ class TestServerMigrate(TestServer): '--block-migration', self.server.id, ] verifylist = [ - ('live', None), + ('live_migration', False), ('block_migration', True), ('disk_overcommit', False), ('wait', False), @@ -4587,7 +4586,7 @@ class TestServerMigrate(TestServer): '--disk-overcommit', self.server.id, ] verifylist = [ - ('live', None), + ('live_migration', False), ('block_migration', None), ('disk_overcommit', True), ('wait', False), @@ -4608,7 +4607,6 @@ class TestServerMigrate(TestServer): '--host', 'fakehost', self.server.id, ] verifylist = [ - ('live', None), ('live_migration', False), ('host', 'fakehost'), ('block_migration', None), @@ -4630,70 +4628,11 @@ class TestServerMigrate(TestServer): self.assertNotCalled(self.servers_mock.migrate) def test_server_live_migrate(self): - arglist = [ - '--live', 'fakehost', self.server.id, - ] - verifylist = [ - ('live', 'fakehost'), - ('live_migration', False), - ('host', None), - ('block_migration', None), - ('disk_overcommit', False), - ('wait', False), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.24') - - with mock.patch.object(self.cmd.log, 'warning') as mock_warning: - result = self.cmd.take_action(parsed_args) - - self.servers_mock.get.assert_called_with(self.server.id) - self.server.live_migrate.assert_called_with(block_migration=False, - disk_over_commit=False, - host='fakehost') - self.assertNotCalled(self.servers_mock.migrate) - self.assertIsNone(result) - # A warning should have been logged for using --live. - mock_warning.assert_called_once() - self.assertIn('The --live option has been deprecated.', - str(mock_warning.call_args[0][0])) - - def test_server_live_migrate_host_pre_2_30(self): - # Tests that the --host option is not supported for --live-migration - # before microversion 2.30 (the test defaults to 2.1). - arglist = [ - '--live-migration', '--host', 'fakehost', self.server.id, - ] - verifylist = [ - ('live', None), - ('live_migration', True), - ('host', 'fakehost'), - ('block_migration', None), - ('disk_overcommit', False), - ('wait', False), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - ex = self.assertRaises(exceptions.CommandError, self.cmd.take_action, - parsed_args) - - # Make sure it's the error we expect. - self.assertIn('--os-compute-api-version 2.30 or greater is required ' - 'when using --host', str(ex)) - - self.servers_mock.get.assert_called_with(self.server.id) - self.assertNotCalled(self.servers_mock.live_migrate) - self.assertNotCalled(self.servers_mock.migrate) - - def test_server_live_migrate_no_host(self): # Tests the --live-migration option without --host or --live. arglist = [ '--live-migration', self.server.id, ] verifylist = [ - ('live', None), ('live_migration', True), ('host', None), ('block_migration', None), @@ -4702,26 +4641,22 @@ class TestServerMigrate(TestServer): ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - with mock.patch.object(self.cmd.log, 'warning') as mock_warning: - result = self.cmd.take_action(parsed_args) + result = self.cmd.take_action(parsed_args) self.servers_mock.get.assert_called_with(self.server.id) - self.server.live_migrate.assert_called_with(block_migration=False, - disk_over_commit=False, - host=None) + self.server.live_migrate.assert_called_with( + block_migration=False, + disk_over_commit=False, + host=None) self.assertNotCalled(self.servers_mock.migrate) self.assertIsNone(result) - # Since --live wasn't used a warning shouldn't have been logged. - mock_warning.assert_not_called() def test_server_live_migrate_with_host(self): - # Tests the --live-migration option with --host but no --live. # This requires --os-compute-api-version >= 2.30 so the test uses 2.30. arglist = [ '--live-migration', '--host', 'fakehost', self.server.id, ] verifylist = [ - ('live', None), ('live_migration', True), ('host', 'fakehost'), ('block_migration', None), @@ -4743,53 +4678,42 @@ class TestServerMigrate(TestServer): self.assertNotCalled(self.servers_mock.migrate) self.assertIsNone(result) - def test_server_live_migrate_without_host_override_live(self): - # Tests the --live-migration option without --host and with --live. - # The --live-migration option will take precedence and a warning is - # logged for using --live. + def test_server_live_migrate_with_host_pre_v230(self): + # Tests that the --host option is not supported for --live-migration + # before microversion 2.30 (the test defaults to 2.1). arglist = [ - '--live', 'fakehost', '--live-migration', self.server.id, + '--live-migration', '--host', 'fakehost', self.server.id, ] verifylist = [ - ('live', 'fakehost'), ('live_migration', True), - ('host', None), + ('host', 'fakehost'), ('block_migration', None), ('disk_overcommit', False), ('wait', False), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - with mock.patch.object(self.cmd.log, 'warning') as mock_warning: - result = self.cmd.take_action(parsed_args) + ex = self.assertRaises( + exceptions.CommandError, self.cmd.take_action, + parsed_args) + + # Make sure it's the error we expect. + self.assertIn( + '--os-compute-api-version 2.30 or greater is required ' + 'when using --host', str(ex)) self.servers_mock.get.assert_called_with(self.server.id) - self.server.live_migrate.assert_called_with(block_migration=False, - disk_over_commit=False, - host=None) + self.assertNotCalled(self.servers_mock.live_migrate) self.assertNotCalled(self.servers_mock.migrate) - self.assertIsNone(result) - # A warning should have been logged for using --live. - mock_warning.assert_called_once() - self.assertIn('The --live option has been deprecated.', - str(mock_warning.call_args[0][0])) - - def test_server_live_migrate_live_and_host_mutex(self): - # Tests specifying both the --live and --host options which are in a - # mutex group so argparse should fail. - arglist = [ - '--live', 'fakehost', '--host', 'fakehost', self.server.id, - ] - self.assertRaises( - utils.ParserException, - self.check_parser, self.cmd, arglist, verify_args=[]) def test_server_block_live_migrate(self): arglist = [ - '--live', 'fakehost', '--block-migration', self.server.id, + '--live-migration', + '--block-migration', + self.server.id, ] verifylist = [ - ('live', 'fakehost'), + ('live_migration', True), ('block_migration', True), ('disk_overcommit', False), ('wait', False), @@ -4802,18 +4726,21 @@ class TestServerMigrate(TestServer): result = self.cmd.take_action(parsed_args) self.servers_mock.get.assert_called_with(self.server.id) - self.server.live_migrate.assert_called_with(block_migration=True, - disk_over_commit=False, - host='fakehost') + self.server.live_migrate.assert_called_with( + block_migration=True, + disk_over_commit=False, + host=None) self.assertNotCalled(self.servers_mock.migrate) self.assertIsNone(result) def test_server_live_migrate_with_disk_overcommit(self): arglist = [ - '--live', 'fakehost', '--disk-overcommit', self.server.id, + '--live-migration', + '--disk-overcommit', + self.server.id, ] verifylist = [ - ('live', 'fakehost'), + ('live_migration', True), ('block_migration', None), ('disk_overcommit', True), ('wait', False), @@ -4826,9 +4753,10 @@ class TestServerMigrate(TestServer): result = self.cmd.take_action(parsed_args) self.servers_mock.get.assert_called_with(self.server.id) - self.server.live_migrate.assert_called_with(block_migration=False, - disk_over_commit=True, - host='fakehost') + self.server.live_migrate.assert_called_with( + block_migration=False, + disk_over_commit=True, + host=None) self.assertNotCalled(self.servers_mock.migrate) self.assertIsNone(result) @@ -4839,7 +4767,6 @@ class TestServerMigrate(TestServer): self.server.id, ] verifylist = [ - ('live', None), ('live_migration', True), ('block_migration', None), ('disk_overcommit', True), @@ -4866,63 +4793,13 @@ class TestServerMigrate(TestServer): 'The --disk-overcommit and --no-disk-overcommit options ', str(mock_warning.call_args[0][0])) - def test_server_live_migrate_with_false_value_options(self): - arglist = [ - '--live', 'fakehost', '--no-disk-overcommit', - '--shared-migration', self.server.id, - ] - verifylist = [ - ('live', 'fakehost'), - ('block_migration', False), - ('disk_overcommit', False), - ('wait', False), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.24') - - result = self.cmd.take_action(parsed_args) - - self.servers_mock.get.assert_called_with(self.server.id) - self.server.live_migrate.assert_called_with(block_migration=False, - disk_over_commit=False, - host='fakehost') - self.assertNotCalled(self.servers_mock.migrate) - self.assertIsNone(result) - - def test_server_live_migrate_225(self): - arglist = [ - '--live', 'fakehost', self.server.id, - ] - verifylist = [ - ('live', 'fakehost'), - ('block_migration', None), - ('wait', False), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - self.app.client_manager.compute.api_version = \ - api_versions.APIVersion('2.25') - - result = self.cmd.take_action(parsed_args) - - self.servers_mock.get.assert_called_with(self.server.id) - # No disk_overcommit and block_migration defaults to auto with - # microversion >= 2.25 - self.server.live_migrate.assert_called_with( - block_migration='auto', - host='fakehost') - self.assertNotCalled(self.servers_mock.migrate) - self.assertIsNone(result) - @mock.patch.object(common_utils, 'wait_for_status', return_value=True) def test_server_migrate_with_wait(self, mock_wait_for_status): arglist = [ '--wait', self.server.id, ] verifylist = [ - ('live', None), + ('live_migration', False), ('block_migration', None), ('disk_overcommit', False), ('wait', True), @@ -4942,7 +4819,7 @@ class TestServerMigrate(TestServer): '--wait', self.server.id, ] verifylist = [ - ('live', None), + ('live_migration', False), ('block_migration', None), ('disk_overcommit', False), ('wait', True), diff --git a/releasenotes/notes/remove-deprecated-server-migrate-live-option-28ec43ee210124dc.yaml b/releasenotes/notes/remove-deprecated-server-migrate-live-option-28ec43ee210124dc.yaml new file mode 100644 index 000000000..bc31aa269 --- /dev/null +++ b/releasenotes/notes/remove-deprecated-server-migrate-live-option-28ec43ee210124dc.yaml @@ -0,0 +1,8 @@ +--- +upgrade: + - | + The deprecated ``--live`` option of the ``server migrate`` command has + been removed. This was problematic as it required a host argument and + would result in a forced live migration to a host, bypassing the + scheduler. It has been replaced by a ``--live-migration`` option and + optional ``--host`` option.