dockerfile: Docker image to support large page sizes #11164
Conversation
Signed-off-by: Marcus Sorensen <msorensen@nvidia.com>
mlsorensen
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
WalkthroughThe change adds a build-time argument Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
dockerfiles/Dockerfile.largepage (3)
37-60: Refactor duplicate package installation logic.The
apt-get installblocks inbuilder-base(lines 37–60) and thedebugstage (lines 236–267) share ~25 common packages (e.g.,libssl3,libcurl4,libsasl2-2, etc.), creating maintenance overhead. If this is intentional (debug stage includes additional tooling likegdb,valgrind,tcpdump), document the rationale. Otherwise, consider extracting the common base packages into a shared stage or base image.Also applies to: 236-267
126-177: Deb-extractor stage is comprehensive but operationally complex.The extraction of Debian packages (lines 126–177) into a
/dpkgroot filesystem for distroless compatibility is a sophisticated approach. The logic correctly:
- Downloads all runtime dependencies
- Extracts package contents and control metadata
- Cleans up unnecessary files (docs, man pages)
However, this approach is fragile if upstream packages change their structure or dependencies. Consider:
- Pinning package versions to a specific snapshot date for reproducibility
- Adding a validation step to confirm all critical libraries are present
- Documenting the expected package set (e.g., library coverage for TLS, compression, database plugins)
Would you like me to generate a validation script or enhanced package snapshot strategy?
216-275: Debug stage provides comprehensive tooling for troubleshooting.The debug image includes extensive instrumentation (gdb, valgrind, tcpdump, nmap, strace, etc.) which is appropriate for development and diagnostics. The image is deliberately left with a
CMD(noENTRYPOINT) to allow interactive shell access. However, this creates a large image footprint. Consider:
- Documenting expected use cases for the debug variant
- Evaluating if all installed tools are necessary or if they can be installed on-demand
- Adding a
.dockerignorepattern or build-time flag to skip debug image builds in production pipelines if desired
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dockerfiles/Dockerfile.largepage(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Learnt from: aminvakil
Repo: fluent/fluent-bit PR: 10844
File: conf/fluent-bit:13-15
Timestamp: 2025-09-14T09:46:09.531Z
Learning: For fluent-bit Debian packaging, /opt/fluent-bit/bin/ is the appropriate installation path since the package may be installed from non-official Debian sources, making /opt compliant with FHS for optional software packages.
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
📚 Learning: 2025-09-14T09:46:09.531Z
Learnt from: aminvakil
Repo: fluent/fluent-bit PR: 10844
File: conf/fluent-bit:13-15
Timestamp: 2025-09-14T09:46:09.531Z
Learning: For fluent-bit Debian packaging, /opt/fluent-bit/bin/ is the appropriate installation path since the package may be installed from non-official Debian sources, making /opt compliant with FHS for optional software packages.
Applied to files:
dockerfiles/Dockerfile.largepage
🔇 Additional comments (6)
dockerfiles/Dockerfile.largepage (6)
100-101: Verify the intention of CFLAGS default value.Setting
CFLAGS="-v"(verbose compiler output) as the default is unusual. This flag increases build output verbosity which can slow builds and clutter logs. Clarify whether this is:
- Intentional for debugging the initial build
- A temporary setting that should be removed
- Meant to be configurable via
--build-arg CFLAGS=...without a verbose defaultConsider whether a more neutral default (e.g., empty string or standard optimization flags like
-O2) would be preferable.
58-58: Verifyapt-get satisfycompatibility and behavior.Both the
builder-base(line 58) anddebug(line 265) stages useapt-get satisfyto resolve cmake version constraints. While this syntax is valid in modern Debian (bookworm), confirm that:
- The version constraint
"cmake (<< 4.0)"is properly evaluated- The command fails gracefully if no satisfying version is found
- This approach is consistent with your build infrastructure expectations
If targeting older Debian releases in the future, note that
apt-get satisfymay not be available.Also applies to: 265-265
75-75: Verify WAMR build-target conditional logic.Line 81 uses bash conditional syntax within a
RUNcommand to conditionally appendWAMR_BUILD_TARGETtoEXTRA_CMAKE_FLAGS. The logic is correct, but clarify:
- Is
WAMR_BUILD_TARGETalways provided, or is the conditional necessary for backward compatibility?- Should this defaulting logic be documented or moved to a separate helper script for clarity?
The current approach works but could be more explicit.
Also applies to: 81-82
1-14: Multi-architecture build setup is well-documented.The header comments (lines 4–13) provide clear instructions for setting up QEMU and Docker Buildx for multi-platform builds. The subsequent QEMU image stages (lines 19–25) and qemu-user-static copies are standard and correct for cross-compilation support.
185-214: Production image appropriately uses distroless base.The production stage (
FROM gcr.io/distroless/cc-debian12) provides a minimal, secure runtime with:
- Extracted runtime dependencies from deb-extractor
- SSL certificates
- Proper metadata labels and versioning
- Correct ENTRYPOINT and CMD configuration
This aligns with security best practices for container images.
76-77: Jemalloc large-page configuration propagates correctly through the build system.Verification confirms that the
FLB_JEMALLOC_OPTIONSvariable set in the Dockerfile (lines 76-77) is properly received by CMakeLists.txt, parsed viaseparate_arguments(), and passed to jemalloc's configure command. The CMakeLists.txt conditional correctly handles both explicitly-provided and default cases, and the integration properly supports builder-time customization via--build-arg FLB_JEMALLOC_OPTIONS=....
|
Yeah I'd be inclined to add this as an option to the existing Dockerfile otherwise it's going to be forgotten when we need to update that. Can we make it an option like we do for the packaging ones then just add it to the matrix of container builds? We'll have to use a separate tag as you say, rather than large page I'd be inclined to specify the actual value for those with even larger pages! |
|
I'm also going to see if I can merge it to our downstream distro at https://github.com/FluentDo/agent because we already have multiple variants in place there (UBI and distroless) so adding a large page one makes sense too and the CI will be simpler. We do a weekly release as well so it'll be available sooner. |
|
We'll need a docs PR as well to indicate the tags to use |
|
Thanks. Does it sound good to just add a build arg to the regular Dockerfile that can be used to customize this setting, leaving the default behavior as-is? Is there something I can do to help here or do you prefer to own it? |
Yes
Feel free to update the pr with the changes, the less I do the better! |
Signed-off-by: Marcus Sorensen <msorensen@nvidia.com>
mlsorensen
force-pushed
the
dockerfile-large-pages
branch
from
9e5ab62 to
6fffd8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dockerfiles/Dockerfile (1)
75-77: CMakeLists.txt integration verified—code is correct. Comment improvement is optional.Verification confirms that
CMakeLists.txtproperly recognizes and handlesFLB_JEMALLOC_OPTIONS:
- Line 1143-1144: Variable is checked and defaults to
"--with-lg-quantum=3"when undefined or empty- Line 1147: Safe parsing via
separate_arguments()prevents injection- Line 1154: Variable is passed to jemalloc's configure command
The Dockerfile implementation is correct and maintains backward compatibility.
Optional refinement: The comment "e.g., page size" could be more concrete. Consider updating to reference actual jemalloc options like
--with-lg-page=16or--with-lg-quantum=3to help users understand valid values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dockerfiles/Dockerfile(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
dockerfiles/Dockerfile
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
dockerfiles/Dockerfile
🔇 Additional comments (1)
dockerfiles/Dockerfile (1)
96-96: CMake flag properly formatted and confirmed in CMakeLists.txt.The
FLB_JEMALLOC_OPTIONSflag is correctly defined inCMakeLists.txt(lines 1143–1154), with proper handling: it checks if defined, sets a sensible default (--with-lg-quantum=3), parses it into a list, and uses it in the jemalloc configure command. The Dockerfile correctly passes this flag via quoted variable expansion, matching the expected pattern.
|
I just pushed a commit that lets you set arbitrary jemalloc options. We would still need something in the build/publish pipeline to set and build this flavor of image for users. Note we have to provide --with-lg-quantum if we override, normally this is a default setting in Resulting image build with this build arg showed this at compile time: And with no build arg specified: |
2 participants
This is an attempt at fixing #11163, supporting variable pagesize from 4k to 64k by passing the large page jemalloc flag. This allows for ARM64 support with a variety of kernel page sizes.
I copied the existing
dockerfiles/Dockerfile, creating adockerfiles/Dockerfile.largepageand applying the appropriate pages. I tested the multi-arch image resulting from this Dockerfile in a mixed environment with both x86 and ARM64 servers and it works, no longer see jemalloc errors about unsupported page size.I am assuming we don't want to just make this the default, as it could cause some inefficiency in environments where little RAM is needed. Perhaps if you all think 4k mallocs are not useful for fluent-bit memory patterns then it can just be made default. Either way, the goal is for the project to publish a unified image that can run in a mixed environment.
This MR could be reworked in a few ways, looking for guidance. In all cases I imagine publishing
fluentd/fluent-bit:${version}-lgpageandfluentd/fluent-bit:${version}-lgpage-debugmulti-arch images similar to how there are-windowsimages, if we don't want this to be default.Dockerfile.largepage.Dockerfile.largepageover the existingDockerfile, but set the jemalloc flag to 4k--with-lg-page=12to get the pre-existing behavior. Then override the build arg with--with-lg-page=16in build scripts to publish the large page images.I'm a fan of the third one, however this current implementation is lower risk by not modifying the existing files at all.
In any case, I can make the Dockerfile changes and can test locally, but I will need help incorporating it as a part of build/publish. I'll refrain from selecting the package testing until I get input on how to change this MR.
Thanks in advance!
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit