[SERVER-50002] ReplicaSetAwareService test should just use dummy services Created: 29/Jul/20  Updated: 29/Oct/23  Resolved: 12/Aug/20

Status: Closed
Project: Core Server
Component/s: Sharding
Affects Version/s: None
Fix Version/s: 4.7.0

Type: Bug Priority: Major - P3
Reporter: Pierlauro Sciarelli Assignee: Kevin Pulo
Resolution: Fixed Votes: 0
Labels: PM-1645-Milestone-3
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
is depended on by SERVER-49193 Recover the VectorClock on shard prim... Closed
Related
related to SERVER-46201 Implement a generic ReplicaSetStateAw... Closed
Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Sharding 2020-08-24
Participants:
Linked BF Score: 0

 Description   

The ReplicaSetAwareService test is mistakenly ending up calling the VectorClockMongoD onStepUpComplete.

So far the test has always worked because such method call always resulted in a NOP, but that will not be the case anymore with the changes that are going to be introduced in SERVER-49193.



 Comments   
Comment by Githook User [ 12/Aug/20 ]

Author:

{'name': 'Kevin Pulo', 'email': 'kevin.pulo@mongodb.com', 'username': 'devkev'}

Message: SERVER-50002 fix ReplicaSetAwareService unittest linking
Branch: master
https://github.com/mongodb/mongo/commit/8ddfcea4915e55fe85349b7ed792abd0d41d075d

Comment by Githook User [ 12/Aug/20 ]

Author:

{'name': 'Kevin Pulo', 'email': 'kevin.pulo@mongodb.com', 'username': 'devkev'}

Message: SERVER-50002 separate unittest binary for ReplicaSetAwareService
Branch: master
https://github.com/mongodb/mongo/commit/1f91497779392e1dae31904639b40e43d77c75bd

Comment by Kaloian Manassiev [ 31/Jul/20 ]

kevin.pulo, passing this to you, since Pierlauro will be on vacation in the next couple of weeks.

Comment by Andrew Morrow (Inactive) [ 31/Jul/20 ]

kevin.pulo - It is fine to add a new CppUnitTest with fewer dependencies if you must. Collapsing the unit test libraries was a desperation move in the first place, when we were still on the very slow VS2017 linker and when we didn't have --link-model=dynamic in the required waterfall and patch builders. Yes, adding the new binary will cost us something, but most likely not enough to adversely affect developer experience in a noticeable way.

Comment by Kevin Pulo [ 31/Jul/20 ]

Argh, it's via db_repl_coordinator_test -> repl_coordinator_test_fixture -> db/service_context_d_test_fixture -> db/service_context_d -> db/s/sharding_runtime_d -> db/vector_clock_mongod. And a similar path exists for the other existing CppUnitTests under db/repl, topology_version_observer_test and db_repl_cloners_test.

Prior to SERVER-48924 the test was in db_unittests, but it had the same problem there:

kev@devkev-4:~/g/mongodb/mongodb/mongo-build$ ldd build/install/bin/db_repl_test | grep vector_clock
        libvector_clock_mongod.so => /home/kev/g/mongodb/mongodb/mongo-build/build/install/bin/../lib/libvector_clock_mongod.so (0x00007fe8ef789000)
        libvector_clock_mutable.so => /home/kev/g/mongodb/mongodb/mongo-build/build/install/bin/../lib/libvector_clock_mutable.so (0x00007fe8e9e18000)
        libvector_clock.so => /home/kev/g/mongodb/mongodb/mongo-build/build/install/bin/../lib/libvector_clock.so (0x00007fe8e927f000)
kev@devkev-4:~/g/mongodb/mongodb/mongo-build$ ldd build/install/bin/db_repl_coordinator_test | grep vector_clock
        libvector_clock_mongod.so => /home/kev/g/mongodb/mongodb/mongo-build/build/install/bin/../lib/libvector_clock_mongod.so (0x00007f7548053000)
        libvector_clock_mutable.so => /home/kev/g/mongodb/mongodb/mongo-build/build/install/bin/../lib/libvector_clock_mutable.so (0x00007f754405c000)
        libvector_clock.so => /home/kev/g/mongodb/mongodb/mongo-build/build/install/bin/../lib/libvector_clock.so (0x00007f754286b000)
kev@devkev-4:~/g/mongodb/mongodb/mongo-build$ ldd build/install/bin/db_unittests | grep vector_clock
        libvector_clock_mongod.so => /home/kev/g/mongodb/mongodb/mongo-build/build/install/bin/../lib/libvector_clock_mongod.so (0x00007f5d31d6c000)
        libvector_clock_mutable.so => /home/kev/g/mongodb/mongodb/mongo-build/build/install/bin/../lib/libvector_clock_mutable.so (0x00007f5d2b86b000)
        libvector_clock.so => /home/kev/g/mongodb/mongodb/mongo-build/build/install/bin/../lib/libvector_clock.so (0x00007f5d2acd2000)

So it looks like we need a "plain" unittest binary, that doesn't inherit any of the *_d libs — particularly not db/s/sharding_runtime_d, db/service_context_d, or db/service_context_d_test_fixture (though db/service_context and db/service_context_test_fixture will be needed). Since currently there doesn't seem to be any such CppUnitTest, we'll probably have to create it (client_out_of_line_executor_test appears to be precedent, though no clues explaining why it needs to be on its own). But since we try to minimise the number of separate binaries, we should check with the SDP Build team first. acm, thoughts?

The only other possibility I can think of would be to remove the automatic registration of ReplicaSetAwareServices, and instead require manual registration during mongod/mongos/test fixture startup. The problem here is that since ServiceContext decorations are MONGO_INITIALIZER-based, this still might not allow different tests to have different ReplicaSetAwareServices...

Comment by Pierlauro Sciarelli [ 30/Jul/20 ]

Unfortunately, even moving it under db_repl_coordinator_test doesn't work since vector_clock_mongod is somehow indirectly linked also there. EVG patch showing that the test still fails.

Comment by Kevin Pulo [ 30/Jul/20 ]

So sharding_runtime_d is the only lib that vector_clock_mongod is linked to. In db/repl/, that lib (sharding_runtime_d) is only linked by rs_rollback, rollback_impl, oplog_application, and serveronly_repl. Of these 4, the db_repl_test unittest binary links directly to rs_rollback and rollback_impl, and to oplog_application via oplog_applier_impl_test_fixture. So this is how vector_clock_mongod is getting into db_repl_test, and it seems like we won't be able to break that link.

The only places in db/repl/ that link replica_set_aware_service are repl_coordinator_impl, primary_only_service, and db_repl_test. primary_only_service_test.cpp is also in db_repl_test, so we can't move replica_set_aware_service_test.cpp to be alongside it. However, since the main consumer of replica_set_aware_service is repl_coordinator_impl, perhaps we can move replica_set_aware_service_test.cpp (and the corresponding replica_set_aware_service LIBDEP) out of db_repl_test, and instead into db_repl_coordinator_test? That unittest seems to have a deliberately limited set of libs, so should allow replica_set_aware_service_test.cpp to run without vector_clock_mongod.

Comment by Kaloian Manassiev [ 30/Jul/20 ]

Is this a problem, because the VectorClock happens to be linked in ReplicaSetAwareService test binary. Most likely, it needs to be pulled into its own binary unfortunately. CC kevin.pulo.

Generated at Thu Feb 08 05:21:27 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.