Merge "Fix parsing of zookeeper jobboard backend options"

6 files changed, 145 insertions, 10 deletions

diff --git a/releasenotes/notes/fix-zookeeper-option-parsing-f9d37fbc39af47f4.yaml b/releasenotes/notes/fix-zookeeper-option-parsing-f9d37fbc39af47f4.yaml
new file mode 100644
index 0000000..3f81b29
--- /dev/null
+++ b/releasenotes/notes/fix-zookeeper-option-parsing-f9d37fbc39af47f4.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fixed an issue when the configuration options of the zookeeper jobboard
+    backend were passed as strings, the string ''"False"'' was wrongly
+    interpreted as ''True''. Now the string ''"False"'' is interpreted as the
+    ''False'' boolean.
diff --git a/taskflow/jobs/backends/impl_zookeeper.py b/taskflow/jobs/backends/impl_zookeeper.py
index fc9399a..1e45f0f 100644
--- a/taskflow/jobs/backends/impl_zookeeper.py
+++ b/taskflow/jobs/backends/impl_zookeeper.py
@@ -28,6 +28,7 @@ from kazoo.protocol import states as k_states
 from kazoo.recipe import watchers
 from oslo_serialization import jsonutils
 from oslo_utils import excutils
+from oslo_utils import strutils
 from oslo_utils import timeutils
 from oslo_utils import uuidutils
 
@@ -829,7 +830,8 @@ class ZookeeperJobBoard(base.NotifyingJobBoard):
             excp.raise_with_cause(excp.JobFailure,
                                   "Failed to connect to zookeeper")
         try:
-            if self._conf.get('check_compatible', True):
+            if strutils.bool_from_string(
+                    self._conf.get('check_compatible'), default=True):
                 kazoo_utils.check_compatible(self._client, self.MIN_ZK_VERSION)
             if self._worker is None and self._emit_notifications:
                 self._worker = futurist.ThreadPoolExecutor(max_workers=1)
diff --git a/taskflow/persistence/backends/impl_zookeeper.py b/taskflow/persistence/backends/impl_zookeeper.py
index 1189723..e75826e 100644
--- a/taskflow/persistence/backends/impl_zookeeper.py
+++ b/taskflow/persistence/backends/impl_zookeeper.py
@@ -20,6 +20,7 @@ import contextlib
 from kazoo import exceptions as k_exc
 from kazoo.protocol import paths
 from oslo_serialization import jsonutils
+from oslo_utils import strutils
 
 from taskflow import exceptions as exc
 from taskflow.persistence import path_based
@@ -161,7 +162,8 @@ class ZkConnection(path_based.PathBasedConnection):
     def validate(self):
         with self._exc_wrapper():
             try:
