From 861a6ae21bdd576e7ecbaf1ad175893e680aaf77 Mon Sep 17 00:00:00 2001 From: Jose Luis Franco Arza Date: Tue, 3 Jul 2018 11:38:37 +0200 Subject: [PATCH] Add --roles to update run CLI command. To provide a consistent upgrades/updates CLI, the same --roles and --nodes argument is added into minor updates CLI command. The use of these two arguments is mutually exclusive, either we specify a set of nodes or controlplane roles in which the minor update will be run. Change-Id: I1971b19b28049c98697631d6b0e190bdda078fcb --- .../overcloud_update/test_overcloud_update.py | 46 +++++++++++----- tripleoclient/v1/overcloud_update.py | 54 ++++++++++++------- 2 files changed, 69 insertions(+), 31 deletions(-) diff --git a/tripleoclient/tests/v1/overcloud_update/test_overcloud_update.py b/tripleoclient/tests/v1/overcloud_update/test_overcloud_update.py index 91a10a855..286c20282 100644 --- a/tripleoclient/tests/v1/overcloud_update/test_overcloud_update.py +++ b/tripleoclient/tests/v1/overcloud_update/test_overcloud_update.py @@ -131,13 +131,14 @@ class TestOvercloudUpdateRun(fakes.TestOvercloudUpdateRun): def test_update_with_playbook_and_user(self, mock_open, mock_execute, mock_expanduser, update_ansible): mock_expanduser.return_value = '/home/fake/' - argslist = ['--nodes', 'Compute', + argslist = ['--roles', 'Compute', '--playbook', 'fake-playbook.yaml', '--ssh-user', 'tripleo-admin'] verifylist = [ - ('nodes', 'Compute'), + ('roles', 'Compute'), ('static_inventory', None), - ('playbook', 'fake-playbook.yaml') + ('playbook', 'fake-playbook.yaml'), + ('ssh_user', 'tripleo-admin') ] parsed_args = self.check_parser(self.cmd, argslist, verifylist) @@ -159,12 +160,12 @@ class TestOvercloudUpdateRun(fakes.TestOvercloudUpdateRun): @mock.patch('os.path.expanduser') @mock.patch('oslo_concurrency.processutils.execute') @mock.patch('six.moves.builtins.open') - def test_update_with_all_playbooks(self, mock_open, mock_execute, - mock_expanduser, update_ansible): + def test_update_roles_with_all_playbooks(self, mock_open, mock_execute, + mock_expanduser, update_ansible): mock_expanduser.return_value = '/home/fake/' - argslist = ['--nodes', 'Compute', '--playbook', 'all'] + argslist = ['--roles', 'Compute', '--playbook', 'all'] verifylist = [ - ('nodes', 'Compute'), + ('roles', 'Compute'), ('static_inventory', None), ('playbook', 'all') ] @@ -189,14 +190,14 @@ class TestOvercloudUpdateRun(fakes.TestOvercloudUpdateRun): @mock.patch('os.path.expanduser') @mock.patch('oslo_concurrency.processutils.execute') @mock.patch('six.moves.builtins.open') - def test_update_with_all_nodes_default_all_playbooks( + def test_update_nodes_with_all_playbooks( self, mock_open, mock_execute, mock_expanduser, update_ansible): mock_expanduser.return_value = '/home/fake/' - argslist = ['--nodes', 'all'] + argslist = ['--nodes', 'compute-0, compute-1'] verifylist = [ ('static_inventory', None), ('playbook', 'all'), - ('nodes', 'all') + ('nodes', 'compute-0, compute-1') ] parsed_args = self.check_parser(self.cmd, argslist, verifylist) with mock.patch('os.path.exists') as mock_exists: @@ -205,7 +206,7 @@ class TestOvercloudUpdateRun(fakes.TestOvercloudUpdateRun): for book in constants.MINOR_UPDATE_PLAYBOOKS: update_ansible.assert_any_call( self.app.client_manager, - nodes=None, + nodes='compute-0, compute-1', inventory_file=mock_open().read(), playbook=book, ansible_queue_name=constants.UPDATE_QUEUE, @@ -218,8 +219,8 @@ class TestOvercloudUpdateRun(fakes.TestOvercloudUpdateRun): @mock.patch('os.path.expanduser') @mock.patch('oslo_concurrency.processutils.execute') @mock.patch('six.moves.builtins.open') - def test_update_with_no_nodes(self, mock_open, mock_execute, - mock_expanduser, update_ansible): + def test_update_with_no_nodes_or_roles(self, mock_open, mock_execute, + mock_expanduser, update_ansible): mock_expanduser.return_value = '/home/fake/' argslist = [] verifylist = [ @@ -229,6 +230,25 @@ class TestOvercloudUpdateRun(fakes.TestOvercloudUpdateRun): self.assertRaises(ParserException, lambda: self.check_parser( self.cmd, argslist, verifylist)) + @mock.patch('tripleoclient.workflows.package_update.update_ansible', + autospec=True) + @mock.patch('os.path.expanduser') + @mock.patch('oslo_concurrency.processutils.execute') + @mock.patch('six.moves.builtins.open') + def test_update_with_nodes_and_roles(self, mock_open, mock_execute, + mock_expanduser, update_ansible): + mock_expanduser.return_value = '/home/fake/' + argslist = ['--roles', 'Controller', + '--nodes', 'overcloud-controller-1'] + verifylist = [ + ('roles', 'Controller'), + ('nodes', 'overcloud-controller-1'), + ('static_inventory', None), + ('playbook', 'all') + ] + self.assertRaises(ParserException, lambda: self.check_parser( + self.cmd, argslist, verifylist)) + class TestOvercloudUpdateConverge(fakes.TestOvercloudUpdateConverge): diff --git a/tripleoclient/v1/overcloud_update.py b/tripleoclient/v1/overcloud_update.py index 6be9e8658..fe414324f 100644 --- a/tripleoclient/v1/overcloud_update.py +++ b/tripleoclient/v1/overcloud_update.py @@ -98,19 +98,37 @@ class UpdateRun(command.Command): def get_parser(self, prog_name): parser = super(UpdateRun, self).get_parser(prog_name) - parser.add_argument('--nodes', - action="store", - required=True, - help=_("Required parameter. This specifies the " - "overcloud nodes to run the minor update " - "playbooks on. You can use the name of " - "a specific node, or the name of the role " - "(e.g. Compute). You may also use the " - "special value 'all' to run the minor " - "on all nodes. In all cases the minor " - "update ansible playbook is executed on " - "one node at a time (with serial 1)") - ) + nodes_or_roles = parser.add_mutually_exclusive_group(required=True) + nodes_or_roles.add_argument( + '--nodes', action="store", help=_( + "A string that identifies a single node " + "or comma-separated list of nodes to be " + "updated in parallel in this minor update " + "run invocation. For example: --nodes " + "\"compute-0, compute-1, compute-5\". " + "NOTE: Using this parameter with nodes of " + "controlplane roles (e.g. \"--nodes " + "controller-1\") is NOT supported and WILL " + "end badly unless you include ALL nodes of " + "that role as a comma separated string. You " + "should instead use the --roles parameter " + "for controlplane roles and specify the " + "role name.") + ) + nodes_or_roles.add_argument( + '--roles', action="store", help=_( + "A string that identifies the role or " + "comma-separated list of roles to be " + "updated in this minor update run " + "invocation. " + "NOTE: Nodes of specified role(s) are " + "updated in parallel. This is REQUIRED for " + "controlplane roles (e.g., \"Compute\"), " + "you may consider instead using the --nodes " + "argument to limit the upgrade to a " + "specific node or list (comma separated " + "string) of nodes.") + ) parser.add_argument('--playbook', action="store", default="all", @@ -158,15 +176,15 @@ class UpdateRun(command.Command): stack = parsed_args.stack # Run ansible: + roles = parsed_args.roles nodes = parsed_args.nodes - if nodes == 'all': - # unset this, the ansible action deals with unset 'limithosts' - nodes = None + limit_hosts = roles or nodes playbook = parsed_args.playbook inventory = oooutils.get_tripleo_ansible_inventory( parsed_args.static_inventory, parsed_args.ssh_user, stack) - oooutils.run_update_ansible_action(self.log, clients, nodes, inventory, - playbook, constants.UPDATE_QUEUE, + oooutils.run_update_ansible_action(self.log, clients, limit_hosts, + inventory, playbook, + constants.UPDATE_QUEUE, constants.MINOR_UPDATE_PLAYBOOKS, package_update, parsed_args.ssh_user)