[benchmarker] Add virtual user cleanup - #1704
Conversation
| try: | ||
| _, _, query = await self.user.run_sync_client_call( | ||
| dss_instance.delete_op_intent, | ||
| id=op_intent_id, |
There was a problem hiding this comment.
Above it's self.op_intent_refs[flight.id] = op_intent_ref and there is passed as id for op_intent, is it correct, instead of op_intent_ref.id ?
There was a problem hiding this comment.
Wow, good catch -- the keys are definitely flight IDs rather than op intent IDs, so I'm going to have to look into how I was able to observe some op intents being successfully removed.
There was a problem hiding this comment.
I've fixed the ID confusion, but this issue still worries me. I ran single_s2_cell.jsonnet specifically to catch things like this and one of my points of verification was to note that I saw at least some messages like "FPU2_1's SCDHandler cleaned up 1 op intents" which I took to indicate that the cleanup code paths were being fully exercised. But, I don't see how I could have seen those messages before making the fix for this issue in 0685ec3 since there shouldn't have been any op intents with the IDs of flights. I'm attaching my console output for another run on ba28748 that also contains the expected "SCDHandler cleaned up N op intents" messages, but now I'm nervous I might not be interpreting that evidence correctly. Can you think of any way I could have seen that message before fixing this issue you pointed out, or must I be remembering wrong?
There was a problem hiding this comment.
Can you check in the report if the queries generated 404 or something else?
Because the code is treating those as success no? So a wrong ID won't be visible
| logger.info(f"Cleaning up {len(virtual_users)} virtual users...") | ||
| cleanup_start = datetime.now(UTC) | ||
| for virtual_user in virtual_users: | ||
| await virtual_user.cleanup() |
There was a problem hiding this comment.
Nit/question: one cleanup generating and error will prevent others ones to cleanup, is that on purpose?
There was a problem hiding this comment.
Yes, I think generally unhandled exceptions anywhere in a program should be raised quickly and loudly. And exceptions should only be handled when we're confident normal program execution can continue in a correct and consistent manner despite the exception. If we don't know what exception happened in cleanup, I don't think we could know we can continue in a correct and consistent manner. A QueryError is caught and handled in the SCDHandler cleanup so that shouldn't be a factor.
2 participants
Currently, benchmarker's virtual users are likely to leave the workspace dirty as increasing contention results in more operations failing, including operations that would clean up previous resources. This isn't a problem for disposable environments, but it is a problem for more permanent environments.
This PR adds a cleanup action for virtual users which is invoked leisurely after the users' main workflows have completed, and implements SCD op intent deletion during cleanup for virtual users performing SCD. I'm expecting a future PR to add ISA deletion during cleanup for virtual users performing NetRID.
I think this will be important to make an adaptive search to find/characterize the maximum load more quickly because that will require reliably reducing the load after performance became unstable before continuing (presumably successfully) at a lower load.