-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Description
Title: Poor documentation (potential synchronization issue) around BaseIntegrationTest::pre_worker_start_test_steps_ variable
Description:
The BaseIntegrationTest data member pre_worker_start_test_steps_ is described @ https://github.com/envoyproxy/envoy/blob/master/test/integration/integration.h#L224 as "Steps that should be done prior to the workers starting. E.g., xDS pre-init." However, looking at the actual execution of those steps @ https://github.com/envoyproxy/envoy/blob/master/test/integration/server.cc#L77, there is no synchronization that guarantees either that a) the steps are run before the workers are started, or b) the steps are run after the server is actually created.
I spoke with @htuch offline, and he believes that pre_worker_start_test_steps_ is generally used for what I'd call "server initialization coroutines"; i.e. things in the control plane that server startup is dependent on, but which also block on server initialization. Such routines would need to be started after the server is created, but not blocking on server initialization being complete, with no other synchronization requirements. That is exactly the context in which those routines are run in the current test framework.
Given the above analysis, I'm going to take the lack of synchronization around executing that routine as being a lack of documentation, and treat this issue as being to update the name and documentation of that data member. But if anyone reading this issue thinks that pre_worker_start_test_steps_ has additional requirements for synchronization or is used for other work, they should comment and I'll expand the goals of this issue (and probably take it off my todo list :-}).
@alyssawilk for her test deflake work, in case this is relevant.