We don't expect a single rollout-operator to update multiple webhooks, especially since we'll be looking for one with grafana.com/namespace label.

So, instead of listing them, we can specify just one name, and restrict the ClusterRole to that specific webhook name.

Reproduction steps:

Install mimir from grafana/helm-charts#1205 , enable for example store-gateway zone aware replication , i.e. via custome values.yaml:

rollout_operator:
  enabled: true
store_gateway:
  zone_aware_replication:
    enabled: true

After installation, write a letter into the mimir.config , just to alter its checksum.

Expected (works without rollout op): store-gateway Pods are restarted to take in the new configuration.

Actual: nothing happens, Pods are not restarted.

Additional info:
Rollout operator prints reconciled store-gateway statefulsets messages.

Before change to config, the statefullset state is:

apiVersion: apps/v1
kind: StatefulSet
metadata:
  annotations:
    checksum/config: 7fc741ee52baf2c3d69f77aed2cda62113c36f9c878b86742f5f02a4cfd1427a
    meta.helm.sh/release-name: krajo
    meta.helm.sh/release-namespace: dev
    rollout-max-unavailable: "10"
  creationTimestamp: "2022-04-28T16:16:58Z"
  generation: 2
  labels:
    app.kubernetes.io/component: store-gateway
    app.kubernetes.io/instance: krajo
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: mimir
    app.kubernetes.io/part-of: memberlist
    app.kubernetes.io/version: 2.0.0
    helm.sh/chart: mimir-distributed-2.0.9
    rollout-group: store-gateway
    zone: zone-a
  name: krajo-mimir-store-gateway-zone-a
  namespace: dev
  resourceVersion: "2897289"
  selfLink: /apis/apps/v1/namespaces/dev/statefulsets/krajo-mimir-store-gateway-zone-a
  uid: 88d76d77-f8e2-4023-8775-b460b078d4a2
spec:
  podManagementPolicy: OrderedReady
  replicas: 1
  revisionHistoryLimit: 10
  selector:
    matchLabels:
      app.kubernetes.io/component: store-gateway
      app.kubernetes.io/instance: krajo
      app.kubernetes.io/name: mimir
      rollout-group: store-gateway
      zone: zone-a
  serviceName: krajo-mimir-store-gateway-headless
  template:
    metadata:
      annotations:
        checksum/config: 7fc741ee52baf2c3d69f77aed2cda62113c36f9c878b86742f5f02a4cfd1427a
      creationTimestamp: null
      labels:
        app.kubernetes.io/component: store-gateway
        app.kubernetes.io/instance: krajo
        app.kubernetes.io/managed-by: Helm
        app.kubernetes.io/name: mimir
        app.kubernetes.io/part-of: memberlist
        app.kubernetes.io/version: 2.0.0
        helm.sh/chart: mimir-distributed-2.0.9
        rollout-group: store-gateway
        zone: zone-a
    spec:
      affinity:
        podAntiAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
          - labelSelector:
              matchExpressions:
              - key: target
                operator: In
                values:
                - store-gateway
              - key: target
                operator: NotIn
                values:
                - store-gateway-zone-a
            topologyKey: kubernetes.io/hostname
      containers:
      - args:
        - -target=store-gateway
        - -config.file=/etc/mimir/mimir.yaml
        - -store-gateway.sharding-ring.instance-availability-zone=zone-a
        image: grafana/mimir:2.0.0
        imagePullPolicy: IfNotPresent
        name: store-gateway
        ports:
        - containerPort: 8080
          name: http-metrics
          protocol: TCP
        - containerPort: 9095
          name: grpc
          protocol: TCP
        readinessProbe:
          failureThreshold: 3
          httpGet:
            path: /ready
            port: http-metrics
            scheme: HTTP
          initialDelaySeconds: 60
          periodSeconds: 10
          successThreshold: 1
          timeoutSeconds: 1
        resources:
          requests:
            cpu: 100m
            memory: 512Mi
        securityContext:
          readOnlyRootFilesystem: true
        terminationMessagePath: /dev/termination-log
        terminationMessagePolicy: File
        volumeMounts:
        - mountPath: /etc/mimir
          name: config
        - mountPath: /var/mimir
          name: runtime-config
        - mountPath: /data
          name: storage
      dnsPolicy: ClusterFirst
      restartPolicy: Always
      schedulerName: default-scheduler
      securityContext: {}
      serviceAccount: krajo-mimir
      serviceAccountName: krajo-mimir
      terminationGracePeriodSeconds: 240
      volumes:
      - name: config
        secret:
          defaultMode: 420
          secretName: krajo-mimir-config
      - configMap:
          defaultMode: 420
          name: krajo-mimir-runtime
        name: runtime-config
  updateStrategy:
    type: OnDelete
  volumeClaimTemplates:
  - apiVersion: v1
    kind: PersistentVolumeClaim
    metadata:
      creationTimestamp: null
      name: storage
    spec:
      accessModes:
      - ReadWriteOnce
      resources:
        requests:
          storage: 5Gi
      storageClassName: microk8s-hostpath
      volumeMode: Filesystem
    status:
      phase: Pending
