From 9c9bb9430cdf68178f993c578fa828882f08a796 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Sun, 2 Aug 2026 08:21:04 +1000 Subject: [PATCH] test: gate image shell scripts in CI and pin the spot vCPU quota ordering (#641) * test: gate scripts/images/ shell suites + shellcheck in CI The eks-node/rds-postgres *_test.sh suites and shellcheck only ever ran by hand: spinifex.yml filters pushes on Go-only paths and no job invoked them, so a broken helper could land unnoticed. Add a `make test-images` target and a path-filtered image-scripts.yml workflow so both run automatically on scripts/images/** and images/** changes, kept separate from preflight so image-asset churn doesn't run on every Go contributor's commit. Also clears the shellcheck findings that would have started this gate red (SC1090 on a runtime-resolved source path, SC2154 x2 on eval-assigned variables shellcheck can't trace). * test: pin vCPU quota gate ordering in RequestSpotInstances Nothing asserted that RequestSpotInstances runs the vCPU quota gate before dispatching capacity or launching a VM, so a regression that dropped or reordered EnforceLaunch would pass silently. Add an ordering test using a real quota.Service (embedded JetStream KV) and real NATS responders standing in for the capacity and launch daemons, recording a global call sequence and asserting the gate's index precedes both. Verified by removing the gate: the first quota-KV read then comes from the post-launch charge instead, landing after capacity dispatch and launch, and the ordering assertion fails as expected. --- .github/workflows/image-scripts.yml | 39 +++++ Makefile | 16 +- scripts/images/eks-node/eks-node-role.sh | 2 + scripts/images/eks-node/mulga-mgmt-net.sh | 4 + .../ec2/spotinstance/quota_gate_order_test.go | 141 ++++++++++++++++++ 5 files changed, 201 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/image-scripts.yml create mode 100644 spinifex/gateway/ec2/spotinstance/quota_gate_order_test.go diff --git a/.github/workflows/image-scripts.yml b/.github/workflows/image-scripts.yml new file mode 100644 index 000000000..878a5483d --- /dev/null +++ b/.github/workflows/image-scripts.yml @@ -0,0 +1,39 @@ +name: Image Scripts + +# Separate from spinifex.yml on purpose: scripts/images/ and images/ are shell +# and mkosi assets baked into system images, not Go, so they get their own +# path-filtered gate instead of running on every Go contributor's push (see +# ui.yml for the same pattern applied to the frontend). +on: + push: + branches: + - main + - dev + paths: + - scripts/images/** + - images/** + - .github/workflows/image-scripts.yml + pull_request: + branches: + - main + - dev + paths: + - scripts/images/** + - images/** + - .github/workflows/image-scripts.yml + workflow_dispatch: + +permissions: + contents: read + +jobs: + image_scripts: + name: Image Scripts + runs-on: ubuntu-26.04 + + steps: + - name: Checkout Code + uses: actions/checkout@v7 + + - name: Run image script tests + shellcheck + run: make test-images diff --git a/Makefile b/Makefile index 0b190d923..f5faef84a 100644 --- a/Makefile +++ b/Makefile @@ -190,6 +190,20 @@ test-actions: @echo -e "\n....Running action tests...." LOG_IGNORE=1 go test -timeout 60s ./.github/actions/... +# Shell suites + shellcheck for scripts/images/ helpers baked into system +# images. Kept out of `preflight` (a dedicated CI job gates it on +# scripts/images/** changes instead) so image-asset churn doesn't run on +# every Go contributor's commit. +test-images: + @echo -e "\n....Running scripts/images/**/*_test.sh...." + @for t in $$(find scripts/images -name '*_test.sh' | sort); do \ + echo "-- $$t"; \ + bash "$$t" || exit 1; \ + done + @echo -e "\n....Running shellcheck over scripts/images/**/*.sh...." + shellcheck -S warning $$(find scripts/images -name '*.sh' | sort) + @echo " test-images ok" + # Check that new/changed code meets coverage threshold (runs tests first) diff-coverage: test-cover @QUIET=$(QUIET) scripts/diff-coverage.sh $(COVERPROFILE) @@ -319,7 +333,7 @@ distro-arm64: distro-clean: rm -rf dist/ -.PHONY: build build-ui build-installer build-lb-agent build-ecs-agent build-system-image build-eks-node-image import-eks-node-image publish-eks-node-image build-ecs-node-image import-ecs-node-image build-rds-postgres-image import-rds-postgres-image build-microvm-image install-microvm go_build preflight test test-cover test-race diff-coverage bench test-actions test-harness test-integration manifest-check manifest-lint manifest-lint-update \ +.PHONY: build build-ui build-installer build-lb-agent build-ecs-agent build-system-image build-eks-node-image import-eks-node-image publish-eks-node-image build-ecs-node-image import-ecs-node-image build-rds-postgres-image import-rds-postgres-image build-microvm-image install-microvm go_build preflight test test-cover test-race diff-coverage bench test-actions test-images test-harness test-integration manifest-check manifest-lint manifest-lint-update \ deploy reinstall clean \ install-system install-go install-aws quickinstall \ lint fix govulncheck nilaway \ diff --git a/scripts/images/eks-node/eks-node-role.sh b/scripts/images/eks-node/eks-node-role.sh index 603aeec2b..e077dfe87 100755 --- a/scripts/images/eks-node/eks-node-role.sh +++ b/scripts/images/eks-node/eks-node-role.sh @@ -100,6 +100,8 @@ fi # (K3S_URL/K3S_TOKEN live there). ROLE="" if [ -f "${ENVFILE}" ]; then + # ENVFILE is a runtime-resolved, test-overridable path. + # shellcheck disable=SC1090 ROLE=$(. "${ENVFILE}"; printf '%s' "${SPINIFEX_K3S_ROLE:-}") fi if [ -z "${ROLE}" ] && [ -f "${AGENT_ENVFILE}" ]; then diff --git a/scripts/images/eks-node/mulga-mgmt-net.sh b/scripts/images/eks-node/mulga-mgmt-net.sh index 84a8786be..e715bf488 100644 --- a/scripts/images/eks-node/mulga-mgmt-net.sh +++ b/scripts/images/eks-node/mulga-mgmt-net.sh @@ -82,6 +82,8 @@ for n in 0 1 2 3 4 5; do eval "cidr=\${NIC${n}_CIDR:-}" ip link set "$iface" up + # dhcp is assigned above via eval; shellcheck can't trace dynamic names. + # shellcheck disable=SC2154 if [ "$dhcp" = "1" ]; then if dhcp_acquire "$iface"; then echo "[mulga-mgmt-net] data NIC $iface ($mac) up via DHCP" @@ -119,6 +121,8 @@ for n in 0 1 2 3 4 5; do # egress and (with a link-local /16) hijack IMDS. Enforce it — a DHCP client # racing this NIC may have added one before we set it static. eval "isdefault=\${NIC${n}_DEFAULT:-}" + # isdefault is assigned above via eval; shellcheck can't trace dynamic names. + # shellcheck disable=SC2154 if [ "$isdefault" != "1" ]; then ip route del default dev "$iface" 2>/dev/null || true fi diff --git a/spinifex/gateway/ec2/spotinstance/quota_gate_order_test.go b/spinifex/gateway/ec2/spotinstance/quota_gate_order_test.go new file mode 100644 index 000000000..7fdeb6257 --- /dev/null +++ b/spinifex/gateway/ec2/spotinstance/quota_gate_order_test.go @@ -0,0 +1,141 @@ +package gateway_ec2_spotinstance + +import ( + "context" + "encoding/json" + "fmt" + "sync" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + handlers_quota "github.com/mulgadc/spinifex/spinifex/handlers/quota" + "github.com/mulgadc/spinifex/spinifex/testutil" + "github.com/mulgadc/spinifex/spinifex/types" + "github.com/nats-io/nats.go" + "github.com/nats-io/nats.go/jetstream" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// callOrder records the arrival order of named events from concurrent NATS +// callbacks and the quota KV, giving a single global sequence to assert on. +type callOrder struct { + mu sync.Mutex + events []string +} + +func (o *callOrder) record(name string) { + o.mu.Lock() + defer o.mu.Unlock() + o.events = append(o.events, name) +} + +// indexOf returns the first position of name in the recorded sequence, or -1 +// if it never happened. +func (o *callOrder) indexOf(name string) int { + o.mu.Lock() + defer o.mu.Unlock() + for i, e := range o.events { + if e == name { + return i + } + } + return -1 +} + +// recordingKV wraps a real jetstream.KeyValue and records every Get as a +// "quota-check" event. EnforceLaunch's only externally observable action is +// CheckVCPU's read of the counter, so instrumenting Get alone pins the moment +// the quota gate actually ran, using the real gate rather than a stand-in. +type recordingKV struct { + jetstream.KeyValue + + order *callOrder +} + +func (r *recordingKV) Get(ctx context.Context, key string) (jetstream.KeyValueEntry, error) { + r.order.record("quota-check") + return r.KeyValue.Get(ctx, key) +} + +// TestRequestSpotInstances_QuotaGateRunsBeforeDispatchAndLaunch pins the +// ordering the vCPU quota gate depends on: EnforceLaunch must run before the +// capacity query (spinifex.node.status) and before the per-node VM launch +// (ec2.RunInstances..), not merely "at some point" during the +// request. All three legs run for real: a real quota.Service backed by an +// embedded JetStream KV, and real NATS responders standing in for a capacity +// daemon and a launch daemon. +func TestRequestSpotInstances_QuotaGateRunsBeforeDispatchAndLaunch(t *testing.T) { + _, nc, js := testutil.StartTestJetStream(t) + + kv, err := js.CreateKeyValue(t.Context(), jetstream.KeyValueConfig{ + Bucket: handlers_quota.KVBucketAccountUsage, + History: 1, + }) + require.NoError(t, err) + + order := &callOrder{} + quota := handlers_quota.New(handlers_quota.Limits{Enabled: true, VCPUs: 1000}, &recordingKV{KeyValue: kv, order: order}) + + const instanceType = "t3.micro" + const node = "node-1" + + // Stand-in capacity daemon: replies with one node that has room. + statusSub, err := nc.Subscribe("spinifex.node.status", func(msg *nats.Msg) { + order.record("capacity-dispatch") + resp := types.NodeStatusResponse{ + Node: node, + InstanceTypes: []types.InstanceTypeCap{{Name: instanceType, Available: 5}}, + } + data, _ := json.Marshal(resp) + _ = nc.Publish(msg.Reply, data) + }) + require.NoError(t, err) + defer statusSub.Unsubscribe() + + // Stand-in launch daemon on the node the capacity daemon advertised. + launchSub, err := nc.Subscribe(fmt.Sprintf("ec2.RunInstances.%s.%s", instanceType, node), func(msg *nats.Msg) { + order.record("vm-launch") + reservation := ec2.Reservation{ + ReservationId: aws.String("r-test"), + Instances: []*ec2.Instance{{InstanceId: aws.String("i-test"), InstanceType: aws.String(instanceType)}}, + } + data, _ := json.Marshal(reservation) + _ = msg.Respond(data) + }) + require.NoError(t, err) + defer launchSub.Unsubscribe() + + // Stand-in SIR-persist daemon so the request completes cleanly. + putSub, err := nc.Subscribe("ec2.PutSpotInstanceRequests", func(msg *nats.Msg) { + _ = msg.Respond([]byte("{}")) + }) + require.NoError(t, err) + defer putSub.Unsubscribe() + + time.Sleep(50 * time.Millisecond) // let subscriptions propagate + + input := &ec2.RequestSpotInstancesInput{ + LaunchSpecification: &ec2.RequestSpotLaunchSpecification{ + ImageId: aws.String("ami-0abcdef1234567890"), + InstanceType: aws.String(instanceType), + }, + InstanceCount: aws.Int64(1), + } + + _, err = RequestSpotInstances(context.Background(), input, nc, nil, "test-account", "ap-southeast-2a", nil, quota, 1) + require.NoError(t, err) + + quotaIdx := order.indexOf("quota-check") + capacityIdx := order.indexOf("capacity-dispatch") + launchIdx := order.indexOf("vm-launch") + + require.NotEqualf(t, -1, quotaIdx, "EnforceLaunch never touched the quota gate (events: %v)", order.events) + require.NotEqualf(t, -1, capacityIdx, "capacity dispatch never ran (events: %v)", order.events) + require.NotEqualf(t, -1, launchIdx, "VM launch never ran (events: %v)", order.events) + + assert.Less(t, quotaIdx, capacityIdx, "quota gate must run before capacity dispatch") + assert.Less(t, quotaIdx, launchIdx, "quota gate must run before VM launch") +}