- Notifications
You must be signed in to change notification settings - Fork926
fix: agent disconnects from coordinator#7430
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
Uh oh!
There was an error while loading.Please reload this page.
| sendClientNode(&tailnet.Node{}) | ||
| clientNodes := <-agentNodeChan | ||
| require.Len(t, clientNodes, 1) | ||
| // wait longer than the internal wait timeout. | ||
| // this tests for regression of https://github.com/coder/coder/issues/7428 | ||
| time.Sleep(tailnet.WriteTimeout * 3 / 2) |
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.
Perhaps we should shorten the write timeout for tests so that we don't add a long delay? Even though we're running tests concurrently, if we have a lot of these it'll end up impacting test times.
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 mean, I could.
I could make the write timeout an option, and plumb it through. However, when we run these tests in parallel, it can slow them down quite a bit, so I wouldn't want to set the timer too short, lest we get a flaky test under load. Maybe 1 second? 500 ms?
The current sleep is 7.5s, and I'd be able to shave it down to maybe 750ms, which doesn't really feel worth the trouble to me.
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.
To avoid plumbing, how about something like:
varWriteTimeout=func() time.Duration {ifinTest() {return1*time.Second}return5*time.Second}()
?
Maybe that'd betestutil.InTest() or just an inline check in this package. I think that'd be fine for now, at some point we may want a better way to know all places where we may be modifying behavior for tests.
Perhaps not ideal since this would affect all tests. I'll leave this up to you.
Signed-off-by: Spike Curtis <spike@coder.com>
fixes#7428