status:
  availableReplicas: 1
  collisionCount: 0
  currentRevision: krajo-mimir-store-gateway-zone-a-6795c75577
  observedGeneration: 2
  readyReplicas: 1
  replicas: 1
  updateRevision: krajo-mimir-store-gateway-zone-a-6795c75577

After the upgrade:

apiVersion: apps/v1
kind: StatefulSet
metadata:
  annotations:
    checksum/config: 58576abe6a9fb051e078095d03be1c4c3906e36da1651cf7fdf6cd8c39c30171
    meta.helm.sh/release-name: krajo
    meta.helm.sh/release-namespace: dev
    rollout-max-unavailable: "10"
  creationTimestamp: "2022-04-28T16:16:58Z"
  generation: 3
  labels:
    app.kubernetes.io/component: store-gateway
    app.kubernetes.io/instance: krajo
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: mimir
    app.kubernetes.io/part-of: memberlist
    app.kubernetes.io/version: 2.0.0
    helm.sh/chart: mimir-distributed-2.0.9
    rollout-group: store-gateway
    zone: zone-a
  name: krajo-mimir-store-gateway-zone-a
  namespace: dev
  resourceVersion: "2902316"
  selfLink: /apis/apps/v1/namespaces/dev/statefulsets/krajo-mimir-store-gateway-zone-a
  uid: 88d76d77-f8e2-4023-8775-b460b078d4a2
spec:
  podManagementPolicy: OrderedReady
  replicas: 1
  revisionHistoryLimit: 10
  selector:
    matchLabels:
      app.kubernetes.io/component: store-gateway
      app.kubernetes.io/instance: krajo
      app.kubernetes.io/name: mimir
      rollout-group: store-gateway
      zone: zone-a
  serviceName: krajo-mimir-store-gateway-headless
  template:
    metadata:
      annotations:
        checksum/config: 58576abe6a9fb051e078095d03be1c4c3906e36da1651cf7fdf6cd8c39c30171
      creationTimestamp: null
      labels:
        app.kubernetes.io/component: store-gateway
        app.kubernetes.io/instance: krajo
        app.kubernetes.io/managed-by: Helm
        app.kubernetes.io/name: mimir
        app.kubernetes.io/part-of: memberlist
        app.kubernetes.io/version: 2.0.0
        helm.sh/chart: mimir-distributed-2.0.9
        rollout-group: store-gateway
        zone: zone-a
    spec:
      affinity:
        podAntiAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
          - labelSelector:
              matchExpressions:
              - key: target
                operator: In
                values:
                - store-gateway
              - key: target
                operator: NotIn
                values:
                - store-gateway-zone-a
            topologyKey: kubernetes.io/hostname
      containers:
      - args:
        - -target=store-gateway
        - -config.file=/etc/mimir/mimir.yaml
        - -store-gateway.sharding-ring.instance-availability-zone=zone-a
        image: grafana/mimir:2.0.0
        imagePullPolicy: IfNotPresent
        name: store-gateway
        ports:
        - containerPort: 8080
          name: http-metrics
          protocol: TCP
        - containerPort: 9095
          name: grpc
          protocol: TCP
        readinessProbe:
          failureThreshold: 3
          httpGet:
            path: /ready
            port: http-metrics
            scheme: HTTP
          initialDelaySeconds: 60
          periodSeconds: 10
          successThreshold: 1
          timeoutSeconds: 1
        resources:
          requests:
            cpu: 100m
            memory: 512Mi
        securityContext:
          readOnlyRootFilesystem: true
        terminationMessagePath: /dev/termination-log
        terminationMessagePolicy: File
        volumeMounts:
        - mountPath: /etc/mimir
          name: config
        - mountPath: /var/mimir
          name: runtime-config
        - mountPath: /data
          name: storage
      dnsPolicy: ClusterFirst
      restartPolicy: Always
      schedulerName: default-scheduler
      securityContext: {}
      serviceAccount: krajo-mimir
      serviceAccountName: krajo-mimir
      terminationGracePeriodSeconds: 240
      volumes:
      - name: config
        secret:
          defaultMode: 420
          secretName: krajo-mimir-config
      - configMap:
          defaultMode: 420
          name: krajo-mimir-runtime
        name: runtime-config
  updateStrategy:
    type: OnDelete
  volumeClaimTemplates:
  - apiVersion: v1
    kind: PersistentVolumeClaim
    metadata:
      creationTimestamp: null
      name: storage
    spec:
      accessModes:
      - ReadWriteOnce
      resources:
        requests:
          storage: 5Gi
      storageClassName: microk8s-hostpath
      volumeMode: Filesystem
    status:
      phase: Pending
