cephadm: remove get_deployment_container

Remove get_deployment_container replacing all calls to it with calls to
to_deployment_container. Now, callers can inject modifications to the
container object between calls or even have to_deployment_container
update container objects that were constructed in some other way.

Signed-off-by: John Mulligan <jmulligan@redhat.com>

diff --git a/src/cephadm/cephadm.py b/src/cephadm/cephadm.py
index 23420c2b473d15ab09a53f768e3329748f1e6b16..bd0a1c97f4b3b9ad05289ad89a27b03a6bafb0c9 100755 (executable)
--- a/src/cephadm/cephadm.py
+++ b/src/cephadm/cephadm.py
@@ -243,7 +243,8 @@ class Ceph(ContainerDaemonForm):
         uid, gid = self.uid_gid(ctx)
         make_var_run(ctx, ctx.fsid, uid, gid)
 
-        ctr = get_deployment_container(ctx, self.identity)
+        ctr = get_container(ctx, self.identity)
+        ctr = to_deployment_container(ctx, ctr)
         config_json = fetch_configs(ctx)
         if self.identity.daemon_type == 'mon' and config_json is not None:
             if 'crush_location' in config_json:
@@ -464,7 +465,8 @@ class SNMPGateway(ContainerDaemonForm):
             raise Error('config is missing destination attribute(<ip>:<port>) of the target SNMP listener')
 
     def container(self, ctx: CephadmContext) -> CephContainer:
-        return get_deployment_container(ctx, self.identity)
+        ctr = get_container(ctx, self.identity)
+        return to_deployment_container(ctx, ctr)
 
     def uid_gid(self, ctx: CephadmContext) -> Tuple[int, int]:
         return self.uid, self.gid
@@ -626,7 +628,8 @@ class Monitoring(ContainerDaemonForm):
 
     def container(self, ctx: CephadmContext) -> CephContainer:
         self._prevalidate(ctx)
-        return get_deployment_container(ctx, self.identity)
+        ctr = get_container(ctx, self.identity)
+        return to_deployment_container(ctx, ctr)
 
     def uid_gid(self, ctx: CephadmContext) -> Tuple[int, int]:
         return self.extract_uid_gid(ctx, self.identity.daemon_type)
@@ -814,7 +817,8 @@ class NFSGanesha(ContainerDaemonForm):
         return 'nfs'
 
     def container(self, ctx: CephadmContext) -> CephContainer:
-        return get_deployment_container(ctx, self.identity)
+        ctr = get_container(ctx, self.identity)
+        return to_deployment_container(ctx, ctr)
 
     def customize_container_endpoints(
         self, endpoints: List[EndPoint], deployment_type: DeploymentType
@@ -1023,7 +1027,9 @@ done
         subident = DaemonSubIdentity(
             self.fsid, self.daemon_type, self.daemon_id, 'tcmu'
         )
-        tcmu_container = get_deployment_container(self.ctx, subident)
+        tcmu_container = to_deployment_container(
+            self.ctx, get_container(self.ctx, subident)
+        )
         # TODO: Eventually we don't want to run tcmu-runner through this script.
         # This is intended to be a workaround backported to older releases
         # and should eventually be removed in at least squid onward
@@ -1032,7 +1038,8 @@ done
         return tcmu_container
 
     def container(self, ctx: CephadmContext) -> CephContainer:
-        return get_deployment_container(ctx, self.identity)
+        ctr = get_container(ctx, self.identity)
+        return to_deployment_container(ctx, ctr)
 
     def config_and_keyring(
         self, ctx: CephadmContext
@@ -1181,7 +1188,8 @@ class CephNvmeof(ContainerDaemonForm):
         ]
 
     def container(self, ctx: CephadmContext) -> CephContainer:
-        return get_deployment_container(ctx, self.identity)
+        ctr = get_container(ctx, self.identity)
+        return to_deployment_container(ctx, ctr)
 
     def uid_gid(self, ctx: CephadmContext) -> Tuple[int, int]:
         return 167, 167  # TODO: need to get properly the uid/gid
@@ -1265,7 +1273,8 @@ class CephExporter(ContainerDaemonForm):
             raise Error(f'Directory does not exist. Got: {self.sock_dir}')
 
     def container(self, ctx: CephadmContext) -> CephContainer:
-        return get_deployment_container(ctx, self.identity)
+        ctr = get_container(ctx, self.identity)
+        return to_deployment_container(ctx, ctr)
 
     def uid_gid(self, ctx: CephadmContext) -> Tuple[int, int]:
         return extract_uid_gid(ctx)
@@ -1378,7 +1387,8 @@ class HAproxy(ContainerDaemonForm):
         ]
 
     def container(self, ctx: CephadmContext) -> CephContainer:
-        return get_deployment_container(ctx, self.identity)
+        ctr = get_container(ctx, self.identity)
+        return to_deployment_container(ctx, ctr)
 
 ##################################
 
@@ -1490,7 +1500,8 @@ class Keepalived(ContainerDaemonForm):
         return mounts
 
     def container(self, ctx: CephadmContext) -> CephContainer:
-        return get_deployment_container(ctx, self.identity)
+        ctr = get_container(ctx, self.identity)
+        return to_deployment_container(ctx, ctr)
 
 
 ##################################
