Add default podAntiAffinity to PodTemplateSpec #319

fossedihelm · 2022-03-02T10:12:46Z

What this PR does / why we need it:
Default PodTemplateSpec now has a podAntiAffinity that makes pod replicas to be scheduled on different nodes.
This affinity will be merged with the requested one.
The merge affect nodeAffinity, podAffinity, podAntiaffinity, tolerations, and nodeSelector
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Release note:

TemplateValidator has a podAntiAffinity that makes pod replicas to be scheduled on different nodes.

iholder101

Thanks @fossedihelm! Great job! 👍

Overall looks great.

Small note about my perspective on comments:
IMO comments should be used to explain complicated code that cannot be simplified easily. In such cases comments need to expose as less implementation details as possible, i.e. explain what the codes does and not how it does it. Also, comments should never point out the obvious (explain something that is immediately obvious from looking at the code).

The reason I'm even mentioning this is because comments can easily do more harm than good. The danger is that the code changes but the comments stay outdated. This can lead to a very confusing code.

Example for what I'm referring to:

// Iterating over Pod's node selector
for i := Pod.NodeSelector { ... }

What do you think? :)

internal/operands/template-validator/reconcile.go

fossedihelm · 2022-03-02T13:33:54Z

@iholder-redhat Thanks for your review. Agree with your perspective about the comments. In this case, they aren't necessary. They are leftovers because, in the first implementation, I wrote all the merge stuff inside the injectPlacementMetadata function and they help reading the different blocks, due to its cyclomatic complexity. Having created specific function for each block comments are not necessary anymore, as you said. I will remove them. 🙂

fossedihelm · 2022-03-02T13:54:48Z

@iholder-redhat I pushed the requested changes. I also push a change on validator_test file: it is the fix of the failing tests. Thanks

fossedihelm · 2022-03-03T13:09:31Z

/test ci/prow/e2e-functests

openshift-ci · 2022-03-03T13:09:33Z

@fossedihelm: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test e2e-functests
/test e2e-single-node-functests
/test e2e-upgrade-functests
/test images
/test unittests

Use /test all to run all jobs.

In response to this:

/test ci/prow/e2e-functests

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.

kubevirt-bot · 2022-03-03T13:09:33Z

@fossedihelm: No presubmit jobs available for kubevirt/ssp-operator@master

In response to this:

/test ci/prow/e2e-functests

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.

fossedihelm · 2022-03-03T13:10:03Z

/test e2e-functests

kubevirt-bot · 2022-03-03T13:10:04Z

@fossedihelm: No presubmit jobs available for kubevirt/ssp-operator@master

In response to this:

/test e2e-functests

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.

fossedihelm · 2022-03-03T13:11:08Z

/retest

fossedihelm · 2022-03-03T14:56:14Z

/test e2e-functests

kubevirt-bot · 2022-03-03T14:56:18Z

@fossedihelm: No presubmit jobs available for kubevirt/ssp-operator@master

In response to this:

/test e2e-functests

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.

akrejcir

Thanks for the PR.
My comments are mostly about code style. You can ignore them, if you want to. Except the last one about using client in reconcile_test.go, that would be better.

akrejcir · 2022-03-03T15:08:26Z

internal/operands/template-validator/reconcile.go

 		return
 	}
