feat: Eviction controller #942
Conversation
Arvindthiru
force-pushed
the
EvictionImpl
branch
from
0d52af4
to
e910804
Compare
Arvindthiru
force-pushed
the
EvictionImpl
branch
from
c1e80cd
to
0b76225
Compare
| var crp placementv1beta1.ClusterResourcePlacement | ||
| if err := r.Client.Get(ctx, types.NamespacedName{Name: eviction.Spec.PlacementName}, &crp); err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| return runtime.Result{}, err |
There was a problem hiding this comment.
controller.NewAPIServerError(
| if evictionTargetBinding == nil { | ||
| evictionTargetBinding = &crbList.Items[i] | ||
| } else { | ||
| klog.V(2).InfoS(evictionInvalidMultipleCRB, "clusterResourcePlacementEviction", evictionName, "clusterResourcePlacement", eviction.Spec.PlacementName) |
There was a problem hiding this comment.
how can this happen? We don't have two bindings point to the same cluster
There was a problem hiding this comment.
This check was added to cover the case handled here https://github.com/Azure/fleet/blob/main/pkg/controllers/rollout/controller.go#L224
| var db placementv1alpha1.ClusterResourcePlacementDisruptionBudget | ||
| if err := r.Client.Get(ctx, types.NamespacedName{Name: crp.Name}, &db); err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| return runtime.Result{}, err |
There was a problem hiding this comment.
controller.NewAPIServerError(
| } | ||
| markEvictionExecuted(&eviction, fmt.Sprintf(evictionAllowedPDBSpecified, disruptionsAllowed, availableBindings, desiredBindings, totalBindings)) | ||
| } else { | ||
| markEvictionNotExecuted(&eviction, fmt.Sprintf(evictionBlockedPDBSpecified, disruptionsAllowed, availableBindings, desiredBindings, totalBindings)) |
There was a problem hiding this comment.
this will stop the eviction forever as per line 71?
There was a problem hiding this comment.
disruptionsAllowed is always <= 0 when it is blocked, that number seems not useful
| if err := r.deleteClusterResourceBinding(ctx, evictionTargetBinding); err != nil { | ||
| return runtime.Result{}, err | ||
| } | ||
| markEvictionExecuted(&eviction, fmt.Sprintf(evictionAllowedPDBSpecified, disruptionsAllowed, availableBindings, desiredBindings, totalBindings)) |
There was a problem hiding this comment.
do user care "disruptionsAllowed" number?
| var desiredBindings int | ||
| switch crp.Spec.Policy.PlacementType { | ||
| case placementv1beta1.PickAllPlacementType: | ||
| desiredBindings = len(crbList.Items) |
There was a problem hiding this comment.
The list includes bindings that are "noScheduled", it's not the accurate representation of the desired number of bindings
| if evictionTargetBinding == nil { | ||
| evictionTargetBinding = &crbList.Items[i] | ||
| } else { | ||
| klog.V(2).InfoS(evictionInvalidMultipleCRB, "clusterResourcePlacementEviction", evictionName, "clusterResourcePlacement", eviction.Spec.PlacementName) |
There was a problem hiding this comment.
Hi Arvind! I think our scheduler/rollout controller has been configured to not create two bindings for the same cluster.
| } | ||
| } | ||
| var disruptionsAllowed int | ||
| if db.Spec.MaxUnavailable != nil { |
There was a problem hiding this comment.
Hi Arvind! The two fields are mutually exclusive and on the API definition there hasn't been validation rules yet; considering that our webhooks offer best-effort protection only; it might be better for the code here to be a switch clause (one outcome only) rather than multiple if checks (can run multiple times).
There was a problem hiding this comment.
We have API validation to prevent this concern https://github.com/Azure/fleet/blob/main/apis/placement/v1alpha1/disruptionbudget_types.go#L30
But I do agree that there should only be one outcome though
| } | ||
|
|
||
| // isEvictionAllowed calculates if eviction allowed based on available bindings and spec specified in placement disruption budget. | ||
| func isEvictionAllowed(desiredBindings int, bindings []placementv1beta1.ClusterResourceBinding, db placementv1alpha1.ClusterResourcePlacementDisruptionBudget) (bool, int, int) { |
There was a problem hiding this comment.
What if the user is trying to evict an unavailable binding?
There was a problem hiding this comment.
I think we need unavailable binding policy similar to https://kubernetes.io/docs/tasks/run-application/configure-pdb/#unhealthy-pod-eviction-policy
| reasonClusterResourcePlacementEvictionExecuted = "ClusterResourcePlacementEvictionExecuted" | ||
| reasonClusterResourcePlacementEvictionNotExecuted = "ClusterResourcePlacementEvictionNotExecuted" | ||
|
|
||
| evictionInvalidMissingCRP = "Failed to find cluster resource placement targeted by eviction" |
There was a problem hiding this comment.
Hi Arvind! Just a nit: would you mind adding a Message suffix/prefix to the variables, just to make things a bit more clear?
There was a problem hiding this comment.
Also, binding are generally speaking considered to be our internal APIs, to keep things a bit more clear on the user end, it might make more sense to just use cluster references in the message part.
| // SetupWithManager sets up the controller with the Manager. | ||
| func (r *Reconciler) SetupWithManager(mgr runtime.Manager) error { | ||
| return runtime.NewControllerManagedBy(mgr). | ||
| WithOptions(ctrl.Options{MaxConcurrentReconciles: 1}). // set the max number of concurrent reconciles |
There was a problem hiding this comment.
A nit: we might need to add a note here to explain why we have to use 1 max. concurrent reconciliations for now.
Description of your changes
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
UTs and ITs
Special notes for your reviewer
Changes to rollout controller needs to be present to add E2E tests