]> git.apps.os.sepia.ceph.com Git - ceph.git/commitdiff
cephadm: remove a non-functional line in _rm_cluster
authorJohn Mulligan <jmulligan@redhat.com>
Sun, 19 Nov 2023 21:06:38 +0000 (16:06 -0500)
committerJohn Mulligan <jmulligan@redhat.com>
Tue, 2 Jan 2024 14:30:21 +0000 (09:30 -0500)
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 <jmulligan@redhat.com>
src/cephadm/cephadm.py

index 5d5c2a72eb5a6034981f17947974188797e63a44..23352651f27d2e6ffd6facbb838b7218a1a15ef5 100755 (executable)
@@ -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])