Chart: Add `topologySpreadConstraints` for default backend. #11197

msfidelis · 2024-04-04T10:10:19Z

What this PR does / why we need it:

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
CVE Report (Scanner found CVE and adding report)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

fixes #11194

How Has This Been Tested?

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added unit and/or e2e tests to cover my changes.
All new and existing tests passed.

k8s-ci-robot · 2024-04-04T10:10:29Z

Hi @msfidelis. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

netlify · 2024-04-04T10:10:37Z

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	`adeb875`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/660fd3ac4dfa6500089d5a90

Gacko · 2024-04-04T10:41:55Z

/assign

Gacko · 2024-04-04T10:42:11Z

/kind feature
/priority backlog

Gacko · 2024-04-04T10:46:52Z

/retitle Chart: Add topologySpreadConstraints for default backend.

Gacko · 2024-04-04T10:47:01Z

/triage accepted

Gacko

I left a comment about where and how to add the new value. Also it would be nice to at least have some unit tests for that. I know, it does not exist for the controller either, but maybe you can add some for the both of them.

charts/ingress-nginx/values.yaml

Gacko · 2024-04-04T14:01:57Z

/label tide/merge-method-squash

Gacko · 2024-04-04T14:35:07Z

You forgot to re-generate the documentation. Also please move the values to the suggested place in the values.yaml. Thank you!

charts/ingress-nginx/values.yaml

Gacko

See my suggestion. I thought I already suggested that earlier. And still, please move the new value right below the affinity section. Thank you!

Gacko · 2024-04-05T09:38:21Z

charts/ingress-nginx/values.yaml

@@ -1075,6 +1075,26 @@ defaultBackend:
  priorityClassName: ""
  # -- Labels to be added to the default backend resources
  labels: {}
+  # -- Topology spread constraints rely on node labels to identify the topology domain(s) that each Node is in.
+  ## Ref: https://kubernetes.io/docs/concepts/workloads/pods/pod-topology-spread-constraints/


Suggested change

## Ref: https://kubernetes.io/docs/concepts/workloads/pods/pod-topology-spread-constraints/

# Ref.: https://kubernetes.io/docs/concepts/workloads/pods/pod-topology-spread-constraints/

Gacko

Added some last suggestions. We are close to merging. ;)

Gacko · 2024-04-05T10:08:50Z

charts/ingress-nginx/values.yaml

@@ -998,6 +997,26 @@ defaultBackend:
  #    effect: "NoSchedule|PreferNoSchedule|NoExecute(1.6 only)"

  affinity: {}
+  # -- Topology spread constraints rely on node labels to identify the topology domain(s) that each Node is in.
+  # Ref.: https://kubernetes.io/docs/concepts/workloads/pods/pod-topology-spread-constraints/
+  #  


You can safely omit this line. The way we are currently doing it for controller.topologySpreadConstraints isn't perfect, so don't relate to that.

Gacko · 2024-04-05T10:09:02Z

charts/ingress-nginx/values.yaml

@@ -998,6 +997,26 @@ defaultBackend:
  #    effect: "NoSchedule|PreferNoSchedule|NoExecute(1.6 only)"

  affinity: {}
+  # -- Topology spread constraints rely on node labels to identify the topology domain(s) that each Node is in.
+  # Ref.: https://kubernetes.io/docs/concepts/workloads/pods/pod-topology-spread-constraints/
+  #  


Suggested change

#

Gacko · 2024-04-05T10:09:15Z

charts/ingress-nginx/values.yaml

+  #       app.kubernetes.io/component: default-backend
+  #   topologyKey: kubernetes.io/hostname
+  #   maxSkew: 1
+  #   whenUnsatisfiable: ScheduleAnyway  


Suggested change

# whenUnsatisfiable: ScheduleAnyway

# whenUnsatisfiable: ScheduleAnyway

Gacko · 2024-04-05T10:09:27Z

charts/ingress-nginx/values.yaml

@@ -308,7 +308,6 @@ controller:
  #   topologyKey: kubernetes.io/hostname
  #   maxSkew: 1
  #   whenUnsatisfiable: ScheduleAnyway
-


Please keep that line.

Gacko · 2024-04-05T10:10:13Z

Also, I will take care about the unit tests later. So no worries about that.

feature(default_backend): topologySpread support feature(default_backend): topologySpread support feature(default_backend): helm-docs feature(default_backend): helm-docs feature(default_backend): helm-docs feature(default_backend): helm-docs feature(default_backend): nit feature(default_backend): nit feature(default_backend): nit

Gacko · 2024-04-05T10:34:16Z

charts/ingress-nginx/values.yaml

@@ -308,7 +308,7 @@ controller:
  #   topologyKey: kubernetes.io/hostname
  #   maxSkew: 1
  #   whenUnsatisfiable: ScheduleAnyway
-
+  


Thanks for adding it back :) Seems you also added some trailing whitespaces, CI doesn't like them.

Sorry for the troubles 😢

Gacko · 2024-04-05T10:44:29Z

/ok-to-test
/lgtm

