Skip to content

Add logic for finding next available port for gRPC if provided one is in use #1752

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 7, 2019
Merged

Add logic for finding next available port for gRPC if provided one is in use #1752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/skaffold/app/cmd/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ func AddRunDeployFlags(cmd *cobra.Command) {

func AddRunDevFlags(cmd *cobra.Command) {
cmd.Flags().BoolVar(&opts.EnableRPC, "enable-rpc", false, "Enable gRPC for exposing Skaffold events (true by default for `skaffold dev`)")
cmd.Flags().StringVar(&opts.RPCPort, "rpc-port", ":50051", "tcp port to expose event API")
cmd.Flags().IntVar(&opts.RPCPort, "rpc-port", constants.DefaultRPCPort, "tcp port to expose event API")
cmd.Flags().StringVarP(&opts.ConfigurationFile, "filename", "f", "skaffold.yaml", "Filename or URL to the pipeline file")
cmd.Flags().BoolVar(&opts.Notification, "toot", false, "Emit a terminal beep after the deploy is complete")
cmd.Flags().StringArrayVarP(&opts.Profiles, "profile", "p", nil, "Activate profiles by name")
Expand Down
10 changes: 5 additions & 5 deletions docs/content/en/docs/references/cli/_index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ Flags:
{{end}})
-p, --profile stringArray Activate profiles by name
-q, --quiet Suppress the build output and print image built on success
--rpc-port string tcp port to expose event API (default ":50051")
--rpc-port int tcp port to expose event API (default 50051)
--skip-tests Whether to skip the tests after building
--toot Emit a terminal beep after the deploy is complete

Expand Down Expand Up @@ -157,7 +157,7 @@ Flags:
-f, --filename string Filename or URL to the pipeline file (default "skaffold.yaml")
-n, --namespace string Run deployments in the specified namespace
-p, --profile stringArray Activate profiles by name
--rpc-port string tcp port to expose event API (default ":50051")
--rpc-port int tcp port to expose event API (default 50051)
--skip-tests Whether to skip the tests after building
--toot Emit a terminal beep after the deploy is complete

Expand Down Expand Up @@ -198,7 +198,7 @@ Flags:
-l, --label stringArray Add custom labels to deployed objects. Set multiple times for multiple labels.
-n, --namespace string Run deployments in the specified namespace
-p, --profile stringArray Activate profiles by name
--rpc-port string tcp port to expose event API (default ":50051")
--rpc-port int tcp port to expose event API (default 50051)
--skip-tests Whether to skip the tests after building
--tail Stream logs from deployed objects
--toot Emit a terminal beep after the deploy is complete
Expand Down Expand Up @@ -245,7 +245,7 @@ Flags:
-n, --namespace string Run deployments in the specified namespace
--port-forward Port-forward exposed container ports within pods (default true)
-p, --profile stringArray Activate profiles by name
--rpc-port string tcp port to expose event API (default ":50051")
--rpc-port int tcp port to expose event API (default 50051)
--skip-tests Whether to skip the tests after building
--tail Stream logs from deployed objects (default true)
--toot Emit a terminal beep after the deploy is complete
Expand Down Expand Up @@ -375,7 +375,7 @@ Flags:
-l, --label stringArray Add custom labels to deployed objects. Set multiple times for multiple labels.
-n, --namespace string Run deployments in the specified namespace
-p, --profile stringArray Activate profiles by name
--rpc-port string tcp port to expose event API (default ":50051")
--rpc-port int tcp port to expose event API (default 50051)
--skip-tests Whether to skip the tests after building
-t, --tag string The optional custom tag to use for images which overrides the current Tagger configuration
--tail Stream logs from deployed objects
Expand Down
9 changes: 5 additions & 4 deletions integration/rpc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package integration

