Merge pull request #583 from element-hq/bbz/nodeSelector-tests

nodeSelector tests

Author: benbz

charts/matrix-stack/source/common/sub_schema_values.yaml.j2

@@ -175,12 +175,12 @@ imagePullSecrets: []
 
 {% macro labels(global=false, key='labels') %}
 ## Labels to add to all manifest {{ 'for all components in this chart' if global else 'for this component' }}
-labels: {}
+{{ key }}: {}
 {%- endmacro %}
 
 {% macro nodeSelector(key='nodeSelector') %}
 ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-# {{ key }}: {}
+{{ key }}: {}
 {%- endmacro %}
 
 {% macro persistentVolumeClaim(key) %}
@@ -194,9 +194,10 @@ labels: {}
   size: 10Gi
 
   ## The StorageClass to be used by the constructed PersistentVolumeClaim.
-  ## Will use the cluster default if not provided
+  ## Will use the cluster default if not provided.
+  ## If changed after initial usage the PersistentVolumeClaim could be recreated causing data-loss.
   ## Ignored if existingClaim is provided
-  # storageClass:
+  # storageClassName:
 
   ## Whether to instruct Helm to keep or delete the constructed PersistentVolumeClaim when uninstalling the chart
   ## Ignored if existingClaim is provided

charts/matrix-stack/values.yaml

@@ -149,7 +149,7 @@ initSecrets:
   ##   value: "bar"
   extraEnv: []
   ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-  # nodeSelector: {}
+  nodeSelector: {}
   ## A subset of PodSecurityContext. PodSecurityContext holds pod-level security attributes and common container settings
   podSecurityContext:
     ## A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to
@@ -276,7 +276,7 @@ deploymentMarkers:
   ##   value: "bar"
   extraEnv: []
   ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-  # nodeSelector: {}
+  nodeSelector: {}
   ## A subset of PodSecurityContext. PodSecurityContext holds pod-level security attributes and common container settings
   podSecurityContext:
     ## A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to
@@ -491,7 +491,7 @@ matrixRTC:
   ##   value: "bar"
   extraEnv: []
   ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-  # nodeSelector: {}
+  nodeSelector: {}
   ## A subset of PodSecurityContext. PodSecurityContext holds pod-level security attributes and common container settings
   podSecurityContext:
     ## A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to
@@ -743,7 +743,7 @@ matrixRTC:
       # seccompProfile:
       #  type: RuntimeDefault
     ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-    # nodeSelector: {}
+    nodeSelector: {}
     ## A subset of PodSecurityContext. PodSecurityContext holds pod-level security attributes and common container settings
     podSecurityContext:
       ## A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to
@@ -977,7 +977,7 @@ elementWeb:
     # seccompProfile:
     #  type: RuntimeDefault
   ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-  # nodeSelector: {}
+  nodeSelector: {}
   ## A subset of PodSecurityContext. PodSecurityContext holds pod-level security attributes and common container settings
   podSecurityContext:
     ## A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to
@@ -1188,7 +1188,7 @@ haproxy:
   ##   value: "bar"
   extraEnv: []
   ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-  # nodeSelector: {}
+  nodeSelector: {}
   ## A subset of PodSecurityContext. PodSecurityContext holds pod-level security attributes and common container settings
   podSecurityContext:
     ## A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to
@@ -1560,7 +1560,7 @@ matrixAuthenticationService:
     annotations: {}
 
   ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-  # nodeSelector: {}
+  nodeSelector: {}
 
   ## Workload tolerations allows Pods that are part of this (sub)component to 'tolerate' any taint that matches the triple <key,value,effect> using the matching operator <operator>.
   ##
@@ -1818,7 +1818,7 @@ matrixAuthenticationService:
       # seccompProfile:
       #  type: RuntimeDefault
     ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-    # nodeSelector: {}
+    nodeSelector: {}
     ## A subset of PodSecurityContext. PodSecurityContext holds pod-level security attributes and common container settings
     podSecurityContext:
       ## A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to
@@ -2083,9 +2083,10 @@ postgres:
     size: 10Gi
 
     ## The StorageClass to be used by the constructed PersistentVolumeClaim.
-    ## Will use the cluster default if not provided
+    ## Will use the cluster default if not provided.
+    ## If changed after initial usage the PersistentVolumeClaim could be recreated causing data-loss.
     ## Ignored if existingClaim is provided
-    # storageClass:
+    # storageClassName:
 
     ## Whether to instruct Helm to keep or delete the constructed PersistentVolumeClaim when uninstalling the chart
     ## Ignored if existingClaim is provided
@@ -2150,7 +2151,7 @@ postgres:
     # seccompProfile:
     #  type: RuntimeDefault
   ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-  # nodeSelector: {}
+  nodeSelector: {}
   ## A subset of PodSecurityContext. PodSecurityContext holds pod-level security attributes and common container settings
   podSecurityContext:
     ## A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to
@@ -2363,9 +2364,10 @@ synapse:
       size: 10Gi
 
       ## The StorageClass to be used by the constructed PersistentVolumeClaim.
-      ## Will use the cluster default if not provided
+      ## Will use the cluster default if not provided.
+      ## If changed after initial usage the PersistentVolumeClaim could be recreated causing data-loss.
       ## Ignored if existingClaim is provided
-      # storageClass:
+      # storageClassName:
 
       ## Whether to instruct Helm to keep or delete the constructed PersistentVolumeClaim when uninstalling the chart
       ## Ignored if existingClaim is provided
@@ -3682,7 +3684,7 @@ synapse:
   ##   - synapse.ess.localhost
   hostAliases: []
   ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-  # nodeSelector: {}
+  nodeSelector: {}
   ## A subset of PodSecurityContext. PodSecurityContext holds pod-level security attributes and common container settings
   podSecurityContext:
     ## A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to
@@ -3899,7 +3901,7 @@ synapse:
     ##   value: "bar"
     extraEnv: []
     ## NodeSelector is a selector which must be true for the pod to fit on a node. Selector which must match a node's labels for the pod to be scheduled on that node. More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
-    # nodeSelector: {}
+    nodeSelector: {}
     ## A subset of PodSecurityContext. PodSecurityContext holds pod-level security attributes and common container settings
     podSecurityContext:
       ## A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to

newsfragments/583.fixed.md

@@ -0,0 +1,13 @@
+Fix Postgres and Synapse Media `storageClassName` configuration not being respected.
+
+**Warning** Previously `synapse.media.storage.storageClass` and `postgres.storage.storageClass`
+were in the values file and associated schema. These values were accidentally silently ignored
+and all chart-managed `PersistentVolumeClaims` were constructed without `spec.storageClassName`
+set, using the cluster default `StorageClass`.
+
+The values file and associated schema have been updated so that the values are now
+`synapse.media.storage.storageClassName` and `postgres.storage.storageClassName`. The previous
+values are disallowed by the schema. Setting these values after the initial install could 
+cause the `PersistentVolumeClaims` to be recreated, with associated data-loss. Only set
+`synapse.media.storage.storageClassName` or `postgres.storage.storageClassName` on initial
+installation.

newsfragments/583.internal.md

@@ -0,0 +1 @@
+CI: test nodeSelectors are appropriately configured.

tests/manifests/__init__.py

@@ -16,6 +16,7 @@ class PropertyType(Enum):
     Ingress = "ingress"
     Labels = "labels"
     LivenessProbe = "livenessProbe"
+    NodeSelector = "nodeSelector"
     PodSecurityContext = "podSecurityContext"
     Postgres = "postgres"
     Replicas = "replicas"
@@ -215,6 +216,7 @@ def __post_init__(self):
 
         sidecar_values_file_path_overrides = {
             # Not possible, will come from the parent component
+            PropertyType.NodeSelector: ValuesFilePath.not_supported(),
             PropertyType.PodSecurityContext: ValuesFilePath.not_supported(),
             PropertyType.ServiceAccount: ValuesFilePath.not_supported(),
             PropertyType.Tolerations: ValuesFilePath.not_supported(),
@@ -349,6 +351,7 @@ def make_synapse_worker_sub_component(worker_name: str, worker_type: str) -> Sub
         PropertyType.HostAliases: ValuesFilePath.read_elsewhere("synapse", "hostAliases"),
         PropertyType.Image: ValuesFilePath.read_elsewhere("synapse", "image"),
         PropertyType.Labels: ValuesFilePath.read_elsewhere("synapse", "labels"),
+        PropertyType.NodeSelector: ValuesFilePath.read_elsewhere("synapse", "nodeSelector"),
         PropertyType.PodSecurityContext: ValuesFilePath.read_elsewhere("synapse", "podSecurityContext"),
         PropertyType.ServiceAccount: ValuesFilePath.read_elsewhere("synapse", "serviceAccount"),
         PropertyType.ServiceMonitor: ValuesFilePath.read_elsewhere("synapse", "serviceMonitor"),
@@ -564,6 +567,7 @@ def make_synapse_worker_sub_component(worker_name: str, worker_type: str) -> Sub
                     PropertyType.Image: ValuesFilePath.read_elsewhere("synapse", "image"),
                     # Job so no livenessProbe
                     PropertyType.LivenessProbe: ValuesFilePath.not_supported(),
+                    PropertyType.NodeSelector: ValuesFilePath.read_elsewhere("synapse", "nodeSelector"),
                     PropertyType.PodSecurityContext: ValuesFilePath.read_elsewhere("synapse", "podSecurityContext"),
                     PropertyType.Resources: ValuesFilePath.read_elsewhere("synapse", "resources"),
                     # Job so no readinessProbe

tests/manifests/test_basic.py

@@ -95,12 +95,24 @@ async def test_default_values_file_sets_stub_values(base_values):
     unset_marker = "XXXX unset XXX"
     for deployable_details in all_deployables_details:
         extraEnv = deployable_details.get_helm_values(base_values, PropertyType.Env, default_value=unset_marker)
+        nodeSelector = deployable_details.get_helm_values(
+            base_values, PropertyType.NodeSelector, default_value=unset_marker
+        )
         if deployable_details.has_workloads:
             assert extraEnv == [], f"{deployable_details.name} has default {extraEnv=} rather than []"
+            # Might be None iff a `not_supported` values file path override is set, e.g. for Sidecars
+            # default_value=unset_marker means that an omitted `nodeSelector` in the values file won't
+            # return None here
+            assert nodeSelector == {} or nodeSelector is None, (
+                f"{deployable_details.name} has default {nodeSelector=} rather than {{}}"
+            )
         else:
             assert extraEnv == unset_marker, (
                 f"{deployable_details.name} has default {extraEnv=} rather than being unset"
             )
+            assert nodeSelector == unset_marker, (
+                f"{deployable_details.name} has default {nodeSelector=} rather than being unset"
+            )
 
         hostAliases = deployable_details.get_helm_values(
             base_values, PropertyType.HostAliases, default_value=unset_marker

tests/manifests/test_pod_nodeSelector.py

@@ -0,0 +1,48 @@
+# Copyright 2025 New Vector Ltd
+#
+# SPDX-License-Identifier: AGPL-3.0-only
+
+import random
+import string
+
+import pytest
+
+from . import DeployableDetails, PropertyType, values_files_to_test
+from .utils import (
+    iterate_deployables_workload_parts,
+    template_id,
+    template_to_deployable_details,
+)
+
+
+@pytest.mark.parametrize("values_file", values_files_to_test)
+@pytest.mark.asyncio_cooperative
+async def test_pod_has_no_nodeSelector_by_default(templates):
+    for template in templates:
+        if template["kind"] in ["Deployment", "StatefulSet", "Job"]:
+            pod_spec = template["spec"]["template"]["spec"]
+            assert "nodeSelector" not in pod_spec, (
+                f"{template_id(template)} has a default nodeSelector when one isn't configured"
+            )
+
+
+@pytest.mark.parametrize("values_file", values_files_to_test)
+@pytest.mark.asyncio_cooperative
+async def test_pod_gets_configured_nodeSelector(values, make_templates, release_name):
+    def set_nodeSelector(deployable_details: DeployableDetails):
+        nodeSelector = {"k8s.element.io/testing": "".join(random.choices(string.ascii_lowercase))}
+        deployable_details.set_helm_values(values, PropertyType.NodeSelector, nodeSelector)
+
+    iterate_deployables_workload_parts(set_nodeSelector)
+    for template in await make_templates(values):
+        if template["kind"] in ["Deployment", "StatefulSet", "Job"]:
+            pod_spec = template["spec"]["template"]["spec"]
+            assert "nodeSelector" in pod_spec, (
+                f"{template_id(template)} doesn't have a nodeSelector when one is configured"
+            )
+
+            deployable_details = template_to_deployable_details(template)
+            expected_nodeSelector = deployable_details.get_helm_values(values, PropertyType.NodeSelector)
+            assert pod_spec["nodeSelector"] == expected_nodeSelector, (
+                f"{template_id(template)} has an unexpected nodeSelector"
+            )