ceph-disk: Add fix subcommand #13310
Conversation
b-ranto
force-pushed
the
wip-ceph-disk-fix
branch
from
ae2df10
to
8c86cba
Compare
src/ceph-disk/ceph_disk/main.py
Outdated
| formatter_class=argparse.RawDescriptionHelpFormatter, | ||
| description=textwrap.fill(textwrap.dedent("""\ | ||
| """)), | ||
| help='fix SELinux labells and/or file permissions') |
src/ceph-disk/ceph_disk/main.py
Outdated
| '+', | ||
| '-exec', | ||
| 'restorecon', | ||
| '{}', |
|
also need to update the ceph-disk manpage. |
b-ranto
force-pushed
the
wip-ceph-disk-fix
branch
from
8c86cba
to
7433c5f
Compare
|
@dachary Hmm, I am thinking there is not much to test in the unit/integration tests. The function just calls few shell commands and it only does so if SELinux is enabled and the ceph daemons are down. I don't think we want to enable SELinux/stop daemons in the unit/integration tests, right? I think this sounds like more of a job for teuthology to test this. |
|
qa/workunits/ceph-disk/ is a teuthology job, that's what I meant by "integration tests". Testing the various error cases to verify they are handled properly may be very tedious with integration tests though and it's probably enough to write unit tests to make sure they do not fail because of a typo when we need them the most ;-) |
b-ranto
force-pushed
the
wip-ceph-disk-fix
branch
from
6c41268
to
2f1a9e2
Compare
|
OK so I have pushed a patch that adds a unit test support. Weirdly, while testing this locally I had to use class TestCephDiskDeactivateAndDestroy instead of TestCephDisk which inherits only from object and its tests are not run. Also, I do have a bit of a question about this. The test only works when it is run as root (we are doing chown and restorecon which require root) and in order to simulate it, I use a tmp directory. The test will fail when run as a regular user (not enough rights) but works fine when run as root/with sudo. Is this ok? Can I expect these to be run as root? It looks like my test is the only one requiring it at the moment (at least from those run by pytest). |
b-ranto
force-pushed
the
wip-ceph-disk-fix
branch
2 times, most recently
from
b4a872e
to
7a9a351
Compare
|
Nevermind, I found a better way to unit test this by overriding the command* functions. This no longer needs root nor does it do any permanent changes to the system but it still exercises the python code. |
src/ceph-disk/ceph_disk/main.py
Outdated
| if args.all: | ||
| for directory, uid, gid, blocking, recursive in fix_table: | ||
| c = [ | ||
| 'find', |
There was a problem hiding this comment.
@b-ranto , could you fix the errors reported by flake8?
see https://jenkins.ceph.com/job/ceph-pull-requests/18221/consoleFull#1877343199bf3c8a86-8876-45aa-9016-a5bacffc83b0
There was a problem hiding this comment.
@tchaikov sure, the flake8 errors should be fixed, now
b-ranto
force-pushed
the
wip-ceph-disk-fix
branch
from
7a9a351
to
9ae468d
Compare
|
@b-ranto you need to write a test that asserts it does the right thing. |
b-ranto
force-pushed
the
wip-ceph-disk-fix
branch
from
9ae468d
to
c827775
Compare
|
@dachary I've added few asserts |
|
@tchaikov after fixing the indentation errors, the jenkins build/test job works fine |
|
It turns out, I forgot to add few more directories. We need to fix /var/log/ceph, /var/run/ceph and /etc/ceph as well. I fixed that in the latest commit. Can you think of any other files/directories that need to be fixed? |
b-ranto
force-pushed
the
wip-ceph-disk-fix
branch
from
0fd9947
to
d1dea1c
Compare
|
@tchaikov there are no integration tests for this change therefore it won't help to run the ceph-disk suite. |
This subcommand will fix the SELinux labels and/or file permissions on ceph data (/var/lib/ceph). The command is also optimized to run the commands in parallel (per sub-dir in /var/lib/ceph) and do restorecon and chown at the same time to take advantage of the caching mechanisms. Signed-off-by: Boris Ranto <branto@redhat.com>
This will simulate the command* functions to not actually run anything thus excercising the python code directly. It also checks that the proper (sub-strings) are in the output. Signed-off-by: Boris Ranto <branto@redhat.com>
It turns out I forgot several more directories that needs to be fixed by this script. We need to fix /var/log/ceph, /var/run/ceph and /etc/ceph as well. Signed-off-by: Boris Ranto <branto@redhat.com>
This adds the ability to restore the labels of the underlying system data in addition to ceph data. Signed-off-by: Boris Ranto <branto@redhat.com>
b-ranto
force-pushed
the
wip-ceph-disk-fix
branch
from
d1dea1c
to
8d81af4
Compare
|
@tchaikov rebased |
|
@tchaikov Would you please take a look at this? |
ghost
merged commit 
This subcommand will fix the SELinux labels and/or file permissions on
ceph data (/var/lib/ceph).
The command is also optimized to run the commands in parallel (per
sub-dir in /var/lib/ceph) and do restorecon and chown at the same time
to take advantage of the caching mechanisms.
Signed-off-by: Boris Ranto branto@redhat.com