search

superfly / flyctl

Command line tools for fly.io services
https://fly.io
Apache License 2.0
1.36k stars 224 forks source link

Add scheduled machines to fly.toml #3640

Open lillian-b opened 3 weeks ago

lillian-b commented 3 weeks ago

Needed this for something, thought I'd throw it upstream.

lillian-b commented 3 weeks ago

Oops, I thought SkipLaunch was a CLI flag like in fly m create. No problem, I will add tests early next week (I am away for the rest of the week).

lillian-b commented 2 weeks ago

@dangra I added tests. one issue: there is one failing one that checks the schedule field of a non-scheduled machine is 24/7, but I can't find any reference to that value anywhere, nor is that value actually set on a real non-scheduled machine. what is the correct value there?

dangra commented 2 weeks ago

there is one failing one that checks the schedule field of a non-scheduled machine is 24/7,but I can't find any reference to that value anywhere, nor is that value actually set on a real non-scheduled machine. what is the correct value there?

the '24/7' value is a placeholder, the idea is that any machine config field that isn't directly configurable from fly.toml must remain unchanged when updating an existing machine with that field set. Your change is breaking this assumption because it makes MachineConfig.Schedule depend on a value configurable from fly.toml.

Before, for an application to set a machine schedule, it has to fly deploy to create new machines and then fly machine update --schedule hourly MACHINEID to set a schedule; as schedules weren't configurable with fly.toml, flyctl had to keep its value on the next machine updates caused by fly deploy. I must say this is fragile; fly deploy replaces (not updates) machines on some conditions so schedules may get lost in the process. This is why I think we can make schedules always depends on fly.toml and reset MachineConfig.Schedule if no schedule is defined in the application config.

That said, please remove the 24/7 and add a new testcase for the [[schedule]] section like in internal/appconfig/testdata/tomachine*.toml

lillian-b commented 2 weeks ago

in that case, I would probably be worried that people with existing scheduled machines are going to have their schedules removed. should there be a warning printed in that case?

I will add the new test later.

dangra commented 2 weeks ago

in that case, I would probably be worried that people with existing scheduled machines are going to have their schedules removed. should there be a warning printed in that case?

This is a valid concern, I did a quick check on existing machines and there are a few (20 something) that relies on the old behavior from keep working.

there is a way to know when not to reset the machine Schedule is by checking the flyctl's version set as machine metadata. The metadata key we are looking for is fly_flyctl_version and its value looks like 0.1.119, 0.2.72 and so on (there are a few 2024.06.20-dev* that can be ignored)

Point is, after we detect the flyctl version is pre the version your patch is going in, another metadata key should be added so following fly deploys keep respecting it, because fly_flyctl_version is updated to current flyctl version on deploy.

makes sense?