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

RCORE-1928 Use baasaas for baas integration tests in CI #7423

Merged
merged 33 commits into from Mar 12, 2024
Merged

Conversation

jbreams
Copy link
Contributor

@jbreams jbreams commented Mar 6, 2024

What, How & Why?

This updates the baas integration tests in CI to use baas managed via baasaas rather than spinning up an ubuntu machine via host.create.

☑️ ToDos

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

Copy link

coveralls-official bot commented Mar 6, 2024

Pull Request Test Coverage Report for Build jonathan.reams_3083

Details

  • 201 of 291 (69.07%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on jbr/baasaas at 91.027%

Changes Missing Coverage Covered Lines Changed/Added Lines %
test/object-store/util/sync/sync_test_utils.cpp 6 23 26.09%
test/object-store/util/sync/baas_admin_api.cpp 187 260 71.92%
Totals Coverage Status
Change from base Build 2121: 91.0%
Covered Lines: 238243
Relevant Lines: 261727

💛 - Coveralls

@jbreams jbreams changed the title Jbr/baasaas RCORE-1928 Use baasaas for baas integration tests in CI Mar 6, 2024
@jbreams jbreams marked this pull request as ready for review March 6, 2024 22:02
@jbreams jbreams requested review from mpobrien and ironage March 6, 2024 22:02
Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good!

return {};
}

static std::string unquote_string(std::string_view possibly_quoted_string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this a public shared utility function from sync_test_utils.hpp instead of copying it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


~Baasaas()
{
stop();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does baasaas have a TTL or something just in case our tests on PRs crash and leave a bunch of containers running?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they'll get reaped automatically after about 60 minutes.
Ideally, it would be great to guarantee these get cleaned up even if the test process itself dies. Maybe we could have the setup write the container ID to a file that the evergreen task could check for in the "post" task, or something like that. Seems like this should be relatively uncommon though so I think it would be fine to just revisit this later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a lock file to this type so it will create a file called baasaas_instance.lock in the working directory of the tests and remove it after the instance is stopped. I also added the ability to specify a container ID so the tests can still interact with baasaas to get their base urls and stuff, but won't tear down the container if it was started manually to make this easier to work with the baasaas cli script.

{
app::Request request;
request.url = util::format(
"https://us-east-1.aws.data.mongodb-api.com/app/baas-container-service-autzb/endpoint/%1", api_path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you leave a comment about who owns this app and if there is any documentation about the endpoints?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. I've also made it so we can override this via an env variable.

@@ -977,18 +989,17 @@ tasks:
test_label: objstore-local
test_executable_name: "realm-object-store-tests"
verbose_test_output: true
disable_tests_against_baas: true
- func: "check branch state"

# These are baas object store tests that run against baas running on a remote host
- name: baas-integration-tests
tags: [ "test_suite", "for_pull_requests", "requires_baas" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit confusing to have both the disable_tests_against_baas var and requires_baas. Can we have only one of these for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately no, requires_baas is a tag rather than an expansion which tells evergreen which tasks to run and disable_tests_against_baas is an expansion used in commands to tell them how to compile the object-store tests.

- func: "check branch state"

# These are baas object store tests that run against baas running on a remote host
- name: baas-integration-tests
tags: [ "test_suite", "for_pull_requests", "requires_baas" ]
exec_timeout_secs: 3600
commands:
- func: "launch remote baas"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to eventually get rid of some of the launch/wait shell scripts, but I guess we'd have to wait to port over the network tests to this as well. Is there a viable path to doing this with baasaas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was gonna do that in a separate PR.

@jbreams jbreams requested a review from ironage March 12, 2024 15:56
@ironage
Copy link
Contributor

ironage commented Mar 12, 2024

Can you double check this run, it looks like a new kind of failure to me?

[2024/03/12 09:00:36.610] 4: /System/Volumes/Data/data/mci/afc16f9a9d450c759de0ef5d7a93bcbb/realm-core/test/object-store/util/sync/baas_admin_api.cpp:724: [realm-core-14.1.0] Assertion failed: resp.http_status_code >= 200 && resp.http_status_code < 300 with (util::format("url: %1 request: %2, reply: %3", m_url, body.dump(), resp.body)) =  ["url: http://18.212.184.129:32875/api/admin/v3.0/groups/65f07c16b1f6f93c1205954f/apps/65f07c22b1f6f93c120597bd/services/65f07c24b1f6f93c120597ee/config request: {\"sync\":{\"database_name\":\"test_data_c_api_client_reset_tests_65f07c22dbdf61a7bc02b837\",\"is_recovery_mode_disabled\":false,\"partition\":{\"key\":\"realm_id\",\"permissions\":{\"read\":true,\"write\":true},\"required\":false,\"type\":\"string\"},\"state\":\"enabled\"}}, reply: {\"error\":\"failed sync post-commit hook: error initializing stores: error ensuring indexes on \\\"__realm_sync.session_tracker\\\": (DatabaseDropPending) Cannot create collection __realm_sync.session_tracker - database is in the process of being dropped.\"}"]

Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once CI succeeds, or the failures are investigated and appear unrelated.

@jbreams
Copy link
Contributor Author

jbreams commented Mar 12, 2024

@mpobrien, I think the failure @ironage is calling out is known? i remember talking to you about failed sync post-commit hook: error initializing stores: error ensuring indexes on \\\"__realm_sync.session_tracker\\\": (DatabaseDropPending) Cannot create collection __realm_sync.session_tracker - database is in the process of being dropped.

@mpobrien
Copy link

mpobrien commented Mar 12, 2024

https://jira.mongodb.org/browse/BAAS-28824 should resolve that issue - it's due to the interaction between an optimization to drop the DB when terminating sync (when there's only one app in the DB) and mongodb's restriction that you can't modify collections or indexes when DB drops are in progress.
As a workaround until https://jira.mongodb.org/browse/BAAS-28824 is in, you could just create a second app in the backing DB and leave it there for the duration of the entire test suite. This would prevent the optimization from kicking in, which should eliminate that failure mode.
https://jira.mongodb.org/browse/BAAS-28824 is unfortunately not the quick win I thought it would be as it requires threading a flag down into some hard-to-reach spots, but I will try to prioritize it soon if there's no other viable way around it for now

@jbreams
Copy link
Contributor Author

jbreams commented Mar 12, 2024

@ironage and @mpobrien , could I get a quick review on the last commit? I updated the how-to-build.md file to reflect using baasaas and I made it so we create the "default" app on process-launch rather than first use so we'll always have a pbs app around to try to prevent the database is in the process of being dropped problem. I also updated the changelog.


Due to MongoDB security policies, running baas requires company issued credentials.
Copy link

@mpobrien mpobrien Mar 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be worth also pointing readers to https://wiki.corp.mongodb.com/display/10GEN/%28Device+Sync%29+Using+Docker+to+run+a+BAAS+server+instance and encouraging that workflow to be used for local development (the container is identical to what you get from launching one on baasaas). Baasaas should be reserved mostly for CI tasks.

how-to-build.md Outdated
You can tell the object-store tests to use a specific version of baas with the
`BAASAAS_START_MODE` environment variable, which can either be `githash`, `patchid`,
or `branch`. If you specify a start mode, you need to tell it which githash or
branch name to start with via the `BAASAAS_REF_SPEC` environment variable. Ommitting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Ommitting -> Omitting

Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jbreams jbreams merged commit 666bb25 into master Mar 12, 2024
35 of 37 checks passed
@jbreams jbreams deleted the jbr/baasaas branch March 12, 2024 23:18
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants