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

rbd-mirror: separate ImageReplayer handling from Replayer #13803

Merged
merged 3 commits into from Apr 9, 2017

Conversation

Projects
None yet
2 participants
@trociny
Copy link
Contributor

trociny commented Mar 6, 2017

Fixes: http://tracker.ceph.com/issues/18785
Signed-off-by: Mykola Golub mgolub@mirantis.com

@trociny trociny requested a review from dillaman Mar 6, 2017


Mutex::Locker locker(m_lock);

ImageReplayer<I> *image_replayer = ImageReplayer<I>::create(

This comment has been minimized.

Copy link
@dillaman

dillaman Mar 9, 2017

Contributor

Notifications might be repeated -- so need to protect against the potential that the image replayer has already been created for this global id. That is why it might make sense to combine the acquire and update notifications.


Mutex::Locker locker(m_lock);

auto it = m_image_replayers.find(global_image_id);

This comment has been minimized.

Copy link
@dillaman

dillaman Mar 9, 2017

Contributor

Same comment here -- might be a repeated RPC message.

auto it = m_image_replayers.find(global_image_id);
assert(it != m_image_replayers.end());

ImageReplayer<I> *old_image_replayer = it->second;

This comment has been minimized.

Copy link
@dillaman

dillaman Mar 9, 2017

Contributor

I'd avoid shutting down a replayer and starting a new one upon update. For now, there won't be peer updates -- but in the future, let ImageReplayer add the smarts since it will only care to attach to a new peer if it's not running (since there is no primary).

@@ -4,6 +4,7 @@ add_library(rbd_mirror_types STATIC
set(rbd_mirror_internal
ClusterWatcher.cc
ImageDeleter.cc
ImageMapper.cc

This comment has been minimized.

Copy link
@dillaman

dillaman Mar 9, 2017

Contributor

I'd vote to skip class from this for this PR -- it's going to require a lot of refinement before it truly stubs out the necessary interactions

This comment has been minimized.

Copy link
@trociny

trociny Mar 9, 2017

Author Contributor

The problem is that the Replayer needs to track in some way what images have already been acquired. It needs this to make a decision to release an image that was deleted in the peer pool. I expect it will be getting this info from ImageMapper in future, thus an extremely simplified version has been added.

I could just add a temporary ImageMapper class (or whatever it is called) to Replayer. Or add a method to InstanceReplayer to get e.g. a list of acquired images and use this instead, but I suppose this is not a way we a going to do this eventually so it would also be a temporary solution?

This comment has been minimized.

Copy link
@dillaman

dillaman Mar 9, 2017

Contributor

That's true, I guess -- fine to leave it. I should probably rebase #12364 and see if I can work around the recreated image race condition.

@trociny trociny force-pushed the trociny:wip-18785 branch 2 times, most recently from 99b2a65 to cb612c1 Mar 10, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Mar 10, 2017

@dillaman updated. It has not been tested well after update yet (running the tests), but I just wanted to know if you like this version -- I added handling of repeated RPC message, removed "update" method and added set_remote_images method to ImageReplayer , which is used now in acquire_image.

@trociny trociny force-pushed the trociny:wip-18785 branch 3 times, most recently from ceacd85 to 0f06129 Mar 10, 2017

@trociny trociny force-pushed the trociny:wip-18785 branch 3 times, most recently from 5d90f95 to 08b7d50 Apr 3, 2017

@trociny trociny changed the title [DNM] rbd-mirror A/A: separate ImageReplayer handling from Replayer rbd-mirror A/A: separate ImageReplayer handling from Replayer Apr 6, 2017

@dillaman dillaman changed the title rbd-mirror A/A: separate ImageReplayer handling from Replayer rbd-mirror: separate ImageReplayer handling from Replayer Apr 7, 2017

template <typename ImageCtxT = librbd::ImageCtx>
class InstanceReplayer {
public:
struct Peer {

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 7, 2017

Contributor

Nit: can you move this to types.h?

struct RemoteImage {
std::string mirror_uuid;
std::string image_id;
librados::IoCtx io_ctx;

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 7, 2017

Contributor

Nit: can this be renamed PeerImage and inherit from the newly moved Peer?



static InstanceReplayer* create(
Threads<ImageCtxT> *threads, std::shared_ptr<ImageDeleter> image_deleter,

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 7, 2017

Contributor

Nit: can you indent 4 spaces when wrap down like this to provide clear separation to the body of the function.


template <typename I>
InstanceReplayer<I>::InstanceReplayer(
Threads<I> *threads, std::shared_ptr<ImageDeleter> image_deleter,

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 7, 2017

Contributor

Nit: prefer indent 4 spaces for clarity

Context *ctx = new FunctionContext(
[this] (int r) {
cancel_image_state_check_task();
Mutex::Locker locker(m_lock);

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 7, 2017

Contributor

Nit: move locker directly into wait_for_ops

This comment has been minimized.

Copy link
@trociny

trociny Apr 7, 2017

Author Contributor

Actually, the lock is not needed in wait_for_ops. I just wanted to make it similar to other xyz()/handle_xyz() functions.

Ok, I removed the lock in wait_for_ops (and create_async_context_callback, which is not needed after this).

namespace mirror {
namespace instance_watcher {

struct ImagePeer {

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 7, 2017

Contributor

Nit: can we rename this PeerImageId?

auto local_image_id = image_replayer->get_local_image_id();
if (local_image_id.empty()) {
dout(20) << global_image_id << ": unknown local_image_id"
<< " (image not exist or primary), skipping delete" << dendl;

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 7, 2017

Contributor

Nit: "... image does not exist ..."

start_image_replayer(image_replayer);
}
});

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 7, 2017

Contributor

I think we need to start and op here and finish it in the context above before each return point since we need to ensure zero percent possibility of the ImageDeleter calling back into a deleted instance replayer.

Mykola Golub
rbd-mirror: add set_remote_images method to ImageReplayer
Signed-off-by: Mykola Golub <mgolub@mirantis.com>

@trociny trociny force-pushed the trociny:wip-18785 branch from 08b7d50 to ec393c3 Apr 7, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Apr 7, 2017

@dillaman updated

dout(20) << "image deleter result: r=" << r << ", "
<< "global_image_id=" << global_image_id << dendl;
if (r == -ESTALE || r == -ECANCELED) {
return;

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 7, 2017

Contributor

m_async_op_tracker.finish_op()

Mutex::Locker locker(m_lock);
auto it = m_image_replayers.find(global_image_id);
if (it == m_image_replayers.end()) {
return;

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 7, 2017

Contributor

m_async_op_tracker.finish_op()

Mykola Golub added some commits Mar 18, 2017

Mykola Golub
Mykola Golub
rbd-mirror A/A: InstanceReplayer class to acquire and release images
Signed-off-by: Mykola Golub <mgolub@mirantis.com>

@trociny trociny force-pushed the trociny:wip-18785 branch from ec393c3 to 4931e31 Apr 7, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Apr 7, 2017

@dillaman Thanks! I have just moved finish_op above returns.

@dillaman dillaman merged commit 0b61e11 into ceph:master Apr 9, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@trociny trociny deleted the trociny:wip-18785 branch Apr 9, 2017

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