Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds automated daily ClickHouse Docker volume backups to remote storage, retention rotation, restore tooling with container health checks, systemd scheduling, and an operational runbook. The previous migration documentation is removed. ChangesClickHouse backup and restore
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SystemdTimer
participant BackupService
participant BackupScript
participant ClickHouseDocker
participant StorageBox
SystemdTimer->>BackupService: trigger daily backup
BackupService->>BackupScript: execute backup script
BackupScript->>ClickHouseDocker: resolve volume and archive data
BackupScript->>StorageBox: upload archive via SCP
BackupScript->>StorageBox: rotate expired archives
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 7
🧹 Nitpick comments (4)
clickhouse/clickhouse-backup.service (1)
14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
MemoryMaxinstead of legacyMemoryLimit.
MemoryLimitis a cgroup v1 legacy directive. On systems with cgroup v2 (default on most modern Linux distributions),MemoryMaxis the correct directive andMemoryLimitmay not be honored as expected. Also noteIOWeight=100is the systemd default — if the intent is to minimize IO impact on the running ClickHouse instance, a lower value (e.g.,IOWeight=10) would be more effective.🤖 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 `@clickhouse/clickhouse-backup.service` around lines 14 - 17, Update the resource-limit directives in the service configuration to use the cgroup v2-compatible MemoryMax setting instead of legacy MemoryLimit, and lower IOWeight from its default value to reduce backup I/O impact on ClickHouse.Source: Linters/SAST tools
clickhouse/backup-clickhouse.sh (1)
72-111: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffConsider adding post-upload verification.
The backup is uploaded via
scpbut never verified on the remote side. A size check or checksum comparison would catch silent transfer corruption or truncation, especially important for a daily automated pipeline where failures could go unnoticed until a restore is needed.🤖 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 `@clickhouse/backup-clickhouse.sh` around lines 72 - 111, The perform_backup function should verify the remote backup after scp succeeds, using a remote size or checksum comparison against the local archive before reporting success. Add this validation to the upload flow, retain cleanup on verification failure, and ensure failures return an error instead of marking the backup complete.clickhouse/restore-clickhouse.sh (2)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sedpattern doesn't escape the.beforetar.
s/.tar//matches any character beforetar(e.g.,Xtar). Uses/\.tar//for precision. In practice this works because filenames follow theclickhouse-backup-YYYY-MM-DD.tarpattern, but the regex is technically imprecise.♻️ Proposed fix
- date=$(echo "$filename" | sed 's/clickhouse-backup-//;s/.tar//') + date=$(echo "$filename" | sed 's/clickhouse-backup-//;s/\.tar//')🤖 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 `@clickhouse/restore-clickhouse.sh` at line 41, Update the sed expression in the filename parsing assignment to escape the dot in the tar suffix, so only the literal “.tar” extension is removed while preserving the existing prefix removal.
182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare and assign separately to avoid masking return values (SC2155).
local current_backup=...andlocal archive_size=...mask the exit code of the command substitution. Static analysis flags both lines.♻️ Proposed fix
- local current_backup="${volume_path}.pre-restore-$(date '+%Y%m%d-%H%M%S')" + local current_backup + current_backup="${volume_path}.pre-restore-$(date '+%Y%m%d-%H%M%S')"- local archive_size=$(du -h "$temp_backup" | cut -f1) + local archive_size + archive_size=$(du -h "$temp_backup" | cut -f1)Also applies to: 198-198
🤖 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 `@clickhouse/restore-clickhouse.sh` at line 182, Update the restore script’s backup and archive-size variable declarations to avoid combining local declaration with command substitution. Declare current_backup and archive_size separately, then assign each command-substitution result afterward, preserving the existing values and execution flow.Source: Linters/SAST tools
🤖 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 `@clickhouse/BACKUP_SETUP.md`:
- Around line 170-171: Update the SSH setup instructions around the private-key
copy commands to avoid copying or duplicating private key material into root’s
SSH directory. Recommend using ssh-copy-id or invoking SSH with sudo while
reusing the existing user SSH configuration, without assuming a fixed id_rsa key
name.
- Line 7: Update BACKUP_SETUP.md to consistently document uncompressed .tar
archives matching backup-clickhouse.sh and the restore script: change .tar.gz
examples and tar -czf to .tar and tar -cf, remove gzip/compression wording and
size claims, and revise transfer/archive descriptions accordingly.
In `@clickhouse/backup-clickhouse.sh`:
- Around line 107-110: In the upload failure branch, preserve the scp pipeline
exit status before running rm -f "$temp_backup", then use that saved status in
the error call. Update the surrounding backup upload flow without changing the
cleanup behavior.
In `@clickhouse/clickhouse-backup.service`:
- Around line 6-17: Add an explicit TimeoutStartSec setting to the [Service]
configuration for the backup service, using a duration long enough for tar and
scp to complete production backups without systemd terminating them. Keep the
existing backup command and resource limits unchanged.
In `@clickhouse/restore-clickhouse.sh`:
- Around line 182-189: The safety backup created in the restore flow is never
cleaned up, allowing repeated restores to accumulate full-volume copies. Update
the backup handling around current_backup and the restore success path to remove
the pre-restore backup after a successful restore, while retaining it when
restoration fails for recovery. Ensure cleanup uses the existing backup path
safely and preserves the empty-volume behavior.
- Around line 193-215: Validate the downloaded archive in the restore flow
before the destructive volume cleanup: run tar integrity/listing verification on
temp_backup and abort with the existing error-handling pattern if it fails. Only
execute the rm -rf cleanup and subsequent extraction after verification
succeeds, preserving the existing successful extraction path.
- Around line 37-54: Update list_backups around the size extraction and
human-readable conversion to validate that size is non-empty and numeric before
running the integer comparisons. For invalid sftp metadata, skip that backup
entry or otherwise continue gracefully without triggering set -e; preserve the
existing formatting for valid sizes.
---
Nitpick comments:
In `@clickhouse/backup-clickhouse.sh`:
- Around line 72-111: The perform_backup function should verify the remote
backup after scp succeeds, using a remote size or checksum comparison against
the local archive before reporting success. Add this validation to the upload
flow, retain cleanup on verification failure, and ensure failures return an
error instead of marking the backup complete.
In `@clickhouse/clickhouse-backup.service`:
- Around line 14-17: Update the resource-limit directives in the service
configuration to use the cgroup v2-compatible MemoryMax setting instead of
legacy MemoryLimit, and lower IOWeight from its default value to reduce backup
I/O impact on ClickHouse.
In `@clickhouse/restore-clickhouse.sh`:
- Line 41: Update the sed expression in the filename parsing assignment to
escape the dot in the tar suffix, so only the literal “.tar” extension is
removed while preserving the existing prefix removal.
- Line 182: Update the restore script’s backup and archive-size variable
declarations to avoid combining local declaration with command substitution.
Declare current_backup and archive_size separately, then assign each
command-substitution result afterward, preserving the existing values and
execution flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6999fada-1598-4147-90a4-6440d08db2a9
📒 Files selected for processing (6)
clickhouse/BACKUP_SETUP.mdclickhouse/MIGRATION.mdclickhouse/backup-clickhouse.shclickhouse/clickhouse-backup.serviceclickhouse/clickhouse-backup.timerclickhouse/restore-clickhouse.sh
💤 Files with no reviewable changes (1)
- clickhouse/MIGRATION.md
|
|
||
| ## Overview | ||
|
|
||
| - **Backup Method**: Compressed tar archives transferred via SCP |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Documentation says .tar.gz (compressed) but scripts use .tar (uncompressed) — major cross-file inconsistency.
The backup script (backup-clickhouse.sh) explicitly uses tar -cf with no compression (comment: "no compression - ClickHouse data is already compressed"), and the restore script uses tar -xf with no decompression. However, this document consistently references .tar.gz, compression, and gzip:
- Line 7: "Compressed tar archives" → should be "Uncompressed tar archives"
- Lines 117-119:
clickhouse-backup-2025-11-06.tar.gz→ should be.tar - Line 155: "Downloads compressed archive" → should be "Downloads archive"
- Line 264:
tar -czf /tmp/test-backup.tar.gz→ should betar -cf /tmp/test-backup.tar - Lines 285-286: "compressed tar.gz archives" and "Compression typically reduces size by 50-70%" → not applicable
- Line 369: "creates tar.gz archive" → should be "creates tar archive"
- Line 374: "compressed archive size" → should be "archive size"
- Line 378: "Transfers compressed data only" → incorrect
This could cause operators to look for .tar.gz files that don't exist, or run manual test commands with compression that produce different archives.
Also applies to: 117-119, 155-155, 264-264, 285-286, 369-369, 374-374, 378-378
🤖 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 `@clickhouse/BACKUP_SETUP.md` at line 7, Update BACKUP_SETUP.md to consistently
document uncompressed .tar archives matching backup-clickhouse.sh and the
restore script: change .tar.gz examples and tar -czf to .tar and tar -cf, remove
gzip/compression wording and size claims, and revise transfer/archive
descriptions accordingly.
| cp ~/.ssh/id_rsa /root/.ssh/ | ||
| cp ~/.ssh/config /root/.ssh/ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
SSH private key copy instructions are a security risk.
cp ~/.ssh/id_rsa /root/.ssh/ copies a private key to root's SSH directory. This duplicates the key material, broadens access, and assumes the key is named id_rsa. Consider recommending ssh-copy-id or using sudo with the existing SSH config instead.
🤖 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 `@clickhouse/BACKUP_SETUP.md` around lines 170 - 171, Update the SSH setup
instructions around the private-key copy commands to avoid copying or
duplicating private key material into root’s SSH directory. Recommend using
ssh-copy-id or invoking SSH with sudo while reusing the existing user SSH
configuration, without assuming a fixed id_rsa key name.
| else | ||
| rm -f "$temp_backup" | ||
| error "Upload failed with exit code: ${PIPESTATUS[0]}" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
PIPESTATUS[0] is reset by rm before it's used in the error message.
In the else branch, rm -f "$temp_backup" executes first and overwrites PIPESTATUS, so ${PIPESTATUS[0]} in the error call always reflects rm's exit code (0) rather than scp's. The error message will always read "Upload failed with exit code: 0".
🐛 Proposed fix
log "Upload completed successfully"
rm -f "$temp_backup"
return 0
else
- rm -f "$temp_backup"
- error "Upload failed with exit code: ${PIPESTATUS[0]}"
+ local scp_exit=${PIPESTATUS[0]}
+ rm -f "$temp_backup"
+ error "Upload failed with exit code: $scp_exit"
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.
| else | |
| rm -f "$temp_backup" | |
| error "Upload failed with exit code: ${PIPESTATUS[0]}" | |
| fi | |
| else | |
| local scp_exit=${PIPESTATUS[0]} | |
| rm -f "$temp_backup" | |
| error "Upload failed with exit code: $scp_exit" | |
| fi |
🤖 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 `@clickhouse/backup-clickhouse.sh` around lines 107 - 110, In the upload
failure branch, preserve the scp pipeline exit status before running rm -f
"$temp_backup", then use that saved status in the error call. Update the
surrounding backup upload flow without changing the cleanup behavior.
| [Service] | ||
| Type=oneshot | ||
| User=root | ||
| WorkingDirectory=/home/rybbit | ||
| ExecStart=/bin/bash /home/rybbit/clickhouse/backup-clickhouse.sh | ||
| StandardOutput=journal | ||
| StandardError=journal | ||
|
|
||
| # Resource limits | ||
| CPUQuota=50% | ||
| MemoryLimit=2G | ||
| IOWeight=100 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add TimeoutStartSec — the systemd default of 90s will kill any real backup.
Without an explicit timeout, systemd applies DefaultTimeoutStartSec (typically 90s). A tar + scp of a production ClickHouse volume will almost certainly exceed this, causing systemd to terminate the process mid-backup — leaving orphaned temp files and no uploaded archive.
🔒 Proposed fix
[Service]
Type=oneshot
User=root
+TimeoutStartSec=1h
WorkingDirectory=/home/rybbit
ExecStart=/bin/bash /home/rybbit/clickhouse/backup-clickhouse.sh📝 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.
| [Service] | |
| Type=oneshot | |
| User=root | |
| WorkingDirectory=/home/rybbit | |
| ExecStart=/bin/bash /home/rybbit/clickhouse/backup-clickhouse.sh | |
| StandardOutput=journal | |
| StandardError=journal | |
| # Resource limits | |
| CPUQuota=50% | |
| MemoryLimit=2G | |
| IOWeight=100 | |
| [Service] | |
| Type=oneshot | |
| User=root | |
| TimeoutStartSec=1h | |
| WorkingDirectory=/home/rybbit | |
| ExecStart=/bin/bash /home/rybbit/clickhouse/backup-clickhouse.sh | |
| StandardOutput=journal | |
| StandardError=journal | |
| # Resource limits | |
| CPUQuota=50% | |
| MemoryLimit=2G | |
| IOWeight=100 |
🤖 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 `@clickhouse/clickhouse-backup.service` around lines 6 - 17, Add an explicit
TimeoutStartSec setting to the [Service] configuration for the backup service,
using a duration long enough for tar and scp to complete production backups
without systemd terminating them. Keep the existing backup command and resource
limits unchanged.
| echo "ls ${BACKUP_BASE_DIR}/clickhouse-backup-*.tar" | sftp -b - "$STORAGE_BOX_HOST" 2>/dev/null | grep "clickhouse-backup-" | while read -r line; do | ||
| # Extract filename from sftp output | ||
| filename=$(echo "$line" | awk '{print $NF}') | ||
| if [[ "$filename" == clickhouse-backup-*.tar ]]; then | ||
| date=$(echo "$filename" | sed 's/clickhouse-backup-//;s/.tar//') | ||
| # Get file size | ||
| size=$(echo "$line" | awk '{print $5}') | ||
| # Convert bytes to human readable | ||
| if [ "$size" -gt 1073741824 ]; then | ||
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1073741824}")GB" | ||
| elif [ "$size" -gt 1048576 ]; then | ||
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1048576}")MB" | ||
| else | ||
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1024}")KB" | ||
| fi | ||
| echo " $date ($size_hr)" | ||
| fi | ||
| done | sort -r |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
list_backups may crash under set -e if sftp output format is unexpected.
The integer comparisons [ "$size" -gt 1073741824 ] (lines 45, 47) will fail with "integer expression expected" if awk '{print $5}' returns empty or non-numeric output (e.g., sftp listing format differs, or a file has unexpected metadata). With set -e active, this aborts the entire listing instead of degrading gracefully. Add a numeric guard before the comparison.
🛡️ Proposed fix: guard against non-numeric size
# Get file size
size=$(echo "$line" | awk '{print $5}')
- # Convert bytes to human readable
- if [ "$size" -gt 1073741824 ]; then
+ # Convert bytes to human readable (guard against non-numeric or empty size)
+ if [[ "$size" =~ ^[0-9]+$ ]] && [ "$size" -gt 1073741824 ]; then
size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1073741824}")GB"
- elif [ "$size" -gt 1048576 ]; then
+ elif [[ "$size" =~ ^[0-9]+$ ]] && [ "$size" -gt 1048576 ]; then
size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1048576}")MB"
else
- size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1024}")KB"
+ if [[ "$size" =~ ^[0-9]+$ ]]; then
+ size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1024}")KB"
+ else
+ size_hr="unknown"
+ fi
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.
| echo "ls ${BACKUP_BASE_DIR}/clickhouse-backup-*.tar" | sftp -b - "$STORAGE_BOX_HOST" 2>/dev/null | grep "clickhouse-backup-" | while read -r line; do | |
| # Extract filename from sftp output | |
| filename=$(echo "$line" | awk '{print $NF}') | |
| if [[ "$filename" == clickhouse-backup-*.tar ]]; then | |
| date=$(echo "$filename" | sed 's/clickhouse-backup-//;s/.tar//') | |
| # Get file size | |
| size=$(echo "$line" | awk '{print $5}') | |
| # Convert bytes to human readable | |
| if [ "$size" -gt 1073741824 ]; then | |
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1073741824}")GB" | |
| elif [ "$size" -gt 1048576 ]; then | |
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1048576}")MB" | |
| else | |
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1024}")KB" | |
| fi | |
| echo " $date ($size_hr)" | |
| fi | |
| done | sort -r | |
| echo "ls ${BACKUP_BASE_DIR}/clickhouse-backup-*.tar" | sftp -b - "$STORAGE_BOX_HOST" 2>/dev/null | grep "clickhouse-backup-" | while read -r line; do | |
| # Extract filename from sftp output | |
| filename=$(echo "$line" | awk '{print $NF}') | |
| if [[ "$filename" == clickhouse-backup-*.tar ]]; then | |
| date=$(echo "$filename" | sed 's/clickhouse-backup-//;s/.tar//') | |
| # Get file size | |
| size=$(echo "$line" | awk '{print $5}') | |
| # Convert bytes to human readable (guard against non-numeric or empty size) | |
| if [[ "$size" =~ ^[0-9]+$ ]] && [ "$size" -gt 1073741824 ]; then | |
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1073741824}")GB" | |
| elif [[ "$size" =~ ^[0-9]+$ ]] && [ "$size" -gt 1048576 ]; then | |
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1048576}")MB" | |
| else | |
| if [[ "$size" =~ ^[0-9]+$ ]]; then | |
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1024}")KB" | |
| else | |
| size_hr="unknown" | |
| fi | |
| fi | |
| echo " $date ($size_hr)" | |
| fi | |
| done | sort -r |
🤖 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 `@clickhouse/restore-clickhouse.sh` around lines 37 - 54, Update list_backups
around the size extraction and human-readable conversion to validate that size
is non-empty and numeric before running the integer comparisons. For invalid
sftp metadata, skip that backup entry or otherwise continue gracefully without
triggering set -e; preserve the existing formatting for valid sizes.
| local current_backup="${volume_path}.pre-restore-$(date '+%Y%m%d-%H%M%S')" | ||
| if [ -d "$volume_path" ] && [ "$(ls -A "$volume_path" 2>/dev/null)" ]; then | ||
| log "Creating safety backup of current data: $current_backup" | ||
| cp -r "$volume_path" "$current_backup" || log "Warning: Failed to create safety backup" | ||
| else | ||
| log "Volume is empty, skipping safety backup" | ||
| current_backup="N/A (volume was empty)" | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Safety backup via cp -r of entire volume is never cleaned up and can exhaust disk.
Each restore creates a full copy of the volume at ${volume_path}.pre-restore-<timestamp>. These are never removed — repeated restores accumulate multiple full-data copies on the Docker volume partition, which can fill the disk. Additionally, cp -r of a large ClickHouse dataset can take significant time and space. Consider documenting cleanup instructions or implementing automatic cleanup after a successful restore.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 182-182: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 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 `@clickhouse/restore-clickhouse.sh` around lines 182 - 189, The safety backup
created in the restore flow is never cleaned up, allowing repeated restores to
accumulate full-volume copies. Update the backup handling around current_backup
and the restore success path to remove the pre-restore backup after a successful
restore, while retaining it when restoration fails for recovery. Ensure cleanup
uses the existing backup path safely and preserves the empty-volume behavior.
| if ! scp "${STORAGE_BOX_HOST}:${BACKUP_BASE_DIR}/${backup_file}" "$temp_backup" 2>&1 | tee -a "$LOG_FILE"; then | ||
| rm -f "$temp_backup" | ||
| error "Download failed" | ||
| fi | ||
|
|
||
| local archive_size=$(du -h "$temp_backup" | cut -f1) | ||
| log "Download completed (size: $archive_size)" | ||
|
|
||
| # Clear existing data | ||
| log "Clearing existing data in volume..." | ||
| rm -rf "${volume_path:?}"/* "${volume_path:?}"/.[!.]* "${volume_path:?}"/..?* 2>/dev/null || true | ||
|
|
||
| # Extract archive to volume (no decompression - ClickHouse data is already compressed) | ||
| log "Extracting backup archive..." | ||
| if tar -xf "$temp_backup" -C "$volume_path" 2>&1 | tee -a "$LOG_FILE"; then | ||
| log "Restore completed successfully" | ||
| log "Safety backup retained at: $current_backup" | ||
| rm -f "$temp_backup" | ||
| return 0 | ||
| else | ||
| rm -f "$temp_backup" | ||
| error "Failed to extract archive" | ||
| fi |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
No archive integrity check before destructive rm -rf of volume contents.
The script downloads the archive (line 193), then clears the volume (line 203), then extracts (line 207). If the download is corrupted or truncated, the volume is already empty when extraction fails, leaving no data and only the safety backup (which may have failed silently at line 185). Verify the archive with tar -tf "$temp_backup" > /dev/null before clearing.
🛡️ Proposed fix: verify archive before clearing volume
local archive_size=$(du -h "$temp_backup" | cut -f1)
log "Download completed (size: $archive_size)"
+ # Verify archive integrity before destructive operations
+ log "Verifying archive integrity..."
+ if ! tar -tf "$temp_backup" > /dev/null 2>&1; then
+ rm -f "$temp_backup"
+ error "Archive is corrupted or incomplete, aborting before any data changes"
+ fi
+
# Clear existing data
log "Clearing existing data in volume..."📝 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.
| if ! scp "${STORAGE_BOX_HOST}:${BACKUP_BASE_DIR}/${backup_file}" "$temp_backup" 2>&1 | tee -a "$LOG_FILE"; then | |
| rm -f "$temp_backup" | |
| error "Download failed" | |
| fi | |
| local archive_size=$(du -h "$temp_backup" | cut -f1) | |
| log "Download completed (size: $archive_size)" | |
| # Clear existing data | |
| log "Clearing existing data in volume..." | |
| rm -rf "${volume_path:?}"/* "${volume_path:?}"/.[!.]* "${volume_path:?}"/..?* 2>/dev/null || true | |
| # Extract archive to volume (no decompression - ClickHouse data is already compressed) | |
| log "Extracting backup archive..." | |
| if tar -xf "$temp_backup" -C "$volume_path" 2>&1 | tee -a "$LOG_FILE"; then | |
| log "Restore completed successfully" | |
| log "Safety backup retained at: $current_backup" | |
| rm -f "$temp_backup" | |
| return 0 | |
| else | |
| rm -f "$temp_backup" | |
| error "Failed to extract archive" | |
| fi | |
| if ! scp "${STORAGE_BOX_HOST}:${BACKUP_BASE_DIR}/${backup_file}" "$temp_backup" 2>&1 | tee -a "$LOG_FILE"; then | |
| rm -f "$temp_backup" | |
| error "Download failed" | |
| fi | |
| local archive_size=$(du -h "$temp_backup" | cut -f1) | |
| log "Download completed (size: $archive_size)" | |
| # Verify archive integrity before destructive operations | |
| log "Verifying archive integrity..." | |
| if ! tar -tf "$temp_backup" > /dev/null 2>&1; then | |
| rm -f "$temp_backup" | |
| error "Archive is corrupted or incomplete, aborting before any data changes" | |
| fi | |
| # Clear existing data | |
| log "Clearing existing data in volume..." | |
| rm -rf "${volume_path:?}"/* "${volume_path:?}"/.[!.]* "${volume_path:?}"/..?* 2>/dev/null || true | |
| # Extract archive to volume (no decompression - ClickHouse data is already compressed) | |
| log "Extracting backup archive..." | |
| if tar -xf "$temp_backup" -C "$volume_path" 2>&1 | tee -a "$LOG_FILE"; then | |
| log "Restore completed successfully" | |
| log "Safety backup retained at: $current_backup" | |
| rm -f "$temp_backup" | |
| return 0 | |
| else | |
| rm -f "$temp_backup" | |
| error "Failed to extract archive" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 198-198: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 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 `@clickhouse/restore-clickhouse.sh` around lines 193 - 215, Validate the
downloaded archive in the restore flow before the destructive volume cleanup:
run tar integrity/listing verification on temp_backup and abort with the
existing error-handling pattern if it fails. Only execute the rm -rf cleanup and
subsequent extraction after verification succeeds, preserving the existing
successful extraction path.
…eamline documentation.
… simplify scheduling.
- Added detailed scenarios for restoring backups: existing server and fresh server setups. - Updated the restore script to include prerequisite checks, volume management, and improved logging. - Enhanced the backup service configuration by removing unnecessary options for simplicity. - Improved error handling and logging in backup and restore scripts for better troubleshooting.
…nectivity checks - Changed SSH command from 'echo' to 'pwd' in backup and restore scripts to accommodate Hetzner storage box's restricted shell. - Updated documentation to reflect the change in SSH connection testing and clarified expected output.
…ectivity - Modified rsync commands in both backup and restore scripts to include the SFTP subsystem, ensuring compatibility with Hetzner Storage Box's restricted shell. - Updated comments to clarify the use of the SFTP option.
…rmission preservation - Modified rsync commands in both backup and restore scripts to utilize the `--rsync-path="rsync --fake-super"` option, ensuring compatibility with Hetzner Storage Box's restricted shell. - Updated comments to clarify the use of the fake super option for preserving permissions without root access.
…mproved compatibility - Replaced rsync with tar for creating compressed backup archives, enhancing efficiency and compatibility with Hetzner Storage Box. - Updated backup and restore scripts to utilize SCP for transferring archives, ensuring secure and reliable uploads. - Modified documentation to reflect changes in backup methods and prerequisites, including the removal of rsync references. - Improved error handling and logging for backup and restore processes.
…archives - Changed backup and restore scripts to create and extract uncompressed tar archives instead of compressed ones, as ClickHouse data is already compressed. - Improved error handling during backup creation to account for warnings when files change during the process. - Updated file extensions in backup and restore scripts from `.tar.gz` to `.tar` for consistency.
- Replaced SSH command with SFTP to list available ClickHouse backup files, improving compatibility with Hetzner Storage Box's restricted shell. - Enhanced file size reporting by converting bytes to human-readable format (GB, MB, KB). - Updated the output format for better clarity on backup dates and sizes.
…kup restoration process - Updated logging to indicate the restoration attempt from a specific backup file. - Removed the file existence check due to limitations with Hetzner's restricted shell, noting that SCP will fail if the file does not exist.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
clickhouse/restore-clickhouse.sh (3)
37-54: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNon-numeric/empty
sizecan crash the listing loop underset -e(still unresolved).
[ "$size" -gt 1073741824 ]will error out with "integer expression expected" ifawkreturns empty/non-numeric output, aborting the whole listing instead of degrading gracefully. Unchanged from the prior review round.🛡️ Proposed fix
size=$(echo "$line" | awk '{print $5}') - if [ "$size" -gt 1073741824 ]; then + if [[ "$size" =~ ^[0-9]+$ ]] && [ "$size" -gt 1073741824 ]; then size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1073741824}")GB" - elif [ "$size" -gt 1048576 ]; then + elif [[ "$size" =~ ^[0-9]+$ ]] && [ "$size" -gt 1048576 ]; then size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1048576}")MB" else - size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1024}")KB" + if [[ "$size" =~ ^[0-9]+$ ]]; then + size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1024}")KB" + else + size_hr="unknown" + fi fi🤖 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 `@clickhouse/restore-clickhouse.sh` around lines 37 - 54, Validate the `size` value extracted in the restore listing loop before the numeric comparisons. When `awk` produces an empty or non-numeric value, assign a safe fallback (such as zero or an unavailable-size label) and continue listing without triggering an error under `set -e`; preserve the existing human-readable conversions for valid sizes.
182-189: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSafety backup (
cp -r) is never cleaned up, still unresolved.Each restore leaves a full
.pre-restore-<timestamp>copy of the volume on disk indefinitely; repeated restores accumulate full-data copies and can exhaust disk space. Unchanged from the prior review round.🧰 Proposed fix: clean up after a successful restore
if tar -xf "$temp_backup" -C "$volume_path" 2>&1 | tee -a "$LOG_FILE"; then log "Restore completed successfully" - log "Safety backup retained at: $current_backup" + if [ -d "$current_backup" ]; then + rm -rf "$current_backup" + log "Safety backup removed after successful restore" + fi rm -f "$temp_backup" return 0🤖 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 `@clickhouse/restore-clickhouse.sh` around lines 182 - 189, Update the restore flow around the current_backup safety-copy logic to remove the created .pre-restore backup after a successful restore. Preserve the backup when restoration fails so it remains available for recovery, and avoid attempting cleanup when current_backup is the empty-volume sentinel or when backup creation failed.
193-215: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNo archive integrity check before destructive
rm -rfof volume contents, still unresolved.The volume is cleared (line 203) before the downloaded archive is verified. A truncated/corrupted download leaves the volume empty with extraction failing afterward, relying solely on the (possibly-failed) safety backup. Unchanged from the prior review round.
🛡️ Proposed fix
local archive_size=$(du -h "$temp_backup" | cut -f1) log "Download completed (size: $archive_size)" + # Verify archive integrity before destructive operations + log "Verifying archive integrity..." + if ! tar -tf "$temp_backup" > /dev/null 2>&1; then + rm -f "$temp_backup" + error "Archive is corrupted or incomplete, aborting before any data changes" + fi + # Clear existing data log "Clearing existing data in volume..."🤖 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 `@clickhouse/restore-clickhouse.sh` around lines 193 - 215, The restore flow must validate the downloaded archive before clearing the ClickHouse volume. In the archive handling around the tar extraction and destructive rm -rf, add an integrity/listability check for temp_backup and abort with cleanup via the existing error path if validation fails; only proceed to clear volume_path after validation succeeds.clickhouse/backup-clickhouse.sh (1)
101-111: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
PIPESTATUS[0]is reset byrmbefore it's used in the error message (still unresolved).In the
elsebranch,rm -f "$temp_backup"runs before${PIPESTATUS[0]}is read, so the error always reportsrm's exit code (0), notscp's actual failure code. This is unchanged from the prior review round.🐛 Proposed fix
log "Upload completed successfully" rm -f "$temp_backup" return 0 else - rm -f "$temp_backup" - error "Upload failed with exit code: ${PIPESTATUS[0]}" + local scp_exit=${PIPESTATUS[0]} + rm -f "$temp_backup" + error "Upload failed with exit code: $scp_exit" fi🤖 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 `@clickhouse/backup-clickhouse.sh` around lines 101 - 111, In the failed-upload branch of the backup function, capture the scp pipeline status from PIPESTATUS[0] immediately before running rm, then use that saved value in the error message. Keep the temporary-file cleanup and existing success behavior 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 `@clickhouse/backup-clickhouse.sh`:
- Around line 72-99: Update perform_backup so it does not archive the live
ClickHouse volume while the database is running; use ClickHouse’s snapshot-aware
ALTER TABLE ... FREEZE or native BACKUP flow, or stop the ClickHouse container
before creating the tar archive. Ensure the existing archive and transfer
workflow operates on the consistent snapshot or stopped volume.
In `@clickhouse/restore-clickhouse.sh`:
- Around line 138-168: Update start_container so it returns a nonzero status
when the health-check loop reaches max_wait without observing a healthy
container, instead of falling through after the warning; preserve successful
returns for healthy, already-running, and nonexistent-container cases. In main,
check the result of start_container and, on failure, log the health-check
warning and manual docker logs verification guidance rather than presenting the
restore as unconditionally successful.
- Around line 30-58: Update list_backups so an empty result from the sftp
listing and grep pipeline does not trigger set -euo pipefail termination.
Explicitly handle the no-backups case, preserving the normal listing and sorting
behavior, and ensure the function still prints the usage message before exiting.
---
Duplicate comments:
In `@clickhouse/backup-clickhouse.sh`:
- Around line 101-111: In the failed-upload branch of the backup function,
capture the scp pipeline status from PIPESTATUS[0] immediately before running
rm, then use that saved value in the error message. Keep the temporary-file
cleanup and existing success behavior unchanged.
In `@clickhouse/restore-clickhouse.sh`:
- Around line 37-54: Validate the `size` value extracted in the restore listing
loop before the numeric comparisons. When `awk` produces an empty or non-numeric
value, assign a safe fallback (such as zero or an unavailable-size label) and
continue listing without triggering an error under `set -e`; preserve the
existing human-readable conversions for valid sizes.
- Around line 182-189: Update the restore flow around the current_backup
safety-copy logic to remove the created .pre-restore backup after a successful
restore. Preserve the backup when restoration fails so it remains available for
recovery, and avoid attempting cleanup when current_backup is the empty-volume
sentinel or when backup creation failed.
- Around line 193-215: The restore flow must validate the downloaded archive
before clearing the ClickHouse volume. In the archive handling around the tar
extraction and destructive rm -rf, add an integrity/listability check for
temp_backup and abort with cleanup via the existing error path if validation
fails; only proceed to clear volume_path after validation succeeds.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cf69503-8079-4f15-ab8e-8dd09f1d496d
📒 Files selected for processing (6)
clickhouse/BACKUP_SETUP.mdclickhouse/MIGRATION.mdclickhouse/backup-clickhouse.shclickhouse/clickhouse-backup.serviceclickhouse/clickhouse-backup.timerclickhouse/restore-clickhouse.sh
💤 Files with no reviewable changes (1)
- clickhouse/MIGRATION.md
✅ Files skipped from review due to trivial changes (1)
- clickhouse/clickhouse-backup.timer
🚧 Files skipped from review as they are similar to previous changes (2)
- clickhouse/clickhouse-backup.service
- clickhouse/BACKUP_SETUP.md
| # Perform backup using tar + scp | ||
| perform_backup() { | ||
| local volume_path="$1" | ||
| local backup_date="$2" | ||
| local backup_file="clickhouse-backup-${backup_date}.tar" | ||
| local temp_backup="/tmp/${backup_file}" | ||
|
|
||
| log "Starting backup..." | ||
| log "Source: $volume_path" | ||
| log "Destination: ${STORAGE_BOX_HOST}:${BACKUP_BASE_DIR}/${backup_file}" | ||
|
|
||
| # Create tar archive (no compression - ClickHouse data is already compressed) | ||
| # --ignore-failed-read: Continue even if files are deleted/changed during backup (normal for live DB) | ||
| log "Creating tar archive..." | ||
| if tar -cf "$temp_backup" --ignore-failed-read -C "$volume_path" . 2>&1 | tee -a "$LOG_FILE"; then | ||
| local archive_size=$(du -h "$temp_backup" | cut -f1) | ||
| log "Archive created successfully (size: $archive_size)" | ||
| else | ||
| local tar_exit=$? | ||
| # Exit code 1 means "some files differ" which is normal for live databases | ||
| if [ $tar_exit -eq 1 ]; then | ||
| local archive_size=$(du -h "$temp_backup" | cut -f1) | ||
| log "Archive created with warnings (size: $archive_size) - this is normal for live databases" | ||
| else | ||
| rm -f "$temp_backup" | ||
| error "Failed to create archive (exit code: $tar_exit)" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the backup/restore scripts and any ClickHouse backup docs/config.
git ls-files | rg 'clickhouse|backup|restore|README|docs' -n
printf '\n--- backup-clickhouse.sh outline ---\n'
ast-grep outline clickhouse/backup-clickhouse.sh --view expanded
printf '\n--- restore-related files ---\n'
fd -a 'restore*.sh|*restore*' . 2>/dev/null || true
fd -a 'backup*.sh|*backup*' . 2>/dev/null || true
printf '\n--- search for tar/FREEZE/clickhouse-backup/native BACKUP references ---\n'
rg -n --hidden --glob '!**/.git/**' 'ALTER TABLE .*FREEZE|clickhouse-backup|BACKUP/RESTORE|docker exec|tar -cf|--ignore-failed-read|restore script|stop.*container|shadow/' .Repository: rybbit-io/rybbit
Length of output: 39344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- clickhouse/backup-clickhouse.sh (selected lines) ---\n'
sed -n '1,180p' clickhouse/backup-clickhouse.sh | cat -n
printf '\n--- clickhouse/restore-clickhouse.sh (selected lines) ---\n'
sed -n '1,320p' clickhouse/restore-clickhouse.sh | cat -n
printf '\n--- clickhouse/BACKUP_SETUP.md (selected lines) ---\n'
sed -n '1,220p' clickhouse/BACKUP_SETUP.md | cat -nRepository: rybbit-io/rybbit
Length of output: 24722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- clickhouse/clickhouse-backup.service ---\n'
sed -n '1,200p' clickhouse/clickhouse-backup.service | cat -n
printf '\n--- clickhouse/clickhouse-backup.timer ---\n'
sed -n '1,120p' clickhouse/clickhouse-backup.timer | cat -nRepository: rybbit-io/rybbit
Length of output: 991
Use a snapshot-aware ClickHouse backup path This job tars the live volume while ClickHouse keeps running, so merges/inserts can race with the archive and leave you with a torn backup. Prefer ALTER TABLE ... FREEZE/native BACKUP, or stop the container before archiving the volume.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 87-87: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 93-93: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 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 `@clickhouse/backup-clickhouse.sh` around lines 72 - 99, Update perform_backup
so it does not archive the live ClickHouse volume while the database is running;
use ClickHouse’s snapshot-aware ALTER TABLE ... FREEZE or native BACKUP flow, or
stop the ClickHouse container before creating the tar archive. Ensure the
existing archive and transfer workflow operates on the consistent snapshot or
stopped volume.
| list_backups() { | ||
| log "Listing available backups on storage box..." | ||
| echo "" | ||
| echo "Available backups:" | ||
| echo "==================" | ||
|
|
||
| # Use sftp to list files (works with Hetzner Storage Box restricted shell) | ||
| echo "ls ${BACKUP_BASE_DIR}/clickhouse-backup-*.tar" | sftp -b - "$STORAGE_BOX_HOST" 2>/dev/null | grep "clickhouse-backup-" | while read -r line; do | ||
| # Extract filename from sftp output | ||
| filename=$(echo "$line" | awk '{print $NF}') | ||
| if [[ "$filename" == clickhouse-backup-*.tar ]]; then | ||
| date=$(echo "$filename" | sed 's/clickhouse-backup-//;s/.tar//') | ||
| # Get file size | ||
| size=$(echo "$line" | awk '{print $5}') | ||
| # Convert bytes to human readable | ||
| if [ "$size" -gt 1073741824 ]; then | ||
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1073741824}")GB" | ||
| elif [ "$size" -gt 1048576 ]; then | ||
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1048576}")MB" | ||
| else | ||
| size_hr="$(awk "BEGIN {printf \"%.1f\", $size/1024}")KB" | ||
| fi | ||
| echo " $date ($size_hr)" | ||
| fi | ||
| done | sort -r | ||
|
|
||
| echo "" | ||
| echo "Usage: $0 YYYY-MM-DD" | ||
| exit 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f() {
echo "A" | grep "B" | while read -r line; do echo "$line"; done | sort -r
echo "reached end of f"
}
f
echo "after f"Repository: rybbit-io/rybbit
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f() {
echo "A" | grep "B" | while read -r line; do echo "$line"; done | sort -r
echo "reached end of f"
}
f
echo "after f"Repository: rybbit-io/rybbit
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the restore script around the relevant section and shell options.
cat -n clickhouse/restore-clickhouse.sh | sed -n '1,120p'Repository: rybbit-io/rybbit
Length of output: 4784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the rest of the restore script, especially the main dispatch.
cat -n clickhouse/restore-clickhouse.sh | sed -n '120,260p'Repository: rybbit-io/rybbit
Length of output: 5596
Handle the empty-backup case in list_backups clickhouse/restore-clickhouse.sh:37-54
With set -euo pipefail, a no-match from grep "clickhouse-backup-" makes the whole pipeline fail and exits the script before the usage message. Guard the empty case explicitly or append || true to this pipeline so first-time runs show a clean “no backups found” path.
🤖 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 `@clickhouse/restore-clickhouse.sh` around lines 30 - 58, Update list_backups
so an empty result from the sftp listing and grep pipeline does not trigger set
-euo pipefail termination. Explicitly handle the no-backups case, preserving the
normal listing and sorting behavior, and ensure the function still prints the
usage message before exiting.
| start_container() { | ||
| if ! container_exists; then | ||
| log "Container does not exist, skipping start" | ||
| log "You can now start your container with: docker compose up -d" | ||
| return 0 | ||
| fi | ||
|
|
||
| log "Starting ClickHouse container..." | ||
|
|
||
| if ! docker ps --format '{{.Names}}' | grep -q "^${CONTAINER_NAME}$"; then | ||
| docker start "$CONTAINER_NAME" || error "Failed to start container" | ||
| log "Container started successfully" | ||
|
|
||
| # Wait for container to be healthy | ||
| log "Waiting for container to become healthy..." | ||
| local max_wait=60 | ||
| local waited=0 | ||
| while [ $waited -lt $max_wait ]; do | ||
| if docker inspect --format='{{.State.Health.Status}}' "$CONTAINER_NAME" 2>/dev/null | grep -q "healthy"; then | ||
| log "Container is healthy" | ||
| return 0 | ||
| fi | ||
| sleep 2 | ||
| waited=$((waited + 2)) | ||
| done | ||
|
|
||
| log "Warning: Container did not become healthy within ${max_wait}s" | ||
| else | ||
| log "Container is already running" | ||
| fi | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Health-check timeout doesn't fail start_container, so main() reports success even when the container never became healthy.
When the container doesn't become healthy within max_wait, the function only logs a warning and falls through, returning success (0) by virtue of the last log call. main() then unconditionally prints "Restore completed successfully!" regardless of actual container health, which can mask a broken restore.
🛡️ Proposed fix
done
- log "Warning: Container did not become healthy within ${max_wait}s"
+ log "Warning: Container did not become healthy within ${max_wait}s"
+ return 1
elseAnd in main() (outside this range), use the result to adjust the final message:
# around line 279
if ! start_container; then
log "WARNING: Restore data was written, but the container failed its health check."
echo "Verify manually before trusting this restore: docker logs -f $CONTAINER_NAME"
fi🤖 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 `@clickhouse/restore-clickhouse.sh` around lines 138 - 168, Update
start_container so it returns a nonzero status when the health-check loop
reaches max_wait without observing a healthy container, instead of falling
through after the warning; preserve successful returns for healthy,
already-running, and nonexistent-container cases. In main, check the result of
start_container and, on failure, log the health-check warning and manual docker
logs verification guidance rather than presenting the restore as unconditionally
successful.
1 participant
Summary by CodeRabbit