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.

Sign up for GitHub

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

Deflake tracing tests #10328

Closed
wants to merge 11 commits into from
Closed

Deflake tracing tests #10328

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0d3bd0c
Add some diagnostic improvements
ashishb-solo Nov 12, 2024
d878d79
Add retry logic to the flaky test
ashishb-solo Nov 12, 2024
4c67867
Add changelog
ashishb-solo Nov 12, 2024
3c20fe8
Fix changelog issue link
ashishb-solo Nov 12, 2024
a93c427
Merge main into deflake-tracing-tests
soloio-bulldozer[bot] Nov 12, 2024
5a8a471
Merge refs/heads/main into deflake-tracing-tests
soloio-bulldozer[bot] Nov 12, 2024
d88155b
Merge refs/heads/main into deflake-tracing-tests
soloio-bulldozer[bot] Nov 13, 2024
81995f1
Adding changelog file to new location
Nov 13, 2024
a5f2cf5
Deleting changelog file from old location
Nov 13, 2024
82785b3
Fix published Helm docs from main branch (#10334)
sheidkamp Nov 13, 2024
738b661
Merge refs/heads/main into deflake-tracing-tests
soloio-bulldozer[bot] Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions changelog/v1.18.0-beta35/deflake-tracing-tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
changelog:
- type: NON_USER_FACING
description: >-
Add some extra spacing to print statements to make them easier to read.
- type: NON_USER_FACING
issueLink: https://github.com/solo-io/gloo/issues/10327
resolvesIssue: false
description: >-
Fix a flaky test by adding retry logic.
1 change: 1 addition & 0 deletions pkg/utils/kubeutils/kubectl/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ func (c *Cli) CurlFromPod(ctx context.Context, podOpts PodExecOptions, options .
"--max-time",
"5",
}, curlArgs...)
fmt.Printf("Executing curl command: %s\n", args)
ashishb-solo marked this conversation as resolved.
Show resolved Hide resolved

stdout, stderr, err := c.ExecuteOn(ctx, c.kubeContext, args...)

Expand Down
8 changes: 8 additions & 0 deletions test/kubernetes/e2e/features/tracing/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,14 @@ func (s *testingSuite) TestSpanNameTransformationsWithoutRouteDecorator() {
})),
curl.WithHostHeader(testHostname),
curl.WithPort(gatewayProxyPort),
// this request sometimes times out, so let's add some retries
Copy link
Contributor

Choose a reason for hiding this comment

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

What is unique about this test that means it benefits from a retry? If there is something unique that would be good to callout. If other tests are susceptible, would we be better adding this as a default to the AssertEventuallyConsistentCurlResponse function instead and letting callers who don't want retries opt out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since my guess is that this is a timing issue, i think really only speculate here, but here are a few possible reasons:

  1. we define a non-default gateway - maybe this is causing some sort of translation delay.

  2. we also define a new service that routes to the separate port on the new gateway. maybe that service needs some kind of "warm up" time or something. or possibly there's some sort of timing issue or race condition in kubernetes' routing logic.

  3. something to do wih the opentelemetry collector, possibly having to do with verifying the presense of the upstream. this was something david suggested when we were discussing this initially.

all of these possibilities sound a little ridiculous to me to be honest. i could spend more time investigating this to find the root cause, but i don't feel like it's worth the effort given how difficult it is to reproduce this at this point. and since i've gone through the effort of pausing the cluster after the failure and demonstrating that things are still in a consistent state and subsequent requests are working, i think it's safe to just add some retries here since that's more or less what i was doing when i reproduced the issue locally.

there is one alternate approach that we could consider here, and that would be adding a small delay in the AssertEventuallyConsistentCurlResponse call that we're using in the test. that would preempt additional flakes that could hypothetically be affected by this same issue. if you think that's a better approach, then i'm happy to do that instead.

finally, i think it's also worth considering that we can't really know what fixes the flake until we try merging something and observing whether this flake continues to occur. so i'd like to try something, but i'm happy to continue discussion on whatever we think is the best thing to try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would we be better adding this as a default to the AssertEventuallyConsistentCurlResponse function instead and letting callers who don't want retries opt out?

this is something that i did consider. personally, i don't particularly like it when i call a function with a set of options and then observe the function changing said options under my feet. but there is a benefit of preempting this flake a occurring again, so i'd feel more comfortable doing that in one of the following two ways:

  1. add a delay in AssertEventuallyConsistentCurlResponse itself

  2. consider making AssertEventuallyConsistentCurlResponse slightly less strict about its requirements. for example, maybe we could make the Consistently block try again a few times, or replace it with something like "eventually, we get 5 successful requests in a row" or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure I understand the "delay" idea. Isn't the goal of first having an eventually, is that that operates as the delay and continues iterating until it's ready.

As for reducing the strict requirements, I agree, we may need to come up with ways to ensure this works. In my mind, curl retries were the best way to do this. What do you think wrapping the consistently in an eventually block will help test, that having retries into our requests would not?

// https://github.com/solo-io/gloo/actions/runs/11773028593/job/32789301648
// we have not experienced this in
// TestSpanNameTransformationsWithRouteDecorator, so leave it out
// from there for now
curl.WithRetries(3, 1, 10),
curl.WithPath(pathWithoutRouteDescriptor),
curl.Silent(),
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's valuable for this test, should we make the silent argument a default?

},
&matchers.HttpResponse{
StatusCode: http.StatusOK,
Expand All @@ -150,6 +157,7 @@ func (s *testingSuite) TestSpanNameTransformationsWithRouteDecorator() {
curl.WithHostHeader("example.com"),
curl.WithPort(gatewayProxyPort),
curl.WithPath(pathWithRouteDescriptor),
curl.Silent(),
},
&matchers.HttpResponse{
StatusCode: http.StatusOK,
Expand Down
6 changes: 3 additions & 3 deletions test/kubernetes/testutils/assertions/curl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (p *Provider) AssertEventualCurlReturnResponse(
//nolint:bodyclose // The caller of this assertion should be responsible for ensuring the body close - if the response is not needed for the test, AssertEventualCurlResponse should be used instead
curlHttpResponse = transforms.WithCurlResponse(curlResponse)
g.Expect(curlHttpResponse).To(matchers.HaveHttpResponse(expectedResponse))
fmt.Printf("success: %+v", curlResponse)
fmt.Printf("success: %+v\n\n\n", curlResponse)
}).
WithTimeout(currentTimeout).
WithPolling(pollingInterval).
Expand Down Expand Up @@ -93,7 +93,7 @@ func (p *Provider) AssertCurlReturnResponse(
// Do the transform in a separate step instead of a WithTransform to avoid having to do it twice
curlHttpResponse := transforms.WithCurlResponse(curlResponse)
Expect(curlHttpResponse).To(matchers.HaveHttpResponse(expectedResponse))
fmt.Printf("success: %+v", curlResponse)
fmt.Printf("success: %+v\n\n\n", curlResponse)

return curlHttpResponse
}
Expand Down Expand Up @@ -132,7 +132,7 @@ func (p *Provider) AssertEventuallyConsistentCurlResponse(

expectedResponseMatcher := WithTransform(transforms.WithCurlResponse, matchers.HaveHttpResponse(expectedResponse))
g.Expect(res).To(expectedResponseMatcher)
fmt.Printf("success: %+v", res)
fmt.Printf("success: %+v\n\n\n", res)
}).
WithTimeout(pollTimeout).
WithPolling(pollInterval).
Expand Down
Loading