status:
  availableReplicas: 1
  collisionCount: 0
  currentRevision: krajo-mimir-store-gateway-zone-a-6b64ccdc98
  observedGeneration: 3
  readyReplicas: 1
  replicas: 1
  updateRevision: krajo-mimir-store-gateway-zone-a-6b64ccdc98

I've added the checksum on statefulset itself as annotation but didn't help.

Import ordering is not currently enforced which is causing noise. This repository should introduce formatting and linting for import ordering like the Mimir repository does.

If a pod is in the process of being created for a statefulset, the downscale webhook will reject an attempt to change a resource that would cause pods to downscale:

level=error ts=2023-07-18T01:23:33.406992007Z name=ingester-zone-a resource=statefulsets namespace=mimir-dev-11 request_gvk="apps/v1, Kind=StatefulSet" old_replicas=225 new_replicas=5 msg="downscale not allowed due to error" err="Post "http://ingester-zone-a-218.ingester-zone-a.mimir-dev-11.svc.cluster.local:80/ingester/prepare-shutdown": dial tcp: lookup ingester-zone-a-218.ingester-zone-a.mimir-dev-11.svc.cluster.local on 10.188.0.10:53: no such host"

This was discovered when an HPA was scaling up too aggressively, and when trying to revert the change that caused that, the downscale webhook rejected the change since the statefulset was currently upscaling.

Context

There are some resources in Mimir that can't be downscaled directly: the ingesters. While we're developing an automated way to perform the downscaling operation (that involves calling and endpoint on the resources and waiting a certain amount of time), we would like to find a way that would prevent them from being accidentally downscaled (manual operations on the cluster, gitops revert, etc.)

It would also be great to find a way that would be extensible to the automated solution in the future.

Challenges

One of the challenges to have in mind is to distinguish whether a resource is being deleted because it's being re-scheduled or updated (a rolling release, for instance) or it's being downscaled.

Solution alternatives

Using kubernetes finalizers

A finalizer can be set on a kubernetes resource to prevent it from being deleted (until the finalizer is removed). We considered using this mechanism and allow only rollouts and re-schedulings. We would prevent the pods from being deleted if a downscale is detected, triggering an alert metric and requiring an operator intervention.

However, we had to discard this approach because even though Kubernetes doesn't delete the pod with a finalizer, it does stop the containers on the deletion request.

Use a preStop hook and a huge terminationGracePeriodSeconds

Another alternative would be to use a preStop hook on the ingesters, that would detect a downscale operation and a huge terminationGracePeriodSeconds period that would be used to avoid the downscaling and alerting the operator about that fact.

This approach has some downsides:

