Skip to content

Improve performance of AzureContainerInstanceHook.exists using direct container lookup#63567

Draft
SameerMesiah97 wants to merge 1 commit intoapache:mainfrom
SameerMesiah97:AzureContainterInstanceHook-Exists-Fix
Draft

Improve performance of AzureContainerInstanceHook.exists using direct container lookup#63567
SameerMesiah97 wants to merge 1 commit intoapache:mainfrom
SameerMesiah97:AzureContainterInstanceHook-Exists-Fix

Conversation

@SameerMesiah97
Copy link
Contributor

Description

This change improves the performance of AzureContainerInstanceHook.exists by replacing the current scan of all container groups in a resource group with a direct lookup using container_groups.get(resource_group, name). Instead of listing and iterating over all container groups, the hook now performs a single API call to determine whether the container group exists.

Rationale

Listing container groups can require pagination and multiple API calls as the number of resources in a resource group grows. Using the Azure SDK’s direct lookup endpoint avoids this overhead and aligns with the intended API usage.

Tests

Added/modified unit tests that verify that:

  • the hook returns True when container_groups.get succeeds for an existing container group
  • the hook returns False when container_groups.get raises ResourceNotFoundError
  • unexpected exceptions raised by container_groups.get are propagated rather than being swallowed

Backwards Compatibility

No change in external behavior. The method still returns True when the container group exists and False when it does not.

@SameerMesiah97 SameerMesiah97 requested a review from dabla as a code owner March 13, 2026 21:59
@SameerMesiah97 SameerMesiah97 force-pushed the AzureContainterInstanceHook-Exists-Fix branch from 7c49e52 to 7520c69 Compare March 14, 2026 12:44
@SameerMesiah97 SameerMesiah97 marked this pull request as draft March 14, 2026 15:54
@Vamsi-klu
Copy link
Contributor

Hey @SameerMesiah97, this looks good — replacing the list-and-scan with a direct .get() call is a clear win, and it follows the same pattern already used elsewhere in this provider (e.g., wasb.py for blob existence checks, key_vault.py for secrets).

One thing worth calling out that the PR description doesn't mention: the old test_exists_with_existing test was actually buggy. It asserted False even for the "exists" case, because ContainerGroup.name on the mock was None and never matched the lookup name. So the positive case was never really tested. Your new tests fix this — nice catch even if it was accidental.

A couple of small things:

  • Missing newsfragment — since this is a provider improvement, you'd probably want one at providers/microsoft/azure/newsfragments/63567.improvement.rst.

  • Minor: in test_exists_unexpected_exception, pytest.raises(Exception) is pretty broad. Adding match="Unexpected Exception" would make it more precise, though not a blocker.

Otherwise this is clean and ready to go from my perspective.

@SameerMesiah97 SameerMesiah97 force-pushed the AzureContainterInstanceHook-Exists-Fix branch from 7520c69 to e2666f5 Compare March 14, 2026 21:48
container_groups.get(resource_group, name) call. This avoids listing and
pagination when checking whether a container group exists.

Update tests to mock container_groups.get and cover success, not found, and error cases.
@SameerMesiah97 SameerMesiah97 force-pushed the AzureContainterInstanceHook-Exists-Fix branch from e2666f5 to 2a1b214 Compare March 14, 2026 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants