diff --git a/reconcile/ocm_machine_pools.py b/reconcile/ocm_machine_pools.py index fc2a7efb08..6b499e3705 100644 --- a/reconcile/ocm_machine_pools.py +++ b/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) diff --git a/reconcile/test/test_ocm_machine_pools.py b/reconcile/test/test_ocm_machine_pools.py index 2a6ecf0497..b5bcef97ae 100644 --- a/reconcile/test/test_ocm_machine_pools.py +++ b/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", ), - ) - ], + ] + }, ) diff --git a/reconcile/test/utils/test_mr_machine_pool_updates.py b/reconcile/test/utils/test_mr_machine_pool_updates.py new file mode 100644 index 0000000000..46ce7b7f0b --- /dev/null +++ b/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() diff --git a/reconcile/utils/mr/cluster_machine_pool_updates.py b/reconcile/utils/mr/cluster_machine_pool_updates.py index 0d36468879..bc26238001 100644 --- a/reconcile/utils/mr/cluster_machine_pool_updates.py +++ b/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}