[benchmarker] Add SCD behavior to benchmarker#1585
Conversation
mickmis
left a comment
mickmis
left a comment
There was a problem hiding this comment.
LGTM, just a few minor comments or nit plus:
I think there would be value in running this in the CI (or at least a light version of it if it is heavy to run) to ensure we don't introduce regression in the future. Maybe in a follow-up PR though?
| and op.completed_at.datetime >= step_start_time | ||
| and op.type in relevant_ops | ||
| and op.completed_at.datetime <= now |
There was a problem hiding this comment.
nit: unclear to me why are checks on completed_at necessary?
There was a problem hiding this comment.
The >= step_start_time is critical for selecting the operations for the step. The <= now is much less critical when this is called right at the end of the step, but it's protective in case this method were called (perhaps only in some future implementation change) not right at the end of the step in the future.
| executor, | ||
| coordinator, | ||
| wrapped_record_op, | ||
| Random(random.randint(0, 2 << 31)), |
There was a problem hiding this comment.
nit: any reason for creating a new Random with this specific seed?
There was a problem hiding this comment.
The main idea is to make virtual user behavior as repeatable as practical. If a single Random was used between all users, the random ordering of events (due to, e.g., differing/random DSS and netem latency) would quickly make user behavior completely unrepeatable. With a random generator per user, at least some of that variability is removed.
Also, Random isn't thread-safe so we want a different Random for each user :)
|
|
||
| def wrapped_record_op(op: ExecutedOperation) -> None: | ||
| update_status_time() | ||
| if not op.successful: |
There was a problem hiding this comment.
I don't understand this change, is it correct?
There was a problem hiding this comment.
The purpose of update_status_time is to print a status message to the user periodically if no other status messages have been printed (so the user isn't left wondering if things are still working after 5 minutes with no feedback). Since there is no log activity for successful operations, we don't want to reset the periodic-status timer for successful operations. (to answer the question: yes)
| ): | ||
| yield FlightAction( | ||
| timestamp=datetime.now(UTC), | ||
| start=partial( |
There was a problem hiding this comment.
partial is new to me! Interesting
There was a problem hiding this comment.
Me as well; Gemini said to do it this way rather than a lambda because of the way parameter arguments are captured with the two different approaches. Though that was for the previous NetRID/ISA behavior; I'm copying that pattern in this PR but didn't have appreciable AI help on this PR.
| if success: | ||
| return list(self.get_create_actions(flight)) | ||
| else: | ||
| return [] |
There was a problem hiding this comment.
Shouldn't we fail somehow here instead of just not returning any action? IIUC this will result in a apparently successful run that did nothing?
There was a problem hiding this comment.
The method signature being implemented is effectively a meta-async pattern so the result is just "what additional tasks should I run after completing this one?" The answer to that here is "none" so I think the empty list is appropriate. The error is captured by recording an unsuccessful operation (line 250) which will result in the flight being considered unsuccessful.
| ) | ||
|
|
||
| if not success: | ||
| return [] |
There was a problem hiding this comment.
Same as other comment here: no failure?
There was a problem hiding this comment.
This failure should be captured on line 395.
Co-authored-by: Mickaël Misbach <mickael@misba.ch>
Co-authored-by: Mickaël Misbach <mickael@misba.ch> a482c64
2 participants
This PR works toward #1566 by adding strategic conflict detection (SCD) capabilities to benchmarker. Specifically, when enabled, virtual flight planner users will create ASTM F3548-21 operational intent references for their flights and delete them when their flight is complete.
Example output from single_s2_cell.jsonnet on a local v0.22.0 DSS instance with
The bulk of the new implementation in this PR is in users/flight_planner/scd.py which contains the logic to perform SCD for a single user including one-time creation of a subscription, upsertion of operational intents including requested OVNs and retries, and deletion of the operational intent. This is supported by the new configurations/user/astm/scd.py containing most of the configuration specification for this behavior.
A new element that turned out to be critical for this use case is a coordination mechanism for virtual users. This means of coordination is an array of predefined pubsub topics (coordination groups) where coordinating users can exchange shared messages. SCD uses this mechanism to exchange OVNs without implementing any of the flight details endpoints. Requested OVNs were essential to minimize race conditions in the contentious single_s2_cell.jsonnet configuration and even with sharing of requested OVNs before issuing the op intent reference creation request, the creation requests will often race and even 2 retries isn't enough to recover under higher loads.