-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix flaky vreplication tests: correct logic that checks for workflow state in test helper #17498
Fix flaky vreplication tests: correct logic that checks for workflow state in test helper #17498
Conversation
Signed-off-by: Rohit Nayak <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Rohit Nayak <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17498 +/- ##
=======================================
Coverage 67.68% 67.68%
=======================================
Files 1584 1584
Lines 254466 254466
=======================================
+ Hits 172235 172244 +9
+ Misses 82231 82222 -9 ☔ View full report in Codecov by Sentry. |
shardStreams := gjson.Get(output, "workflows.0.shard_streams") | ||
// We need to wait for all streams in all shard streams to have the desired state. | ||
shardStreams.ForEach(func(shardStreamId, shardStream gjson.Result) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you used https://gjson.dev with sample outputs to be sure that this is different? I thought that this would get all streams, but maybe not:
streams := gjson.Get(output, "workflows.0.shard_streams.*.streams")
streams.ForEach(func(streamId, stream gjson.Result) bool { // For each stream
Just a note that there's no need to backport this PR as the code you're modifying here is new from this recent PR: #17441
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I confirmed that workflows.0.shard_streams.*.streams
only returns 1 stream. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had tested the json parsing locally and that is how I figured out the problem and the fix. The gjson
document is quite confusing with regard to wildcard filtering!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @rohit-nayak-ps !
shardStreams := gjson.Get(output, "workflows.0.shard_streams") | ||
// We need to wait for all streams in all shard streams to have the desired state. | ||
shardStreams.ForEach(func(shardStreamId, shardStream gjson.Result) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I confirmed that workflows.0.shard_streams.*.streams
only returns 1 stream. Thanks!
Description
The current check for a workflow to reach a state (typically
Running
) was only checking for the first stream while parsing the json result of a workflow status check. This causes flakiness when there are multiple streams and the first one has reachedRunning
but others are, say,Copying
.This PR fixes the logic to indeed iterate over all streams.
The flakiness was introduced by #17441 in v22, so no need to backport.
Related Issue(s)
Checklist
Deployment Notes