-
Notifications
You must be signed in to change notification settings - Fork 230
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
Even more remote heartbeat improvements #3594
Conversation
942c367
to
18dd107
Compare
Having the logic for creating a remote builder be in `web` doesn't make sense. The majority of the work can be done in `flyctl`, and as such, remove another point of failure in a different component
Since these operations are just queries (and don't mutate state), we can feel free to retry them as many times as needed
UNIT TEST WHOOOOO
If we get a non 500 error, then there's no point in having multiple seconds waiting, we should just fail.
For whatever reason, if we pass app.Name to destroy the app when we encounter an issue creating the builder, we pass nil (in unit tests). Not sure why, but all good Secondly, we should sleep between API requests
a3506f5
to
a4ebb29
Compare
c70b7fa
to
88c35b3
Compare
return nil, nil, err | ||
} | ||
|
||
if validateBuilderErr != NoBuilderApp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this block won't occur because NoBuilderApp
is an instance of ValidateBuilderError
which is checked with the errors.As()
above, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't believe so. errors.As just checks if it's a ValidateBuilderError or not, and we only return before this check if it isn't a validation error.
flapsClient := flapsutil.ClientFromContext(ctx) | ||
|
||
var volumes []fly.Volume | ||
numRetries := 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use a "backoff" library elsewhere for retries. Maybe that's better to use than rolling your own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only issue with the backoff library is that i need to exit early if we don't get a server error, instead of retrying multiple times (for example, if we get a 404, it's likely a user error)
9f704ed
to
98f45b5
Compare
This reverts commit 4fb6bd4. Ben T made some changes that should let this work again. We'll need to add a preflight test to ensure that this doesn't break in the future, however.
* Restart remote builder machine if it's stopped * Fix tests * Reapply "Even more remote heartbeat improvements (#3594)" This reverts commit 4fb6bd4. Ben T made some changes that should let this work again. We'll need to add a preflight test to ensure that this doesn't break in the future, however. * Add the recreate builder flag This allows users forcing recreating the remote builder, even if it's valid * Add a test for deploying and recreating the remote builder * Catch and trace errors
Change Summary
What and Why:
Having the logic for creating a remote builder be in
web
doesn't make sense. The majority of the work can be done inflyctl
, and as such, remove another point of failure in a different componentHow:
Move all of the logic from EnsureMachineRemoteBuilder from web to flyctl
Related to:
Documentation