Repository: kubermatic/machine-controller
Pull request #1557 (2023-02-08 11:13:30)

Generate correct node affinity if no values are passed
**What this PR does / why we need it**:
We discovered during QA that if you pass affinity settings for KubeVirt machines like this one:

```yaml
affinity:
  nodeAffinityPreset:
    key: test
    type: hard
 ```
 
 machine-controller will fail to create the VM because of this error:
 
 ```
 E0208 10:40:53.418009       1 machine_controller.go:382] Failed to reconcile machine "mbhl5h4xkc-worker-bbxjzj-544cc46784-tz4rk": failed to create machine at cloudprovider, due to failed to create vmi: admission webhook "virtualmachine-validator.kubevirt.io" denied the request: spec.template.spec.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].values: Required value: must be specified when `operator` is 'In' or 'NotIn'
 ```
 
 This changes the generated expression to use a different operator if no values are passed.

**Which issue(s) this PR fixes**:
<!--optional, in `fixes #<issue number>` format, will close the issue(s) when PR gets merged-->
xref https://github.com/kubermatic/kubermatic/issues/11741

**What type of PR is this?**
/kind bug

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change? Then add your Release Note here**:
<!--
Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
Generate correct KubeVirt node affinity if no values for matching are passed
```

**Documentation**:
<!--
Please do one of the following options:
- Add a link to the existing documentation
- Add a link to the kubermatic/docs pull request
- If no documentation change is applicable then add:
  - TBD (documentation will be added later)
  - NONE (no documentation needed for this PR)
-->
```documentation
NONE
```


kubermatic-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/1557#" title="Author self-approved">embik</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign mfranczy for approval by writing `/assign @mfranczy` in a comment. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

<details open>
Needs approval from an approver in each of these files:

- **[pkg/cloudprovider/provider/kubevirt/OWNERS](https://github.com/kubermatic/machine-controller/blob/main/pkg/cloudprovider/provider/kubevirt/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mfranczy"]} -->
hdurand0710: looks good but can you please add a unit test in `provider_test.go`(data in `testdata` folder) for this?
embik: > looks good but can you please add a unit test in `provider_test.go`(data in `testdata` folder) for this?

Sure, good point!
kubermatic-bot: @embik: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-machine-controller-yamllint | e7e24bc3e84ccd4586dcd2f2c60b70ec80f14fcc | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/1557/pull-machine-controller-yamllint/1623289196804313088) | true | `/test pull-machine-controller-yamllint`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=1557)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
kubermatic-bot: @embik: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-machine-controller-e2e-invalid-objects-get-rejected | 7180038ea88d7ac3c8f718537bd83bcbebe53ec9 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/1557/pull-machine-controller-e2e-invalid-objects-get-rejected/1623289967956463616) | true | `/test pull-machine-controller-e2e-invalid-objects-get-rejected`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=1557)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
embik: /retest
kubermatic-bot: LGTM label has been added.  <details>Git tree hash: 7c3f7402392d1aed994c8b7858b22fe1b9d1545b</details>
hdurand0710: /approve
kubermatic-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/1557#" title="Author self-approved">embik</a>*, *<a href="https://github.com/kubermatic/machine-controller/pull/1557#issuecomment-1422518186" title="Approved">hdurand0710</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/cloudprovider/provider/kubevirt/OWNERS](https://github.com/kubermatic/machine-controller/blob/main/pkg/cloudprovider/provider/kubevirt/OWNERS)~~ [hdurand0710]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/cloudprovider/provider/kubevirt/provider.go b/pkg/cloudprovider/provider/kubevirt/provider.go
index 28654c0f..044439b1 100644
--- a/pkg/cloudprovider/provider/kubevirt/provider.go
+++ b/pkg/cloudprovider/provider/kubevirt/provider.go
@@ -852,6 +852,19 @@ func getDataVolumeTemplates(config *Config, dataVolumeName string) []kubevirtv1.
 func getAffinity(config *Config, matchKey, matchValue string) *corev1.Affinity {
 	affinity := &corev1.Affinity{}
 
+	expressions := []corev1.NodeSelectorRequirement{
+		{
+			Key:      config.NodeAffinityPreset.Key,
+			Operator: corev1.NodeSelectorOperator(metav1.LabelSelectorOpExists),
+		},
+	}
+
+	// change the operator if any values were passed for node affinity matching
+	if len(config.NodeAffinityPreset.Values) > 0 {
+		expressions[0].Operator = corev1.NodeSelectorOperator(metav1.LabelSelectorOpIn)
+		expressions[0].Values = config.NodeAffinityPreset.Values
+	}
+
 	// NodeAffinity
 	switch config.NodeAffinityPreset.Type {
 	case softAffinityType:
@@ -860,13 +873,7 @@ func getAffinity(config *Config, matchKey, matchValue string) *corev1.Affinity {
 				{
 					Weight: 1,
 					Preference: corev1.NodeSelectorTerm{
-						MatchExpressions: []corev1.NodeSelectorRequirement{
-							{
-								Key:      config.NodeAffinityPreset.Key,
-								Values:   config.NodeAffinityPreset.Values,
-								Operator: corev1.NodeSelectorOperator(metav1.LabelSelectorOpIn),
-							},
-						},
+						MatchExpressions: expressions,
 					},
 				},
 			},
@@ -876,13 +883,7 @@ func getAffinity(config *Config, matchKey, matchValue string) *corev1.Affinity {
 			RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{
 				NodeSelectorTerms: []corev1.NodeSelectorTerm{
 					{
-						MatchExpressions: []corev1.NodeSelectorRequirement{
-							{
-								Key:      config.NodeAffinityPreset.Key,
-								Values:   config.NodeAffinityPreset.Values,
-								Operator: corev1.NodeSelectorOperator(metav1.LabelSelectorOpIn),
-							},
-						},
+						MatchExpressions: expressions,
 					},
 				},
 			},


Test patch:
diff --git a/pkg/cloudprovider/provider/kubevirt/provider_test.go b/pkg/cloudprovider/provider/kubevirt/provider_test.go
index cdab28d2..1ec62cc0 100644
--- a/pkg/cloudprovider/provider/kubevirt/provider_test.go
+++ b/pkg/cloudprovider/provider/kubevirt/provider_test.go
@@ -60,6 +60,7 @@ type kubevirtProviderSpecConf struct {
 	OperatingSystem          string
 	TopologySpreadConstraint bool
 	Affinity                 bool
+	AffinityValues           bool
 	SecondaryDisks           bool
 	OsImageSource            imageSource
 }
@@ -86,9 +87,11 @@ func (k kubevirtProviderSpecConf) rawProviderSpec(t *testing.T) []byte {
 		"affinity": {
 		  "nodeAffinityPreset": {
 		    "type": "hard",
-			"key": "key1",
-			"values": [
+			"key": "key1"
+            {{- if .AffinityValues }}
+			, "values": [
 				"foo1", "foo2" ]
+            {{- end }}
 		  }
 		},
 		{{- end }}
@@ -194,7 +197,11 @@ func TestNewVirtualMachine(t *testing.T) {
 		},
 		{
 			name:     "affinity",
-			specConf: kubevirtProviderSpecConf{Affinity: true},
+			specConf: kubevirtProviderSpecConf{Affinity: true, AffinityValues: true},
+		},
+		{
+			name:     "affinity-no-values",
+			specConf: kubevirtProviderSpecConf{Affinity: true, AffinityValues: false},
 		},
 		{
 			name:     "secondary-disks",
diff --git a/pkg/cloudprovider/provider/kubevirt/testdata/affinity-no-values.yaml b/pkg/cloudprovider/provider/kubevirt/testdata/affinity-no-values.yaml
new file mode 100644
index 00000000..f304b2a6
--- /dev/null
+++ b/pkg/cloudprovider/provider/kubevirt/testdata/affinity-no-values.yaml
@@ -0,0 +1,83 @@
+apiVersion: kubevirt.io/v1
+kind: VirtualMachine
+metadata:
+  annotations:
+  labels:
+    cluster.x-k8s.io/cluster-name: cluster-name
+    cluster.x-k8s.io/role: worker
+    kubevirt.io/vm: affinity-no-values
+    md: md-name
+  name: affinity-no-values
+  namespace: test-namespace
+spec:
+  dataVolumeTemplates:
+    - metadata:
+        creationTimestamp: null
+        name: affinity-no-values
+      spec:
+        pvc:
+          accessModes:
+            - ReadWriteOnce
+          resources:
+            requests:
+              storage: 10Gi
+          storageClassName: longhorn
+        source:
+          http:
+            url: http://x.y.z.t/ubuntu.img
+  running: true
+  template:
+    metadata:
+      labels:
+        cluster.x-k8s.io/cluster-name: cluster-name
+        cluster.x-k8s.io/role: worker
+        kubevirt.io/vm: affinity-no-values
+        md: md-name
+    spec:
+      affinity:
+        nodeAffinity: # Section present if nodeAffinityPreset.type != ""
+          requiredDuringSchedulingIgnoredDuringExecution:
+            nodeSelectorTerms:
+              - matchExpressions:
+                  - key: key1
+                    operator: Exists
+      domain:
+        devices:
+          disks:
+            - disk:
+                bus: virtio
+              name: datavolumedisk
+            - disk:
+                bus: virtio
+              name: cloudinitdisk
+          interfaces:
+            - macAddress: b6:f5:b4:fe:45:1d
+              name: default
+              bridge: {}
+        resources:
+          limits:
+            cpu: "2"
+            memory: 2Gi
+          requests:
+            cpu: "2"
+            memory: 2Gi
+      networks:
+        - name: default
+          pod: {}
+      terminationGracePeriodSeconds: 30
+      topologyspreadconstraints:
+        - maxskew: 1
+          topologykey: kubernetes.io/hostname
+          whenunsatisfiable: ScheduleAnyway
+          labelselector:
+            matchlabels:
+              md: md-name
+      volumes:
+        - dataVolume:
+            name: affinity-no-values
+          name: datavolumedisk
+        - cloudInitNoCloud:
+            secretRef:
+              name: udsn
+          name: cloudinitdisk
+      evictionStrategy: External