From 7063ffb3cacadbaf2e47c4e6d7b2cf8d694c83a7 Mon Sep 17 00:00:00 2001 From: Rikimaru Honjo Date: Thu, 23 Feb 2017 16:59:26 +0900 Subject: [PATCH] Add a validation about options for server migrate command The behavior of server migrate command are different depending on whether user specify --live option or not. server migrate command will call live migration API if user specify --live option. Ohterwise server migrate command will call migration(cold migration) API. Now then, "--block-migraiton" option and "--disk-overcommit" option only affect live-migration. But, openstackclient doesn't warn user if user specify these options without "--live". But, user can't recognize that specifying options are ignored. This patch adds a validation that checks whether or not user specify these options without "--live". Change-Id: Ifa278abb23ecdba4b13f3742998359ac74eb7ad4 Closes-bug: #1662755 --- openstackclient/compute/v2/server.py | 4 + .../tests/unit/compute/v2/test_server.py | 203 ++++++++++++++++++ 2 files changed, 207 insertions(+) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index d33c631a17..1f91e6d940 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -1104,6 +1104,10 @@ class MigrateServer(command.Command): disk_over_commit=parsed_args.disk_overcommit, ) else: + if parsed_args.block_migration or parsed_args.disk_overcommit: + raise exceptions.CommandError("--live must be specified if " + "--block-migration or " + "--disk-overcommit is specified") server.migrate() if parsed_args.wait: diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 249902bca4..6bdb51a91e 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -1185,6 +1185,209 @@ class TestServerLock(TestServer): self.run_method_with_servers('lock', 3) +class TestServerMigrate(TestServer): + + def setUp(self): + super(TestServerMigrate, self).setUp() + + methods = { + 'migrate': None, + 'live_migrate': None, + } + self.server = compute_fakes.FakeServer.create_one_server( + methods=methods) + + # This is the return value for utils.find_resource() + self.servers_mock.get.return_value = self.server + + self.servers_mock.migrate.return_value = None + self.servers_mock.live_migrate.return_value = None + + # Get the command object to test + self.cmd = server.MigrateServer(self.app, None) + + def test_server_migrate_no_options(self): + arglist = [ + self.server.id, + ] + verifylist = [ + ('live', None), + ('block_migration', False), + ('disk_overcommit', False), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + self.servers_mock.get.assert_called_with(self.server.id) + self.server.migrate.assert_called_with() + self.assertNotCalled(self.servers_mock.live_migrate) + self.assertIsNone(result) + + def test_server_migrate_with_block_migration(self): + arglist = [ + '--block-migration', self.server.id, + ] + verifylist = [ + ('live', None), + ('block_migration', True), + ('disk_overcommit', False), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.assertRaises(exceptions.CommandError, self.cmd.take_action, + parsed_args) + + 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_migrate_with_disk_overcommit(self): + arglist = [ + '--disk-overcommit', self.server.id, + ] + verifylist = [ + ('live', None), + ('block_migration', False), + ('disk_overcommit', True), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.assertRaises(exceptions.CommandError, self.cmd.take_action, + parsed_args) + + 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(self): + arglist = [ + '--live', 'fakehost', self.server.id, + ] + verifylist = [ + ('live', 'fakehost'), + ('block_migration', False), + ('disk_overcommit', False), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + 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_block_live_migrate(self): + arglist = [ + '--live', 'fakehost', '--block-migration', self.server.id, + ] + verifylist = [ + ('live', 'fakehost'), + ('block_migration', True), + ('disk_overcommit', False), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + 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.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, + ] + verifylist = [ + ('live', 'fakehost'), + ('block_migration', False), + ('disk_overcommit', True), + ('wait', False), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + 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.assertNotCalled(self.servers_mock.migrate) + self.assertIsNone(result) + + 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) + + 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) + + @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), + ('block_migration', False), + ('disk_overcommit', False), + ('wait', True), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + result = self.cmd.take_action(parsed_args) + + self.servers_mock.get.assert_called_with(self.server.id) + self.server.migrate.assert_called_with() + self.assertNotCalled(self.servers_mock.live_migrate) + self.assertIsNone(result) + + @mock.patch.object(common_utils, 'wait_for_status', return_value=False) + def test_server_migrate_with_wait_fails(self, mock_wait_for_status): + arglist = [ + '--wait', self.server.id, + ] + verifylist = [ + ('live', None), + ('block_migration', False), + ('disk_overcommit', False), + ('wait', True), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.assertRaises(SystemExit, self.cmd.take_action, parsed_args) + + self.servers_mock.get.assert_called_with(self.server.id) + self.server.migrate.assert_called_with() + self.assertNotCalled(self.servers_mock.live_migrate) + + class TestServerPause(TestServer): def setUp(self):