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

Implement autosuspend #3733

Merged
merged 2 commits into from
Jul 19, 2024
Merged

Implement autosuspend #3733

merged 2 commits into from
Jul 19, 2024

Conversation

matttpt
Copy link
Contributor

@matttpt matttpt commented Jul 12, 2024

Change Summary

What and Why:

Add autosuspend support:

  • auto_suspend_machines in fly.toml now accepts "off", "stop", and "suspend" in addition to boolean values.
  • The --autostop flag for fly machines create/fly machines run/fly machines update accepts off, stop, and suspend in addition to boolean values.

Note that these changes should be backward compatible.

How:

  • Upgrade fly-go to use the new fly.MachineAutostop type, and make the necessary changes to use it.
  • Convert the fly machines commands' --autostop flag to a string, and parse it so as to accept the new values as well as the old boolean values.

Related to:


Documentation

  • Fresh Produce
  • In superfly/docs, or asked for help from docs team
  • n/a

@@ -223,11 +224,22 @@ func (md *machineDeployment) deployCreateMachinesForGroups(ctx context.Context,
}

services := groupConfig.AllServices()
autostopSetting := fly.MachineAutostopSuspend
Copy link
Member

@dangra dangra Jul 16, 2024

Choose a reason for hiding this comment

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

why isn't fly.MachineAutostopOff the default? below logic doesn't play well when there are no services; it will default to include the group in groupsWithAutosuspendEnabled.

I don't follow why you insist in using a out-of-loop variable (the autostopSetting) instead of something like:

for _, s := range services {
  switch v := s.AutostopMachines; {
  case v == nil || *v == fly.MachineAutostopOff:
    // skip
  case *v == fly.MachineAutostopStop:
    groupsWithAutostopEnabled[name] = true
  case *v == fly.MachineAutostopSuspend:
    groupsWithAutosuspendEnabled[name] = true
  }
} 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agh, thanks, I goofed this again!

I'm trying to take the minimum of the autostop settings across all of the process group's services, because that is what the proxy will use to decide how to downscale the group. E.g. if the process group has three services, two with auto_stop_machines = true and one with auto_stop_machines = false, then the proxy will take no action because of the third, not-autostoppable service. But (if I understand correctly) the existing logic would print that autostop is enabled anyway.

@matttpt matttpt force-pushed the autosuspend branch 2 times, most recently from 500302c to bb37439 Compare July 16, 2024 17:56
if len(services) > 0 {
// The proxy will use the most restrictive (which, in terms
// of the fly.MachineAutostop type, is the least) autostop
// setting across all of the group's services.
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@billyb2 billyb2 mentioned this pull request Jul 18, 2024
3 tasks
@dangra
Copy link
Member

dangra commented Jul 19, 2024

Ready to approve and merge? asking because the merged fly-go changes make it difficult to work on some changes I am trying to add :)

This updates fly-go and switches flyctl to using the new tri-state
fly.MachineAutostop type for the `auto_stop_machines` setting.
Autosuspend can be enabled by setting `auto_stop_machines = "suspend"`.
(Note that `fly.toml` parsing continues to support the original boolean
settings for this field.)
This flag now accepts the new string values for autostop ("off", "stop",
"suspend") in addition to the original boolean ones.
@matttpt
Copy link
Contributor Author

matttpt commented Jul 19, 2024

  • Updated go.mod to reference the main-branch commit of fly-go
  • Updated the help text for the --autostop flag
  • Converted unit-test TOML files to the new "off"/"stop"/"start" format, but also added auto_stop_machines = false to the old-format test to check explicitly that booleans still parse correctly

@dangra apologies for the delay—I think this is ready now!

@matttpt matttpt marked this pull request as ready for review July 19, 2024 16:39
Copy link
Member

@dangra dangra left a comment

Choose a reason for hiding this comment

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

💯 :shipit:

@matttpt matttpt merged commit a239842 into master Jul 19, 2024
37 of 38 checks passed
@matttpt matttpt deleted the autosuspend branch July 19, 2024 16:52
kzys added a commit that referenced this pull request Aug 20, 2024
The test is broken since #3733.
kzys added a commit that referenced this pull request Aug 20, 2024
The test is broken since #3733.
kzys added a commit that referenced this pull request Aug 21, 2024
* test: waiting for a minute is not enough for `fly pg import`

`fly pg import` makes an ephemeral machine which could linger for
more than a minute sometimes.

* test: TestAppsV2Example was broken

The test is broken since #3733.

* test: don't skip TestAppsV2Example

This might be the reason the test was broken.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants