Add --roles and --nodes params to overcloud upgrade run cli

This adds a required, mutually exclusive argument group for
--roles and --nodes. --roles replaces the existing --role.

This shifts the semantics of upgrading 'one at a time' (compute)
vs 'all together' (controller) to the operator. You can either
specify roles or nodes but not both. Using nodes for contollers
is not supported but we can only warn.

  openstack overcloud upgrade run --roles Controller
  openstack overcloud upgrade run --nodes overcloud-compute-0 overcloud-compute-1
  #for the really ambitious
  openstack overcloud upgrade run --roles Controller Networker

Change-Id: I0418883557113b5a67cfeaf181614dc5fe32ca94

releasenotes/notes/queens-upgrade-cli-add-roles-nodes.yaml-6ac6ecda01286006.yaml

@@ -0,0 +1,16 @@
+---
+upgrade:
+  - |
+    This adds the new --roles and --nodes parameters for the Queens major
+    upgrade cli, specifically for the 'openstack overcloud upgrade run' which
+    executes the ansible playbooks on overcloud nodes.
+
+        openstack overcloud upgrade run --nodes compute-0 compute-1
+        openstack overcloud upgrade run --roles Controller
+
+    Nodes for controlplane roles (the default 'Controller' role for example)
+    need to be upgraded using the --roles parameter as these nodes must be
+    upgraded together/in parallel.
+
+    For non controlplane roles the --nodes parameter can be used to limit the
+    upgrade run to one or more nodes as specified by the operator.

tripleoclient/tests/v1/overcloud_upgrade/test_overcloud_upgrade.py

@@ -120,13 +120,13 @@ class TestOvercloudUpgradeRun(fakes.TestOvercloudUpgradeRun):
     @mock.patch('os.path.expanduser')
     @mock.patch('oslo_concurrency.processutils.execute')
     @mock.patch('six.moves.builtins.open')
-    def test_upgrade_with_playbook(self, mock_open, mock_execute,
-                                   mock_expanduser, upgrade_ansible):
+    def test_upgrade_roles_with_playbook(
+            self, mock_open, mock_execute, mock_expanduser, upgrade_ansible):
         mock_expanduser.return_value = '/home/fake/'
-        argslist = ['--nodes', 'Compute', '--playbook',
-                    'fake-playbook.yaml']
+        argslist = ['--roles', 'Compute', 'Controller',
+                    '--playbook', 'fake-playbook.yaml']
         verifylist = [
-            ('nodes', 'Compute'),
+            ('roles', ['Compute', 'Controller']),
             ('static_inventory', None),
             ('playbook', 'fake-playbook.yaml')
         ]
