mirror of
https://github.com/mulgadc/spinifex.git
synced 2026-10-15 17:11:30 +00:00
Josh Siviour
c767f2b6ee
* feat: stamp the owning node's AZ on the instance record RunInstance never persists a node's AZ onto the stored instance, so today only the launching node's own ProjectInstance call can produce a correct AZ for a describe. Add VM.AZ, carry it on InstanceStatus since it is placement observed by the running node, and stamp it in WriteRunningSet alongside Status.LastNode from the node's own config.AZ. * docs: trim the AZ field comment to the project's three-line limit * fix: identify Gather responders by node, not by frame count utils.Gather previously counted reply frames toward completeness, so two subscriptions replying from one host (a daemon mid-restart) could satisfy a fan-out while a genuinely configured node stayed silent. Every daemon reply now carries an X-Node-ID header set by the shared response helpers via nats.Msg.RespondMsg, so Gather can attribute each frame to the node that sent it. Gather gains a CollectionMode (CollectServeData preserves today's early-exit behavior; CollectUntilDeadline collects to the full deadline for absence-proof callers) and returns node-identified Frames plus Responders/ SuccessResponders/ErrorResponders sets. Identity mode retains only the first payload seen per node; a disagreeing later frame from the same node flags a conflict but never overwrites the retained data. Node identity uses one namespace across the reply header, config.ClusterConfig.Nodes keys, daemon.Heartbeat.Node, and vm.VM.LastNode, all traced back to Daemon.node - confirmed to already agree, with a new test asserting it at runtime. This is a mechanical caller-side follow-on: every existing utils.Gather caller now reads frame.Data instead of frame directly. * feat: thread configured node set to DescribeInstances GatewayConfig gains NodeIDs, the keys of config.ClusterConfig.Nodes, distinct from the existing ExpectedNodes count. awsgw builds it from cluster config and wires it into GatewayConfig. The DescribeInstances action passes gw.NodeIDs through instead of relying on gw.DiscoverActiveNodes(ctx) at that call site, so completeness can be judged against the nodes that are actually configured rather than the nodes that happened to answer a discovery probe. * fix: judge DescribeInstances completeness by node identity DescribeInstancesChecked now judges completeness on decoded frames against the configured node set (ValidResponders) rather than on Gather's frame/byte counts, which cannot know the describe schema. A node that answers with a nil Reservations list is a valid responder, not an invalid one - the existing merge guard on Reservations != nil governs how frames combine, not whether the node counted as having answered. The explicit-instance-ID 404 path now collects with CollectUntilDeadline so a node that hasn't replied yet cannot be mistaken for a node that has nothing to report; general listing stays on CollectServeData. Any ambiguity - an unidentified frame, a node appearing in both the success and error sets, a node replying twice with disagreeing frames, a Gather cap hit, or an empty/unset node set - suppresses the NotFound rather than risking a false one. * fix: let a describe by instance id stop once every id is found CollectUntilDeadline made every DescribeInstances with explicit ids wait the full 3s fan-out deadline, measured at 4650ms against 1655ms before, on a healthy cluster where all three nodes answer in about a millisecond. Gather gains a Settled predicate, consulted only under CollectUntilDeadline, that ends collection once the replies already answer the question. Absence still pays the whole deadline, and an instance the caller cannot see is filtered out of every frame so it takes the absence path too, which keeps the two NotFound answers indistinguishable in time. * refactor: resolve IMDS instance metadata local-first, drop the fan-out IMDS only ever serves a guest on its own node, so a DescribeInstances fan-out to the whole cluster is unnecessary latency and load. Resolve from the node's own on-disk VM state first, falling back to a direct record-space read for an instance that has moved off this node. Neither path issues a NATS request. * perf: cache the local instance state parse for IMDS lookups IMDS request rate is guest-driven and unbounded, so re-reading and re-parsing the whole on-disk VM state on every request does not scale with VM count on a node. Cache the parsed state and only re-read when the file's mtime or size changes; a stat or parse failure clears the cache and falls back to the record space rather than serving a stale hit. * fix: share ManagedBy visibility rule between DescribeInstances and IMDS Extracts IsInstanceVisibleToCaller so account ownership and the platform-managed hide rule live in one place instead of two independently maintained copies. * refactor: decouple IMDS instance lookup from the daemon package handlers/imds no longer imports daemon: NewIMDSServiceImpl now takes a localStateReader and a recordLoader interface instead of a dataDir string, and vpcd (the composition root for both planes) constructs both adapters, including the stat-guarded local-state cache that used to live inline in handlers/imds. Either adapter may be nil, degrading that source to a miss rather than failing IMDS to start. * test: move the local state reader test to an external package * chore: exempt export_test.go from the external test package check The check's own remedy text tells you to move unexported hooks into an export_test.go, then flags that file for being in-package — which it is by definition, since exporting internals is its whole purpose. * fix: bound how many describes may hold a full fan-out deadline A describe naming an instance the caller cannot see is filtered out of every frame, so it never settles early and holds a gateway goroutine and inbox subscription for the whole 3s deadline. Issuing those is free for the caller, so nothing stopped a burst of them pinning unbounded gateway state. Absence proofs now draw on a fixed budget. Past it, callers get a retryable RequestLimitExceeded rather than being queued, since queueing would let the backlog grow. The decision is taken before any reply is read, so a nonexistent id and another account's id throttle identically and the throttle cannot become the timing oracle the deadline exists to close. * perf: settle describe-by-id on responder coverage, not the full deadline Every describe-by-id was paying the whole 3s fan-out deadline whenever the requested instance was not in a running-instance frame — which includes a stopped instance, a terminated one, an id belonging to another account, and an id that does not exist. Gather gains ResponderGrace: once every ExpectedResponders node has answered, collection closes after a short grace window instead of running to the deadline. Core NATS gives a request only to the subscribers present when it was published, so an overlapping subscription replies alongside the first and one that arrives later never receives the request at all. Waiting past full coverage cannot find a reply that full coverage missed. The window is armed once and never extended, so a node replying repeatedly cannot hold the fan-out open. The full deadline is now paid only when a node stays silent, which is the case it was always meant to cover. The stopped and terminated bucket queries now run alongside the fan-out rather than after it, so their latency overlaps it instead of adding to it. They feed a shared instanceSettler that ends collection as soon as every requested id has been seen, skipping even the grace window. The timing oracle is unchanged. An instance the caller cannot see is filtered out of every source before the reply is sent, so it takes the same path as an id that does not exist: both answer NotFound after full responder coverage, and remain indistinguishable in time. * fix: stop parallel tests racing the terminate backoff seam terminateRetrySleep is a package global, and every test that needed it neutered swapped it and restored the old value from t.Cleanup. Those tests run in parallel, so two cleanups could write the seam at once — a real data race the detector caught once enough parallel tests shared the binary. Installed once in TestMain instead. Nothing in this package wants the real backoff, so there is no value in restoring it per test.
88 lines
2.9 KiB
Bash
Executable File
88 lines
2.9 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
set -euo pipefail
|
|
|
|
# Requires newly added test files to use an external test package.
|
|
#
|
|
# `package foo_test` binds tests to the exported API, so internals can be
|
|
# refactored without rewriting tests, and it lets packages share test doubles
|
|
# (an in-package double is invisible to every other package). Coverage is
|
|
# unaffected — Go compiles internal and external test packages into a single
|
|
# binary per package, so attribution is identical either way.
|
|
#
|
|
# Only ADDED files are checked; existing in-package tests are left alone.
|
|
# A file that genuinely needs unexported access can either opt out with a
|
|
# `//go:build` free marker comment (see ALLOW_MARKER) or move the unexported
|
|
# hook into an export_test.go.
|
|
#
|
|
# Usage: scripts/check-test-package.sh [--base <ref>]
|
|
#
|
|
# Base ref auto-detection matches diff-coverage.sh:
|
|
# main branch → HEAD~1
|
|
# dev branch → origin/main
|
|
# other branch → origin/dev
|
|
|
|
BASE_REF=""
|
|
QUIET="${QUIET:-}"
|
|
ALLOW_MARKER="//test:in-package"
|
|
|
|
while [[ $# -gt 0 ]]; do
|
|
case "$1" in
|
|
--base) BASE_REF="$2"; shift 2 ;;
|
|
-*) echo "Unknown option: $1" >&2; exit 1 ;;
|
|
*) echo "Unexpected argument: $1" >&2; exit 1 ;;
|
|
esac
|
|
done
|
|
|
|
if [[ -z "$BASE_REF" ]]; then
|
|
BRANCH="${GITHUB_REF_NAME:-$(git rev-parse --abbrev-ref HEAD)}"
|
|
case "$BRANCH" in
|
|
main) BASE_REF="HEAD~1" ;;
|
|
dev) BASE_REF="origin/main" ;;
|
|
*) BASE_REF="origin/dev" ;;
|
|
esac
|
|
[[ -z "$QUIET" ]] && echo "Base ref: $BASE_REF (branch: $BRANCH)"
|
|
fi
|
|
|
|
if ! git rev-parse --verify "$BASE_REF" &>/dev/null; then
|
|
echo "Error: base ref '$BASE_REF' not reachable." >&2
|
|
echo "Fetch it first: git fetch origin <branch>" >&2
|
|
exit 1
|
|
fi
|
|
|
|
# e2e and integration suites share a package-level fixture built in
|
|
# main_test.go, so their files cannot move individually. export_test.go is the
|
|
# remedy this check recommends, and it is in-package by definition.
|
|
ADDED=$(git diff "$BASE_REF" HEAD --name-only --diff-filter=A -- '*_test.go' \
|
|
':!tests/e2e/*' ':!tests/integration/*' ':!*export_test.go' || true)
|
|
|
|
if [[ -z "$ADDED" ]]; then
|
|
[[ -z "$QUIET" ]] && echo "No new test files — skipping test package check."
|
|
exit 0
|
|
fi
|
|
|
|
VIOLATIONS=()
|
|
for f in $ADDED; do
|
|
[[ -f "$f" ]] || continue
|
|
if grep -qF "$ALLOW_MARKER" "$f"; then
|
|
continue
|
|
fi
|
|
pkg=$(awk '/^package /{print $2; exit}' "$f")
|
|
if [[ -n "$pkg" && "$pkg" != *_test ]]; then
|
|
VIOLATIONS+=("$f (package $pkg)")
|
|
fi
|
|
done
|
|
|
|
if [[ ${#VIOLATIONS[@]} -gt 0 ]]; then
|
|
echo "ERROR: new test files must use an external test package (package <pkg>_test):" >&2
|
|
for v in "${VIOLATIONS[@]}"; do
|
|
echo " $v" >&2
|
|
done
|
|
echo "" >&2
|
|
echo "Move unexported hooks into export_test.go, or add the comment" >&2
|
|
echo "'$ALLOW_MARKER' to the file with a reason if in-package access is required." >&2
|
|
exit 1
|
|
fi
|
|
|
|
[[ -z "$QUIET" ]] && echo "Test package check OK ($(echo "$ADDED" | wc -w) new file(s))"
|
|
exit 0
|