- Notifications
You must be signed in to change notification settings - Fork1.1k
fix: avoid connection logging crashes in agent#20307
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
| // Do not fail the loop if we fail to report a connection, just | ||
| // log a warning. | ||
| // Related to https://github.com/coder/coder/issues/20194 | ||
| logger.Warn(ctx,"failed to report connection to server",slog.Error(err)) |
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.
what is the consequence for a failed report? I see one action isDisconnect, so do we have hanging connection in our reports?
This would fundamentally change that behavior, which is ok if we can document it. Or have some other kind of timeout/heartbeat to resolve the hanging "connection"
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.
Ah I see,Connection Logs is a new page to me. TIL
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.
It's fine if connection logs get dropped. We document them as best effort
| // If the IP is "localhost" (which it can be in some cases), set it to | ||
| // 127.0.0.1 instead. | ||
| // Related to https://github.com/coder/coder/issues/20194 | ||
| ifip=="localhost" { | ||
| ip="127.0.0.1" | ||
| } |
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.
Technicallylocalhost is not case sensitive. We should cover all bases and handle differentlocalhost casing
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 thought about this, but in the end these will just show up asNULL in the DB which I think is OK.localhost (or any variant) isn't very useful anyway
| // If the IP is "localhost" (which it can be in some cases), set it to | ||
| // 127.0.0.1 instead. | ||
| // Related to https://github.com/coder/coder/issues/20194 | ||
| ifip=="localhost" { |
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.
Do we need to handle this in both agent and coderd? I'd imagine just coderd would suffice but then again doesn't hurt either.
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.
Yeah it probably doesn't need to be fixed here, but I think it's fine. It'd be great to fix the localhost issue entirely cuz it does seem like it shouldn't be happening anyway (should probably be a tailnet IP?)
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.
Yeah, I think tailnet IP would be more appropriate 👍🏻
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
6c99d5e intomainUh oh!
There was an error while loading.Please reload this page.
For main
localhostto127.0.0.1on both the server and the agentNote that the temporary fix for converting invalid IPs to localhost is not required in main since the database no longer forbids NULL for the IP column since#19788
Relates to#20194