Skip to content

ops(rolling-update): add GOMEMLIMIT=1800MiB + --memory=2500m defaults #617

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bootjp merged 5 commits into from
Apr 24, 2026
+111 −2
Merged

ops(rolling-update): add GOMEMLIMIT=1800MiB + --memory=2500m defaults #617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5f7cad9
ops(rolling-update): add GOMEMLIMIT=1800MiB + --memory=2500m defaults
bootjp Apr 24, 2026
a3f0cf0
fix(rolling-update): merge_extra_env without bash 4 associative arrays
bootjp Apr 24, 2026
edd2a91
fix(rolling-update): guard read -a against empty here-strings under s…
bootjp Apr 24, 2026
941ed32
fix(rolling-update): explicit IFS + fail loud on malformed DEFAULT_EX…
bootjp Apr 24, 2026
75db3bf
fix(rolling-update): reject empty-key DEFAULT_EXTRA_ENV entries
bootjp Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions scripts/rolling-update.env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,10 @@ SSH_STRICT_HOST_KEY_CHECKING="accept-new"
RAFTADMIN_REMOTE_BIN="/tmp/elastickv-raftadmin"
RAFTADMIN_RPC_TIMEOUT_SECONDS="5"
RAFTADMIN_ALLOW_INSECURE="true"

# OOM defenses applied on 2026-04-24 after kernel OOM-SIGKILL cascades.
# GOMEMLIMIT makes Go GC before the container hits --memory; --memory keeps
# any kill scoped to the container, not host processes. Set either to "" to
# opt out. User EXTRA_ENV keys override matching keys in DEFAULT_EXTRA_ENV.
DEFAULT_EXTRA_ENV="GOMEMLIMIT=1800MiB"
CONTAINER_MEMORY_LIMIT="2500m"
106 changes: 104 additions & 2 deletions scripts/rolling-update.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,18 @@ Optional environment:
Each pair must be KEY=VALUE with a non-empty KEY; pairs themselves must not
contain whitespace.

DEFAULT_EXTRA_ENV defaults to "GOMEMLIMIT=1800MiB" (Go runtime soft memory
ceiling; GC works harder before approaching the hard --memory limit so the
kernel OOM killer is not triggered). Merged with EXTRA_ENV before forwarding;
if a user-supplied EXTRA_ENV entry sets the same KEY, the user value wins.
Set DEFAULT_EXTRA_ENV="" to disable the default.

CONTAINER_MEMORY_LIMIT
docker run --memory value (default: 2500m). Hard container-scoped memory
ceiling; any OOM kill is contained to the elastickv container rather than
cascading to host processes (e.g. qemu-guest-agent, systemd). Paired with
GOMEMLIMIT=1800MiB so Go GC preempts the kill. Set to "" to disable.

Notes:
- If RAFT_TO_REDIS_MAP is unset, it is derived automatically from NODES,
RAFT_PORT, and REDIS_PORT.
Expand Down Expand Up @@ -113,6 +125,9 @@ SSH_TARGETS="${SSH_TARGETS:-}"
ROLLING_ORDER="${ROLLING_ORDER:-}"
RAFT_TO_REDIS_MAP="${RAFT_TO_REDIS_MAP:-}"
RAFT_TO_S3_MAP="${RAFT_TO_S3_MAP:-}"
# Container OOM defenses. See usage() for rationale. Empty string disables.
DEFAULT_EXTRA_ENV="${DEFAULT_EXTRA_ENV-GOMEMLIMIT=1800MiB}"
CONTAINER_MEMORY_LIMIT="${CONTAINER_MEMORY_LIMIT-2500m}"

