rabbitmq / cluster-operator

RabbitMQ Cluster Kubernetes Operator
https://www.rabbitmq.com/kubernetes/operator/operator-overview.html
Mozilla Public License 2.0
883 stars 273 forks source link

Support override default topologySpreadContraints #1690

Closed DanielDorado closed 3 months ago

DanielDorado commented 3 months ago

Allow a user to override the default TopologySpreadConstraints. Appending to the default constraint is not supported, so the user should put it explicitly in the override. In a rare case, when appending the default is needed, putting it explicitly in the override is not a big deal.

This closes #1687

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

Additional Context

Local Testing

Please ensure you run the unit, integration and system tests before approving the PR.

To run the unit and integration tests:

$ make unit-tests integration-tests

You will need to target a k8s cluster and have the operator deployed for running the system tests.

For example, for a Kubernetes context named dev-bunny:

$ kubectx dev-bunny
$ make destroy deploy-dev
# wait for operator to be deployed
$ make system-tests
Zerpet commented 3 months ago

Our CI is having a rework at the moment. I will verify that tests pass locally before approving. The code changes look correct to me.

mkuratczyk commented 3 months ago

It doesn't seem to address the main issue. Am I missing something?


> cat /tmp/a.yml
apiVersion: rabbitmq.com/v1beta1
kind: RabbitmqCluster
metadata:
  name: a
spec:
  override:
    statefulSet:
      spec:
        template:
          spec:
            containers: []
            topologySpreadConstraints: []

> kubectl apply -f /tmp/a.yml
rabbitmqcluster.rabbitmq.com/a created

> kubectl get -o json sts a-server | jq .spec.template.spec.topologySpreadConstraints
[
  {
    "labelSelector": {
      "matchLabels": {
        "app.kubernetes.io/name": "a"
      }
    },
    "maxSkew": 1,
    "topologyKey": "topology.kubernetes.io/zone",
    "whenUnsatisfiable": "ScheduleAnyway"
  }
]```
Zerpet commented 3 months ago

I think the API server may not differentiate between empty array and nil, once encoded by the server: https://github.com/kubernetes/kubernetes/issues/70281

In which case, this PR won't address the issue 😢

DanielDorado commented 3 months ago

Yes, both of you are right.

It does not allow putting an empty topologySpreadConstraints. Still, it will enable us to overwrite it with a topology spread constraint referencing a node label that exists in our Kubernetes nodes.

As this behavior is a (little) better than not overriding at all, I put the PR anyway.

Our problem is solved, and anybody who needs to overwrite the topologySpreadConstraints because of lack of the "topology.inditex.io/zone" label or because they do not want to use it will be satisfied.

But, anyway, if you want to avoid accepting this solution, I will do a PR with a better solution.

DanielDorado commented 3 months ago

Closed on behalf of #1694