Skip to content

bgp test: return error from upsertBGPCC#44545

Merged
joestringer merged 1 commit intocilium:mainfrom
liyihuang:pr/liyih/bgp-fix
Feb 27, 2026
Merged

bgp test: return error from upsertBGPCC#44545
joestringer merged 1 commit intocilium:mainfrom
liyihuang:pr/liyih/bgp-fix

Conversation

@liyihuang
Copy link
Copy Markdown
Contributor

@liyihuang liyihuang commented Feb 26, 2026

Changes upsertBGPCC helper function to return an error so it can handle EventuallyWith test case.

running go test -c ./operator/pkg/bgp && stress ./bgp.test for 5 mins now and dont see the issue anymore

5m0s: 312 runs so far, 0 failures, 12 active
5m5s: 317 runs so far, 0 failures, 12 active
5m10s: 323 runs so far, 0 failures, 12 active

Fixes: #44434

fix the race condition for the TestRouterIDAllocation bgp test case

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 26, 2026
@liyihuang liyihuang added area/CI Continuous Integration testing issue or flake area/bgp Impacts the Border Gateway Protocol feature. release-note/misc This PR makes changes that have no direct user impact. release-note/ci This PR makes changes to the CI. labels Feb 26, 2026
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 26, 2026
@liyihuang
Copy link
Copy Markdown
Contributor Author

/test

@liyihuang liyihuang marked this pull request as ready for review February 27, 2026 00:10
@liyihuang liyihuang requested a review from a team as a code owner February 27, 2026 00:10
@liyihuang liyihuang requested a review from rastislavs February 27, 2026 00:10
@rastislavs rastislavs removed the release-note/misc This PR makes changes that have no direct user impact. label Feb 27, 2026
Copy link
Copy Markdown
Contributor

@rastislavs rastislavs left a comment

Choose a reason for hiding this comment

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

Thanks!

Changes upsertBGPCC helper function to return an error so it can handle EventuallyWith test case.

Signed-off-by: Liyi Huang  <liyi.huang@isovalent.com>
@liyihuang
Copy link
Copy Markdown
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 27, 2026
if tt.FinalClusterConfigs != nil {
for _, clusterConfig := range tt.FinalClusterConfigs {
upsertBGPCC(req, ctx, f, clusterConfig)
if err := upsertBGPCC(ctx, f, clusterConfig); !assert.NoError(c, err) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: !assert.NoError() is pretty confusing due to the way different people interpret double negatives. Assertions should probably be unconditional too, or parameterized in the test if there's a legitimate error condition the test is looking for.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll go ahead and merge this as-is given the approval from @rastislavs but this seems like some minor tech debt we should consider improving on (also for all other instances in this testsuite, cc @cilium/sig-bgp ).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#44588 to clear this tech debt

@joestringer joestringer added this pull request to the merge queue Feb 27, 2026
Merged via the queue into cilium:main with commit dfef54a Feb 27, 2026
77 checks passed
liyihuang added a commit to liyihuang/cilium that referenced this pull request Mar 1, 2026
Change the double negatives based on the comment cilium#44545 (comment). This update affects the BGP test cases as well as ClusterMesh and IPAM components.

Signed-off-by: Liyi Huang <liyi.huang@isovalent.com>
liyihuang added a commit to liyihuang/cilium that referenced this pull request Mar 1, 2026
Change the double negatives based on the comment
cilium#44545 (comment). This
update affects the BGP test cases as well as ClusterMesh and IPAM
components based on search using assert.No

Signed-off-by: Liyi Huang <liyi.huang@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/bgp Impacts the Border Gateway Protocol feature. area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: operator/pkg/bgp: flake in TestRouterIDAllocation: the object has been modified; resource version mismatch

4 participants