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 A/A: leader should track up/down rbd-mirror instances #13571

Merged
merged 3 commits into from Mar 2, 2017

Conversation

Projects
None yet
2 participants
@trociny
Copy link
Contributor

trociny commented Feb 21, 2017

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

@trociny trociny requested a review from dillaman Feb 21, 2017

assert(m_lock.is_locked());
assert(!m_instances);

m_instances.reset(new Instances<I>(m_threads, m_ioctx));

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 23, 2017

Contributor

Nit: use a ::create method so the mocks can be tested

@@ -32,6 +33,17 @@ struct HeartbeatPayload {
void dump(Formatter *f) const;
};

struct HeartbeatAckPayload {

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 23, 2017

Contributor

I was thinking that you could directly use the ack payload sent in response to the HeartbeatPayload message. That ensures the acks are sent to the leader that actually initiated the message -- and eliminates any potential race during a leader transition.

https://github.com/ceph/ceph/blob/master/src/include/rados/librados.hpp#L1078

@@ -762,6 +835,24 @@ void LeaderWatcher<I>::handle_heartbeat(Context *on_notify_ack) {
m_acquire_attempts = 0;
cancel_timer_task();
get_locker();
notify_heartbeat_ack();

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 23, 2017

Contributor

Nit: suggest using the implicit ack generated below by on_notify_ack->complete(0)

@trociny trociny force-pushed the trociny:wip-18784 branch from eaf9e7c to 6b6eef9 Feb 23, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 23, 2017

@dillaman updated

@trociny trociny force-pushed the trociny:wip-18784 branch from 6b6eef9 to 88c1a3b Feb 24, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 24, 2017

@dillaman some issues have been found and fixed after testing:

  1. LeaderWatcher, in handle_notify_heartbeat(), did not filter out its own id.
  2. In Instances, if InstanceWatcher::remove_instance() was executed for long time by some reason, it was possible that the second remove_instance() was started at that time resulting in dereferencing dead object.

Still I am observing some oddities in tests I need to investigate.

I updated Replayer admin socket to output current list of instances.

Also, I am thinking if just looking for notifier_id when parsing heartbeat ack is enough. A random client can be watching the object at that time and will be added to instances table and later removed. This should not cause any issues, still may be it is a good idea to add payload to ack?

@trociny trociny force-pushed the trociny:wip-18784 branch from 88c1a3b to 07c9b97 Feb 24, 2017

@trociny trociny changed the title [DNM] rbd-mirror A/A: leader should track up/down rbd-mirror instances rbd-mirror A/A: leader should track up/down rbd-mirror instances Feb 24, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 24, 2017

@dillaman I believe I have fixed the tests instabilities I observed. They were due to too small "remove after" timeout, set initially to rbd_mirror_leader_heartbeat_interval * rbd_mirror_leader_max_missed_heartbeats, which was 10 seconds by default.

I set it to rbd_mirror_leader_heartbeat_interval * (1 + rbd_mirror_leader_max_missed_heartbeats + rbd_mirror_leader_max_acquire_attempts_before_break), it corresponds to the time interval after which we blacklist a not responding leader, 30 sec by default. (Not sure we need a special config option for this)

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 24, 2017

@dillaman As for jenkins failure, I was able to reproduce this running ./bin/unittest_rbd_mirror --gtest_repeat=10000 --gtest_filter=TestLeaderWatcher.Two on a loaded system. This is also reproduced on master so is not related to this PR.

It fails here

#0  entity_addr_t::parse (this=this@entry=0x7f04e8e73e7c, s=<optimized out>, s@entry=0x7f04d0000df0 "-", end=end@entry=0x0) at /home/mgolub/ceph.ci/src/msg/msg_types.cc:79
79          *end = s + 1;
(gdb) bt
#0  entity_addr_t::parse (this=this@entry=0x7f04e8e73e7c, s=<optimized out>, s@entry=0x7f04d0000df0 "-", end=end@entry=0x0) at /home/mgolub/ceph.ci/src/msg/msg_types.cc:79
#1  0x000055b1e5d27e49 in cls_cxx_list_watchers (hctx=hctx@entry=0x7f04d0000bc0, watchers=watchers@entry=0x7f04e8e73fd0)
    at /home/mgolub/ceph.ci/src/test/librados_test_stub/LibradosTestStub.cc:1216
#2  0x00007f04e54080ea in mirror::image_status_remove_down (hctx=0x7f04d0000bc0) at /home/mgolub/ceph.ci/src/cls/rbd/cls_rbd.cc:3539
#3  0x00007f04e5409289 in mirror_image_status_remove_down (hctx=<optimized out>, in=in@entry=0x7f04d80080b8, out=out@entry=0x0) at /home/mgolub/ceph.ci/src/cls/rbd/cls_rbd.cc:4264
#4  0x000055b1e5d37d39 in librados::TestIoCtxImpl::exec (this=0x55b1e6b23e30, oid="rbd_mirroring", handler=0x55b1e6b1d140, cls=<optimized out>, method=<optimized out>, inbl=..., outbl=0x0, 
    snapc=...) at /home/mgolub/ceph.ci/src/test/librados_test_stub/TestIoCtxImpl.cc:156

when the test is waiting on on_acquire.wait(). And I see the problem with entity_addr_t::parse, that it does not check end for '-' address, as it does in general case:

diff --git a/src/msg/msg_types.cc b/src/msg/msg_types.cc
index 1d118e9..67a699c 100644
--- a/src/msg/msg_types.cc
+++ b/src/msg/msg_types.cc
@@ -76,7 +76,9 @@ bool entity_addr_t::parse(const char *s, const char **end)
     newtype = TYPE_MSGR2;
   } else if (*s == '-') {
     *this = entity_addr_t();
-    *end = s + 1;
+    if (end) {
+      *end = s + 1;
+    }
     return true;
   }

