Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Topology Spread Constraints support #410

Merged
merged 6 commits into from
Nov 1, 2023
Merged

Add Topology Spread Constraints support #410

merged 6 commits into from
Nov 1, 2023

Conversation

halim-lee
Copy link
Collaborator

What this PR does / why we need it?:

  • Topology Spread Constraints support

Does this PR introduce a user-facing change?

  • User guide
  • CHANGELOG.md

Which issue(s) this PR fixes:

Fixes #

@halim-lee halim-lee requested a review from arturdzm July 13, 2022 13:10
@halim-lee
Copy link
Collaborator Author

Related: #340

@kabicin kabicin requested review from leochr and removed request for arturdzm October 31, 2023 17:47
Copy link
Member

@leochr leochr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Added some comments.

func CustomizeTopologySpreadConstraints(pts *corev1.PodTemplateSpec, zoneDomainMatchLabels map[string]string) {
pts.Spec.TopologySpreadConstraints = make([]corev1.TopologySpreadConstraint, 0)
if len(zoneDomainMatchLabels) > 0 {
pts.Spec.TopologySpreadConstraints = append(pts.Spec.TopologySpreadConstraints, corev1.TopologySpreadConstraint{
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a similar constraint using topology key kubernetes.io/hostname? It'll find the most ideal node I believe as per this statement in the k8s doc:

"When a Pod defines more than one topologySpreadConstraint, those constraints are combined using a logical AND operation: the kube-scheduler looks for a node for the incoming Pod that satisfies all the configured constraints."

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think it's okay to add here. It's been documented to not always work the way we expect (because the scheduler adds one pod at a time). But in such cases, we are covered by the ScheduleAnyway flag and our pod anti-affinity rules.

@@ -1215,6 +1232,25 @@ func GetWatchNamespaces() ([]string, error) {
return watchNamespaces, nil
}

// MergeTopologySpreadConstraints returns the union of all TopologySpreadConstraint lists. The order of lists passed into the
// func, defines the importance.
func MergeTopologySpreadConstraints(constraints ...[]corev1.TopologySpreadConstraint) []corev1.TopologySpreadConstraint {
Copy link
Member

Choose a reason for hiding this comment

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

Customers could set default constraints for the entire cluster. But those defaults would be overridden by our default constraints. It'll be good to consider providing an option/way, so they can disable our defaults without having to override each constraint (using the relevant topology key)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added flag .spec.topologySpreadConstraints.disableOperatorDefaults. It defaults to false, so operator default TopologySpreadConstraints will be applied if the flag isn't specified. However, this also means upgrading the operator will cause brief downtime for the pod to apply our default constraints.

@kabicin kabicin requested a review from leochr November 1, 2023 19:14
Copy link
Member

@leochr leochr left a comment

Choose a reason for hiding this comment

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

@kabicin Thanks for addressing the review comments. Just one minor comment about the description.

@@ -146,6 +146,21 @@ type RuntimeComponentSpec struct {
// Security context for the application container.
// +operator-sdk:csv:customresourcedefinitions:order=25,type=spec,displayName="Security Context"
SecurityContext *corev1.SecurityContext `json:"securityContext,omitempty"`

// Topology spread constraints for the application pod.
Copy link
Member

Choose a reason for hiding this comment

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

This line can be removed, so the comment above RuntimeComponentTopologySpreadConstraints type's struct can be used

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, I removed line 150.

@kabicin kabicin requested a review from leochr November 1, 2023 20:41
@leochr leochr merged commit 42a1eb2 into main Nov 1, 2023
@leochr leochr deleted the topology branch November 1, 2023 20:49
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