build: publish the RDMA build as warp.rdma - #501
Conversation
Declare warp-rdma as a build variant so the release pipeline carries the S3-over-RDMA binary alongside the stock one, and add the goreleaser build that produces it. The variant links libminiocpp through cgo, so unlike the stock build it cannot be cross-compiled: it is limited to linux on the release host's own architecture. CUDA is dlopened rather than linked, so the single binary still serves both --rdma=cpu and --rdma=gpu and no CUDA package is needed to build. The release host needs libminiocpp built with RDMA plus libibverbs, librdmacm and libnuma. MINIOCPP_PREFIX points at the install prefix when it is not /usr/local, and WARP_SKIP_RDMA=true drops the variant from a release if the host is not provisioned for it. Note the published binary resolves libminiocpp and the cuObj libraries from the host at run time; the self-contained archive that bundles them is still built separately by scripts/build-rdma.sh. Requires qreleaser with variants support (miniohq/q#180). Until that ships, qreleaser rejects unknown top-level keys, so this must not merge first.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds static RDMA linking, conditional ChangesRDMA build and release pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI as qreleaser-test workflow
participant Dependencies as minio-cpp and vcpkg
participant GoReleaser
participant Packager as release-post-transform.sh and pkger
participant Artifacts as warp and warp-rdma
CI->>Dependencies: Build and install RDMA dependencies
GoReleaser->>Artifacts: Build the conditional RDMA artifact
Packager->>Artifacts: Stage RDMA binaries
Packager->>Packager: Generate deb, rpm, and apk packages
CI->>Artifacts: Verify standard and RDMA behavior
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.goreleaser/qreleaser.yaml:
- Around line 58-59: Update the CGO_CFLAGS and CGO_LDFLAGS entries in the
GoReleaser environment configuration to preserve inherited values while
appending the MINIOCPP_PREFIX include and library flags. Ensure custom CUDA and
other existing process-environment paths remain available, using the existing
MINIOCPP_PREFIX default.
- Line 59: Add the runtime loader path for MINIOCPP_PREFIX to the CGO_LDFLAGS
entry, alongside the existing link-time flags, using the same
envOrDefault-derived lib directory and disabling new dtags as requested.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a2537869-1d4e-4948-b964-fedf374e628c
📒 Files selected for processing (3)
.gitignore.goreleaser/qreleaser.yaml.qreleaser.yml
release-test runs the full qreleaser pipeline, so it now reaches the warp-rdma build and failed on miniocpp/c_api.h: the runner had none of the RDMA prerequisites a real release host needs. Provision them the way go-rdma.yml already does -- pinned vcpkg, an explicit CMake, and libminiocpp built with RDMA installed system-wide -- then remove the checkouts again, since the workspace is what qreleaser releases from. Assert the result rather than just building it: warp-rdma must land beside the stock binary, run, and accept --rdma=cpu where the stock binary refuses it. That last check is what would catch the variant silently becoming a copy of the stock binary under a different name.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/qreleaser-test.yml:
- Line 42: Pin all workflow actions to immutable maintained commit SHAs: replace
actions/checkout@v4 at .github/workflows/qreleaser-test.yml lines 42 and 51, and
lukka/get-cmake@v4.4.2 at line 64, while preserving each action’s existing
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b834eef1-e967-43c8-a90b-83681b7b81f6
📒 Files selected for processing (1)
.github/workflows/qreleaser-test.yml
release-test got past the build and then failed in the transform: ERROR: Binary not found: dist/warp-rdma_linux_amd64_v1/warp qreleaser already has a convention for this. Its transform treats "-rdma" in a build id exactly like "-fips": it strips the flavor to find the project, expects the binary inside to carry the plain name, and suffixes the published files. The container config has carried rdma_enabled and rdma_archs for a while on the same basis. So drop the variants declaration and follow it: keep id warp-rdma while naming the binary warp, which yields warp.rdma and warp.<version>.rdma beside the stock warp. This needs nothing from qreleaser that is not already released. Also apply the review points on the build environment. goreleaser replaces same-named variables rather than merging them, so inherited CGO_CFLAGS and CGO_LDFLAGS are now carried through instead of discarded, and MINIOCPP_PREFIX is added to the runtime search path so a non-default prefix still resolves. That rpath is emitted as DT_RUNPATH rather than DT_RPATH on purpose: this binary is published for hosts that supply their own libminiocpp, and DT_RPATH would outrank their LD_LIBRARY_PATH. Verified against the released toolkit: goreleaser produces both builds, its transform yields warp/warp.rdma with matching versioned names and checksums, both binaries run, and only warp.rdma accepts --rdma=cpu.
harshavardhana
changed the title
Fixes Applied SuccessfullyFixed 3 file(s) based on 3 CodeRabbit feedback item(s): 2 applied, 1 deferred. Files modified:
Commit: Preserve inherited Add a runtime search path for Pin every action to an immutable commit SHA — deferred, consistent with the same decision on #500. All six warp workflows use floating tags; pinning one file would be inconsistent, and it is better done repo-wide in its own PR. Also in this commit: the release-test failureSeparate from the review, |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/qreleaser-test.yml (1)
41-45: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the
minio-cppcheckout to a reviewed commit SHA.Because
refis omitted for a secondary repository,actions/checkoutusesminio-cpp’s default branch. Changes to that branch can alter the RDMA build without a reviewed Warp change. Addref: <reviewed commit SHA>and update it deliberately when the dependency changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/qreleaser-test.yml around lines 41 - 45, Update the minio-cpp checkout step in the workflow to specify a reviewed commit SHA via the actions/checkout ref input, rather than relying on the repository’s default branch; retain the existing repository and path settings and change the SHA deliberately when updating the dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/qreleaser-test.yml:
- Around line 152-161: Update the CLI validation around the stock and RDMA
binaries: capture the stock warp output and assert it contains the exact
expected “--rdma=cpu requires the RDMA build of warp...” error, then run
warp.rdma without masking its exit status and assert the documented
post-validation result while retaining the existing check that it does not
report the stock-build error.
In @.goreleaser/qreleaser.yaml:
- Around line 66-71: Update the CGO_LDFLAGS configuration in the qreleaser build
settings so libminiocpp’s indirect cuObj dependencies remain loadable from a
custom MINIOCPP_PREFIX. Ensure the produced runtime metadata deliberately
provides the required transitive search behavior, preserving DT_RUNPATH where
required by adding --enable-new-dtags, or use an intentional DT_RPATH strategy
instead; do not rely on the test workflow’s ldconfig setup.
---
Outside diff comments:
In @.github/workflows/qreleaser-test.yml:
- Around line 41-45: Update the minio-cpp checkout step in the workflow to
specify a reviewed commit SHA via the actions/checkout ref input, rather than
relying on the repository’s default branch; retain the existing repository and
path settings and change the SHA deliberately when updating the dependency.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 385ee24d-6710-450d-9874-ae528ebc040b
📒 Files selected for processing (2)
.github/workflows/qreleaser-test.yml.goreleaser/qreleaser.yaml
Pin the dynamic tag with --enable-new-dtags rather than leaving it to the host linker's default, which varies by distribution. DT_RUNPATH stays the deliberate choice, since DT_RPATH would outrank LD_LIBRARY_PATH and stop a user of the published binary substituting their own libminiocpp. Document what that rpath cannot do: DT_RUNPATH is not transitive, and libminiocpp carries no runpath of its own, so its cuObj dependencies resolve from the host's library search path no matter what the executable says. A non-default MINIOCPP_PREFIX has to be on that path anyway. Assert specific outcomes in the release check. It discarded the flavor's exit status and only required a string to be absent, so a crash or any unrelated failure would have passed. The stock binary must now refuse for the documented reason, and the flavor must get past the RDMA gate and fail at the network.
Fixes Applied SuccessfullyFixed 2 file(s) based on 2 CodeRabbit feedback item(s), both applied. Files modified:
Commit: Keep indirect
So the tag is now pinned with Assert the expected RDMA CLI errors. Correct as reported: the flavor's exit status was discarded and the check only required a string to be absent, so a crash or any unrelated failure would have passed. The stock binary must now refuse for the documented reason, and the flavor must get past the RDMA gate and fail at the network layer. Both assertions were run against real binaries from a transformed release layout before pushing. CI
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/qreleaser-test.yml (2)
143-147: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAssert versioned RDMA artifacts and validate checksums.
Line 143 only prints the release files. The workflow does not require the versioned
warp.<version>.rdmaartifact or validate either generated.sha256sumfile. A flavor-layout or checksum regression can pass whilewarp.rdmastill exists. Add explicit file assertions and runsha256sum -cfrom${ARCH_DIR}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/qreleaser-test.yml around lines 143 - 147, Update the release validation steps after the file listing to explicitly assert that the versioned RDMA artifact warp.<version>.rdma and both generated .sha256sum files exist, then run sha256sum -c from within ${ARCH_DIR} for each checksum file. Keep the existing warp and warp.rdma version checks, while ensuring validation fails if the unversioned flavor remains or checksums are invalid.
41-46: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the
minio-cppsource revision.This checkout follows the upstream
mainbranch because it has noref. A later upstream change can alter or break this release test without a change in this repository. Setrefto a reviewed immutable commit SHA.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/qreleaser-test.yml around lines 41 - 46, Update the minio-cpp checkout step to include a ref specifying a reviewed immutable upstream commit SHA, so the workflow no longer follows the moving main branch. Keep the existing repository, path, and credential settings unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/qreleaser-test.yml:
- Around line 143-147: Update the release validation steps after the file
listing to explicitly assert that the versioned RDMA artifact
warp.<version>.rdma and both generated .sha256sum files exist, then run
sha256sum -c from within ${ARCH_DIR} for each checksum file. Keep the existing
warp and warp.rdma version checks, while ensuring validation fails if the
unversioned flavor remains or checksums are invalid.
- Around line 41-46: Update the minio-cpp checkout step to include a ref
specifying a reviewed immutable upstream commit SHA, so the workflow no longer
follows the moving main branch. Keep the existing repository, path, and
credential settings unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 36bd8fcf-c064-44eb-9b43-a15235015ee1
📒 Files selected for processing (2)
.github/workflows/qreleaser-test.yml.goreleaser/qreleaser.yaml
Link libminiocpp and its vcpkg dependencies into the binary instead of loading them at run time. cgo links with gcc rather than g++, so the C++ runtime and every transitive archive have to be named explicitly; cmake --install places only libminiocpp, so the vcpkg archives are collected beside it first. Only the cuObj libraries stay dynamic, having no static form. That is what makes the RDMA build installable: nothing outside the base system is needed except the cuObj pair, which the package now ships. Package it the way aistor packages minio-rdma, through the post-transform hook qreleaser already calls. pkger reads <binary-name>.<version> while the q layout names the flavor <binary>.<version>.rdma, so the hook stages a copy under the name pkger expects, builds deb/rpm/apk, and renames them warp-rdma_* on the way into the layout. The packages are drop-in rather than co-installable: the payload is /usr/local/bin/warp, so a customer installs the RDMA build instead of the stock one. A distinct warp.rdma package would need pkger to apply warp's semver rules to a flavored app name, which it does not; aistor avoids this only because minio uses date-based versions. The binary carries an rpath of /usr/lib/warp, where the package installs the cuObj libraries, and the deps file declares the host RDMA stack those libraries load. CUDA is deliberately not declared: it is dlopened and only --rdma=gpu needs it, so the package installs and runs --rdma=cpu without it. Verified end to end: static tarball with no libminiocpp in NEEDED, goreleaser build carrying the /usr/lib/warp rpath, the hook producing warp-rdma packages whose payload runs and accepts --rdma=cpu.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/qreleaser-test.yml (1)
145-170: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAssert generated RDMA packages and their Debian payload.
findonly logs artifacts. The workflow can pass whenpkgeremits no package in the expected layout. Assert the deb, RPM, and APK files exist. Inspect the deb payload and dependencies so the package path, cuObj libraries, and host RDMA requirements remain covered.Proposed validation
"${ARCH_DIR}/warp.rdma" --version + for package in \ + "${ARCH_DIR}"/warp-rdma_*.deb \ + "${ARCH_DIR}"/warp-rdma-*.rpm \ + "${ARCH_DIR}"/warp-rdma_*.apk; do + if [ ! -e "${package}" ]; then + echo "missing RDMA package: ${package}" >&2 + exit 1 + fi + done + + deb_package="$(printf '%s\n' "${ARCH_DIR}"/warp-rdma_*.deb | head -n1)" + dpkg-deb -c "${deb_package}" | grep -q 'usr/local/bin/warp' + dpkg-deb -c "${deb_package}" | grep -q 'usr/lib/warp/libcufile_rdma.so.1.18.0' + dpkg-deb -f "${deb_package}" Depends | grep -q 'libibverbs1' # The RDMA flavor must land beside the stock binary rather than🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/qreleaser-test.yml around lines 145 - 170, Extend the workflow validation before the binary checks to assert that pkger generated the expected Debian, RPM, and APK artifacts in the release layout rather than only listing files. Inspect the Debian package payload and dependency metadata, verifying the expected package path, cuObj libraries, and host RDMA requirements; keep the existing runtime checks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/go-rdma.yml:
- Around line 71-80: Pin both Checkout minio-cpp steps to the v0.5.0 tag by
adding the checkout action’s ref setting: update .github/workflows/go-rdma.yml
lines 71-80 and .github/workflows/qreleaser-test.yml lines 74-82. No other
workflow changes are needed.
---
Outside diff comments:
In @.github/workflows/qreleaser-test.yml:
- Around line 145-170: Extend the workflow validation before the binary checks
to assert that pkger generated the expected Debian, RPM, and APK artifacts in
the release layout rather than only listing files. Inspect the Debian package
payload and dependency metadata, verifying the expected package path, cuObj
libraries, and host RDMA requirements; keep the existing runtime checks
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: caaf8f2e-031f-4edf-8aff-a0e00607b9bd
📒 Files selected for processing (7)
.github/workflows/go-rdma.yml.github/workflows/qreleaser-test.yml.goreleaser/qreleaser.yamlpkg-scripts/rdma-contents.yamlpkg-scripts/rdma-deps.yamlscripts/build-rdma.shscripts/release-post-transform.sh
Build rdma failed with undefined references to __cxa_throw, operator delete and _M_dispose: go-rdma.yml builds with a plain `go build -tags=rdma`, relying on minio-go's own -lminiocpp, which stopped dragging in the C++ runtime and the vcpkg archives once libminiocpp became static. The list of libraries had been copied into the build script and the goreleaser config, and CI was the copy I missed. Put it in scripts/rdma-cgo-libs.txt and have all three read it -- the script with cat, goreleaser with mustReadFile, the workflow with cat -- so they cannot drift apart again. Verified by reproducing the CI path locally: go build and go test -race under -tags=rdma both succeed, and the goreleaser build still links statically with the /usr/lib/warp rpath.
1 participant
Follow-up to #500, which shipped the RDMA build itself. This publishes it through the release pipeline as
warp.rdma, alongside the stockwarp.Using qreleaser's existing RDMA flavor
qreleaser already knows about RDMA builds. Its transform treats
-rdmain a build id exactly the way it treats-fips: it strips the flavor to find the project, expects the binary inside the dist directory to carry the plain name, and suffixes the published files. The container config has carriedrdma_enabled/rdma_archson the same basis.So the build id is
warp-rdmawhile the binary stayswarp, and the release directory ends up with:An earlier revision of this PR declared a
variantsentry instead. That was the wrong mechanism for this pipeline —release-testbuilt both binaries fine and then failed in the transform withBinary not found: dist/warp-rdma_linux_amd64_v1/warp, which is the flavor convention telling us what it expected. Following the existing convention needs nothing from qreleaser that is not already released.What this adds
.goreleaser/qreleaser.yaml— the RDMA build:CGO_ENABLED=1,-tags=kqueue,rdma, linux/amd64..github/workflows/qreleaser-test.yml— provisions libminiocpp sorelease-testgenuinely exercises the RDMA release path, and asserts the result..gitignore—cufile.log, which libcufile writes into the working directory whenever an RDMA binary runs.The build links libminiocpp through cgo, so unlike the stock build it cannot be cross-compiled and is limited to the release host's architecture. CUDA is dlopened rather than linked, so the one binary serves both
--rdma=cpuand--rdma=gpu, and no CUDA package is needed to build it.CI now covers the release path
release-testpreviously reached the RDMA build on a bare runner and died onminiocpp/c_api.h. It now builds libminiocpp the same waygo-rdma.ymldoes — pinned vcpkg, explicit CMake — and then asserts, rather than merely building:warp.rdma --versionruns from the release layout, so the flavor survived the transformwarprefuses--rdma=cpuwarp.rdmadoes not report "requires the RDMA build"Both are named
warpinside their dist directories, so without that third check the flavor could silently be a copy of the stock binary and nothing would notice.Build environment
Two review points applied here. goreleaser replaces same-named environment variables rather than merging them — I verified this empirically — so inherited
CGO_CFLAGS/CGO_LDFLAGSare now carried through instead of silently dropped.MINIOCPP_PREFIXis also added to the runtime search path so a non-default prefix resolves.That rpath is deliberately
DT_RUNPATH, notDT_RPATH.DT_RPATHoutranksLD_LIBRARY_PATH, which would stop a user of the published binary pointing at their own libminiocpp. (DT_RPATHis right for the self-contained tarball, where the libraries are bundled and cuObj must resolve transitively — that archive is built separately byscripts/build-rdma.sh.)Verification
Against the released toolkit
RELEASE.2026-08-11T19-37-29Z: goreleaser produces both builds, its transform yields the layout above with matching versioned names and checksums, both binaries run, onlywarp.rdmaaccepts--rdma=cpu, andreadelfconfirmsRUNPATHrather thanRPATH.Note for the release host
It needs libminiocpp built with RDMA plus
libibverbs,librdmacmandlibnuma; setMINIOCPP_PREFIXif that is not/usr/local. Without them the build fails at the goreleaser stage — before the tag is pushed, so it is recoverable — or setWARP_SKIP_RDMA=trueto drop it from a release. The published binary resolves libminiocpp and the cuObj libraries from the host at run time.Summary by CodeRabbit
New Features
Bug Fixes
Tests