Extend ability to set topologySpreadConstraints to all component workloads #589

relu · 2024-07-25T11:28:44Z

This should allow users to configure topologySpreadhConstraints for all operator-managed workloads.

frouioui

the operator-latest.yaml file has to be updated with the latest CRD too. Additionally, a follow-up PR will have to be made on the vitess repository to update the given operator.yaml file in the examples.

pkg/apis/planetscale/v2/etcdlockserver_types.go

mattlord

Thanks, @relu ! It looks good to me. I only had a few minor questions and suggestions. I can come back to this quickly when you have a chance to respond.

Thanks again!

mattlord · 2024-08-05T23:55:55Z

pkg/operator/contenthash/topology_spread_constraints.go

+		if tsc.MinDomains != nil {
+			minDomainsSeconds = fmt.Sprint(*tsc.MinDomains)
+		}
+		writeStringHash(h, minDomainsSeconds)


Suggested change

if tsc.MinDomains != nil {

minDomainsSeconds = fmt.Sprint(*tsc.MinDomains)

}

writeStringHash(h, minDomainsSeconds)

if tsc.MinDomains != nil {

writeStringHash(h, string(*tsc.MinDomains))

}

mattlord · 2024-08-06T00:01:07Z

pkg/operator/contenthash/topology_spread_constraints_test.go

