From 24895f10a0336b4b53323544b3f6be340ce4e62f Mon Sep 17 00:00:00 2001 From: Rian McGuire Date: Wed, 8 Jul 2026 16:07:09 +1000 Subject: [PATCH] fix(deploy): base launch decision on pre-deploy state On a deploy retry, updateMachineConfig rebuilt the launch input from a re-fetched oldMachine whose live State could be mid-churn (replacing/stopped/created). skipLaunch() then inferred skip_launch=true from that transient state, so updated machine was left stopped even though the machine was started prior to deploy. Make skipLaunch the single source of truth for whether a machine should run after the deploy, and always feed it a stable input: the machine's immutable pre-deploy state. --- .../deploy/machines_deploymachinesapp.go | 8 +-- .../command/deploy/machines_launchinput.go | 8 +-- internal/command/deploy/plan.go | 57 ++++++++++++------- internal/command/deploy/plan_test.go | 54 +++++++++++++----- 4 files changed, 87 insertions(+), 40 deletions(-) diff --git a/internal/command/deploy/machines_deploymachinesapp.go b/internal/command/deploy/machines_deploymachinesapp.go index e873fbbdc2..de71bf1139 100644 --- a/internal/command/deploy/machines_deploymachinesapp.go +++ b/internal/command/deploy/machines_deploymachinesapp.go @@ -694,7 +694,7 @@ func (md *machineDeployment) updateExistingMachinesWRecovery(ctx context.Context // TODO(billy) do machine checks here return md.updateUsingBlueGreenStrategy(ctx, updateEntries) case "immediate": - return md.updateMachinesWRecovery(ctx, oldAppState, &newAppState, nil, updateMachineSettings{ + return md.updateMachinesWRecovery(ctx, oldAppState, oldAppState, &newAppState, nil, updateMachineSettings{ pushForward: true, skipHealthChecks: true, skipSmokeChecks: true, @@ -724,7 +724,7 @@ func (md *machineDeployment) updateExistingMachinesWRecovery(ctx context.Context } newCanaryAppState.Machines = []*fly.Machine{canaryMach} - if err := md.updateMachinesWRecovery(ctx, &canaryAppState, &newCanaryAppState, nil, updateMachineSettings{ + if err := md.updateMachinesWRecovery(ctx, &canaryAppState, &canaryAppState, &newCanaryAppState, nil, updateMachineSettings{ pushForward: true, skipHealthChecks: md.skipHealthChecks, skipSmokeChecks: md.skipSmokeChecks, @@ -740,7 +740,7 @@ func (md *machineDeployment) updateExistingMachinesWRecovery(ctx context.Context return fmt.Errorf("failed to refresh app state after canary: %w", err) } - return md.updateMachinesWRecovery(ctx, refreshedState, &newAppState, nil, updateMachineSettings{ + return md.updateMachinesWRecovery(ctx, oldAppState, refreshedState, &newAppState, nil, updateMachineSettings{ pushForward: true, skipHealthChecks: md.skipHealthChecks, skipSmokeChecks: md.skipSmokeChecks, @@ -749,7 +749,7 @@ func (md *machineDeployment) updateExistingMachinesWRecovery(ctx context.Context case "rolling": fallthrough default: - return md.updateMachinesWRecovery(ctx, oldAppState, &newAppState, nil, updateMachineSettings{ + return md.updateMachinesWRecovery(ctx, oldAppState, oldAppState, &newAppState, nil, updateMachineSettings{ pushForward: true, skipHealthChecks: md.skipHealthChecks, skipSmokeChecks: md.skipSmokeChecks, diff --git a/internal/command/deploy/machines_launchinput.go b/internal/command/deploy/machines_launchinput.go index 0e4eaabf1b..3ad7bdc603 100644 --- a/internal/command/deploy/machines_launchinput.go +++ b/internal/command/deploy/machines_launchinput.go @@ -28,7 +28,7 @@ func (md *machineDeployment) launchInputForRestart(origMachineRaw *fly.Machine) ID: origMachineRaw.ID, Config: mConfig, Region: origMachineRaw.Region, - SkipLaunch: skipLaunch(origMachineRaw, mConfig), + SkipLaunch: shouldSkipLaunch(origMachineRaw, mConfig), MinSecretsVersion: minvers, }, nil } @@ -81,7 +81,7 @@ func (md *machineDeployment) launchInputForLaunch(processGroup string, guest *fl return &fly.LaunchMachineInput{ Region: region, Config: mConfig, - SkipLaunch: skipLaunch(nil, mConfig), + SkipLaunch: shouldSkipLaunch(nil, mConfig), MinSecretsVersion: minvers, }, nil } @@ -241,7 +241,7 @@ func (md *machineDeployment) launchInputForUpdate(origMachineRaw *fly.Machine) ( ID: mID, Region: origMachineRaw.Region, Config: mConfig, - SkipLaunch: skipLaunch(origMachineRaw, mConfig), + SkipLaunch: shouldSkipLaunch(origMachineRaw, mConfig), RequiresReplacement: machineShouldBeReplaced, MinSecretsVersion: minvers, }, nil @@ -292,7 +292,7 @@ func (md *machineDeployment) setMachineReleaseData(mConfig *fly.MachineConfig) { // Skip launching currently-stopped or suspended machines if: // * any services use autoscaling (autostop or autostart). // * it is a standby machine -func skipLaunch(origMachineRaw *fly.Machine, mConfig *fly.MachineConfig) bool { +func shouldSkipLaunch(origMachineRaw *fly.Machine, mConfig *fly.MachineConfig) bool { state := "" if origMachineRaw != nil { state = origMachineRaw.State diff --git a/internal/command/deploy/plan.go b/internal/command/deploy/plan.go index 5faf6e963b..408a1dc505 100644 --- a/internal/command/deploy/plan.go +++ b/internal/command/deploy/plan.go @@ -58,8 +58,11 @@ type AppState struct { } type machinePairing struct { - oldMachine *fly.Machine - newMachine *fly.Machine + // originalMachine is this machine as read before the deploy began. It's not re-read on recovery retries. + // nil for machines being created during the deploy. + originalMachine *fly.Machine + oldMachine *fly.Machine + newMachine *fly.Machine } // appState returns the app's state from Flaps. @@ -109,7 +112,7 @@ type updateMachineSettings struct { const rollingStrategyMaxConcurrentGroups = 16 -func (md *machineDeployment) updateMachinesWRecovery(ctx context.Context, oldAppState, newAppState *AppState, statusLogger statuslogger.StatusLogger, settings updateMachineSettings) error { +func (md *machineDeployment) updateMachinesWRecovery(ctx context.Context, originalAppState, oldAppState, newAppState *AppState, statusLogger statuslogger.StatusLogger, settings updateMachineSettings) error { ctx, span := tracing.GetTracer().Start( ctx, "update_machines_w_recovery", trace.WithAttributes(attribute.Bool("push_forward", settings.pushForward)), @@ -121,6 +124,12 @@ func (md *machineDeployment) updateMachinesWRecovery(ctx context.Context, oldApp ctx, cancel = ctrlc.HookCancelableContext(ctx, cancel) defer cancel() + originalMachines := make(map[string]*fly.Machine) + if originalAppState != nil { + for _, machine := range originalAppState.Machines { + originalMachines[machine.ID] = machine + } + } oldMachines := make(map[string]*fly.Machine) for _, machine := range oldAppState.Machines { oldMachines[machine.ID] = machine @@ -139,7 +148,7 @@ func (md *machineDeployment) updateMachinesWRecovery(ctx context.Context, oldApp machineChecksPassed: settings.skipHealthChecks, smokeChecksPassed: settings.skipSmokeChecks, }) - machineTuples = append(machineTuples, machinePairing{oldMachine: oldMachine, newMachine: newMachine}) + machineTuples = append(machineTuples, machinePairing{originalMachine: originalMachines[oldMachine.ID], oldMachine: oldMachine, newMachine: newMachine}) } } @@ -151,7 +160,7 @@ func (md *machineDeployment) updateMachinesWRecovery(ctx context.Context, oldApp machineChecksPassed: settings.skipHealthChecks, smokeChecksPassed: settings.skipSmokeChecks, }) - machineTuples = append(machineTuples, machinePairing{oldMachine: nil, newMachine: newMachine}) + machineTuples = append(machineTuples, machinePairing{originalMachine: originalMachines[newMachine.ID], oldMachine: nil, newMachine: newMachine}) } } @@ -309,7 +318,7 @@ func (md *machineDeployment) updateMachinesWRecovery(ctx context.Context, oldApp if err != nil { return err } - err = md.updateMachinesWRecovery(ctx, currentState, newAppState, sl, updateMachineSettings{ + err = md.updateMachinesWRecovery(ctx, originalAppState, currentState, newAppState, sl, updateMachineSettings{ pushForward: false, skipHealthChecks: settings.skipHealthChecks, skipSmokeChecks: settings.skipSmokeChecks, @@ -345,6 +354,7 @@ func (md *machineDeployment) updateProcessGroup(ctx context.Context, machineTupl group.SetLimit(poolSize) for _, machPair := range machineTuples { + originalMachine := machPair.originalMachine oldMachine := machPair.oldMachine newMachine := machPair.newMachine @@ -356,6 +366,14 @@ func (md *machineDeployment) updateProcessGroup(ctx context.Context, machineTupl return nil } + // Determine if the machine should be started after deployment. This is based + // on the state of originalMachine, which was captured pre-deploy and won't + // change during deploy recovery retries. + skipLaunch := false + if newMachine != nil { + skipLaunch = shouldSkipLaunch(originalMachine, newMachine.Config) + } + var machineID string if oldMachine != nil { machineID = oldMachine.ID @@ -382,7 +400,7 @@ func (md *machineDeployment) updateProcessGroup(ctx context.Context, machineTupl } machineCheckResult := checkResult.(*healthcheckResult) - err := md.updateMachineWChecks(gCtx, oldMachine, newMachine, sl, md.io, machineCheckResult) + err := md.updateMachineWChecks(gCtx, oldMachine, newMachine, skipLaunch, sl, md.io, machineCheckResult) if err != nil { sl.LogStatus(statuslogger.StatusFailure, err.Error()) span.RecordError(err) @@ -565,7 +583,7 @@ func isEmptyOrNilSlice(v interface{}) bool { return rv.Kind() == reflect.Slice && rv.Len() == 0 } -func (md *machineDeployment) updateMachineWChecks(ctx context.Context, oldMachine, newMachine *fly.Machine, sl statuslogger.StatusLine, io *iostreams.IOStreams, healthcheckResult *healthcheckResult) error { +func (md *machineDeployment) updateMachineWChecks(ctx context.Context, oldMachine, newMachine *fly.Machine, skipLaunch bool, sl statuslogger.StatusLine, io *iostreams.IOStreams, healthcheckResult *healthcheckResult) error { ctx, span := tracing.GetTracer().Start(ctx, "update_machine_w_checks", trace.WithAttributes( attribute.Bool("smoke_checks", healthcheckResult.smokeChecksPassed), attribute.Bool("machine_checks", healthcheckResult.machineChecksPassed), @@ -578,7 +596,7 @@ func (md *machineDeployment) updateMachineWChecks(ctx context.Context, oldMachin var err error - machine, lease, err = md.updateOrCreateMachine(ctx, oldMachine, newMachine, sl) + machine, lease, err = md.updateOrCreateMachine(ctx, oldMachine, newMachine, skipLaunch, sl) // if machine is nil and the lease is nil, it means we don't need to check on this machine if err != nil || (machine == nil && lease == nil) { span.RecordError(err) @@ -588,8 +606,7 @@ func (md *machineDeployment) updateMachineWChecks(ctx context.Context, oldMachin lm := mach.NewLeasableMachine(md.flapsClient, io, md.app.Name, machine, false) - shouldStart := lo.Contains([]string{"started", "replacing"}, newMachine.State) - span.SetAttributes(attribute.Bool("should_start", shouldStart)) + span.SetAttributes(attribute.Bool("should_start", !skipLaunch)) if !healthcheckResult.machineChecksPassed || !healthcheckResult.smokeChecksPassed { sl.LogStatus(statuslogger.StatusRunning, fmt.Sprintf("Waiting for machine %s to reach a good state", machine.ID)) @@ -601,7 +618,7 @@ func (md *machineDeployment) updateMachineWChecks(ctx context.Context, oldMachin } } - if !shouldStart { + if skipLaunch { sl.LogStatus(statuslogger.StatusSuccess, fmt.Sprintf("Machine %s is now in a good state", machine.ID)) return nil @@ -649,7 +666,7 @@ func (md *machineDeployment) updateMachineWChecks(ctx context.Context, oldMachin return nil } -func (md *machineDeployment) updateOrCreateMachine(ctx context.Context, oldMachine, newMachine *fly.Machine, sl statuslogger.StatusLine) (*fly.Machine, *fly.MachineLease, error) { +func (md *machineDeployment) updateOrCreateMachine(ctx context.Context, oldMachine, newMachine *fly.Machine, skipLaunch bool, sl statuslogger.StatusLine) (*fly.Machine, *fly.MachineLease, error) { ctx, span := tracing.GetTracer().Start(ctx, "update_or_create_machine") defer span.End() @@ -668,7 +685,7 @@ func (md *machineDeployment) updateOrCreateMachine(ctx context.Context, oldMachi } else { span.AddEvent("Updating old machine") sl.LogStatus(statuslogger.StatusRunning, fmt.Sprintf("Updating machine config for %s", oldMachine.ID)) - machine, err := md.updateMachineConfig(ctx, oldMachine, newMachine.Config, sl, newMachine.State == "replacing") + machine, err := md.updateMachineConfig(ctx, oldMachine, newMachine.Config, sl, newMachine.State == "replacing", skipLaunch) if err != nil { span.RecordError(err) @@ -681,7 +698,7 @@ func (md *machineDeployment) updateOrCreateMachine(ctx context.Context, oldMachi } else if newMachine != nil { span.AddEvent("Creating a new machine") sl.LogStatus(statuslogger.StatusRunning, fmt.Sprintf("Creating machine for %s", newMachine.ID)) - machine, err := md.createMachine(ctx, newMachine.Config, newMachine.Region) + machine, err := md.createMachine(ctx, newMachine.Config, newMachine.Region, skipLaunch) if err != nil { span.RecordError(err) @@ -807,7 +824,7 @@ func (md *machineDeployment) acquireMachineLease(ctx context.Context, machID str return lease, nil } -func (md *machineDeployment) updateMachineConfig(ctx context.Context, oldMachine *fly.Machine, newMachineConfig *fly.MachineConfig, sl statuslogger.StatusLine, shouldReplace bool) (*fly.Machine, error) { +func (md *machineDeployment) updateMachineConfig(ctx context.Context, oldMachine *fly.Machine, newMachineConfig *fly.MachineConfig, sl statuslogger.StatusLine, shouldReplace bool, skipLaunch bool) (*fly.Machine, error) { ctx, span := tracing.GetTracer().Start(ctx, "update_machine_config") defer span.End() if compareConfigs(ctx, oldMachine.Config, newMachineConfig) { @@ -820,6 +837,7 @@ func (md *machineDeployment) updateMachineConfig(ctx context.Context, oldMachine } input.Config = newMachineConfig input.RequiresReplacement = input.RequiresReplacement || shouldReplace + input.SkipLaunch = skipLaunch lm := mach.NewLeasableMachine(md.flapsClient, md.io, md.app.Name, oldMachine, false) entry := &machineUpdateEntry{ @@ -834,10 +852,11 @@ func (md *machineDeployment) updateMachineConfig(ctx context.Context, oldMachine return entry.leasableMachine.Machine(), nil } -func (md *machineDeployment) createMachine(ctx context.Context, machConfig *fly.MachineConfig, region string) (*fly.Machine, error) { +func (md *machineDeployment) createMachine(ctx context.Context, machConfig *fly.MachineConfig, region string, skipLaunch bool) (*fly.Machine, error) { machine, err := md.flapsClient.Launch(ctx, md.app.Name, fly.LaunchMachineInput{ - Config: machConfig, - Region: region, + Config: machConfig, + Region: region, + SkipLaunch: skipLaunch, }) if err != nil { return nil, err diff --git a/internal/command/deploy/plan_test.go b/internal/command/deploy/plan_test.go index 0aa3ea9494..33a426c18e 100644 --- a/internal/command/deploy/plan_test.go +++ b/internal/command/deploy/plan_test.go @@ -119,7 +119,7 @@ func TestUpdateMachineConfig(t *testing.T) { }, appConfig: &appconfig.Config{AppName: "myapp"}, } - _, err := md.updateMachineConfig(ctx, oldMachine, newMachineConfig, sl, false) + _, err := md.updateMachineConfig(ctx, oldMachine, newMachineConfig, sl, false, false) // updating a config with the same config should result in a noop, and shouldn't even use flaps assert.NoError(t, err) @@ -155,19 +155,47 @@ func TestUpdateMachineConfig(t *testing.T) { md.flapsClient = flapsClient // ensure that we're actually creating a new machine when we replace it - machine, err := md.updateMachineConfig(ctx, oldMachine, newMachineConfig, sl, true) + machine, err := md.updateMachineConfig(ctx, oldMachine, newMachineConfig, sl, true, false) assert.NoError(t, err) assert.Equal(t, machine.Config, newMachineConfig) assert.NotEqual(t, machine.ID, "machine2") assert.True(t, destroyedMachine) // just a regular machine update - machine, err = md.updateMachineConfig(ctx, oldMachine, newMachineConfig, sl, false) + machine, err = md.updateMachineConfig(ctx, oldMachine, newMachineConfig, sl, false, false) assert.NoError(t, err) assert.NotNil(t, machine) assert.Equal(t, machine.Config, newMachineConfig) } +func TestSkipLaunch(t *testing.T) { + t.Parallel() + + noStandbys := &fly.MachineConfig{Image: "image"} + standby := &fly.MachineConfig{Image: "image", Standbys: []string{"other"}} + + cases := []struct { + name string + machine *fly.Machine + config *fly.MachineConfig + want bool // true = skip launch (leave stopped) + }{ + {"new machine (nil) launches", nil, noStandbys, false}, + {"started stays started", &fly.Machine{State: "started"}, noStandbys, false}, + {"starting launches", &fly.Machine{State: "starting"}, noStandbys, false}, + {"failed launches", &fly.Machine{State: "failed"}, noStandbys, false}, + {"stopped stays stopped", &fly.Machine{State: "stopped"}, noStandbys, true}, + {"suspended stays stopped", &fly.Machine{State: "suspended"}, noStandbys, true}, + {"standby stays stopped", &fly.Machine{State: "stopped"}, standby, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, shouldSkipLaunch(tc.machine, tc.config)) + }) + } +} + func withQuietIOStreams(ctx context.Context) context.Context { ios, _, _, _ := iostreams.Test() @@ -314,7 +342,7 @@ func TestUpdateMachines(t *testing.T) { } acquiredLeases = sync.Map{} - err := md.updateMachinesWRecovery(ctx, oldAppState, newAppState, nil, settings) + err := md.updateMachinesWRecovery(ctx, oldAppState, oldAppState, newAppState, nil, settings) assert.NoError(t, err) // let's make sure we retry deploys a few times @@ -336,14 +364,14 @@ func TestUpdateMachines(t *testing.T) { }, nil } acquiredLeases = sync.Map{} - err = md.updateMachinesWRecovery(ctx, oldAppState, newAppState, nil, settings) + err = md.updateMachinesWRecovery(ctx, oldAppState, oldAppState, newAppState, nil, settings) assert.NoError(t, err) assert.Equal(t, 3, numFailures) numFailures = 0 maxNumFailures = 10 acquiredLeases = sync.Map{} - err = md.updateMachinesWRecovery(ctx, oldAppState, newAppState, nil, settings) + err = md.updateMachinesWRecovery(ctx, oldAppState, oldAppState, newAppState, nil, settings) assert.Error(t, err) var sentUnrecoverable atomic.Bool @@ -363,7 +391,7 @@ func TestUpdateMachines(t *testing.T) { } } acquiredLeases = sync.Map{} - err = md.updateMachinesWRecovery(ctx, oldAppState, newAppState, nil, settings) + err = md.updateMachinesWRecovery(ctx, oldAppState, oldAppState, newAppState, nil, settings) assert.Error(t, err) var unrecoverableErr *unrecoverableError assert.ErrorAs(t, err, &unrecoverableErr) @@ -516,7 +544,7 @@ func TestUpdateMachinesWithNewMachine(t *testing.T) { skipLeaseAcquisition: false, } - err := md.updateMachinesWRecovery(ctx, oldAppState, newAppState, nil, settings) + err := md.updateMachinesWRecovery(ctx, oldAppState, oldAppState, newAppState, nil, settings) assert.NoError(t, err) } @@ -607,7 +635,7 @@ func TestUpdateProcessGroupScaleUpErrorDoesNotPanic(t *testing.T) { skipLeaseAcquisition: false, } - err := md.updateMachinesWRecovery(ctx, oldAppState, newAppState, nil, settings) + err := md.updateMachinesWRecovery(ctx, oldAppState, oldAppState, newAppState, nil, settings) assert.Error(t, err) assert.Contains(t, err.Error(), "new-machine", "error must reference the new machine's ID; if oldMachine.ID was used, the test would have panicked instead") @@ -697,7 +725,7 @@ func TestUpdateOrCreateMachine(t *testing.T) { // destroy old machine reset() - mach, lease, err := md.updateOrCreateMachine(ctx, oldMachine, nil, sl) + mach, lease, err := md.updateOrCreateMachine(ctx, oldMachine, nil, false, sl) assert.NoError(t, err) assert.True(t, destroyedMachine) assert.Nil(t, mach) @@ -705,7 +733,7 @@ func TestUpdateOrCreateMachine(t *testing.T) { // update old machine reset() - mach, lease, err = md.updateOrCreateMachine(ctx, oldMachine, newMachine, sl) + mach, lease, err = md.updateOrCreateMachine(ctx, oldMachine, newMachine, false, sl) assert.NoError(t, err) assert.True(t, updatedMachine) assert.NotNil(t, mach) @@ -714,7 +742,7 @@ func TestUpdateOrCreateMachine(t *testing.T) { // create new machine reset() - mach, lease, err = md.updateOrCreateMachine(ctx, nil, newMachine, sl) + mach, lease, err = md.updateOrCreateMachine(ctx, nil, newMachine, false, sl) assert.NoError(t, err) assert.True(t, createMachine) assert.NotNil(t, mach) @@ -723,7 +751,7 @@ func TestUpdateOrCreateMachine(t *testing.T) { // new and old machines are nil, so noop reset() - mach, lease, err = md.updateOrCreateMachine(ctx, nil, nil, sl) + mach, lease, err = md.updateOrCreateMachine(ctx, nil, nil, false, sl) assert.NoError(t, err) assert.False(t, destroyedMachine) assert.False(t, updatedMachine)