• ingesters would need to know about their surroundings: either they would talk directly to k8s api or they would need to talk to the Kubernetes API. Both solutions don't seem to be great.
• misuse of terminationGracePeriodSeconds could create different issues.
• in case of downscale, the operator would need to manually intervent the pod (as it would be already in process of deletion)

Using a dynamic admission controller

Kubernetes can be configured to call a webhook whenever a change is applied, and reject it if it doesn't meet certain requirements. We think that this solution fits the best our requirements.

We would add a special label to the resources that we don't want to be downscaled, let's say grafana.com/no-downscale and create a webhook configuration that would call a rollout-operator's endpoint, like this:

kind: ValidatingWebhookConfiguration
metadata:
  name: no-downscale-default
webhooks:
- admissionReviewVersions:
  - v1
  clientConfig:
    service:
      name: rollout-operator
      namespace: default
      path: /admission/no-downscale
      port: 443
  failurePolicy: Fail
  name: no-downscale.default.namespace.local
  objectSelector:
    matchLabels:
      grafana.com/namespace: default
      grafana.com/no-downscale: "true"
  rules:
  - apiGroups:
    - apps
    apiVersions:
    - v1
    operations:
    - UPDATE
    resources:
    - statefulsets
    - deployments
    - replicasets
    scope: Namespaced
  sideEffects: None

Since webhooks are not namespaced, but we probably don't want to handle the entire cluster with just one rollout-operator, I'm proposing here to add a grafana.com/namespace label to the objects that we want to validate, and match them in the objectSelector. Unfortunately, even though there is a namespaceSelector property for webhooks, it doesn't seem to be able to match the namespace's name, only its labels, and it seems more reasonable to ask someone to add a label on the resources they want to validate rather than on the entire namespace. In any case this decision is only up to the webhook configuration and wouldn't change the webhook implementation in the rollout-operator.

There would be an /admission/no-downscale endpoint on the rollout-operator that would validate the admission requests from Kubernetes on the matched updates.

So far so good, but I found several challenges for this implementation.

Challenge: webhooks have to be served over https

Webhooks need to be served over https, so we need either to configure some certificates on the rollout-operator or to generate some self-signed ones and set them on the webhook configurations. Both approaches are non-trivial.

Configuring certificates on the rollout-operator

There are plenty of examples on the internet on how to generate certificates and provision them in a webhook server, either using a set of manual operations involving a Certificate Signing Requests from Kuberentes, or using something like cert-manager, to generate it and automatically inject it to both rollout-operator deployment and the webhook configuration.

Even though it's possible, I think it's too much hassle for someone deploying the rollout operator, and while this approach should be supported, there should be also an easier self-configurable way.

Self-signed certificates generation

It's possible to generate self-signed certificates for the webhook, configuring the CA Bundle on the webhook.

This presents two further challenges:

Persisting the self-signed certificate upon restarts

Given we have a webhook configured with a self-signed certificate, we should reuse the same certificate for the webhook server (served by our rollout-operator) after a restart. The solution that we can use for this is a Kubernetes secret: letting rollout-operator a RBAC authorization to read, create and update a secret, it can check whether a secret already exists, and use that certificate, or create one and persist it. The implementation of this is fairly simple.

There's a downside of this which is the fact that the secret would stay in Kuberentes if the rollout-operator is removed, since rollout-operator doesn't have enough self-awareness to set the proper ownerReference on that secret. This is something we think we can live with.

Using the self-signed certificate on the webhook configuration

Once we have the self-signed certificate, we could just create the webhook in our Kubernetes cluster from the rollout operator itself. This would however give rollout-operator the ability of creating cluster-wide webhooks, which seems dangerous, and it would also have the downside mentioned before of leaving the webhook dangling whenever the webhook is removed.

We decided to go another way, and inspired by the way cert-manager injects signed certificates on certain objects, we could have a special label on the webhooks configuration that need a self-signed certificate to be injected:

metadata:
  labels:
    grafana.com/inject-rollout-operator-ca: "true"
    grafana.com/namespace: default

This seems to be less intrusive and more compatible with alternatives like using cert-manager.

Challenge: matching scale subresource changes

