From d53dbce6c243c39ad07a690bd4b84b7df80132d1 Mon Sep 17 00:00:00 2001 From: scttfrdmn <3011922+scttfrdmn@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:52:07 -0700 Subject: [PATCH] fix: a sweep honours --ttl/--idle-timeout/--cost-limit instead of logging them (#525) buildLaunchConfigFromParams "start[s] with an empty config" and the sweep dispatch copied only Region/InstanceType/Name off the base config, so no CLI spend control reached a single row. The --no-detach branch then printed "Using safeguards: ttl=4h, idle-timeout=" from the very variables it was about to throw away, which is worse than dropping them quietly: it affirmatively tells the operator a cap is active at the instant it is discarded. cost_limit: in a param file did not work either -- there was no parser case, so it fell through to the unknown-key arm and became a PARAM_cost_limit env var that capped nothing. With both routes broken a sweep had NO per-instance dollar cap at all, and its only bound was the zombie guard's unrelated 1h *idle* timeout: never fires on a compute-bound row, yet still shows up in `spawn list` and reads as bounded. The flags are now applied as sweep defaults:, which is the one place that reaches both orchestration paths -- the foreground path merges defaults into every row, and the detached path uploads them to S3 for the Lambda orchestrator, which does the same. (That orchestrator is not in this repo, so its half is not verifiable here; the seam is the shared defaults map.) cost_limit: is now a real parser case that ERRORS on a value it cannot read instead of leaving the cap at 0, since a silently zeroed cost limit means "no cap" -- the opposite of what was written. Precedence is most-specific-wins: a row's own params: > the CLI flag > the file's defaults:. The row level keeps a GPU row on a shorter leash than the rest of the sweep; the CLI level makes `--ttl 30m` on a file that says `ttl: 8h` do what it looks like it does. Two knock-on fixes in the same path: * --estimate-only now prices the TTL you passed. The estimator reads the per-row ttl and defaults to 1h, so `--ttl 4h` on a file with no ttl: was previously quoted at a quarter of its worst case. * the --no-detach guard checks the merged PER-ROW bound instead of the CLI variable. A ttl: in the param file used to be refused by that guard even though it was the value that actually reached the instances, so the only way through was to pass --ttl to satisfy the check and have it discarded -- one flag to pass the guard, another to take effect, with nothing saying so. The per-row form also catches a file that bounds only SOME of its rows (invisible to the old check, since one --ttl satisfied it and then reached nothing) and names the offending rows, because "some row is unbounded" is not actionable on a 30-row sweep. Tests assert on the EC2 tags spored and the reaper actually enforce, not on any in-process value: a test that checked "the flag was parsed" would have passed for the whole life of the bug, because the flag WAS parsed -- onto a config nobody copied from. --- CHANGELOG.md | 31 ++ cmd/launch_sweep.go | 124 +++++++- cmd/sweep.go | 56 ++++ cmd/sweep_spend_controls_test.go | 255 +++++++++++++++++ test/e2e/tier0_sweep_spend_controls_test.go | 298 ++++++++++++++++++++ 5 files changed, 760 insertions(+), 4 deletions(-) create mode 100644 cmd/sweep_spend_controls_test.go create mode 100644 test/e2e/tier0_sweep_spend_controls_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f1b173f..6017ef8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,37 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed +- **A parameter sweep silently discarded `--ttl`, `--idle-timeout` and + `--cost-limit`, then logged `Using safeguards: ttl=...`** (#525). + `buildLaunchConfigFromParams` starts from an empty config and the sweep + dispatch copied only `Region`/`InstanceType`/`Name` off the base config, so no + CLI spend control reached a single row — while the `--no-detach` branch printed + a safeguards line from the very variables it was about to throw away, which is + worse than dropping them quietly: it affirmatively tells the operator a cap is + active at the instant it is discarded. `cost_limit:` in a param file did not + work either — there was no parser case, so it fell through to the unknown-key + arm and became a `PARAM_cost_limit` env var that capped nothing. With both + routes broken a sweep had no per-instance dollar cap at all, and its only bound + was the zombie guard's unrelated 1h *idle* timeout, which never fires on a + compute-bound row yet still shows up in `spawn list` and reads as bounded. + + The flags are now applied as sweep `defaults:` — the one place that reaches both + orchestration paths, since the foreground path merges defaults into every row + and the detached path uploads them to S3 for the Lambda orchestrator — and + `cost_limit:` is a real parser case that *errors* on a value it cannot read + rather than leaving the cap at 0. Precedence is most-specific-wins: a row's own + `params:` > the CLI flag > the file's `defaults:`. + + Two knock-on fixes in the same path: `--estimate-only` now prices the TTL you + passed (previously `--ttl 4h` on a file with no `ttl:` was quoted at the + estimator's 1h default, understating the worst case fourfold), and the + `--no-detach` guard checks the merged **per-row** bound instead of the CLI + variable. A `ttl:` in the param file used to be refused by that guard even + though it was the value which actually reached the instances, so the only way + through was to pass `--ttl` to satisfy the check and have it discarded — one + flag to pass the guard, another to take effect. The per-row form also catches a + file that bounds only *some* of its rows, which a check on the CLI variable + could not see, and names the offending rows. - **`--estimate-only` launched every row of a parameter sweep instead of estimating it** (#524). The flag was checked only inside `launchSweepDetached`, so any sweep that took the *foreground* path diff --git a/cmd/launch_sweep.go b/cmd/launch_sweep.go index cfc6a84..23f5b5a 100644 --- a/cmd/launch_sweep.go +++ b/cmd/launch_sweep.go @@ -46,6 +46,26 @@ func launchParameterSweep(ctx context.Context, baseConfig *aws.LaunchConfig, pla return fmt.Errorf("either --param-file or --params must be specified for parameter sweep") } + // The CLI spend-control flags used to be dropped on the floor for a sweep + // (#525). buildLaunchConfigFromParams "start[s] with an empty config" and the + // dispatch below copies only Region/InstanceType/Name off baseConfig, so + // --ttl, --idle-timeout and --cost-limit never reached a single row — while + // the --no-detach branch below printed "Using safeguards: ttl=..." from the + // very variables it was about to discard, which is worse than dropping them + // quietly: it affirmatively tells the operator a safeguard is in place. + // + // They are applied here as sweep *defaults*, which is the one place that + // reaches BOTH orchestration paths: the foreground path merges Defaults into + // every row (buildLaunchConfigFromParams), and the detached path uploads + // Defaults to S3 for the Lambda orchestrator, which does the same. + // + // This runs before everything that consumes them: the --no-detach safeguard + // log, the --estimate-only preview (whose per-row duration comes from + // defaults.ttl, so before this fix `--ttl 4h --estimate-only` on a file with + // no ttl: quoted a 1h estimate), and applyIdleTimeoutDefault, which now sees + // an explicit --ttl and stops substituting an unrelated 1h *idle* timeout. + appliedControls := applyCLISpendControlsToSweep(paramFormat) + // AUTO-ENABLE DETACHED MODE for parameter sweeps to prevent zombie instances // If the CLI disconnects (laptop sleep/shutdown), detached mode ensures: // - Sweep state persists in DynamoDB @@ -76,11 +96,26 @@ func launchParameterSweep(ctx context.Context, baseConfig *aws.LaunchConfig, pla // User explicitly disabled detached mode - warn about zombie instances fmt.Fprintf(os.Stderr, "\n⚠️ WARNING: --no-detach specified\n") fmt.Fprintf(os.Stderr, " If CLI disconnects (laptop sleep/shutdown), instances may become zombies.\n") - if ttl == "" && idleTimeout == "" { - fmt.Fprintf(os.Stderr, "\n❌ ERROR: --no-detach requires --ttl or --idle-timeout to prevent zombie instances\n") - return fmt.Errorf("--no-detach requires --ttl or --idle-timeout for safety") + // The bound is checked per ROW, against the merged (defaults + row) values + // the launch will actually use, rather than against the CLI variables + // alone. Two reasons, both from #525: + // + // * a `ttl:` in the param file is a real bound — it is the one that used + // to reach the instances — but this check refused it, so the only way + // through was to pass --ttl purely to satisfy the check and then have + // its value discarded. One flag to pass the guard, another to take + // effect, with nothing saying so. + // * a per-row check also catches a file that bounds only SOME of its + // rows, which a check on the CLI variables cannot see at all. + if unbounded := sweepRowsWithoutBound(paramFormat); len(unbounded) > 0 { + fmt.Fprintf(os.Stderr, "\n❌ ERROR: --no-detach requires a TTL or idle timeout on EVERY row to prevent zombie instances\n") + fmt.Fprintf(os.Stderr, " Unbounded: %s\n", strings.Join(unbounded, ", ")) + fmt.Fprintf(os.Stderr, " Fix with --ttl/--idle-timeout for the whole sweep, or ttl:/idle_timeout: under defaults: or on those rows.\n") + return fmt.Errorf("--no-detach requires a TTL or idle timeout on every row for safety (%d unbounded)", len(unbounded)) } - fmt.Fprintf(os.Stderr, " Using safeguards: ttl=%s, idle-timeout=%s\n\n", ttl, idleTimeout) + defTTL, defIdle := sweepRowBound(paramFormat.Defaults, nil) + fmt.Fprintf(os.Stderr, " Using safeguards: ttl=%s, idle-timeout=%s\n", defTTL, defIdle) + fmt.Fprintf(os.Stderr, " (sweep defaults, CLI flags already folded in; a row's own value wins — #525)\n\n") } // Generate sweep ID @@ -138,6 +173,10 @@ func launchParameterSweep(ctx context.Context, baseConfig *aws.LaunchConfig, pla if detach { fmt.Fprintf(os.Stderr, " Orchestration: Lambda (detached)\n") } + if len(appliedControls) > 0 { + fmt.Fprintf(os.Stderr, " From the command line: %s (each row's own value wins)\n", + strings.Join(appliedControls, ", ")) + } fmt.Fprintf(os.Stderr, "\n") // --estimate-only is handled HERE, above the detached/foreground dispatch @@ -619,6 +658,83 @@ func launchWithRollingQueue(ctx context.Context, awsClient *aws.Client, launchCo return launchedInstances, failures, successCount, nil } +// sweepRowBound returns the TTL and idle timeout that will actually apply to one +// row: the row's own value if it has one, otherwise the sweep default. Pass a nil +// row to read the defaults themselves. +// +// A non-string value (`ttl: 3600` rather than `ttl: 1h`) reads as absent, which +// matches what buildLaunchConfigFromParams does with it — the type assertion +// there fails and the field stays empty. Agreeing with the parser is the point: +// it means the --no-detach guard refuses that file loudly instead of letting it +// launch with a bound the parser quietly dropped. +func sweepRowBound(defaults, row map[string]interface{}) (ttl, idle string) { + pick := func(key string) string { + for _, m := range []map[string]interface{}{row, defaults} { + if v, ok := m[key]; ok { + if s, ok := v.(string); ok && s != "" { + return s + } + } + } + return "" + } + return pick("ttl"), pick("idle_timeout") +} + +// sweepRowsWithoutBound labels every row that would launch with neither a TTL nor +// an idle timeout, for the --no-detach guard's error message. Naming the rows +// matters: on a 30-row file "some row is unbounded" is not actionable. +func sweepRowsWithoutBound(paramFormat *ParamFileFormat) []string { + var out []string + for i, row := range paramFormat.Params { + if ttl, idle := sweepRowBound(paramFormat.Defaults, row); ttl == "" && idle == "" { + label := fmt.Sprintf("row %d", i) + if it, ok := row["instance_type"].(string); ok && it != "" { + label += " (" + it + ")" + } + out = append(out, label) + } + } + return out +} + +// applyCLISpendControlsToSweep writes the CLI spend-control flags into a sweep's +// `defaults:` so they actually reach each row (#525), and returns a human-readable +// list of what it set for the launch header. +// +// Precedence, most specific wins: +// +// a row's own params: > the CLI flag > the param file's defaults: +// +// So a per-row `ttl:` (a GPU row on a shorter leash, say) still wins, while a flag +// passed at invocation time beats a value checked into the file — which is the +// same direction cobra/viper use for flags versus config, and the reason +// `--ttl 30m` on a file that says `ttl: 8h` does what it looks like it does. +// +// A non-empty ttl/idleTimeout here can only have come from the command line: +// applyLaunchDefaults (which would otherwise fold in ~/.spawn/config.yaml) runs +// *after* the sweep early-return in launch_single.go, so ~/.spawn defaults never +// silently outrank the param file. +func applyCLISpendControlsToSweep(paramFormat *ParamFileFormat) []string { + if paramFormat.Defaults == nil { + paramFormat.Defaults = make(map[string]interface{}) + } + var applied []string + if ttl != "" { + paramFormat.Defaults["ttl"] = ttl + applied = append(applied, "ttl="+ttl) + } + if idleTimeout != "" { + paramFormat.Defaults["idle_timeout"] = idleTimeout + applied = append(applied, "idle-timeout="+idleTimeout) + } + if costLimit > 0 { + paramFormat.Defaults["cost_limit"] = costLimit + applied = append(applied, fmt.Sprintf("cost-limit=$%.2f", costLimit)) + } + return applied +} + // estimateSweepOnly prints a per-row cost estimate for a sweep and returns // without launching anything. This is the entirety of --estimate-only's // behaviour on the sweep path; both orchestration paths reach it from the single diff --git a/cmd/sweep.go b/cmd/sweep.go index edc2c09..82a8fbb 100644 --- a/cmd/sweep.go +++ b/cmd/sweep.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "path/filepath" + "strconv" + "strings" "time" "github.com/spore-host/spawn/pkg/aws" @@ -70,6 +72,44 @@ func parseParamFile(path string) (*ParamFileFormat, error) { }, nil } +// parseCostLimit coerces a param-file `cost_limit:` value to USD. It accepts every +// shape a YAML/JSON/CSV param file can produce for a number — `8`, `8.50`, and the +// quoted `"8.50"` — because the three parsers disagree about which Go type an +// unquoted number becomes, and a type mismatch here would zero the only +// per-instance dollar cap a sweep has (#525). Anything else is an error, never a +// silent 0: a disabled cap must be spelled `cost_limit: 0`, not typo'd into one. +func parseCostLimit(val interface{}) (float64, error) { + var f float64 + switch v := val.(type) { + case float64: + f = v + case float32: + f = float64(v) + case int: + f = float64(v) + case int64: + f = float64(v) + case json.Number: + parsed, err := v.Float64() + if err != nil { + return 0, fmt.Errorf("%q is not a number", v.String()) + } + f = parsed + case string: + parsed, err := strconv.ParseFloat(strings.TrimSpace(v), 64) + if err != nil { + return 0, fmt.Errorf("%q is not a number", v) + } + f = parsed + default: + return 0, fmt.Errorf("expected a number in USD, got %T (%v)", val, val) + } + if f < 0 { + return 0, fmt.Errorf("must not be negative, got %v", f) + } + return f, nil +} + // buildLaunchConfigFromParams merges defaults with parameter overrides func buildLaunchConfigFromParams(defaults, params map[string]interface{}, sweepID, sweepName string, index, total int) (aws.LaunchConfig, error) { // Start with an empty config @@ -133,6 +173,22 @@ func buildLaunchConfigFromParams(defaults, params map[string]interface{}, sweepI if s, ok := val.(string); ok { config.IdleTimeout = s } + case "cost_limit": + // Terminate/stop when compute spend reaches this many USD, i.e. the + // param-file form of --cost-limit. Before #525 there was no case for + // it, so `cost_limit: 8` fell through to the default: arm and became a + // PARAM_cost_limit env var that capped nothing — a spend control that + // parsed, launched, and did nothing. + // + // Unlike the string cases above this one *errors* on a value it cannot + // use instead of leaving the field zero. A mistyped ttl costs you the + // difference between two timeouts; a mistyped cost limit silently + // removes the only per-instance dollar cap on this path. + f, err := parseCostLimit(val) + if err != nil { + return config, fmt.Errorf("cost_limit: %w", err) + } + config.CostLimit = f case "hibernate_on_idle": if b, ok := val.(bool); ok { config.HibernateOnIdle = b diff --git a/cmd/sweep_spend_controls_test.go b/cmd/sweep_spend_controls_test.go new file mode 100644 index 0000000..61e4682 --- /dev/null +++ b/cmd/sweep_spend_controls_test.go @@ -0,0 +1,255 @@ +package cmd + +import ( + "encoding/json" + "strings" + "testing" +) + +// Unit coverage for the #525 fix. The end-to-end guarantee — that these values +// reach the EC2 tags spored enforces — lives in +// test/e2e/tier0_sweep_spend_controls_test.go; what is cheap to pin here is the +// precedence rule and the refusal to read a cost limit it cannot parse. + +// TestApplyCLISpendControlsToSweep_Precedence: an explicit flag outranks the +// param file's defaults:, and a flag left unset never overwrites the file. +func TestApplyCLISpendControlsToSweep_Precedence(t *testing.T) { + // These are package-level launch flags; restore them so test order cannot + // leak a spend control into an unrelated test. + defer func(t0, i0 string, c0 float64) { ttl, idleTimeout, costLimit = t0, i0, c0 }(ttl, idleTimeout, costLimit) + + tests := []struct { + name string + fileDefaults map[string]interface{} + flagTTL, flagIdle string + flagCost float64 + wantTTL, wantIdle interface{} + wantCost interface{} + wantAppliedContainsNone bool + }{ + { + name: "flags win over the file's defaults", + fileDefaults: map[string]interface{}{"ttl": "8h", "idle_timeout": "2h"}, + flagTTL: "30m", flagIdle: "10m", flagCost: 3.5, + wantTTL: "30m", wantIdle: "10m", wantCost: 3.5, + }, + { + name: "unset flags leave the file alone", + fileDefaults: map[string]interface{}{"ttl": "8h", "cost_limit": 9.0}, + wantTTL: "8h", + wantCost: 9.0, + wantAppliedContainsNone: true, + }, + { + name: "--cost-limit 0 means disabled, not 'set to zero'", + fileDefaults: map[string]interface{}{"cost_limit": 9.0}, + flagCost: 0, + wantCost: 9.0, // untouched: 0 is the flag's own default + wantAppliedContainsNone: true, + }, + { + name: "a nil defaults map is created, not panicked on", + fileDefaults: nil, + flagTTL: "1h", + wantTTL: "1h", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ttl, idleTimeout, costLimit = tc.flagTTL, tc.flagIdle, tc.flagCost + pf := &ParamFileFormat{Defaults: tc.fileDefaults} + + applied := applyCLISpendControlsToSweep(pf) + + for key, want := range map[string]interface{}{ + "ttl": tc.wantTTL, "idle_timeout": tc.wantIdle, "cost_limit": tc.wantCost, + } { + if want == nil { + continue + } + if got := pf.Defaults[key]; got != want { + t.Errorf("defaults[%q] = %v (%T), want %v (%T)", key, got, got, want, want) + } + } + if tc.wantAppliedContainsNone && len(applied) != 0 { + t.Errorf("reported %v as applied from the command line, but no flag was set", applied) + } + if !tc.wantAppliedContainsNone && len(applied) == 0 { + t.Error("a flag was set but nothing was reported as applied — the launch header " + + "would not mention it") + } + }) + } +} + +// TestApplyCLISpendControlsToSweep_RowStillWins: the injection targets defaults:, +// so a row's own value must still override it after the merge. This is the pair +// to the precedence test above — injecting into the wrong map would have +// clobbered every per-row override with a global one. +func TestApplyCLISpendControlsToSweep_RowStillWins(t *testing.T) { + defer func(t0 string, c0 float64) { ttl, costLimit = t0, c0 }(ttl, costLimit) + ttl, costLimit = "4h", 10 + + pf := &ParamFileFormat{ + Params: []map[string]interface{}{ + {"instance_type": "c5.large"}, + {"instance_type": "g6e.xlarge", "ttl": "30m", "cost_limit": 2}, + }, + } + applyCLISpendControlsToSweep(pf) + + for i, want := range []struct { + ttl string + cost float64 + }{{"4h", 10}, {"30m", 2}} { + cfg, err := buildLaunchConfigFromParams(pf.Defaults, pf.Params[i], "sw", "sw", i, 2) + if err != nil { + t.Fatalf("row %d: %v", i, err) + } + if cfg.TTL != want.ttl { + t.Errorf("row %d: TTL = %q, want %q", i, cfg.TTL, want.ttl) + } + if cfg.CostLimit != want.cost { + t.Errorf("row %d: CostLimit = %v, want %v", i, cfg.CostLimit, want.cost) + } + } +} + +// TestSweepRowsWithoutBound pins what the --no-detach guard now checks: the +// merged, per-row bound rather than the CLI variable. The last case is the one +// the old check could not see — a file that bounds some rows and not others. +func TestSweepRowsWithoutBound(t *testing.T) { + tests := []struct { + name string + defaults map[string]interface{} + params []map[string]interface{} + want []string + }{ + { + name: "bounded by defaults", + defaults: map[string]interface{}{"ttl": "1h"}, + params: []map[string]interface{}{{"instance_type": "c5.large"}}, + }, + { + name: "an idle timeout counts as a bound", + defaults: map[string]interface{}{"idle_timeout": "30m"}, + params: []map[string]interface{}{{"instance_type": "c5.large"}}, + }, + { + name: "bounded per row", + params: []map[string]interface{}{{"instance_type": "c5.large", "ttl": "1h"}}, + }, + { + name: "nothing anywhere", + params: []map[string]interface{}{{"instance_type": "c5.large"}}, + want: []string{"row 0 (c5.large)"}, + }, + { + name: "an empty string is not a bound", + defaults: map[string]interface{}{"ttl": ""}, + params: []map[string]interface{}{{"instance_type": "c5.large"}}, + want: []string{"row 0 (c5.large)"}, + }, + { + // The parser's type assertion drops a non-string ttl, so treating it + // as absent here is what makes the guard agree with what the launch + // will actually do rather than with what the file appears to say. + name: "a non-string ttl is not a bound (the parser drops it too)", + defaults: map[string]interface{}{"ttl": 3600}, + params: []map[string]interface{}{{"instance_type": "c5.large"}}, + want: []string{"row 0 (c5.large)"}, + }, + { + name: "only SOME rows bounded — invisible to a CLI-variable check", + params: []map[string]interface{}{ + {"instance_type": "c5.large", "ttl": "1h"}, + {"instance_type": "c5.xlarge"}, + {"instance_type": "c5.2xlarge", "idle_timeout": "20m"}, + {}, + }, + want: []string{"row 1 (c5.xlarge)", "row 3"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := sweepRowsWithoutBound(&ParamFileFormat{Defaults: tc.defaults, Params: tc.params}) + if strings.Join(got, "|") != strings.Join(tc.want, "|") { + t.Errorf("sweepRowsWithoutBound() = %v, want %v", got, tc.want) + } + }) + } +} + +// TestBuildLaunchConfigFromParams_CostLimit: `cost_limit:` is a real config key, +// not a PARAM_* passthrough. Pre-fix it had no parser case, so it landed in +// Parameters and capped nothing. +func TestBuildLaunchConfigFromParams_CostLimit(t *testing.T) { + cfg, err := buildLaunchConfigFromParams( + map[string]interface{}{"cost_limit": 8}, nil, "sw", "sw", 0, 1) + if err != nil { + t.Fatalf("buildLaunchConfigFromParams: %v", err) + } + if cfg.CostLimit != 8 { + t.Errorf("CostLimit = %v, want 8", cfg.CostLimit) + } + if v, ok := cfg.Parameters["cost_limit"]; ok { + t.Errorf("cost_limit also leaked to Parameters[%q] = %q — it would ride as a "+ + "PARAM_cost_limit env var that caps nothing", "cost_limit", v) + } +} + +// TestBuildLaunchConfigFromParams_BadCostLimitErrors: an unusable cost limit +// fails the sweep instead of quietly becoming 0 (= no cap). +func TestBuildLaunchConfigFromParams_BadCostLimitErrors(t *testing.T) { + for _, bad := range []interface{}{"eight", "", true, []interface{}{1}, -3} { + _, err := buildLaunchConfigFromParams( + map[string]interface{}{"cost_limit": bad}, nil, "sw", "sw", 0, 1) + if err == nil { + t.Errorf("cost_limit=%#v: expected an error, got none — a 0 cost limit is no cap", bad) + continue + } + if !strings.Contains(err.Error(), "cost_limit") { + t.Errorf("cost_limit=%#v: error %q does not name the offending key", bad, err) + } + } +} + +// TestParseCostLimit covers the shapes the three param-file formats produce for +// the same number: YAML gives int for `8` and float64 for `8.50`, JSON may hand +// back json.Number, and CSV values arrive as strings. +func TestParseCostLimit(t *testing.T) { + ok := map[string]interface{}{ + "yaml int": 8, + "yaml float": 8.5, + "int64": int64(8), + "float32": float32(8), + "json.Number": json.Number("8.5"), + "csv string": "8.5", + "padded string": " 8.5 ", + "zero disables": 0, + } + for name, in := range ok { + got, err := parseCostLimit(in) + if err != nil { + t.Errorf("%s (%#v): unexpected error %v", name, in, err) + continue + } + if got != 8 && got != 8.5 && got != 0 { + t.Errorf("%s (%#v): got %v", name, in, got) + } + } + for name, in := range map[string]interface{}{ + "prose": "eight dollars", + "currency": "$8", + "empty": "", + "bool": true, + "nil": nil, + "negative": -1, + } { + if got, err := parseCostLimit(in); err == nil { + t.Errorf("%s (%#v): expected an error, got %v", name, in, got) + } + } +} diff --git a/test/e2e/tier0_sweep_spend_controls_test.go b/test/e2e/tier0_sweep_spend_controls_test.go new file mode 100644 index 0000000..ea707a0 --- /dev/null +++ b/test/e2e/tier0_sweep_spend_controls_test.go @@ -0,0 +1,298 @@ +//go:build e2e_tier0 + +package e2e + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/ec2" +) + +// Tier 0 regression coverage for #525: the CLI spend controls must actually +// reach the instances a parameter sweep launches. +// +// buildLaunchConfigFromParams "start[s] with an empty config" and the sweep +// dispatch copied only Region/InstanceType/Name off the base config, so --ttl, +// --idle-timeout and --cost-limit were silently discarded for every row — while +// launch_sweep.go printed "Using safeguards: ttl=4h, idle-timeout=" from the +// variables it was about to throw away. `cost_limit:` in the param file did not +// work either: there was no parser case, so it became a PARAM_cost_limit env var +// that capped nothing. Both routes failed, which left a sweep with no +// per-instance dollar cap at all. +// +// The assertions below are on EC2 tags rather than on any in-process value, +// because the tags are what spored and the out-of-band reaper actually enforce. +// A test that asserted "the flag was parsed" would have passed throughout the +// bug's life: the flag WAS parsed, onto a config nobody copied from. +// +// All three tests take the FOREGROUND path (--no-detach), which is the one that +// provisions in-process and therefore the one Tier 0 can observe end to end. The +// detached path is covered at the same seam by construction — both paths read +// the same `defaults:` map, and the detached one uploads it to S3 for the Lambda +// orchestrator — but that orchestrator does not live in this repo, so its half +// of the fix is not verifiable here. Stated rather than implied. + +// writeSweepFile writes a param file with the given body and returns its path. +func writeSweepFile(t *testing.T, body string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "sweep.yaml") + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write param file: %v", err) + } + return path +} + +// tagsByInstanceType reads every instance out of EC2 and returns its tags keyed +// by instance type, which is how a sweep's rows are told apart here. It fails if +// two instances share a type, since that would make the mapping ambiguous. +func (e *spawnEnv) tagsByInstanceType() map[string]map[string]string { + e.t.Helper() + out, err := e.EC2Client().DescribeInstances(context.Background(), &ec2.DescribeInstancesInput{}) + if err != nil { + e.t.Fatalf("DescribeInstances: %v", err) + } + byType := map[string]map[string]string{} + for _, r := range out.Reservations { + for _, inst := range r.Instances { + itype := string(inst.InstanceType) + if _, dup := byType[itype]; dup { + e.t.Fatalf("two instances of type %s — cannot attribute tags to a row", itype) + } + tags := map[string]string{} + for _, tg := range inst.Tags { + tags[aws.ToString(tg.Key)] = aws.ToString(tg.Value) + } + byType[itype] = tags + } + } + return byType +} + +// launchForegroundSweep runs a real (foreground) sweep and requires exit 0. +func (e *spawnEnv) launchForegroundSweep(paramFile string, extra ...string) { + e.t.Helper() + args := append([]string{ + "launch", "spend-check", + "--param-file", paramFile, + "--region", "us-east-1", + "--no-detach", + "--wait-for-running=false", + "--wait-for-ssh=false", + "-y", + }, extra...) + stdout, stderr, code := e.run(args...) + if code != 0 { + e.t.Fatalf("spawn %v: expected exit 0, got %d\nstdout:\n%s\nstderr:\n%s", + args, code, stdout, stderr) + } +} + +// requireTag asserts one tag's exact value, and reports every tag on the +// instance when it is missing — the pre-fix failure was an ABSENT tag, and +// "spawn:ttl not found" on its own does not show that the launch otherwise +// succeeded. +func requireTag(t *testing.T, where string, tags map[string]string, key, want string) { + t.Helper() + got, ok := tags[key] + if !ok { + t.Errorf("%s: tag %s is absent, want %q. Tags present: %v", where, key, want, tags) + return + } + if got != want { + t.Errorf("%s: tag %s = %q, want %q", where, key, got, want) + } +} + +// TestTier0_SweepAppliesCLISpendControls is the core #525 case: a param file that +// declares no bounds of its own, launched with the spend controls on the command +// line. Pre-fix every row came up with spawn:ttl absent and no spawn:cost-limit, +// bounded only by the unrelated 1h *idle* timeout the zombie guard substitutes — +// which never fires on a compute-bound row. +func TestTier0_SweepAppliesCLISpendControls(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + on_complete: terminate +params: + - instance_type: c5.large + - instance_type: c5.xlarge +`) + env.launchForegroundSweep(file, "--ttl", "1h", "--cost-limit", "3") + + byType := env.tagsByInstanceType() + if len(byType) != 2 { + t.Fatalf("expected 2 instances (one per row), got %d: %v", len(byType), byType) + } + for itype, tags := range byType { + requireTag(t, itype, tags, "spawn:ttl", "1h") + requireTag(t, itype, tags, "spawn:cost-limit", "3.0000") + // The idle timeout must NOT be substituted once an explicit --ttl + // arrives: applyIdleTimeoutDefault only fills in when both are empty, + // and pre-fix the CLI ttl never got that far, so an idle timeout the + // user never asked for showed up in its place and read as a bound. + if v, ok := tags["spawn:idle-timeout"]; ok { + t.Errorf("%s: spawn:idle-timeout=%q was substituted despite an explicit --ttl", itype, v) + } + } +} + +// TestTier0_SweepRowTTLBeatsCLI pins the precedence: most specific wins, so a +// row's own ttl: outranks the command line. This is what lets a matrix put an +// expensive GPU row on a shorter leash than the rest of the sweep, and it is the +// half of the fix that could plausibly have been implemented backwards. +func TestTier0_SweepRowTTLBeatsCLI(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + on_complete: terminate +params: + - instance_type: c5.large + - instance_type: c5.xlarge + ttl: 30m +`) + env.launchForegroundSweep(file, "--ttl", "4h") + + byType := env.tagsByInstanceType() + requireTag(t, "row without its own ttl", byType["c5.large"], "spawn:ttl", "4h") + requireTag(t, "row with ttl: 30m", byType["c5.xlarge"], "spawn:ttl", "30m") +} + +// TestTier0_SweepCLIBeatsFileDefaults is the other half of the precedence rule: a +// flag passed at invocation time outranks a value checked into the file's +// defaults:. Without this, `--ttl 30m` on a file that says `ttl: 8h` would look +// like it worked and leave the 8h bound in place. +func TestTier0_SweepCLIBeatsFileDefaults(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + on_complete: terminate + ttl: 8h +params: + - instance_type: c5.large +`) + env.launchForegroundSweep(file, "--ttl", "30m") + + requireTag(t, "file says 8h, CLI says 30m", env.tagsByInstanceType()["c5.large"], "spawn:ttl", "30m") +} + +// TestTier0_SweepParamFileCostLimit covers the param-file route on its own, with +// no --cost-limit on the command line. `cost_limit:` had no parser case, so it +// fell through to the unknown-key arm and became the PARAM_cost_limit env var / +// spawn:param:cost_limit tag — a spend control that parsed, launched, tagged, +// and capped nothing (#526 is the general form of that failure). +func TestTier0_SweepParamFileCostLimit(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + on_complete: terminate + ttl: 1h + cost_limit: 5 +params: + - instance_type: c5.large +`) + env.launchForegroundSweep(file) + + tags := env.tagsByInstanceType()["c5.large"] + requireTag(t, "cost_limit: 5 in defaults", tags, "spawn:cost-limit", "5.0000") + if v, ok := tags["spawn:param:cost_limit"]; ok { + t.Errorf("cost_limit still leaked to spawn:param:cost_limit=%q — it is a real config "+ + "key now, not a PARAM_* passthrough", v) + } +} + +// TestTier0_SweepFileTTLSatisfiesNoDetach: the --no-detach guard used to read the +// CLI ttl variable, so a param file carrying its own `ttl:` was refused even +// though that value is the one which actually reached the instances. The only way +// through was to pass --ttl to satisfy the guard and have its value discarded — +// one flag to pass the check, another to take effect. A file-provided bound is +// now accepted, and the instance gets it. +func TestTier0_SweepFileTTLSatisfiesNoDetach(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + on_complete: terminate + ttl: 2h +params: + - instance_type: c5.large +`) + env.launchForegroundSweep(file) // no --ttl on the command line + + requireTag(t, "ttl: 2h from the file", env.tagsByInstanceType()["c5.large"], "spawn:ttl", "2h") +} + +// TestTier0_SweepNoDetachNamesUnboundedRows: the guard is per row now, so a file +// that bounds only some of its rows is refused — and the refusal names the rows, +// because "some row is unbounded" is not actionable on a 30-row sweep. The old +// CLI-variable check could not see this case at all: one --ttl satisfied it and +// then reached nothing. +func TestTier0_SweepNoDetachNamesUnboundedRows(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + on_complete: terminate +params: + - instance_type: c5.large + ttl: 1h + - instance_type: c5.xlarge +`) + stdout, stderr, code := env.run( + "launch", "partly-bounded", + "--param-file", file, + "--region", "us-east-1", + "--no-detach", + "--wait-for-running=false", + "--wait-for-ssh=false", + "-y", + ) + if code == 0 { + t.Errorf("expected a non-zero exit when a row has no bound, got 0\nstderr:\n%s", stderr) + } + if !strings.Contains(stderr, "c5.xlarge") { + t.Errorf("the error does not name the unbounded row (c5.xlarge)\nstdout:\n%s\nstderr:\n%s", + stdout, stderr) + } + if strings.Contains(stderr, "row 0") { + t.Errorf("row 0 has ttl: 1h and must not be reported as unbounded\nstderr:\n%s", stderr) + } + env.requireNothingLaunched("a sweep with one unbounded row") +} + +// TestTier0_SweepRejectsUnusableCostLimit: a cost limit that cannot be read must +// fail the launch, not default to 0. A silent 0 means "no cap", which is the +// opposite of what the operator wrote — and the only signal would have been an +// instance with no spawn:cost-limit tag, which nobody inspects until the bill +// arrives. +// +// The --ttl on the command line and the "cost_limit" assertion on the message are +// both load-bearing. Without them this test PASSED against the pre-fix tree: the +// old --no-detach guard rejected the file for having no CLI --ttl, long before +// anything looked at cost_limit, so a non-zero exit on its own proved nothing +// about the behaviour under test. Same trap the rest of this repo's spend checks +// keep hitting — a check that cannot fail is worse than no check. +func TestTier0_SweepRejectsUnusableCostLimit(t *testing.T) { + env := startSpawnSubstrate(t) + file := writeSweepFile(t, `defaults: + on_complete: terminate + cost_limit: eight dollars +params: + - instance_type: c5.large +`) + stdout, stderr, code := env.run( + "launch", "bad-cost-limit", + "--param-file", file, + "--region", "us-east-1", + "--no-detach", + "--ttl", "1h", + "--wait-for-running=false", + "--wait-for-ssh=false", + "-y", + ) + if code == 0 { + t.Errorf("expected a non-zero exit for an unparseable cost_limit, got 0\nstdout:\n%s\nstderr:\n%s", + stdout, stderr) + } + if !strings.Contains(stderr, "cost_limit") { + t.Errorf("the failure does not name cost_limit, so it may be failing for an unrelated "+ + "reason\nstdout:\n%s\nstderr:\n%s", stdout, stderr) + } + env.requireNothingLaunched("unparseable cost_limit") +}