Skip to content
This repository was archived by the owner on May 16, 2023. It is now read-only.

add topologySpreadConstraints #1514

Closed

Conversation

jgregmac
Copy link

PR supports feature requested here: #1513

  • add topologySpreadConstraints to the logstash statefulSets
  • added supporting values to values.yaml with in-line doc link.
  • Added and ran templating test
  • Added value to the chart README.

- add topologySpreadConstraints to the logstash statefulSets
- added supporting values to values.yaml with in-line doc link.
- Added and ran templating test
- Added value to the chart README.
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Dec 20, 2021

💚 CLA has been signed

@botelastic
Copy link

botelastic bot commented Jun 1, 2022

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

@botelastic
Copy link

botelastic bot commented Jul 1, 2022

This PR has been automatically closed because it has not had recent activity since being marked as stale. Please reopen when work resumes.

@botelastic botelastic bot closed this Jul 1, 2022
@@ -115,6 +115,15 @@ spec:
podAffinity:
{{ toYaml . | indent 10 }}
{{- end }}
{{- if .Values.topologySpreadConstraints.enabled }}
topologySpreadConstraints:
- maxSkew: {{ .Values.topologySpreadConstraints.maxSkew }}

Choose a reason for hiding this comment

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

@jgregmac this should be a loop to allow for more than one constraint.

Copy link
Author

Choose a reason for hiding this comment

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

@cdenneen This recommendation has been implemented in my fork, although I do not see that change reflected in the "changed files" view of this PR. The recent merge from master is reflected in the commit history though, so I am puzzled by that. Perhaps I need to file a new PR?

Choose a reason for hiding this comment

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

https://github.com/elastic/helm-charts/blob/main/CONTRIBUTING.md#requirements-for-submiting-a-pull-request

Maybe new PR is necessary and looks like bumping Chart is necessary requirement too.

@cdenneen
Copy link

@jmlrt allowing issues and PRs to auto stale/close isn't really helpful.
Please re-open this PR and Issue #1513.

@jgregmac Possibly need to handle multiple like this:

spec:
  topologySpreadConstraints:
    - maxSkew: 1
      topologyKey: "topology.kubernetes.io/zone"
      whenUnsatisfiable: ScheduleAnyway
      labelSelector:
        matchLabels:
          dev: jjones
    - maxSkew: 1
      topologyKey: "kubernetes.io/hostname"
      whenUnsatisfiable: ScheduleAnyway
      labelSelector:
        matchLabels:
          dev: jjones
    - maxSkew: 1
      topologyKey: "karpenter.sh/capacity-type"
      whenUnsatisfiable: ScheduleAnyway
      labelSelector:
        matchLabels:
          dev: jjones

@cdenneen
Copy link

@jgregmac you have to sign the CLA before this can be reviewed by folks at @elastic

@jgregmac
Copy link
Author

jgregmac commented Oct 20, 2022 via email

@cdenneen
Copy link

@jgregmac please rebase

Copy link

@cdenneen cdenneen left a comment

Choose a reason for hiding this comment

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

LGTM

@mateuszadamkatana
Copy link

mateuszadamkatana commented Nov 17, 2022

@cdenneen @jgregmac

One year later.. 🎉
Small change, instead of if condition I use with

#1734

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants