Skip to content
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

mgr / volumes: background purge queue for subvolumes #28003

Merged
merged 14 commits into from Jul 16, 2019

Conversation

@vshankar
Copy link
Contributor

commented May 7, 2019

Standalone mixin classes to support background purge operation
for deleted subvolumes (either by assigning a thread per subvol
group or having a pool of worker threads for all subvolumes).

The usage of subvolume group (and its association with the
classes) is kept very loose, especially when a dedicated
worker thread is assigned to a subvolume group. Worker threads
get created when a subvolume belonging to a particular subvolume
group is added to the purge queue.

Signed-off-by: Venky Shankar vshankar@redhat.com

@vshankar vshankar added the cephfs label May 7, 2019

@vshankar vshankar requested a review from batrick May 7, 2019

@vshankar vshankar requested a review from ajarr May 7, 2019

@vshankar

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Note that changes are required in PR #27594 -- I'll post that as a separate PR.

@vshankar vshankar force-pushed the vshankar:wip-subvol-purge-queue branch from 52b98a0 to 0b478f4 May 28, 2019

@vshankar vshankar changed the title [WIP] fs: background purge queue for fs subvolumes mgr / volumes: background purge queue for subvolumes May 28, 2019

@vshankar vshankar marked this pull request as ready for review May 28, 2019

@@ -118,6 +119,7 @@ class VolumeClient(object):
def __init__(self, mgr):
self.mgr = mgr
self.connection_pool = ConnectionPool(self.mgr.rados)
self.purge_queue = ThreadPoolPurgeQueueMixin(self, 4)

This comment has been minimized.

Copy link
@vshankar

vshankar May 28, 2019

Author Contributor

note that thread count is hardcoded as of now -- this needs to be configurable.

@vshankar

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

this is based on top of PR #28082 w/ background purge integrated with [sub]volume module.

@vshankar vshankar requested a review from sebastian-philipp May 28, 2019

@vshankar vshankar force-pushed the vshankar:wip-subvol-purge-queue branch from 0b478f4 to 0259c09 May 28, 2019

@vshankar vshankar force-pushed the vshankar:wip-subvol-purge-queue branch from 0259c09 to 46f52bb May 29, 2019

@vshankar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@ajarr commit c1d7733 ("mgr / volumes: maintain connection pool for fs volumes") in this pr touches changes introduced by the pr #28082 -- that's why I include those commits here..

@vshankar vshankar force-pushed the vshankar:wip-subvol-purge-queue branch 4 times, most recently from 0b645b3 to 4a952ed Jun 10, 2019

@vshankar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@ajarr @batrick -- please review.

I had to introduce some additional changes in the connection pool logic to make this work with #27856. The reason being that since the (now python) tests are executed as teuthology tasks, I see teuthology (at least with vstart_runner) removing (and recreating) filesystem at times in-between executing tests. This was causing the test to hang as the cached filesystem handle (from the pool) is invalid since the filesystem got removed behind the scene. I added a workaround for this by checking the filesystem id (cached vs what's in fs map), and reconnect to the filesystem if required. #27856 passes with these changes.

@batrick

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

On that note, @ajarr @vshankar do we have logic in the mgr/volumes plugin to handle a failed rank or a deleted fs?

@vshankar vshankar force-pushed the vshankar:wip-subvol-purge-queue branch from 4a952ed to 837e075 Jun 11, 2019

@ktdreyer

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@batrick I think we wanted to cherry-pick this for nautilus for Ceph CSI. I don't see a tracker.ceph.com ticket for that - want to file one?

@batrick

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

needs rebase

also @vshankar please make a ticket for this feature so it can be backproted.

@vshankar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

http://tracker.ceph.com/issues/40036 -- will include in commit message.

@ajarr

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@vshankar , please update the PR whenever you can

@vshankar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@vshankar , please update the PR whenever you can

I was waiting for your review comments so I can address everything...

vshankar added 4 commits Jun 20, 2019
test: add basic purge queue validation test
.. and since we have async subvolume deletes now, check
trash directory for emptiness in other tests.

Fixes: http://tracker.ceph.com/issues/40036
Signed-off-by: Venky Shankar <vshankar@redhat.com>
mgr / volumes: use negative error codes everywhere
cephfs python binding returns positive error code. mgr/volumes
incorrectly does error code checks assuming the error codes to
be negative.

