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

GODRIVER-2292 Support dedicated load balancer port in tests. #852

Merged
merged 1 commit into from Feb 17, 2022

Conversation

matthewdale
Copy link
Collaborator

@matthewdale matthewdale commented Feb 14, 2022

GODRIVER-2292

DRIVERS-1983 updated the load balancer mongo-orchestration config to support the new "loadBalancerPort" server option which allows the server to act more correctly in load balancer tests, including returning a "serviceID" in "hello" responses. Update the test runner to pass the new LOAD_BALANCER="true" option to mongo-orchestration and remove support for mocking "serviceID".

Changes:

  • Set LOAD_BALANCER="true" and pass LOAD_BALANCER value to mongo-orchestration.
  • Remove "serviceID" mocking.
  • Skip some load-balanced initial DNS seedlist discovery tests until GODRIVER-2312 is done.

@matthewdale matthewdale force-pushed the godriver2292-add-lb-port branch 7 times, most recently from 76f6ddf to e1e1996 Compare February 16, 2022 19:23
@matthewdale matthewdale marked this pull request as ready for review February 16, 2022 19:23
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for filing GODRIVER-2312.

# The new "loadBalancerPort" option is supported starting with server 5.2, which responds
# correctly to "hello" commands with a service ID when behind a load balancer. Only run load
# balancer tests on server 5.2+.
# TODO(GODRIVER-2292): Add version "5.2" when it's available on Evergreen.
Copy link
Contributor

Choose a reason for hiding this comment

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

You may be able to remove this TODO. Alternatively, add rapid to the version list.

From this slack conversation I interpret the intent to be that drivers to test against the following set of server versions:

  • Supported major releases (for Go: 2.6, 3.0, 3.4, 3.6, 4.0, 4.2, 4.4, 5.0)
  • The latest build from evergreen (e.g. 5.3.0-alpha...)
  • The latest rapid release (currently 5.2 only, not 5.1).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. It sounds like ideally we'd want all Evergreen tasks that test against latest to probably also test against rapid. You could separate that work into another ticket and leave a relevant TODO here if that seems overboard for this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I didn't know that existed. It's weird that "rapid" refers to version 5.2, but using the literal version string "5.2" results in a configuration error. At any rate, that sounds like a good change, so I'll add "rapid" to the list.

I'll also create a ticket to update all tasks that use "latest" to also use "rapid".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created ticket GODRIVER-2316 to add "rapid" version to test matrix.

Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

LGTM, just left a comment about rapid.

# The new "loadBalancerPort" option is supported starting with server 5.2, which responds
# correctly to "hello" commands with a service ID when behind a load balancer. Only run load
# balancer tests on server 5.2+.
# TODO(GODRIVER-2292): Add version "5.2" when it's available on Evergreen.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. It sounds like ideally we'd want all Evergreen tasks that test against latest to probably also test against rapid. You could separate that work into another ticket and leave a relevant TODO here if that seems overboard for this PR.

@@ -116,7 +125,7 @@ func runSeedlistDiscoveryTest(mt *mtest.T, file string) {
}
for _, host := range test.Hosts {
_, err := getServerByAddress(host, topo)
assert.Nil(mt, err, "did not find host %v", host)
assert.Nil(mt, err, "error finding host %q: %v", host, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants