feat(reconstruction-server): single command to scale#29
Conversation
There was a problem hiding this comment.
Pull request overview
This PR converts the reconstruction-server from a Deployment to a StatefulSet to enable stable pod identities and integrate a secret pool mechanism for per-pod credentials. The changes support scaling with unique secrets assigned to each pod based on its ordinal index.
Changes:
- Converted Kubernetes Deployment to StatefulSet with stable pod identities
- Added secret pool configuration to assign unique credentials to each pod based on ordinal
- Implemented validation to ensure secretPool.size >= replicaCount and other configuration requirements
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| charts/reconstruction-server/values.yaml | Added replicaCount and secretPool configuration options |
| charts/reconstruction-server/templates/statefulset.yaml | Converted Deployment to StatefulSet, added secret mounting and credential injection logic |
| charts/reconstruction-server/templates/_helpers.tpl | Added validation function to enforce secretPool configuration constraints |
| charts/reconstruction-server/Chart.yaml | Bumped chart and app versions to 0.3.0 |
Comments suppressed due to low confidence (1)
charts/reconstruction-server/templates/statefulset.yaml:1
- The abbreviation 'std' in the namePrefix is ambiguous. Consider using a more descriptive name like 'reconstruction-server-secret-' to clarify that this prefix is for secret resources.
{{- include "reconstruction-server.validate" . }}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - -ec | ||
| args: | ||
| - | | ||
| ordinal="${HOSTNAME##*-}" |
There was a problem hiding this comment.
The shell parameter expansion assumes HOSTNAME ends with a hyphen followed by digits. If the hostname format is unexpected, the ordinal extraction will fail silently. Consider adding validation or a more explicit error message explaining the expected hostname format (e.g., 'Expected hostname format: -').
| ordinal="${HOSTNAME##*-}" | |
| if [ -z "${HOSTNAME}" ]; then | |
| echo "Expected hostname format: <name>-<ordinal>, but HOSTNAME is empty" >&2 | |
| exit 1 | |
| fi | |
| case "${HOSTNAME}" in | |
| *-[0-9][0-9]*) | |
| ordinal="${HOSTNAME##*-}" | |
| ;; | |
| *) | |
| echo "Expected hostname format: <name>-<ordinal>, got '${HOSTNAME}'" >&2 | |
| exit 1 | |
| ;; | |
| esac |
| ordinal="${HOSTNAME##*-}" | ||
| secret_dir="{{ .Values.secretPool.mountPath }}/${ordinal}" | ||
| if [ -z "${ordinal}" ] || [ ! -d "${secret_dir}" ]; then | ||
| echo "missing secret directory for pod ordinal ${ordinal}" >&2 |
There was a problem hiding this comment.
When ordinal is empty due to hostname parsing failure, the error message 'missing secret directory for pod ordinal' is misleading. Consider updating to 'Failed to extract pod ordinal from hostname or secret directory not found' to clarify both potential failure modes.
| echo "missing secret directory for pod ordinal ${ordinal}" >&2 | |
| echo "Failed to extract pod ordinal from hostname or secret directory not found (ordinal: ${ordinal})" >&2 |
| export REG_SECRET="$(cat "${secret_dir}/REG_SECRET")" | ||
| export SECP256K1_PRIVHEX="$(cat "${secret_dir}/SECP256K1_PRIVHEX")" |
There was a problem hiding this comment.
If either secret file is missing or unreadable, the cat command will fail but the error may not clearly indicate which specific secret file caused the failure. Consider adding explicit checks for each file's existence before reading, or adding better error context (e.g., 'Failed to read REG_SECRET from ${secret_dir}/REG_SECRET').
| export REG_SECRET="$(cat "${secret_dir}/REG_SECRET")" | |
| export SECP256K1_PRIVHEX="$(cat "${secret_dir}/SECP256K1_PRIVHEX")" | |
| reg_secret_file="${secret_dir}/REG_SECRET" | |
| if [ ! -r "${reg_secret_file}" ]; then | |
| echo "Failed to read REG_SECRET from ${reg_secret_file}" >&2 | |
| exit 1 | |
| fi | |
| secp256k1_privhex_file="${secret_dir}/SECP256K1_PRIVHEX" | |
| if [ ! -r "${secp256k1_privhex_file}" ]; then | |
| echo "Failed to read SECP256K1_PRIVHEX from ${secp256k1_privhex_file}" >&2 | |
| exit 1 | |
| fi | |
| export REG_SECRET="$(cat "${reg_secret_file}")" | |
| export SECP256K1_PRIVHEX="$(cat "${secp256k1_privhex_file}")" |
| runAsNonRoot: true | ||
| runAsUser: 1000 | ||
|
|
||
| deploymentStrategy: |
There was a problem hiding this comment.
This seems no longer in used
This PR need the following one:
which drop the support for URL. As I understand from @robinlindh, we want to allow multiple "worker" operate under the same wallet |
Converted reconstruction-server to a StatefulSet with a secret pool + HPA