Skip to content
Merged
Changes from all commits
Commits
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
48 changes: 36 additions & 12 deletions scripts/live-chaos/lib.sh
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,11 @@ require_mount_namespace() {
require_command mount "Install util-linux mount support."
local probe_dir="$WORKDIR/.vtop-mountns-probe"
mkdir -p "$probe_dir"
# The single quotes are deliberate: `$1` must be expanded by the INNER bash
# -c, which receives $probe_dir as its positional argument. Expanding it here
# would inline the path into the script text and break on any path needing
# quoting.
# shellcheck disable=SC2016
if ! unshare -rm bash -c \
'mount -t tmpfs -o size=1m tmpfs "$1" && touch "$1/probe"' bash "$probe_dir" \
> /dev/null 2>&1; then
Expand Down Expand Up @@ -296,6 +301,25 @@ require_binaries() {
# Config emission
# ---------------------------------------------------------------------------

# install_config <path> — read a config body from stdin and put it in place
# atomically.
#
# Config emitters are called per-invocation, not once: `meta_admin` re-emits
# its client config on every call. Scenario 01 runs a proposal loop in the
# background while the foreground drives membership changes, so two writers
# re-emit the same path concurrently — and a plain `> "$cfg"` redirect
# truncates in place, leaving a window where a reader parses a half-written
# file. That surfaced as `missing field 'endpoint'`, an error about the
# harness's own scratch file that looks like a product failure and points
# nowhere near the race. `mv` within a directory is atomic: a reader sees
# either the previous complete config or the new one, never a fragment.
install_config() {
local path="$1" tmp
tmp="$(mktemp "$path.XXXXXX")" || return 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Using mktemp here adds an unconditional dependency that the documented exact CHAOS_WORKDIR mode does not preflight. On a minimal supported host without mktemp, the run reaches config emission and fails only then; preflight mktemp for both modes or provide a supported fallback.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/live-chaos/lib.sh, line 318:

<comment>Using `mktemp` here adds an unconditional dependency that the documented exact `CHAOS_WORKDIR` mode does not preflight. On a minimal supported host without `mktemp`, the run reaches config emission and fails only then; preflight `mktemp` for both modes or provide a supported fallback.</comment>

<file context>
@@ -296,6 +301,25 @@ require_binaries() {
+# either the previous complete config or the new one, never a fragment.
+install_config() {
+  local path="$1" tmp
+  tmp="$(mktemp "$path.XXXXXX")" || return 1
+  cat > "$tmp"
+  mv -f "$tmp" "$path"
</file context>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: Every emitted config file now silently changes permissions from the shell's default umask (typically 0644) to 0600, because mktemp creates the temp file with mode 0600 and mv preserves that mode. Today that is harmless — the harness launches vtop-node as the same user — but it is an unannounced regression across all 12 emitters, and it will break any future consumer that reads these configs as a different user (e.g. a container mount, sudo-run node, or a shared workdir). Consider pinning the final mode so the change in behavior is explicit and stable.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/live-chaos/lib.sh, line 318:

<comment>Every emitted config file now silently changes permissions from the shell's default umask (typically 0644) to 0600, because `mktemp` creates the temp file with mode 0600 and `mv` preserves that mode. Today that is harmless — the harness launches vtop-node as the same user — but it is an unannounced regression across all 12 emitters, and it will break any future consumer that reads these configs as a different user (e.g. a container mount, sudo-run node, or a shared workdir). Consider pinning the final mode so the change in behavior is explicit and stable.</comment>

<file context>
@@ -296,6 +301,25 @@ require_binaries() {
+# either the previous complete config or the new one, never a fragment.
+install_config() {
+  local path="$1" tmp
+  tmp="$(mktemp "$path.XXXXXX")" || return 1
+  cat > "$tmp"
+  mv -f "$tmp" "$path"
</file context>

cat > "$tmp"
mv -f "$tmp" "$path"
Comment on lines +319 to +320

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: A failed config write can still replace the live config with a partial file because cat's status is ignored and mv always runs. On ENOSPC or another I/O error, readers get an atomically renamed but truncated YAML instead of the previous complete config; remove the temp file and return before renaming when cat fails.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/live-chaos/lib.sh, line 319:

<comment>A failed config write can still replace the live config with a partial file because `cat`'s status is ignored and `mv` always runs. On ENOSPC or another I/O error, readers get an atomically renamed but truncated YAML instead of the previous complete config; remove the temp file and return before renaming when `cat` fails.</comment>

<file context>
@@ -296,6 +301,25 @@ require_binaries() {
+install_config() {
+  local path="$1" tmp
+  tmp="$(mktemp "$path.XXXXXX")" || return 1
+  cat > "$tmp"
+  mv -f "$tmp" "$path"
+}
</file context>
Suggested change
cat > "$tmp"
mv -f "$tmp" "$path"
if ! cat > "$tmp"; then
rm -f "$tmp"
return 1
fi
mv -f "$tmp" "$path"

}

# emit_meta_config <node-id> <peer-ids...>
emit_meta_config() {
local id="$1"; shift
Expand Down Expand Up @@ -327,7 +351,7 @@ emit_meta_config() {
done
fi
echo "observability: { listen: \"$(meta_metrics_addr "$id")\" }"
} > "$cfg"
} | install_config "$cfg"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: An installation failure is hidden from callers because every emitter prints $cfg after the pipeline regardless of its status, so command-substitution callers can start nodes or CLIs with a missing or stale config. Return the pipeline failure before printing the path (for example, } | install_config "$cfg" || return 1) in each emitter.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/live-chaos/lib.sh, line 354:

<comment>An installation failure is hidden from callers because every emitter prints `$cfg` after the pipeline regardless of its status, so command-substitution callers can start nodes or CLIs with a missing or stale config. Return the pipeline failure before printing the path (for example, `} | install_config "$cfg" || return 1`) in each emitter.</comment>

<file context>
@@ -327,7 +351,7 @@ emit_meta_config() {
     fi
     echo "observability: { listen: \"$(meta_metrics_addr "$id")\" }"
-  } > "$cfg"
+  } | install_config "$cfg"
   echo "$cfg"
 }
</file context>

echo "$cfg"
}

Expand All @@ -353,7 +377,7 @@ emit_admin_config_as() {
echo "ca_cert: $CERTS/ca.pem"
echo "client_cert: $CERTS/$cert.pem"
echo "client_key: $CERTS/$cert-key.pem"
} > "$cfg"
} | install_config "$cfg"
echo "$cfg"
}

Expand Down Expand Up @@ -386,7 +410,7 @@ emit_leader_config() {
echo "native_tls: { ca: $CERTS/ca.pem, cert: $CERTS/data-1.pem, key: $CERTS/data-1-key.pem }"
echo "principal_id: $PRINCIPAL_ID"
echo "observability: { listen: \"$(data_metrics_addr 0)\" }"
} > "$cfg"
} | install_config "$cfg"
echo "$cfg"
}

