Allow statefulsets to be scaled up and down based on each other #62
Conversation
56quarters
force-pushed
the
56quarters-replicas
branch
from
9437b0b
to
1f04d88
Compare
Allow statefulsets within a rollout group to declare a "leader" and set their number of replicas based on the number of replicas in the leader statefulset. Scaling up is allowed immediately while scaling down must respect the minimum amount of time between scale downs. For example at minimum, 12 hours needs to pass between scale downs of Mimir ingester zones. Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters
force-pushed
the
56quarters-replicas
branch
from
6604159
to
32ddfe1
Compare
pkg/controller/controller.go
Outdated
| @@ -198,6 +199,10 @@ func (c *RolloutController) reconcile(ctx context.Context) error { | |||
| groups := groupStatefulSetsByLabel(sets, RolloutGroupLabel) | |||
| var reconcileErrs error | |||
| for groupName, groupSets := range groups { | |||
| if err := c.reconcileStatefulSetsGroupReplicas(ctx, groupName, groupSets); err != nil { | |||
There was a problem hiding this comment.
I don't really like the way this functionality integrates with the existing reconcile loop, it feels "bolted on" to me. However, I did it like this to minimize changes to the existing logic and make it easier to develop. WDYT?
There was a problem hiding this comment.
Why you're calling this function here? I would feel more natural to call it inside reconcileStatefulSetsGroup(). Few reasons (not in order of importance):
- The logic here should accounted by the metrics tracked by
reconcileStatefulSetsGroup() sortStatefulSets()is already called byreconcileStatefulSetsGroup()- I think it's safer that if we do any change to replicas, we skip the rest of "rollout" reconciliation in this loop and we'll re-reconcile in the next iteration. This allows us to avoid to take in account "what happens if in the same reconciliation loop we both do changes to replicas and delete pods to rollout changes"?
There was a problem hiding this comment.
Makes sense, thank you. I'll refactor a bit to move the replica reconciliation around.
|
|
||
| - For `ingester-zone-a`, add the following: | ||
| - Labels: | ||
| - `grafana.com/min-time-between-zones-downscale=12h` (change the value here to an appropriate duration) |
There was a problem hiding this comment.
I'd like to make both of these labels into annotations because they don't need to be labels but wanted to get this functionality rolled out first. We'll have to coordinate with Loki for that change.
pkg/controller/controller.go
Outdated
| @@ -198,6 +199,10 @@ func (c *RolloutController) reconcile(ctx context.Context) error { | |||
| groups := groupStatefulSetsByLabel(sets, RolloutGroupLabel) | |||
| var reconcileErrs error | |||
| for groupName, groupSets := range groups { | |||
| if err := c.reconcileStatefulSetsGroupReplicas(ctx, groupName, groupSets); err != nil { | |||
There was a problem hiding this comment.
Why you're calling this function here? I would feel more natural to call it inside reconcileStatefulSetsGroup(). Few reasons (not in order of importance):
- The logic here should accounted by the metrics tracked by
reconcileStatefulSetsGroup() sortStatefulSets()is already called byreconcileStatefulSetsGroup()- I think it's safer that if we do any change to replicas, we skip the rest of "rollout" reconciliation in this loop and we'll re-reconcile in the next iteration. This allows us to avoid to take in account "what happens if in the same reconciliation loop we both do changes to replicas and delete pods to rollout changes"?
|
Very clean work! Agree with Marco's comments, especially about changing replicas when reconciling each statefulset group. |
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Allow statefulsets within a rollout group to declare a "leader" and set their number of replicas based on the number of replicas in the leader statefulset. Scaling up is allowed immediately while scaling down must respect the minimum amount of time between scale downs. For example at minimum, 12 hours needs to pass between scale downs of Mimir ingester zones.