rbd-mirror: leader watcher should always initialize status watcher

In the case of active/active, both the leader and the followers will be
sending status updates for their images. Therefore, all rbd-mirror daemons
should be registered.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

3 files changed, 85 insertions, 113 deletions

diff --git a/src/test/rbd_mirror/test_mock_LeaderWatcher.cc b/src/test/rbd_mirror/test_mock_LeaderWatcher.cc
index 7922ade5eb9..a4ee8fecd4b 100644
--- a/src/test/rbd_mirror/test_mock_LeaderWatcher.cc
+++ b/src/test/rbd_mirror/test_mock_LeaderWatcher.cc
@@ -463,11 +463,11 @@ TEST_F(TestMockLeaderWatcher, InitShutdown) {
   expect_construct(mock_managed_lock);
   MockLeaderWatcher leader_watcher(m_mock_threads, m_local_io_ctx, &listener);
 
-  // Inint
+  // Init
+  expect_init(mock_mirror_status_watcher, 0);
   C_SaferCond on_heartbeat_finish;
   expect_is_leader(mock_managed_lock, false, false);
   expect_try_acquire_lock(mock_managed_lock, 0);
-  expect_init(mock_mirror_status_watcher, 0);
   expect_init(mock_instances, 0);
   expect_acquire_notify(mock_managed_lock, listener, 0);
   expect_notify_heartbeat(mock_managed_lock, &on_heartbeat_finish);
@@ -478,10 +478,9 @@ TEST_F(TestMockLeaderWatcher, InitShutdown) {
   // Shutdown
   expect_release_notify(mock_managed_lock, listener, 0);
   expect_shut_down(mock_instances, 0);
-  expect_shut_down(mock_mirror_status_watcher, 0);
-  expect_is_leader(mock_managed_lock, false, false);
   expect_release_lock(mock_managed_lock, 0);
   expect_shut_down(mock_managed_lock, true, 0);
+  expect_shut_down(mock_mirror_status_watcher, 0);
   expect_is_leader(mock_managed_lock, false, false);
 
   leader_watcher.shut_down();
@@ -501,11 +500,11 @@ TEST_F(TestMockLeaderWatcher, InitReleaseShutdown) {
   expect_construct(mock_managed_lock);
   MockLeaderWatcher leader_watcher(m_mock_threads, m_local_io_ctx, &listener);
 
-  // Inint
+  // Init
+  expect_init(mock_mirror_status_watcher, 0);
   C_SaferCond on_heartbeat_finish;
   expect_is_leader(mock_managed_lock, false, false);
   expect_try_acquire_lock(mock_managed_lock, 0);
-  expect_init(mock_mirror_status_watcher, 0);
   expect_init(mock_instances, 0);
   expect_acquire_notify(mock_managed_lock, listener, 0);
   expect_notify_heartbeat(mock_managed_lock, &on_heartbeat_finish);
@@ -517,8 +516,6 @@ TEST_F(TestMockLeaderWatcher, InitReleaseShutdown) {
   expect_is_leader(mock_managed_lock, false, true);
   expect_release_notify(mock_managed_lock, listener, 0);
   expect_shut_down(mock_instances, 0);
-  expect_shut_down(mock_mirror_status_watcher, 0);
-  expect_is_leader(mock_managed_lock, false, false);
   C_SaferCond on_release;
   expect_release_lock(mock_managed_lock, 0, &on_release);
 
@@ -527,12 +524,13 @@ TEST_F(TestMockLeaderWatcher, InitReleaseShutdown) {
 
   // Shutdown
   expect_shut_down(mock_managed_lock, false, 0);
+  expect_shut_down(mock_mirror_status_watcher, 0);
   expect_is_leader(mock_managed_lock, false, false);
 
   leader_watcher.shut_down();
 }
 
-TEST_F(TestMockLeaderWatcher, AcquireError) {
+TEST_F(TestMockLeaderWatcher, InitStatusWatcherError) {
   MockManagedLock mock_managed_lock;
   MockMirrorStatusWatcher mock_mirror_status_watcher;
   MockInstances mock_instances;
@@ -547,39 +545,26 @@ TEST_F(TestMockLeaderWatcher, AcquireError) {
   expect_construct(mock_managed_lock);
   MockLeaderWatcher leader_watcher(m_mock_threads, m_local_io_ctx, &listener);
 
-  // Inint
-  C_SaferCond on_heartbeat_finish;
-  expect_is_leader(mock_managed_lock, false, false);
-  expect_try_acquire_lock(mock_managed_lock, -EAGAIN);
-  expect_get_locker(mock_managed_lock, librbd::managed_lock::Locker(), -ENOENT);
-  expect_try_acquire_lock(mock_managed_lock, 0);
-  expect_init(mock_mirror_status_watcher, 0);
-  expect_init(mock_instances, 0);
-  expect_acquire_notify(mock_managed_lock, listener, 0);
-  expect_notify_heartbeat(mock_managed_lock, &on_heartbeat_finish);
-
-  ASSERT_EQ(0, leader_watcher.init());
-  ASSERT_EQ(0, on_heartbeat_finish.wait());
+  // Init
+  expect_init(mock_mirror_status_watcher, -EINVAL);
+  ASSERT_EQ(-EINVAL, leader_watcher.init());
 
   // Shutdown
-  expect_release_notify(mock_managed_lock, listener, 0);
-  expect_shut_down(mock_instances, 0);
+  expect_shut_down(mock_managed_lock, false, 0);
   expect_shut_down(mock_mirror_status_watcher, 0);
   expect_is_leader(mock_managed_lock, false, false);
-  expect_release_lock(mock_managed_lock, 0);
-  expect_shut_down(mock_managed_lock, true, 0);
-  expect_is_leader(mock_managed_lock, false, false);
 
   leader_watcher.shut_down();
 }
 
-TEST_F(TestMockLeaderWatcher, ReleaseError) {
+TEST_F(TestMockLeaderWatcher, AcquireError) {
   MockManagedLock mock_managed_lock;
   MockMirrorStatusWatcher mock_mirror_status_watcher;
   MockInstances mock_instances;
   MockListener listener;
 
   expect_is_shutdown(mock_managed_lock);
+  expect_is_leader(mock_managed_lock);
   expect_destroy(mock_managed_lock);
 
   InSequence seq;
@@ -587,11 +572,13 @@ TEST_F(TestMockLeaderWatcher, ReleaseError) {
   expect_construct(mock_managed_lock);
   MockLeaderWatcher leader_watcher(m_mock_threads, m_local_io_ctx, &listener);
 
-  // Inint
+  // Init
+  expect_init(mock_mirror_status_watcher, 0);
   C_SaferCond on_heartbeat_finish;
   expect_is_leader(mock_managed_lock, false, false);
+  expect_try_acquire_lock(mock_managed_lock, -EAGAIN);
+  expect_get_locker(mock_managed_lock, librbd::managed_lock::Locker(), -ENOENT);
   expect_try_acquire_lock(mock_managed_lock, 0);
-  expect_init(mock_mirror_status_watcher, 0);
   expect_init(mock_instances, 0);
   expect_acquire_notify(mock_managed_lock, listener, 0);
   expect_notify_heartbeat(mock_managed_lock, &on_heartbeat_finish);
@@ -599,20 +586,12 @@ TEST_F(TestMockLeaderWatcher, ReleaseError) {
   ASSERT_EQ(0, leader_watcher.init());
   ASSERT_EQ(0, on_heartbeat_finish.wait());
 
-  // Release
-  expect_is_leader(mock_managed_lock, false, true);
-  expect_release_notify(mock_managed_lock, listener, -EINVAL);
-  expect_shut_down(mock_instances, 0);
-  expect_shut_down(mock_mirror_status_watcher, -EINVAL);
-  expect_is_leader(mock_managed_lock, false, false);
-  C_SaferCond on_release;
-  expect_release_lock(mock_managed_lock, -EINVAL, &on_release);
-
-  leader_watcher.release_leader();
-  ASSERT_EQ(0, on_release.wait());
-
   // Shutdown
-  expect_shut_down(mock_managed_lock, false, 0);
+  expect_release_notify(mock_managed_lock, listener, 0);
+  expect_shut_down(mock_instances, 0);
+  expect_release_lock(mock_managed_lock, 0);
+  expect_shut_down(mock_managed_lock, true, 0);
+  expect_shut_down(mock_mirror_status_watcher, 0);
   expect_is_leader(mock_managed_lock, false, false);
 
   leader_watcher.shut_down();
@@ -644,6 +623,7 @@ TEST_F(TestMockLeaderWatcher, Break) {
   MockLeaderWatcher leader_watcher(m_mock_threads, m_local_io_ctx, &listener);
 
   // Init
+  expect_init(mock_mirror_status_watcher, 0);
   expect_is_leader(mock_managed_lock, false, false);
   for (int i = 0; i < max_acquire_attempts; i++) {
     expect_try_acquire_lock(mock_managed_lock, -EAGAIN);
@@ -653,7 +633,6 @@ TEST_F(TestMockLeaderWatcher, Break) {
   expect_break_lock(mock_managed_lock, locker, 0, &on_break);
   C_SaferCond on_heartbeat_finish;
   expect_try_acquire_lock(mock_managed_lock, 0);
-  expect_init(mock_mirror_status_watcher, 0);
   expect_init(mock_instances, 0);
   expect_acquire_notify(mock_managed_lock, listener, 0);
   expect_notify_heartbeat(mock_managed_lock, &on_heartbeat_finish);
@@ -664,10 +643,9 @@ TEST_F(TestMockLeaderWatcher, Break) {
   // Shutdown
   expect_release_notify(mock_managed_lock, listener, 0);
   expect_shut_down(mock_instances, 0);
-  expect_shut_down(mock_mirror_status_watcher, 0);
-  expect_is_leader(mock_managed_lock, false, false);
   expect_release_lock(mock_managed_lock, 0);
   expect_shut_down(mock_managed_lock, true, 0);
+  expect_shut_down(mock_mirror_status_watcher, 0);
   expect_is_leader(mock_managed_lock, false, false);
 
   leader_watcher.shut_down();
diff --git a/src/tools/rbd_mirror/LeaderWatcher.cc b/src/tools/rbd_mirror/LeaderWatcher.cc
index 7b2eb0c58f2..a376cc1a6b3 100644
--- a/src/tools/rbd_mirror/LeaderWatcher.cc
+++ b/src/tools/rbd_mirror/LeaderWatcher.cc
@@ -125,19 +125,18 @@ void LeaderWatcher<I>::handle_register_watch(int r) {
   dout(20) << "r=" << r << dendl;
 
   Context *on_finish = nullptr;
-  {
-    Mutex::Locker timer_locker(m_threads->timer_lock);
+  if (r < 0) {
     Mutex::Locker locker(m_lock);
-
-    if (r < 0) {
-      derr << "error registering leader watcher for " << m_oid << " object: "
-           << cpp_strerror(r) << dendl;
-    } else {
-      schedule_acquire_leader_lock(0);
-    }
-
+    derr << "error registering leader watcher for " << m_oid << " object: "
+         << cpp_strerror(r) << dendl;
+    assert(m_on_finish != nullptr);
     std::swap(on_finish, m_on_finish);
+  } else {
+    Mutex::Locker locker(m_lock);
+    init_status_watcher();
+    return;
   }
+
   on_finish->complete(r);
 }
 
@@ -185,7 +184,7 @@ void LeaderWatcher<I>::handle_shut_down_leader_lock(int r) {
     derr << "error shutting down leader lock: " << cpp_strerror(r) << dendl;
   }
 
-  unregister_watch();
+  shut_down_status_watcher();
 }
 
 template <typename I>
@@ -416,7 +415,7 @@ void LeaderWatcher<I>::handle_post_acquire_leader_lock(int r,
   m_on_finish = on_finish;
   m_ret_val = 0;
 
-  init_status_watcher();
+  init_instances();
 }
 
 template <typename I>
@@ -712,20 +711,20 @@ void LeaderWatcher<I>::handle_init_status_watcher(int r) {
 
   Context *on_finish = nullptr;
   {
+    Mutex::Locker timer_locker(m_threads->timer_lock);
     Mutex::Locker locker(m_lock);
 
-    if (r == 0) {
-      init_instances();
-      return;
+    if (r < 0) {
+      derr << "error initializing mirror status watcher: " << cpp_strerror(r)
+           << cpp_strerror(r) << dendl;
+    } else {
+      schedule_acquire_leader_lock(0);
     }
 
-    derr << "error initializing mirror status watcher: " << cpp_strerror(r)
-         << dendl;
-    m_status_watcher->destroy();
-    m_status_watcher = nullptr;
     assert(m_on_finish != nullptr);
-    std::swap(m_on_finish, on_finish);
+    std::swap(on_finish, m_on_finish);
   }
+
   on_finish->complete(r);
 }
 
@@ -747,31 +746,16 @@ template <typename I>
 void LeaderWatcher<I>::handle_shut_down_status_watcher(int r) {
   dout(20) << "r=" << r << dendl;
 
-  Context *on_finish = nullptr;
-  {
-    Mutex::Locker locker(m_lock);
-
-    m_status_watcher->destroy();
-    m_status_watcher = nullptr;
-
-    if (r < 0) {
-      derr << "error shutting mirror status watcher down: " << cpp_strerror(r)
-           << dendl;
-    }
-
-    if (m_ret_val != 0) {
-      r = m_ret_val;
-    }
-
-    if (!is_leader(m_lock)) {
-      // ignore on releasing
-      r = 0;
-    }
+  Mutex::Locker locker(m_lock);
+  m_status_watcher->destroy();
+  m_status_watcher = nullptr;
 
-    assert(m_on_finish != nullptr);
-    std::swap(m_on_finish, on_finish);
+  if (r < 0) {
+    derr << "error shutting mirror status watcher down: " << cpp_strerror(r)
+         << dendl;
   }
-  on_finish->complete(r);
+
+  unregister_watch();
 }
 
 template <typename I>
@@ -793,18 +777,22 @@ template <typename I>
 void LeaderWatcher<I>::handle_init_instances(int r) {
   dout(20) << "r=" << r << dendl;
 
-  Mutex::Locker locker(m_lock);
-
+  Context *on_finish = nullptr;
   if (r < 0) {
+    Mutex::Locker locker(m_lock);
     derr << "error initializing instances: " << cpp_strerror(r) << dendl;
-    m_ret_val = r;
     m_instances->destroy();
     m_instances = nullptr;
-    shut_down_status_watcher();
+
+    assert(m_on_finish != nullptr);
+    std::swap(m_on_finish, on_finish);
+  } else {
+    Mutex::Locker locker(m_lock);
+    notify_listener();
     return;
   }
 
-  notify_listener();
+  on_finish->complete(r);
 }
 
 template <typename I>
@@ -826,12 +814,17 @@ void LeaderWatcher<I>::handle_shut_down_instances(int r) {
   dout(20) << "r=" << r << dendl;
   assert(r == 0);
 
-  Mutex::Locker locker(m_lock);
+  Context *on_finish = nullptr;
+  {
+    Mutex::Locker locker(m_lock);
 
-  m_instances->destroy();
-  m_instances = nullptr;
+    m_instances->destroy();
+    m_instances = nullptr;
 
-  shut_down_status_watcher();
+    assert(m_on_finish != nullptr);
+    std::swap(m_on_finish, on_finish);
+  }
+  on_finish->complete(r);
 }
 
 template <typename I>
diff --git a/src/tools/rbd_mirror/LeaderWatcher.h b/src/tools/rbd_mirror/LeaderWatcher.h
index d7f5732796e..e1149960208 100644
--- a/src/tools/rbd_mirror/LeaderWatcher.h
+++ b/src/tools/rbd_mirror/LeaderWatcher.h
@@ -51,13 +51,16 @@ private:
    * @verbatim
    *
    *  <uninitialized> <------------------------------ WAIT_FOR_TASKS
-   *     | (init)      ^                                      ^
-   *     v             *                                      |
-   *  CREATE_OBJECT  * *  (error)                     UNREGISTER_WATCH
-   *     |             *                                      ^
-   *     v             *                                      |
-   *  REGISTER_WATCH * *                              SHUT_DOWN_LEADER_LOCK
-   *     |                                                    ^
+   *     | (init)            ^                                ^
+   *     v                   *                                |
+   *  CREATE_OBJECT  * * * * *  (error)               UNREGISTER_WATCH
+   *     |                   *                                ^
+   *     v                   *                                |
+   *  REGISTER_WATCH * * * * *                        SHUT_DOWN_STATUS_WATCHER
+   *     |                   *                                ^
+   *     v                   *                                |
+   *  INIT_STATUS_WATCHER  * *                        SHUT_DOWN_LEADER_LOCK
+   *     |                                                    |
    *     |           (no leader heartbeat and acquire failed) |
    *     | BREAK_LOCK <-------------------------------------\ |
    *     |    |                 (no leader heartbeat)       | | (shut down)
@@ -73,16 +76,14 @@ private:
    *     |                   *                           ^
    * ....|...................*....................  .....|.....................
    * .   v                   *                   .  .    |       post_release .
-   * .INIT_STATUS_WATCHER  * * (error)           .  .NOTIFY_LOCK_RELEASED     .
-   * .   |                   ^                   .  .....^.....................
-   * .   v         (error)   |                   .       |
-   * .INIT_INSTANCES *> SHUT_DOWN_STATUS_WATCHER .   RELEASE_LEADER_LOCK
+   * .INIT_INSTANCES * * * * *                   .  .NOTIFY_LOCK_RELEASED     .
+   * .   |                                       .  .....^.....................
+   * .   v                                       .       |
+   * .NOTIFY_LISTENER                            .   RELEASE_LEADER_LOCK
    * .   |                                       .       ^
    * .   v                                       .  .....|.....................
-   * .NOTIFY_LISTENER                            .  .SHUT_DOWN_STATUS_WATCHER .
-   * .   |                                       .  .    ^                    .
-   * .   v                                       .  .    |                    .
-   * .NOTIFY_LOCK_ACQUIRED          post_acquire .  .SHUT_DOWN_INSTANCES      .
+   * .NOTIFY_LOCK_ACQUIRED                       .  .    |                    .
+   * .   |                          post_acquire .  .SHUT_DOWN_INSTANCES      .
    * ....|........................................  .    ^                    .
    *     v                                          .    |                    .
    *  <leader> -----------------------------------> .NOTIFY_LISTENER          .