Fix(cv_deploy): Properly clean-up unused AVD-managed container assignments (#6650)

Co-authored-by: Guillaume Mulocher <gmulocher@arista.com>

Author: carl-baillargeon

ansible_collections/arista/avd/plugins/action/cv_workflow.py

@@ -237,7 +237,7 @@ async def deploy(self, validated_args: dict, result: dict) -> dict:
                 result_object.deployed_interface_tags,
                 result_object.deployed_cv_pathfinder_metadata,
                 result_object.removed_configs,
-                result_object.removed_static_config_root_containers,
+                result_object.removed_static_config_containers,
                 result_object.removed_static_config_configlets,
                 result_object.removed_device_tags,
                 result_object.removed_interface_tags,

python-avd/pyavd/_cv/workflows/deploy_static_config_studio_manifest_to_cv.py

@@ -44,13 +44,8 @@ async def deploy_static_config_studio_manifest_to_cv(manifest: AvdManifest, depl
 
     # Perform synchronization tasks.
     await _sync_configlets(cv_manifest=cv_manifest, deployment_result=deployment_result, cv_client=cv_client)
-    existing_containers_by_id = await _sync_containers(cv_manifest=cv_manifest, deployment_result=deployment_result, cv_client=cv_client)
-    await _sync_studio_roots(
-        cv_manifest=cv_manifest,
-        deployment_result=deployment_result,
-        cv_client=cv_client,
-        existing_containers_by_id=existing_containers_by_id,
-    )
+    await _sync_containers(cv_manifest=cv_manifest, deployment_result=deployment_result, cv_client=cv_client)
+    await _sync_studio_roots(cv_manifest=cv_manifest, deployment_result=deployment_result, cv_client=cv_client)
 
     # Done.
     LOGGER.info("deploy_static_config_studio_manifest_to_cv: Completed manifest deployment for workspace '%s'.", workspace_id)
@@ -83,8 +78,21 @@ async def _sync_containers(cv_manifest: CVManifest, deployment_result: DeployToC
     else:
         LOGGER.info("deploy_static_config_studio_manifest_to_cv: No container creations or updates are needed.")
 
-    # Return all existing containers from CloudVision to be further processed if needed.
-    return existing_containers_by_id
+    # Delete unused AVD-managed containers.
+    desired_container_ids = {container.id for container in cv_manifest.containers}
+    containers_to_delete = {
+        container_id: cast("str", container.display_name)
+        for container_id, container in existing_containers_by_id.items()
+        if container_id.startswith(AVD_ENTITY_PREFIX) and container_id not in desired_container_ids
+    }
+
+    if containers_to_delete:
+        LOGGER.info("deploy_static_config_studio_manifest_to_cv: Removing %d AVD-managed containers which are no longer used.", len(containers_to_delete))
+        deployment_result.removed_static_config_containers.extend(containers_to_delete.values())
+        # TODO: Build a 'delete_configlet_containers' gRPC API
+        await gather(*[cv_client.delete_configlet_container(workspace_id=workspace_id, assignment_id=container_id) for container_id in containers_to_delete])
+    else:
+        LOGGER.info("deploy_static_config_studio_manifest_to_cv: No AVD-managed container deletions are needed.")
 
 
 async def _sync_configlets(cv_manifest: CVManifest, deployment_result: DeployToCvResult, cv_client: CVClient) -> None:
@@ -117,11 +125,9 @@ async def _sync_configlets(cv_manifest: CVManifest, deployment_result: DeployToC
         LOGGER.info("deploy_static_config_studio_manifest_to_cv: No AVD-managed configlet deletions are needed.")
 
 
-async def _sync_studio_roots(
-    cv_manifest: CVManifest, deployment_result: DeployToCvResult, cv_client: CVClient, existing_containers_by_id: dict[str, ConfigletAssignment]
-) -> None:
+async def _sync_studio_roots(cv_manifest: CVManifest, deployment_result: DeployToCvResult, cv_client: CVClient) -> None:
     """
-    Synchronize Studio root containers. Update root container assignments and delete unused AVD-managed ones.
+    Synchronize Studio root containers. Update root container assignments.
 
     Note:
         During an update, this function reorders root containers. All AVD-managed
@@ -139,17 +145,14 @@ async def _sync_studio_roots(
         default_value=[],
     )
 
-    # Calculate which desired roots are missing and which existing AVD-managed roots are stale.
+    # Calculate which desired roots are missing.
     desired_root_ids = [container.id for container in cv_manifest.containers if container.is_root]
     desired_root_ids_set = set(desired_root_ids)
     existing_root_ids_set = set(existing_root_ids)
     missing_ids = desired_root_ids_set - existing_root_ids_set
-    stale_avd_ids = {
-        container_id for container_id in existing_root_ids_set if container_id.startswith(AVD_ENTITY_PREFIX) and container_id not in desired_root_ids_set
-    }
 
     # Update the Studio root container list if necessary, preserving any manually added (non-AVD) root containers.
-    if missing_ids or stale_avd_ids:
+    if missing_ids:
         LOGGER.info("deploy_static_config_studio_manifest_to_cv: Updating Studio root container assignment list...")
         manual_ids = [container_id for container_id in existing_root_ids if not container_id.startswith(AVD_ENTITY_PREFIX)]
         new_ordered_ids = desired_root_ids + manual_ids
@@ -162,19 +165,3 @@ async def _sync_studio_roots(
         )
     else:
         LOGGER.info("deploy_static_config_studio_manifest_to_cv: Studio root container assignments are already in the desired state.")
-
-    # Delete stale AVD-managed root containers that are no longer needed.
-    if stale_avd_ids:
-        LOGGER.info("deploy_static_config_studio_manifest_to_cv: Removing %d stale AVD-managed root containers...", len(stale_avd_ids))
-        deployment_result.removed_static_config_root_containers.extend(
-            [
-                cast("str", existing_container.display_name)
-                for container_id in stale_avd_ids
-                if (existing_container := existing_containers_by_id.get(container_id)) is not None
-            ]
-        )
-
-        # TODO: Build a 'delete_configlet_containers' gRPC API
-        await gather(*[cv_client.delete_configlet_container(workspace_id=workspace_id, assignment_id=container_id) for container_id in stale_avd_ids])
-    else:
-        LOGGER.info("deploy_static_config_studio_manifest_to_cv: No AVD-managed root container deletions are needed.")

python-avd/pyavd/_cv/workflows/models.py

@@ -187,7 +187,7 @@ class DeployToCvResult:
     skipped_interface_tags: list[CVInterfaceTag] = field(default_factory=list)
     skipped_cv_pathfinder_metadata: list[CVPathfinderMetadata] = field(default_factory=list)
     removed_configs: list[str] = field(default_factory=list)
-    removed_static_config_root_containers: list[str] = field(default_factory=list)
+    removed_static_config_containers: list[str] = field(default_factory=list)
     removed_static_config_configlets: list[str] = field(default_factory=list)
     removed_device_tags: list[CVDeviceTag] = field(default_factory=list)
     removed_interface_tags: list[CVInterfaceTag] = field(default_factory=list)

python-avd/tests/pyavd/cv/workflows/test_deploy_static_config_studio_manifest_to_cv.py

@@ -98,7 +98,7 @@ async def test_initial_deployment(self, mock_cv_client: MagicMock, avd_initial_m
         assert len(deployment_result.deployed_static_config_containers) == 3
         assert not deployment_result.skipped_static_config_containers
         assert not deployment_result.removed_static_config_configlets
-        assert not deployment_result.removed_static_config_root_containers
+        assert not deployment_result.removed_static_config_containers
 
     async def test_no_changes_run(self, mock_cv_client: MagicMock, avd_initial_manifest: AvdManifest, deployment_result: DeployToCvResult) -> None:
         """Test a subsequent run where the AVD manifest has not changed."""
@@ -228,7 +228,7 @@ async def test_updates_and_removals(self, mock_cv_client: MagicMock, deployment_
         assert len(deployment_result.deployed_static_config_containers) == 2
         assert len(deployment_result.skipped_static_config_containers) == 2  # ROOT and CNT_LEAF2 were skipped
         assert deployment_result.removed_static_config_configlets == ["CF_UNUSED"]
-        assert deployment_result.removed_static_config_root_containers == ["UNUSED_ROOT"]
+        assert deployment_result.removed_static_config_containers == ["UNUSED_ROOT"]
 
     async def test_root_container_reordering_and_manual_preservation(self, mock_cv_client: MagicMock, deployment_result: DeployToCvResult) -> None:
         """Test reordering AVD root containers, deleting a stale one and preserving a manually-added root container."""
@@ -277,4 +277,50 @@ async def test_root_container_reordering_and_manual_preservation(self, mock_cv_c
         # Verify deployment result object.
         assert len(deployment_result.deployed_static_config_containers) == 1
         assert len(deployment_result.skipped_static_config_containers) == 1  # AVD_ROOT2 was skipped
-        assert deployment_result.removed_static_config_root_containers == ["AVD_ROOT1"]
+        assert deployment_result.removed_static_config_containers == ["AVD_ROOT1"]
+
+    async def test_non_root_container_deletion(self, mock_cv_client: MagicMock, deployment_result: DeployToCvResult) -> None:
+        """Test that non-root (child) containers are properly deleted when removed from the manifest."""
+        # Initial state on CV.
+        cf_leaf1_id = generate_id("CF_LEAF1")
+        root_id, cnt_leaf1_id, cnt_leaf2_id = generate_id("ROOT"), generate_id("ROOT/CNT_LEAF1"), generate_id("ROOT/CNT_LEAF2")
+
+        existing_containers = [
+            create_grpc_container(container_id=root_id, name="ROOT", description="Root container", query="device:*", child_ids=[cnt_leaf1_id, cnt_leaf2_id]),
+            create_grpc_container(
+                container_id=cnt_leaf1_id, name="CNT_LEAF1", description="LEAF1 container", query="device:LEAF1", configlet_ids=[cf_leaf1_id]
+            ),
+            create_grpc_container(container_id=cnt_leaf2_id, name="CNT_LEAF2", description="LEAF2 container", query="device:LEAF2"),
+        ]
+
+        mock_cv_client.get_configlet_containers.return_value = existing_containers
+        mock_cv_client.get_configlets.return_value = []
+        mock_cv_client.get_studio_inputs_with_path.return_value = [root_id]
+
+        # New desired state from AVD: ROOT with only CNT_LEAF1, CNT_LEAF2 is removed from the manifest.
+        cfl1 = AvdConfiglet(name="CF_LEAF1", file=Path("/path/to/cfl1.cfg"))
+        cnt_leaf1 = AvdContainer(name="CNT_LEAF1", tag_query="device:LEAF1", description="LEAF1 container", configlets=(cfl1.name,))
+        root_container = AvdContainer(name="ROOT", tag_query="device:*", description="Root container", sub_containers=(cnt_leaf1,))
+
+        updated_manifest = AvdManifest(configlets=(cfl1,), containers=(root_container,))
+
+        await deploy_static_config_studio_manifest_to_cv(updated_manifest, deployment_result, mock_cv_client)
+
+        # Verify the non-root child container was deleted.
+        mock_cv_client.delete_configlet_container.assert_called_once_with(workspace_id=deployment_result.workspace.id, assignment_id=cnt_leaf2_id)
+
+        # Verify ROOT container was updated because child_ids changed.
+        mock_cv_client.set_configlet_containers.assert_called_once()
+        pushed_containers = mock_cv_client.set_configlet_containers.call_args[1]["containers"]
+        assert len(pushed_containers) == 1
+        assert pushed_containers[0][1] == "ROOT"
+
+        # Studio roots should NOT be updated as they haven't changed.
+        mock_cv_client.set_studio_inputs.assert_not_called()
+
+        # Verify deployment result object.
+        assert len(deployment_result.deployed_static_config_configlets) == 1
+        assert len(deployment_result.deployed_static_config_containers) == 1  # ROOT was updated
+        assert len(deployment_result.skipped_static_config_containers) == 1  # CNT_LEAF1 was skipped
+        assert deployment_result.removed_static_config_containers == ["CNT_LEAF2"]
+        assert not deployment_result.removed_static_config_configlets