Implement zoneTracker for coordinating downscales across statefulsets using a file in object storage for the prepare_downscale admission webhook #96

JordanRushing · 2023-11-14T17:35:04Z

This PR introduces the idea of a zoneTracker using a JSON-encoded file stored in object storage to save timestamps for coordinating downscales between statefulsets with the prepare-downscale admission webhook.

The file's contents:

{
  "ingester-zone-a": {
    "lastDownscaled": "2023-12-14T23:37:41Z"
  },
  "ingester-zone-b": {
    "lastDownscaled": "2023-12-14T23:37:41Z"
  },
  "ingester-zone-c": {
    "lastDownscaled": "2023-12-14T23:37:41Z"
  }
}

When rolling out the prepare-downscale admission webhook to support ingester autoscaling in Loki, we encountered differences in behavior w.r.t the last-downscale annotation that the rollout operator was applying to ingester statefulsets between the major cloud service provider's managed Kubernetes control planes.

HPA controllers in EKS/AKS fail to scale the relevant ingester statefulset after a valid AdmissionRequest is approved in the prepare-downscale webhook due to resourceVersion mismatch errors related to the last-downscale annotation being applied to the ingester statefulsets. We currently believe this is due to differences in managed control plane configurations between the providers but were unable to pinpoint a root cause even after working with the relevant support team for AWS.

Initially, we attempted to submit the annotation patch as a JSONPatch within the AdmissionResponse itself to solve this problem but even that patch was not correctly applied to the ingester statefulsets so downscales were not correctly coordinated.

As a workaround, we considered tracking this state in a file instead of a Kubernetes annotation. A draft PR was opened to do this and it worked with supported changes in the Loki API, but with trade-offs from saving this state on local disk.

After further consideration, we decided to store this file in object storage instead of PVs to simplify and fortify this state tracking with the strong read-after-write consistency guarantees that are provided by the three largest cloud provider's object storage services.

This has been successfully tested as a drop-in replacement for existing namespaces that were using the annotation to coordinate downscales prior on AWS/GCP/Azure (it essentially applies two conditionals to the existing code). It would be possible to add an in-memory bucket option to be configured as well if desired.

go mod vendor & go mod tidy were run as well.

TODO

Initial zone file creation
Bucket configuration and support s3/gcs/abs
More tests

… using a file in object storage for the prepare_downscale admission webhook Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

…itial zone file if not present Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

cstyan

Personally I'd look for a way to start breaking up prepareDownscale into some more functions. Especially with the amount of nested conditionals, it's getting a bit difficult to read. Maybe everything that happens in the block for if the zoneTracker file is being used can be a separate function?

cstyan · 2023-11-17T01:10:04Z

pkg/admission/prep_downscale.go

+	var zt *zoneTracker
+	if useZoneTracker {
+		// TODO(jordanrushing): fix bucket creation semantics and wire-in config supporting multiple CSPs
+		bkt := objstore.NewInMemBucket()
+		zt = newZoneTracker(bkt, "testkey")
+	}
+
+	return prepareDownscale(ctx, logger, ar, api, client, zt)


Like I mentioned in our 1:1, I think the easiest option for now is to turn the zoneTracker param into a string. If it's not nil, require that it be one of gcs, s3, whatever azure storage is called, etc. That's probably easier than trying to wire up a config file, configmap that populates that config file, etc. so that there could be a zoneTracker storage config in the file that specifies the storage backend.

I ended up adding the necessary config with flags passed to the zoneTracker instantiation before passing zt through to prepareDownscale

cstyan · 2023-11-17T01:14:56Z

pkg/admission/prep_downscale.go

