Skip to content

fix: Prevent speculative file growth and mmap state on MaxSize exceeded on Windows env.#1205

Closed
Elbehery wants to merge 2 commits into
etcd-io:mainfrom
Elbehery:20260521_support_data_size_limit
Closed

fix: Prevent speculative file growth and mmap state on MaxSize exceeded on Windows env.#1205
Elbehery wants to merge 2 commits into
etcd-io:mainfrom
Elbehery:20260521_support_data_size_limit

Conversation

@Elbehery

@Elbehery Elbehery commented May 21, 2026

Copy link
Copy Markdown
Member

Fixes #1108

Continuation of #1130

cc @ahrtr @ivanvc @fuweid @serathius

@k8s-ci-robot

k8s-ci-robot commented May 21, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details Needs approval from an approver in each of these files:

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

@Elbehery
Elbehery force-pushed the 20260521_support_data_size_limit branch 3 times, most recently from da58b8a to 9d3e334 Compare May 21, 2026 13:28
@Elbehery

Elbehery commented May 21, 2026

Copy link
Copy Markdown
Member Author

@ahrtr following up on #1130 (comment)

I think we need to disable InitialMmapSize for Windows; in other words, we don't use it for Windows.

so instead of disabling the initMmap, we guard it with

 if int64(sz) > fi.Size() {
      if err := db.file.Truncate(int64(sz)); err != nil { ... }
  }

to eliminate it's side effect.

Please let me know if this is sufficient

Also I have added all the commits from #1130, authored by @mattsains 👍🏽

Thanks for your patience and support 🙏🏽

@Elbehery Elbehery mentioned this pull request May 21, 2026
Closed
17 tasks
@Elbehery
Elbehery force-pushed the 20260521_support_data_size_limit branch from 9d3e334 to 493227e Compare May 21, 2026 15:46
fuweid
fuweid reviewed May 22, 2026
View reviewed changes

@fuweid fuweid left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To fix windows issue, I think we need to do check both allocate and grow. just in case the InitialMapSize is way larger than MaxSize.

Comment thread db.go Outdated
Comment thread db.go
Comment thread db.go
@ahrtr

ahrtr commented May 23, 2026

Copy link
Copy Markdown
Member

To avoid long back and forth review communication, please just apply my PR #1206 into your PR, thx

@ahrtr ahrtr mentioned this pull request May 23, 2026
Closed
@ahrtr

ahrtr commented May 25, 2026

Copy link
Copy Markdown
Member

To avoid long back and forth review communication, please just apply my PR #1206 into your PR, thx

@Elbehery If you don't have bandwidth, do you mind that I take over this task?

@Elbehery

Elbehery commented May 25, 2026

Copy link
Copy Markdown
Member Author

Hello

I just saw the comment.

Will follow up tomorrow morning

It is almost done, I want finalize it if possible plz

@ahrtr

ahrtr commented May 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Hello

I just saw the comment.

Will follow up tomorrow morning

It is almost done, I want finalize it if possible plz

Sure. Please add all related test cases in the first commit, and ensure all test cases fail. Then apply my PR/commit as the second commit, and ensure all test cases pass.

@Elbehery
Elbehery force-pushed the 20260521_support_data_size_limit branch from 1640b45 to 725b012 Compare May 26, 2026 09:43
@Elbehery

Elbehery commented May 26, 2026

Copy link
Copy Markdown
Member Author

@ahrtr ptal 👍🏽

@ahrtr

ahrtr commented May 26, 2026

Copy link
Copy Markdown
Member

@Elbehery can you orignaize this PR so that we only have two commits as mentioned above #1205 (comment)?

@Elbehery
Elbehery force-pushed the 20260521_support_data_size_limit branch 2 times, most recently from 29e967a to 77a4979 Compare May 26, 2026 11:22
@Elbehery Elbehery self-assigned this May 26, 2026
@Elbehery

Elbehery commented May 26, 2026

Copy link
Copy Markdown
Member Author

/ok-to-test

@Elbehery

Elbehery commented May 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

@ahrtr updated but the CI is not being triggered :/

