k8s: update PortForwardStatus after initializing forwards#4658
Conversation
There are 3 cases where status gets updated for a forward: * Forward started successfully (StartedAt set as well as local port, which is useful if a randomized one was requested) * Forwarder instance can't be created (bad config) * Forwarder returns an error after start To simplify error handling and keep the status update logic as centralized as possible, the actual initialization changed slightly to happen in a single method. There is some very basic logic to avoid spamming the API server with updates on errors: it will only attempt to do an update once a second if continuously failing. (Even then, it will attempt to avoid a no-op update if the error hasn't actually changed.)
| return | ||
| } | ||
| select { | ||
| case <-doneCh: |
There was a problem hiding this comment.
should we also select on ctx.Done()?
There was a problem hiding this comment.
Shouldn't matter (if context is canceled, ForwardPorts() should return, so doneCh will get closed) but can't hurt anything and might avoid a nasty surprise in the future if we refactor - added in 83f8bc3
|
|
||
| // Otherwise, repeat the loop, maybe logging the error | ||
| if err != nil { | ||
| logger.Get(ctx).Infof("Reconnecting... Error port-forwarding %s (%d -> %d): %v", |
There was a problem hiding this comment.
hmm....it seems like we still want this logging to tell the user that we're reconnecting?
right now, it looks like we never print the error message at all....
There was a problem hiding this comment.
Yep, good catch. Re-added in 91a5eb6.
FWIW I kept the logic the same in that it will log every invocation (that "maybe" in the comment was incorrect), but it might make sense to make this conditional along with API update to avoid flooding the logs (if you pick a privileged port it's...a lot)
2 participants
There are 3 cases where status gets updated for a forward:
port, which is useful if a randomized one was requested)
To simplify error handling and keep the status update logic
as centralized as possible, the actual initialization changed
slightly to happen in a single method.
There is some very basic logic to avoid spamming the API
server with updates on errors: it will only attempt to do
an update once a second if continuously failing. (Even then,
it will attempt to avoid a no-op update if the error hasn't
actually changed.)
(This is based off Maia's original stub PR: #4621)