[Robustness test] Patch the return time during validation with MaxInt64 in robustness test

[henrybear327]

Instead of marking the return time with -1 during operation data collection phase, we patch the failed request's response time during validation so we get to preserve the original data, and we get to avoid the problem we see mentioned in issue #19579.

Reference: #19579

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: henrybear327
Once this PR has been reviewed and has the lgtm label, please assign serathius for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[henrybear327]

@serathius for the record, the robustness test that I ran locally over lunch passed.

As discussed on Slack, I will now:

• select some of the past issues, e.g. test-robustness-issue18089 to validate if it still fails as expected
• check if the visualization looks ok by force-generating it

If all is good, we modify the logic to not set the return value to -1, but during validation when there is an error contained in the operation, we modify it to max int so we can preserve the original response time.

[henrybear327]

test-robustness-issue17780, test-robustness-issue18089, and test-robustness-issue19179 are successfully reproduced.

[henrybear327]

One force-generated report

TestRobustnessExploratory_EtcdHighTraffic_ClusterOfSize3_blackholePeerNetwork.zip

[henrybear327]

@serathius Because of this removal, the unit tests would have to be updated, due to this pre-condition failing.

But if I am not mistaken, each client is sending the requests sequentially, thus, the requests in the tests can be non-overlapping / sequentially executed requests, right?

[serathius]

Yes, updating unit tests here makes sense.

[henrybear327]

@serathius can you help me understand what's the purpose of this variable? :) I am not sure what case is this variable trying to cover after reading through and experimenting with it during bug fixing.

Thank you!

[serathius]

I was not expecting need to change this code. Please revert

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.78%. Comparing base (29878cf) to head (a1c249b).
Report is 8 commits behind head on main.

Additional details and impacted files

see 58 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19633      +/-   ##
==========================================
+ Coverage   64.12%   68.78%   +4.65%     
==========================================
  Files         421      421              
  Lines       35854    35854              
==========================================
+ Hits        22993    24661    +1668     
+ Misses      11443     9764    -1679     
- Partials     1418     1429      +11     

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e27246...a1c249b. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[henrybear327]

/retest

[serathius]

This should happen before we patch operations, because then patching will not have opportunity to modify the return time.

[serathius]

Please revert

[henrybear327]

Discussed offline, we will start a new PR with minimal changes and improved append validation rules.