@@ -137,7 +137,7 @@ class TestOvercloudUpgradeRun(fakes.TestOvercloudUpgradeRun):
             self.cmd.take_action(parsed_args)
             upgrade_ansible.assert_called_once_with(
                 self.app.client_manager,
-                nodes='Compute',
+                nodes=['Compute', 'Controller'],
                 inventory_file=mock_open().read(),
                 playbook='fake-playbook.yaml',
                 ansible_queue_name=constants.UPGRADE_QUEUE
@@ -148,12 +148,12 @@ class TestOvercloudUpgradeRun(fakes.TestOvercloudUpgradeRun):
     @mock.patch('os.path.expanduser')
     @mock.patch('oslo_concurrency.processutils.execute')
     @mock.patch('six.moves.builtins.open')
-    def test_upgrade_with_all_playbooks(self, mock_open, mock_execute,
-                                        mock_expanduser, upgrade_ansible):
+    def test_upgrade_role_all_playbooks(
+            self, mock_open, mock_execute, mock_expanduser, upgrade_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')
         ]
@@ -165,7 +165,7 @@ class TestOvercloudUpgradeRun(fakes.TestOvercloudUpgradeRun):
             for book in constants.MAJOR_UPGRADE_PLAYBOOKS:
                 upgrade_ansible.assert_any_call(
                     self.app.client_manager,
-                    nodes='Compute',
+                    nodes=['Compute'],
                     inventory_file=mock_open().read(),
                     playbook=book,
                     ansible_queue_name=constants.UPGRADE_QUEUE
@@ -176,11 +176,82 @@ class TestOvercloudUpgradeRun(fakes.TestOvercloudUpgradeRun):
     @mock.patch('os.path.expanduser')
     @mock.patch('oslo_concurrency.processutils.execute')
     @mock.patch('six.moves.builtins.open')
-    def test_upgrade_with_no_nodes(self, mock_open, mock_execute,
-                                   mock_expanduser, upgrade_ansible):
+    def test_upgrade_nodes_with_playbook(
+            self, mock_open, mock_execute, mock_expanduser, upgrade_ansible):
+        mock_expanduser.return_value = '/home/fake/'
+        argslist = ['--nodes', 'compute-0', 'compute-1',
+                    '--playbook', 'fake-playbook.yaml']
+        verifylist = [
+            ('nodes', ['compute-0', 'compute-1']),
+            ('static_inventory', None),
+            ('playbook', 'fake-playbook.yaml')
+        ]
+
+        parsed_args = self.check_parser(self.cmd, argslist, verifylist)
+        with mock.patch('os.path.exists') as mock_exists:
+            mock_exists.return_value = True
+            self.cmd.take_action(parsed_args)
+            upgrade_ansible.assert_called_once_with(
+                self.app.client_manager,
+                nodes=['compute-0', 'compute-1'],
+                inventory_file=mock_open().read(),
+                playbook='fake-playbook.yaml',
+                ansible_queue_name=constants.UPGRADE_QUEUE
+            )
+
+    @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_upgrade_node_all_playbooks(
+            self, mock_open, mock_execute, mock_expanduser, upgrade_ansible):
+        mock_expanduser.return_value = '/home/fake/'
+        argslist = ['--nodes', 'swift-1', '--playbook', 'all']
+        verifylist = [
+            ('nodes', ['swift-1']),
+            ('static_inventory', None),
+            ('playbook', 'all')
+        ]
+
+        parsed_args = self.check_parser(self.cmd, argslist, verifylist)
+        with mock.patch('os.path.exists') as mock_exists:
+            mock_exists.return_value = True
+            self.cmd.take_action(parsed_args)
+            for book in constants.MAJOR_UPGRADE_PLAYBOOKS:
+                upgrade_ansible.assert_any_call(
+                    self.app.client_manager,
+                    nodes=['swift-1'],
+                    inventory_file=mock_open().read(),
+                    playbook=book,
+                    ansible_queue_name=constants.UPGRADE_QUEUE
+                )
+
+    @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_upgrade_no_nodes_or_roles(self, mock_open, mock_execute,
+                                       mock_expanduser, upgrade_ansible):
         mock_expanduser.return_value = '/home/fake/'
         argslist = []
+        verifylist = []
+        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_upgrade_nodes_and_roles(self, mock_open, mock_execute,
+                                     mock_expanduser, upgrade_ansible):
+        mock_expanduser.return_value = '/home/fake/'
+        argslist = ['--roles', 'Compute', '--nodes', 'overcloud-controller-1']
         verifylist = [
+            ('roles', ['Compute']),
+            ('nodes', ['overcloud-controller-1']),
             ('static_inventory', None),
             ('playbook', 'all')
         ]

tripleoclient/v1/overcloud_upgrade.py

@@ -102,15 +102,26 @@ class UpgradeRun(command.Command):
 
     def get_parser(self, prog_name):
         parser = super(UpgradeRun, self).get_parser(prog_name)
-        parser.add_argument('--nodes',
-                            action="store",
-                            required=True,
-                            help=_("Required parameter. This specifies the "
-                                   "overcloud nodes to run the major upgrade "
-                                   "playbooks on. You can use the name of "
-                                   "a specific node, or the name of the role "
-                                   "(e.g. Compute).")
-                            )
+        nodes_or_roles = parser.add_mutually_exclusive_group(required=True)
+        nodes_or_roles.add_argument(
+            '--nodes', action="store", nargs='+', help=_(
+                "Use this to identify a single node or list of nodes to be "
+                "upgraded in parallel in this upgrade run invocation. "
+                "NOTE: Using this parameter with nodes of controlplane roles "
+                "(e.g. \"Controller\") is NOT supported and WILL end badly "
+                "unless you include ALL nodes of that role. It is preferable "
+                "to instead specify the role name with the --roles parameter.")
+        )
+        nodes_or_roles.add_argument(
+            '--roles', action="store", nargs='+', help=_(
+                "Specify the role or list of roles to be upgraded in this "
+                "upgrade run invocation. "
+                "NOTE: nodes of specified role(s) are upgraded in parallel. "
+                "This is REQUIRED for controlplane roles. For non "
+                "controlplane roles (e.g., \"Compute\"), you may consider "
+                "instead using the --nodes argument to limit the upgrade to "
+                "a specific node or nodes.")
+        )
         parser.add_argument('--playbook',
                             action="store",
                             default="all",
@@ -143,9 +154,10 @@ class UpgradeRun(command.Command):
     def take_action(self, parsed_args):
         self.log.debug("take_action(%s)" % parsed_args)
         clients = self.app.client_manager
-
         # Run ansible:
+        roles = parsed_args.roles
         nodes = parsed_args.nodes
+        limit_hosts = roles or nodes
         playbook = parsed_args.playbook
         inventory = oooutils.get_tripleo_ansible_inventory(
             parsed_args.static_inventory)
@@ -155,7 +167,7 @@ class UpgradeRun(command.Command):
         for book in upgrade_playbooks:
             self.log.debug("Running major upgrade ansible playbook %s " % book)
             package_update.update_ansible(
-                clients, nodes=nodes,
+                clients, nodes=limit_hosts,
                 inventory_file=inventory,
                 playbook=book,
                 ansible_queue_name=constants.UPGRADE_QUEUE)