-
Notifications
You must be signed in to change notification settings - Fork 507
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
Use --bootstrap to wait for remote to become active #1103
Conversation
@jedevc Can we remove this step now since these changes: buildx/.github/workflows/e2e.yml Lines 150 to 163 in 43968ff
|
56d4ce3
to
c245f30
Compare
Signed-off-by: Justin Chadwell <[email protected]>
} | ||
|
||
func (d *Driver) Info(ctx context.Context) (*driver.Info, error) { | ||
c, err := d.Client(ctx) | ||
if err != nil { | ||
return nil, errors.Wrapf(driver.ErrNotConnecting, err.Error()) | ||
return &driver.Info{ |
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 think this should still return error and we should check for "network error" on the caller side.
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 agree about preserving the error to be able to display - I'm not sure about how much to change the caller side. Info
gets called before Boot
, which requires some fiddly logic changes in generic code to ignore network errors in that case.
I think there's two scenarios for errors here:
- Failure to connect to the endpoint providing the driver (can't connect to kubernetes, can't connect to docker daemon, etc) - this kind of failure means it's not worth trying to bootstrap, something is wrong.
- Failure to connect to the individual driver node (as in this case) - bootstrapping here is fine
Could we maybe rework status to be a struct and add an Err
field to it? Then we could express a status of a node that's inactive
because of an err
. It currently feels like the error returned from here is quite tied to it's status, and is a little inconsistent across the drivers.
Previously, the
--bootstrap
flag didn't do anything with the remote driver - now it should wait until the remote becomes active, with a small linear backoff.This should help improve scenarios where the remote may not come up instantly if it has been started just before as was the case in the issue that caused #1094.