Skip to content

Conversation

@cfiehe
Copy link

@cfiehe cfiehe commented Feb 19, 2025

This commit adds the possibility to specify multiple topology spread constraints. These constrains are combined using a logical AND operation and control pod distribution across a cluster.

Signed-off-by: Christoph Fiehe fiehe@gmx.de

@CLAassistant
Copy link

CLAassistant commented Feb 19, 2025

CLA assistant check
All committers have signed the CLA.

@cfiehe cfiehe force-pushed the make_topology_spread_more_flexible branch 2 times, most recently from 8300b48 to 0083314 Compare February 19, 2025 12:28
maxSkew: {{ .Values.topologySpreadConstraints.maxSkew }}
topologyKey: {{ .Values.topologySpreadConstraints.topologyKey }}
whenUnsatisfiable: {{ .Values.topologySpreadConstraints.whenUnsatisfiable }}
{{- toYaml . | nindent 8 }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit problematic because it removes the label selectors.

Honestly, I'm not sure how other charts solve this out there.

Would it make sense to merge the list with the label selectors?

Copy link
Author

@cfiehe cfiehe Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we can do - maybe it is not the nicest solution - is to change the code part into:

      {{- with .Values.topologySpreadConstraints }}
      topologySpreadConstraints:
      {{- range . }}
      - {{ . | toYaml | indent 8 | trim }}
        labelSelector:
          matchLabels:
            {{- include "sftpgo.selectorLabels" $ | nindent 12 }}
      {{- end }}
      {{- end }}

Someone can drop the labelSelector in his override and may specify something like:

topologySpreadConstraints:
  - maxSkew: 1
    topologyKey: kubernetes.io/hostname
    whenUnsatisfiable: DoNotSchedule
    matchLabelKeys:
      - pod-template-hash

The result is the following:

      topologySpreadConstraints:
      - matchLabelKeys:
        - pod-template-hash
        maxSkew: 1
        topologyKey: kubernetes.io/hostname
        whenUnsatisfiable: DoNotSchedule
        labelSelector:
          matchLabels:
            app.kubernetes.io/name: sftpgo
            app.kubernetes.io/instance: sftpgo

It is the same approach that is used here.

What do you think?

Copy link
Author

@cfiehe cfiehe Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved version with merge:

      {{- with .Values.topologySpreadConstraints }}
      topologySpreadConstraints:
      {{- range $constraint := . }}
      {{- if not $constraint.labelSelector }}
      {{- $selectorLabels := fromYaml (include "sftpgo.selectorLabels" $)}}
      {{- $constraint = merge $constraint (dict "labelSelector" (dict "matchLabels" $selectorLabels)) }}
      {{- end }}
      - {{ $constraint | toYaml | indent 8 | trim }}
      {{- end }}
      {{- end }}

This version adds the default, when no labelSelector has been specified - which should be the normal case.

@cfiehe cfiehe force-pushed the make_topology_spread_more_flexible branch 5 times, most recently from 4ccc230 to 22187b4 Compare February 20, 2025 08:59
…s the possibility to specify multiple topology spread constraints. These constrains are combined using a logical AND operation and control pod distribution across a cluster.

Signed-off-by: Christoph Fiehe <fiehe@gmx.de>
@cfiehe cfiehe force-pushed the make_topology_spread_more_flexible branch from 22187b4 to 1702419 Compare December 11, 2025 11:12
@cfiehe
Copy link
Author

cfiehe commented Dec 11, 2025

@sagikazarmark @drakkan
Are there any chances to get this enhancement merged?

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.

3 participants