this was not an issue till now since mgr/volumes mostly does a
`raise VolumeException()` for the most part followed by the exception
being displayed to the operator (one exception is catching cephfs
ObjectNotFound error, in which case -errno.ENOENT is returned
(and checked whereever required)).

Signed-off-by: Venky Shankar <vshankar@redhat.com>
mgr / volumes: wrap rmtree() call within try..except block
This will be invoked by purge threads, so be sure to catch all
exceptions and return proper error codes appropriately.

Signed-off-by: Venky Shankar <vshankar@redhat.com>
test: cleanup removing all subvolumes before removing subvolume group
Test `test_subvolume_create_with_desired_mode_in_group()` creates three
subvolume in a subvolume group. During cleanup, it only removed two of
the three subvolumes. This causes failure when removing the subvolume
group since it's not empty.

Signed-off-by: Venky Shankar <vshankar@redhat.com>

@vshankar vshankar force-pushed the vshankar:wip-subvol-purge-queue branch from 568bd1b to aec1d90 Jul 8, 2019

@vshankar

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@ajarr please take a look. I'll schedule a teuthology run shortly.

@vshankar

This comment has been minimized.

@vshankar

This comment has been minimized.

@ajarr

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

@batrick, can you please take a look

@batrick

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

@ajarr, i'm in the middle of reviewing this now.

@batrick

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

@ajarr when does this need to be merged by?

@ajarr

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@ajarr when does this need to be merged by?

By next week

@batrick
Copy link
Member

left a comment

d.d_name is a byte string. In python3 you'll have to explicitly decode it to a string.

We actually want to do the reverse I think: leave it as a byte string everywhere except when presenting it to the user as a message/output/exception.

paths may not be decodeable so we shouldn't try.

I will add a commit that changes this.

src/pybind/mgr/volumes/fs/subvolume.py Outdated Show resolved Hide resolved
src/pybind/mgr/volumes/fs/subvolume.py Outdated Show resolved Hide resolved
@batrick

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

more comments on the way...

pybind/mgr/volumes: remove unused property
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
batrick added 5 commits Jul 10, 2019
pybind/mgr/subvolumes: use bytes for paths
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
pybind/mgr/volumes: cleanup fs removal
In Nautilus, the simplest sequence is:

    fs fail name
    fs rm name --yes-i-really-mean-it

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
pybind/mgr/volumes: use existing client provided recursive mkdir
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
pybind/mgr/volumes: refactor trash readdir
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
pybind/mgr/volumes: print exceptions in purge thread
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

@batrick batrick force-pushed the vshankar:wip-subvol-purge-queue branch from cdf70c5 to ecf8502 Jul 16, 2019

@batrick

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

test branch:https://shaman.ceph.com/builds/ceph/wip-pdonnell-testing-20190716.044422/28079b5d783554e56a8858680f8b182dc71da0cd/

tests will run in 210 minutes.

@ajarr I'm satisfied to merge this. I've added a few changes. I do have a few code hygiene requests but they can be addressed when Venky returns from PTO. I'll merge this tomorrow morning assuming no issues with the tests.

client: do not return EEXIST for mkdirs
Behavior should be similar to `mkdir -p`.

Introduced-by: 26905ca
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

@batrick batrick merged commit b049eb9 into ceph:master Jul 16, 2019

4 of 5 checks passed

make check (arm64) make check failed
Details
Docs: build check OK - docs built
Details
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
batrick added a commit that referenced this pull request Jul 16, 2019
Merge PR #28003 into master
* refs/pull/28003/head:
	client: do not return EEXIST for mkdirs
	pybind/mgr/volumes: print exceptions in purge thread
	pybind/mgr/volumes: refactor trash readdir
	pybind/mgr/volumes: use existing client provided recursive mkdir
	pybind/mgr/volumes: cleanup fs removal
	pybind/mgr/subvolumes: use bytes for paths
	pybind/mgr/volumes: remove unused property
	test: add basic purge queue validation test
	mgr / volumes: schedule purge job for volumes on init
	mgr / volumes: purge queue for async subvolume delete
	mgr / volumes: maintain connection pool for fs volumes
	mgr / volumes: wrap rmtree() call within try..except block
	mgr / volumes: use negative error codes everywhere
	test: cleanup removing all subvolumes before removing subvolume group

Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

@vshankar vshankar deleted the vshankar:wip-subvol-purge-queue branch Jul 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.