mgr/volumes: protection for "fs volume rm" command #30407

joscollin · 2019-09-16T13:27:01Z

Fixes: https://tracker.ceph.com/issues/41841

batrick

Needs a test otherwise LGTM.

joscollin · 2019-09-16T23:06:51Z

Needs a test otherwise LGTM.

It was a quick WIP in between my other tasks. I will update the PR today. Thanks.

joscollin · 2019-09-17T06:51:01Z

@batrick Bugs found are fixed and Updated the tests. Please review again.

joscollin · 2019-09-18T04:29:23Z

http://pulpito.front.sepia.ceph.com/jcollin-2019-09-18_00:16:09-fs-wip-B41841-yes-really-mean-it-distro-basic-smithi/

The tests are passed, but I don't see the _delete_test_volume execution in the passed tests^.

src/pybind/mgr/volumes/fs/volume.py

qa/tasks/cephfs/test_volumes.py

jtlayton · 2019-09-23T11:08:14Z

src/pybind/mgr/volumes/fs/volume.py

+        if confirm is not None and confirm == "--yes-i-really-mean-it":
+            command = {'prefix': 'fs rm', 'fs_name': fs_name, 'yes_i_really_mean_it': True}
+        else:
+            command = {'prefix': 'fs rm', 'fs_name': fs_name, 'yes_i_really_mean_it': False}


Is it correct to check the --yes-i-really-mean-it flag after the previous 'fs fail' mon command? It seems like you might want to check for confirmation before making any changes.

Yes, I think that's better. Do you mean before the command = {'prefix': 'fs fail', 'fs_name': fs_name} line?

Yes, exactly. That means you'll want to do a separate check for it and bail out early instead of just passing the right value to "fs rm".

@jtlayton Fixed now.

joscollin · 2019-09-25T05:23:03Z

http://pulpito.front.sepia.ceph.com/jcollin-2019-09-24_17:28:16-fs-wip-B41841-yes-really-mean-it-distro-basic-smithi/

