Propagate Context #908
Propagate Context #908
Conversation
|
This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days). |
There was a problem hiding this comment.
I think it's a good step. It implements what we talked about in the fast, with a real explanation of why we use context.
Yet, I am not the best person to judge wheter this is enough or whether it's the right context. (And I sadly don't have enough time to delve into it).
Would you mind explaining why it mattered for you? And why did you want to use background context, please? And why you used oklog?
Thank you.
Passing down a context makes sure that when a caller wants to cancel an operation, some function down the line will not block until it returns. Instead it would probably return a context canceled.
This is normally the context you use in the "root" of the application. https://pkg.go.dev/context#Background
At the moment kured starts 2 concurrent processes (go threads) in its main function and the metrics server that blocks the main thread forever. |
Propagate a context from the main function to wherever it is needed. Use `github.com/oklog/run` run groups to handle the life cycle of the go routines running the rebooter and metrics server. Ref: kubereboot#234 and kubereboot#808 Signed-off-by: leonnicolas <[email protected]>
leonnicolas
force-pushed
the
propagate_context
branch
from
10ee650 to
97d236d
Compare
cmd/kured/main.go
Outdated
| @@ -421,22 +425,23 @@ func (kb KubernetesBlockingChecker) isBlocked() bool { | |||
| return false | |||
| } | |||
|
|
|||
| func rebootBlocked(blockers ...RebootBlocker) bool { | |||
| func rebootBlocked(ctx context.Context, blockers ...RebootBlocker) bool { | |||
| // TODO: can we do that concurrently | |||
There was a problem hiding this comment.
So that we don't commit a // TODO comment, could you add an issue describing this optimization instead? (agree that this is out of scope for this PR)
thank you!
Instead create an issue to address the possible optimization. Signed-off-by: leonnicolas <[email protected]>
|
This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days). |
|
Now that you commented on this PR, I think everyone can follow your train of thoughts in the future. I am fine with merging this. The slight issue I have right now, is my lack of lab to test this. Can anyone test this and see the impact on stuff like the prom' integration? If it still works for someone, I am okay to go forward. |
|
This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days). |
Propagate a context from the main function to wherever it is needed.
Use
github.com/oklog/runrun groups to handle the life cycle of the goroutines running the rebooter and metrics server.
Ref: #234
and #808
Signed-off-by: leonnicolas [email protected]