-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: Configure proxy container for graceful termination #425
Conversation
Labeled |
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 add something to the documentation for this?
c429d91
to
27ff556
Compare
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.
Sounds good on #400. Could we update the title? I'm not sure exactly what the scope is as written.
s.addProxyContainerEnvVar(p, "CSQL_PROXY_EXIT_ZERO_ON_SIGTERM", "true") | ||
|
||
// Configure the pre-stop hook for /quitquitquit | ||
c.Lifecycle = &corev1.Lifecycle{ |
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.
Should we add a test that verifies this behavior?
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.
done.
55cf55b
to
35e33ff
Compare
35e33ff
to
73ea5ad
Compare
@@ -911,6 +915,91 @@ func TestPodTemplateAnnotations(t *testing.T) { | |||
|
|||
} | |||
|
|||
func TestQuitUrlEnvVar(t *testing.T) { |
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.
URL.
Watch out for initialisms.
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.
csqls[2].ObjectMeta.Generation = 3 | ||
|
||
var wantQuitUrlsEnv = fmt.Sprintf( | ||
"http://localhost:%d/quitquitquit http://localhost:%d/quitquitquit http://localhost:%d/quitquitquit", workload.DefaultAdminPort, |
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.
nit: shorter line length is easier to read. how about this?
var wantQuitUrlsEnv = strings.Join(
[]string{
fmt.Sprinf("http://localhost:%d/quitquitquit", workload.DefaultAdminPort),
fmt.Sprinf("http://localhost:%d/quitquitquit", workload.DefaultAdminPort),
fmt.Sprinf("http://localhost:%d/quitquitquit", workload.DefaultAdminPort),
},
" ",
)
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.
|
||
func TestPreStopHook(t *testing.T) { | ||
|
||
var ( |
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.
var u = ...
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.
73ea5ad
to
9a92643
Compare
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.
Updated based on Eno's comments.
@@ -911,6 +915,91 @@ func TestPodTemplateAnnotations(t *testing.T) { | |||
|
|||
} | |||
|
|||
func TestQuitUrlEnvVar(t *testing.T) { |
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.
csqls[2].ObjectMeta.Generation = 3 | ||
|
||
var wantQuitUrlsEnv = fmt.Sprintf( | ||
"http://localhost:%d/quitquitquit http://localhost:%d/quitquitquit http://localhost:%d/quitquitquit", workload.DefaultAdminPort, |
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.
|
||
func TestPreStopHook(t *testing.T) { | ||
|
||
var ( |
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.
9a92643
to
38c4a9a
Compare
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.
LGTM
We need to pin the next release of the proxy before we can merge this, right?
No, this will work with the current proxy version and represents a significant improvement on the operator behavior. It will work even better when the proxy is released with PR# 1947 We can merge this PR as-is. And then when the proxy is released, we will update the default proxy image version in a separate PR. |
Add the following configuration to the workload pods so that the proxy can gracefully exit
when the main container is done.
Configure the proxy container to exit 0 when it is terminated. Send a SIGTERM to the proxy container. We want
the proxy to exit with code 0, indicating a clean termination. Without this change the proxy container would
exit with exit code 140 (meaning terminated), which would cause kubernetes to report the
pod as "exited in an error state."
Configure a workload lifecycle handler so that kubernetes calls GET /quitquitquit before terminating the proxy container.
This should give the proxy container the chance to exit gracefully before kubernetes sends a SIGTERM to the
proxy process.
Always enable the /quitquitquit proxy api.
Always set the CSQL_QUIT_URLS environment variable to a space-separated list of proxy quitquitquit urls. This way, when
the main workload container is ready to exit, it can on workload pods. When a job or cronjob's main process
is done, that container can iterate over
Fixes #361