tests: avoid test port conflicts#10963
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/XL
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR moves MCS listener initialization earlier, records resolved bound addresses, updates advertise/name config from those values, normalizes empty test listen inputs, replaces temp URL allocation in integration tests, and refactors one cluster-ID test. ChangesCore server changes
Estimated code review effort: 4 (Complex) | ~45 minutes Cluster ID check test refactor
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rleungx
force-pushed
the
fix-test-port-conflicts
branch
from
e274147 to
45eba90
Compare
ti-chi-bot
Bot
added
release-note-none
rleungx
force-pushed
the
fix-test-port-conflicts
branch
from
45eba90 to
2bd14ff
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/mcs/server/server.go`:
- Around line 212-226: In buildActualListenAddr, the unspecified-host
normalization currently hardcodes 127.0.0.1 for both IPv4 and IPv6, which
changes the address family; update the host handling so
net.ParseIP(host).IsUnspecified() preserves the original family by mapping IPv6
unspecified addresses to ::1 and IPv4 unspecified addresses to 127.0.0.1. Keep
the fix localized to buildActualListenAddr and verify
GetActualListenAddr/ResolveAdvertiseListenAddr continue to return a reachable
URL when listenURL.Host or addr.String() comes from an IPv6 bind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ce468be-3371-42c2-af62-555a8368b02b
📒 Files selected for processing (18)
pkg/mcs/resourcemanager/server/server.gopkg/mcs/router/server/server.gopkg/mcs/scheduling/server/server.gopkg/mcs/server/server.gopkg/mcs/server/server_test.gopkg/mcs/tso/server/server.gotests/integrations/mcs/discovery/register_test.gotests/integrations/mcs/keyspace/tso_keyspace_group_test.gotests/integrations/mcs/members/member_test.gotests/integrations/mcs/resourcemanager/resource_manager_test.gotests/integrations/mcs/resourcemanager/server_test.gotests/integrations/mcs/router/server_test.gotests/integrations/mcs/tso/server_test.gotests/integrations/tso/client_test.gotests/integrations/tso/consistency_test.gotests/integrations/tso/server_test.gotests/server/server_test.gotests/testutil.go
rleungx
force-pushed
the
fix-test-port-conflicts
branch
from
2bd14ff to
9dfb8d8
Compare
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (58.97%) is below the target coverage (74.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #10963 +/- ##
==========================================
+ Coverage 79.20% 79.23% +0.02%
==========================================
Files 541 541
Lines 75450 75818 +368
==========================================
+ Hits 59762 60073 +311
- Misses 11456 11490 +34
- Partials 4232 4255 +23
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
fix-test-port-conflicts
branch
from
9dfb8d8 to
9f82f85
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/mcs/server/server_test.go (1)
33-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIgnored error from
Close().
svr.GetListener().Close()return value is discarded. As per coding guidelines,**/*.go: "No ignored errors unless allowed in.golangci.ymlerrcheck exclusions." Test cleanup errors are usually low-risk, but consider asserting viarequire.NoError.🔧 Proposed fix
- defer svr.GetListener().Close() + defer func() { + re.NoError(svr.GetListener().Close()) + }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/mcs/server/server_test.go` around lines 33 - 46, The test cleanup in TestInitListenerWithKernelSelectedPort is ignoring the error returned by GetListener().Close(). Update the deferred close on the listener to check the returned error with require.NoError so the cleanup path follows the error-handling rules; use the existing svr and GetListener() call site to locate the fix.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/mcs/scheduling/server/server.go`:
- Around line 166-168: The startup cleanup path in server.go ignores the result
of closing the listener, which can hide resource cleanup failures and violate
errcheck rules. In the listener cleanup block around s.GetListener(), capture
the Close() return value, check it, and log or otherwise handle any close error
instead of discarding it; keep the fix localized to the startup error path in
the server setup logic.
In `@pkg/mcs/tso/server/server.go`:
- Around line 177-179: The cleanup path around s.GetListener() is ignoring the
error returned by Close(), which violates the errcheck/resource-cleanup rule.
Update the listener shutdown logic in the server startup/cleanup flow to capture
and handle the Close() result instead of discarding it, and log the failure with
enough context when err is already set. Use the existing s.GetListener() access
pattern and the surrounding err != nil branch to locate the fix.
---
Nitpick comments:
In `@pkg/mcs/server/server_test.go`:
- Around line 33-46: The test cleanup in TestInitListenerWithKernelSelectedPort
is ignoring the error returned by GetListener().Close(). Update the deferred
close on the listener to check the returned error with require.NoError so the
cleanup path follows the error-handling rules; use the existing svr and
GetListener() call site to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89b113cb-852f-4eaf-83e0-e3b3200a86dd
📒 Files selected for processing (18)
pkg/mcs/resourcemanager/server/server.gopkg/mcs/router/server/server.gopkg/mcs/scheduling/server/server.gopkg/mcs/server/server.gopkg/mcs/server/server_test.gopkg/mcs/tso/server/server.gotests/integrations/mcs/discovery/register_test.gotests/integrations/mcs/keyspace/tso_keyspace_group_test.gotests/integrations/mcs/members/member_test.gotests/integrations/mcs/resourcemanager/resource_manager_test.gotests/integrations/mcs/resourcemanager/server_test.gotests/integrations/mcs/router/server_test.gotests/integrations/mcs/tso/server_test.gotests/integrations/tso/client_test.gotests/integrations/tso/consistency_test.gotests/integrations/tso/server_test.gotests/server/server_test.gotests/testutil.go
🚧 Files skipped from review as they are similar to previous changes (14)
- tests/integrations/mcs/resourcemanager/server_test.go
- tests/integrations/mcs/router/server_test.go
- tests/integrations/mcs/resourcemanager/resource_manager_test.go
- tests/integrations/tso/server_test.go
- tests/integrations/mcs/discovery/register_test.go
- tests/integrations/tso/consistency_test.go
- tests/integrations/mcs/members/member_test.go
- tests/testutil.go
- tests/server/server_test.go
- tests/integrations/tso/client_test.go
- tests/integrations/mcs/keyspace/tso_keyspace_group_test.go
- pkg/mcs/router/server/server.go
- pkg/mcs/resourcemanager/server/server.go
- pkg/mcs/server/server.go
|
/retest |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
fix-test-port-conflicts
branch
from
0abf888 to
2552141
Compare
|
|
||
| // ResolveAdvertiseListenAddr returns actualListenAddr when advertiseAddr was left | ||
| // unspecified or still points at a kernel-selected port. | ||
| func ResolveAdvertiseListenAddr(advertiseAddr, actualListenAddr string) string { |
There was a problem hiding this comment.
When advertiseAddr uses port 0, this currently returns actualListenAddr directly, which drops the explicitly configured advertise host.
For example, with listen-addr=http://127.0.0.1:0 and advertise-listen-addr=http://10.0.0.5:0, the registered address becomes http://127.0.0.1:<port> instead of the expected http://10.0.0.5:<port>. That can publish an incorrect service discovery endpoint.
I think we should only use the full actual listen address when advertiseAddr == "". If advertise only has a zero port, preserve its scheme/host and replace only the port with the listener actual bound port.
A small unit test can cover this case:
require.Equal(t,
"http://10.0.0.5:12345",
ResolveAdvertiseListenAddr("http://10.0.0.5:0", "http://127.0.0.1:12345"))
|
There is still an uncovered port-allocation path in the test helpers. This PR moves the single-server helpers to the
These helpers close the temporary listener and later bind the real server to the same port, so they keep the same TOCTOU port-conflict window. They are used by many integration tests via Please consider moving these cluster helpers to the same |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
fix-test-port-conflicts
branch
from
b0dfa3b to
d08263d
Compare
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Linking issues" section in the CONTRIBUTING.md. |
ti-chi-bot
Bot
added
size/XXL
|
/retest |
|
@rleungx: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: None
Fix flaky test failures like:
One observed case was
pull-unit-test-next-gen-2for PR #10943, whereTestCheckClusterIDrestarted an embedded etcd instance with cluster A's old listen URLs after another test cluster had already been started. More generally, several MCS integration helpers allocated a free port withtempurl.Alloc()and only bound it later, leaving a TOCTOU window.What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
Test details:
go test ./tests/server -run '^TestCheckClusterID$' -count=1go test ./pkg/mcs/server ./pkg/mcs/tso/server ./pkg/mcs/scheduling/server ./pkg/mcs/router/servercd tests/integrations && go test ./mcs/router ./mcs/tso ./mcs/members ./mcs/discovery ./mcs/keyspace ./mcs/resourcemanager ./tso -run '^$'cd tests/integrations && go test ./mcs/resourcemanager -run '^TestResourceManagerServer$' -count=1cd tests/integrations && go test ./mcs/tso -run '^TestTSOServerTestSuite/TestTSOServerStartAndStopNormally$' -count=1Note:
go test ./pkg/mcs/resourcemanager/server -run '^TestCleanUpTicker$' -count=1still fails locally with the existing stale-record count assertion; that failure is unrelated to this port-conflict change.Release note
Summary by CodeRabbit