diff --git a/internal/command/deploy/machines_deploymachinesapp.go b/internal/command/deploy/machines_deploymachinesapp.go index e873fbbdc2..17f9832f4a 100644 --- a/internal/command/deploy/machines_deploymachinesapp.go +++ b/internal/command/deploy/machines_deploymachinesapp.go @@ -288,9 +288,14 @@ func (md *machineDeployment) inferCanaryGuest(processGroup string) *fly.MachineG return canaryGuest } -// deployCanaryMachines creates canary machines for each process group. -// The canary machines are destroyed before returning to the caller. -func (md *machineDeployment) deployCanaryMachines(ctx context.Context) (err error) { +// deployCanaryMachines creates and health-checks a canary machine for each +// process group, returning the canaries to the caller. Unlike a smoke test, +// the canaries are registered in DNS so they serve traffic, and they are kept +// running until the caller rolls the existing machines. This keeps n+1 healthy +// serving instances available throughout the roll, avoiding downtime for +// single-machine apps. The caller is responsible for destroying the returned +// canaries (see destroyCanaryMachines) once the roll is complete. +func (md *machineDeployment) deployCanaryMachines(ctx context.Context) (canaries []machine.LeasableMachine, err error) { ctx, span := tracing.GetTracer().Start(ctx, "deploy_canary") defer span.End() @@ -299,6 +304,8 @@ func (md *machineDeployment) deployCanaryMachines(ctx context.Context) (err erro sl := statuslogger.Create(ctx, total, true) defer sl.Destroy(false) + canaries = make([]machine.LeasableMachine, total) + errors, ctx := errgroup.WithContext(ctx) for idx, name := range groupsInConfig { @@ -313,11 +320,9 @@ func (md *machineDeployment) deployCanaryMachines(ctx context.Context) (err erro name := name idx := idx errors.Go(func() error { - var err error lm, err := md.spawnMachineInGroup(ctx, name, nil, withMeta(metadata{key: "fly_canary", value: "true"}), withGuest(md.inferCanaryGuest(name)), - withDns(&fly.DNSConfig{SkipRegistration: true}), ) if err != nil { tracing.RecordError(span, err, "failed to provision canary machine") @@ -327,15 +332,10 @@ func (md *machineDeployment) deployCanaryMachines(ctx context.Context) (err erro return err } - defer func() { - if err == nil { - if destroyErr := machcmd.Destroy(ctx, md.app.Name, lm.Machine(), true); destroyErr != nil { - err = destroyErr - } - } - }() + // Safe to assign without a lock: each goroutine writes a distinct index. + canaries[idx] = lm - if err = md.runTestMachines(ctx, lm.Machine(), sl.Line(idx)); err != nil { + if err := md.runTestMachines(ctx, lm.Machine(), sl.Line(idx)); err != nil { tracing.RecordError(span, err, "failed to run test machine for canary machine") firstLine, _, _ := strings.Cut(err.Error(), "\n") statuslogger.LogfStatus(ctx, statuslogger.StatusFailure, "Failed to run test machine for canary machine: %s", firstLine) @@ -343,15 +343,33 @@ func (md *machineDeployment) deployCanaryMachines(ctx context.Context) (err erro return err } - return err + return nil }) } if err := errors.Wait(); err != nil { - return err + // Clean up any canaries that were created before the failure so we + // don't leak them when the deployment aborts. + md.destroyCanaryMachines(ctx, canaries) + + return nil, err } - return nil + return canaries, nil +} + +// destroyCanaryMachines destroys the canary machines created by +// deployCanaryMachines. nil entries (canaries that were never created) are +// skipped, and destroy failures are logged but do not abort the deployment. +func (md *machineDeployment) destroyCanaryMachines(ctx context.Context, canaries []machine.LeasableMachine) { + for _, lm := range canaries { + if lm == nil { + continue + } + if err := machcmd.Destroy(ctx, md.app.Name, lm.Machine(), true); err != nil { + terminal.Warnf("failed to destroy canary machine %s: %s", lm.FormattedMachineId(), err) + } + } } // Create machines for new process groups @@ -479,9 +497,16 @@ func (md *machineDeployment) deployMachinesApp(ctx context.Context) error { md.warnAboutProcessGroupChanges(processGroupMachineDiff) if md.strategy == "canary" && !md.isFirstDeploy { - if err := md.deployCanaryMachines(ctx); err != nil { + canaries, err := md.deployCanaryMachines(ctx) + if err != nil { return err } + // Keep the canaries serving traffic alongside the existing machines + // while they are rolled, then tear them down once the roll is complete + // (or has failed). Destroying them only after the roll guarantees an + // extra healthy instance is always available, avoiding downtime for + // single-machine apps (issue #4745). + defer md.destroyCanaryMachines(ctx, canaries) } // Destroy machines that don't fit the current process groups diff --git a/internal/command/deploy/machines_deploymachinesapp_test.go b/internal/command/deploy/machines_deploymachinesapp_test.go index cad79721e2..5cbef43805 100644 --- a/internal/command/deploy/machines_deploymachinesapp_test.go +++ b/internal/command/deploy/machines_deploymachinesapp_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/superfly/fly-go" "github.com/superfly/fly-go/flaps" "github.com/superfly/flyctl/internal/appconfig" @@ -76,3 +77,64 @@ func TestDeployMachinesApp(t *testing.T) { err := md.deployMachinesApp(ctx) assert.NoError(t, err) } + +// TestDeployCanaryKeepsInstanceDuringRoll guards against issue #4745: a canary +// deploy must not destroy the canary machine before the existing machines are +// rolled. Destroying the canary first leaves a window with no extra serving +// instance, which causes downtime for single-machine (n=1) apps. The canary +// must outlive the roll so that n+1 instances exist throughout it. +func TestDeployCanaryKeepsInstanceDuringRoll(t *testing.T) { + ios, _, _, _ := iostreams.Test() + client := &mockFlapsClient{} + webClient := &mock.Client{ + GetAppLogsFunc: func(ctx context.Context, appName, token, region, instanceID string) (entries []fly.LogEntry, nextToken string, err error) { + return nil, "", nil + }, + } + // The n=1 case from the issue: a single existing serving machine. + client.machines = []*fly.Machine{ + {ID: "m1", LeaseNonce: "m1-lease", Config: &fly.MachineConfig{Metadata: map[string]string{fly.MachineConfigMetadataKeyFlyProcessGroup: "app"}}}, + } + md := &machineDeployment{ + app: &flaps.App{}, + io: ios, + colorize: ios.ColorScheme(), + flapsClient: client, + apiClient: webClient, + strategy: "canary", + appConfig: &appconfig.Config{}, + machineSet: machine.NewMachineSet(client, ios, "", client.machines, false), + skipSmokeChecks: true, + waitTimeout: 1 * time.Second, + } + + // Shorten the NATS timeout since it's likely to require the fallback in CI + natsConnectTimeout = md.waitTimeout + + ctx := context.Background() + ctx = iostreams.NewContext(ctx, ios) + ctx = flapsutil.NewContextWithClient(ctx, client) + err := md.deployMachinesApp(ctx) + assert.NoError(t, err) + + client.mu.Lock() + defer client.mu.Unlock() + + require.NotEmpty(t, client.canaryIDs, "expected a canary machine to be launched") + canaryID := client.canaryIDs[0] + + canaryDestroyIdx := client.eventIndex("destroy:" + canaryID) + require.GreaterOrEqual(t, canaryDestroyIdx, 0, + "canary machine %s was never destroyed; events=%v", canaryID, client.events) + + // The existing machine must be rolled (destroyed and replaced, or updated) + // before the canary is torn down, so that there is always an extra serving + // instance available during the roll. + rollIdx := client.eventIndex("destroy:m1") + require.GreaterOrEqual(t, rollIdx, 0, + "expected existing machine m1 to be rolled; events=%v", client.events) + + assert.Greater(t, canaryDestroyIdx, rollIdx, + "canary %s must be destroyed AFTER the existing machine is rolled to avoid downtime; events=%v", + canaryID, client.events) +} diff --git a/internal/command/deploy/mock_client_test.go b/internal/command/deploy/mock_client_test.go index b7b30d84d3..dbab16cc11 100644 --- a/internal/command/deploy/mock_client_test.go +++ b/internal/command/deploy/mock_client_test.go @@ -35,6 +35,26 @@ type mockFlapsClient struct { machines []*fly.Machine leases map[string]struct{} nextMachineID int + + // events records an ordered log of notable flaps operations (e.g. + // "launch:", "destroy:") so tests can assert ordering, such as + // when a canary machine is destroyed relative to the rolling update. + events []string + // canaryIDs holds the IDs of machines launched with the fly_canary + // metadata, in launch order. + canaryIDs []string +} + +// eventIndex returns the index of the first occurrence of the given event in +// the recorded event log, or -1 if it was never recorded. Callers must hold m.mu. +func (m *mockFlapsClient) eventIndex(event string) int { + for i, e := range m.events { + if e == event { + return i + } + } + + return -1 } func (m *mockFlapsClient) AcquireLease(ctx context.Context, appName, machineID string, ttl *int) (*fly.MachineLease, error) { @@ -112,6 +132,10 @@ func (m *mockFlapsClient) DeleteVolume(ctx context.Context, appName, volumeId st } func (m *mockFlapsClient) Destroy(ctx context.Context, appName string, input fly.RemoveMachineInput, nonce string) (err error) { + m.mu.Lock() + m.events = append(m.events, "destroy:"+input.ID) + m.mu.Unlock() + if m.breakDestroy { return fmt.Errorf("failed to destroy %s", input.ID) } @@ -200,9 +224,15 @@ func (m *mockFlapsClient) Launch(ctx context.Context, appName string, builder fl return nil, fmt.Errorf("failed to launch %s", builder.ID) } m.nextMachineID += 1 + id := fmt.Sprintf("%x", m.nextMachineID) + + m.events = append(m.events, "launch:"+id) + if builder.Config != nil && builder.Config.Metadata["fly_canary"] == "true" { + m.canaryIDs = append(m.canaryIDs, id) + } return &fly.Machine{ - ID: fmt.Sprintf("%x", m.nextMachineID), + ID: id, LeaseNonce: fmt.Sprintf("%x-launch-lease", m.nextMachineID), }, nil }