Skip to content

Conversation

@DanielDorado
Copy link
Contributor

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

Allow a user to  override the default TopologySpreadConstraints. To remove the default constraints without adding new ones,
set this to an empty array. To append to the default constraint is not supported, the user should put it explicitly in the override.
In a rare case, when appending the default is needed, then, to put it explicitly in the override is not a big deal.
@Zerpet
Copy link
Member

Zerpet commented Aug 5, 2024

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
Copy link
Contributor

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
Copy link
Member

Zerpet commented Aug 5, 2024

I think the API server may not differentiate between empty array and nil, once encoded by the server: kubernetes/kubernetes#70281

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

@DanielDorado
Copy link
Contributor Author

DanielDorado commented Aug 5, 2024

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
Copy link
Contributor Author

Closed on behalf of #1694

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support override default topologySpreadContraints

3 participants