if [[ -z "$NODES" ]]; then
echo "NODES is required" >&2
Expand Down Expand Up @@ -427,6 +442,7 @@ update_one_node() {
RAFT_TO_REDIS_MAP="$RAFT_TO_REDIS_MAP_Q" \
RAFT_TO_S3_MAP="$RAFT_TO_S3_MAP_Q" \
EXTRA_ENV="$EXTRA_ENV_Q" \
CONTAINER_MEMORY_LIMIT="$CONTAINER_MEMORY_LIMIT_Q" \
bash -s <<'REMOTE'
set -euo pipefail

Expand Down Expand Up @@ -707,10 +723,20 @@ run_container() {
done
fi

# Optional hard container-scoped memory limit. Keeps any OOM kill contained
# to the elastickv container rather than cascading to host processes
# (e.g. qemu-guest-agent, systemd). Pair with GOMEMLIMIT via EXTRA_ENV so
# the Go runtime GCs before the kernel kills the container.
local memory_flags=()
if [[ -n "${CONTAINER_MEMORY_LIMIT:-}" ]]; then
memory_flags=(--memory "$CONTAINER_MEMORY_LIMIT")
fi

docker run -d \
--name "$CONTAINER_NAME" \
--restart unless-stopped \
--network host \
"${memory_flags[@]}" \
-v "$DATA_DIR:$DATA_DIR" \
"${s3_creds_volume[@]}" \
"${extra_env_flags[@]}" \
Expand Down Expand Up @@ -868,9 +894,85 @@ ensure_remote_raftadmin_binaries
# CR handling additionally covers deploy.env files edited on Windows.
# `${EXTRA_ENV:-}` is required because `set -u` is active and EXTRA_ENV
# may be unset (the variable is optional in deploy.env).
EXTRA_ENV_NORMALISED="${EXTRA_ENV:-}"
EXTRA_ENV_NORMALISED="${EXTRA_ENV_NORMALISED//[$'\t\r\n']/ }"
# Merge DEFAULT_EXTRA_ENV (operator-safety defaults like GOMEMLIMIT) with any
# user-supplied EXTRA_ENV. User-supplied KEYs win over defaults for the same
# KEY; the remote parser forwards pairs via `-e KEY=VALUE` so docker evaluates
# the last occurrence, which means pre-pending defaults is correct: later user
# entries override earlier defaults. We still de-duplicate here so the printed
# command line stays clean.
EXTRA_ENV_USER_NORMALISED="${EXTRA_ENV:-}"
EXTRA_ENV_USER_NORMALISED="${EXTRA_ENV_USER_NORMALISED//[$'\t\r\n']/ }"
EXTRA_ENV_DEFAULT_NORMALISED="${DEFAULT_EXTRA_ENV:-}"
EXTRA_ENV_DEFAULT_NORMALISED="${EXTRA_ENV_DEFAULT_NORMALISED//[$'\t\r\n']/ }"

merge_extra_env() {
local defaults="$1"
local user="$2"
# Portable across Bash 3.2 (macOS default) which lacks associative
# arrays: concatenate user KEYs into a space-padded string and match
# with " KEY " to test set membership. The EXTRA_ENV list is typically
# a handful of entries, so the linear check is negligible.
local -a user_pairs=()
local -a default_pairs=()
local pair key seen=" " merged=""

# Guard the here-strings: on Bash 3.2 (macOS default) `read` on an
# empty here-string returns non-zero, which trips `set -e`. Skip the
# read when the source string is empty — the empty array is the
# intended result either way.
# IFS is explicitly set per-read so a caller's surrounding IFS
# doesn't change how DEFAULT_EXTRA_ENV / EXTRA_ENV are split.
if [[ -n "$user" ]]; then
IFS=$' \t\n' read -r -a user_pairs <<< "$user"
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In Bash, the read command returns a non-zero exit code if it reaches EOF without reading any fields. When set -e is active (as it is here), this will cause the script to exit prematurely if EXTRA_ENV (or user) contains only whitespace (e.g., a trailing space or newline in the environment file). Since [[ -n "$user" ]] is true for a string containing only spaces, the read command will be executed and fail.

Appending || true (or || :) ensures the script continues with an empty array, which is the desired behavior for whitespace-only input.

Suggested change
IFS=$' \t\n' read -r -a user_pairs <<< "$user"
IFS=$' \t\n' read -r -a user_pairs <<< "$user" || true

fi
for pair in "${user_pairs[@]}"; do
[[ -n "$pair" ]] || continue
[[ "$pair" == *=* ]] || continue
key="${pair%%=*}"
seen+="${key} "
done

if [[ -n "$defaults" ]]; then
IFS=$' \t\n' read -r -a default_pairs <<< "$defaults"
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the issue with user_pairs, this read command will trigger an exit under set -e if DEFAULT_EXTRA_ENV is set to a non-empty string that contains only whitespace. While less likely for a default value, it's safer to handle the potential non-zero exit code.

Suggested change
IFS=$' \t\n' read -r -a default_pairs <<< "$defaults"
IFS=$' \t\n' read -r -a default_pairs <<< "$defaults" || true

# Unlike EXTRA_ENV (user-supplied, forgivable typos), DEFAULT_EXTRA_ENV
# is baked into deploy.env — a malformed token there means a
# safeguard we installed deliberately is silently ignored. Fail
# loudly instead of dropping it.
# Three failure modes to catch early:
# - no `=` at all (e.g. GOMEMLIMIT) -> malformed
# - empty key before `=` (e.g. =1800MiB) -> malformed
# (the `*=*` pattern match alone accepts this)
# - empty pair (covered by the continue above)
for pair in "${default_pairs[@]}"; do
[[ -n "$pair" ]] || continue
if [[ "$pair" != *=* ]]; then
echo "rolling-update: malformed DEFAULT_EXTRA_ENV entry '$pair' (expected KEY=VALUE)" >&2
Comment on lines +948 to +949
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject empty-key DEFAULT_EXTRA_ENV entries early

The new DEFAULT_EXTRA_ENV validation only checks whether a token contains =, so an entry like =1800MiB is treated as valid here even though it is not a KEY=VALUE pair. That malformed default is then rejected later by run_container’s key regex check, but at that point the update has already stopped the node’s container, turning a config typo into rollout disruption. Validate a non-empty/valid key in this early guard so bad defaults fail before any node is touched.

Useful? React with 👍 / 👎.

return 1
fi
if [[ "${pair%%=*}" == "" ]]; then
echo "rolling-update: malformed DEFAULT_EXTRA_ENV entry '$pair' (empty key)" >&2
return 1
fi
done
fi
for pair in "${default_pairs[@]}"; do
[[ -n "$pair" ]] || continue
[[ "$pair" == *=* ]] || continue
Comment on lines +959 to +960
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate malformed DEFAULT_EXTRA_ENV entries

merge_extra_env silently drops default tokens that don’t contain = and only forwards the remaining pairs for downstream validation, so a typo in DEFAULT_EXTRA_ENV (for example GOMEMLIMIT1800MiB) makes the rollout continue without the intended safeguard and without any error. This differs from EXTRA_ENV, where malformed entries fail fast, and can hide configuration mistakes in production rollouts.

Useful? React with 👍 / 👎.

key="${pair%%=*}"
if [[ "$seen" != *" ${key} "* ]]; then
merged+="${merged:+ }$pair"
fi
done
for pair in "${user_pairs[@]}"; do
[[ -n "$pair" ]] || continue
merged+="${merged:+ }$pair"
done
printf '%s' "$merged"
}

EXTRA_ENV_NORMALISED="$(merge_extra_env "$EXTRA_ENV_DEFAULT_NORMALISED" "$EXTRA_ENV_USER_NORMALISED")"
EXTRA_ENV_Q="$(printf '%q' "$EXTRA_ENV_NORMALISED")"
CONTAINER_MEMORY_LIMIT_Q="$(printf '%q' "${CONTAINER_MEMORY_LIMIT:-}")"
S3_CREDENTIALS_FILE_Q="$(printf '%q' "${S3_CREDENTIALS_FILE:-}")"
IMAGE_Q="$(printf '%q' "$IMAGE")"
DATA_DIR_Q="$(printf '%q' "$DATA_DIR")"
Expand Down
Loading