Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make lease functions less chatty #4100

Merged
merged 2 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions internal/command/deploy/machines_deploymachinesapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,14 @@ func (md *machineDeployment) updateEntriesGroup(parentCtx context.Context, group
return updatePool.Wait()
}

// releaseLease releases the lease and log the error if any.
func releaseLease(ctx context.Context, m machine.LeasableMachine) {
err := m.ReleaseLease(ctx)
if err != nil {
terminal.Warnf("failed to release lease for machine %s: %s", m.FormattedMachineId(), err)
}
}

func (md *machineDeployment) updateMachineByReplace(ctx context.Context, e *machineUpdateEntry) error {
ctx, span := tracing.GetTracer().Start(ctx, "update_by_replace", trace.WithAttributes(attribute.String("id", e.launchInput.ID)))
defer span.End()
Expand All @@ -981,7 +989,7 @@ func (md *machineDeployment) updateMachineByReplace(ctx context.Context, e *mach
}

lm = machine.NewLeasableMachine(md.flapsClient, md.io, newMachineRaw, false)
defer lm.ReleaseLease(ctx)
defer releaseLease(ctx, lm)
e.leasableMachine = lm
return nil
}
Expand Down Expand Up @@ -1059,7 +1067,7 @@ func (md *machineDeployment) spawnMachineInGroup(ctx context.Context, groupName

lm := machine.NewLeasableMachine(md.flapsClient, md.io, newMachineRaw, false)
statuslogger.Logf(ctx, "Machine %s was created", md.colorize.Bold(lm.FormattedMachineId()))
defer lm.ReleaseLease(ctx)
defer releaseLease(ctx, lm)

// Don't wait for SkipLaunch machines, they are created but not started
if launchInput.SkipLaunch {
Expand Down
2 changes: 1 addition & 1 deletion internal/command/deploy/strategy_bluegreen.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ func (bg *blueGreen) CreateGreenMachines(ctx context.Context) error {
}

greenMachine := machine.NewLeasableMachine(bg.flaps, bg.io, newMachineRaw, true)
defer greenMachine.ReleaseLease(ctx)
defer releaseLease(ctx, greenMachine)

lock.Lock()
defer lock.Unlock()
Expand Down
13 changes: 5 additions & 8 deletions internal/machine/leasable_machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,10 @@ type leasableMachine struct {
leaseNonce string
}

// TODO: make sure the other functions handle showLogs correctly
// NewLeasableMachine creates a wrapper for the given machine.
// A lease must be held before calling this function.
func NewLeasableMachine(flapsClient flapsutil.FlapsClient, io *iostreams.IOStreams, machine *fly.Machine, showLogs bool) LeasableMachine {
// TODO: make sure the other functions handle showLogs correctly
return &leasableMachine{
flapsClient: flapsClient,
io: io,
Expand Down Expand Up @@ -516,6 +518,7 @@ func (lm *leasableMachine) refreshLeaseUntilCanceled(ctx context.Context, durati
}
}

// ReleaseLease releases the lease on this machine.
func (lm *leasableMachine) ReleaseLease(ctx context.Context) error {
lm.mu.Lock()
defer lm.mu.Unlock()
Expand All @@ -536,13 +539,7 @@ func (lm *leasableMachine) ReleaseLease(ctx context.Context) error {
defer cancel()
}

err := lm.flapsClient.ReleaseLease(ctx, lm.machine.ID, nonce)
contextTimedOutOrCanceled := errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled)
if err != nil && (!contextWasAlreadyCanceled || !contextTimedOutOrCanceled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make any difference for contextWasAlreadyCanceled and !contextTimedOutOrCanceled ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! There are places where we call ReleaseLease and not logging any errors from that. Let me adjust them.

terminal.Warnf("failed to release lease for machine %s: %v\n", lm.machine.ID, err)
return err
}
return nil
return lm.flapsClient.ReleaseLease(ctx, lm.machine.ID, nonce)
}

func (lm *leasableMachine) resetLease() {
Expand Down
46 changes: 22 additions & 24 deletions internal/machine/machine_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/superfly/flyctl/internal/tracing"
"github.com/superfly/flyctl/iostreams"
"github.com/superfly/flyctl/terminal"
"golang.org/x/sync/errgroup"
)

type MachineSet interface {
Expand Down Expand Up @@ -49,36 +50,27 @@ func (ms *machineSet) GetMachines() []LeasableMachine {
return ms.machines
}

// AcquireLeases acquires leases on all machines in the set for the given duration.
func (ms *machineSet) AcquireLeases(ctx context.Context, duration time.Duration) error {
if len(ms.machines) == 0 {
return nil
}

results := make(chan error, len(ms.machines))
var wg sync.WaitGroup
// Don't override ctx. Even leaseCtx is cancelled, we still want to release the leases.
eg, leaseCtx := errgroup.WithContext(ctx)
for _, m := range ms.machines {
wg.Add(1)
go func(m LeasableMachine) {
defer wg.Done()
results <- m.AcquireLease(ctx, duration)
}(m)
eg.Go(func() error {
return m.AcquireLease(leaseCtx, duration)
})
}
go func() {
wg.Wait()
close(results)
}()
hadError := false
for err := range results {
if err != nil {
hadError = true
terminal.Warnf("failed to acquire lease: %v\n", err)
}
}
if hadError {

waitErr := eg.Wait()
if waitErr != nil {
terminal.Warnf("failed to acquire lease: %v\n", waitErr)
if err := ms.ReleaseLeases(ctx); err != nil {
terminal.Warnf("error releasing machine leases: %v\n", err)
}
return fmt.Errorf("error acquiring leases on all machines")
return waitErr
}
return nil
}
Expand All @@ -100,6 +92,7 @@ func (ms *machineSet) RemoveMachines(ctx context.Context, machines []LeasableMac
return subset.ReleaseLeases(ctx)
}

// ReleaseLeases releases leases on all machines in this set.
func (ms *machineSet) ReleaseLeases(ctx context.Context) error {
if len(ms.machines) == 0 {
return nil
Expand Down Expand Up @@ -130,10 +123,15 @@ func (ms *machineSet) ReleaseLeases(ctx context.Context) error {
}()
hadError := false
for err := range results {
contextTimedOutOrCanceled := errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled)
if err != nil && (!contextWasAlreadyCanceled || !contextTimedOutOrCanceled) {
hadError = true
terminal.Warnf("failed to release lease: %v\n", err)
if err != nil {
contextTimedOutOrCanceled := errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled)
var ferr *flaps.FlapsError
if errors.As(err, &ferr) && ferr.ResponseStatusCode == http.StatusNotFound {
// Having StatusNotFound is expected, if acquiring this entire set is partially failing.
} else if !contextWasAlreadyCanceled || !contextTimedOutOrCanceled {
hadError = true
terminal.Warnf("failed to release lease: %v\n", err)
}
}
}
if hadError {
Expand Down
Loading