Fix Postgres and Synapse Media storageClassName configuration not being respected.

Author: benbz

charts/matrix-stack/source/common/persistentVolumeClaim.json

@@ -7,7 +7,7 @@
     "size": {
       "type": "string"
     },
-    "storageClass": {
+    "storageClassName": {
       "type": "string"
     },
     "resourcePolicy": {

charts/matrix-stack/values.schema.json

@@ -4856,7 +4856,7 @@
             "size": {
               "type": "string"
             },
-            "storageClass": {
+            "storageClassName": {
               "type": "string"
             },
             "resourcePolicy": {
@@ -5454,7 +5454,7 @@
                 "size": {
                   "type": "string"
                 },
-                "storageClass": {
+                "storageClassName": {
                   "type": "string"
                 },
                 "resourcePolicy": {

newsfragments/582.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/582.internal.md

@@ -0,0 +1 @@
+CI: improve testing of chart managed `PersistentVolumeClaims`.

tests/manifests/__init__.py

@@ -24,6 +24,7 @@ class PropertyType(Enum):
     StartupProbe = "startupProbe"
     ServiceAccount = "serviceAccount"
     ServiceMonitor = "serviceMonitors"
+    Storage = "storage"
     Tolerations = "tolerations"
     TopologySpreadConstraints = "topologySpreadConstraints"
 
@@ -535,6 +536,9 @@ def make_synapse_worker_sub_component(worker_name: str, worker_type: str) -> Sub
     ),
     ComponentDetails(
         name="synapse",
+        values_file_path_overrides={
+            PropertyType.Storage: ValuesFilePath.read_write("synapse", "media", "storage"),
+        },
         has_db=True,
         has_storage=True,
         has_replicas=False,

tests/manifests/test_pvcs.py

@@ -2,10 +2,13 @@
 #
 # SPDX-License-Identifier: AGPL-3.0-only
 
+import random
+import string
+
 import pytest
 
-from . import values_files_to_test
-from .utils import template_to_deployable_details
+from . import DeployableDetails, PropertyType, values_files_to_test
+from .utils import iterate_deployables_parts, template_id, template_to_deployable_details
 
 
 @pytest.mark.parametrize("values_file", values_files_to_test)
@@ -31,3 +34,82 @@ async def test_pvcs_marked_as_being_kept_on_helm_uninstall_by_default(templates)
         if template["kind"] == "PersistentVolumeClaim":
             assert "helm.sh/resource-policy" in template["metadata"]["annotations"]
             assert template["metadata"]["annotations"]["helm.sh/resource-policy"] == "keep"
+
+
+@pytest.mark.parametrize("values_file", values_files_to_test)
+@pytest.mark.asyncio_cooperative
+async def test_pvcs_can_be_marked_as_to_be_deleted_on_helm_uninstall(values, make_templates):
+    iterate_deployables_parts(
+        lambda deployable_details: deployable_details.set_helm_values(
+            values, PropertyType.Storage, {"resourcePolicy": "delete"}
+        ),
+        lambda deployable_details: deployable_details.has_storage,
+    )
+
+    for template in await make_templates(values):
+        if template["kind"] == "PersistentVolumeClaim":
+            assert "helm.sh/resource-policy" in template["metadata"]["annotations"]
+            assert template["metadata"]["annotations"]["helm.sh/resource-policy"] == "delete"
+
+
+@pytest.mark.parametrize("values_file", values_files_to_test)
+@pytest.mark.asyncio_cooperative
+async def test_no_pvcs_created_if_existing_claims_specified(values, make_templates):
+    iterate_deployables_parts(
+        lambda deployable_details: deployable_details.set_helm_values(
+            values, PropertyType.Storage, {"existingClaim": "something"}
+        ),
+        lambda deployable_details: deployable_details.has_storage,
+    )
+
+    for template in await make_templates(values):
+        assert template["kind"] != "PersistentVolumeClaim", (
+            f"{template_id(template)} was created despite referencing existingClaims for all PVCs"
+        )
+
+
+@pytest.mark.parametrize("values_file", values_files_to_test)
+@pytest.mark.asyncio_cooperative
+async def test_size_passed_through_to_created_pvcs(values, make_templates):
+    counter = 1
+
+    def set_pvc_size(deployable_details: DeployableDetails):
+        nonlocal counter
+        deployable_details.set_helm_values(values, PropertyType.Storage, {"size": f"{counter}Gi"})
+        counter += 1
+
+    iterate_deployables_parts(set_pvc_size, lambda deployable_details: deployable_details.has_storage)
+
+    for template in await make_templates(values):
+        if template["kind"] == "PersistentVolumeClaim":
+            deployable_details = template_to_deployable_details(template)
+            pvc_requests = template["spec"]["resources"]["requests"]
+            assert "storage" in pvc_requests, f"{template_id(template)} didn't specify any storage requests"
+
+            expected_size = deployable_details.get_helm_values(values, PropertyType.Storage)["size"]
+            assert expected_size == pvc_requests["storage"], (
+                f"{template_id(template)} didn't respect the configured PVC size"
+            )
+
+
+@pytest.mark.parametrize("values_file", values_files_to_test)
+@pytest.mark.asyncio_cooperative
+async def test_storageClassName_passed_through_to_created_pvcs(values, make_templates):
+    iterate_deployables_parts(
+        lambda deployable_details: deployable_details.set_helm_values(
+            values, PropertyType.Storage, {"storageClassName": "".join(random.choices(string.ascii_lowercase, k=10))}
+        ),
+        lambda deployable_details: deployable_details.has_storage,
+    )
+
+    for template in await make_templates(values):
+        if template["kind"] == "PersistentVolumeClaim":
+            assert "storageClassName" in template["spec"], (
+                f"{template_id(template)} did not set spec.storageClassName despite being configured to"
+            )
+            deployable_details = template_to_deployable_details(template)
+
+            expected_storageClassName = deployable_details.get_helm_values(values, PropertyType.Storage)["storageClassName"]
+            assert expected_storageClassName == template["spec"]["storageClassName"], (
+                f"{template_id(template)} didn't respect the configured PVC storageClassName"
+            )