ahrtr
ahrtr reviewed May 26, 2026
View reviewed changes
Comment thread db_test.go Outdated
Comment on lines +1753 to +1796
t.Run("reads succeed after open with high InitialMmapSize", func(t *testing.T) {
path := copyDB(t)
db, err := btesting.OpenDBWithOption(t, path, &bolt.Options{
MaxSize: 1,
InitialMmapSize: int(baseSize),
})
require.NoError(t, err)
defer db.MustClose()

err = db.View(func(tx *bolt.Tx) error {
b := tx.Bucket([]byte("data"))
require.NotNil(t, b, "bucket 'data' must exist")
require.NotNil(t, b.Get([]byte("0001")), "key 0001 must be readable")
return nil
})
require.NoError(t, err, "read transaction must succeed")
})

// After open with high InitialMmapSize and a generous MaxSize, small
// writes that fit within the limit must succeed.
t.Run("small writes succeed after open with high InitialMmapSize", func(t *testing.T) {
path := copyDB(t)
db, err := btesting.OpenDBWithOption(t, path, &bolt.Options{
MaxSize: int(baseSize) * 4,
InitialMmapSize: int(baseSize) * 2,
})
require.NoError(t, err)
defer db.MustClose()

err = fillDBWithKeys(db, 1, 1)
require.NoError(t, err, "small write must succeed when within MaxSize")
})

// After open, writes that would push past MaxSize must return ErrMaxSizeReached.
t.Run("oversized writes return ErrMaxSizeReached after open with high InitialMmapSize", func(t *testing.T) {
path := copyDB(t)
db, err := btesting.OpenDBWithOption(t, path, &bolt.Options{
MaxSize: int(baseSize),
InitialMmapSize: int(baseSize) / 2,
})
require.NoError(t, err)
defer db.MustClose()

err = fillDBWithKeys(db, 2, 100000)

@ahrtr ahrtr May 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please move them into a separate test case, use table driven sub test

@Elbehery Elbehery May 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed 👍🏽

@serathius

serathius commented May 26, 2026

Copy link
Copy Markdown
Member

/retest

@serathius

serathius commented May 26, 2026

Copy link
Copy Markdown
Member

Looks like bot broke.

Elbehery and others added 2 commits May 26, 2026 13:55
@Elbehery
test: add and update tests for MaxSize and MMap behavior
443486e 
- Update fillDBWithKeys to accept a configurable value size
- Fix TestDB_MaxSizeNotExceeded to use larger values that trigger grow
- Remove Windows skip from TestDB_MaxSizeExceededCanOpenWithHighMmap
- Rewrite TestDB_MaxSizeExceededDoesNotGrow with table-driven sub-tests
  covering InitialMmapSize variants on Windows
- Add TestDB_MaxSizeWithHighInitialMMapDoesNotGrowOnWrite for non-Windows

Signed-off-by: elbehery <elbeherymustafa@gmail.com>
@ahrtr @Elbehery
Reimplement the feature of support setting data file size limit
9e9dae1 
Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>
@Elbehery
Elbehery force-pushed the 20260521_support_data_size_limit branch from 77a4979 to 9e9dae1 Compare May 26, 2026 11:56
@ahrtr ahrtr closed this May 26, 2026
@ahrtr ahrtr reopened this May 26, 2026
@ahrtr

ahrtr commented May 26, 2026

Copy link
Copy Markdown
Member

close & reopen still don't trigger the workflow.

@Elbehery probably raise a new PR?

@Elbehery Elbehery mentioned this pull request May 26, 2026
Merged
@Elbehery

Elbehery commented May 26, 2026

Copy link
Copy Markdown
Member Author

raised #1210

@serathius

serathius commented May 26, 2026

Copy link
Copy Markdown
Member

Maybe it's Github that broke

@Elbehery

Elbehery commented May 26, 2026

Copy link
Copy Markdown
Member Author

Maybe it's Github that broke

yes #1210 (comment)

@Elbehery

Elbehery commented May 26, 2026

Copy link
Copy Markdown
Member Author

/retest

@Elbehery

Elbehery commented May 26, 2026

Copy link
Copy Markdown
Member Author

closed in favor of #1210

@Elbehery Elbehery closed this May 26, 2026
@Elbehery
Elbehery deleted the 20260521_support_data_size_limit branch May 27, 2026 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahrtr ahrtr ahrtr left review comments
+1 more reviewer
@fuweid fuweid fuweid left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@Elbehery Elbehery

Labels

ok-to-test size/L

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

When a db write fails because MaxSize is reached, the db client should still accept future reads and writes

5 participants

@Elbehery @k8s-ci-robot @ahrtr @serathius @fuweid