Prepare downscale webhook

[JordanRushing]

This PR follows #25 and implements a prep_downscale mutating admission webhook that, after receiving a request that scales down a resource, sends a HTTP POST to pods in a StatefulSet/Deployment/ReplicaSet that are labeled with grafana.com/prep-downscale: true.

The endpoint targets for the requests are crafted by extracting and combining the grafana.com/prep-downscale-http-path and grafana.com/prep-downscale-http-port labels along with the diff in the number of replicas.

An example scenario for when this is useful:

• KEDA evaluates a metric query and suggests to scale down the replicas in a StatefulSet
• The pods in the StatefulSet implement a prep_shutdown HTTP endpoint that tells them to expect to be terminated on the next SIGTERM and manage their state appropriately
• The new mutating admission webhook activates, validates the scale down, successfully hits the prep_shutdown HTTP endpoints on relevant pods, and allows the admission request thus resulting in the pods being deleted gracefully

[colega]

👋 Thank you for taking this task.

Did a quick first review, and have some feedback:

• This has to be a MutatingWebhook, and it should state that it has side-effects.
  • Edit: I was wrong here, it can't be a mutating webhook really, because when the operation is /scale we can't mutate anything, but we still have side-effects.
• We should add an annotation to the downscaled resource saying when it was downscaled last time, so we can validate that and disallow downscaling other zones when one of the zones was recently downscaled.

[CLAassistant]

All committers have signed the CLA.

[colega]

Can we write an integration test please?

[MichelHollands]

As this PR is getting bigger and bigger I propose to do this in a separate PR.

[colega]

We need to make sure it's not a dry run.

[MichelHollands]

This has been added.

[colega]

I wonder if this can trigger a race condition (something updating sts in between our read & update). Can we do a Patch() operation here instead?

[MichelHollands]

Patch does work better here.

[colega]

This is looking very good! Thank you for addressing all my feedback. Left one last comment about updating operation, but otherwise LGTM!

[DylanGuedes]

looking good, just a few questions/suggestions

[DylanGuedes]

how large can be the response_body? depending on the size we might prefer to not log it

[MichelHollands]

For Loki and Mimir at the moment it will be empty. They will log the error in more detail so you have to look there.

[DylanGuedes]

WDYT?

Suggested change

	for i := 0; i < int(diff); i++ {
	index := int(*oldReplicas) - i - 1 // nr in statefulset
	eps[i].url = fmt.Sprintf("%v-%v.%v.%v.svc.cluster.local:%s/ingester/%s",
	ar.Request.Name, // pod name
	index,
	ar.Request.Name, // svc name
	ar.Request.Namespace,
	port,
	path,
	)
	eps[i].index = index
	}
	for i := int(*newReplicas); i < int(*oldReplicas); i++ {
	eps[i].url = fmt.Sprintf("%v-%v.%v.%v.svc.cluster.local:%s/ingester/%s",
	ar.Request.Name, // pod name
	index,
	ar.Request.Name, // svc name
	ar.Request.Namespace,
	port,
	path,
	)
	eps[i].index = index
	}

[MichelHollands]

That doesn't define a field called index. Do you mean using i instead of index?

[DylanGuedes]

yep, mostly because iterating from 0 to diff and evaluating index as oldreplicas - i - 1, which is a little confusing, just iterate from newReplicas to oldReplicas.

[DylanGuedes]

WDYT of having a dedicated "err" field for the err? Like:
("msg", "non-2xx received", "err", err, ...)

motiavtion is: it would allow us of searching for errors precisely by doing {mystream} |= "err="

[MichelHollands]

Yes, that makes it consistent with the other error log messages as well.

[andyasp]

Looks like this is getting close to being merged. Remember to add an entry to the CHANGELOG (and sorry for the previous dependency update conflict!)

[MichelHollands]

Looks like this is getting close to being merged. Remember to add an entry to the CHANGELOG (and sorry for the previous dependency update conflict!)

The CHANGELOG has been updated as well as the README.md. The e2e tests were fixed. However in the CI the last test fails occasionally. I'm not sure how to fix it at the moment.

[MichelHollands]

The TestExpiringCertificate was a bit flaky. After increasing one timeout there it seems a lot more stable.
Can someone merge this? I don't have permission. The same might apply for making a new release.

[colega]

Nit: during this last review, I found it hard to follow the code with all the noise of these denied responses. I would consider not introducing a var for them, and defining a method that would format the Allowed: false response, like this:

Suggested change

	reviewResponse := v1.AdmissionResponse{
	Allowed: false,
	Result: &metav1.Status{
	Message: fmt.Sprintf("downscale of %s/%s in %s from %d to %d replicas is not allowed because the %v label is not set or empty.", ar.Request.Resource.Resource, ar.Request.Name, ar.Request.Namespace, *oldReplicas, *newReplicas, PrepDownscalePortKey),
	},
	}
	level.Warn(logger).Log("msg", fmt.Sprintf("downscale not allowed because the %v label is not set or empty", PrepDownscalePortKey))
	return &reviewResponse
	level.Warn(logger).Log("msg", fmt.Sprintf("downscale not allowed because the %v label is not set or empty", PrepDownscalePortKey))
	return deny("downscale of %s/%s in %s from %d to %d replicas is not allowed because the %v label is not set or empty.", ar.Request.Resource.Resource, ar.Request.Name, ar.Request.Namespace, *oldReplicas, *newReplicas, PrepDownscalePortKey)
	// ...
	// deny returns a *v1.AdmissionResponse with Allowed: false and the message provided formatted with as in fmt.Sprintf.
	func deny(msg, args ...any) *v1.AdmissionResponse {
	return v1.AdmissionResponse{
	Allowed: false,
	Result: &metav1.Status{
	Message: fmt.Sprintf(msg, args...),
	},
	}
	}

IMO this would make this entire method shorter and easier to follow.

[MichelHollands]

Good idea. The function was added manually.

[colega]

Should we fail if this sts was downscaled few moments ago? Can we downscale again the same statefulset? I'm trying to think of corner cases that would bite us here.

[MichelHollands]

For Loki we set the stabilizationWindowSeconds in the Keda ScaledObject (which is passed to the HPA). This makes sure there is a certain time between downscales. If this is set big enough the other statefulsets should have time to finish their downscales at which point another scale down should be possible.

[colega]

I was thinking about a human operator having to do something manually. However, a human can just delete the annotation as a break-glass mechanism so it's ok.

[colega]

Thank you!