Fix --limit with commas

Kayobe allows specifying a --limit argument, which is passed through to
Ansible. In some cases we wish to add an intersection with a group. This
allows us to reuse playbooks for the seed, overcloud etc.

For example, the lvm.yml playbook specifies a host list of
seed-hypervisor:seed:overcloud. When executed as part of a kayobe
overcloud host configure command, Kayobe passes a limit of overcloud. If
the user specifies a --limit argument, this gets intersected with the
overcloud limit: host1:&overcloud.

The problem happens if the user specifies multiple parts to the host
pattern in their limit using a comma, e.g. host1,host2. This results in
host1,host2:&overcloud. Ansible ignores the colon, and treats this as
host1 or host2:&overcloud.

The solution is to use a comma to join the patterns if the user has used
a comma: host1,host2,&overcloud

Change-Id: Ibe42fa372c6fa0c539d2c2b0e238601286dc213d
Story: 2008255
Task: 41111

kayobe/ansible.py

@@ -146,8 +146,8 @@ def build_args(parsed_args, playbooks,
     if check or (parsed_args.check and check is None):
         cmd += ["--check"]
     if not ignore_limit and (parsed_args.limit or limit):
-        limits = [l for l in [parsed_args.limit, limit] if l]
-        cmd += ["--limit", ":&".join(limits)]
+        limit_arg = utils.intersect_limits(parsed_args.limit, limit)
+        cmd += ["--limit", limit_arg]
     if parsed_args.skip_tags:
         cmd += ["--skip-tags", parsed_args.skip_tags]
     if parsed_args.tags or tags:

kayobe/kolla_ansible.py

@@ -131,8 +131,8 @@ def build_args(parsed_args, command, inventory_filename, extra_vars=None,
             extra_var_value = utils.quote_and_escape(extra_var_value)
             cmd += ["-e", "%s=%s" % (extra_var_name, extra_var_value)]
     if parsed_args.kolla_limit or limit:
-        limits = [l for l in [parsed_args.kolla_limit, limit] if l]
-        cmd += ["--limit", ":&".join(limits)]
+        limit_arg = utils.intersect_limits(parsed_args.kolla_limit, limit)
+        cmd += ["--limit", limit_arg]
     if parsed_args.kolla_skip_tags:
         cmd += ["--skip-tags", parsed_args.kolla_skip_tags]
     if parsed_args.kolla_tags or tags:

kayobe/tests/unit/test_utils.py

@@ -19,6 +19,7 @@ from unittest import mock
 
 import yaml
 
+from kayobe import exception
 from kayobe import utils
 
 
@@ -127,3 +128,39 @@ key2: value2
         expected = os.path.normpath("/tmp/test/local/")
         result = utils._detect_install_prefix(path)
         self.assertEqual(expected, os.path.normpath(result))
+
+    def test_intersect_limits_no_arg_no_cli(self):
+        result = utils.intersect_limits(None, None)
+        self.assertEqual("", result)
+
+    def test_intersect_limits_arg_no_cli(self):
+        result = utils.intersect_limits("foo", None)
+        self.assertEqual("foo", result)
+
+    def test_intersect_limits_arg_no_cli_comma(self):
+        result = utils.intersect_limits("foo,bar", None)
+        self.assertEqual("foo,bar", result)
+
+    def test_intersect_limits_arg_no_cli_colon(self):
+        result = utils.intersect_limits("foo:bar", None)
+        self.assertEqual("foo:bar", result)
+
+    def test_intersect_limits_arg_no_cli_colon_and_comma(self):
+        self.assertRaises(exception.Error,
+                          utils.intersect_limits, "foo:bar,baz", None)
+
+    def test_intersect_limits_no_arg_cli(self):
+        result = utils.intersect_limits(None, "foo")
+        self.assertEqual("foo", result)
+
+    def test_intersect_limits_arg_and_cli(self):
+        result = utils.intersect_limits("foo", "bar")
+        self.assertEqual("foo:&bar", result)
+
+    def test_intersect_limits_arg_and_cli_comma(self):
+        result = utils.intersect_limits("foo,bar", "baz")
+        self.assertEqual("foo,bar,&baz", result)
+
+    def test_intersect_limits_arg_and_cli_colon(self):
+        result = utils.intersect_limits("foo:bar", "baz")
+        self.assertEqual("foo:bar:&baz", result)

kayobe/utils.py

@@ -22,6 +22,8 @@ import sys
 
 import yaml
 
+from kayobe import exception
+
 
 LOG = logging.getLogger(__name__)
 
@@ -195,3 +197,28 @@ def escape_jinja(string):
 
     b64_value = base64.b64encode(string.encode())
     return ''.join(('{{', "'", b64_value.decode(), "' | b64decode ", '}}'))
+
+
+def intersect_limits(args_limit, cli_limit):
+    """Create an Ansible host pattern of the intersection of two patterns.
+
+    :param args_limit: user-specified limit, or None.
+    :param cli_limit: limit originating from this CLI, or None.
+    :returns: a string representing an intersection of the two patterns.
+    """
+    # NOTE(mgoddard): Ansible uses either commas (,) or colons (:) to separate
+    # parts of a host pattern. An intersection is specified using a separator
+    # followed by an ampersand (&). If a mix of comma and colon separators is
+    # used, Ansible picks one and treats the other as part of the host pattern.
+    # This leads to hard to diagnose errors. Try to determine which separator
+    # the user has specified, and be consistent. Error if both are used.
+    if args_limit and ',' in args_limit:
+        if ':' in args_limit:
+            raise exception.Error("Invalid format for host limit argument. "
+                                  "Cannot mix commas and colons to separate "
+                                  "hosts")
+        separator = ',&'
+    else:
+        separator = ':&'
+    limits = [l for l in [args_limit, cli_limit] if l]
+    return separator.join(limits)

releasenotes/notes/fix-limit-with-commas-04a357b0b7ef0371.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixes an issue when using the ``--limit`` argument with a host pattern
+    including commas. See `story 2008255
+    <https://storyboard.openstack.org/#!/story/2008255>`__ for details.