+	if podSpec == nil {


This check can be removed, because the calling function never passes nil as podSpec. But if you want to keep this check, it can return on nil, because the function will be noop.

akrejcir · 2022-03-03T15:11:17Z

internal/operands/template-validator/reconcile.go

+	nodePlacement := componentConfig.Placement
+	if nodePlacement.NodeSelector == nil {
+		nodePlacement.NodeSelector = make(map[string]string)
+	}


This if block can be safely removed. Iterating over nil map is the same as iterating over an empty map.

akrejcir · 2022-03-03T15:12:21Z

internal/operands/template-validator/reconcile.go

+		if podSpec.Affinity == nil {
+			podSpec.Affinity = nodePlacement.Affinity.DeepCopy()
+		} else {
+			if nodePlacement.Affinity.NodeAffinity != nil {


Can you move these != nil checks inside the respective functions?

akrejcir · 2022-03-03T15:14:19Z

internal/operands/template-validator/reconcile.go

+		}
+	}
+
+	if len(nodePlacement.Tolerations) != 0 {


This whole if block can be replaced with one line:

podSpec.Tolerations = append(podSpec.Tolerations, nodePlacement.Tolerations...)

Appending to a nil slice allocates a new slice, so you don't need to check if it is nil, and appending an empty slice is a noop.

akrejcir · 2022-03-03T15:15:22Z

internal/operands/template-validator/reconcile.go

+func mergeNodeAffinity(currentAffinity *v1.Affinity, configNodeAffinity *v1.NodeAffinity) {
+	if currentAffinity.NodeAffinity == nil {
+		currentAffinity.NodeAffinity = configNodeAffinity.DeepCopy()
+	} else {


Can you add early return instead of else block?

akrejcir · 2022-03-03T15:28:48Z

internal/operands/template-validator/reconcile_test.go

+								},
+							},
+						},
+						TopologyKey: "kubernetes.io/hostname",


This string is used in many places, can you extract it to a constant?

akrejcir · 2022-03-03T15:29:15Z

internal/operands/template-validator/reconcile_test.go

+
+		var setPodAntiAffinity = func(request *common.Request) {
+			request.Instance.Spec.TemplateValidator.Placement.Affinity.PodAntiAffinity = antiAffinity
+			return


This return can be removed.

akrejcir · 2022-03-03T15:29:26Z

internal/operands/template-validator/reconcile_test.go

+		}
+		var setPodAffinity = func(request *common.Request) {
+			request.Instance.Spec.TemplateValidator.Placement.Affinity.PodAffinity = podAffinity
+			return


This return can be removed.

akrejcir · 2022-03-03T15:29:31Z

internal/operands/template-validator/reconcile_test.go

+		}
+		var setNodeAffinity = func(request *common.Request) {
+			request.Instance.Spec.TemplateValidator.Placement.Affinity.NodeAffinity = nodeAffinity
+			return


This return can be removed.

akrejcir · 2022-03-03T15:32:44Z

internal/operands/template-validator/reconcile_test.go

+			}
+			res, err := operand.Reconcile(&request)
+			Expect(err).ToNot(HaveOccurred())
+			deploymentResult := res[4]


Can you use the client to get the deployment instead of accessing the result of the call? The resource in result is only used for getting names and logging.

You can use:

deployment := &apps.Deployment{} key := client.ObjectKeyFromObject(newDeployment(namespace, replicas, "test-img")) Expect(request.Client.Get(request.Context, key, deployment)).To(Succeed())

fossedihelm · 2022-03-03T16:31:59Z

@akrejcir Thanks for your review. It was constructive and instructive. I'm going to address your suggestions. Thanks

Default PodTemplateSpec now has a podAntiAffinity that makes replicas to be scheduled on different node. This affinity will be merged with the requested one. The merge affect nodeAffinity, podAffinity, podAntiaffinity, tolerations and nodeSelector Signed-off-by: fossedihelm <ffossemo@redhat.com>

sonarcloud · 2022-03-04T07:48:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

fossedihelm · 2022-03-04T11:29:09Z

/retest

akrejcir · 2022-03-04T12:13:02Z

/lgtm

akrejcir · 2022-03-04T12:32:33Z

I've noticed that the commit message is not split into a title and body. When I run git branch -vv it shows the whole commit message as title.
But you don't need to change it now.

fossedihelm · 2022-03-04T13:07:09Z

I've noticed that the commit message is not split into a title and body. When I run git branch -vv it shows the whole commit message as title. But you don't need to change it now.

Thanks. I think I forgot a \n. 🙁

akrejcir · 2022-03-07T10:00:56Z

/approve

kubevirt-bot · 2022-03-07T10:01:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrejcir

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:

~~OWNERS~~ [akrejcir]

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

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 2, 2022

kubevirt-bot requested review from akrejcir and borod108 March 2, 2022 10:12

kubevirt-bot added the size/L label Mar 2, 2022

iholder101 reviewed Mar 2, 2022

View reviewed changes

internal/operands/template-validator/reconcile.go Outdated Show resolved Hide resolved

internal/operands/template-validator/reconcile.go Outdated Show resolved Hide resolved

internal/operands/template-validator/reconcile.go Outdated Show resolved Hide resolved

fossedihelm force-pushed the template_anti_affinity branch from 035442e to 67f1518 Compare March 2, 2022 13:53

fossedihelm force-pushed the template_anti_affinity branch 4 times, most recently from 7d82980 to 6dee28f Compare March 3, 2022 07:29

akrejcir requested changes Mar 3, 2022

View reviewed changes

kubevirt-bot assigned akrejcir Mar 3, 2022

fossedihelm force-pushed the template_anti_affinity branch from 6dee28f to de4c3f1 Compare March 4, 2022 07:47

fossedihelm requested a review from akrejcir March 4, 2022 07:47

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2022

kubevirt-bot merged commit 0e5bfec into kubevirt:master Mar 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default podAntiAffinity to PodTemplateSpec #319

Add default podAntiAffinity to PodTemplateSpec #319

fossedihelm commented Mar 2, 2022

iholder101 left a comment

fossedihelm commented Mar 2, 2022

fossedihelm commented Mar 2, 2022

fossedihelm commented Mar 3, 2022

openshift-ci bot commented Mar 3, 2022

kubevirt-bot commented Mar 3, 2022

fossedihelm commented Mar 3, 2022

kubevirt-bot commented Mar 3, 2022

fossedihelm commented Mar 3, 2022

fossedihelm commented Mar 3, 2022

kubevirt-bot commented Mar 3, 2022

akrejcir left a comment

akrejcir Mar 3, 2022

akrejcir Mar 3, 2022

akrejcir Mar 3, 2022

akrejcir Mar 3, 2022

akrejcir Mar 3, 2022

akrejcir Mar 3, 2022

akrejcir Mar 3, 2022

akrejcir Mar 3, 2022

akrejcir Mar 3, 2022

akrejcir Mar 3, 2022

fossedihelm commented Mar 3, 2022

sonarcloud bot commented Mar 4, 2022

fossedihelm commented Mar 4, 2022

akrejcir commented Mar 4, 2022

akrejcir commented Mar 4, 2022

fossedihelm commented Mar 4, 2022

akrejcir commented Mar 7, 2022

kubevirt-bot commented Mar 7, 2022

Add default podAntiAffinity to PodTemplateSpec #319

Add default podAntiAffinity to PodTemplateSpec #319

Conversation

fossedihelm commented Mar 2, 2022

iholder101 left a comment

Choose a reason for hiding this comment

fossedihelm commented Mar 2, 2022

fossedihelm commented Mar 2, 2022

fossedihelm commented Mar 3, 2022

openshift-ci bot commented Mar 3, 2022

kubevirt-bot commented Mar 3, 2022

fossedihelm commented Mar 3, 2022

kubevirt-bot commented Mar 3, 2022

fossedihelm commented Mar 3, 2022

fossedihelm commented Mar 3, 2022

kubevirt-bot commented Mar 3, 2022

akrejcir 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

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

fossedihelm commented Mar 3, 2022

sonarcloud bot commented Mar 4, 2022

fossedihelm commented Mar 4, 2022

akrejcir commented Mar 4, 2022

akrejcir commented Mar 4, 2022

fossedihelm commented Mar 4, 2022

akrejcir commented Mar 7, 2022

kubevirt-bot commented Mar 7, 2022