+				level.Warn(logger).Log("msg", "downscale not allowed due to error while parsing downscale timestamps from the zone file", "err", err)
+				return deny(
+					"downscale of %s/%s in %s from %d to %d replicas is not allowed because parsing parsing downscale timestamps from the zone file failed.",


do we need a log line here and the returning of a warning? couldn't it be just returning the result, or an empty result and an error? then we only deal with logging once where ever it is we're calling prepareDownscale if the result is that we're denying the downscale

I think that's fair, generally I'm just trying to follow the pattern we've implemented elsewhere in this webhook. I don't think the extra logging has any real negative trade-offs as far as I can tell and this way depending on if you're looking at the operator directly or the admission responses you have visibility into the root cause of the failure.

cstyan · 2023-11-17T01:17:14Z

pkg/admission/prep_downscale.go

+		} else {
+			foundSts, err := findDownscalesDoneMinTimeAgo(stsList, ar.Request.Name)
+			if err != nil {
+				level.Warn(logger).Log("msg", "downscale not allowed due to error while parsing downscale annotations", "err", err)


pkg/admission/zone_tracker.go

MichelHollands · 2023-11-21T16:06:28Z

pkg/admission/prep_downscale.go

-		}
+		// If using zoneTracker instead of downscale annotations, check the zone file for recent downscaling.
+		// Otherwise, check the downscale annotations.
+		if zt != nil {


You could make 2 types (one with the zones file and one with the existing logic) here that implement the same interface. That way the code for them is not intermingled as here.

Good suggestion for a follow-up PR IMO.

pkg/admission/zone_tracker.go

MichelHollands · 2023-11-21T16:13:56Z

pkg/admission/prep_downscale.go

+				return deny(msg)
+			}
+		} else {
+			foundSts, err := findDownscalesDoneMinTimeAgo(stsList, ar.Request.Name)


Do we still need the old way? If the initial load of the zones adds the waiting time to the initial times won't that be enough to migrate?

The use of the two conditionals is to maintain compatibility for users of the prepare_downscale webhook that don't have issues with the kubernetes annotations for this metadata and don't want to use the new zoneTracker.

cstyan · 2023-11-21T23:14:26Z

Mentioned this on slack but posting here too to see if anyone else agrees; it might make sense to change the naming a bit? The name zoneTracker is a bit ambiguous in this case imo, and I think it doesn't really match what's actually happening right?

What we're actually tracking is more like the time at which a zone was last autoscaled in order to prevent downscaling too frequently.

My first though is that zoneTracker implies we're tracking the zones, like which zones are active for use or something like that. zoneScalingTracker, scalingEventTracker, downscaleTracker would all be less ambiguous imo, but I don't feel strongly enough to block you.

…storage; update zoneTracker tests; run `go mod vendor` & `go mod tidy` Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

…on; update tests; go mod vendor & go mod tidy Signed-off-by: JordanRushing <rushing.jordan@gmail.com> Update mockBucket instantiation in test; deny admission requests when failing to bootstrap the s3 bucket client Signed-off-by: JordanRushing <rushing.jordan@gmail.com> Clean up s3 config struct Signed-off-by: JordanRushing <rushing.jordan@gmail.com> Change s3 config creation semantics for zoneTracker Signed-off-by: JordanRushing <rushing.jordan@gmail.com> Add support for gcs and azure object storage clients; go mod vendor & go mod tidy Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

JordanRushing · 2023-12-06T22:18:24Z

Personally I'd look for a way to start breaking up prepareDownscale into some more functions. Especially with the amount of nested conditionals, it's getting a bit difficult to read. Maybe everything that happens in the block for if the zoneTracker file is being used can be a separate function?

@cstyan I think that's valuable feedback and I'd be happy to open a separate PR to simplify the webhook code and break things out into better functions.

MichelHollands · 2023-12-07T13:19:47Z

cmd/rollout-operator/main.go

@@ -87,6 +103,32 @@ func (cfg config) validate() error {
 	if (cfg.kubeAPIURL == "") != (cfg.kubeConfigFile == "") {
 		return errors.New("either configure both Kubernetes API URL and config file or none of them")
 	}
+	if cfg.useZoneTracker {


The code in this branch belongs better in zone_tracker.go

I don't know if I agree here, this validates the required config fields to launch with the zt following the pattern we use to validate the required kube config as well.

cmd/rollout-operator/main.go

pkg/admission/zone_tracker.go

pkg/admission/prep_downscale.go

pkg/admission/zone_tracker.go

jhalterman · 2023-12-12T01:46:52Z

Left a few comments we should consider, but this seems nice overall. Might be nice for @56quarters or @andyasp to weigh in on this as well.

56quarters · 2023-12-12T14:51:22Z

This PR introduces the idea of a zoneTracker using a JSON-encoded file stored in object storage to save timestamps for coordinating downscales between statefulsets with the prepare-downscale admission webhook.

Has any consideration been given to using a configmap in Kubernetes instead of introducing a dependency on object storage for the rollout-operator?

JordanRushing · 2023-12-13T17:30:43Z

This PR introduces the idea of a zoneTracker using a JSON-encoded file stored in object storage to save timestamps for coordinating downscales between statefulsets with the prepare-downscale admission webhook.

Has any consideration been given to using a configmap in Kubernetes instead of introducing a dependency on object storage for the rollout-operator?

Great question @56quarters! Yes, originally I was considering using a configmap after some deliberation with the team but ultimately, as far as I understand, etcd runs as a part of the managed control plane in EKS/AKS and the differences in the managed control planes between providers + poor visibility motivated this PR so I wanted to move this metadata outside of that.

I wonder if it might be better to move the annotation code path to using a configmap rather than the zoneTracker?

Do you have any concerns about introducing an object storage dependency to an optional code path here? The original code path is currently unchanged.

…ones; fix deadlock in lastDownscaled Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

…g; update comment on loadZones to indicate unique sts name assumption for keys

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

jhalterman · 2023-12-15T00:14:41Z

Talking to Jordan, he plans to followup with an attempt at accomplishing this via ConfigMap, which sounds good.

JordanRushing · 2023-12-15T18:54:58Z

Talking to Jordan, he plans to followup with an attempt at accomplishing this via ConfigMap, which sounds good.

I think we can close this PR in favor of #107!

JordanRushing added 3 commits November 14, 2023 11:24

Implement zoneTracker for coordinating downscales across statefulsets…

f5be68c

… using a file in object storage for the prepare_downscale admission webhook Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Merge branch 'main' into prep-downscale-last-downscale-objstore

7d1958f

Expand tests; fix deadlocks in zoneTracker; add logic to create an in…

60b37ec

…itial zone file if not present Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

cstyan reviewed Nov 17, 2023

View reviewed changes

MichelHollands reviewed Nov 21, 2023

View reviewed changes

JordanRushing added 5 commits November 28, 2023 15:36

Add zoneTracker support for s3; validate new config flags for object …

bd9ef2d

…storage; update zoneTracker tests; run `go mod vendor` & `go mod tidy` Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Fix s3 config env variable names

b11ec77

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Add endpoint to Azure bucket client config; update config validation

f2b53ef

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Add additional zoneTracker tests

93efd5d

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

JordanRushing marked this pull request as ready for review December 6, 2023 22:10

JordanRushing requested a review from a team as a code owner December 6, 2023 22:10

Remove redundant defer r.close() from loadZones

7bd2c6d

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

MichelHollands reviewed Dec 7, 2023

View reviewed changes

jhalterman reviewed Dec 12, 2023

View reviewed changes

cmd/rollout-operator/main.go Show resolved Hide resolved

jhalterman reviewed Dec 12, 2023

View reviewed changes

pkg/admission/zone_tracker.go Outdated Show resolved Hide resolved

jhalterman reviewed Dec 12, 2023

View reviewed changes

pkg/admission/zone_tracker.go Show resolved Hide resolved

jhalterman reviewed Dec 12, 2023

View reviewed changes

pkg/admission/zone_tracker.go Show resolved Hide resolved

jhalterman reviewed Dec 12, 2023

View reviewed changes

pkg/admission/prep_downscale.go Show resolved Hide resolved

jhalterman reviewed Dec 12, 2023

View reviewed changes

pkg/admission/zone_tracker.go Outdated Show resolved Hide resolved

JordanRushing added 4 commits December 13, 2023 11:40

Remove unused ctx from function definitions; update comment for saveZ…

6e4bf58

…ones; fix deadlock in lastDownscaled Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Merge branch 'main' into prep-downscale-last-downscale-objstore

0f40551

Resolve merge conflicts with main; run go mod tidy & go mod vendor

0b07920

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Convert zoneTracker zones map to use zoneInfo struct instead of strin…

c269119

…g; update comment on loadZones to indicate unique sts name assumption for keys

JordanRushing added 2 commits December 14, 2023 17:41

Clean up loadZones function

f62c470

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Fix setDownscaled to actually update the zoneInfo in the map

72c1cda

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

jhalterman approved these changes Dec 15, 2023

View reviewed changes

JordanRushing mentioned this pull request Dec 15, 2023

Implement zoneTracker for coordinating downscales across statefulsets using a ConfigMap for the prepare_downscale admission webhook #107

Merged

JordanRushing closed this Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement zoneTracker for coordinating downscales across statefulsets using a file in object storage for the prepare_downscale admission webhook #96

Implement zoneTracker for coordinating downscales across statefulsets using a file in object storage for the prepare_downscale admission webhook #96

JordanRushing commented Nov 14, 2023 •

edited

cstyan left a comment

cstyan Nov 17, 2023

JordanRushing Dec 6, 2023

cstyan Nov 17, 2023

JordanRushing Dec 13, 2023

cstyan Nov 17, 2023

MichelHollands Nov 21, 2023

JordanRushing Dec 6, 2023

MichelHollands Nov 21, 2023

JordanRushing Dec 6, 2023

cstyan commented Nov 21, 2023

JordanRushing commented Dec 6, 2023 •

edited

MichelHollands Dec 7, 2023

JordanRushing Dec 13, 2023

jhalterman commented Dec 12, 2023

56quarters commented Dec 12, 2023

JordanRushing commented Dec 13, 2023

jhalterman commented Dec 15, 2023

JordanRushing commented Dec 15, 2023

Implement zoneTracker for coordinating downscales across statefulsets using a file in object storage for the prepare_downscale admission webhook #96

Implement zoneTracker for coordinating downscales across statefulsets using a file in object storage for the prepare_downscale admission webhook #96

Conversation

JordanRushing commented Nov 14, 2023 • edited

TODO

cstyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstyan commented Nov 21, 2023

JordanRushing commented Dec 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhalterman commented Dec 12, 2023

56quarters commented Dec 12, 2023

JordanRushing commented Dec 13, 2023

jhalterman commented Dec 15, 2023

JordanRushing commented Dec 15, 2023

JordanRushing commented Nov 14, 2023 •

edited

JordanRushing commented Dec 6, 2023 •

edited