-
Notifications
You must be signed in to change notification settings - Fork 28
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
Bucket drain events using semaphore #49
Conversation
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
==========================================
- Coverage 73.06% 73.05% -0.02%
==========================================
Files 12 12
Lines 995 1002 +7
==========================================
+ Hits 727 732 +5
- Misses 206 207 +1
- Partials 62 63 +1 Continue to review full report at Codecov.
|
Test
Resultsdelay in processing time gradually increasing (blue) clearly visible, gaps are 170s of sleep time as expected. Memory consumption seem to hover around 500mb per batch of 32 x I have also bumped up the examples to request 2048Mi Edge cases to considerin scenarios where the max-drain-timeout is set to a large number, and drains are taking a very long time (e.g. due to PDB / scale up), AND a large number of nodes is terminating, it might take a long time to start deregistration for last batches, however this scenario is highly unlikely as it would mean all 32 kubectl threads are being delayed - also this can be configured by either setting a lower timeout or adding more parallel drains (and memory). |
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.
LG
@@ -101,6 +104,7 @@ func init() { | |||
serveCmd.Flags().StringVar(&queueName, "queue-name", "", "the name of the SQS queue to consume lifecycle hooks from") | |||
serveCmd.Flags().StringVar(&kubectlLocalPath, "kubectl-path", "/usr/local/bin/kubectl", "the path to kubectl binary") | |||
serveCmd.Flags().StringVar(&logLevel, "log-level", "info", "the logging level (info, warning, debug)") | |||
serveCmd.Flags().Int64Var(&maxDrainConcurrency, "max-drain-concurrency", 32, "maximum number of node drains to process in parallel") |
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.
You might just want the unsigned int uint
instead, then you just convert to int64 for the semaphore.NewWeighted(int64(maxDrainConcurrency))
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.
What is the difference with using int
and converting? uint
is simply not accepting negative numbers, it would also be possible to use int
and convert to int64
, no?
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.
I guess my point is, if we know we only use this variable as an int64
why do we need an extra conversion?
Fixes #48
Limit is currently set to 32 concurrent terminations, once drain is completed semaphore is released.
Example:
80 instances are terminated > 32 will begin draining, others are blocked (heartbeat is sent) > once drain is completed semaphore is released > next batch starts draining.
This causes a delay in handling lifecycle events up to (TerminatingInstances / 32 * MaxDrainTime)
TBD:
Testing: