Merge pull request #126 from ceph/mergify/bp/pacific/pr-124

ceph_config: get_or_set_option() always does a 'get' (backport #124)

library/ceph_config.py

@@ -8,9 +8,9 @@
 
 from ansible.module_utils.basic import AnsibleModule  # type: ignore
 try:
-    from ansible.module_utils.ceph_common import exit_module, build_base_cmd_sh  # type: ignore
+    from ansible.module_utils.ceph_common import exit_module, build_base_cmd_shell, fatal  # type: ignore
 except ImportError:
-    from module_utils.ceph_common import exit_module, build_base_cmd_sh  # type: ignore
+    from module_utils.ceph_common import exit_module, build_base_cmd_shell, fatal  # type: ignore
 
 import datetime
 
@@ -89,8 +89,11 @@ def get_or_set_option(module: "AnsibleModule",
                       who: str,
                       option: str,
                       value: str) -> Tuple[int, List[str], str, str]:
-    cmd = build_base_cmd_sh(module)
-    cmd.extend(['ceph', 'config', 'get', who, option])
+    cmd = build_base_cmd_shell(module)
+    cmd.extend(['ceph', 'config', action, who, option])
+
+    if action == 'set':
+        cmd.append(value)
 
     rc, out, err = module.run_command(cmd)
 
@@ -133,9 +136,11 @@ def main() -> None:
     changed = False
 
     rc, cmd, out, err = get_or_set_option(module, 'get', who, option, value)
+    if rc:
+        fatal(message=f"Can't get current value. who={who} option={option}", module=module)
 
     if action == 'set':
-        if value == out:
+        if value.lower() == out:
             out = 'who={} option={} value={} already set. Skipping.'.format(who, option, value)
         else:
             rc, cmd, out, err = get_or_set_option(module, action, who, option, value)

library/ceph_orch_apply.py

@@ -20,9 +20,9 @@
 
 from ansible.module_utils.basic import AnsibleModule  # type: ignore
 try:
-    from ansible.module_utils.ceph_common import exit_module, build_base_cmd  # type: ignore
+    from ansible.module_utils.ceph_common import exit_module, build_base_cmd_orch  # type: ignore
 except ImportError:
-    from module_utils.ceph_common import exit_module, build_base_cmd
+    from module_utils.ceph_common import exit_module, build_base_cmd_orch
 import datetime
 
 
@@ -72,7 +72,7 @@
 
 def apply_spec(module: "AnsibleModule",
                data: str) -> Tuple[int, List[str], str, str]:
-    cmd = build_base_cmd(module)
+    cmd = build_base_cmd_orch(module)
     cmd.extend(['apply', '-i', '-'])
     rc, out, err = module.run_command(cmd, data=data)
 

library/ceph_orch_daemon.py

@@ -8,9 +8,9 @@
 
 from ansible.module_utils.basic import AnsibleModule  # type: ignore
 try:
-    from ansible.module_utils.ceph_common import retry, exit_module, build_base_cmd, fatal  # type: ignore
+    from ansible.module_utils.ceph_common import retry, exit_module, build_base_cmd_orch, fatal  # type: ignore
 except ImportError:
-    from module_utils.ceph_common import retry, exit_module, build_base_cmd, fatal  # type: ignore
+    from module_utils.ceph_common import retry, exit_module, build_base_cmd_orch, fatal  # type: ignore
 
 import datetime
 import json
@@ -77,7 +77,7 @@
 def get_current_state(module: "AnsibleModule",
                       daemon_type: str,
                       daemon_id: str) -> Tuple[int, List[str], str, str]:
-    cmd = build_base_cmd(module)
+    cmd = build_base_cmd_orch(module)
     cmd.extend(['ps', '--daemon_type',
                 daemon_type, '--daemon_id',
                 daemon_id, '--format', 'json',
@@ -90,7 +90,7 @@ def get_current_state(module: "AnsibleModule",
 def update_daemon_status(module: "AnsibleModule",
                          action: str,
                          daemon_name: str) -> Tuple[int, List[str], str, str]:
-    cmd = build_base_cmd(module)
+    cmd = build_base_cmd_orch(module)
     cmd.extend(['daemon', action, daemon_name])
     rc, out, err = module.run_command(cmd)
 

library/ceph_orch_host.py

@@ -20,9 +20,9 @@
 
 from ansible.module_utils.basic import AnsibleModule  # type: ignore
 try:
-    from ansible.module_utils.ceph_common import exit_module, build_base_cmd  # type: ignore
+    from ansible.module_utils.ceph_common import exit_module, build_base_cmd_orch  # type: ignore
 except ImportError:
-    from module_utils.ceph_common import exit_module, build_base_cmd
+    from module_utils.ceph_common import exit_module, build_base_cmd_orch
 import datetime
 import json
 
@@ -105,7 +105,7 @@
 
 
 def get_current_state(module: "AnsibleModule") -> Tuple[int, List[str], str, str]:
-    cmd = build_base_cmd(module)
+    cmd = build_base_cmd_orch(module)
     cmd.extend(['host', 'ls', '--format', 'json'])
     rc, out, err = module.run_command(cmd)
 
@@ -119,7 +119,7 @@ def update_label(module: "AnsibleModule",
                  action: str,
                  host: str,
                  label: str = '') -> Tuple[int, List[str], str, str]:
-    cmd = build_base_cmd(module)
+    cmd = build_base_cmd_orch(module)
     cmd.extend(['host', 'label', action,
                 host, label])
     rc, out, err = module.run_command(cmd)
@@ -135,7 +135,7 @@ def update_host(module: "AnsibleModule",
                 name: str,
                 address: str = '',
                 labels: List[str] = None) -> Tuple[int, List[str], str, str]:
-    cmd = build_base_cmd(module)
+    cmd = build_base_cmd_orch(module)
     cmd.extend(['host', action, name])
     if action == 'add':
         cmd.append(address)

module_utils/ceph_common.py

@@ -23,46 +23,33 @@ def _retry(*args, **kwargs):
     return decorator
 
 
-def build_base_cmd_sh(module: "AnsibleModule") -> List[str]:
-
-    cmd = ['cephadm', 'shell']
+def build_base_cmd(module: "AnsibleModule") -> List[str]:
+    cmd = ['cephadm']
+    docker = module.params.get('docker')
+    image = module.params.get('image')
 
-    fsid = module.params.get('fsid')
-    if fsid:
-        cmd.extend(['--fsid', fsid])
+    if docker:
+        cmd.append('--docker')
+    if image:
+        cmd.extend(['--image', image])
 
     return cmd
 
 
-def build_cmd(module: "AnsibleModule",
-              cli_binary: bool,
-              cmd: str) -> List[str]:
-
-    _cmd = []
+def build_base_cmd_shell(module: "AnsibleModule") -> List[str]:
+    cmd = build_base_cmd(module)
     fsid = module.params.get('fsid')
 
-    if not cli_binary:
-        _cmd.extend(['cephadm', 'shell'])
-        if fsid:
-            _cmd.extend(['--fsid', fsid])
-        _cmd.append('ceph')
-
-    _cmd.extend(cmd.split(' '))
+    cmd.append('shell')
 
-    return _cmd
+    if fsid:
+        cmd.extend(['--fsid', fsid])
 
+    return cmd
 
-def build_base_cmd(module: "AnsibleModule"):
-    cmd = ['cephadm']
-    fsid = module.params.get('fsid')
 
-    if module.params.get('docker'):
-        cmd.append('--docker')
-    if module.params.get('image'):
-        cmd.extend(['--image', module.params.get('image')])
-    cmd.append('shell')
-    if fsid:
-        cmd.extend(['--fsid', fsid])
+def build_base_cmd_orch(module: "AnsibleModule") -> List[str]:
+    cmd = build_base_cmd_shell(module)
     cmd.extend(['ceph', 'orch'])
 
     return cmd

tests/module_utils/test_ceph_common.py

@@ -9,32 +9,27 @@ def setup_method(self):
         self.fake_params = {'foo': 'bar'}
         self.fake_module.params = self.fake_params
 
-    def test_build_base_cmd_with_fsid_arg(self):
+    def test_build_base_cmd_orch_with_fsid_arg(self):
         expected_cmd = ['cephadm', 'shell', '--fsid', '123', 'ceph', 'orch']
         self.fake_module.params = {'fsid': '123'}
-        cmd = ceph_common.build_base_cmd(self.fake_module)
+        cmd = ceph_common.build_base_cmd_orch(self.fake_module)
         assert cmd == expected_cmd
 
-    def test_build_base_cmd_with_image_arg(self):
+    def test_build_base_cmd_orch_with_image_arg(self):
         expected_cmd = ['cephadm', '--image', 'quay.io/ceph-ci/ceph:main', 'shell', 'ceph', 'orch']
         self.fake_module.params = {'image': 'quay.io/ceph-ci/ceph:main'}
-        cmd = ceph_common.build_base_cmd(self.fake_module)
+        cmd = ceph_common.build_base_cmd_orch(self.fake_module)
         assert cmd == expected_cmd
 
-    def test_build_base_cmd_with_docker_arg(self):
+    def test_build_base_cmd_orch_with_docker_arg(self):
         expected_cmd = ['cephadm', '--docker', 'shell', 'ceph', 'orch']
         self.fake_module.params = {'docker': True}
-        cmd = ceph_common.build_base_cmd(self.fake_module)
+        cmd = ceph_common.build_base_cmd_orch(self.fake_module)
         assert cmd == expected_cmd
 
-    def test_build_base_cmd_no_arg(self):
+    def test_build_base_cmd_orch_no_arg(self):
         expected_cmd = ['cephadm', 'shell', 'ceph', 'orch']
-        cmd = ceph_common.build_base_cmd(self.fake_module)
-        assert cmd == expected_cmd
-
-    def test_build_base_cmd_sh_no_arg(self):
-        expected_cmd = ['cephadm', 'shell']
-        cmd = ceph_common.build_base_cmd_sh(self.fake_module)
+        cmd = ceph_common.build_base_cmd_orch(self.fake_module)
         assert cmd == expected_cmd
 
     def test_fatal(self):