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
ResultsBatching: delay in processing time gradually increasing (blue) clearly visible, gaps are 170s of sleep time as expected. Memory consumption: 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). |
| @@ -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.
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.
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.
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: