Deflake tracing tests

[ashishb-solo]

Description

See issue link for flake details. This pull request attempts to address a flake by adding retries to one particular request that is resulting in failures.

Additionally, the test output was a bit hard to read, so this pull request makes two changes to address that:

1. Add the -s flag to avoid progress bars appearing in the curl stderr

2. Add some newlines that were missing from a few printf statements

API changes

Nee

Code changes

Add retry logic in the test and some print statements

CI changes

Nil

Docs changes

None

Context

Issue link has information on the test flakes

Interesting decisions

I decided to add the retry logic to the failing requests only. We could added the retry attempts in this block to preempt other flakes like this one from occurring, but that could also have an adverse effect on other tests and feels like a bad separation of privileges, so I preferred not going with that solution. Happy to change my mind here though.

Testing steps

See issue link

Notes for reviewers

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works

[solo-changelog-bot]

Issues linked to changelog:
solo-io#10327

[github-actions]

Visit the preview URL for this PR (updated for commit 738b661):

https://gloo-edge--pr10328-deflake-tracing-test-4k7yuir8.web.app

_{(expires Wed, 20 Nov 2024 20:41:26 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 77c2b86e287749579b7ff9cadb81e099042ef677}

[sam-heilbron]

lgtm

[sam-heilbron]

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?

[ashishb-solo]

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.

[ashishb-solo]

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.

[sam-heilbron]

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?

[sam-heilbron]

makes sense to me! If this is a useful starting point I think we add it for this test. If we find it effective, I would argue that we shoudl consider approaching a standard that other tests can use as well

[sam-heilbron]

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

[jenshu]

@ashishb-solo are you still working on this or can it be closed?