import (
"context"
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -50,7 +51,7 @@ func TestEventLog(t *testing.T) {
defer deleteNs()

cancel := make(chan bool)
addr := ":12345"
addr := "12345"
go RunSkaffoldNoFail(cancel, "dev", "testdata/dev", ns.Name, "", nil, "--rpc-port", addr)
defer func() { cancel <- true }()

Expand All @@ -60,7 +61,7 @@ func TestEventLog(t *testing.T) {
var client proto.SkaffoldServiceClient
attempts := 0
for {
conn, err = grpc.Dial(addr, grpc.WithInsecure())
conn, err = grpc.Dial(fmt.Sprintf(":%s", addr), grpc.WithInsecure())
if err != nil {
t.Logf("unable to establish skaffold grpc connection: retrying...")
attempts++
Expand Down Expand Up @@ -147,7 +148,7 @@ func TestGetState(t *testing.T) {

// start a skaffold dev loop on an example
cancel := make(chan bool)
addr := ":12345"
addr := "12345"
go RunSkaffoldNoFail(cancel, "dev", "testdata/dev", ns.Name, "", nil, "--rpc-port", addr)
defer func() { cancel <- true }()

Expand All @@ -157,7 +158,7 @@ func TestGetState(t *testing.T) {
var client proto.SkaffoldServiceClient
attempts := 0
for {
conn, err = grpc.Dial(addr, grpc.WithInsecure())
conn, err = grpc.Dial(fmt.Sprintf(":%s", addr), grpc.WithInsecure())
if err != nil {
t.Logf("unable to establish skaffold grpc connection: retrying...")
attempts++
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/config/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ type SkaffoldOptions struct {
DefaultRepo string
PreBuiltImages []string
Command string
RPCPort string
RPCPort int
}

// Labels returns a map of labels to be applied to all deployed
Expand Down
2 changes: 2 additions & 0 deletions pkg/skaffold/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ const (
SkaffoldPluginValue = "1337"
SkaffoldPluginName = "SKAFFOLD_PLUGIN_NAME"
DockerBuilderPluginName = "docker"

DefaultRPCPort = 50051
)

var (
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/event/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func InitializeState(build *latest.BuildConfig, deploy *latest.DeployConfig, opt

func SetupRPCClient(opts *config.SkaffoldOptions) error {
pluginMode = true
conn, err := grpc.Dial(opts.RPCPort, grpc.WithInsecure())
conn, err := grpc.Dial(fmt.Sprintf(":%d", opts.RPCPort), grpc.WithInsecure())
if err != nil {
return errors.Wrap(err, "opening gRPC connection to remote skaffold process")
}
Expand Down
14 changes: 11 additions & 3 deletions pkg/skaffold/event/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ package event

import (
"context"
"fmt"
"net"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/event/proto"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"

empty "github.com/golang/protobuf/ptypes/empty"
"github.com/pkg/errors"
Expand Down Expand Up @@ -62,14 +65,19 @@ func (s *server) Handle(ctx context.Context, event *proto.Event) (*empty.Empty,
}

// newStatusServer creates the grpc server for serving the state and event log.
func newStatusServer(port string) (func() error, error) {
if port == "" {
func newStatusServer(originalPort int) (func() error, error) {
if originalPort == -1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why this would be -1? i see its called here with default port 50051?

skaffold/pkg/skaffold/event/event.go

Line 103 in 80d0347

serverShutdown, err = newStatusServer(opts.RPCPort)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was for testing as a way to bypass creating an RPC server in some cases. I probably should have removed it.

return func() error { return nil }, nil
}
l, err := net.Listen("tcp", port)
port := util.GetAvailablePort(originalPort)
if port != originalPort && originalPort != constants.DefaultRPCPort {
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't port != constants.DefaultRPCPort suffice?
is this to guard against condition where util.GetAvailbalePort returns -1?
Maybe we shd define an constant PORT_NOT_FOUND and assign it to -1 just to make this readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this logic is basically "if the user provided a different port than the skaffold default, but we couldn't use that port because it was already in use, warn them that we're using a different port than they might expect".

if the user didn't provide a port, they probably don't care that skaffold didn't use its own default. the port being used will still be in the logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah.. ok.

logrus.Warnf("provided port %d already in use: using %d instead", originalPort, port)
}
l, err := net.Listen("tcp", fmt.Sprintf(":%d", port))
if err != nil {
return func() error { return nil }, errors.Wrap(err, "creating listener")
}
logrus.Infof("starting gRPC server on port %d", port)

s := grpc.NewServer()
proto.RegisterSkaffoldServiceServer(s, &server{})
Expand Down