5s - introduce fuzzed write transaction delay knob for simulation#12464
Conversation
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
e5e2017 to
cc54dc3
Compare
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
cc54dc3 to
2f30599
Compare
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
| #include "flow/UnitTest.h" | ||
| #include "flow/flow.h" | ||
|
|
||
| int getSimulatedTxnTimeoutSeconds() { |
There was a problem hiding this comment.
fyi for reviewers: this was just moved and renamed, logic is still the same
dlambrig
left a comment
There was a problem hiding this comment.
LGTM assuming all failed tests are proven to be unrelated.
The PR name is a little cryptic, for historical record could you rename it to something like "introduce fuzzed write transaction delay knob for simulation"
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
| // When setting such knobs, we want both client and server knob to have its value updated | ||
| // Short circuiting would mean that server knob FOO won't be updated if client knob FOO was updated | ||
| // Instead, we attempt setting client and server knobs in separate statements, and return true | ||
| // if at least one of the set attempts was succesful. |
There was a problem hiding this comment.
When setting such knobs, we want both client and server knob to have its value updated
Is that the case for all such knobs? This PR automatically forces that change on all such knobs. But this maybe the correct behavior (or the behavior that we want in most cases) so it is probably fine.
There was a problem hiding this comment.
"ClientKnobCollection" is following a similar model: (set a flow knob || set a client knob). We are applying this change only to ServerKnobs and not to ClientKnobs.
When setting such knobs, we want both client and server knob to have its value updated
This happens only if there is no such flow knob (in which case only the flow knob and the server knob would get updated). That's probably fine.
There was a problem hiding this comment.
But this maybe the correct behavior (or the behavior that we want in most cases) so it is probably fine.
Since this only affects knobs with same names, and there are very little of them, my guess is we've not defined this behavior in the past. I personally feel it's the right behavior because it's unexpected if you update FOO via ServerKnobCollection, and only client FOO is updated but server FOO is not.
Let me actually do the exercise of finding how many knobs do we have which have the same name across client and server. It'd be good to double check that this new behavior does not conflict with anything. I can report back here.
"ClientKnobCollection" is following a similar model: (set a flow knob || set a client knob)
Yes, I wasn't sure if this behavior was needed there but regardless, keeping the behavior consistent is a good idea. I can do that in a follow-up PR after this.
There was a problem hiding this comment.
These are the knobs that share the same name between ClientKnobs and ServerKnobs:
- FASTRESTORE_ATOMICOP_WEIGHT
- GLOBAL_CONFIG_REFRESH_TIMEOUT
- MAX_WRITE_TRANSACTION_LIFE_VERSIONS (the example you mentioned)
- VERSIONS_PER_SECOND
(3) and (4) we already know about.
(1) is interesting, client knob is used but server knob is not, I think I can delete the server knob in a follow-up PR.
(2)'s value is 10 in production across client and server. But under randomize && buggify, the server value can be 1 sometimes. It looks intentional so I'll leave it as is.
| @@ -38,7 +33,7 @@ void ServerKnobs::initialize(Randomize randomize, ClientKnobs* clientKnobs, IsSi | |||
|
|
|||
| // Versions -- knobs that control 5s timeout | |||
| init( VERSIONS_PER_SECOND, 1e6 ); | |||
There was a problem hiding this comment.
This knob's value should be same as the corresponding client knob's value (which is what I think you were trying to do in your other commit)?
There was a problem hiding this comment.
Please confirm that this is a valid change, thanks!
There was a problem hiding this comment.
Yes, doing an experiment to gather data before responding. Will report back.
There was a problem hiding this comment.
This should work, let me make this change in the follow-up PR alongside other changes discussed.
Seems to be joshua infra issue: [Container] 2025/11/05 19:47:49.149493 Running command python3 -m joshua.joshua tail --errors --xml ${ENSEMBLE_ID} [Container] 2025/11/05 19:50:42.560957 Running command if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL |
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
* remove server FASTRESTORE_ATOMICOP_WEIGHT * address other feedback * fix spacing * revert VERSIONS_PER_SECOND change
…ple#12464) * 5s: fuzz write knob * self review * self review * self review * looks like AutomaticIdempotency.toml works, so revert my changes there * fix spacing
* remove server FASTRESTORE_ATOMICOP_WEIGHT * address other feedback * fix spacing * revert VERSIONS_PER_SECOND change
Description
Changes:
Testing
250K: 20251104-062454-praza-5s-milestone1-iter34--341964d848d7cf63 compressed=True data_size=37405675 duration=13264619 ended=250000 fail=1 fail_fast=10 max_runs=250000 pass=249999 priority=100 remaining=0 runtime=2:12:16 sanity=False started=250000 stopped=20251104-083710 submitted=20251104-062454 timeout=5400 username=praza-5s-milestone1-iter34-2f30599007142c33e0ffeedd9ac4c98eb6c1d8fc.
The 1 failure is: fdbserver -r simulation -f /root/src/foundationdb/tests/slow/SwizzledCycleTest.toml --buggify on --seed 195531608. TLDR: I don't think it's related to this change, see below. I am still investigating this issue though to see if it needs a separate fix.
Sev40:
This is happening because of this code:
foundationdb/fdbrpc/sim2.actor.cpp
Lines 1097 to 1098 in b720c6d
I verified adding a trace event that the value is less than -0.0001:
From the backtrace, this is coming from:
foundationdb/fdbserver/TLogServer.actor.cpp
Lines 2989 to 2990 in 516351d
I do not think it is related. To confirm, I ensured for this run, both txn timeout knobs were set to 5s:
Code-Reviewer Section
The general pull request guidelines can be found here.
Please check each of the following things and check all boxes before accepting a PR.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branchormainif this is the youngest branch)