cephadm: added remove-file command for client file cleanup

Add a cephadm remove-file subcommand that deletes only regular files.
Have the mgr call it from CephadmServe._write_client_files when pruning
stale client keyrings instead of running ssh rm, so removal goes through
the same cephadm/invoker path as other host operations.

Fixes: https://tracker.ceph.com/issues/74045
Signed-off-by: Shweta Bhosale <Shweta.Bhosale1@ibm.com>

diff --git a/doc/man/8/cephadm.rst b/doc/man/8/cephadm.rst
index c290bf7712d8bc53f35653bb96ad3d4ec15fdbf2..1d352ded429accc7581d5495eb1eb87075aa11e9 100644 (file)
--- a/doc/man/8/cephadm.rst
+++ b/doc/man/8/cephadm.rst
@@ -13,7 +13,7 @@ Synopsis
 |             [--log-dir LOG_DIR] [--logrotate-dir LOGROTATE_DIR]
 |             [--unit-dir UNIT_DIR] [--verbose] [--timeout TIMEOUT]
 |             [--retry RETRY] [--no-container-init]
-|             {version,pull,inspect-image,ls,list-networks,list-rdma,adopt,rm-daemon,rm-cluster,run,shell,enter,ceph-volume,unit,logs,bootstrap,deploy,check-host,check-online,prepare-host,prepare-host-sudo-hardening,setup-ssh-user,add-repo,rm-repo,install,list-images,update-osd-service}
+|             {version,pull,inspect-image,ls,list-networks,list-rdma,adopt,rm-daemon,rm-cluster,remove-file,run,shell,enter,ceph-volume,unit,logs,bootstrap,deploy,check-host,check-online,prepare-host,prepare-host-sudo-hardening,setup-ssh-user,add-repo,rm-repo,install,list-images,update-osd-service}
 |               ...
 
 
@@ -92,6 +92,8 @@ Synopsis
 
 | **cephadm** **check-online**
 
+| **cephadm** **remove-file** [-h] [--fsid FSID] --path PATH
+
 | **cephadm** **prepare-host**
 
 | **cephadm** **add-repo** [-h] [--release RELEASE] [--version VERSION]
@@ -300,6 +302,19 @@ This command is primarily intended for cephadm internals (for example, the
 offline host watcher), rather than direct operator workflows.
 
 
+remove-file
+-----------
+
+Remove a regular file on the local host. Missing paths are ignored.
+Refuses directories and symbolic links, only plain files are
+removed.
+
+Arguments:
+
+* [--fsid FSID]   cluster FSID (passed automatically when invoked by the orchestrator)
+* --path PATH     absolute path of the file to remove (required)
+
+
 deploy
 ------
 

diff --git a/src/cephadm/cephadm.py b/src/cephadm/cephadm.py
index 2365054f42dd4a9d2ee82f7a8a116ee9a57ed730..d49e96ea45be0fa3afa3b549ba24216af3e830f8 100755 (executable)
--- a/src/cephadm/cephadm.py
+++ b/src/cephadm/cephadm.py
@@ -4643,6 +4643,29 @@ def command_gather_facts(ctx: CephadmContext) -> None:
     print(host.dump())
 
 
+def command_remove_file(ctx: CephadmContext) -> int:
+    """Remove a regular file on the host
+    """
+    norm = Path(os.path.normpath(str(Path(ctx.remove_file_path).expanduser())))
+
+    if not norm.is_absolute():
+        raise Error(f'Can not remove non-absolute path: {norm}')
+    try:
+        if not norm.exists():
+            return 0
+        # Refuse symlinks explicitly because is_file() follows them
+        if norm.is_symlink() or not norm.is_file():
+            raise Error(f'Can not remove non-regular file: {norm}')
+
+        norm.unlink()
+
+    except FileNotFoundError:
+        return 0
+    except OSError as e:
+        raise Error(f'failed to remove {norm}: {e}')
+    return 0
+
+
 ##################################
 
 
@@ -5697,6 +5720,18 @@ def _get_parser():
         'gather-facts', help='gather and return host related information (JSON format)')
     parser_gather_facts.set_defaults(func=command_gather_facts)
 
+    parser_remove_file = subparsers.add_parser(
+        'remove-file', help='remove a file on the host')
+    parser_remove_file.set_defaults(func=command_remove_file)
+    parser_remove_file.add_argument(
+        '--fsid',
+        help='cluster FSID')
+    parser_remove_file.add_argument(
+        '--path',
+        required=True,
+        dest='remove_file_path',
+        help='absolute path of the file to remove')
+
     parser_maintenance = subparsers.add_parser(
         'host-maintenance', help='Manage the maintenance state of a host')
     parser_maintenance.add_argument(
@@ -5845,7 +5880,7 @@ def main() -> None:
                     command_add_repo,
                     command_rm_repo,
                     command_install,
-                    command_bootstrap
+                    command_bootstrap,
                 ]:
             check_container_engine(ctx)
         # command handler

diff --git a/src/cephadm/tests/test_cephadm.py b/src/cephadm/tests/test_cephadm.py
index 7d2402d8b9b30ed0f0001f5353638d4dd2af023a..efba1cf777ebeda61bbfd26b0cc406e78e69af89 100644 (file)
--- a/src/cephadm/tests/test_cephadm.py
+++ b/src/cephadm/tests/test_cephadm.py
@@ -106,6 +106,44 @@ class TestCephAdm(object):
         _attempt_bind.side_effect = os_error
         assert port_in_use(empty_ctx, _cephadm.EndPoint('0.0.0.0', 9100)) == False
 
+    def test_command_remove_file(self, cephadm_fs):
+        rm_path = '/tmp/cephadm-remove-file-test'
+        cephadm_fs.create_file(rm_path, contents='x')
+        with with_cephadm_ctx(
+            ['remove-file', '--fsid', '00000000-0000-0000-0000-0000deadbeef', '--path', rm_path]
+        ) as ctx:
+            assert _cephadm.command_remove_file(ctx) == 0
+        assert not cephadm_fs.exists(rm_path)
+
+    def test_command_remove_file_missing_ok(self, cephadm_fs):
+        missing = '/tmp/cephadm-remove-file-missing'
+        with with_cephadm_ctx(
+            ['remove-file', '--fsid', '00000000-0000-0000-0000-0000deadbeef', '--path', missing]
+        ) as ctx:
+            assert _cephadm.command_remove_file(ctx) == 0
+
+    def test_command_remove_file_refuses_directory(self, cephadm_fs):
+        dpath = '/tmp/cephadm-remove-file-isdir'
+        cephadm_fs.create_dir(dpath)
+        with with_cephadm_ctx(
+            ['remove-file', '--fsid', '00000000-0000-0000-0000-0000deadbeef', '--path', dpath]
+        ) as ctx:
+            with pytest.raises(_cephadm.Error, match='Can not remove non-regular file'):
+                _cephadm.command_remove_file(ctx)
+        assert cephadm_fs.exists(dpath)
+
+    def test_command_remove_file_refuses_symlink(self, cephadm_fs):
+        target = '/tmp/cephadm-remove-file-symtarget'
+        link = '/tmp/cephadm-remove-file-symlink'
+        cephadm_fs.create_file(target, contents='x')
+        cephadm_fs.create_symlink(link, target)
+        with with_cephadm_ctx(
+            ['remove-file', '--fsid', '00000000-0000-0000-0000-0000deadbeef', '--path', link]
+        ) as ctx:
+            with pytest.raises(_cephadm.Error, match='Can not remove non-regular file'):
+                _cephadm.command_remove_file(ctx)
+        assert cephadm_fs.exists(link)
+
     @mock.patch('cephadm.socket.socket.bind')
     @mock.patch('cephadm.logger')
     def test_port_in_use_special_cases(self, _logger, _bind):

diff --git a/src/pybind/mgr/cephadm/serve.py b/src/pybind/mgr/cephadm/serve.py
index e45ec846df2130db0b289505cceedda7da992332..7d9922607171cf76b08941b928cc0def98d7ba71 100644 (file)
--- a/src/pybind/mgr/cephadm/serve.py
+++ b/src/pybind/mgr/cephadm/serve.py
@@ -1399,8 +1399,9 @@ class CephadmServe:
             if path == '/etc/ceph/ceph.conf':
                 continue
             self.log.info(f'Removing {host}:{path}')
-            cmd = ssh.RemoteCommand(ssh.Executables.RM, ['-f', path])
-            self.mgr.ssh.check_execute_command(host, cmd)
+            with self.mgr.async_timeout_handler(host, f'cephadm remove-file ({path})'):
+                self.mgr.wait_async(self._run_cephadm(
+                    host, cephadmNoImage, 'remove-file', ['--path', path]))
             updated_files = True
             self.mgr.cache.removed_client_file(host, path)
         if updated_files:

diff --git a/src/pybind/mgr/cephadm/tests/test_cephadm.py b/src/pybind/mgr/cephadm/tests/test_cephadm.py
index b97b7f175f22c76dccc9c5f29101aff676b6130d..bb5933152a7ea683cd88c4fedb610a10631ecd88 100644 (file)
--- a/src/pybind/mgr/cephadm/tests/test_cephadm.py
+++ b/src/pybind/mgr/cephadm/tests/test_cephadm.py
@@ -15,7 +15,7 @@ from cephadm.inventory import (
 )
 from cephadm.services.osd import OSD, OSDRemovalQueue, OsdIdClaims
 from cephadm.services.nvmeof import NvmeofService
-from cephadm.utils import SpecialHostLabels
+from cephadm.utils import SpecialHostLabels, cephadmNoImage
 
 try:
     from typing import List
@@ -2346,6 +2346,20 @@ class TestCephadm(object):
         CephadmServe(cephadm_module)._write_client_files({}, 'host2')
         CephadmServe(cephadm_module)._write_client_files({}, 'host3')
 
+    @mock.patch('cephadm.serve.CephadmServe._run_cephadm', new_callable=mock.AsyncMock)
+    def test_write_client_files_remove_calls_cephadm_remove_file(self, _run_cephadm, cephadm_module):
+        _run_cephadm.return_value = ([''], [''], 0)
+        cephadm_module.inventory.add_host(HostSpec('host1', '10.0.0.1'))
+        cephadm_module.cache.prime_empty_host('host1')
+        stale = '/var/lib/ceph/fsid/config/foo.keyring'
+        cephadm_module.cache.update_client_file('host1', stale, 'digest', 0o600, 0, 0)
+        CephadmServe(cephadm_module)._write_client_files({'host1': {}}, 'host1')
+        _run_cephadm.assert_called_once()
+        pos_args = _run_cephadm.call_args[0]
+        # Bound method mock: call_args do not include self.
+        assert pos_args[0:4] == ('host1', cephadmNoImage, 'remove-file', ['--path', stale])
+        assert stale not in cephadm_module.cache.get_host_client_files('host1')
+
     @mock.patch('cephadm.CephadmOrchestrator.mon_command')
     @mock.patch("cephadm.inventory.HostCache.get_host_client_files")
     def test_dont_write_etc_ceph_client_files_when_turned_off(self, _get_client_files, _mon_command, cephadm_module):