Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not exit NSG monitor when unable to get subnet #3372

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

tsatam
Copy link
Collaborator

@tsatam tsatam commented Jan 26, 2024

Which issue this PR addresses:

Proposed behavioral change for the refactor performed in #3358

Original Jiras for feature: ARO-4892 and ARO-4967

What this PR does / why we need it:

In the NSG monitor, we currently exit early if we encounter an error when trying to retrieve a subnet. This PR changes the behavior to continue trying to evaluate the subnets and to ignore ones with errors.

The implementation as-is was causing flaky test failures, as the order of subnet evaluation was not consistent, leading to mocked Subnet GET calls not always occurring as expected.

Test plan for issue:

  • Existing unit tests continue to pass
    • were also run multiple times to ensure the flake is gone

Is there any documentation that needs to be updated for this PR?

no

@tsatam
Copy link
Collaborator Author

tsatam commented Jan 26, 2024

@nwnt I'd like to get your review on this approach and if this makes sense to do within the monitor, or if we should evaluate a different approach to handle this test flake.

@nwnt
Copy link
Contributor

nwnt commented Jan 29, 2024

@tsatam I think this is definitely better than it currently is. Thank you very much for the improvement.

@tsatam tsatam added the loki label Jan 30, 2024
Copy link
Collaborator

@mociarain mociarain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SudoBrendan SudoBrendan merged commit 9c6eab8 into Azure:master Feb 1, 2024
17 checks passed
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.

5 participants