This test fail because fs volume rm doesn't give a CommandFailedError even if we try to remove the non-existent volume. fs volume rm always returns success. We shouldn't modify the fs volume rm code in favour of the test code. So I think I will skip the removal confirming part in test_volume_rm (check if it's gone). @batrick What do you suggest?

ajarr · 2019-09-26T08:45:24Z

src/pybind/mgr/volumes/module.py

@@ -37,7 +37,8 @@ class Module(orchestrator.OrchestratorClientMixin, MgrModule):
        },
        {
            'cmd': 'fs volume rm '
-                   'name=vol_name,type=CephString',
+                   'name=vol_name,type=CephString '
+                   'name=yes_i_really_mean_it,type=CephString,req=false ',


This is what I see when I check the help,

$ ceph fs volume rm --help fs volume rm <vol_name> {<yes_i_really_mean_it>} Delete a CephFS volume

It's not obvious to me that I've to pass --yes-i-really-mean-it to delete the fs volume. Please include that in the desc below. e.g.,
Delete a FS volume by passing --yes-i-really-mean-it flag.

And prefer that you change the option name to yes-i-really-mean it. This way the help would look like,

fs volume rm <vol_name> {<yes-i-really-mean-it>}

ajarr · 2019-09-26T08:55:02Z

src/pybind/mgr/volumes/fs/volume.py

-    def remove_filesystem(self, fs_name):
+    def remove_filesystem(self, fs_name, confirm):
+        if confirm != "--yes-i-really-mean-it":
+            return -errno.EPERM, "", "this is a DESTRUCTIVE operation and will make " \


s/and/that/

ajarr · 2019-09-26T09:11:00Z

qa/tasks/cephfs/test_volumes.py

+
+                #check if it's gone
+                try:
+                    self._fs_cmd("volume", "rm", self.volname, "--yes-i-really-mean-it")


To check whether volume exists you can do this, https://github.com/ceph/ceph/blob/master/qa/tasks/cephfs/filesystem.py#L601

ajarr · 2019-09-26T09:13:37Z

http://pulpito.front.sepia.ceph.com/jcollin-2019-09-24_17:28:16-fs-wip-B41841-yes-really-mean-it-distro-basic-smithi/

This test fail because fs volume rm doesn't give a CommandFailedError even if we try to remove the non-existent volume. fs volume rm always returns success. We shouldn't modify the fs volume rm code in favour of the test code. So I think I will skip the removal confirming part in test_volume_rm (check if it's gone). @batrick What do you suggest?

I've suggested a better way to check whether a volume exists. Also, we'd want the fs volume rm to fail if the volume doesn't exist so that the command behaves similarly to fs suvolumegroup rm and fs subvolume rm. This can be in a different PR.

joscollin · 2019-09-26T09:32:33Z

Also, we'd want the fs volume rm to fail if the volume doesn't exist so that the command behaves similarly to fs suvolumegroup rm and fs subvolume rm. This can be in a different PR.

I think I will fix that in this PR itself, so that the test function works fine.

ajarr · 2019-09-26T09:41:52Z

qa/tasks/cephfs/test_volumes.py

+            self._fs_cmd("volume", "rm", self.volname)
+        except CommandFailedError as ce:
+            if ce.exitstatus != errno.EPERM:
+                raise


Please raise with a useful message, something like, we expected the fs volume rm command to fail with EPERM

joscollin · 2019-09-27T05:38:49Z

@ajarr Fixed all the comments from #30407 (review) till the end, as discussed yesterday. Please verify them.

The teuthology tests are running. I will post it when passed.

ajarr · 2019-09-27T06:31:16Z

qa/tasks/cephfs/test_volumes.py

+                try:
+                    self._fs_cmd("volume", "rm", self.volname, "--yes-i-really-mean-it")
+                except CommandFailedError as ce:
+                    #re-raise EPERM


why do you need a try catch block here? You're just re-raising the exception You're not handling the exception in any special way.

We expect the volume rm <self.volname> --yes-i-really-mean-it to succeed here. If it succeeds we don't re-raise. But if it fails, then it should mostly be a permission issue. May be I could give a check if ce.exitstatus == errno.EPERM then raise

You don't need a try catch block here. If the command fails, let it fail.

ajarr · 2019-09-27T06:40:00Z

qa/tasks/cephfs/test_volumes.py

+            self._fs_cmd("volume", "rm", self.volname)
+        except CommandFailedError as ce:
+            if ce.exitstatus != errno.EPERM:
+                raise RuntimeError("expected the 'fs volume rm' command to fail with EPERM, " \


You don't need a \ to break a long line when you're enclosing the string within brackets and using quotes?

Right, I will update it.

ajarr · 2019-09-27T06:55:57Z

src/pybind/mgr/volumes/fs/volume.py

+        if confirm != "--yes-i-really-mean-it":
+            return -errno.EPERM, "", "this is a DESTRUCTIVE operation that will make " \
+                "data in your filesystem permanently inaccessible. " \
+                "Add --yes-i-really-mean-it if you are sure you wish to continue."


I don't find this instruction clear. Add '--yes-i-really-meant' where? The user has to re-issue the command and append the flag, right?

e.g.,

See the stderr message when you try to delete a RADOS pool.

$ ceph osd pool delete cephfs.a.data Error EPERM: WARNING: this will *PERMANENTLY DESTROY* all data stored in pool cephfs.a.data. If you are *ABSOLUTELY CERTAIN* that is what you want, pass the pool name *twice*, followed by --yes-i-really-really-mean-it.

So maybe something like,

Error EPERM: WARNING: this will *PERMANENTLY DESTROY* all data stored in the filesystem <fs-name>. If you are *ABSOLUTELY CERTAIN* that is what you want, re-issue the command followed by --yes-i-really-mean-it.

Fixes: https://tracker.ceph.com/issues/41841 Signed-off-by: Jos Collin <jcollin@redhat.com>

Fixes: ceph#30381 (comment) Signed-off-by: Jos Collin <jcollin@redhat.com>

joscollin · 2019-09-27T09:52:53Z

@ajarr Fixed

joscollin · 2019-09-27T23:16:15Z

http://pulpito.front.sepia.ceph.com/jcollin-2019-09-27_15:47:30-fs-wip-B41841-yes-really-mean-it-distro-basic-smithi/

ajarr

LGTM

ajarr · 2019-09-30T11:21:12Z

http://pulpito.front.sepia.ceph.com/jcollin-2019-09-27_15:47:30-fs-wip-B41841-yes-really-mean-it-distro-basic-smithi/

Jos, are you able to access this link? I can't. Did the tests pass?

joscollin · 2019-09-30T11:26:04Z

http://pulpito.front.sepia.ceph.com/jcollin-2019-09-27_15:47:30-fs-wip-B41841-yes-really-mean-it-distro-basic-smithi/

Jos, are you able to access this link? I can't. Did the tests pass?

The tests are passed. Please check teuthology if you are not able to access the url.

jcollin@teuthology:/a$ cd /a/jcollin-2019-09-27_15\:47\:30-fs-wip-B41841-yes-really-mean-it-distro-basic-smithi/
jcollin@teuthology:/a/jcollin-2019-09-27_15:47:30-fs-wip-B41841-yes-really-mean-it-distro-basic-smithi$ ls
4339473  4339474  coverage.log  results.log

Those^ are the jobs.

The comments have been resolved.

joscollin added bug-fix cephfs Ceph File System labels Sep 16, 2019

batrick requested changes Sep 16, 2019

View reviewed changes

joscollin force-pushed the wip-B41841-yes-really-mean-it branch from a7f7601 to 6f7cc09 Compare September 17, 2019 06:47

joscollin changed the title ~~[wip] mgr/volumes: protection for fs volume rm command~~ mgr/volumes: protection for fs volume rm command Sep 17, 2019

joscollin added the needs-review label Sep 17, 2019

joscollin requested review from batrick and ajarr September 17, 2019 06:51

joscollin added the wip-jcollin-testing label Sep 17, 2019

ajarr mentioned this pull request Sep 20, 2019

doc: protection for fs volume rm command #30469

Closed

joscollin force-pushed the wip-B41841-yes-really-mean-it branch 2 times, most recently from 772b982 to 59ceac8 Compare September 20, 2019 09:37

joscollin requested a review from jtlayton September 21, 2019 13:21

batrick previously requested changes Sep 23, 2019

View reviewed changes

src/pybind/mgr/volumes/fs/volume.py Outdated Show resolved Hide resolved

qa/tasks/cephfs/test_volumes.py Outdated Show resolved Hide resolved

jtlayton reviewed Sep 23, 2019

View reviewed changes

joscollin force-pushed the wip-B41841-yes-really-mean-it branch 2 times, most recently from 52c6372 to e982223 Compare September 24, 2019 12:40

ajarr reviewed Sep 26, 2019

View reviewed changes

joscollin force-pushed the wip-B41841-yes-really-mean-it branch 3 times, most recently from 3ccaee7 to 7844a5e Compare September 27, 2019 04:14

ajarr reviewed Sep 27, 2019

View reviewed changes

joscollin added 3 commits September 27, 2019 15:17

mgr/volumes: protection for 'fs volume rm' command

59cf9be

Fixes: https://tracker.ceph.com/issues/41841 Signed-off-by: Jos Collin <jcollin@redhat.com>

qa/tasks: add/update tests for --yes-i-really-mean-it

f1b9e90

Fixes: https://tracker.ceph.com/issues/41841 Signed-off-by: Jos Collin <jcollin@redhat.com>

doc: protection for 'fs volume rm' command

a58a6cd

Fixes: ceph#30381 (comment) Signed-off-by: Jos Collin <jcollin@redhat.com>

joscollin force-pushed the wip-B41841-yes-really-mean-it branch from 7844a5e to a58a6cd Compare September 27, 2019 09:49

ajarr approved these changes Sep 30, 2019

View reviewed changes

ajarr requested a review from batrick September 30, 2019 11:17

ajarr merged commit 0bfcace into ceph:master Oct 1, 2019

joscollin deleted the wip-B41841-yes-really-mean-it branch October 1, 2019 08:19

smithfarm changed the title ~~mgr/volumes: protection for fs volume rm command~~ mgr/volumes: protection for "fs volume rm" command Oct 7, 2019

smithfarm mentioned this pull request Oct 7, 2019

nautilus: mgr/volumes: protection for "fs volume rm" command #30768

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgr/volumes: protection for "fs volume rm" command #30407

mgr/volumes: protection for "fs volume rm" command #30407

joscollin commented Sep 16, 2019

batrick left a comment

joscollin commented Sep 16, 2019

joscollin commented Sep 17, 2019

joscollin commented Sep 18, 2019

jtlayton Sep 23, 2019

joscollin Sep 23, 2019

jtlayton Sep 23, 2019 •

edited

joscollin Sep 24, 2019

joscollin commented Sep 25, 2019 •

edited

ajarr Sep 26, 2019 •

edited

ajarr Sep 26, 2019

ajarr Sep 26, 2019

ajarr Sep 26, 2019 •

edited

ajarr commented Sep 26, 2019 •

edited

joscollin commented Sep 26, 2019

ajarr Sep 26, 2019 •

edited

joscollin commented Sep 27, 2019

ajarr Sep 27, 2019 •

edited

joscollin Sep 27, 2019

ajarr Sep 27, 2019

ajarr Sep 27, 2019

joscollin Sep 27, 2019

ajarr Sep 27, 2019

joscollin commented Sep 27, 2019

joscollin commented Sep 27, 2019

ajarr left a comment

ajarr commented Sep 30, 2019

joscollin commented Sep 30, 2019

mgr/volumes: protection for "fs volume rm" command #30407

mgr/volumes: protection for "fs volume rm" command #30407

Conversation

joscollin commented Sep 16, 2019

batrick left a comment

Choose a reason for hiding this comment

joscollin commented Sep 16, 2019

joscollin commented Sep 17, 2019

joscollin commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtlayton Sep 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joscollin commented Sep 25, 2019 • edited

ajarr Sep 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajarr Sep 26, 2019 • edited

Choose a reason for hiding this comment

ajarr commented Sep 26, 2019 • edited

joscollin commented Sep 26, 2019

ajarr Sep 26, 2019 • edited

Choose a reason for hiding this comment

joscollin commented Sep 27, 2019

ajarr Sep 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joscollin commented Sep 27, 2019

joscollin commented Sep 27, 2019

ajarr left a comment

Choose a reason for hiding this comment

ajarr commented Sep 30, 2019

joscollin commented Sep 30, 2019

jtlayton Sep 23, 2019 •

edited

joscollin commented Sep 25, 2019 •

edited

ajarr Sep 26, 2019 •

edited

ajarr Sep 26, 2019 •

edited

ajarr commented Sep 26, 2019 •

edited

ajarr Sep 26, 2019 •

edited

ajarr Sep 27, 2019 •

edited