Skip to content

5s - fuzz read knob value in simulation#12440

Merged
spraza merged 1 commit into
apple:mainfrom
spraza:5s-milestone1-iter31
Oct 13, 2025
Merged

5s - fuzz read knob value in simulation#12440
spraza merged 1 commit into
apple:mainfrom
spraza:5s-milestone1-iter31

Conversation

@spraza

@spraza spraza commented Oct 10, 2025

Copy link
Copy Markdown
Collaborator

Description

This PR makes the simulation fuzzing logic for MAX_READ_TRANSACTION_LIFE_VERSIONS simpler by picking a value in range [1, 10] with a lower weight, otherwise 5s is picked. The previous logic was based on buggify, the goal of which is to ensure system behaves correctly in extreme or rare scenarios, including ones with fault injection. But now as we're looking to extend the 5s limit in FDB, we should expect that different txn timeout values are normal / expected.

In addition, a truePercent utility is added to DeterministicRandom. Unit tests are added to ensure correctness of the utility.

Testing

100K: 20251010-173457-praza-5s-milestone1-iter31-5ea5963b8b2444924 compressed=True data_size=37454516 duration=5705360 ended=100000 fail_fast=10 max_runs=100000 pass=100000 priority=100 remaining=0 runtime=0:56:20 sanity=False started=100000 stopped=20251010-183117 submitted=20251010-173457 timeout=5400 username=praza-5s-milestone1-iter31-5ea5963b8b244492439547e4bd481f49bec41949

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.

  • The PR has a description, explaining both the problem and the solution.
  • The description mentions which forms of testing were done and the testing seems reasonable.
  • Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

  • This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
  • There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

@spraza spraza marked this pull request as draft October 10, 2025 06:48
@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-ide on Linux RHEL 9

  • Commit ID: ce6a1c9
  • Duration 0:26:04
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: ce6a1c9
  • Duration 0:42:39
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

  • Commit ID: ce6a1c9
  • Duration 0:46:05
  • Result: ❌ FAILED
  • Error: Error while executing command: docker build --label "org.foundationdb.version=${FDB_VERSION}" --label "org.foundationdb.build_date=${BUILD_DATE}" --label "org.foundationdb.commit=${COMMIT_SHA}" --progress plain --build-arg FDB_VERSION="${FDB_VERSION}" --build-arg FDB_LIBRARY_VERSIONS="${FDB_VERSION}" --build-arg FDB_WEBSITE="${FDB_WEBSITE}" --tag foundationdb/foundationdb-kubernetes-sidecar:${FDB_VERSION}-${COMMIT_SHA}-1 --file Dockerfile --target foundationdb-kubernetes-sidecar .. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: ce6a1c9
  • Duration 0:47:10
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux RHEL 9

  • Commit ID: ce6a1c9
  • Duration 1:12:25
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang on Linux RHEL 9

  • Commit ID: ce6a1c9
  • Duration 1:12:55
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: ce6a1c9
  • Duration 1:15:05
  • Result: ❌ FAILED
  • Error: Build has timed out.
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@spraza spraza marked this pull request as ready for review October 10, 2025 18:40

@saintstack saintstack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (I like the test)

@jzhou77 jzhou77 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@spraza spraza merged commit 05d36e1 into apple:main Oct 13, 2025
5 of 7 checks passed

// With 1% probability, expect around 100 true out of 10000
// Allow for some variance (between 70-130 for 99% confidence)
ASSERT(trueCount1 >= 70 && trueCount1 <= 130);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the methodology used for picking this range? This is an assertion so the assumption is that "trueCount1" is always guaranteed to be in this range - trying to understand how we can provide such a guarantee (based on the number of trials used in this test).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the same question for the assertions used below.

@spraza spraza Oct 14, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(didn't get notified of this message because PR was merged, seeing this now)

Great question. I did some statistical math behind this. Basically I am modeling this output (1/0) of the function as a random variable X that follows a binomial distribution. In this 1% case, the parameters are p = 0.01, n = 10,000. If you crunch the numbers, mean would be 100, variance would be 99, stddev is 9.95. [70,130] was picked as +/- 3 stddevs around the mean. 3 stddevs mean we'd cover 99.7% of the outcomes.

99.7% could seem low i.e. if you run the same unit test 100K times, you can expect ~300 failures. But the detail here is that all these tests have a fixed seed to the rng. So if one run passes, all runs should practically pass all the time given fixed seed. I actually ran just this specific unit test 100K times and confirmed that:

20251014-191109-praza-unit-test-stats-493ef-423a88f556d07cbe compressed=True data_size=37856982 duration=104992 ended=100000 fail_fast=10 max_runs=100000 pass=100000 priority=100 remaining=0 runtime=0:15:17 sanity=False started=100000 stopped=20251014-192626 submitted=20251014-191109 timeout=5400 username=praza-unit-test-stats-493ef8974bd70a62e55e2ace986fc7bb861850a0

So technically we can pick a tighter bound than 3 stddevs given we have apriori knowledge of the seed. The reason I picked 3 stddevs was that if the rng implementation ever changes (e.g. if we switch to a different rng library), then I wanted some protection that the test still passes even though the randomness is different (still deterministic) now. In other words, if we were to change rng implementation 100K times, this test could fail 300 times. Ofcourse we almost never change rng so 3 stddevs seemed pretty safe.

@sbodagala sbodagala Oct 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your math, assuming binomial distribution is the way to approximate the outcomes (not sure if Poisson distribution/other distributions are better approximations though). But they are all approximations (no guarantees), hence my question about the use of ASSERT here.

I think the key here is that the test will keep on succeeding if it succeeds once (on each platform) (since the seed is the same in each run) - a comment along those lines would have made it easier to see why this test won't be flaky. Thanks for your reply!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, I will add a comment and send the PR your way for review.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment PR: #12462

@sbodagala

Copy link
Copy Markdown
Contributor

Added a couple of comments above, for my own understanding. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants