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

DRIVERS-2224 Fix load-balanced DNS seedlist discovery tests for new dedicated lb port. #1148

Merged

Conversation

matthewdale
Copy link
Contributor

@matthewdale matthewdale commented Feb 16, 2022

Currently the mongo-orchestration changes made with DRIVERS-1983 break some initial DNS seedlist discovery load-balanced tests because the driver must now connect to the load balancer to work correctly when in load-balanced mode. See this comment for more details about the test failures.

BUILD-14668 adds two new SRV records:

  • _mongodb._tcp.test23.test.build.10gen.cc. 86400 IN SRV 8000 localhost.test.build.10gen.cc.
  • _mongodb._tcp.test24.test.build.10gen.cc. 86400 IN SRV 8000 localhost.test.build.10gen.cc.

and one new TXT record:

  • test24.test.build.10gen.cc. 86400 IN TXT "loadBalanced=true"

This PR updates the initial-dns-seedlist-discovery/load-balancer spec tests to use those new recods to work with the new/updated DNS SRV records and document the SRV record updates in the README.

Note! This PR MUST NOT be merged until BUILD-14668 is complete.

@jmikola
Copy link
Member

jmikola commented Feb 16, 2022

@matthewdale: I realized I recently touched these tests in #1069 for DRIVERS-1519, but we don't actually run these tests in PHP so I was working blind there.

@benjirewis: Do you have bandwidth to review this in my place?

@benjirewis benjirewis requested review from benjirewis and removed request for jmikola February 17, 2022 16:02
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.

I think this LGTM. Let me know if my understanding in the comment below is correct.

Let's not merge these changes until BUILD-14668 is done, and you're able to verify that these tests now pass in the Go driver (i.e. GODRIVER-2312 is done).

the tests. The missing ``test.`` sub-domain in the SRV record target for
``test12`` is deliberate. ``test22`` is used to test a custom service name
(``customname``).
Notes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning up.

_mongodb._tcp.test21.test.build.10gen.cc. 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_customname._tcp.test22.test.build.10gen.cc 86400 IN SRV 27017 localhost.test.build.10gen.cc
_customname._tcp.test22.test.build.10gen.cc 86400 IN SRV 27017 localhost.test.build.10gen.cc.
_mongodb._tcp.test23.test.build.10gen.cc. 86400 IN SRV 8000 localhost.test.build.10gen.cc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to clarify my understanding. From offline conversation, it sounds like load balancer tests make use of the test1, test3, test4 and test20 records. The test1 and test4 records seem to be used to test that drivers fail during validation, so updating the port from 27017 (the mongos) to 8000 (the actual LB) is not necessary. The test3 record is used to test a successful connection, so the port should be updated, but that record is also used in the load-balanced/srvMaxHosts-zero test which expects to find 27017. So, we need a new test23 record that is identical to test3 but points to port 8000. Let me know if that all sounds right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct (the test3 record is currently used in various replica-set tests, but you're right that it's required to stay the same for other tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, update: This change is now strictly additive. There are two new records, test23 (replaces test3) and test24 (replaces test20).

@matthewdale matthewdale force-pushed the drivers1983-fix-dns-seed-lb-test branch from fbd6890 to 2661836 Compare February 25, 2022 21:33
@matthewdale matthewdale changed the title DRIVERS-1983 Use lb port for lb DNS seedlist tests and add DNS record. DRIVERS-1983 Add two new DNS SRV records for lb DNS seedlist tests pointing at the lb port. Feb 25, 2022
@matthewdale matthewdale force-pushed the drivers1983-fix-dns-seed-lb-test branch from 2661836 to 0b11a20 Compare February 25, 2022 21:48
@matthewdale matthewdale changed the title DRIVERS-1983 Add two new DNS SRV records for lb DNS seedlist tests pointing at the lb port. DRIVERS-2224 Add two new DNS SRV records for lb DNS seedlist tests pointing at the lb port. Mar 2, 2022
@matthewdale matthewdale force-pushed the drivers1983-fix-dns-seed-lb-test branch from 0b11a20 to 475971e Compare March 2, 2022 18:13
@matthewdale matthewdale changed the title DRIVERS-2224 Add two new DNS SRV records for lb DNS seedlist tests pointing at the lb port. DRIVERS-2224 Fix load-balanced DNS seedlist discovery tests for new dedicated lb port. Mar 2, 2022
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

Thanks!

@durran durran merged commit 5d1b42a into mongodb:master Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants