From 133b23f13e434cf01f59d04d6b83cb03df05b43b Mon Sep 17 00:00:00 2001 From: John Mulligan Date: Sun, 19 Nov 2023 16:06:38 -0500 Subject: [PATCH] cephadm: remove a non-functional line in _rm_cluster Remove a line from _rm_cluster that has no effect. The line uses call_throws to execute an `rm -rf` command. The argument contains asterisk chars that indicate that the file(s)/dir(s) to be created are expected to match the given pattern. However, globs do not work in this context in contemporary versions of cephadm. To double check I added the following temporary unit test: ``` def test_does_it_glob(tmp_path): from cephadmlib.call_wrappers import call_throws d1 = (tmp_path / 'dir1') d1.mkdir() fns = ['f1.txt', 'f2.txt', 'f3.txt', 'f4'] for fn in fns: with open(d1/fn, "w") as fh: fh.write("xxx") assert d1.exists() for fn in fns: assert (d1 / fn).exists() ctx = FakeContext() call_throws(ctx, ['rm', '-rf', f'{d1}/*.txt']) print('files:', os.listdir(d1)) assert d1.exists() for fn in fns: if fn.endswith('.txt'): assert not (d1 / fn).exists() else: assert (d1 / fn).exists() ``` If globs worked in this context this test would have passed. It does not. I confirmed that the current implementation of call/call_wrapper does not execute the command in a shell context. I wondered if it was possible that an earlier version of cephadm did execute this command in a shell context and some changes along the way changed the behavior. I tracked the origin of the line back to 16ebc620349f6e7c9afa6b992c85900f56fcfca3 the first change to implement rm-cluster. In this commit the code uses subprocess.check_output directly. I am familiar with check_output and unless `shell=True` is passed this function doesn't execute the args in a shell context. The `shell=True` argument is not passed to check_output. This means that the very first implementation of this line suffered from the same issue - it would have no effect on any files except one named with actual asterisk (`*`) characters. While I'm sure there was a good intention behind this line, the fact that it persisted in the code so long in a non-functional state and no one noticed in both production and qa testing makes me feel that it can be safely removed with no negative effect. Removing the line simplifies the code and avoids needing to spend effort unit-testing or manually checking a fix. Signed-off-by: John Mulligan --- src/cephadm/cephadm.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/cephadm/cephadm.py b/src/cephadm/cephadm.py index 5d5c2a72eb5a6..23352651f27d2 100755 --- a/src/cephadm/cephadm.py +++ b/src/cephadm/cephadm.py @@ -4331,8 +4331,6 @@ def _rm_cluster(ctx: CephadmContext, keep_logs: bool, zap_osds: bool) -> None: if not keep_logs: # rm logs call_throws(ctx, ['rm', '-rf', ctx.log_dir + '/' + ctx.fsid]) - call_throws(ctx, ['rm', '-rf', ctx.log_dir - + '/*.wants/ceph-%s@*' % ctx.fsid]) # rm logrotate config call_throws(ctx, ['rm', '-f', ctx.logrotate_dir + '/ceph-%s' % ctx.fsid]) -- 2.39.5