[TestFlexCounter] Use flag state rather than non-atomic counter for bulkChunksize test#1986
Open
justin-wong-ce wants to merge 1 commit into
Open
[TestFlexCounter] Use flag state rather than non-atomic counter for bulkChunksize test#1986justin-wong-ce wants to merge 1 commit into
justin-wong-ce wants to merge 1 commit into
Conversation
The existing `initialCheckCount` is not a reliable way to determine the state of the test-mocked configuration. This causes many racey conditions and makes the test flaky. An example would be a poll occuring in between test iterations where the `chunkSize` is being changed/ Swapping to use a callback-based flag to explictly set when the configuration is fully applied and when it is not. Signed-off-by: Justin Wong <jvwong@arista.com>
justin-wong-ce
changed the title
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
justin-wong-ce
changed the title
justin-wong-ce
changed the title
justin-wong-ce
changed the title
Contributor
Author
|
Unrelated CI fail: |
Contributor
Author
|
/azpw retry |
Collaborator
|
Retrying failed(or canceled) jobs... |
Collaborator
|
Retrying failed(or canceled) stages in build 1161819: ✅Stage TestAsan:
✅Stage Test:
|
Contributor
Author
|
/azpw retry |
Collaborator
|
Retrying failed(or canceled) jobs... |
Collaborator
|
Retrying failed(or canceled) stages in build 1161819: ✅Stage TestAsan:
✅Stage Test:
|
theasianpianist
approved these changes
Jul 16, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the FlexCounter.bulkChunksize unit test to reduce flakiness by replacing the initialCheckCount-based “initialization vs runtime” heuristic with an explicit phase signal driven by a callback from the shared testAddRemoveCounter helper.
Changes:
- Add an optional
onSetupCompletecallback totestAddRemoveCounterand invoke it once the test’s FlexCounter/plugins/counters setup is complete. - Replace
initialCheckCountlogic inFlexCounter.bulkChunksizewith aruntimeReadyphase flag used by themock_bulkGetStatsimplementation. - Adjust bulk chunk size assertions to align with the new phase gating.
| uint32_t unifiedBulkChunkSize = 0; | ||
| int32_t initialCheckCount; | ||
| int32_t partialSupportingBulkObjectFactor; | ||
| bool runtimeReady = false; |
Comment on lines
1578
to
1582
| if (unifiedBulkChunkSize > 0) | ||
| { | ||
| if (object_count != unifiedBulkChunkSize | ||
| && number_of_counters == allCounters.size() | ||
| && object_count > 1) | ||
| { | ||
| EXPECT_EQ(object_count, unifiedBulkChunkSize); | ||
| } | ||
| continue; | ||
| } | ||
| // Skip re-probe polls (single-object capability probes | ||
| // that happen after the merge-back, with object_count==1). | ||
| // The per-counter assertions below check steady-state | ||
| // per-prefix chunk sizes, which only apply when | ||
| // object_count > 1. This matches the documented intent | ||
| // in the comment above and the unified-path guard. | ||
| if (object_count == 1) | ||
| { | ||
| EXPECT_EQ(object_count, unifiedBulkChunkSize); | ||
| continue; | ||
| } |
| // non zero unifiedBulkChunkSize indicates all counter IDs share the same bulk chunk size | ||
| uint32_t unifiedBulkChunkSize = 0; | ||
| int32_t initialCheckCount; | ||
| int32_t partialSupportingBulkObjectFactor; |
Contributor
|
@justin-wong-ce please review the copilot comments, esp the one about |
Contributor
Author
Ack, will be looking at this soon |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of PR
Summary:
The existing
initialCheckCountis not a reliable way to determine the state of the test-mocked configuration. This causes race conditions and makes the test flaky.An example would be a poll occuring in between test iterations where the
chunkSizeis being changed.Swapping to use a callback-based flag to explictly set when the configuration is fully applied and when it is not.
Fixes #1978
Type of change
Approach
What is the motivation for this PR?
FlexConuter.bulkChunksizeis flaky, causing many PR CIs to be flaky.Work item tracking
How did you do it?
Use a callback-flag mechanism to so that the test knows what to expect and what checks to run based on an explicitly known state.
How did you verify/test it?
Tests fully pass with
./tests --gtest_filter=FlexCounter.*-*addRemoveDashMeterCounter* --gtest_break_on_failure --gtest_repeat=100.Excluding the
addRemoveDashMeterCountertest since it is known to be flaky (#1985).Any platform specific information?
Documentation