@@ -0,0 +1,64 @@
+/*
+Copyright 2020 PlanetScale Inc.


We should use 2024 for new files.

mattlord · 2024-08-06T00:05:31Z

pkg/operator/update/podspec.go

-		srcMap, srcErr := metav1.LabelSelectorAsMap(srcObj.LabelSelector)
-		if srcErr != nil {
-			// There is no return of error for this function
-			// or its callers, which makes this a bit of a hack
-			srcMap = map[string]string{}
-		}
 		// If this item is already there, update it.
 		for dstIndex := range *dst {
 			dstObj := &(*dst)[dstIndex]
-			dstMap, dstErr := metav1.LabelSelectorAsMap(dstObj.LabelSelector)
-			if dstErr != nil {
-				dstMap = map[string]string{}
-			}
-			if reflect.DeepEqual(dstMap, srcMap) {


I'm not sure why this code was added, so also not sure that this is safe. Reads to me like this was primarily about handling nils and invalid/unexpected data which we lose here. Can you please explain the reasoning?

The way this was intended to work was that the list of corev1.TopologySpreadConstraint items would be matched exclusively on the labelSelector, causing any previously existing items having the same labelSelector to be overridden.
This behavior doesn't make sense because the labelSelector will most likely be the same across all topology spread constraint items. Instead, we should only append to the list and not touch any existing values.

I've simplified the code a bit and added some test examples.

I think it's reasonable to do a DeepEqual here as it will catch any potential change to the Topology Spread Constraints.

However, I am a bit afraid of the performance implication that reflect.DeepEqual brings. Since we are calling it in a nested loop, if the user defines many constraints on their vttablet or etcd pods we might be left with a longer update. To avoid this potential issue, could we create a method to compare two constraints?

frouioui · 2024-10-07T16:48:55Z

Hey @relu! Are you still working on this? It would be nice to include this in the upcoming v2.14.0 release of the vitess-operator if we can. The RC-1 is scheduled in a week, and GA late October.

…kloads Signed-off-by: Aurel Canciu <[email protected]>

Signed-off-by: Aurel Canciu <[email protected]>

frouioui

Besides the comments I have left it looks good to me. Do you have a manual E2E test for this, if so, could you share the script and output with us?

Also, please open a follow-up PR on vitessio/vitess to update the operator.yaml with what you changed on this PR in operator-latest.yaml.

frouioui · 2024-10-21T17:11:12Z

docs/api.md

@@ -1227,6 +1227,20 @@ ServiceOverrides
 <p>Tolerations allow you to schedule pods onto nodes with matching taints.</p>
 </td>
 </tr>
+<tr>
+<td>
+<code>topologySpreadConstraints</code></br>


The way we generate documentation changed in #625. We no longer use </br> as it can break in Markdown. Do you mind merging main onto your branch and re-generating the docs?

frouioui · 2024-10-21T17:25:47Z

pkg/operator/contenthash/topology_spread_constraints.go

+		writeStringHash(h, strings.Join(tsc.MatchLabelKeys, ""))
+		if tsc.MinDomains != nil {
+			writeStringHash(h, string(*tsc.MinDomains))
+		}
+		if tsc.NodeAffinityPolicy != nil {
+			writeStringHash(h, string(*tsc.NodeAffinityPolicy))
+		}
+		if tsc.NodeTaintsPolicy != nil {
+			writeStringHash(h, string(*tsc.NodeTaintsPolicy))
+		}


That is a good catch. I am guessing the hash we were previously computing were not encapsulating all the information we could add?

frouioui · 2024-10-21T17:28:29Z

pkg/operator/contenthash/topology_spread_constraints_test.go

+	if nilHash == zeroHash {
+		t.Errorf("nilHash = zeroHash = %v; expected different values", nilHash)
+	}


Instead of checking the equality manually we should use require.NotEqual. It is something we have adopted a while ago on vitessio/vitess. We should start doing the same for new tests on the operator. Same applies to TestTopologySpreadConstraintNodeAffinityPolicysNil and TestTopologySpreadConstraintNodeTaintsPolicysNil.

frouioui · 2024-10-21T17:31:33Z

pkg/operator/contenthash/topology_spread_constraints_test.go

+}
+
+func TestTopologySpreadConstraintNodeAffinityPolicysNil(t *testing.T) {
+	// Make sure nil is distinguishable from 0.


The comment does not entirely make sense here, same for the following test

frouioui · 2024-10-21T17:56:50Z

pkg/operator/update/podspec.go

-		srcMap, srcErr := metav1.LabelSelectorAsMap(srcObj.LabelSelector)
-		if srcErr != nil {
-			// There is no return of error for this function
-			// or its callers, which makes this a bit of a hack
-			srcMap = map[string]string{}
-		}
 		// If this item is already there, update it.
 		for dstIndex := range *dst {
 			dstObj := &(*dst)[dstIndex]
-			dstMap, dstErr := metav1.LabelSelectorAsMap(dstObj.LabelSelector)
-			if dstErr != nil {
-				dstMap = map[string]string{}
-			}
-			if reflect.DeepEqual(dstMap, srcMap) {


I think it's reasonable to do a DeepEqual here as it will catch any potential change to the Topology Spread Constraints.

However, I am a bit afraid of the performance implication that reflect.DeepEqual brings. Since we are calling it in a nested loop, if the user defines many constraints on their vttablet or etcd pods we might be left with a longer update. To avoid this potential issue, could we create a method to compare two constraints?

relu force-pushed the extend-tsc-usage branch from ce748b8 to ae4f0ba Compare July 25, 2024 20:26

frouioui reviewed Jul 30, 2024

View reviewed changes

pkg/apis/planetscale/v2/etcdlockserver_types.go Show resolved Hide resolved

mattlord changed the title ~~Extend ability to set topologySpreadhConstraints to all component workloads~~ Extend ability to set topologySpreadConstraints to all component workloads Aug 1, 2024

mattlord reviewed Aug 6, 2024

View reviewed changes

relu added 5 commits October 16, 2024 20:30

Extend ability to set topologySpreadhConstraints to all component wor…

80547c7

…kloads Signed-off-by: Aurel Canciu <[email protected]>

Improve update and contenthash logic for ToplogoSpreadConstraints

bace806

Signed-off-by: Aurel Canciu <[email protected]>

Update operator-latest.yaml

257e25b

Signed-off-by: Aurel Canciu <[email protected]>

Code review fixes for topology_spread_constraints

e600832

Signed-off-by: Aurel Canciu <[email protected]>

Update podspec and improve TestTopologySpreadConstraints

ad5616b

Signed-off-by: Aurel Canciu <[email protected]>

relu force-pushed the extend-tsc-usage branch from 30ee721 to ad5616b Compare October 16, 2024 18:35

frouioui requested review from frouioui and mattlord October 21, 2024 15:25

frouioui reviewed Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend ability to set topologySpreadConstraints to all component workloads #589

Extend ability to set topologySpreadConstraints to all component workloads #589

relu commented Jul 25, 2024

frouioui left a comment

mattlord left a comment

mattlord Aug 5, 2024

mattlord Aug 6, 2024

mattlord Aug 6, 2024

relu Oct 16, 2024

frouioui Oct 21, 2024

frouioui commented Oct 7, 2024

frouioui left a comment •

edited

Loading

frouioui Oct 21, 2024

frouioui Oct 21, 2024

frouioui Oct 21, 2024

frouioui Oct 21, 2024

frouioui Oct 21, 2024

Extend ability to set topologySpreadConstraints to all component workloads #589

Are you sure you want to change the base?

Extend ability to set topologySpreadConstraints to all component workloads #589

Conversation

relu commented Jul 25, 2024

frouioui left a comment

Choose a reason for hiding this comment

mattlord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frouioui commented Oct 7, 2024

frouioui left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frouioui left a comment •

edited

Loading