feat: Add pdb support for target allocator #2411
Conversation
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
|
I'd say that failing test isn't related with my changes, could you reenqueue it? |
|
🤦 |
.chloggen/target-allocator-pdb.yaml
Outdated
| change_type: enhancement | ||
|
|
||
| # The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) | ||
| component: operator |
There was a problem hiding this comment.
this should be target allocator
There was a problem hiding this comment.
I added to the operator because the changes have been done in operator code. I mean, although it applies to the target allocator, the changed code is released as part of the operator (it's not part of target-allocator bin but operator bin)
I'll update this with the other comments once they are solved
| apiVersion: v1 | ||
| kind: Namespace | ||
| metadata: | ||
| name: create-pdbs-target-allocator |
There was a problem hiding this comment.
why a custom namespace is needed?
There was a problem hiding this comment.
I use a custom namespace for testing all the labels because app.kubernetes.io/instance is build namespace.crName. But I can remove that validation if you think it's not useful
There was a problem hiding this comment.
that can be unit-tested as well
| @@ -0,0 +1,149 @@ | |||
| # This creates four different deployments for checking different pdb options: | |||
There was a problem hiding this comment.
I like the test completeness but this is already tested with unit tests. It seems enough to just keep a single instance.
There was a problem hiding this comment.
I did the same for the collector pdb. Should I remove them there too? I added all the cases because it allows IntOrString as type (and it's quite "magic" for me).
It doesn't have any extra cost and covers it but I can remove them, keeping just one, as you prefer
There was a problem hiding this comment.
Those can be removed as well
There was a problem hiding this comment.
Are we ok merging this before we have a separate TargetAllocator CRD? @jaronoff97 @pavolloffay
I hope so xD #2261 (comment) |
apis/v1alpha1/collector_webhook.go
Outdated
| // we set MaxUnavailable 1, which will work even if there is | ||
| // just one replica, not blocking node drains but preventing | ||
| // out-of-the-box from disruption generated by them with replicas > 1 | ||
| if r.Spec.TargetAllocator.Enabled && r.Spec.TargetAllocator.PodDisruptionBudget == nil { |
There was a problem hiding this comment.
we should also only set this for the consistent hashing strategy
There was a problem hiding this comment.
I added a check for that in the manifest generator, is it needed in the defaulter too?
There was a problem hiding this comment.
nvm, ignore my question, after reading your second comment, it makes sense totally xD
I am fine with merging this. We need to spend some time how the TA CRD will look like in the v1alpha2 |
|
I'll address all the comments later on |
Signed-off-by: Jorge Turrado <[email protected]>
|
I've addressed all the feedback (at least I hope so), PTAL |
Signed-off-by: Jorge Turrado <[email protected]>
|
@swiatekm-sumo could you please re-review? The PR cannot be merged without your approval |
* feat: Add pdb support for target allocator Signed-off-by: Jorge Turrado <[email protected]> * fix e2e tests Signed-off-by: Jorge Turrado <[email protected]> * fix e2e tests Signed-off-by: Jorge Turrado <[email protected]> * update bundle Signed-off-by: Jorge Turrado <[email protected]> * Address feedback Signed-off-by: Jorge Turrado <[email protected]> * Change log level Signed-off-by: Jorge Turrado <[email protected]> --------- Signed-off-by: Jorge Turrado <[email protected]>
Description: Target allocator supports pdb
Link to tracking Issue: #2261
Testing: Some unit tests and e2e tests have been added to validate the content
Documentation: I've updated api docs