k8s-ci-robot · 2024-04-05T10:44:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gacko, msfidelis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~charts/ingress-nginx/OWNERS~~ [Gacko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Gacko · 2024-04-05T10:45:03Z

Thanks a lot (again) for your contribution!

k8s-ci-robot requested review from cpanato and Gacko April 4, 2024 10:10

k8s-ci-robot added needs-priority size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 4, 2024

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 4, 2024

k8s-ci-robot assigned Gacko Apr 4, 2024

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority labels Apr 4, 2024

k8s-ci-robot changed the title ~~DefaultBackend: topologySpreadConstraints~~ Chart: Add topologySpreadConstraints for default backend. Apr 4, 2024

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 4, 2024

Gacko reviewed Apr 4, 2024

View reviewed changes

charts/ingress-nginx/values.yaml Outdated Show resolved Hide resolved

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 4, 2024

Gacko reviewed Apr 4, 2024

View reviewed changes

charts/ingress-nginx/values.yaml Outdated Show resolved Hide resolved

charts/ingress-nginx/values.yaml Outdated Show resolved Hide resolved

msfidelis force-pushed the feature/topology_spread_constraint_default_backend branch from d151996 to faf9e9f Compare April 4, 2024 22:45

Gacko reviewed Apr 5, 2024

View reviewed changes

msfidelis force-pushed the feature/topology_spread_constraint_default_backend branch from faf9e9f to 5a81a66 Compare April 5, 2024 09:59

msfidelis force-pushed the feature/topology_spread_constraint_default_backend branch from 3842aef to 1366a3b Compare April 5, 2024 10:03

Gacko reviewed Apr 5, 2024

View reviewed changes

msfidelis force-pushed the feature/topology_spread_constraint_default_backend branch 2 times, most recently from 7d863d2 to 95d20aa Compare April 5, 2024 10:28

msfidelis force-pushed the feature/topology_spread_constraint_default_backend branch from 95d20aa to adeb875 Compare April 5, 2024 10:34

Gacko reviewed Apr 5, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 5, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2024

k8s-ci-robot merged commit e9509e2 into kubernetes:main Apr 5, 2024
18 checks passed

Gacko mentioned this pull request Apr 5, 2024

Chart: Add unit tests for default backend & topology spread constraints. #11218

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chart: Add `topologySpreadConstraints` for default backend. #11197

Chart: Add `topologySpreadConstraints` for default backend. #11197

msfidelis commented Apr 4, 2024

k8s-ci-robot commented Apr 4, 2024

netlify bot commented Apr 4, 2024 •

edited

Loading

Gacko commented Apr 4, 2024

Gacko commented Apr 4, 2024

Gacko commented Apr 4, 2024

Gacko commented Apr 4, 2024

Gacko left a comment

Gacko commented Apr 4, 2024

Gacko commented Apr 4, 2024

Gacko left a comment

Gacko Apr 5, 2024

Gacko left a comment

Gacko Apr 5, 2024

Gacko Apr 5, 2024

Gacko Apr 5, 2024

Gacko Apr 5, 2024

Gacko commented Apr 5, 2024

Gacko Apr 5, 2024

msfidelis Apr 5, 2024

Gacko commented Apr 5, 2024

k8s-ci-robot commented Apr 5, 2024

Gacko commented Apr 5, 2024

	## Ref: https://kubernetes.io/docs/concepts/workloads/pods/pod-topology-spread-constraints/
	# Ref.: https://kubernetes.io/docs/concepts/workloads/pods/pod-topology-spread-constraints/

	# whenUnsatisfiable: ScheduleAnyway
	# whenUnsatisfiable: ScheduleAnyway

Chart: Add topologySpreadConstraints for default backend. #11197

Chart: Add topologySpreadConstraints for default backend. #11197

Conversation

msfidelis commented Apr 4, 2024

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Apr 4, 2024

netlify bot commented Apr 4, 2024 • edited Loading

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Gacko commented Apr 4, 2024

Gacko commented Apr 4, 2024

Gacko commented Apr 4, 2024

Gacko commented Apr 4, 2024

Gacko left a comment

Choose a reason for hiding this comment

Gacko commented Apr 4, 2024

Gacko commented Apr 4, 2024

Gacko left a comment

Choose a reason for hiding this comment

Gacko Apr 5, 2024

Choose a reason for hiding this comment

Gacko left a comment

Choose a reason for hiding this comment

Gacko Apr 5, 2024

Choose a reason for hiding this comment

Gacko Apr 5, 2024

Choose a reason for hiding this comment

Gacko Apr 5, 2024

Choose a reason for hiding this comment

Gacko Apr 5, 2024

Choose a reason for hiding this comment

Gacko commented Apr 5, 2024

Gacko Apr 5, 2024

Choose a reason for hiding this comment

msfidelis Apr 5, 2024

Choose a reason for hiding this comment

Gacko commented Apr 5, 2024

k8s-ci-robot commented Apr 5, 2024

Gacko commented Apr 5, 2024

Chart: Add `topologySpreadConstraints` for default backend. #11197

Chart: Add `topologySpreadConstraints` for default backend. #11197

netlify bot commented Apr 4, 2024 •

edited

Loading