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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
apiserver: add lifecycle signal for preshutdown hook #110096
Conversation
/assign @wojtek-t @aojea @p0lyn0mial @sttts |
ref: #110026 |
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.
Just one small nit.
@@ -429,7 +429,7 @@ func (s *GenericAPIServer) PrepareRun() preparedGenericAPIServer { | |||
// | | | |||
// ShutdownInitiated (shutdownInitiatedCh) | | |||
// | | | |||
// (ShutdownDelayDuration) (PreShutdownHooks) | |||
// (ShutdownDelayDuration) (PreShutdownHooksStopped) |
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.
The convention I was using was basically that inside ()
I was marking something that is happening.
The signals themselves are marked without () (and if there is something in()
after the signal, it's in alias in the code.
So I think you need to:
- leave this line as it was (basically signalling that preshutdown hooks are running at this point)
- switch line 424 to:
PreShutdownHooksStopped (preShutdownHooksHasStoppedCh
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.
fixed
/triage accepted |
9b1c727
to
f5f14e2
Compare
f5f14e2
to
b1f7b60
Compare
@@ -143,6 +147,7 @@ func newLifecycleSignals() lifecycleSignals { | |||
return lifecycleSignals{ | |||
ShutdownInitiated: newNamedChannelWrapper("ShutdownInitiated"), | |||
AfterShutdownDelayDuration: newNamedChannelWrapper("AfterShutdownDelayDuration"), | |||
PreShutdownHooksStopped: newNamedChannelWrapper("PreShutdownHooksStopped"), |
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.
don't we have a test that should fail now?
kubernetes/staging/src/k8s.io/apiserver/pkg/server/genericapiserver_graceful_termination_test.go
Lines 227 to 232 in f727b5a
lifecycleSignalOrderExpected := []string{ | |
string("ShutdownInitiated"), | |
string("AfterShutdownDelayDuration"), | |
string("HTTPServerStoppedListening"), | |
string("InFlightRequestsDrained"), | |
} |
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.
no, i am not wrapping PreShutdownHooksStopped
in the test yet, preshutdown hook is still buggy, #110026 fixes the bug and the test wraps the hook as well.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tkashem, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Ref #108483
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: