tests for MR

Signed-off-by: Gerd Oberlechner <[email protected]>

reconcile/ocm_machine_pools.py

@@ -547,34 +547,31 @@ def calculate_diff(
     return diffs, errors
 
 
+def _drift_updates(
+    current_pools: list[AbstractPool],
+    desired: DesiredMachinePool,
+) -> list[ClusterMachinePoolV1]:
+    current_machine_pools = {pool.id: pool for pool in current_pools}
+    return [
+        spec
+        for pool in desired.pools
+        if (current_pool := current_machine_pools.get(pool.q_id))
+        and (spec := current_pool.get_pool_spec_to_update(pool)) is not None
+    ]
+
+
 def calculate_spec_drift(
     current_state: Mapping[str, list[AbstractPool]],
     desired_state: Mapping[str, DesiredMachinePool],
-) -> list[tuple[str, ClusterMachinePoolV1]]:
+) -> dict[str, list[ClusterMachinePoolV1]]:
     """
     Finds spec drifts between OCM and app-interface and returns a list of them.
     """
-    current_machine_pools = {
-        (cluster_name, machine_pool.id): machine_pool
-        for cluster_name, machine_pools in current_state.items()
-        for machine_pool in machine_pools
-    }
-
-    desired_machine_pools = {
-        (desired.cluster_name, desired_machine_pool.q_id): desired_machine_pool
-        for desired in desired_state.values()
-        for desired_machine_pool in desired.pools
+    return {
+        desired_state[k].cluster_path: updates
+        for k in current_state.keys() & desired_state.keys()
+        if (updates := _drift_updates(current_state[k], desired_state[k]))
     }
-    spec_drift_updates: list[tuple[str, ClusterMachinePoolV1]] = []
-    for pool_key, current_pool in current_machine_pools.items():
-        desired_pool = desired_machine_pools.get(pool_key)
-        if not desired_pool:
-            continue
-        update = current_pool.get_pool_spec_to_update(desired_pool)
-        if update:
-            cluster_path = desired_state[pool_key[0]].cluster_path
-            spec_drift_updates.append((cluster_path, update))
-    return spec_drift_updates
 
 
 def update_ocm(dry_run: bool, diffs: Iterable[PoolHandler], ocm_map: OCMMap) -> None:
@@ -588,17 +585,17 @@ def update_ocm(dry_run: bool, diffs: Iterable[PoolHandler], ocm_map: OCMMap) ->
 def update_app_interface(
     dry_run: bool,
     gitlab_project_id: Optional[str],
-    diffs: list[tuple[str, ClusterMachinePoolV1]],
+    diffs: dict[str, list[ClusterMachinePoolV1]],
 ) -> None:
     if not diffs:
         return
 
-    mr = ClustersMachinePoolUpdates()
-    for cluster_path, pool_update in diffs:
-        pool_dict = {
-            k: v for k, v in pool_update.dict(by_alias=True).items() if v is not None
+    mr = ClustersMachinePoolUpdates(
+        machine_pool_updates={
+            cluster_path: [pool.dict(by_alias=True) for pool in pool_updates]
+            for cluster_path, pool_updates in diffs.items()
         }
-        mr.add_machine_pool_update(cluster_path, pool_dict)
+    )
     if not dry_run:
         with mr_client_gateway.init(gitlab_project_id=gitlab_project_id) as mr_cli:
             mr.submit(cli=mr_cli)

reconcile/test/test_ocm_machine_pools.py

@@ -1143,9 +1143,8 @@ def test_update_app_interface_with_subnet(
     update_app_interface_mock.assert_called_once_with(
         False,
         None,
-        [
-            (
-                hypershift_cluster.path,
+        {
+            hypershift_cluster.path: [
                 ClusterMachinePoolV1(
                     id="workers",
                     instance_type="m5.xlarge",
@@ -1155,6 +1154,6 @@ def test_update_app_interface_with_subnet(
                     taints=None,
                     subnet="subnet-1",
                 ),
-            )
-        ],
+            ]
+        },
     )

reconcile/test/utils/test_mr_machine_pool_updates.py

@@ -0,0 +1,114 @@
+from io import StringIO
+from typing import Any
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+from ruamel.yaml import YAML
+
+from reconcile.test.fixtures import Fixtures
+from reconcile.utils.mr.cluster_machine_pool_updates import ClustersMachinePoolUpdates
+from reconcile.utils.ruamel import create_ruamel_instance
+
+
+@pytest.fixture
+def fxt() -> Fixtures:
+    return Fixtures("clusters")
+
+
+@pytest.fixture
+def cluster_spec(fxt: Fixtures) -> dict[str, Any]:
+    return fxt.get_anymarkup("rosa_hcp_spec_ai.yml")
+
+
+@pytest.fixture
+def cluster_spec_raw(fxt: Fixtures) -> str:
+    return fxt.get("rosa_hcp_spec_ai.yml")
+
+
+@pytest.fixture
+def yaml() -> YAML:
+    return create_ruamel_instance(explicit_start=True, width=4096)
+
+
+@pytest.fixture
+def gitlab_cli(cluster_spec_raw: str) -> MagicMock:
+    cli = MagicMock()
+    cli.project.files.get.return_value = cluster_spec_raw.encode()
+    return cli
+
+
+def test_normalized_machine_pool_updates() -> None:
+    mr = ClustersMachinePoolUpdates({
+        "/path": [
+            {
+                "id": "worker",
+                "instance_type": "m5.xlarge",
+                "autoscaling": None,
+                "subnet": "subnet-1",
+            }
+        ]
+    })
+    assert mr.normalized_machine_pool_updates() == {
+        "/path": [
+            {
+                "id": "worker",
+                "instance_type": "m5.xlarge",
+                "subnet": "subnet-1",
+            }
+        ]
+    }
+
+
+@patch.object(ClustersMachinePoolUpdates, "cancel", autospec=True, return_value=None)
+def test_mr_machine_pool_update_changes_to_spec(
+    cancel_mock: Mock, cluster_spec_raw: str, gitlab_cli: MagicMock, yaml: YAML
+) -> None:
+    mr = ClustersMachinePoolUpdates({
+        "/path": [
+            {
+                "id": "worker",
+                "instance_type": "m5.xlarge",
+                "autoscaling": None,
+                "subnet": "subnet-1",
+            }
+        ]
+    })
+    mr.branch = "abranch"
+    mr.process(gitlab_cli)
+
+    with StringIO() as stream:
+        cluster_spec = yaml.load(cluster_spec_raw)
+        cluster_spec["machinePools"][0]["subnet"] = "subnet-1"
+        yaml.dump(cluster_spec, stream)
+        new_content = stream.getvalue()
+
+    gitlab_cli.update_file.assert_called_once_with(
+        branch_name="abranch",
+        file_path="data/path",
+        commit_message="update cluster data/path machine-pool fields",
+        content=new_content,
+    )
+    cancel_mock.assert_not_called()
+
+
+@pytest.mark.parametrize(
+    "no_op_changes",
+    [
+        {"/path": []},
+        {},
+    ],
+)
+@patch.object(ClustersMachinePoolUpdates, "cancel", autospec=True, return_value=None)
+def test_mr_machine_pool_update_no_changes(
+    cancel_mock: Mock,
+    cluster_spec_raw: str,
+    gitlab_cli: MagicMock,
+    yaml: YAML,
+    no_op_changes: dict,
+) -> None:
+    mr = ClustersMachinePoolUpdates(no_op_changes)
+    mr.branch = "abranch"
+    mr.process(gitlab_cli)
+
+    gitlab_cli.update_file.assert_not_called()
+    cancel_mock.assert_called()

reconcile/utils/mr/cluster_machine_pool_updates.py

@@ -10,20 +10,15 @@
 class ClustersMachinePoolUpdates(MergeRequestBase):
     name = "create_cluster_machine_pool_updates_mr"
 
-    def __init__(self) -> None:
-        self.machine_pool_updates: dict[str, list[dict[str, Any]]] = {}
+    def __init__(self, machine_pool_updates: dict[str, list[dict[str, Any]]]) -> None:
+        self.machine_pool_updates: dict[str, list[dict[str, Any]]] = (
+            machine_pool_updates
+        )
 
         super().__init__()
 
         self.labels = []
 
-    def add_machine_pool_update(
-        self, cluster_name: str, machine_pool: dict[str, Any]
-    ) -> None:
-        if cluster_name not in self.machine_pool_updates:
-            self.machine_pool_updates[cluster_name] = []
-        self.machine_pool_updates[cluster_name].append(machine_pool)
-
     @property
     def title(self) -> str:
         return f"[{self.name}] machine pool updates"
@@ -32,10 +27,21 @@ def title(self) -> str:
     def description(self) -> str:
         return DecisionCommand.APPROVED.value
 
+    def normalized_machine_pool_updates(self) -> dict[str, list[dict[str, Any]]]:
+        return {
+            cluster_path: [
+                clean_none_values_from_dict(update) for update in pool_updates
+            ]
+            for cluster_path, pool_updates in self.machine_pool_updates.items()
+        }
+
     def process(self, gitlab_cli: GitLabApi) -> None:
         yaml = create_ruamel_instance(explicit_start=True, width=4096)
         changes = False
-        for cluster_path, machine_pool_updates in self.machine_pool_updates.items():
+        for (
+            cluster_path,
+            machine_pool_updates,
+        ) in self.normalized_machine_pool_updates().items():
             cluster_fs_path = f"data{cluster_path}"
             if not machine_pool_updates:
                 continue
@@ -45,7 +51,7 @@ def process(self, gitlab_cli: GitLabApi) -> None:
             )
             content = yaml.load(raw_file.decode())
             if "machinePools" not in content:
-                self.cancel("machinePools missing. Nothing to do.")
+                self.cancel(f"{cluster_fs_path} misses machinePools. Nothing to do.")
 
             for machine_pool in machine_pool_updates:
                 for mp in content["machinePools"]:
@@ -57,7 +63,7 @@ def process(self, gitlab_cli: GitLabApi) -> None:
                 yaml.dump(content, stream)
                 new_content = stream.getvalue()
 
-            msg = f"update cluster {content['name']} spec fields"
+            msg = f"update cluster {cluster_fs_path} machine-pool fields"
             gitlab_cli.update_file(
                 branch_name=self.branch,
                 file_path=cluster_fs_path,
@@ -67,3 +73,7 @@ def process(self, gitlab_cli: GitLabApi) -> None:
 
         if not changes:
             self.cancel("Clusters are up to date. Nothing to do.")
+
+
+def clean_none_values_from_dict(d: dict[str, Any]) -> dict[str, Any]:
+    return {k: v for k, v in d.items() if v is not None}