After impementing a working solution, in the manual testing phase I realized that when the scale subresource is being modified (for example, by doing kubectl scale ...) the webhook does not match the resource, as the Scale subresouce doesn't have any labels at all. I have created an issue on the Kubernetes project for this.

(Note also that matching the scale subresouces requires declaring them as ["statefulsets/scale", "deployments/scale", "replicasets/scale"] in the resouces section of the webhook rule).

A possible workaround would be to match all scale operations in the namespace, retrieving the parent resource and manually matching its labels. In order to match all scale operations in the namespace, we would need to make a label on the namespace itself and use namespaceSelector, instead of the custom grafana.com/namespace approach mentioned above, since again, Scale sub-resource doesn't have any labels.

Questions

• Can we do that?
  • Yes, it doesn't seem very complicated.
• Should we do that?
  • It seems that this should be a responsibility of Kubernetes itself.
• Will Kubernetes do this change?
  • Maybe. Or maybe not.
• Should we wait until Kubernetes does this?
  • Probably not, because even if Kubernetes would do the change, it would require rollout-operator to be ran on a very new Kubernetes version, which is a big requirement.

Implementation

I have a working code in #25 but it's still pending some tests and solving the scale issue.

I haven't been able to get CI to pass on the first try with this project yet. It's nearly always an issue with the integration test for ensuring automatically renewing the self-signed TLS cert works. In order to improve the reliability and our confidence in rollout-operator tests, the test and code under test should be reworked so it can be tested without spinning up an entire Kubernetes cluster.

At present the rollout operator prevents concurrent rollout operations, but doesn’t prevent concurrent downscale + rollout operations. This can happen, for example, if the controller is currently rolling zone B or C, and then something downscales zone A.

In order to guard against this, I propose using an annotation on the resource being scaled to represent a “lock” across the scaling group. Ex:

grafana.com/scaling-lock: rollout

Creating a scaling-lock

When the rollout operator reconciles, it will choose a resource, if any, that already has a scaling lock to reconcile first, allowing reentrancy. If none exist, it will add a scaling-lock annotation to the sts when it begins rolling its pods. A scaling lock should not be needed though if the controller is just adjusting replicas up/down (since last-downscaled will already provide protection here), only when rolling pods.

Validating the scaling-lock

When the scaling-lock is present on any resource in the rollout-group, the downscale webhook will reject downscale attempts for resources that don't have the scaling lock. For example: if the scaling lock is present on the zone B statefulset, zone A's statefulset cannot be downscaled.

Removing the scaling-lock

When the rollout-operator reconciles a resource, if it sees no non-ready pods and has nothing to do for a resource, it will clear the scaling-lock for that resource if any is present.

Other alternatives

While this could also be accomplished by extending the last-updated annotation to be updated by the rollout-operator’s controller, that would block downscales from taking place for some time after rollouts are complete, which is not necessary. We just want to prevent concurrent scaling from occurring.

While #79 described a situation that could happen commonly, there are some less likely but still possible race conditions that we might consider guarding against.

Concurrent downscale + rollout

If a downscale is in progress but hasn't updated the last-downscale annotation yet, it's possible that a rollout could start concurrently, bypassing the min-time-between-zones-downscale check in the controller.

Concurrent downscale + downscale

If downscales for two zones occur concurrently, they can both be accepted before either has updated the last-downscale annotation, bypassing the min-time-between-zones-downscale check in the prepare-downscale webhook.

Potential fix

One reliable way to serialize operations across different statefulsets is to lock on a common resource, such as with an annotation on a common CRD, using a resourceVersion on updates. Absent that, we could add a lock annotation to a statefulset and use a double check (check, lock, check) when adding it to guard against races, though I'm not positive this is completely safe.

We're currently checking hasStatefulSetNotReadyPods before considering the zone reconciled:

However, we've seen a case where all pods of a statefulset were deleted, but no new pods were created yet. This caused the listNotReadyPodsByStatefulSet method return no pods at all, as there were no pods in any state.

This caused an outage, as next zone was terminated immediately, before no pods were available in the first one.

I think that the code should check that there are at least as many pods as replicas desired by the statefulset, and then it should check whether some of them are unready.

The rollout-operator currently requires that each pod controlled by a StatefulSet managed by rollout-operator has the name label and its value is the name of the StatefulSet controlling it:

