Skip to content
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

Add InitialMmapSize to bolt options #13178

Merged
merged 7 commits into from Nov 23, 2021
Merged

Add InitialMmapSize to bolt options #13178

merged 7 commits into from Nov 23, 2021

Conversation

raskchanky
Copy link
Contributor

https://hashicorp.atlassian.net/browse/VAULT-2552

Since we're rapidly accumulating options that we want to share between raft.db and vault.db, I refactored the freelistOptions() function to be more generally boltOptions(). I'm using strconv.IntSize to determine whether we're on a 32bit or 64bit architecture, since we only want the InitialMmapSize to be set for 64bit platforms. I'm including a few screenshots here of a simple program I wrote to test that this works the way I think it should.

This is the program in its entirety, running locally on my MBP.
CleanShot 2021-11-16 at 09 26 26

This is the same program, running on 32bit Debian Linux, in a VM.
CleanShot 2021-11-16 at 09 29 26

In addition to the program and its output, I also included the output of a few different utilities, in an effort to demonstrate that this VM is really running 32bit Linux.

I also included a few tests, one 32 bit specific, to make sure that we're not setting InitialMmapSize on 32bit platforms.

@ncabatoff
Copy link
Collaborator

I don't understand the package build failures, but it's suspicious that all the failing ones are 32-bit targets.

@raskchanky
Copy link
Contributor Author

@ncabatoff I tried compiling this on my 32bit VM and it errored on this line:
o.InitialMmapSize = 100 * 1024 * 1024 * 1024
saying that it overflows int.

So it appears that I need to re-work this to use platform specific build tags instead.

@vercel vercel bot temporarily deployed to Preview – vault November 17, 2021 20:05 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 17, 2021 20:05 Inactive
@raskchanky
Copy link
Contributor Author

I'm also not sure if this requires a changelog or not, since it's not a user visible change. Thoughts?

@ncabatoff
Copy link
Collaborator

I'm also not sure if this requires a changelog or not, since it's not a user visible change. Thoughts?

My position is that if a change affects the compiled binary, it deserves a changelog.

physical/raft/raft.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 18, 2021 19:26 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 18, 2021 19:26 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 18, 2021 22:58 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 18, 2021 22:58 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 18, 2021 23:06 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 18, 2021 23:06 Inactive
Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

LGTM but you have failing tests

@vercel vercel bot temporarily deployed to Preview – vault-storybook November 22, 2021 17:55 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 22, 2021 17:55 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 22, 2021 19:33 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 22, 2021 19:33 Inactive
@raskchanky raskchanky merged commit 17cb1c6 into main Nov 23, 2021
@raskchanky raskchanky deleted the vault-2552 branch November 23, 2021 04:17
qk4l pushed a commit to qk4l/vault that referenced this pull request Feb 4, 2022
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.

None yet

2 participants