RCORE-1872 Sync client should allow server bootstrapping at any time #7440

michael-wb · 2024-03-08T21:03:16Z

What, How & Why?

Add the ability for the server to send a bootstrap to the server at any time, initially needed to support the handle role changes without a client reset. With these changes a single message/single changeset bootstrap for the current query version will be applied like a regular download message, however a multi-message or single message/multi changeset bootstrap for the current query version will be applied like a regular bootstrap.

These changes also include a test to validate the role change bootstrap and verify the local data is updated to reflect the new permissions. Additional tests are forthcoming in a couple of future PRs.

The version of baasaas and baas used with these changes are based off a branch that is pending review and the protocol version was bumped to v14 in order to support these changes without the feature flag.

Fixes #7584, #7326

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
~~[ ] C-API, if public C++ API changed~~
~~[ ] bindgen/spec.yml, if public C++ API changed~~

…o mwb/role-change-bootstraps

src/realm/object-store/sync/sync_session.cpp

src/realm/sync/noinst/client_impl_base.cpp

…o mwb/role-change-bootstraps

jbreams · 2024-04-18T17:22:30Z

test/object-store/sync/flx_sync.cpp

+TEST_CASE("flx: role change bootstrap", "[sync][flx][baas][role_change][bootstrap]") {
+    const Schema person_schema{{"Person",
+                                {{"_id", PropertyType::ObjectId, Property::IsPrimary{true}},
+                                 {"age", PropertyType::Int | PropertyType::Nullable},


It looks like the only field in this schema that actually matter are "role". Do we need to make up fake age and firstName/lastName properties? how about just a role and a name property so each object is {_id: ObjectId(), role: "manager", name: "manager-1" }

I was going to remove the extra fields, but actually, I need the extra data so I can ensure adding the employees back to the local data generates multiple bootstrap messages.

test/object-store/sync/flx_sync.cpp

jbreams · 2024-04-18T18:17:20Z

test/object-store/sync/flx_sync.cpp

+    auto logger = util::Logger::get_default_logger();
+    FLXSyncTestHarness harness("flx_role_change_bootstrap", {person_schema, {"role", "firstName", "lastName"}});
+    auto& app_session = harness.session().app_session();
+    // Enable the role change bootstraps


why are these commented out?

They currently don't work with baasaas and generate an error...

Is that because this depends on https://jira.mongodb.org/browse/BAAS-28604, or something else?
If so, can you reference that in here like TODO (BAAS-28604): ... and be sure to link that ticket to RCORE-1872?

Yes - this code depends on using the feature flag instead of the protocol version for now. Once the feature is released, the client and server will coordinate the version bump when it will be merged from the feature branch to master. I added a comment to reflect this.

test/object-store/sync/flx_sync.cpp

jbreams · 2024-04-18T20:10:19Z

test/object-store/sync/flx_sync.cpp

+                multi_msg = true;
+        }
+        // Verify a bootstrap occurred if multiple messages were received
+        REQUIRE((!multi_msg || machina.get() == TestState::bs_complete));


don't we need to wait for the state machine to reach bs_complete before we try to get this state value? i'm also not sure what multi_msg is supposed to mean/do?

Are we trying to assert here whether there should be multiple messages or just checking that if there happen to be multiple messages that they end in a complete bootstrap? Can some bootstraps in this test only have one message - how do we enforce that?

I was using the wait_for_download() to wait for the bootstrap to complete, since the server is not supposed to send the MARK response message until after the role change server initiated bootstrap.

Since the integration of the bootstrap is performed when the final bootstrap message is received, the MARK response is not processed until after the bootstrap has been integrated, so the state machine will be updated to the expected state before the wait_for_download() returns.

test/object-store/sync/flx_sync.cpp

jbreams · 2024-04-18T20:43:30Z

src/realm/sync/client.cpp

+
+    // If this is the first LastInBatch=true message after one or more
+    // LastInBatch=false messages, this is the end of a bootstrap
+    if (m_last_download_batch_state == DownloadBatchState::MoreToCome) {


i wish we didn't have to add this state tracking, but I think I see why we need it.

Yeah - it's so we can catch the LastInBatch=false => LastInBatch=true sequence for the server initiated bootstraps. Can you think of a better way?

…o mwb/role-change-bootstraps

src/realm/sync/noinst/pending_bootstrap_store.cpp

danieltabacaru · 2024-04-22T21:43:00Z

test/object-store/sync/flx_sync.cpp

+    FLXSyncTestHarness harness("flx_role_change_bootstrap", {person_schema, {"role", "firstName", "lastName"}});
+    auto& app_session = harness.session().app_session();
+    // Enable the role change bootstraps
+    // REQUIRE(app_session.admin_api.set_feature_flag(app_session.server_app_id, "allow_permissions_bootstrap",


are we going to need this api in the tests? if not, i'm wondering if we should keep the code

Maybe? - right now, the feature is enabled for v14 on the branch I am using, but it may go back to the feature flag once that branch is merged into master.
These will be no longer needed, and therefore removed, once the feature branch is ready to merge into master.

danieltabacaru · 2024-04-24T10:58:54Z

test/object-store/sync/flx_sync.cpp

@@ -4927,6 +4927,255 @@ TEST_CASE("flx: nested collections in mixed", "[sync][flx][baas]") {
    CHECK(nested_list.get_any(1) == "foo");
 }

+TEST_CASE("flx: role change bootstrap", "[sync][flx][baas][role_change][bootstrap]") {


just my 2 cents, but I think the test(s) can be simplified a great deal by doing the following:

instead of using register_connection_change_callback you can use the debug hook and check the client receives a 200 error

for query version 1 you can count all non-empty download messages (the role change bootstrap essentially) and assert on that (additionally, once all messages are received (last_in_batch=true for BootstrapMessageProcessed) you can check the PendingBootstrapStore stored the right data)

finally, you assert the expected data is in the realm

if you want multiple bootstrap in a single test, you can optionally pause the session, change the roles, and resume the session to test role changes detection at ident

Good points - thanks for the recommendations

didn't mention the state machine, but I'm not sure you need it, do you?

The state machine is being used for two reasons:

Wait for the 200 error to be received

Verify the order of receiving a server initiated bootstrap with multiple messages, so we track the order/steps of MoreToCome, LastInBatch and then bootstrap processed.

Isn't wait_for_download enough for all of this? and you can do the tracking in on_sync_client_event_hook

…o mwb/role-change-bootstraps

jbreams

just a hopefully useful comment about using test command futures.

jbreams · 2024-06-06T20:53:09Z

test/object-store/sync/flx_sync.cpp

+                            REQUIRE(cur_state == TestState::reconnect_received);
+                            if (auto session = weak_session.lock()) {
+                                logger->trace("ROLE CHANGE: sending PAUSE test command after resumed");
+                                test_command_futures.push_back(pause_download_builder(*session, true));


It looks like the test_command_futures vector and associated machinery is just to verify that you've reached the session_resumed state after the test command has been sent. So it might be simpler to let the future do that for you with something like this.

if (send_test_command) { REQUIRE(cur_state == TestState::reconnect_received); auto session = weak_session.lock(); REQUIRE(session); pause_download_builder.get_async([&](StatusWith<std::string> payload) { REQUIRE(payload.is_ok()); machina.transition_with([&](TestState state) { return TestState::session_resumed; }); }); return std::nullopt; } else { return TestState::session_resumed; }

That simplifies things definitely. And the only thing the future callback needs to check for is success; it doesn't need to do anything with the state machine.

…o mwb/role-change-bootstraps

danieltabacaru · 2024-06-07T07:56:56Z

src/realm/sync/noinst/client_impl_base.hpp

@@ -1470,6 +1467,10 @@ inline void ClientImpl::Session::connection_established(bool fast_reconnect)
        ++m_target_download_mark;
    }

+    // Call SessionResumed before sending the BIND Message to


I am not sure I understand this comment and why it has to be here (same below)

It is primarily here so it doesn't get moved. The role change test needs to be notified of the session resumed/connected prior to sending the BIND message so it can queue up the test command to be sent before the IDENT message. If this notification happens after the BIND message, there isn't enough time for the test to queue up the test command before the IDENT message is sent.

I updated the message to hopefully be more clear.

I see. I find it a bit hack-ish though. IIUC, there is a scheduling problem because send_test_command posts to the event loop. What if instead we update m_pending_test_commands directly under a lock? And then you can invoke send_test_command from the event loop when sending BIND (you actually don't need the lock if you create a new method only to be invoked from the event loop). Would that work for your tests? @jbreams what do you think of this approach?

I agree, but there isn't a good place to do this, unless I put plumbing in the sync session to add directly to the list of test commands. This is the only thing available in the event hook callback functions.

The current approach ensures the callback to add the test command gets posted before the BIND message is sent and the callback for async_write_binary() is run on the event loop to send the next message after the BIND.

fwiw, i think this is fine for now. another approach could be to send the test commands when you get the 200 disconnect since i think we preserve the list of pending test commands across disconnects, but i wouldn't want to hold this project up over it.

Yes - the list of test commands sticks around for the lifetime of the ClientImpl::Session and the only time commands are removed are when a response is received or the session object is destroyed.
The current send_test_command() logic checks to make sure the session is currently active, but that is easy enough to update if needed.

the session only becomes inactive if you pause() it or destroy the realm - i think in the tests we have right now it should survive a disconnect/reconnect.

danieltabacaru · 2024-06-07T07:59:55Z

src/realm/sync/protocol.hpp

@@ -60,6 +60,9 @@ namespace sync {
 //   13 Support for syncing collections (lists and dictionaries) in Mixed columns and
 //      collections of Mixed
 //
+//   14 Support for server initiated bootstraps, including bootstraps for role/


you need to update get_current_protocol_version() to return the new version

I was waiting until this feature was about to be merged, but it really doesn't matter, since I'm using a feature branch and the feature is behind a feature flag.

…o mwb/role-change-bootstraps

…e change test

…o mwb/role-change-bootstraps

danieltabacaru · 2024-06-07T21:28:52Z

test/object-store/sync/flx_sync.cpp

+
+    auto setup_harness = [&](FLXSyncTestHarness& harness, TestParams params) {
+        auto& app_session = harness.session().app_session();
+        /** TODO: Remove when switching to use Protocol version in RCORE-1972 */


you're doing it as part of this pr.

But the server hasn't been updated to use the protocol version yet... I believe it is still behind the feature flag

Yes - it is just controlled by a feature flag

danieltabacaru · 2024-06-07T21:34:36Z

test/object-store/sync/flx_sync.cpp

+
+        // Add client reset callback to verify a client reset doesn't happen
+        config.sync_config->notify_before_client_reset = [&](std::shared_ptr<Realm>) {
+            did_client_reset = true;


nit: FAIL() (and no need for did_client_reset)

…o mwb/role-change-bootstraps

cla-bot bot added the cla: yes label Mar 8, 2024

github-actions bot assigned michael-wb Mar 8, 2024

michael-wb changed the title ~~Updated bootstrap store to handle server initiated bootstraps~~ RCORE-1872 Sync client should allow server bootstrapping at any time Mar 8, 2024

Michael Wilkerson-Barker added 2 commits March 27, 2024 10:13

First round of changes for server-initiated bootstraps

d09e4a3

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

ae2574e

…o mwb/role-change-bootstraps

michael-wb force-pushed the mwb/role-change-bootstraps branch from 7ec0cc0 to ae2574e Compare April 3, 2024 19:07

Michael Wilkerson-Barker added 6 commits April 9, 2024 23:59

Added test for role change bootstraps

43391ca

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

35b1483

…o mwb/role-change-bootstraps

Updated test for handle role bootstraps

3424431

Updated baas/baasaas to use branch with fixes

e0d6ac1

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

f3eb7e0

…o mwb/role-change-bootstraps

updated changelog

7356ba2

This was linked to issues Apr 12, 2024

Bump sync protocol version to v14 #7326

Closed

Sync client should allow server bootstrapping at any time #7584

Closed

michael-wb marked this pull request as ready for review April 12, 2024 00:57

michael-wb requested review from danieltabacaru and jbreams April 12, 2024 00:57

Michael Wilkerson-Barker added 4 commits April 12, 2024 09:18

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

9e32480

…o mwb/role-change-bootstraps

Updated test to verify bootstrap actually occurred

4431985

Fixed tsan warning

b87bc65

Move instead of copy

8447461

jbreams reviewed Apr 15, 2024

View reviewed changes

src/realm/object-store/sync/sync_session.cpp Outdated Show resolved Hide resolved

src/realm/sync/noinst/client_impl_base.cpp Outdated Show resolved Hide resolved

Michael Wilkerson-Barker added 2 commits April 15, 2024 17:11

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

75a1d13

…o mwb/role-change-bootstraps

Updates from review; added comments to clarify bootstrap detection logic

44d8e12

michael-wb requested a review from jbreams April 16, 2024 13:12

jbreams reviewed Apr 18, 2024

View reviewed changes

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

944fb30

…o mwb/role-change-bootstraps

danieltabacaru reviewed Apr 22, 2024

View reviewed changes

src/realm/sync/noinst/pending_bootstrap_store.cpp Outdated Show resolved Hide resolved

danieltabacaru reviewed Apr 24, 2024

View reviewed changes

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

a9807d3

…o mwb/role-change-bootstraps

Michael Wilkerson-Barker added 4 commits June 6, 2024 11:25

Updated role change test to use test commands

83d2a5c

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

5f2a7e1

…o mwb/role-change-bootstraps

Fixed lint warning

51b54eb

Update resume and ident message handling

93386d4

jbreams reviewed Jun 6, 2024

View reviewed changes

Michael Wilkerson-Barker added 4 commits June 6, 2024 17:26

Updated future waits for the pause/resume test command

7e2c5a7

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

b2fcf51

…o mwb/role-change-bootstraps

Added session connected event for when session multiplexing is disabled

239d162

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

841f1d8

…o mwb/role-change-bootstraps

danieltabacaru reviewed Jun 7, 2024

View reviewed changes

Michael Wilkerson-Barker added 2 commits June 7, 2024 10:14

Updates from review; updated baas commit to include timing fix

0f00b92

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

1ffddd0

…o mwb/role-change-bootstraps

michael-wb requested review from jbreams and danieltabacaru June 7, 2024 15:26

Michael Wilkerson-Barker added 3 commits June 7, 2024 12:29

Removed todo comment

04624e9

Added wait_until() to state machine to wait for callback; updated rol…

84a4c23

…e change test

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

3e97840

…o mwb/role-change-bootstraps

danieltabacaru reviewed Jun 7, 2024

View reviewed changes

Michael Wilkerson-Barker added 4 commits June 7, 2024 18:13

Updates from review

c4e6a9e

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

3649284

…o mwb/role-change-bootstraps

Updated changelog after release

aede51b

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

0d55022

…o mwb/role-change-bootstraps

michael-wb requested a review from danieltabacaru June 10, 2024 14:58

danieltabacaru approved these changes Jun 11, 2024

View reviewed changes

Merge branch 'feature/role-change' of github.com:realm/realm-core int…

a40bafc

…o mwb/role-change-bootstraps

jbreams approved these changes Jun 11, 2024

View reviewed changes

michael-wb merged commit a988773 into feature/role-change Jun 11, 2024
35 of 39 checks passed

michael-wb deleted the mwb/role-change-bootstraps branch June 11, 2024 17:13

michael-wb linked an issue Jun 20, 2024 that may be closed by this pull request

Bump sync protocol version to v14 #7326

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RCORE-1872 Sync client should allow server bootstrapping at any time #7440

RCORE-1872 Sync client should allow server bootstrapping at any time #7440

michael-wb commented Mar 8, 2024 •

edited

Loading

jbreams Apr 18, 2024

michael-wb Apr 19, 2024

jbreams Apr 18, 2024

michael-wb Apr 19, 2024

mpobrien May 2, 2024

michael-wb May 3, 2024

jbreams Apr 18, 2024

jbreams Apr 18, 2024

michael-wb Apr 19, 2024

jbreams Apr 18, 2024

michael-wb Apr 19, 2024

danieltabacaru Apr 22, 2024

michael-wb Apr 30, 2024

danieltabacaru Apr 24, 2024

michael-wb Apr 26, 2024

danieltabacaru Apr 30, 2024

michael-wb Apr 30, 2024

danieltabacaru May 9, 2024

jbreams left a comment

jbreams Jun 6, 2024

michael-wb Jun 6, 2024

danieltabacaru Jun 7, 2024

michael-wb Jun 7, 2024

danieltabacaru Jun 7, 2024 •

edited

Loading

michael-wb Jun 7, 2024

jbreams Jun 7, 2024

michael-wb Jun 7, 2024

jbreams Jun 7, 2024

danieltabacaru Jun 7, 2024

michael-wb Jun 7, 2024

danieltabacaru Jun 7, 2024

michael-wb Jun 7, 2024

michael-wb Jun 7, 2024

danieltabacaru Jun 7, 2024 •

edited

Loading

RCORE-1872 Sync client should allow server bootstrapping at any time #7440

RCORE-1872 Sync client should allow server bootstrapping at any time #7440

Conversation

michael-wb commented Mar 8, 2024 • edited Loading

What, How & Why?

☑️ ToDos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbreams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

michael-wb commented Mar 8, 2024 •

edited

Loading

danieltabacaru Jun 7, 2024 •

edited

Loading

danieltabacaru Jun 7, 2024 •

edited

Loading