ROX-33792: optimize CLI artifact upload by removing tar/gzip bundling#20662
Open
janisz wants to merge 7 commits into
Open
ROX-33792: optimize CLI artifact upload by removing tar/gzip bundling#20662janisz wants to merge 7 commits into
janisz wants to merge 7 commits into
Conversation
Problem: The workflow uses tar/gzip to bundle CLI binaries before upload, taking ~1 minute per build. The original rationale was to preserve file permissions, but actions/upload-artifact@v7 preserves permissions natively. Solution (Phase 1 of 3): - Remove tar bundling step in pre-build-cli job - Upload bin/ directory directly instead of creating cli-build.tgz - Update 2 download locations to use path: . (preserves directory structure) - Remove tar extraction steps (no longer needed) - Add verification step to confirm binaries remain executable Expected savings: ~30-40 seconds per build for CLI artifacts. This is Phase 1 - Phase 2 will optimize Go binaries artifacts similarly. User request: "Look this step in .github/workflows/build.yaml 'Run tar -cvzf go-binaries-build.tgz bin/linux_amd64' take about a minute, do we really need it? Maybe it will be faster if we use native upload artifact upload of a full directory" Note: This change was partially generated with AI assistance. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new verification step uses
stat -c '%a', which is GNU-specific; if any jobs ever run on macOS runners this will fail, so consider either guarding by OS or using a more portable way to print permissions. - Since the workflow now relies on
actions/upload-artifact@v7preserving permissions, it may be helpful to explicitly pin or assert that minimum version in the workflow (or add a brief comment) so future upgrades don’t unintentionally break this assumption.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new verification step uses `stat -c '%a'`, which is GNU-specific; if any jobs ever run on macOS runners this will fail, so consider either guarding by OS or using a more portable way to print permissions.
- Since the workflow now relies on `actions/upload-artifact@v7` preserving permissions, it may be helpful to explicitly pin or assert that minimum version in the workflow (or add a brief comment) so future upgrades don’t unintentionally break this assumption.
## Individual Comments
### Comment 1
<location path=".github/workflows/build.yaml" line_range="576-588" />
<code_context>
run: |
- tar xvzf cli-build.tgz
+ # Verify binaries are executable
+ for binary in bin/*/roxctl bin/*/roxagent; do
+ if [ -f "$binary" ]; then
+ if [ ! -x "$binary" ]; then
+ echo "ERROR: $binary is not executable"
+ ls -la "$binary"
+ exit 1
+ fi
+ echo "✓ $binary is executable ($(stat -c '%a' "$binary"))"
+ fi
</code_context>
<issue_to_address>
**suggestion:** The loop silently passes if no binaries match the glob, which may mask missing artifacts.
If the artifact layout changes or these files are missing, the globs won’t match, `[ -f "$binary" ]` will always be false, and the step will still succeed, hiding missing/mis-packaged artifacts. Please add an explicit check for the “no matches” case (for example, by first expanding into positional parameters and checking `$#`, or using `nullglob` and failing when the resulting list is empty).
```suggestion
- name: Verify binary permissions after artifact download
run: |
# Verify binaries are executable
shopt -s nullglob
binaries=(bin/*/roxctl bin/*/roxagent)
if [ ${#binaries[@]} -eq 0 ]; then
echo "ERROR: no binaries found matching bin/*/roxctl or bin/*/roxagent"
exit 1
fi
for binary in "${binaries[@]}"; do
if [ -f "$binary" ]; then
if [ ! -x "$binary" ]; then
echo "ERROR: $binary is not executable"
ls -la "$binary"
exit 1
fi
echo "✓ $binary is executable ($(stat -c '%a' "$binary"))"
fi
done
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
📝 WalkthroughWalkthroughGitHub workflow CI/CD changes migrate CLI build artifacts from tarball format to direct directory upload. The pre-build CLI job now exports ChangesCLI Build Artifact Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/build.yaml:
- Around line 576-588: The verification loop that iterates over "for binary in
bin/*/roxctl bin/*/roxagent;" currently only checks executability if files
exist, allowing the step to succeed when the glob matches nothing; update the
step to detect the case where no binaries were found and fail the job—e.g.,
track whether any iteration found an existing file (a counter or flag) inside
the loop that checks "-f \"$binary\"" and after the loop exit with non-zero
status and an explanatory error if the counter is zero so the workflow fails
when no CLI binaries were downloaded.
🪄 Autofix (Beta)
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d68dcc14-7b4f-4528-978a-7f4a82f67482
📒 Files selected for processing (1)
.github/workflows/build.yaml
Problem: The previous commit had two issues:1. CLI artifact uploaded with `path: bin/` (trailing slash) which uploads CONTENTS of bin/, not the bin/ directory itself, breaking directory structure 2. Go binaries still used tar/gzip bundling Root cause: When uploading `path: dir/`, it uploads dir contents. When downloading with `path: .`, files go to workspace root without the dir/ prefix. Solution:- CLI: Change upload to `path: bin` (no trailing slash) to upload the directory itself - Go binaries: Apply same optimization - remove tar bundling, upload directories directly - Update all 4 go-binaries download locations to add `path: .` and remove tar extraction This combines Phase 1 (CLI) fix and Phase 2 (Go binaries) optimization: - Fixes directory structure for CLI stubs (PR builds work without ci-build-cli label) - Removes tar/gzip overhead for go-binaries across all architectures - Expected savings: ~2-3 minutes per PR build (CLI: 59s + Go binaries: ~90-110s per arch) Verified: Permission check shows binaries remain executable (755). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
janisz
changed the title
Problem: Previous attempt to upload go-binaries as raw directory failed because: - Uncompressed artifact was 434 MB (too large for reliable download) - GitHub Actions download failed after 5 retries with network errors - All build-and-push-main jobs failed Root cause: Large directory artifacts are less reliable than single files. The 434MB uncompressed directory exceeded GitHub's reliable transfer limits. Solution: Hybrid approach for go-binaries only (CLI stays as-is): - Use `tar -cf` (WITHOUT gzip compression) to bundle binaries - Creates single tarball file (reliable download/extract) - GitHub Actions applies fast zstd compression to the tarball - Skips slow gzip compression step (~15-30s savings) - Download/extract uses `tar -xf` (fast, no decompression needed) Result: - CLI: Direct directory upload (59s savings) - WORKING - Go binaries: tar (no gzip) + GH Actions zstd (~15-30s savings per arch) - Total expected savings: ~90-150s per PR build Verified upload timings (from successful run): - CLI: 1 second (vs 60s before) - Go binaries arm64: 29 seconds (vs 60s before) - Go binaries amd64: 44 seconds (vs 60s before) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes: 1. Add compression-level parameter to upload-artifact-with-retry wrapper 2. Set compression-level: 1 for both CLI and go-binaries uploads 3. Remove permission verification step (permissions confirmed preserved) Rationale: - compression-level: 1 provides minimal compression with maximum speed - zstd level 1 is significantly faster than level 6 (default) with only slightly larger artifacts - Permission check validated in previous runs - no longer needed Expected improvement: - Faster artifact upload due to minimal compression overhead - Cleaner workflow without redundant verification step Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Problem: Unnecessarily using tar when compression-level: 1 makes it obsolete.
Solution: Upload directories directly with compression-level: 1:
- Removed tar bundling step for go-binaries
- Removed all 4 tar extraction steps
- Upload path: bin/linux_${{ matrix.arch }} directly
- Fast zstd level 1 compression handles everything
Result:
- Simpler workflow (no tar/untar steps)
- Faster: no tar creation overhead (~5-10s per arch)
- Same benefits: compression-level 1 is fast and efficient
Both CLI and go-binaries now use same approach:
- Direct directory upload
- compression-level: 1
- path: . for downloads
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Problem: Direct directory upload (even with compression-level: 1) still fails during download. Artifact download failed after 5 retries for go-binaries. Root cause: GitHub Actions artifact download is more reliable with single files than directories containing many files. The 434MB directory with hundreds of binary files is unreliable to download, regardless of compression. Solution: Use tar as a BUNDLING mechanism (not for compression): - tar -cf creates single tarball file (reliable for download/upload) - compression-level: 1 applies to the tarball (fast zstd compression) - tar -xf extracts on download (minimal overhead) Key insight: tar's value is creating a SINGLE FILE, not compression. Single files download reliably; large multi-file directories don't. Performance vs original (tar+gzip): - Original: tar -cvzf (slow gzip compression + bundling) - New: tar -cf (bundling only) + compression-level: 1 (fast zstd) - Savings: Skip gzip (~15-30s), use faster zstd level 1 CLI artifacts remain direct upload (small, works fine). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
🚀 Build Images ReadyImages are ready for commit 5d30617. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-1033-g5d306174ea |
janisz
force-pushed
the
upliad-full-directory
branch
2 times, most recently
from
0f0c6fb to
745369e
Compare
Replace tar -cvzf (gzip) with tar -cf + compression-level: 1 (fast zstd) to reduce artifact upload time in CI builds. - tar -cvzf → tar -cf (no gzip) - compression-level: 1 (fast zstd by GitHub Actions) - tar xvzf → tar -xf Tar bundling still required as single-file artifacts download more reliably than multi-file directories. Performance: - CLI artifact: 60s → 1s (59s improvement) - Go binaries amd64: 60s → 22s (38s improvement) - Total: ~97s (1.6min) saved per build Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
janisz
force-pushed
the
upliad-full-directory
branch
from
745369e to
5d30617
Compare
|
@janisz: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem:
tar -cvzfbundling in artifact uploads took ~60s per artifact due to gzip compression overhead.Solution: Replace gzip with faster approach:
tar -cf(bundling without compression)compression-level: 1(fast zstd compression by GitHub Actions)Considered alternatives:
of binaries)
Why tar without gzip:
compression-level: 1uses fast zstd modePerformance results:
User-facing documentation
above OR is not needed
Testing and quality
functionality is gated by a feature flag
inspected
Automated testing
How I validated my change
passed