Add BAAS Admin API command to start/revert PBS->FLX server migration #6366

michael-wb · 2023-03-07T01:33:30Z

What, How & Why?

Add a functions to the admin api test interface for:

triggering the PBS->FLX migration and rollback operations
checking the status of the migration/rollback

Add a test to verify the operation and demonstrate/test the operation of the new admin api functions.

Fixes #6343

☑️ ToDos

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

michael-wb · 2023-03-13T04:45:47Z

The failure on Jenkins is due to receiving an empty changeset for the bootstrap data after the migration to FLX has occurred. I posted a note about this on the PBS FLX migration channel.

danieltabacaru · 2023-03-13T09:02:08Z

CHANGELOG.md

@@ -63,6 +63,7 @@
 * Add CAPI test for Binding Callback Thread Observer. ([PR #6156](https://github.com/realm/realm-core/pull/6156))
 * Implement MigrationStore to support migration from PBS to FLX ([PR #6324](https://github.com/realm/realm-core/pull/6324))
 * Removed overloads of `Session::bind()` that allow binding a sync session to server other than the one configured in `Session::Config` ([PR #6358](https://github.com/realm/realm-core/pull/6358)).
+* Add admin api and test for performing the PBS->FLX migration and roll back on the server. (PR [#6366](https://github.com/realm/realm-core/pull/6366))


rebase this because master was released

Thanks - I did a rebase and it still put it in the wrong place - fixed

danieltabacaru · 2023-03-13T09:28:40Z

test/object-store/util/baas_admin_api.hpp

+        bool complete = false;
+    };
+
+    MigrationStatus get_migration_status(const std::string& app_id) const;


What happens if this is called without a migration in place?

If a migration is not currently active, it will return a MigrationStatus with an empty status message and the isMigrated value will reflect the current migration state.

danieltabacaru · 2023-03-13T09:29:16Z

test/object-store/util/baas_admin_api.hpp

@@ -119,6 +120,17 @@ class AdminAPISession {
    bool is_sync_terminated(const std::string& app_id) const;
    bool is_initial_sync_complete(const std::string& app_id) const;

+    struct MigrationStatus {
+        std::string statusMessage;
+        std::string errorMessage;


nit: should this be optional?

Good point - made it optional

danieltabacaru · 2023-03-13T09:30:36Z

test/object-store/sync/flx_migration.cpp

@@ -0,0 +1,200 @@
+#include <catch2/catch_all.hpp>
+#include <chrono>


nit: add this after the realm includes

danieltabacaru · 2023-03-13T09:31:12Z

test/object-store/sync/flx_migration.cpp

+#include <catch2/catch_all.hpp>
+#include <chrono>
+
+#include "flx_sync_harness.hpp"


nit: I would use full paths and angled brackets

Updated the includes for the files in the src/ directory to use angled brackets. For the include files in the test/ directory, I updated them to use the appropriate path for the include file (similar to the other files in test/object-store/)

danieltabacaru · 2023-03-13T09:46:15Z

test/object-store/sync/flx_migration.cpp

+                status = app_session.admin_api.get_migration_status(app_session.server_app_id);
+                if (logger && last_status != status.statusMessage) {
+                    last_status = status.statusMessage;
+                    logger->debug("PBS->FLX Migration status: %1", last_status);


we could also be waiting for the status of a rollback ("FLX->PBS....")

Updated the PBS->FLX string to be dynamic based on the operation being performed (i.e. PBS->FLX if migrate_to_flx is true, otherwise FLX->PBS)

danieltabacaru · 2023-03-13T09:47:09Z

test/object-store/sync/flx_migration.cpp

+    REQUIRE(switch_to_flx == status.isMigrated);
+    if (logger) {
+        if (!status.errorMessage.empty())
+            logger->debug("PBS->FLX Migration returned error: %1", status.errorMessage);


same comment as above

danieltabacaru · 2023-03-13T09:52:08Z

test/object-store/sync/flx_migration.cpp

+    REQUIRE(status.errorMessage.empty());
+}
+
+// Populates a FLXSyncTestHarness with the g_large_array_schema with objects that are large enough that


This comment is not accurate

danieltabacaru · 2023-03-13T11:06:05Z

test/object-store/sync/flx_migration.cpp

+    trigger_server_migration(session.app_session(), false, logger_ptr);
+
+    {
+        SyncTestFile pbs_config(session.app(), partition1, server_app_config.schema);


we should check partition2 too.

danieltabacaru · 2023-03-13T11:19:31Z

test/object-store/sync/flx_migration.cpp

+    trigger_server_migration(session.app_session(), true, logger_ptr);
+
+    {
+        SyncTestFile flx_config(session.app()->current_user(), server_app_config.schema,


I am not sure I understand this test. Shouldn't the user connect as PBS to trigger the migration on the client?
As it stands, it's a native flx user which connects and gets the pbs data (still valuable). In this case, it should create "normal" subscriptions then (a subscription on Dog class without the partition string).

Edit: I think I finally understand the test: it's how the client reconnects after the migration is triggered on the client (as flx and creates subscriptions using the partition key(s)). I still think we should have a test with a native flx user which didn't go through the migration (my second sentence above).

This test is less about using the client migration feature and more about validating being able to trigger a migration on the server and connecting to it as PBS (in not migrated state) or FLX (in migrated state). The FLX subscriptions I created are representative of the subscriptions that will be automatically generated.

We will need to add a separate test to verify the client migration, once it is complete.

danieltabacaru · 2023-03-13T12:50:45Z

The failure on Jenkins is due to receiving an empty changeset for the bootstrap data after the migration to FLX has occurred. I posted a note about this on the PBS FLX migration channel.

There is another failure, a build warning:
catching polymorphic type 'class std::runtime_error' by value [-Wcatch-value=] at flx_migration.cpp:41

danieltabacaru

LGTM

michael-wb · 2023-03-14T21:33:49Z

Kiro has a BAAS update https://github.com/10gen/baas/pull/8980 that will address the 0 == 5 failure when checking the table size after the PBS->FLX migration has occurred.

…on-admin-api

jbreams

Just some nits/questions.

jbreams · 2023-03-15T15:43:03Z

test/object-store/sync/flx_migration.cpp

+
+using namespace realm;
+
+static void trigger_server_migration(const AppSession& app_session, bool switch_to_flx,


since we never call this with a non-null logger parameter, let's just remove the default nullptr value.

My goal was to make it optional in case future tests didn't use a logger.

We def shouldn't do this in this PR, but I kinda think we should make it so there's always a logger available in tests. Like when you create the TestAppSession or something you get a logger that's properly configured.

I agree, similar to the logger in TestContext for the core-and sync-tests. I'll create a task and do that separately.

jbreams · 2023-03-15T15:43:44Z

test/object-store/sync/flx_migration.cpp

+}
+
+// Add a set of count number of Dog objects to the realm
+static std::vector<ObjectId> fill_test_data(SyncTestFile& config, std::string partition, int start = 1, int count = 5)


since we never call these without specifying the values for start/count, let's remove their default values.

jbreams · 2023-03-15T15:47:15Z

test/object-store/sync/flx_migration.cpp

+    // Add some objects with the provided partition value
+    for (int i = 0; i < count; i++, ++start) {
+        auto id = ObjectId::gen();
+        auto obj = Object::create(c, realm, "Dog",


This is a nit. Feel free to ignore me here 😄, but i kinda don't like re-using the "Dog" classes from the app integration tests. Can we name this a non-silly name like Object and have non-fake properties like "breed"?

Sure, I was using the default schema that was included in default_app_config()

jbreams · 2023-03-15T15:49:09Z

test/object-store/util/baas_admin_api.cpp

+        try {
+            return progress_endpoint.get_json();
+        }
+        catch (const std::exception& e) {


This is a nit, but shouldn't we just let this exception get thrown so we get a stacktrace in the test logs?

I was trying to catch the case where the http status was not between 200-299, but it is actually a terminate, which can't be caught, so I'll put it back.

jbreams · 2023-03-15T15:49:20Z

test/object-store/util/baas_admin_api.cpp

+    }();
+    auto errorMessage = progress_result["errorMessage"];
+    if (errorMessage.is_string() && !errorMessage.get<std::string>().empty()) {
+        status.errorMessage.emplace(errorMessage.get<std::string>());


Should we not just throw here and on 735?

Sure, I can change these to throw

…on-admin-api

jbreams · 2023-03-16T17:41:43Z

test/object-store/sync/flx_migration.cpp

+
+using namespace realm;
+
+static void trigger_server_migration(const AppSession& app_session, bool switch_to_flx,


We def shouldn't do this in this PR, but I kinda think we should make it so there's always a logger available in tests. Like when you create the TestAppSession or something you get a logger that's properly configured.

cla-bot bot added the cla: yes label Mar 7, 2023

github-actions bot assigned michael-wb Mar 7, 2023

michael-wb force-pushed the mwb/migration-admin-api branch from cb85651 to cafa320 Compare March 7, 2023 05:07

Michael Wilkerson-Barker added 4 commits March 11, 2023 23:21

Added flx migration admin api and test for migration and roll back

f25f612

Updated changelog

bf44023

Added migration status and error responses

e8b995e

Updated jenkins mongodb realm version

f041764

michael-wb force-pushed the mwb/migration-admin-api branch from cafa320 to f041764 Compare March 12, 2023 04:21

Michael Wilkerson-Barker added 6 commits March 12, 2023 11:05

Fix log level after merge

f960f3b

Updated test to wait for initial bootstrap

1d0ac2e

clang-format

829c594

Updated test

86a6893

More updates to test...

efdc6e9

Fixed tests

53a7a65

michael-wb marked this pull request as ready for review March 13, 2023 04:44

michael-wb requested review from jbreams and danieltabacaru March 13, 2023 04:45

danieltabacaru reviewed Mar 13, 2023

View reviewed changes

Updates from review

5ad4bec

danieltabacaru approved these changes Mar 13, 2023

View reviewed changes

Merge branch 'master' of github.com:realm/realm-core into mwb/migrati…

71a3b5f

…on-admin-api

jbreams reviewed Mar 15, 2023

View reviewed changes

Michael Wilkerson-Barker added 3 commits March 15, 2023 14:57

Updates from review

c661460

Merge branch 'master' of github.com:realm/realm-core into mwb/migrati…

1dfae93

…on-admin-api

Updated jenkins mongodb realm test docker

988d4a3

jbreams approved these changes Mar 16, 2023

View reviewed changes

michael-wb merged commit c1cd641 into master Mar 16, 2023

michael-wb deleted the mwb/migration-admin-api branch March 16, 2023 17:46

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

		@@ -0,0 +1,200 @@
		#include <catch2/catch_all.hpp>
		#include <chrono>


		using namespace realm;

		static void trigger_server_migration(const AppSession& app_session, bool switch_to_flx,

Add BAAS Admin API command to start/revert PBS->FLX server migration #6366

Add BAAS Admin API command to start/revert PBS->FLX server migration #6366

Uh oh!

Conversation

Uh oh!

What, How & Why?

☑️ ToDos

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!