fix(heimdall): update snapshot download #440
Conversation
WalkthroughThe pull request modifies the Heimdall application's configuration files, specifically enhancing the init container functionality within the Changes
Suggested reviewers
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: 2
🧹 Nitpick comments (2)
charts/heimdall/templates/heimdall/statefulset.yaml (2)
71-76: Enhance lz4 installation robustnessThe installation logic has good error handling but could be improved:
Consider these enhancements:
# Install lz4 dynamically if ! apk add --no-cache lz4; then echo "Failed to install lz4. Trying manual download..." - wget https://dl-cdn.alpinelinux.org/alpine/v3.14/main/x86_64/lz4-1.9.3-r0.apk - apk add --allow-untrusted lz4-1.9.3-r0.apk || { echo "Failed to install lz4 manually. Exiting."; exit 1; } + wget --timeout=30 --tries=3 https://dl-cdn.alpinelinux.org/alpine/v3.14/main/x86_64/lz4-1.9.3-r0.apk + apk add --allow-untrusted lz4-1.9.3-r0.apk || { echo "Failed to install lz4 manually. Exiting."; exit 1; } + rm -f lz4-1.9.3-r0.apk # Cleanup fiConsider:
- Making the lz4 version configurable through Helm values
- Adding package verification using apk keys
110-110: Remove trailing whitespace- fi + fi🧰 Tools
🪛 yamllint (1.35.1)
[error] 110-110: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
charts/heimdall/templates/heimdall/statefulset.yaml(2 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
charts/heimdall/templates/heimdall/statefulset.yaml
[error] 110-110: trailing spaces
(trailing-spaces)
| echo "https://mirror.csclub.uwaterloo.ca/alpine/v3.14/main" > /etc/apk/repositories | ||
| echo "https://mirror.csclub.uwaterloo.ca/alpine/v3.14/community" >> /etc/apk/repositories | ||
| apk update | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving APK repository configuration reliability
The current configuration has several potential issues:
- Single mirror point of failure
- Hardcoded Alpine version
- No fallback repositories
Consider this more resilient approach:
- echo "https://mirror.csclub.uwaterloo.ca/alpine/v3.14/main" > /etc/apk/repositories
- echo "https://mirror.csclub.uwaterloo.ca/alpine/v3.14/community" >> /etc/apk/repositories
+ # Use official repositories first, then mirrors
+ cat > /etc/apk/repositories << EOF
+ https://dl-cdn.alpinelinux.org/alpine/v3.14/main
+ https://dl-cdn.alpinelinux.org/alpine/v3.14/community
+ https://mirror.csclub.uwaterloo.ca/alpine/v3.14/main
+ https://mirror.csclub.uwaterloo.ca/alpine/v3.14/community
+ EOFConsider making the Alpine version configurable through Helm values for better maintainability.
📝 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 "https://mirror.csclub.uwaterloo.ca/alpine/v3.14/main" > /etc/apk/repositories | |
| echo "https://mirror.csclub.uwaterloo.ca/alpine/v3.14/community" >> /etc/apk/repositories | |
| apk update | |
| # Use official repositories first, then mirrors | |
| cat > /etc/apk/repositories << EOF | |
| https://dl-cdn.alpinelinux.org/alpine/v3.14/main | |
| https://dl-cdn.alpinelinux.org/alpine/v3.14/community | |
| https://mirror.csclub.uwaterloo.ca/alpine/v3.14/main | |
| https://mirror.csclub.uwaterloo.ca/alpine/v3.14/community | |
| EOF | |
| apk update | |
| wget -c "${SNAPSHOT_URL}" -O snapshot.tar.lz4 || { echo "Download failed. Exiting."; exit 1; } | ||
| lz4 -d snapshot.tar.lz4 snapshot.tar || { echo "Decompression failed. Exiting."; exit 1; } | ||
| tar -xvf snapshot.tar -C ${HEIMDALLD_HOME}/data || { echo "Extraction failed. Exiting."; exit 1; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve snapshot download reliability and resource management
While the error handling is good, the snapshot management could be enhanced:
Consider these improvements:
- wget -c "${SNAPSHOT_URL}" -O snapshot.tar.lz4 || { echo "Download failed. Exiting."; exit 1; }
- lz4 -d snapshot.tar.lz4 snapshot.tar || { echo "Decompression failed. Exiting."; exit 1; }
- tar -xvf snapshot.tar -C ${HEIMDALLD_HOME}/data || { echo "Extraction failed. Exiting."; exit 1; }
+ # Check available disk space
+ available_space=$(df -P ${HEIMDALLD_HOME} | awk 'NR==2 {print $4}')
+ required_space=$((20 * 1024 * 1024)) # 20GB in KB
+ if [ "${available_space}" -lt "${required_space}" ]; then
+ echo "Insufficient disk space. Required: 20GB, Available: $((available_space/1024/1024))GB. Exiting."; exit 1;
+ fi
+
+ # Download with progress and timeout
+ wget --progress=dot:giga --timeout=3600 -c "${SNAPSHOT_URL}" -O snapshot.tar.lz4.tmp || { echo "Download failed. Exiting."; exit 1; }
+ mv snapshot.tar.lz4.tmp snapshot.tar.lz4
+
+ # Process snapshot with cleanup
+ lz4 -d snapshot.tar.lz4 snapshot.tar || { rm -f snapshot.tar.lz4; echo "Decompression failed. Exiting."; exit 1; }
+ rm -f snapshot.tar.lz4
+
+ tar -xf snapshot.tar -C ${HEIMDALLD_HOME}/data || { rm -f snapshot.tar; echo "Extraction failed. Exiting."; exit 1; }
+ rm -f snapshot.tarConsider:
- Adding SHA256 verification of the downloaded snapshot
- Implementing a retry mechanism for failed downloads
- Adding progress logging for better observability
📝 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.
| wget -c "${SNAPSHOT_URL}" -O snapshot.tar.lz4 || { echo "Download failed. Exiting."; exit 1; } | |
| lz4 -d snapshot.tar.lz4 snapshot.tar || { echo "Decompression failed. Exiting."; exit 1; } | |
| tar -xvf snapshot.tar -C ${HEIMDALLD_HOME}/data || { echo "Extraction failed. Exiting."; exit 1; } | |
| # Check available disk space | |
| available_space=$(df -P ${HEIMDALLD_HOME} | awk 'NR==2 {print $4}') | |
| required_space=$((20 * 1024 * 1024)) # 20GB in KB | |
| if [ "${available_space}" -lt "${required_space}" ]; then | |
| echo "Insufficient disk space. Required: 20GB, Available: $((available_space/1024/1024))GB. Exiting."; exit 1; | |
| fi | |
| # Download with progress and timeout | |
| wget --progress=dot:giga --timeout=3600 -c "${SNAPSHOT_URL}" -O snapshot.tar.lz4.tmp || { echo "Download failed. Exiting."; exit 1; } | |
| mv snapshot.tar.lz4.tmp snapshot.tar.lz4 | |
| # Process snapshot with cleanup | |
| lz4 -d snapshot.tar.lz4 snapshot.tar || { rm -f snapshot.tar.lz4; echo "Decompression failed. Exiting."; exit 1; } | |
| rm -f snapshot.tar.lz4 | |
| tar -xf snapshot.tar -C ${HEIMDALLD_HOME}/data || { rm -f snapshot.tar; echo "Extraction failed. Exiting."; exit 1; } | |
| rm -f snapshot.tar |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
charts/heimdall/templates/heimdall/statefulset.yaml (3)
Line range hint
52-97: Consider improving configuration management approachWhile the current implementation works, consider these improvements for better maintainability and reliability:
- Use a configuration management tool or templating instead of sed
- Add validation for environment variables before using them
- Add logging for successful configuration updates
command: - sh - -c - | set -ex HEIMDALLD_HOME="/storage" + # Validate required environment variables + : "${SEEDS:?Required environment variable SEEDS is not set}" + : "${CORS_ALLOWED_ORIGINS:?Required environment variable CORS_ALLOWED_ORIGINS is not set}" + + echo "Starting Heimdall configuration initialization..." # If config hasn't been bootstrapped already, do it if [ ! -f "$HEIMDALLD_HOME/config/config.toml" ]; then heimdalld --home $HEIMDALLD_HOME init + echo "Initialized new Heimdall configuration" fi # Replace init genesis with desired network genesis cp /genesis-config/genesis.json $HEIMDALLD_HOME/config/genesis.json + echo "Updated genesis configuration" cd $HEIMDALLD_HOME/config # Patch config.toml sed -iE "s#^cors_allowed_origins.*#cors_allowed_origins = ${CORS_ALLOWED_ORIGINS}#" config.toml sed -iE "s#^seeds.*#seeds = \"${SEEDS}\"#" config.toml + echo "Updated CORS and seeds configuration" if [ "${METRICS}" = "true" ]; then sed -iE "s#^prometheus \?=.*#prometheus = true#" config.toml sed -iE "s#^prometheus_listen_addr \?=.*#prometheus_listen_addr = \":${METRICS_PORT}\"#" config.toml + echo "Enabled metrics on port ${METRICS_PORT}" else sed -iE "s#^prometheus \?=.*#prometheus = false#" config.toml + echo "Disabled metrics" fi
100-101: Address TODO comment regarding image tagThe comment indicates that the image tag needs to be updated. Consider:
- Using a versioned tag instead of a SHA for better maintainability
- Adding the version to the Helm values for configurability
- ## TODO: update tag - image: ghcr.io/graphops/docker-builds/init-stream-download@sha256:acbe5bff3b273b6e56b7aad20e8368ab9ddb091250b744e41ad66d6e7c2d0901 + image: {{ $values.initContainer.snapshot.image.repository }}:{{ $values.initContainer.snapshot.image.tag }}
98-122: Consider adding readiness probe for snapshot initializationThe snapshot initialization is a critical step. Consider adding a readiness probe to ensure the snapshot is properly loaded before starting the main container.
- name: {{ $componentName }}-init-snapshot image: ghcr.io/graphops/docker-builds/init-stream-download@sha256:acbe5bff3b273b6e56b7aad20e8368ab9ddb091250b744e41ad66d6e7c2d0901 + readinessProbe: + exec: + command: + - /bin/sh + - -c + - | + if [ -f "/storage/from_snapshot" ] && [ -d "/storage/data" ]; then + exit 0 + else + exit 1 + fi + initialDelaySeconds: 10 + periodSeconds: 30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
charts/heimdall/Chart.yaml(1 hunks)charts/heimdall/README.md(2 hunks)charts/heimdall/templates/heimdall/statefulset.yaml(2 hunks)charts/heimdall/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- charts/heimdall/README.md
- charts/heimdall/Chart.yaml
- charts/heimdall/values.yaml
🔇 Additional comments (1)
charts/heimdall/templates/heimdall/statefulset.yaml (1)
109-115: Improve snapshot management reliability
The current snapshot management implementation could be enhanced with better error handling and resource management. A previous review has already suggested improvements including:
- Disk space verification
- Download progress monitoring
- Cleanup of temporary files
- SHA256 verification
- Retry mechanism
Please refer to the previous review comment for the detailed implementation suggestion.
Summary by CodeRabbit
New Features
Bug Fixes
Version Updates