launch: Use appconfig.Compute for Plan guest information.
#3219
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alichay
commented
Jan 31, 2024
internal/command/launch/launch.go
Outdated
Comment on lines
91
to
105
| // Fallback for versions of the UI that don't support a Compute field in the Plan. | ||
| var ( | ||
| io = iostreams.FromContext(ctx) | ||
| defaultGroup = state.appConfig.DefaultProcessName() | ||
| matchedComputeStrong *appconfig.Compute | ||
| matchedComputeWeak *appconfig.Compute | ||
| ) | ||
|
|
||
| { // Temporary: set the appconfig Compute configuration based on the plan | ||
| for _, compute := range state.appConfig.Compute { | ||
| if slices.Contains(compute.Processes, defaultGroup) { | ||
| if matchedComputeStrong != nil { | ||
| fmt.Fprintf(io.Out, "Warning: multiple compute configurations match the default process group %s\n", defaultGroup) | ||
| } | ||
| matchedComputeStrong = compute | ||
| } else if len(compute.Processes) == 0 { | ||
| if matchedComputeWeak != nil { | ||
| fmt.Fprintf(io.Out, "Warning: multiple compute configurations exist that match all processes\n") | ||
| } | ||
| matchedComputeWeak = compute | ||
| } | ||
| } | ||
| // NOTE: the calls to applyGuestToCompute here will *still* clobbers guest config like kernel or gpu, | ||
| // even though we're trying to be good citizens and modifying instead of overwriting the compute config. | ||
| // The only real solution to this is updating the UI to return a Compute field. | ||
| switch { | ||
| case matchedComputeStrong != nil: | ||
| applyGuestToCompute(matchedComputeStrong, state.Plan.Guest()) | ||
| case matchedComputeWeak != nil: | ||
| applyGuestToCompute(matchedComputeWeak, state.Plan.Guest()) | ||
| default: | ||
| state.appConfig.Compute = append(state.appConfig.Compute, guestToCompute(state.Plan.Guest())) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm going to rewrite this. This is 100% "I am very tired" code
dangra
approved these changes
Feb 1, 2024
This moves us closer to how fly.tomls get serialized Unfortunately, we'll have to do some weird plan patching in a later commit to make this compatible with the plan shape the UI currently expects
This means we can write code *now* that expects the Launch Plan to have full compute fields, but also means that when the UI gets support for returning them, then we'll automatically start supporting that.
This allows launch to piggyback off the same logic. Initially I wanted to just copy-paste this into Launch and be done with it, but I had to copy `matchesGroups` too which just turns it into a big mess of unmaintainable code. Not that this is *much* better, but it's a little better (I hope)
alichay
force-pushed
the
chore/launch/use-appconfig-compute
branch
from
2c36929
to
e6ab3a5
Compare
# Conflicts: # internal/command/deploy/deploy.go
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Summary
What and Why: The Launch Plan now uses
appconfig.Computeinstead of the disparate fields likeCPUsorMemoryMB. We still support reading those fields from the UI, though, and throw them into the default process' Compute definition when modifying the plan.This means that the UI could be changed to write the Compute field instead of these individual fields, which would allow defining multiple guests for different groups (although I'm not sure we want that), but importantly it would allow defining things like launching GPU machines.
How: When we create the plan, we fill in both the Compute field, and all the deprecated individual fields. When we read the Plan from the UI, if the Compute field is empty (meaning the UI has not transmitted it back to us), we'll reconstruct it by taking the Compute from the appConfig, and modifying the default group's Compute with the values the UI passed back to us.
This should be totally transparent to users, should make fly.toml import a little bit better, and should future-proof Launch for things to come :)
Related to: conversation with Daniel in #fly-launch
Documentation