Expand Down Expand Up @@ -443,7 +467,7 @@ emit_follower_config() {
echo "replica_listen: \"$(replica_addr "$n")\""
echo "replica_tls: { ca: $CERTS/ca.pem, cert: $CERTS/$cert.pem, key: $CERTS/$cert-key.pem }"
echo "observability: { listen: \"$(data_metrics_addr "$n")\" }"
} > "$cfg"
} | install_config "$cfg"
echo "$cfg"
}

Expand All @@ -460,7 +484,7 @@ emit_node_status_config() {
echo " - { node_uuid: $LEADER_UUID, addr: \"$(replica_addr 0)\", server_name: \"localhost\", role: leader }"
echo " - { node_uuid: $FOLLOWER1_UUID, addr: \"$(replica_addr 1)\", server_name: \"localhost\" }"
echo " - { node_uuid: $FOLLOWER2_UUID, addr: \"$(replica_addr 2)\", server_name: \"localhost\" }"
} > "$cfg"
} | install_config "$cfg"
echo "$cfg"
}

Expand Down Expand Up @@ -488,7 +512,7 @@ emit_client_config_at_epoch() {
emit_range_yaml
echo "server_name: \"localhost\""
echo "tls: { ca: $CERTS/ca.pem, cert: $CERTS/data-1.pem, key: $CERTS/data-1-key.pem }"
} > "$cfg"
} | install_config "$cfg"
echo "$cfg"
}

Expand All @@ -503,7 +527,7 @@ emit_client_config() {
emit_range_yaml
echo "server_name: \"localhost\""
echo "tls: { ca: $CERTS/ca.pem, cert: $CERTS/client.pem, key: $CERTS/client-key.pem }"
} > "$cfg"
} | install_config "$cfg"
echo "$cfg"
}

Expand All @@ -520,7 +544,7 @@ emit_replica_probe_config() {
emit_range_yaml
echo "server_name: \"localhost\""
echo "tls: { ca: $CERTS/ca.pem, cert: $CERTS/data-1.pem, key: $CERTS/data-1-key.pem }"
} > "$cfg"
} | install_config "$cfg"
echo "$cfg"
}

Expand Down Expand Up @@ -833,7 +857,7 @@ emit_leader_config_with_lease() {
{
sed 's/^fencing_epoch: .*/fencing_epoch: 0/' "$WORKDIR/data-leader-leader.yaml"
emit_lease_yaml "$id"
} > "$cfg"
} | install_config "$cfg"
echo "$cfg"
}

Expand Down Expand Up @@ -884,7 +908,7 @@ start_promoted_follower() {
# The promoted follower acquires the lease as ITSELF, so it presents its
# own certificate — not the original leader's and not the metadata node's.
emit_lease_yaml "$id" "$cert"
} > "$cfg"
} | install_config "$cfg"
pid="$(start_node "data-promoted-$n" "data_node_ready" data --config "$cfg")"
echo "$pid"
}
Expand Down Expand Up @@ -917,7 +941,7 @@ start_fenced_old_leader() {
-e "s|^observability: .*|observability: { listen: \"$(data_metrics_addr 3)\" }|" \
"$WORKDIR/data-leader-leader.yaml"
emit_lease_yaml "$id"
} > "$cfg"
} | install_config "$cfg"
pid="$(start_node "data-leader-restarted" "data_node_ready" data --config "$cfg")"
echo "$pid"
}
Expand Down Expand Up @@ -995,7 +1019,7 @@ emit_colocated_config() {
echo " native_tls: { ca: $CERTS/ca.pem, cert: $CERTS/$cert.pem, key: $CERTS/$cert-key.pem }"
echo " principal_id: $PRINCIPAL_ID"
echo "observability: { listen: \"$(data_metrics_addr $((id - 1)))\" }"
} > "$cfg"
} | install_config "$cfg"
echo "$cfg"
}

Expand Down