But right now I don't have an idea why it shows up only sporadically and if this is a sign of some bug in the LeaderWatcher.

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 24, 2017

So, for me it looks like sometimes when the second MirrorStatusWatcher is starting on acquire the leader lock, the first watcher (that should have been already unregistered) is still returned by list_watchers.

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 25, 2017

@dillaman I created a PR for entity_addr_t::parse() issue #13650, but I also found a problem with TestLeaderWatcher.Two. It does not work as expected on librados test stub, because leader watchers have the same client IDs (even if I create separate connections), so all notifications are filtered out as their own. It might lead to the second watcher blacklisting the first one and then crushing on the status watcher init, when parsing the listener address.

I added "skip" for this test on librados test stub.

@dillaman
Copy link
Contributor

dillaman left a comment

lgtm

MockManagedLock::get_instance().construct();
}

virtual ~ManagedLock() {

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 28, 2017

Contributor

Nit: override vs virtual

This comment has been minimized.

Copy link
@trociny

trociny Feb 28, 2017

Author Contributor

@dillaman ManagedLock is a base (not derived) class. I think "virtual" is correct in this case?

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 28, 2017

Contributor

Yup -- sorry. Thought it was a derived class from the quick scan.

@@ -33,6 +34,7 @@ class LeaderWatcher : protected librbd::Watcher {
};

LeaderWatcher(Threads *threads, librados::IoCtx &io_ctx, Listener *listener);
virtual ~LeaderWatcher();

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 28, 2017

Contributor

Nit: override vs virtual

MirrorStatusWatcher(librados::IoCtx &io_ctx, ContextWQ *work_queue);
virtual ~MirrorStatusWatcher();

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 28, 2017

Contributor

Nit: override vs virtual

Mykola Golub added some commits Feb 11, 2017

Mykola Golub
rbd-mirror: class for tracking instances state
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
Mykola Golub
test/rbd_mirror: leader watchers need separate clients for notify to …
…work

Signed-off-by: Mykola Golub <mgolub@mirantis.com>

@trociny trociny force-pushed the trociny:wip-18784 branch from e1b348b to 178484c Feb 28, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 28, 2017

@dillaman I have addressed all your comments but ManagedLock mock class.

@trociny trociny changed the title rbd-mirror A/A: leader should track up/down rbd-mirror instances DNM: rbd-mirror A/A: leader should track up/down rbd-mirror instances Feb 28, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 28, 2017

@dillaman Observing some strange test failures after rebase, need some time to investigate the root cause.

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 28, 2017

I observed a crush running ceph_test_rbd_mirror, but it looks like something has been broken in the master recently -- observing the same crush in the pure master running e.g. RBD_FEATURES=109 ./bin/ceph_test_librbd

#0  0x00007fffef16ad8f in std::unique_ptr<AuthClientHandler, std::default_delete<AuthClientHandler> >::get (this=<optimized out>) at /usr/include/c++/5/bits/unique_ptr.h:305
#1  std::unique_ptr<AuthClientHandler, std::default_delete<AuthClientHandler> >::operator bool (this=<optimized out>) at /usr/include/c++/5/bits/unique_ptr.h:319
#2  MonClient::build_authorizer (this=0x555556098238, service_id=16) at /home/mgolub/ceph.ci/src/mon/MonClient.cc:1139
#3  0x00005555559db062 in librados::RadosClient::ms_get_authorizer (this=<optimized out>, dest_type=<optimized out>, authorizer=0x7fff477fae70, force_new=<optimized out>)
    at /home/mgolub/ceph.ci/src/librados/RadosClient.cc:63
#4  0x00007fffef24a5b0 in Messenger::ms_deliver_get_authorizer (force_new=false, peer_type=16, this=0x55555609a530) at /home/mgolub/ceph.ci/src/msg/Messenger.h:722
#5  AsyncMessenger::get_authorizer (force_new=false, peer_type=16, this=0x55555609a530) at /home/mgolub/ceph.ci/src/msg/async/AsyncMessenger.h:365
#6  AsyncConnection::_process_connection (this=this@entry=0x7fff2c0135f0) at /home/mgolub/ceph.ci/src/msg/async/AsyncConnection.cc:1024
#7  0x00007fffef24f7a8 in AsyncConnection::process (this=0x7fff2c0135f0) at /home/mgolub/ceph.ci/src/msg/async/AsyncConnection.cc:834
#8  0x00007fffef261f61 in EventCenter::process_events (this=this@entry=0x5555560cd670, timeout_microseconds=<optimized out>, timeout_microseconds@entry=30000000)
    at /home/mgolub/ceph.ci/src/msg/async/Event.cc:406
#9  0x00007fffef2669e9 in NetworkStack::<lambda()>::operator()(void) const (__closure=0x555556126f48) at /home/mgolub/ceph.ci/src/msg/async/Stack.cc:46
#10 0x00007fffee9c4c80 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#11 0x00007ffff7bc16ba in start_thread (arg=0x7fff477fe700) at pthread_create.c:333
#12 0x00007fffee12a82d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

@trociny trociny changed the title DNM: rbd-mirror A/A: leader should track up/down rbd-mirror instances rbd-mirror A/A: leader should track up/down rbd-mirror instances Feb 28, 2017

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Feb 28, 2017

retest this please

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Mar 1, 2017

@trociny Should be fixed now that #13685 is merged -- retesting

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Mar 1, 2017

retest this please

2 similar comments
@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Mar 1, 2017

retest this please

@dillaman

This comment has been minimized.

Copy link
Contributor

dillaman commented Mar 2, 2017

retest this please

@dillaman dillaman merged commit 870bc38 into ceph:master Mar 2, 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-18784 branch Mar 2, 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.