I think we can get rid of this requirement, looking for the StatefulSet controlling the pod in a different way. For example, when running kubectl describe pod ... it shows a "Controlled By" field with the name of the StatefulSet controlling it.

Current rollout image does not have an arm64 compatible image

standard_init_linux.go:228: exec user process caused: exec format error

I just moved the build to the dockerfile using the builder pattern and use the builtin support for multi arch from docker buidlx

FROM --platform=$BUILDPLATFORM golang:1.17-alpine AS builder

ADD . /app
WORKDIR /app

ARG TARGETOS
ARG TARGETARCH

RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} \
    go build -ldflags="-s -w" ./cmd/rollout-operator

FROM alpine:3.14

COPY --from=builder /app/rollout-operator /bin/rollout-operator
ENTRYPOINT [ "/bin/rollout-operator" ]

# Create rollout-operator user to run as non-root.
RUN addgroup -g 10000 -S rollout-operator && \
    adduser  -u 10000 -S rollout-operator -G rollout-operator
USER rollout-operator:rollout-operator

ARG revision
LABEL org.opencontainers.image.title="rollout-operator" \
      org.opencontainers.image.source="https://github.com/grafana/rollout-operator" \
      org.opencontainers.image.revision="${revision}"

And to build just specify the architectures (Note arm builds require push ~ images aren't stored locally)

docker buildx build --platform linux/amd64,linux/arm64 --tag your-registry.tld/repo/grafana/rollout-operator:latest --push .

Some flaky tests were previously removed:

• cabaac5
• e038f92

We should consider fixing these so that they're not flaky and restoring them.

It's a form of configuration for the statefulsets and doesn't ever need to be used for selecting them. We should release a version of the rollout-operator that supports either a label or annotation as a migration path before only supporting use of an annotation.

The google.golang.org/protobuf version needs updated to 1.33 to fix the below CVE.

CVE: GHSA-8r3f-844c-mc37

We are trying to scale down ingesters using the MutatingAdmissionWebhook as described here.

The required labels and annotations were added to the objects as shown below:

# labels
grafana.com/prepare-downscale=true

# annotations
grafana.com/prepare-downscale-http-path=ingester/prepare-shutdown
grafana.com/prepare-downscale-http-port=8080

When trying the scale down the statefulset/mimir-ingester-zone-a, the operator failing to resolve the pod when sending HTTP post request, as the fqdn is constructed as <pod_name>.<service_name>.<namespace>.svc.cluster.local.

mimir-ingester-zone-a-1.mimir-ingester-zone-a.mimir.svc.cluster.local

These are the existing services:

mimir-ingester-headless                  ClusterIP   None             <none>        8080/TCP,9095/TCP   19d
mimir-ingester-zone-a                    ClusterIP   <ip_address>     <none>        8080/TCP,9095/TCP   19d
mimir-ingester-zone-b                    ClusterIP   <ip_address>    <none>        8080/TCP,9095/TCP   19d
mimir-ingester-zone-c                    ClusterIP   <ip_address>    <none>        8080/TCP,9095/TCP   19d

In order to resolve the pod, the headless service should be used instead (mimir-ingester-headless). More info

Operator logs

level=error ts=2024-01-16T13:29:51.838387358Z name=mimir-ingester-zone-a resource=statefulsets namespace=mimir request_gvk="autoscaling/v1, Kind=Scale" old_replicas=2 new_replicas=1 url=mimir-ingester-zone-a-1.mimir-ingester-zone-a.mimir.svc.cluster.local:443/ingester/prepare-shutdown index=1 msg="error sending HTTP post request" err="Post \"http://mimir-ingester-zone-a-1.mimir-ingester-zone-a.mimir.svc.cluster.local:443/ingester/prepare-shutdown\": dial tcp: lookup mimir-ingester-zone-a-1.mimir-ingester-zone-a.mimir.svc.cluster.local on <name_server_ip>:53: no such host"

It'd be great if the release process was automated, so that images are built in a repeatable way in a controlled environment.

I'm imagining a GitHub Actions workflow that would respond to a release being created, and automatically build and push the images to Docker Hub.