fix:Unstable ci#3201
Conversation
WalkthroughThe pull request updates the GitHub Actions workflow to add numerous disk-cleanup and space-preserving steps across Ubuntu and CentOS jobs (Free Disk Space, Extreme Disk Cleanup, Create Log Directories, Cleanup Build Artifacts, Cleanup Test Data), renames "Cleanup" to "Cleanup Build Trees", and inserts cleanup stages between phases without changing core build/test commands. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 (1)
.github/workflows/pika.yml (1)
122-127: Inconsistent cleanup strategies between ubuntu and centos warrant alignment.The two workflows diverge in approach:
Aspect Ubuntu CentOS Pre-run cleanup Removes system tool dirs + $AGENT_TOOLSDIRECTORYSkips $AGENT_TOOLSDIRECTORY(container-only, OK)Artifact removal Broad: find . -name "*.o"andfind . -name "*.a"Specific: find ./build -path "*/CMakeFiles/*.o"Pre-integration cleanup Single post-build step Two separate steps (after unit tests + before integration) Cleanup patterns Explicit directory removal Glob patterns ( log*,db*,dump*)While container environments have different constraints than ubuntu-latest, the glob patterns in centos cleanup (lines 161-165, 167-172) could be too broad and inadvertently remove non-test files if naming conventions aren't strictly enforced. Consider using explicit directory paths like ubuntu does.
Also applies to: 144-172
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pika.yml(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_macos
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_centos
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
.github/workflows/pika.yml (2)
104-113: Ubuntu cleanup flow is well-structured with process termination.The
Cleanup Test Datastep correctly terminates pika and codis processes before cleanup and uses thealways()condition to ensure execution even on test failure. The disk reporting (df -h) at each stage helps monitor progress.
189-196: Critical: CentOS cleanup is missing process termination and working directory.The
Cleanup Test Datastep lacks:
- Process termination: Unlike ubuntu (line 104-113), this doesn't kill pika/codis processes before cleanup. Hanging processes can block file deletion and interfere with subsequent CI runs.
- Working directory: No
working_directory: ${{ github.workspace }}/buildspecified, so*.conf *.conf.bakpatterns will execute in the workspace root instead of the build directory where the config files are located.Apply this diff to fix both issues:
- name: Cleanup Test Data if: always() + working-directory: ${{ github.workspace }}/build run: | + pkill -9 pika || true + pkill -9 codis || true rm -rf master_data slave_data rename_data acl1_data acl2_data acl3_data rm -rf codis_data_1 codis_data_2 rm -rf *.conf *.conf.bak df -hLikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/pika.yml (1)
174-179: Cleanup Before Integration Tests is reasonable but potentially redundant.This step overlaps with the previous "Cleanup After Unit Test" cleanup. If both steps use similar patterns, consider consolidating them or clarifying why two separate cleanup steps are needed before integration tests begin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pika.yml(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (go)
- GitHub Check: build_on_centos
- GitHub Check: build_on_macos
- GitHub Check: build_on_ubuntu
🔇 Additional comments (8)
.github/workflows/pika.yml (8)
22-28: Disk space reclamation step is appropriate for CI stability.The cleanup of cached toolchains is a reasonable approach to free disk space before building. AGENT_TOOLSDIRECTORY is a standard GitHub Actions environment variable available on ubuntu-latest runners.
56-60: Cleanup Build Trees expansion is reasonable.Addition of
./depsdirectory removal expands disk reclamation without affecting build correctness since this runs after build completes. The step runs unconditionally, which is appropriate here.
77-83: Verify that integration tests don't depend on cleaned-up artifacts.This aggressively removes intermediate build artifacts (*.o, *.a files, CMakeFiles, _deps directories). Ensure that the "Start codis, pika master and pika slave" step and integration tests do not depend on any of these removed artifacts, particularly CMake-generated configuration files.
104-113: Cleanup Test Data pattern is sound.The
if: always()guard ensures this cleanup runs regardless of test outcome, which is essential for test stability. The use ofpkill -9and|| truefallbacks is appropriate for test cleanup. Working directory context is correct.
122-127: CentOS Free Disk Space step is contextually appropriate.Removal of
$AGENT_TOOLSDIRECTORYis appropriately omitted here since it's a container environment where this variable may not exist. The cleanup approach aligns with the Ubuntu job's intent.
144-151: CentOS cleanup is more conservative than Ubuntu; verify consistency.Ubuntu's "Cleanup Build Artifacts" (lines 77–83) aggressively deletes all *.o, *.a, CMakeFiles, and _deps recursively, while CentOS's approach here uses path-specific find patterns targeting only
CMakeFiles/*.oandCMakeFiles/*.a. This inconsistency could lead to different disk savings and potentially different test outcomes between the two CI paths.Consider aligning the cleanup strategies, or document the intentional differences if they are deliberate.
161-172: Cleanup After Unit Test is CentOS-specific and could delete unintended artifacts.This step uses broad patterns (log*, db*, dump*, dbsync*, *.log, .tmp, core.) that may match non-test artifacts in the workspace. Additionally, this cleanup lacks an
if: always()guard, so it will not run if the Unit Test step fails—potentially leaving artifacts for subsequent integration tests.Recommend: (1) scope patterns to specific test directories or working directory, (2) add
if: always()to ensure cleanup regardless of Unit Test outcome, (3) consider whether Ubuntu should have similar granular cleanup.
210-217: Cleanup Test Data for CentOS follows best practices.The
if: always()guard and directory removal pattern aligns with the Ubuntu cleanup approach and is appropriate for test data lifecycle management.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/support/server.tcl(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
- GitHub Check: build_on_ubuntu
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
tests/support/server.tcl (1)
57-57: LGTM! Necessary variable initialization.The
waitvariable must be initialized before being incremented at line 59. Without this initialization, Tcl would throw an error.
| while 1 { | ||
| # check that the server actually started and is ready for connections | ||
| if {[exec grep "going to start" | wc -l < $stderr] > 0} { | ||
| if {[catch {exec grep "going to start" $stderr}] == 0} { | ||
| break | ||
| } | ||
| puts "Fuck YYB" | ||
| after 10 | ||
| } |
There was a problem hiding this comment.
Add timeout mechanism to prevent infinite loop.
The catch-wrapped grep is an improvement for error handling, but this loop has no timeout or retry limit. If the server never writes "going to start" to stderr (due to startup failure, message format change, or file permission issues), tests will hang indefinitely in CI.
Consider adding a timeout mechanism similar to the server_is_up function:
+ set wait_count 0
+ set max_wait [expr {$::valgrind ? 10000 : 1000}]
while 1 {
# check that the server actually started and is ready for connections
if {[catch {exec grep "going to start" $stderr}] == 0} {
break
}
+ incr wait_count
+ if {$wait_count >= $max_wait} {
+ set err "Server started but ready message not found in stderr after waiting"
+ append err "\nSTDERR content:\n" [exec cat $stderr]
+ start_server_error $config_file $err
+ return
+ }
after 10
}📝 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.
| while 1 { | |
| # check that the server actually started and is ready for connections | |
| if {[exec grep "going to start" | wc -l < $stderr] > 0} { | |
| if {[catch {exec grep "going to start" $stderr}] == 0} { | |
| break | |
| } | |
| puts "Fuck YYB" | |
| after 10 | |
| } | |
| set wait_count 0 | |
| set max_wait [expr {$::valgrind ? 10000 : 1000}] | |
| while 1 { | |
| # check that the server actually started and is ready for connections | |
| if {[catch {exec grep "going to start" $stderr}] == 0} { | |
| break | |
| } | |
| incr wait_count | |
| if {$wait_count >= $max_wait} { | |
| set err "Server started but ready message not found in stderr after waiting" | |
| append err "\nSTDERR content:\n" [exec cat $stderr] | |
| start_server_error $config_file $err | |
| return | |
| } | |
| after 10 | |
| } |
🤖 Prompt for AI Agents
In tests/support/server.tcl around lines 293-299, the while loop waiting for the
"going to start" message can spin forever if the message never appears; add a
timeout/retry limit like server_is_up by recording a start timestamp or max
iterations, incrementing a counter or checking elapsed time each loop, and exit
the loop with an error or nonzero return when the timeout is exceeded (and
optionally log the stderr file contents for diagnostics) instead of looping
indefinitely.
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/pika.yml (1)
216-246: macOS workflow is missing Cleanup Test Data and Full Artifact Cleanup steps.The macOS workflow (lines 216–246) has incomplete cleanup compared to ubuntu/centos:
- Lines 216–220: Only removes
buildtrees; does not remove./depsor build artifacts (*.o, *.a, CMakeFiles).- Post-test cleanup: No
Cleanup Test Datastep withif: always()to terminate pika/codis processes and remove test data directories.- Line 246: Integration tests (
integrate_test.sh) are commented out, which may be intentional but needs clarification.These gaps mean test data and processes accumulate on macOS runners, potentially causing the same CI instability you're trying to fix.
Align macOS cleanup with ubuntu/centos by adding the missing steps:
- name: Cleanup run: | cp deps/lib/libz.1.dylib . cp deps/lib/libz.1.dylib tests/integration/ rm -rf ./buildtrees + rm -rf ./deps +- name: Cleanup Build Artifacts + run: | + find ./build -name "*.o" -type f -delete + find ./build -name "*.a" -type f -delete + rm -rf ./build/CMakeFiles + rm -rf ./build/_deps + df -h - name: Test ... - name: Unit Test ... - name: Start codis, pika master and pika slave ... - name: Run Go E2E Tests ... +- name: Cleanup Test Data + if: always() + working-directory: ${{ github.workspace }} + run: | + pkill -9 pika || true + pkill -9 codis || true + rm -rf master_data slave_data rename_data acl1_data acl2_data acl3_data + rm -rf codis_data_1 codis_data_2 + rm -rf *.conf *.conf.bak + df -hAdditionally, if line 246 (
integrate_test.sh) is intentionally disabled, add a comment explaining why. If it should be enabled, uncomment it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pika.yml(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_centos
- GitHub Check: build_on_macos
- GitHub Check: build_on_ubuntu
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
.github/workflows/pika.yml (4)
22-28: Free Disk Space cleanup is appropriate for ubuntu runner.Pre-build disk reclamation removes non-essential GitHub Actions toolchain directories, reducing OOM/disk-full risk during compilation.
56-83: Cleanup Build Trees and Build Artifacts strategy is sound.Post-build artifact removal targets object files, static libraries, and CMake intermediates, reducing disk footprint before tests. The dual-cleanup approach (buildtrees/deps followed by object files/CMake) is intentional and appropriate.
104-113: Cleanup Test Data step improves CI stability significantly.The
if: always()guard ensures process termination and test-data cleanup even on test failure, eliminating resource leaks and data contamination for subsequent runs. Targeting only test-specific directories (master_data, slave_data, codis_data_*, *.conf) avoids unintended deletions.
122-148: CentOS Free Disk Space and Cleanup Build Trees align with ubuntu pattern.The omission of
$AGENT_TOOLSDIRECTORYremoval in centos is appropriate; container environments may lack this GitHub Actions variable. Cleanup strategy mirrors ubuntu.
| - name: Cleanup Test Data | ||
| if: always() | ||
| working-directory: ${{ github.workspace }}/build | ||
| run: | | ||
| rm -rf master_data slave_data rename_data acl1_data acl2_data acl3_data | ||
| rm -rf codis_data_1 codis_data_2 | ||
| rm -rf *.conf *.conf.bak | ||
| df -h |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add process termination to CentOS Cleanup Test Data for consistency.
The ubuntu Cleanup Test Data (lines 108–109) explicitly terminates pika and codis processes, but the centos equivalent (lines 175–182) omits pkill commands. To ensure consistent cleanup and prevent stale processes from affecting subsequent runs, align centos with ubuntu:
- name: Cleanup Test Data
if: always()
working-directory: ${{ github.workspace }}/build
run: |
+ pkill -9 pika || true
+ pkill -9 codis || true
rm -rf master_data slave_data rename_data acl1_data acl2_data acl3_data
rm -rf codis_data_1 codis_data_2
rm -rf *.conf *.conf.bak
df -h📝 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: Cleanup Test Data | |
| if: always() | |
| working-directory: ${{ github.workspace }}/build | |
| run: | | |
| rm -rf master_data slave_data rename_data acl1_data acl2_data acl3_data | |
| rm -rf codis_data_1 codis_data_2 | |
| rm -rf *.conf *.conf.bak | |
| df -h | |
| - name: Cleanup Test Data | |
| if: always() | |
| working-directory: ${{ github.workspace }}/build | |
| run: | | |
| pkill -9 pika || true | |
| pkill -9 codis || true | |
| rm -rf master_data slave_data rename_data acl1_data acl2_data acl3_data | |
| rm -rf codis_data_1 codis_data_2 | |
| rm -rf *.conf *.conf.bak | |
| df -h |
🤖 Prompt for AI Agents
.github/workflows/pika.yml around lines 175 to 182: the CentOS "Cleanup Test
Data" step removes files but does not terminate pika/codis processes like the
Ubuntu cleanup does; add the same pkill commands used in the Ubuntu cleanup to
kill pika and codis (e.g., pkill -9 -f pika and pkill -9 -f codis) before
removing data files and configs so leftover processes are terminated and cleanup
is consistent between runners.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/pika.yml (1)
195-202: Add process termination to match Ubuntu cleanup pattern.The CentOS cleanup (lines 195–202) is missing the process termination commands that are present in the Ubuntu equivalent (lines 108–109). To ensure consistent cleanup and prevent stale processes from affecting subsequent runs, add process termination before file removal:
- name: Cleanup Test Data if: always() working-directory: ${{ github.workspace }}/build run: | + pkill -9 pika || true + pkill -9 codis || true rm -rf master_data slave_data rename_data acl1_data acl2_data acl3_data rm -rf codis_data_1 codis_data_2 rm -rf *.conf *.conf.bak df -hThis aligns CentOS cleanup with Ubuntu and prevents resource leaks or file-lock conflicts in subsequent CI runs.
🧹 Nitpick comments (2)
.github/workflows/pika.yml (2)
166-170: Consider restricting cleanup patterns to reduce unintended matches.The glob patterns
log*,db*,dump*at the workspace root could match unintended files. While unlikely in a CI context starting from a clean checkout, prefixing with./(e.g.,./log*,./db*) would be more defensive and explicit about scope.run: | # Clean up test data to free space before integration tests - rm -rf ./log* ./db* ./dump* ./dbsync* || true + rm -rf ./log* ./db* ./dump* ./dbsync* || true
172-176: Hardcoded absolute paths suggest potential cleanup/setup mismatch.Creating hardcoded absolute paths after cleanup raises questions: If these directories are needed by the start scripts, why are they deleted in the cleanup step (lines 166–170)? The presence of both suggests either:
- The cleanup step is over-aggressive (deleting directories that should persist), or
- The start scripts should create these directories themselves.
Additionally, the paths include GitHub Actions runner-specific workspace paths (
/__w/pikiwidb/pikiwidb/). Using${{ github.workspace }}would be more maintainable. Consider investigating whether the start scripts should handle directory initialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pika.yml(7 hunks)tests/support/server.tcl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/support/server.tcl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
.github/workflows/pika.yml (6)
22-28: ✓ Appropriate pre-build disk cleanup.Removing large optional tool caches before building reduces disk pressure and is a standard practice. This is well-placed early in the workflow.
56-60: ✓ Extended cleanup to build dependencies.Removing both build trees and
./depsdirectory after build is complete frees space for subsequent phases without affecting the binary artifact.
77-83: ✓ Strategic removal of compilation artifacts.Deleting object and static library files after the binary is built and uploaded is a sound approach to reclaim disk space before test phases.
104-113: ✓ Robust process termination and cleanup with guards.The
always()condition, explicit process termination, and safe error suppression (|| true) provide a reliable cleanup pattern. Process termination before file removal prevents resource conflicts.
122-132: ✓ Aggressive container cleanup is appropriate here.Filesystem-wide cleanup of caches, logs, and temp files is safe within an isolated container and follows CI best practices. The error suppression ensures cleanup doesn't fail the job if some paths can't be accessed.
149-156: ✓ Consistent build cleanup across platforms.The CentOS cleanup mirrors Ubuntu's approach, combining build tree and intermediate artifact removal in one step with proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/pika.yml (1)
217-224: CentOS Cleanup Test Data missing process termination (duplicate from past review).The CentOS Cleanup Test Data step (lines 217–224) does not terminate pika and codis processes before cleanup, unlike the Ubuntu step (lines 108–109). This inconsistency risks leaving stale processes that could affect subsequent test runs or cause resource conflicts.
Align CentOS with Ubuntu by adding process termination:
- name: Cleanup Test Data if: always() working-directory: ${{ github.workspace }}/build run: | + pkill -9 pika || true + pkill -9 codis || true rm -rf master_data slave_data rename_data acl1_data acl2_data acl3_data rm -rf codis_data_1 codis_data_2 rm -rf *.conf *.conf.bak df -h
🧹 Nitpick comments (2)
.github/workflows/pika.yml (2)
128-131: Avoid aggressive recursive filesystem scans across/in Free Disk Space step.Lines 128–131 use
find / ...to recursively search and delete files across the entire filesystem. This approach is problematic in several ways:
- Performance: Scanning the entire filesystem is expensive and can significantly slow down the CI pipeline.
- Safety: Recursive operations on
/can delete files outside the workspace and affect the CI environment or container.- Permission issues: Many directories will be inaccessible, generating noise in logs despite
2>/dev/null || true.Instead, scope cleanup to the workspace and known temporary directories within the container:
- name: Free Disk Space run: | rm -rf /usr/share/dotnet rm -rf /opt/ghc rm -rf /usr/local/share/boost - # 彻底清理临时文件 - find / -type f -name "*.log" -delete 2>/dev/null || true - find / -type f -name "*.tmp" -delete 2>/dev/null || true - find / -name '*cache*' -type d -exec rm -rf {} + 2>/dev/null || true - find / -name '*.bak' -type f -delete 2>/dev/null || true + # Clean workspace and common temporary directories only + find ${{ github.workspace }} -type f \( -name "*.log" -o -name "*.tmp" -o -name "*.bak" \) -delete || true + rm -rf /tmp/* /var/tmp/* 2>/dev/null || true df -h
172-191: Extreme Disk Cleanup step deletes.gitdirectory and runs expensiveduscan.Two concerns in this step:
Line 186:
rm -rf ${{ github.workspace }}/.git || truedeletes the git repository metadata. While this frees space, it may break downstream operations that rely on git (e.g., version info, commit hashes). Verify this is intentional and won't break the build or test phases.Line 191:
du -h --max-depth=2 / 2>/dev/null | sort -hr | head -20scans the entire filesystem for reporting. This is expensive and generally unnecessary for cleanup—reserve such diagnostics for failure debugging rather than normal cleanup flow.Consider:
- Removing the
.gitdeletion unless it is a confirmed blocker for space.- Removing or deferring the
duscan to a separate diagnostics step that runs only on failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pika.yml(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (go)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
🔇 Additional comments (2)
.github/workflows/pika.yml (2)
104-113: Ubuntu Cleanup Test Data implementation is correct.This step properly terminates processes and cleans up test artifacts with appropriate error suppression. No issues flagged.
77-83: Ubuntu Cleanup Build Artifacts step is sound.Targeted removal of object files, static libraries, and CMake build artifacts without affecting the workspace or git metadata. Implementation follows best practices for intermediate build cleanup.
| - name: Create Log Directories | ||
| run: | | ||
| mkdir -p /__w/pikiwidb/pikiwidb/codis/admin/../log | ||
| mkdir -p /__w/pikiwidb/pikiwidb/log | ||
| mkdir -p ./bin || true | ||
| df -h |
There was a problem hiding this comment.
Hardcoded absolute path in Create Log Directories step may not work across environments.
Line 195 uses a hardcoded absolute path:
mkdir -p /__w/pikiwidb/pikiwidb/codis/admin/../log
This path is specific to the GitHub Actions runner layout and will fail if:
- The runner uses a different workspace path.
- The step runs in a different environment or local CI setup.
- The directory structure changes.
Replace with a workspace-relative path using the provided variable:
- name: Create Log Directories
run: |
- mkdir -p /__w/pikiwidb/pikiwidb/codis/admin/../log
- mkdir -p /__w/pikiwidb/pikiwidb/log
+ mkdir -p ${{ github.workspace }}/codis/log
+ mkdir -p ${{ github.workspace }}/log
mkdir -p ./bin || true
df -h🤖 Prompt for AI Agents
.github/workflows/pika.yml around lines 193 to 198: the Create Log Directories
step uses a hardcoded absolute path (/__w/pikiwidb/...) which will break on
different runners; change those mkdir calls to use the workspace-relative
variable (use $GITHUB_WORKSPACE or the Actions expression ${{ github.workspace
}}) so the directories are created relative to the current workspace (e.g.,
mkdir -p "$GITHUB_WORKSPACE/pikiwidb/codis/admin/../log" and similarly for the
other paths), ensuring portability across environments.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/pika.yml (2)
194-195: Replace hardcoded absolute path with workspace-relative variable.Line 194–195 uses hardcoded absolute paths specific to GitHub Actions runner layout. This will fail on different runners or environments.
Apply this diff:
- name: Create Log Directories run: | - mkdir -p /__w/pikiwidb/pikiwidb/codis/admin/../log - mkdir -p /__w/pikiwidb/pikiwidb/log + mkdir -p ${{ github.workspace }}/codis/log + mkdir -p ${{ github.workspace }}/log mkdir -p ./bin || true df -h
216-223: Add process termination to CentOS Cleanup Test Data for consistency.The Ubuntu Cleanup Test Data (lines 108–109) explicitly terminates pika and codis processes, but the CentOS equivalent (lines 216–223) omits
pkillcommands. This inconsistency risks leaving stale processes that could interfere with subsequent runs.Apply this diff to align with Ubuntu cleanup:
- name: Cleanup Test Data if: always() working-directory: ${{ github.workspace }}/build run: | + pkill -9 pika || true + pkill -9 codis || true rm -rf master_data slave_data rename_data acl1_data acl2_data acl3_data rm -rf codis_data_1 codis_data_2 rm -rf *.conf *.conf.bak df -h
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pika.yml(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
- GitHub Check: build_on_ubuntu
- GitHub Check: Analyze (go)
| - name: Extreme Disk Cleanup | ||
| run: | | ||
|
|
||
| rm -rf /usr/local/share/* || true | ||
| rm -rf /usr/share/doc/* || true | ||
| rm -rf /usr/share/man/* || true | ||
| rm -rf /var/cache/* || true | ||
|
|
||
| find ${{ github.workspace }} -name "*.o" -type f -delete || true | ||
| find ${{ github.workspace }} -name "*.a" -type f -delete || true | ||
| find ${{ github.workspace }} -name "*.la" -type f -delete || true | ||
| find ${{ github.workspace }} -name "*.so" -type f -delete || true | ||
| find ${{ github.workspace }} -name "*.pyc" -type f -delete || true | ||
|
|
||
| rm -rf ${{ github.workspace }}/.git || true | ||
|
|
||
| df -h | ||
|
|
||
| echo "Largest directories:" | ||
| du -h --max-depth=2 / 2>/dev/null | sort -hr | head -20 |
There was a problem hiding this comment.
Extreme Disk Cleanup is overly aggressive and risks breaking the build.
This step removes files that may be needed during the build or for binary execution:
- Line 182 (
find ... *.so ... -delete): Removing shared libraries could break the pika binary if it depends on runtime.sofiles in the build directory or system paths. - Line 185 (
rm -rf ... .git): Removing the.gitdirectory in CI workflows is risky; some build steps may reference git metadata or history. - Lines 174–177: System-level cleanup (
/usr/local/share,/usr/share/doc,/var/cache) may interfere with dependencies or system tools needed by the build.
Consider replacing this with targeted, phase-specific cleanup:
- Clean only after successful builds and before the next phase.
- Preserve
.git, shared libraries, and system dependencies. - Target only known-safe directories (build artifacts, test data, caches).
Suggested replacement (conservative approach):
- name: Extreme Disk Cleanup
run: |
-
- rm -rf /usr/local/share/* || true
- rm -rf /usr/share/doc/* || true
- rm -rf /usr/share/man/* || true
- rm -rf /var/cache/* || true
-
- find ${{ github.workspace }} -name "*.o" -type f -delete || true
- find ${{ github.workspace }} -name "*.a" -type f -delete || true
- find ${{ github.workspace }} -name "*.la" -type f -delete || true
- find ${{ github.workspace }} -name "*.so" -type f -delete || true
- find ${{ github.workspace }} -name "*.pyc" -type f -delete || true
-
- rm -rf ${{ github.workspace }}/.git || true
-
- df -h
-
- echo "Largest directories:"
- du -h --max-depth=2 / 2>/dev/null | sort -hr | head -20
+ # Clean only CMake intermediate files and test artifacts
+ find ${{ github.workspace }}/build -name "*.o" -type f -delete || true
+ find ${{ github.workspace }}/build -name "*.a" -type f -delete || true
+ rm -rf ${{ github.workspace }}/build/CMakeFiles || true
+ rm -rf ${{ github.workspace }}/build/_deps || true
+
+ # Do NOT remove:
+ # - .so files (may be runtime dependencies)
+ # - .git (git metadata may be needed)
+ # - System files (could break build tools)
+
+ df -h🤖 Prompt for AI Agents
.github/workflows/pika.yml around lines 171 to 190: the "Extreme Disk Cleanup"
step is overly aggressive (removing system dirs, .so files and the repo .git)
and risks breaking later build steps; replace it with a conservative,
phase-targeted cleanup that only removes known build artifacts and temporary
files (e.g., specific build output directories and compiled object/temp files
inside the workspace), never rm -rf system folders or the workspace .git, avoid
deleting *.so runtime libraries, and run this cleanup only after successful
build/test phases (or scope it to explicit safe paths and rely on actions/cache
for dependency cleanup).
* fix:unstable_CI --------- Co-authored-by: chejinge <chejinge@360.cn>
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.