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

aws: fix test flake when no IMDS can be found #35633

Merged
merged 3 commits into from
Aug 9, 2024

Conversation

nbaws
Copy link
Contributor

@nbaws nbaws commented Aug 8, 2024

Commit Message: aws: fix test flake when no IMDS can be found
Additional Description: Lack of IMDS (169.254.169.254 address) can cause a race condition and crash during testing due to the shutdown of cluster manager. This scenario should not occur normally, as cluster manager will still exist and the lack of IMDS is handled gracefully.
Risk Level: Low
Testing: N/A
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Nigel Brittain <[email protected]>
@nbaws nbaws requested a review from mattklein123 as a code owner August 8, 2024 03:58
Signed-off-by: Nigel Brittain <[email protected]>
Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

Just to make sure I understand, you had a manual test where this crashed?

There is no CI flaky test that this was tripping?

/wait

@@ -52,6 +52,12 @@ class MetadataFetcherImpl : public MetadataFetcher,
ASSERT(!request_);
complete_ = false;
receiver_ = makeOptRef(receiver);

// Stop processing if we are shutting down
if (cm_.isShutdown()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test case emulates shutdown, as well as the nullptr that would have crashed it had the fix not been in place

@nbaws
Copy link
Contributor Author

nbaws commented Aug 8, 2024

Just to make sure I understand, you had a manual test where this crashed?

There is no CI flaky test that this was tripping?

/wait

The test case would crash when 169.254.169.254 was not available on the host. Unfortunately, where the code was written and also the CI system has this available, leading to the test success. This crash occurs during envoy shutdown in the test case where there is still a request pending, which is now handled by this fix.

I'll write a test case to exercise this scenario.

Signed-off-by: Nigel Brittain <[email protected]>
Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

Thank you.

@KBaichoo KBaichoo merged commit a298895 into envoyproxy:main Aug 9, 2024
50 checks passed
asingh-g pushed a commit to asingh-g/envoy that referenced this pull request Aug 20, 2024
Commit Message: aws: fix test flake when no IMDS can be found
Additional Description: Lack of IMDS (169.254.169.254 address) can cause
a race condition and crash during testing due to the shutdown of cluster
manager. This scenario should not occur normally, as cluster manager
will still exist and the lack of IMDS is handled gracefully.

Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: asingh-g <[email protected]>
dcillera pushed a commit to dcillera/envoy-1 that referenced this pull request Sep 10, 2024
Commit Message: aws: fix test flake when no IMDS can be found
Additional Description: Lack of IMDS (169.254.169.254 address) can cause
a race condition and crash during testing due to the shutdown of cluster
manager. This scenario should not occur normally, as cluster manager
will still exist and the lack of IMDS is handled gracefully.

Signed-off-by: Nigel Brittain <[email protected]>
dcillera pushed a commit to dcillera/envoy-1 that referenced this pull request Sep 10, 2024
Commit Message: aws: fix test flake when no IMDS can be found
Additional Description: Lack of IMDS (169.254.169.254 address) can cause
a race condition and crash during testing due to the shutdown of cluster
manager. This scenario should not occur normally, as cluster manager
will still exist and the lack of IMDS is handled gracefully.

Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
phlax pushed a commit that referenced this pull request Sep 10, 2024
Commit Message: aws: fix test flake when no IMDS can be found
Additional Description: Lack of IMDS (169.254.169.254 address) can cause
a race condition and crash during testing due to the shutdown of cluster
manager. This scenario should not occur normally, as cluster manager
will still exist and the lack of IMDS is handled gracefully.

Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Dario Cillerai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants