Fix auto-allocated ports conflicting with explicitly specified ports on service create #93922

knight42 · 2020-08-12T10:35:26Z

Signed-off-by: knight42 anonymousknight96@gmail.com

What type of PR is this?

/kind flake
/kind bug

What this PR does / why we need it:

I added the following debugging:

diff --git pkg/registry/core/service/storage/rest.go pkg/registry/core/service/storage/rest.go
index 0d5379f8c11..8095f386431 100644
--- pkg/registry/core/service/storage/rest.go
+++ pkg/registry/core/service/storage/rest.go
@@ -208,6 +208,8 @@ func (rs *REST) Create(ctx context.Context, obj runtime.Object, createValidation
        // Handle ExternalTraffic related fields during service creation.
        if apiservice.NeedsHealthCheck(service) {
                if err := allocateHealthCheckNodePort(service, nodePortOp); err != nil {
+                       spec := service.Spec
+                       fmt.Printf("NodePort: %d, HealthCheckPort: %d\n", spec.Ports[0].NodePort, spec.HealthCheckNodePort)
                        return nil, errors.NewInternalError(err)
                }
        }

and got:

WARNING: Package "github.com/golang/protobuf/protoc-gen-go/generator" is deprecated.
        A future release of golang/protobuf will delete this package,
        which has long been excluded from the compatibility promise.

I0812 17:52:19.904068   78356 client.go:360] parsed scheme: "endpoint"
I0812 17:52:19.904194   78356 endpoint.go:68] ccResolverWrapper: sending new addresses to cc: [{unix://localhost:83760927833799529510  <nil> 0 <nil>}]
I0812 17:52:19.908507   78356 client.go:360] parsed scheme: "endpoint"
I0812 17:52:19.908588   78356 endpoint.go:68] ccResolverWrapper: sending new addresses to cc: [{unix://localhost:83760927833799529510  <nil> 0 <nil>}]
I0812 17:52:19.909254   78356 once.go:66] CPU time info is unavailable on non-linux or appengine environment.
I0812 17:52:19.914674   78356 client.go:360] parsed scheme: "endpoint"
I0812 17:52:19.914761   78356 endpoint.go:68] ccResolverWrapper: sending new addresses to cc: [{unix://localhost:83760927833799529510  <nil> 0 <nil>}]
I0812 17:52:19.918666   78356 client.go:360] parsed scheme: "endpoint"
I0812 17:52:19.918736   78356 endpoint.go:68] ccResolverWrapper: sending new addresses to cc: [{unix://localhost:83760927833799529510  <nil> 0 <nil>}]

NodePort: 30773, HealthCheckPort: 30773  <-- Please notice this line

W0812 17:52:19.961603   78356 clientconn.go:1223] grpc: addrConn.createTransport failed to connect to {unix://localhost:83760927833799529510  <nil> 0 <nil>}. Err :connection error: desc = "transport: Error while dialing dial unix localhost:83760927833799529510: connect: no such file or directory". Reconnecting...
W0812 17:52:19.961638   78356 clientconn.go:1223] grpc: addrConn.createTransport failed to connect to {unix://localhost:83760927833799529510  <nil> 0 <nil>}. Err :connection error: desc = "transport: Error while dialing dial unix localhost:83760927833799529510: connect: no such file or directory". Reconnecting...
W0812 17:52:19.961971   78356 clientconn.go:1223] grpc: addrConn.createTransport failed to connect to {unix://localhost:83760927833799529510  <nil> 0 <nil>}. Err :connection error: desc = "transport: Error while dialing dial unix localhost:83760927833799529510: connect: no such file or directory". Reconnecting...
--- FAIL: TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation (1.98s)
    rest_test.go:2214: Unexpected failure creating service :Internal error occurred: failed to allocate requested HealthCheck NodePort 30773: provided port is already allocated
FAIL

ERROR: exit status 1

The log shows the root cause is the randomly generated health check nodePort happens to be the same as the service nodePort allocated later.

The flake also uncovers a bug that apiserver could potenially allocate a same port as the user-specified HealthCheck NodePort to be the Service NodePort when creating NodePort or LoadBalancer Service.

Which issue(s) this PR fixes:

xref: #93605 (comment)

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix a bug that apiserver could potenially allocate a same port as the user-specified HealthCheck NodePort to be the Service NodePort when creating `NodePort` or `LoadBalancer` Service.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

knight42 · 2020-08-12T10:35:47Z

/cc @liggitt

pkg/registry/core/service/storage/rest_test.go

liggitt · 2020-08-12T12:41:25Z

The log shows the root cause is the randomly generated health check nodePort happens to be the same as the service nodePort allocated later.

nice catch

knight42 · 2020-08-12T13:50:02Z

@liggitt Now that this PR actually fixes a bug, is it OK to add a release note?

pkg/registry/core/service/storage/rest.go

knight42 · 2020-08-12T16:15:25Z

/hold

knight42 · 2020-08-14T09:00:34Z

@liggitt I think adding tests that describes our expected behavior might be a good start, so I add some tests in 88d4926, and it turns out the problem you pointed out at #93922 (comment) actually exists.

liggitt · 2020-08-14T13:41:07Z

/assign @thockin @bowei
/sig network

Signed-off-by: knight42 <anonymousknight96@gmail.com>

thockin · 2020-09-19T00:13:24Z

pkg/registry/core/service/storage/rest_test.go

+
+	const count = 3
+	// The `PortAllocator` seems to allocate the first port randomly,
+	// So we run the test multiple times to ensure the NodePort is correctly allocated.


The code change seems good, but I am not sure I understand this random + retry. Can you help me understand?

The port allocator would allocate port randomly from the available ports, if we did not retry, the port allocator might happen to allocate the right port and the test got passed.

fejta-bot · 2020-12-19T18:15:58Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2021-01-18T19:01:00Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2021-02-17T19:46:58Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

k8s-ci-robot · 2021-02-17T19:47:07Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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/test-infra repository.

k8s-ci-robot requested review from liggitt, deads2k and justinsb August 12, 2020 10:35

knight42 mentioned this pull request Aug 12, 2020

Stop ignoring unit test flakes #93605

Merged

37 tasks

liggitt reviewed Aug 12, 2020

View reviewed changes

pkg/registry/core/service/storage/rest_test.go Outdated Show resolved Hide resolved

knight42 force-pushed the fix/TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation branch from 6de5593 to 1709887 Compare August 12, 2020 13:26

knight42 requested a review from liggitt August 12, 2020 13:27

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 12, 2020

liggitt reviewed Aug 12, 2020

View reviewed changes

pkg/registry/core/service/storage/rest.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2020

liggitt changed the title ~~test(storage): deflake TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation~~ sig-storage: Fix auto-allocated ports conflicting with explicitly specified ports on service create Aug 13, 2020

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 14, 2020

knight42 force-pushed the fix/TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation branch from cd1dbe1 to 704b9d2 Compare August 14, 2020 12:39

knight42 requested a review from liggitt August 14, 2020 12:43

liggitt changed the title ~~sig-storage: Fix auto-allocated ports conflicting with explicitly specified ports on service create~~ sig-network: Fix auto-allocated ports conflicting with explicitly specified ports on service create Aug 14, 2020

k8s-ci-robot assigned bowei Aug 14, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2020

knight42 force-pushed the fix/TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation branch from 9484aea to dfe481b Compare September 4, 2020 04:44

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2020

knight42 force-pushed the fix/TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation branch from dfe481b to ef97435 Compare September 4, 2020 04:49

refactor: fix initNodePorts

a19fc51

Signed-off-by: knight42 <anonymousknight96@gmail.com>

knight42 force-pushed the fix/TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation branch from ef97435 to a19fc51 Compare September 4, 2020 04:50

thockin reviewed Sep 19, 2020

View reviewed changes

thockin changed the title ~~sig-network: Fix auto-allocated ports conflicting with explicitly specified ports on service create~~ Fix auto-allocated ports conflicting with explicitly specified ports on service create Sep 19, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 19, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 18, 2021

k8s-ci-robot closed this Feb 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix auto-allocated ports conflicting with explicitly specified ports on service create #93922

Fix auto-allocated ports conflicting with explicitly specified ports on service create #93922

knight42 commented Aug 12, 2020 •

edited

knight42 commented Aug 12, 2020

liggitt commented Aug 12, 2020

knight42 commented Aug 12, 2020

knight42 commented Aug 12, 2020

knight42 commented Aug 14, 2020 •

edited

liggitt commented Aug 14, 2020

thockin Sep 19, 2020

knight42 Sep 20, 2020

fejta-bot commented Dec 19, 2020

fejta-bot commented Jan 18, 2021

fejta-bot commented Feb 17, 2021

k8s-ci-robot commented Feb 17, 2021

Fix auto-allocated ports conflicting with explicitly specified ports on service create #93922

Fix auto-allocated ports conflicting with explicitly specified ports on service create #93922

Conversation

knight42 commented Aug 12, 2020 • edited

knight42 commented Aug 12, 2020

liggitt commented Aug 12, 2020

knight42 commented Aug 12, 2020

knight42 commented Aug 12, 2020

knight42 commented Aug 14, 2020 • edited

liggitt commented Aug 14, 2020

thockin Sep 19, 2020

Choose a reason for hiding this comment

knight42 Sep 20, 2020

Choose a reason for hiding this comment

fejta-bot commented Dec 19, 2020

fejta-bot commented Jan 18, 2021

fejta-bot commented Feb 17, 2021

k8s-ci-robot commented Feb 17, 2021

knight42 commented Aug 12, 2020 •

edited

knight42 commented Aug 14, 2020 •

edited