fix(sumologicextension): End the heartbeatloop immediately on shutdown using the parent context #32931
Conversation
github-actions
bot
requested review from
aboguszewski-sumo,
kasia-kujawa,
mat-rumian,
sumo-drosiek and
swiatekm
| // When the context is done ... | ||
| <-ctx.Done() | ||
| // ... cancel the ongoing heartbeat request. | ||
| cancel() |
There was a problem hiding this comment.
I don't understand this solution the way it's currently written. The goroutine won't get past <-ctx.Done() until cancel has already been called. What's added value of calling it twice (once here, and once in the above defer?
What's the purpose of the goroutine here?
There was a problem hiding this comment.
I meant to remove defer cancel(). Essentially what I'm trying to do here is pass the parent context into the heartbeat loop so that when the component stops the heartbeats stop immediately.
| if se.registrationInfo.CollectorCredentialID == "" || se.registrationInfo.CollectorCredentialKey == "" { | ||
| se.logger.Error("Collector not registered, cannot send heartbeat") | ||
| return | ||
| } | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| ctx, cancel := context.WithCancel(ctx) |
There was a problem hiding this comment.
For long running background processes started during Start, the spec requires using context.Background() instead of the passed in context, as explained here.
There was a problem hiding this comment.
thanks for sharing this. I wanted the long running operation (heartbeat) to be cancelled immediately on calling shutdown (hence the parent context). The reason for this is because I think there's some delay before the heartbeats are cancelled on component shutdown, that's causing this flaky test issue
There was a problem hiding this comment.
Having said that perhaps this isn't the best way to do this. Maybe we're better off removing the test until the root cause is confirmed.
There was a problem hiding this comment.
this PR- #32937 removes the flaky test.
…shutdown using context
rnishtala-sumo
force-pushed
the
fix-sumo-extension
branch
from
51ee50d to
89ec0be
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
Cancel the heartbeatloop immediately on shutdown using the parent context. The reason for a flaky test could be because the collector is started and shutdown twice in a test. When this happens the last heartbeat from the previous execution could leak in as the first request from the next collection execution
Restart the test server to ignore any possible heartbeats from a previous heartbeatloop that was shutdown
Link to tracking Issue: #32785
Testing: Unit tests and manual testing of heartbeats