Fix Test Flakiness by adding short sleep in TestMetricsRefresh

[LukeAVanDrie]

This PR addresses flakiness observed in the TestMetricsRefresh test within the pkg/epp/backend/metrics package.

Problem

The root cause of the flakiness was a race condition stemming from the asynchronous nature of the StopRefreshLoop method. The test would:

1. Signal the metrics refresh goroutine to stop via StopRefreshLoop().
2. Immediately update the mock PodMetricsClient to return different metric values.
3. Assert that the PodMetrics instance still held the original metrics.

Occasionally, the refresh goroutine would execute one final time after StopRefreshLoop() was called but before it fully terminated, picking up the new metrics. This led to the assertion failing.

Solution

To resolve this, I added a time.Sleep(pmf.refreshMetricsInterval * 2 /* small buffer for robustness */) before any test assertions are made. This causes the test to sleep for just 2ms. I also added a stopOnce around closing the done channel for robustness.

[k8s-ci-robot]

Hi @LukeAVanDrie. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[netlify]

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	5e482aa
🔍 Latest deploy log	https://app.netlify.com/sites/gateway-api-inference-extension/deploys/6823b93e719617000835a30c
😎 Deploy Preview	https://deploy-preview-824--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[kfswain]

Fixes: #719

[kfswain]

/ok-to-test

[nirrozenbaum]

I agree with @liu-cong's general point, making this function synchronous doesn't sound to me like the right solution.
thinking forward about data layer and extensible scrapers, we might need to run multiple routines for different scrapers.
I wouldn't touch the existing pod metrics code. this is an issue with the test itself.
@LukeAVanDrie your analysis of the issue is great:

1. Signal the metrics refresh goroutine to stop via StopRefreshLoop().
2. Immediately update the mock PodMetricsClient to return different metric values.
3. Assert that the PodMetrics instance still held the original metrics.

Occasionally, the refresh goroutine would execute one final time after StopRefreshLoop() was called but before it fully terminated, picking up the new metrics. This led to the assertion failing.

I think a very easy and straight forward fix to the above problem is to let the test wait until the final call is completed.
taking the scraping timeout as a worst case is sufficient.
so I would fix the flakiness by adding this single code line in the test file:

pm.StopRefreshLoop()
time.Sleep(fetchMetricsTimeout) // THIS IS THE ONLY ADDED LINE
pmc.SetRes(map[types.NamespacedName]*MetricsState{namespacedName: updated})

[liu-cong]

Nir's suggestion makes sense. Though in theory we could still run into a flakiness due to the async nature, adding that timeout should reduce flakiness to near zero in practice I believe.

…

On Tue, May 13, 2025 at 12:34 AM Nir Rozenbaum ***@***.***> wrote: *nirrozenbaum* left a comment (kubernetes-sigs/gateway-api-inference-extension#824) <#824 (comment)> I agree with @liu-cong <https://github.com/liu-cong>'s general point, making this function synchronous doesn't sound to me like the right solution. thinking forward about data layer and extensible scrapers, we might need to run multiple routines for different scrapers. I wouldn't touch the existing pod metrics code. this is an issue with the test itself. @LukeAVanDrie <https://github.com/LukeAVanDrie> your analysis of the issue is great: 1. Signal the metrics refresh goroutine to stop via StopRefreshLoop(). 2. Immediately update the mock PodMetricsClient to return different metric values. 3. Assert that the PodMetrics instance still held the original metrics. Occasionally, the refresh goroutine would execute one final time after StopRefreshLoop() was called but before it fully terminated, picking up the new metrics. This led to the assertion failing. I think a very easy and straight forward fix to the above problem is to let the test wait until the final call is completed. taking the scraping timeout as a worst case is sufficient. so I would fix the bug by adding this single code line in the test file: pm.StopRefreshLoop()time.Sleep(fetchMetricsTimeout) // THIS IS THE ONLY ADDED LINEpmc.SetRes(map[types.NamespacedName]*MetricsState{namespacedName: updated}) — Reply to this email directly, view it on GitHub <#824 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABUVECT6YT2YXAXQKOPUYEL26GOA5AVCNFSM6AAAAAB467IC3WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNZVGM3TOMZSGQ> . You are receiving this because you were mentioned.Message ID: <kubernetes-sigs/gateway-api-inference-extension/pull/824/c2875377324@ github.com>

[kfswain]

time.Sleep(fetchMetricsTimeout) // THIS IS THE ONLY ADDED LINE

this is what i was going to suggest. This solves the problem of the test flake and if we want to change how the metrics loop is handled we can do that later. For the test: we can even add another second as a grace period which would basically reduce the probability of race conditions to zero, even though it will already be very very low

[LukeAVanDrie]

I think a very easy and straight forward fix to the above problem is to let the test wait until the final call is completed. taking the scraping timeout as a worst case is sufficient. so I would fix the flakiness by adding this single code line in the test file:

pm.StopRefreshLoop()
time.Sleep(fetchMetricsTimeout) // THIS IS THE ONLY ADDED LINE
pmc.SetRes(map[types.NamespacedName]*MetricsState{namespacedName: updated})

I incorporated this change and removed the wait group. I did, however, leave the stopOnce for robustness, but I can also remove this if you feel it is unnecessary.

[liu-cong]

/lgtm

[kfswain]

/approve

Thanks!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kfswain, LukeAVanDrie

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [kfswain]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[LukeAVanDrie]

Resolves #719

@kfswain I cannot seem to get this to work.

[kfswain]

Ah, yeah no problem! Most of our slash commands are a signal for the k8s-ci-robot to take some sort of action. the fixes or resolves keyword is a GH concept so no slash: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue

Either way, I manually closed the issue, thanks for doin this