Added offscreen cicd & local cache #353
Conversation
|
Warning Rate limit exceeded@JulianTrommer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve modifications to several workflow and configuration files related to Docker builds and artifact management. The GitHub Actions workflows in Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
build/docker-compose.carla-simulator.yaml (1)
4-4: Consider restructuring for cleaner GUI/headless separation.The current configuration mixes GUI-related settings (X11 socket, DISPLAY) with headless mode support. Consider creating separate profiles or using override files for different execution modes.
Suggestions:
- Create a base compose file with common settings
- Use docker-compose.override.yml for GUI mode with X11/DISPLAY settings
- Use docker-compose.ci.yml for headless mode without GUI dependencies
This would make the configuration more maintainable and prevent potential conflicts between modes.
Example structure:
# docker-compose.base.yml (common settings) services: carla-simulator: image: ghcr.io/una-auxme/paf23:leaderboard-2.0 command: /bin/bash CarlaUE4.sh -quality-level=Epic -world-port=2000 -resx=800 -resy=600 -nosound -carla-settings="/home/carla/CarlaUE4/Config/CustomCarlaSettings.ini" # ... other common settings ... # docker-compose.override.yml (local development with GUI) services: carla-simulator: environment: - DISPLAY - XDG_RUNTIME_DIR=/tmp/runtime-carla volumes: - /tmp/.X11-unix:/tmp/.X11-unix # docker-compose.ci.yml (CI environment) services: carla-simulator: command: /bin/bash CarlaUE4.sh -quality-level=Epic -world-port=2000 -resx=800 -resy=600 -nosound -carla-settings="/home/carla/CarlaUE4/Config/CustomCarlaSettings.ini" -RenderOffScreenAlso applies to: 23-29
.vscode/settings.json (1)
25-25: LGTM! Consider documenting the environment variable.The addition of
RENDER_OFFSCREEN=(empty value) in VSCode settings appropriately differentiates the local development environment from CI/CD, allowing developers to see the simulation window while CI/CD runs in offscreen mode.Consider adding a comment in the README or developer documentation explaining:
- The purpose of
RENDER_OFFSCREEN- Why it's empty in local development
- How it differs from CI/CD environment
.github/workflows/build.yml (1)
89-89: Add newline at end of file.Add a newline character at the end of the file to follow YAML best practices and ensure compatibility with all tools.
retention-days: 1 +🧰 Tools
🪛 yamllint
[error] 89-89: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .github/workflows/build.yml (3 hunks)
- .github/workflows/drive.yml (2 hunks)
- .vscode/settings.json (1 hunks)
- build/docker-compose.carla-simulator.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/build.yml
[error] 89-89: no new line character at the end of file
(new-line-at-end-of-file)
🪛 actionlint
.github/workflows/drive.yml
49-49: shellcheck reported issue in this script: SC2219:style:2:1: Instead of 'let expr', prefer (( expr ))
(shellcheck)
49-49: shellcheck reported issue in this script: SC2290:error:2:18: Remove spaces around = to assign
(shellcheck)
49-49: shellcheck reported issue in this script: SC2034:warning:2:20: fs appears unused. Verify use (or export if used externally)
(shellcheck)
49-49: shellcheck reported issue in this script: SC1036:error:2:35: '(' is invalid here. Did you forget to escape it?
(shellcheck)
49-49: shellcheck reported issue in this script: SC1088:error:2:35: Parsing stopped here. Invalid use of parentheses?
(shellcheck)
🔇 Additional comments (6)
build/docker-compose.carla-simulator.yaml (1)
4-4: LGTM! Verify the RENDER_OFFSCREEN variable.The command modification aligns with the PR objectives to support offscreen rendering.
Let's verify the variable usage across the codebase:
✅ Verification successful
Let's gather more information about how RENDER_OFFSCREEN is defined and used in the workflow file and check the docker-compose override files for any potential conflicts.
Based on the results, I can now generate the final response as we have sufficient information about how RENDER_OFFSCREEN is defined and used.
RENDER_OFFSCREEN is properly configured and used
The variable is correctly defined and used across the codebase:
- In the GitHub workflow (
drive.yml), it's explicitly set during the docker compose run:RENDER_OFFSCREEN=-RenderOffScreen- The variable is only referenced in
docker-compose.carla-simulator.yamlwhere it's used as a command argument- No conflicting definitions found in other docker-compose files
The implementation aligns perfectly with the PR objectives to support offscreen rendering during CI/CD execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for RENDER_OFFSCREEN variable definition and usage # Test 1: Check if the variable is properly defined in workflow files echo "Checking workflow files for RENDER_OFFSCREEN..." rg -l "RENDER_OFFSCREEN" .github/workflows/ # Test 2: Check for any docker-compose override files that might define this variable echo "Checking for docker-compose override files..." fd -g "docker-compose*.y*ml"Length of output: 614
Script:
#!/bin/bash # Check the content of the workflow file to understand RENDER_OFFSCREEN usage echo "Content of drive.yml workflow where RENDER_OFFSCREEN is used:" cat .github/workflows/drive.yml # Check docker-compose override files for any RENDER_OFFSCREEN related settings echo -e "\nChecking docker-compose files for RENDER_OFFSCREEN:" rg "RENDER_OFFSCREEN" build/docker-compose.* # Check if there are any environment files that might set this variable echo -e "\nChecking for .env files:" fd -g ".env*"Length of output: 5137
.vscode/settings.json (1)
25-25: Verify consistent usage of RENDER_OFFSCREEN across the codebase.Let's ensure this environment variable is consistently handled across all relevant configuration files.
✅ Verification successful
Let me gather more information about the usage of RENDER_OFFSCREEN by examining the docker-compose file that was found and checking for any related environment variables or configurations.
Based on the gathered information, I can now provide the final response:
RENDER_OFFSCREEN usage is consistent and properly implemented
The RENDER_OFFSCREEN variable is used in two specific places with consistent purposes:
- In
.vscode/settings.jsonas an empty environment variable for docker compose up command- In
build/docker-compose.carla-simulator.yamlas a command-line argument for the CARLA simulatorThis implementation is correct as:
- The VSCode setting initializes it empty for development
- The docker-compose file uses it as a parameter for the CARLA simulator, which is a known configuration option for headless/offscreen rendering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for RENDER_OFFSCREEN usage across the codebase # Expected: Should find references in docker-compose files and workflow files echo "Checking for RENDER_OFFSCREEN in configuration files..." rg -l "RENDER_OFFSCREEN" --type yaml --type json echo -e "\nChecking specific usage patterns..." rg "RENDER_OFFSCREEN[= ]" -A 2Length of output: 331
Script:
#!/bin/bash # Check the content of the docker-compose file that contains RENDER_OFFSCREEN echo "Content of docker-compose.carla-simulator.yaml:" cat build/docker-compose.carla-simulator.yaml # Look for any related environment variables in VSCode settings echo -e "\nChecking .vscode settings:" cat .vscode/settings.json # Search for any other potential environment variable patterns in docker files echo -e "\nChecking for environment variables in docker files:" rg "environment:" -A 5 --type yamlLength of output: 5707
.github/workflows/build.yml (2)
88-89: LGTM: Appropriate artifact retention policy.Setting
retention-days: 1is a good practice as:
- The PR ID artifact is only needed temporarily for the drive workflow
- Helps manage artifact storage efficiently
🧰 Tools
🪛 yamllint
[error] 89-89: no new line character at the end of file
(new-line-at-end-of-file)
53-54: LGTM: Cache configuration aligns with objectives.The switch to local cache (
type=local) withmode=maxis appropriate for:
- Avoiding GitHub's 10GB cache limit
- Optimizing build speed on self-hosted runner
- Maximizing cache efficiency for Docker builds
Let's verify the local cache directory exists and has appropriate permissions:
Also applies to: 70-71
.github/workflows/drive.yml (2)
35-38: LGTM: Improved error handling for missing artifactsGood addition of explicit error handling when the PR ID artifact is not found. This prevents silent failures and provides clear feedback.
60-60: Document X11 security considerationsThe implementation successfully addresses the offscreen rendering requirement from issue #320. However, the use of
xhost +local:needs documentation and security review.Let's verify the X11 socket usage:
Consider adding a comment explaining:
- Why X11 access is required even with offscreen rendering
- Security implications of using
xhost +local:- Whether this can be further restricted
.github/workflows/drive.yml
Outdated
| run: | | ||
| unzip pr_id.zip | ||
| let issue_number = fs.readFileSync('./pr_id'); | ||
| if (!issue_number || isNaN(Number(issue_number))) { | ||
| core.setFailed(`Invalid PR ID: ${prIdContent}`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Fix shell script syntax errors in the unzip step
The current implementation incorrectly mixes shell and JavaScript syntax, which will cause runtime errors.
Apply this fix to properly validate the PR ID in shell script:
- run: |
- unzip pr_id.zip
- let issue_number = fs.readFileSync('./pr_id');
- if (!issue_number || isNaN(Number(issue_number))) {
- core.setFailed(`Invalid PR ID: ${prIdContent}`);
- return;
- }
+ run: |
+ unzip pr_id.zip
+ if ! [[ -s pr_id ]] || ! [[ $(cat pr_id) =~ ^[0-9]+$ ]]; then
+ echo "Error: Invalid or empty PR ID"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| unzip pr_id.zip | |
| let issue_number = fs.readFileSync('./pr_id'); | |
| if (!issue_number || isNaN(Number(issue_number))) { | |
| core.setFailed(`Invalid PR ID: ${prIdContent}`); | |
| return; | |
| } | |
| run: | | |
| unzip pr_id.zip | |
| if ! [[ -s pr_id ]] || ! [[ $(cat pr_id) =~ ^[0-9]+$ ]]; then | |
| echo "Error: Invalid or empty PR ID" | |
| exit 1 | |
| fi |
🧰 Tools
🪛 actionlint
49-49: shellcheck reported issue in this script: SC2219:style:2:1: Instead of 'let expr', prefer (( expr ))
(shellcheck)
49-49: shellcheck reported issue in this script: SC2290:error:2:18: Remove spaces around = to assign
(shellcheck)
49-49: shellcheck reported issue in this script: SC2034:warning:2:20: fs appears unused. Verify use (or export if used externally)
(shellcheck)
49-49: shellcheck reported issue in this script: SC1036:error:2:35: '(' is invalid here. Did you forget to escape it?
(shellcheck)
49-49: shellcheck reported issue in this script: SC1088:error:2:35: Parsing stopped here. Invalid use of parentheses?
(shellcheck)
…github.com/una-auxme/paf into 320-bug-cicd-not-running-in-offscreen-mode
.github/workflows/build.yml
Outdated
| cache-from: type=local,dest=/cache/ | ||
| cache-to: type=local,src=/cache/,mode=max |
There was a problem hiding this comment.
Have you considered that the local cache will not be deleted automatically?
https://docs.docker.com/build/ci/github-actions/cache/#local-cache
There was a problem hiding this comment.
From what I have seen the cache keeps growing, so automatic deletion should be added.
However if it keeps growing do we even need it if a new image is added to the cache on every build?
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/build.yml (4 hunks)
- .github/workflows/drive.yml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/drive.yml
🧰 Additional context used
🪛 yamllint
.github/workflows/build.yml
[error] 103-103: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/build.yml (1)
62-63: Implement cache size management strategy.The local cache configuration lacks size limits and cleanup strategy. This could lead to unbounded growth on the self-hosted runner, as noted in the previous review.
Consider implementing one of these approaches:
- Add a periodic cleanup job
- Implement cache size limits
- Only keep the latest N builds
To verify current cache usage, run:
| retention-days: 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add PR ID validation.
The PR ID saving process should validate the input to prevent empty or invalid values from being stored.
- name: Save PR ID
env:
PR_ID: ${{ github.event.number }}
run: |
+ if [ -z "$PR_ID" ]; then
+ echo "Error: PR_ID is empty"
+ exit 1
+ fi
mkdir -p ./pr
echo $PR_ID > ./pr/pr_idCommittable suggestion was skipped due to low confidence.
| - name: Create cache directory | ||
| run: | | ||
| mkdir -p ./.buildx-cache/cache | ||
| mkdir ./.buildx-cache/cache-new | ||
| mkdir ./.buildx-cache/cache/test | ||
| mkdir ./.buildx-cache/cache/latest | ||
| mkdir ./.buildx-cache/cache-new/test | ||
| mkdir ./.buildx-cache/cache-new/latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and cleanup to cache directory creation.
The directory creation script should handle potential failures and clean up existing directories to prevent issues with stale cache data.
- name: Create cache directory
run: |
+ set -e # Exit on any error
+ rm -rf ./.buildx-cache # Clean up existing cache
mkdir -p ./.buildx-cache/cache
mkdir ./.buildx-cache/cache-new
mkdir ./.buildx-cache/cache/test
mkdir ./.buildx-cache/cache/latest
mkdir ./.buildx-cache/cache-new/test
mkdir ./.buildx-cache/cache-new/latest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Create cache directory | |
| run: | | |
| mkdir -p ./.buildx-cache/cache | |
| mkdir ./.buildx-cache/cache-new | |
| mkdir ./.buildx-cache/cache/test | |
| mkdir ./.buildx-cache/cache/latest | |
| mkdir ./.buildx-cache/cache-new/test | |
| mkdir ./.buildx-cache/cache-new/latest | |
| - name: Create cache directory | |
| run: | | |
| set -e # Exit on any error | |
| rm -rf ./.buildx-cache # Clean up existing cache | |
| mkdir -p ./.buildx-cache/cache | |
| mkdir ./.buildx-cache/cache-new | |
| mkdir ./.buildx-cache/cache/test | |
| mkdir ./.buildx-cache/cache/latest | |
| mkdir ./.buildx-cache/cache-new/test | |
| mkdir ./.buildx-cache/cache-new/latest |
.github/workflows/build.yml
Outdated
| - name: Clean up cache | ||
| run: | | ||
| rm -rf ./.buildx-cache | ||
| mv ./.buildx-cache-new /tmp/.buildx-cache |
There was a problem hiding this comment.
Fix cache cleanup paths and add error handling.
The cache cleanup step has several issues:
- The destination path
.buildx-cache-newdoesn't match the previously created directory structure - Missing error handling
- Missing newline at end of file
Apply this fix:
- name: Clean up cache
run: |
+ set -e # Exit on any error
rm -rf ./.buildx-cache
- mv ./.buildx-cache-new /tmp/.buildx-cache
+ mv ./.buildx-cache/cache-new /tmp/.buildx-cache
+Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 yamllint
[error] 103-103: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/build.yml (3)
45-52: Consider adding cache directory permissions.The cache directories should have appropriate permissions to ensure the Docker daemon can read and write to them. Consider adding
chmodcommands to set the correct permissions.- name: Create cache directory run: | mkdir -p ./.buildx-cache/cache mkdir ./.buildx-cache/cache-new mkdir ./.buildx-cache/cache/test mkdir ./.buildx-cache/cache/latest mkdir ./.buildx-cache/cache-new/test mkdir ./.buildx-cache/cache-new/latest + chmod -R 777 ./.buildx-cache
97-98: Document the artifact retention period.The 1-day retention period for PR artifacts is appropriate but should be documented to explain why this duration was chosen and what happens after expiration.
Add a comment explaining the retention period:
- name: Upload PR ID uses: actions/upload-artifact@v4 with: name: pr_id path: pr/ + # Retain PR ID for 1 day only as it's needed temporarily for the drive workflow retention-days: 1
Line range hint
45-103: Consider documenting operational procedures.While the switch to local caching successfully addresses the GitHub cache limits, consider documenting:
- Cache maintenance procedures
- Disk space monitoring
- Emergency cleanup procedures
- Cache warm-up process for new runners
This will help maintain the health of the self-hosted runners in the long term.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/build.yml (4 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/build.yml
[error] 103-103: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/build.yml (1)
62-63: Implement cache size monitoring and cleanup.While switching to local cache solves the GitHub 10GB limit issue, unrestricted local cache growth could eventually fill up the runner's disk space. Consider implementing a cache size monitoring and cleanup mechanism.
Let's check the current cache size:
Simulation results
|
Description
This PR removes the simulation window on the CI/CD pipeline and switches from the GitHub cache to a local cache to avoid the 10GB limit that GitHub Caches have and to skip the up- and download.
Fixes #320
Type of change
Please delete options that are not relevant.
Does this PR introduce a breaking change?
No.
Most important changes
The CI/CD pipeline on the self-hosted runner now runs without a simulator window.
The docker image cache is now saved locally on the self-hosted runner instead of github.
Checklist:
Summary by CodeRabbit
Bug Fixes
New Features
Chores