@@ -1546,9 +1557,8 @@ class Tracing(ContainerDaemonForm):
         return self._identity
 
     def container(self, ctx: CephadmContext) -> CephContainer:
-        # TODO(jjm) this looks to be the only container for deployment
-        # not using get_deployment_container. Previous oversight?
-        return get_container(ctx, self.identity)
+        ctr = get_container(ctx, self.identity)
+        return to_deployment_container(ctx, ctr)
 
     def uid_gid(self, ctx: CephadmContext) -> Tuple[int, int]:
         return 65534, 65534
@@ -1686,12 +1696,13 @@ class CustomContainer(ContainerDaemonForm):
 
     def container(self, ctx: CephadmContext) -> CephContainer:
         if self._container is None:
-            self._container = get_deployment_container(
+            ctr = get_container(
                 ctx,
                 self.identity,
                 privileged=self.privileged,
                 ptrace=ctx.allow_ptrace,
             )
+            self._container = to_deployment_container(ctx, ctr)
         return self._container
 
     def init_containers(self, ctx: CephadmContext) -> List[InitContainer]:
@@ -5096,19 +5107,6 @@ def command_registry_login(ctx: CephadmContext) -> int:
 ##################################
 
 
-def get_deployment_container(
-    ctx: CephadmContext,
-    ident: 'DaemonIdentity',
-    privileged: bool = False,
-    ptrace: bool = False,
-    container_args: Optional[List[str]] = None,
-) -> 'CephContainer':
-    # wrapper for get_container specifically for containers made during the `cephadm deploy`
-    # command. Adds some extra things such as extra container args and custom config files
-    c = get_container(ctx, ident, privileged, ptrace, container_args)
-    return to_deployment_container(ctx, c)
-
-
 def to_deployment_container(
     ctx: CephadmContext, ctr: CephContainer
 ) -> CephContainer:

diff --git a/src/cephadm/tests/test_cephadm.py b/src/cephadm/tests/test_cephadm.py
index 7e31b26307c36a47f039c39e996a7c972d2736ee..f933e3bc4708d27dc4fec648561fc1062caaaab7 100644 (file)
--- a/src/cephadm/tests/test_cephadm.py
+++ b/src/cephadm/tests/test_cephadm.py
@@ -329,9 +329,9 @@ class TestCephAdm(object):
     @mock.patch('cephadm.logger')
     @mock.patch('cephadm.fetch_custom_config_files')
     @mock.patch('cephadm.get_container')
-    def test_get_deployment_container(self, _get_container, _get_config, _logger):
+    def test_to_deployment_container(self, _get_container, _get_config, _logger):
         """
-        test get_deployment_container properly makes use of extra container args and custom conf files
+        test to_deployment_container properly makes use of extra container args and custom conf files
         """
 
         ctx = _cephadm.CephadmContext()
@@ -365,7 +365,8 @@ class TestCephAdm(object):
             ptrace=False,
             host_network=True,
         )
-        c = _cephadm.get_deployment_container(ctx, ident)
+        c = _cephadm.get_container(ctx, ident)
+        c = _cephadm.to_deployment_container(ctx, c)
 
         assert '--pids-limit=12345' in c.container_args
         assert '--something' in c.container_args
@@ -380,9 +381,9 @@ class TestCephAdm(object):
     @mock.patch('cephadm.check_unit', lambda *args, **kwargs: (None, 'running', None))
     @mock.patch('cephadm.get_unit_name', lambda *args, **kwargs: 'mon-unit-name')
     @mock.patch('cephadm.extract_uid_gid', lambda *args, **kwargs: (0, 0))
-    @mock.patch('cephadm.get_deployment_container')
+    @mock.patch('cephadm.get_container')
     @mock.patch('cephadm.apply_deploy_config_to_ctx', lambda d, c: None)
-    def test_mon_crush_location(self, _get_deployment_container, _migrate_sysctl, _make_var_run, _deploy_daemon, _file_lock, _logger, monkeypatch):
+    def test_mon_crush_location(self, _get_container, _migrate_sysctl, _make_var_run, _deploy_daemon, _file_lock, _logger, monkeypatch):
         """
         test that crush location for mon is set if it is included in config_json
         """
@@ -390,6 +391,7 @@ class TestCephAdm(object):
         monkeypatch.setattr('cephadmlib.context_getters.fetch_configs', _fetch_configs)
         monkeypatch.setattr('cephadm.fetch_configs', _fetch_configs)
         monkeypatch.setattr('cephadm.read_configuration_source', lambda c: {})
+        monkeypatch.setattr('cephadm.fetch_custom_config_files', mock.MagicMock())
 
         ctx = _cephadm.CephadmContext()
         ctx.name = 'mon.test'
@@ -404,7 +406,7 @@ class TestCephAdm(object):
             'crush_location': 'database=a'
         }
 
-        _get_deployment_container.return_value = _cephadm.CephContainer.for_daemon(
+        _get_container.return_value = _cephadm.CephContainer.for_daemon(
             ctx,
             ident=_cephadm.DaemonIdentity(
                 fsid='9b9d7609-f4d5-4aba-94c8-effa764d96c9',