New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ceph-disk: do not remove mount point if deactive --once #16474

Merged
merged 1 commit into from Aug 2, 2017

Conversation

Projects
None yet
2 participants
@shun-s
Contributor

shun-s commented Jul 21, 2017

ceph-disk: avoid remove '/var/lib/ceph/osd/ceph-xx' directory when when deactivate with --once

when using deactivate with --once, '/var/lib/ceph/osd/ceph-xx' will be
deleted by call ** def unmount of ceph-disk**, resulting next activate failed.

that's not what --once exactly wants. this pr fix this problem

@shun-s shun-s changed the title from [WIP] fix ceph-disk deactivated to [WIP] fix unexpectly delete /var/lib/ceph/xx when deactivated with --once Jul 21, 2017

@tchaikov tchaikov self-requested a review Jul 21, 2017

@shun-s

This comment has been minimized.

Contributor

shun-s commented Jul 22, 2017

hi, tchaikov @tchaikov
looking at the last two jenkins console output of unittest, these failure seems no relations with this pr.
so i rebase master again to pass the unit test.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 24, 2017

retest this please

@shun-s

This comment has been minimized.

Contributor

shun-s commented Jul 24, 2017

ping @tchaikov , please take a review, thanks

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 24, 2017

@shun-s

This comment has been minimized.

Contributor

shun-s commented Jul 25, 2017

just update signed-off from shun-s to Song Shun

@shun-s

This comment has been minimized.

Contributor

shun-s commented Jul 25, 2017

retest this please

@@ -1448,6 +1448,7 @@ def mount(
def unmount(
path,
dont_rm=False,

This comment has been minimized.

@tchaikov

tchaikov Jul 26, 2017

Contributor

@shun-s i'd suggest rename this parameter to do_rm, it would be easier to understand if we could use positive logic.

This comment has been minimized.

@shun-s

shun-s Jul 27, 2017

Contributor

nice, already done

[root@ceph-ci ceph-disk]# git diff
diff --git a/src/ceph-disk/ceph_disk/main.py b/src/ceph-disk/ceph_disk/main.py
index 785ff8d..bf46e5e 100755
--- a/src/ceph-disk/ceph_disk/main.py
+++ b/src/ceph-disk/ceph_disk/main.py
@@ -1448,7 +1448,7 @@ def mount(
‘’‘
def unmount(
path,

-    dont_rm=False,
+    do_rm=True,
 ):

@@ -1472,7 +1472,7 @@ def unmount(
else:
time.sleep(0.5 + retries * 1.0)
retries += 1

-    if dont_rm:
+    if not do_rm:
         return
     os.rmdir(path)
@@ -3940,7 +3942,7 @@ def main_deactivate_locked(args):
with open(os.path.join(mounted_path, 'deactive'), 'w'):
path_set_context(os.path.join(mounted_path, 'deactive'))
unmount(mounted_path)
unmount(mounted_path, args.once)

This comment has been minimized.

@tchaikov

tchaikov Jul 26, 2017

Contributor

and pass the parameter by name. like

unmount(mounted_path, do_rm=not args.once)

This comment has been minimized.

@shun-s

shun-s Jul 27, 2017

Contributor

done

@@ -3942,7 +3942,7 @@ def main_deactivate_locked(args):
with open(os.path.join(mounted_path, 'deactive'), 'w'):
path_set_context(os.path.join(mounted_path, 'deactive'))

-    unmount(mounted_path, args.once)
+    unmount(mounted_path, do_rm=not args.once)
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 26, 2017

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes. in this case, it would be "ceph-disk:"

@tchaikov tchaikov changed the title from [WIP] fix unexpectly delete /var/lib/ceph/xx when deactivated with --once to ceph-disk: do not remove mount point if deactive --once Jul 26, 2017

@tchaikov tchaikov added the bug fix label Jul 26, 2017

@shun-s

This comment has been minimized.

Contributor

shun-s commented Jul 27, 2017

ping @tchaikov , also update test_main.py

diff --git a/src/ceph-disk/tests/test_main.py b/src/ceph-disk/tests/test_main.py
index 6db187d..c5ecaf8 100644
@@ -812,7 +812,7 @@ class TestCephDiskDeactivateAndDestroy(unittest.TestCase):
-                unmount=lambda path, False: True,
+                unmount=lambda path, True: True,
@@ -846,7 +846,7 @@ class TestCephDiskDeactivateAndDestroy(unittest.TestCase):
-                unmount=lambda path, False: True,
+                unmount=lambda path, True: True,
@shun-s

This comment has been minimized.

Contributor

shun-s commented Jul 27, 2017

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes. in this case, it would be "ceph-disk:"

update title with prefix ceph-disk 
@@ -812,7 +812,7 @@ def test_main_deactivate(self, mock_open):
stop_daemon=lambda cluster, osd_id: True,
_remove_osd_directory_files=lambda path, cluster: True,
path_set_context=lambda path: True,
unmount=lambda path: True,
unmount=lambda path, True: True,

This comment has been minimized.

@tchaikov

tchaikov Jul 27, 2017

Contributor

please squash this commit into the previous one.

This comment has been minimized.

@shun-s

shun-s Jul 27, 2017

Contributor

done

This comment has been minimized.

@tchaikov

tchaikov Jul 27, 2017

Contributor

should be

unmount=lambda path, do_rm: True,

This comment has been minimized.

@shun-s

shun-s Jul 28, 2017

Contributor

done

@@ -812,7 +812,7 @@ def test_main_deactivate(self, mock_open):
stop_daemon=lambda cluster, osd_id: True,
_remove_osd_directory_files=lambda path, cluster: True,
path_set_context=lambda path: True,
unmount=lambda path: True,
unmount=lambda path, True: True,

This comment has been minimized.

@tchaikov

tchaikov Jul 27, 2017

Contributor

should be

unmount=lambda path, do_rm: True,
ceph-disk: avoid remove '/var/lib/ceph/osd/ceph-xx' directory when de…
…active with --once

  when using deactivate with --once, '/var/lib/ceph/osd/ceph-xx' will be deleted by call ** def unmount of ceph-disk**,
  resulting next activate failed. that's not what --once exactly wants.

Signed-off-by: Song Shun  <song.shun3@zte.com.cn>
@shun-s

This comment has been minimized.

Contributor

shun-s commented Jul 28, 2017

ping @tchaikov
done with changing True to do_rm

diff --git a/src/ceph-disk/tests/test_main.py b/src/ceph-disk/tests/test_main.py
index ef6d7d4..57e4af2 100644
--- a/src/ceph-disk/tests/test_main.py
+++ b/src/ceph-disk/tests/test_main.py
@@ -812,7 +812,7 @@ class TestCephDiskDeactivateAndDestroy(unittest.TestCase):

-                unmount=lambda path, True: True,
+                unmount=lambda path, do_rm: True,
                 dmcrypt_unmap=lambda part_uuid: True,

@@ -846,7 +846,7 @@ class TestCephDiskDeactivateAndDestroy(unittest.TestCase):

-                unmount=lambda path, True: True,
+                unmount=lambda path, do_rm: True,
                 dmcrypt_unmap=lambda part_uuid: True,

@tchaikov tchaikov added the needs-qa label Jul 28, 2017

@tchaikov

This comment has been minimized.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Aug 1, 2017

the test failures are addressed by d8f59cd in #16722

@tchaikov tchaikov merged commit 9d4e7ec into ceph:master Aug 2, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@shun-s shun-s deleted the shun-s:wip-fix-deactive branch Nov 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment