Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added fix for RPC port detection #5715

Merged

Conversation

dat-boris
Copy link
Contributor

Fixes: #5713
Related: #5590
Merge before/after: None

Description

In #5713 we discover that the new port detection logic does not apply
to GRPC startup, which default starts on Mac on IPv4 loopback.

Test plan:

Locally run multiple version of skaffold, ensure that none
of them have issues with starting up their own RPC server.

Instead of crashing, it now correctly skip previous ports used by RPC

WARN[0000] starting gRPC server on port 50053. (50051 is already in use) 
WARN[0000] starting gRPC HTTP server on port 50054. (50052 is already in use) 

Test plan:
`make test`

Also go locally, and run multiple version of skaffold, ensure that none
of them have issues with starting up their own RPC server.
@dat-boris dat-boris requested a review from a team as a code owner April 21, 2021 22:47
@google-cla google-cla bot added the cla: yes label Apr 21, 2021
@dat-boris
Copy link
Contributor Author

This is my first PR for skaffold so any review comments are much appreciated. I know also #5590 so would be nice if we can confirm that on Windows machine this will not cause any regression (hopefully shouldn't do - it was a revert to previous behaviour for GRPC).

@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #5715 (db6c8c2) into master (7d1db11) will decrease coverage by 0.02%.
The diff coverage is 70.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5715      +/-   ##
==========================================
- Coverage   70.87%   70.85%   -0.03%     
==========================================
  Files         421      421              
  Lines       16050    16055       +5     
==========================================
  Hits        11375    11375              
- Misses       3844     3846       +2     
- Partials      831      834       +3     
Impacted Files Coverage Δ
...kubernetes/portforward/port_forward_integration.go 0.00% <0.00%> (ø)
pkg/skaffold/util/port.go 89.28% <72.72%> (-2.88%) ⬇️
...affold/kubernetes/portforward/kubectl_forwarder.go 69.71% <100.00%> (ø)
...g/skaffold/kubernetes/portforward/pod_forwarder.go 82.00% <100.00%> (ø)
...ffold/kubernetes/portforward/resource_forwarder.go 87.71% <100.00%> (ø)
pkg/skaffold/server/server.go 41.96% <100.00%> (ø)
pkg/skaffold/docker/image.go 78.34% <0.00%> (-1.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d1db11...db6c8c2. Read the comment docs.

@gsquared94 gsquared94 requested a review from briandealwis April 21, 2021 23:01
Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

Thanks for opening this @dat-boris ! Have a few tiny nit comments :)

pkg/skaffold/util/port.go Outdated Show resolved Hide resolved
pkg/skaffold/util/port.go Show resolved Hide resolved
pkg/skaffold/util/port.go Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/S and removed size/M labels Apr 21, 2021
Co-authored-by: Brian de Alwis <bsd@acm.org>
@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 24, 2021
@briandealwis
Copy link
Member

Sorry @dat-boris for the delay — I held off as I wanted to check how we were using GetAvailablePort(). Prior to #5670 we were explicitly passing the interface. I think we should restore that aspect and I'll push that change here.

@briandealwis briandealwis added kokoro:run runs the kokoro jobs on a PR kokoro:force-run forces a kokoro re-run on a PR labels Apr 27, 2021
@kokoro-team kokoro-team removed kokoro:run runs the kokoro jobs on a PR kokoro:force-run forces a kokoro re-run on a PR labels Apr 27, 2021
@briandealwis briandealwis enabled auto-merge (squash) April 27, 2021 18:47
Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @dat-boris for the contribution :)

@briandealwis briandealwis merged commit 7aaa1c7 into GoogleContainerTools:master Apr 27, 2021
@dat-boris
Copy link
Contributor Author

Thank you @briandealwis @MarlonGamez for the reviews! Yeah it make sense to revert to the old GetAvailablePort and improve the check!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPC free port detection is broken
4 participants