-                if self._conf.get('check_compatible', True):
+                if strutils.bool_from_string(
+                        self._conf.get('check_compatible'), default=True):
                     k_utils.check_compatible(self._client, MIN_ZK_VERSION)
             except exc.IncompatibleVersion:
                 exc.raise_with_cause(exc.StorageFailure, "Backend storage is"
diff --git a/taskflow/tests/unit/jobs/test_zk_job.py b/taskflow/tests/unit/jobs/test_zk_job.py
index d93f148..23d0505 100644
--- a/taskflow/tests/unit/jobs/test_zk_job.py
+++ b/taskflow/tests/unit/jobs/test_zk_job.py
@@ -293,3 +293,47 @@ class ZakeJobboardTest(test.TestCase, ZookeeperBoardTestMixin):
             self.assertRaises(excp.NotImplementedError,
                               self.board.register_entity,
                               entity_instance_2)
+
+    def test_connect_check_compatible(self):
+        # Valid version
+        client = fake_client.FakeClient()
+        board = impl_zookeeper.ZookeeperJobBoard(
+            'test-board', {'check_compatible': True},
+            client=client)
+        self.addCleanup(board.close)
+        self.addCleanup(self.close_client, client)
+
+        with base.connect_close(board):
+            pass
+
+        # Invalid version, no check
+        client = fake_client.FakeClient(server_version=(3, 2, 0))
+        board = impl_zookeeper.ZookeeperJobBoard(
+            'test-board', {'check_compatible': False},
+            client=client)
+        self.addCleanup(board.close)
+        self.addCleanup(self.close_client, client)
+
+        with base.connect_close(board):
+            pass
+
+        # Invalid version, check_compatible=True
+        client = fake_client.FakeClient(server_version=(3, 2, 0))
+        board = impl_zookeeper.ZookeeperJobBoard(
+            'test-board', {'check_compatible': True},
+            client=client)
+        self.addCleanup(board.close)
+        self.addCleanup(self.close_client, client)
+
+        self.assertRaises(excp.IncompatibleVersion, board.connect)
+
+        # Invalid version, check_compatible='False'
+        client = fake_client.FakeClient(server_version=(3, 2, 0))
+        board = impl_zookeeper.ZookeeperJobBoard(
+            'test-board', {'check_compatible': 'False'},
+            client=client)
+        self.addCleanup(board.close)
+        self.addCleanup(self.close_client, client)
+
+        with base.connect_close(board):
+            pass
diff --git a/taskflow/tests/unit/test_utils_kazoo_utils.py b/taskflow/tests/unit/test_utils_kazoo_utils.py
new file mode 100644
index 0000000..a28b3e8
--- /dev/null
+++ b/taskflow/tests/unit/test_utils_kazoo_utils.py
@@ -0,0 +1,67 @@
+# -*- coding: utf-8 -*-
+
+#    Copyright (C) Red Hat
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from unittest import mock
+
+from taskflow import test
+from taskflow.utils import kazoo_utils
+
+
+class MakeClientTest(test.TestCase):
+
+    @mock.patch("kazoo.client.KazooClient")
+    def test_make_client_config(self, mock_kazoo_client):
+        conf = {}
+        expected = {
+            'hosts': 'localhost:2181',
+            'logger': mock.ANY,
+            'read_only': False,
+            'randomize_hosts': False,
+            'keyfile': None,
+            'keyfile_password': None,
+            'certfile': None,
+            'use_ssl': False,
+            'verify_certs': True
+        }
+
+        kazoo_utils.make_client(conf)
+
+        mock_kazoo_client.assert_called_once_with(**expected)
+
+        mock_kazoo_client.reset_mock()
+
+        # With boolean passed as strings
+        conf = {
+            'use_ssl': 'True',
+            'verify_certs': 'False'
+        }
+        expected = {
+            'hosts': 'localhost:2181',
+            'logger': mock.ANY,
+            'read_only': False,
+            'randomize_hosts': False,
+            'keyfile': None,
+            'keyfile_password': None,
+            'certfile': None,
+            'use_ssl': True,
+            'verify_certs': False
+        }
+
+        kazoo_utils.make_client(conf)
+
+        mock_kazoo_client.assert_called_once_with(**expected)
+
+        mock_kazoo_client.reset_mock()
diff --git a/taskflow/utils/kazoo_utils.py b/taskflow/utils/kazoo_utils.py
index 505c101..2b9f7b9 100644
--- a/taskflow/utils/kazoo_utils.py
+++ b/taskflow/utils/kazoo_utils.py
@@ -17,6 +17,7 @@
 from kazoo import client
 from kazoo import exceptions as k_exc
 from oslo_utils import reflection
+from oslo_utils import strutils
 
 from taskflow import exceptions as exc
 from taskflow import logging
@@ -24,6 +25,15 @@ from taskflow import logging
 
 LOG = logging.getLogger(__name__)
 
+CONF_TRANSFERS = (
+    ('read_only', strutils.bool_from_string, False),
+    ('randomize_hosts', strutils.bool_from_string, False),
+    ('keyfile', None, None),
+    ('keyfile_password', None, None),
+    ('certfile', None, None),
+    ('use_ssl', strutils.bool_from_string, False),
+    ('verify_certs', strutils.bool_from_string, True))
+
 
 def _parse_hosts(hosts):
     if isinstance(hosts, str):
@@ -193,16 +203,19 @@ def make_client(conf):
     """
     # See: https://kazoo.readthedocs.io/en/latest/api/client.html
     client_kwargs = {
-        'read_only': bool(conf.get('read_only')),
-        'randomize_hosts': bool(conf.get('randomize_hosts')),
         'logger': LOG,
-        'keyfile': conf.get('keyfile', None),
-        'keyfile_password': conf.get('keyfile_password', None),
-        'certfile': conf.get('certfile', None),
-        'use_ssl': conf.get('use_ssl', False),
-        'verify_certs': conf.get('verify_certs', True),
-
     }
+
+    for key, value_type_converter, default in CONF_TRANSFERS:
+        if key in conf:
+            if value_type_converter is not None:
+                client_kwargs[key] = value_type_converter(conf[key],
+                                                          default=default)
+            else:
+                client_kwargs[key] = conf[key]
+        else:
+            client_kwargs[key] = default
+
     # See: https://kazoo.readthedocs.io/en/latest/api/retry.html
     if 'command_retry' in conf:
         client_kwargs['command_retry'] = conf['command_retry']