Merge pull request ceph#54081 from phlogistonjohn/jjm-cephadm-podman-props

cephadm: move some podman specific logic to Podman methods

Reviewed-by: Adam King <[email protected]>

Author: adk3798

src/cephadm/cephadm.py

@@ -59,7 +59,6 @@
     CEPH_DEFAULT_PUBKEY,
     CEPH_KEYRING,
     CEPH_PUBKEY,
-    CGROUPS_SPLIT_PODMAN_VERSION,
     CONTAINER_INIT,
     CUSTOM_PS1,
     DATA_DIR,
@@ -2802,29 +2801,7 @@ def get_container(
             f'--env-file={sg.conf_file_path}'
         )
 
-    # if using podman, set -d, --conmon-pidfile & --cidfile flags
-    # so service can have Type=Forking
-    if isinstance(ctx.container_engine, Podman):
-        runtime_dir = '/run'
-        service_name = f'{ident.unit_name}.service'
-        container_args.extend([
-            '-d', '--log-driver', 'journald',
-            '--conmon-pidfile',
-            f'{runtime_dir}/{service_name}-pid',
-            '--cidfile',
-            f'{runtime_dir}/{service_name}-cid',
-        ])
-        if ctx.container_engine.version >= CGROUPS_SPLIT_PODMAN_VERSION and not ctx.no_cgroups_split:
-            container_args.append('--cgroups=split')
-        # if /etc/hosts doesn't exist, we can be confident
-        # users aren't using it for host name resolution
-        # and adding --no-hosts avoids bugs created in certain daemons
-        # by modifications podman makes to /etc/hosts
-        # https://tracker.ceph.com/issues/58532
-        # https://tracker.ceph.com/issues/57018
-        if not os.path.exists('/etc/hosts'):
-            container_args.extend(['--no-hosts'])
-
+    _update_container_args_for_podman(ctx, ident, container_args)
     return CephContainer.for_daemon(
         ctx,
         ident=ident,
@@ -2840,6 +2817,17 @@ def get_container(
     )
 
 
+def _update_container_args_for_podman(
+    ctx: CephadmContext, ident: DaemonIdentity, container_args: List[str]
+) -> None:
+    if not isinstance(ctx.container_engine, Podman):
+        return
+    service_name = f'{ident.unit_name}.service'
+    container_args.extend(
+        ctx.container_engine.service_args(ctx, service_name)
+    )
+
+
 def extract_uid_gid(ctx, img='', file_path='/var/lib/ceph'):
     # type: (CephadmContext, str, Union[str, List[str]]) -> Tuple[int, int]
 
@@ -3406,7 +3394,7 @@ def get_unit_file(ctx, fsid):
                       'ExecStopPost=-/bin/rm -f %t/%n-pid %t/%n-cid\n'
                       'Type=forking\n'
                       'PIDFile=%t/%n-pid\n')
-        if ctx.container_engine.version >= CGROUPS_SPLIT_PODMAN_VERSION:
+        if ctx.container_engine.supports_split_cgroups:
             extra_args += 'Delegate=yes\n'
 
     docker = isinstance(ctx.container_engine, Docker)

src/cephadm/cephadmlib/container_engines.py

@@ -7,7 +7,11 @@
 from .call_wrappers import call_throws, CallVerbosity
 from .context import CephadmContext
 from .container_engine_base import ContainerEngine
-from .constants import DEFAULT_MODE, MIN_PODMAN_VERSION
+from .constants import (
+    CGROUPS_SPLIT_PODMAN_VERSION,
+    DEFAULT_MODE,
+    MIN_PODMAN_VERSION,
+)
 from .exceptions import Error
 
 
@@ -36,6 +40,44 @@ def __str__(self) -> str:
         version = '.'.join(map(str, self.version))
         return f'{self.EXE} ({self.path}) version {version}'
 
+    @property
+    def supports_split_cgroups(self) -> bool:
+        """Return true if this version of podman supports split cgroups."""
+        return self.version >= CGROUPS_SPLIT_PODMAN_VERSION
+
+    def service_args(
+        self, ctx: CephadmContext, service_name: str
+    ) -> List[str]:
+        """Return a list of arguments that should be added to the engine's run
+        command when starting a long-term service (aka daemon) container.
+        """
+        args = []
+        # if using podman, set -d, --conmon-pidfile & --cidfile flags
+        # so service can have Type=Forking
+        runtime_dir = '/run'
+        args.extend(
+            [
+                '-d',
+                '--log-driver',
+                'journald',
+                '--conmon-pidfile',
+                f'{runtime_dir}/{service_name}-pid',
+                '--cidfile',
+                f'{runtime_dir}/{service_name}-cid',
+            ]
+        )
+        if self.supports_split_cgroups and not ctx.no_cgroups_split:
+            args.append('--cgroups=split')
+        # if /etc/hosts doesn't exist, we can be confident
+        # users aren't using it for host name resolution
+        # and adding --no-hosts avoids bugs created in certain daemons
+        # by modifications podman makes to /etc/hosts
+        # https://tracker.ceph.com/issues/58532
+        # https://tracker.ceph.com/issues/57018
+        if not os.path.exists('/etc/hosts'):
+            args.append('--no-hosts')
+        return args
+
 
 class Docker(ContainerEngine):
     EXE = 'docker'

src/cephadm/cephadmlib/context_getters.py

@@ -6,7 +6,6 @@
 
 from typing import Any, Dict, List, Optional, Tuple, Union
 
-from .constants import CGROUPS_SPLIT_PODMAN_VERSION
 from .container_engines import Podman
 from .context import CephadmContext
 from .exceptions import Error
@@ -186,5 +185,5 @@ def should_log_to_journald(ctx: CephadmContext) -> bool:
         return ctx.log_to_journald
     return (
         isinstance(ctx.container_engine, Podman)
-        and ctx.container_engine.version >= CGROUPS_SPLIT_PODMAN_VERSION
+        and ctx.container_engine.supports_split_cgroups
     )

src/cephadm/tests/fixtures.py

@@ -17,17 +17,26 @@ def import_cephadm():
 
 
 def mock_docker():
-    _cephadm = import_cephadm()
-    docker = mock.Mock(_cephadm.Docker)
+    from cephadmlib.container_engines import Docker
+
+    docker = mock.Mock(Docker)
     docker.path = '/usr/bin/docker'
     return docker
 
 
 def mock_podman():
-    _cephadm = import_cephadm()
-    podman = mock.Mock(_cephadm.Podman)
+    from cephadmlib.container_engines import Podman
+
+    podman = mock.Mock(Podman)
     podman.path = '/usr/bin/podman'
     podman.version = (2, 1, 0)
+    # This next little bit of black magic was adapated from the mock docs for
+    # PropertyMock. We don't use a PropertyMock but the suggestion to call
+    # type(...) from the doc allows us to "borrow" the real
+    # supports_split_cgroups attribute:
+    # https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock
+    type(podman).supports_split_cgroups = Podman.supports_split_cgroups
+    type(podman).service_args = Podman.service_args
     return podman