Add option to set a service nodeport #33319

juanvallejo · 2016-09-22T20:39:16Z

Release note:

Add kubectl --node-port option for specifying the service nodeport

This patch adds the option to set a nodeport when creating a NodePort
service. In case of a port allocation error due to a specified port
being out of the valid range, the error now includes the valid
range. If a --node-port value is not specified, it defaults to zero, in
which case the allocator will default to its current behavior of
assigning an available port.

This patch also adds a new helper function in cmd/util/helpers.go to
retrieve Int32 cobra flags.

Example

# create a nodeport service with an invalid port
$ kubectl create service nodeport mynodeport --tcp=8080:7777 --node-port=1
The Service "mynodeport" is invalid: spec.ports[0].nodePort: Invalid
value: 1: provided port is not in the valid range. Valid ports range
from 30000-32767

# create a nodeport service with a valid port
$ kubectl create service nodeport mynodeport --tcp=8080:7777 --node-port=30000
service "mynodeport" created

# create a nodeport service with a port already in use
$ kubectl create service nodeport mynodeport --tcp=8080:7777 --node-port=30000
The Service "mynodeport" is invalid: spec.ports[0].nodePort: Invalid value: 3000: provided port is already allocated

$ kubectl describe service mynodeport
Name:                   mynodeport
Namespace:              default
Labels:                 app=mynodeport
Selector:               app=mynodeport
Type:                   NodePort
IP:                     172.30.81.254
Port:                   8080-7777       8080/TCP
NodePort:               8080-7777       30000/TCP
Endpoints:              <none>
Session Affinity:       None
No events.

@fabianofranz

This change is

k8s-ci-robot · 2016-09-22T20:39:17Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line.

Regular contributors should join the org to skip this step.

While we transition away from the Jenkins GitHub PR Builder plugin, "ok to test" commenters will need to be on the admin list defined in this file.

UPSTREAM: kubernetes/kubernetes#33319 This patch adds the option to set a nodeport when creating a NodePort service. In case of a port allocation error due to a specified port being out of the valid range, the error now includes the valid range. If a `--node-port` value is not specified, it defaults to zero, in which case the allocator will default to its current behavior of assigning an available port. This patch also adds a new helper function in `cmd/util/helpers.go` to retrieve `Int32` cobra flags. **Example** ``` $ oc create service nodeport mynodeport --tcp=8080:7777 --node-port=1 The Service "mynodeport" is invalid: spec.ports[0].nodePort: Invalid value: 1: provided port is not in the valid range. Valid ports range from 30000-32767 $ oc create service nodeport mynodeport --tcp=8080:7777 --node-port=30000 service "mynodeport" created $ oc describe service mynodeport Name: mynodeport Namespace: default Labels: app=mynodeport Selector: app=mynodeport Type: NodePort IP: 172.30.81.254 Port: 8080-7777 8080/TCP NodePort: 8080-7777 30000/TCP Endpoints: <none> Session Affinity: None No events. ```

fabianofranz · 2016-09-22T20:54:59Z

@k8s-bot ok to test

lavalamp

a few requests

lavalamp · 2016-09-22T20:52:00Z

pkg/registry/core/service/portallocator/allocator.go

@@ -82,7 +82,10 @@ func (r *PortAllocator) Free() int {
 func (r *PortAllocator) Allocate(port int) error {
 	ok, offset := r.contains(port)
 	if !ok {
-		return ErrNotInRange
+		// include valid port range in error


You need to turn ErrNotInRange into a type if you want to add information to it like this. People might be checking for equality with ErrNotInRange and this code would break them.

imo, this will break people who use ErrNotInRange to check allocate error.

Thanks, it's much better now. The compiler will show you any code that needs to be updated. :)

lavalamp · 2016-09-22T20:53:40Z

pkg/kubectl/cmd/create_service.go

@@ -134,6 +134,7 @@ func NewCmdCreateServiceNodePort(f *cmdutil.Factory, cmdOut io.Writer) *cobra.Co
 	cmdutil.AddValidateFlags(cmd)
 	cmdutil.AddPrinterFlags(cmd)
 	cmdutil.AddGeneratorFlags(cmd, cmdutil.ServiceNodePortGeneratorV1Name)
+	cmd.Flags().Int32("node-port", 0, "Port used to expose the service on each node in a cluster.")


Why int32? There's already an int, just use that.

Since NodePort is of type int32, I thought to receive it as that from the flag value, although I could receive it as int and convert it afterwards.

Only 0-65k are actually valid. We specify the bit width in the API because it's a transport format, but that's not important for kubectl flags.

I agree with @lavalamp , we could use int directly here.

lavalamp · 2016-09-22T20:56:02Z

Give this to Phil, for verification that this is something we want to do.

Also what happens if you make a second service that uses a port that's already taken? does it give a nice error message?

juanvallejo · 2016-09-22T21:04:04Z

@lavalamp

Also what happens if you make a second service that uses a port that's already taken? does it give a
nice error message?

Yes, if the port is already taken, it returns:

The Service "mynodeport" is invalid: spec.ports[0].nodePort: Invalid value: <port>: provided port is already allocated

juanvallejo · 2016-09-23T14:30:07Z

@lavalamp Thanks for the feedback, review comments addressed

k8s-ci-robot · 2016-09-23T14:57:53Z

Jenkins GCE e2e failed for commit 35a9794a5a922ff8b0855597bc2945f387098131. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-09-23T17:50:15Z

Jenkins verification failed for commit d63848f480ad0b8af3ba644827573a066571e9ab. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

pwittrock · 2016-09-26T22:56:20Z

@adohe Would you mind doing a pass to make sure this makes sense.

adohe-zz · 2016-09-27T05:14:45Z

@pwittrock sure, I will check this.

pwittrock · 2016-09-27T23:13:18Z

Adding the flag makes sense to me. @adohe looks like @lavalamp has already done a review on this, but makes sense to take a pass and make sure things conform to kubectl best practices

adohe-zz · 2016-09-28T23:57:43Z

@juanvallejo

# create a nodeport service with a port already in use
$ kubectl create service nodeport mynodeport --tcp=8080:7777 --node-port=30000
The Service "mynodeport" is invalid: spec.ports[0].nodePort: Invalid value: 3000: provided port is already allocated

the error output should be 30000, right?

adohe-zz · 2016-09-29T00:04:30Z

@juanvallejo could you link the original issue here? I want to get more context.

juanvallejo · 2016-09-29T14:41:44Z

@adohe

the error output should be 30000, right?

Ah, correct, I just checked, and doing:

$ kubectl create service nodeport mynodeport --tcp=8080:7777 --node-port=3000

gives the error:

The Service "mynodeport" is invalid: spec.ports[0].nodePort: Invalid value: 3000: provided port is not in the valid range

while

kubectl create service nodeport mynodeport --tcp=8080:7777 --node-port=30000

gives the error

The Service "mynodeport" is invalid: spec.ports[0].nodePort: Invalid value: 30000: provided port is already allocated

Once I try to allocate it for the second time.

could you link the original issue here? I want to get more context.

https://bugzilla.redhat.com/show_bug.cgi?id=1378012

k8s-ci-robot · 2016-09-29T15:24:36Z

Jenkins GCI GKE smoke e2e failed for commit dc7f493. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

adohe-zz · 2016-09-29T15:27:48Z

@juanvallejo I think you still need to fix the e2e tests.

juanvallejo · 2016-09-29T17:30:38Z

@k8s-bot gke e2e test this

juanvallejo · 2016-09-29T17:31:05Z

@k8s-bot gci gke e2e test this

adohe-zz · 2016-09-29T23:39:20Z

ERROR: (gcloud.container.clusters.create) Operation [<Operation
 name: u'operation-1475161376626-e6423faf'
 operationType: OperationTypeValueValuesEnum(CREATE_CLUSTER, 1)
 selfLink: u'https://test-container.sandbox.googleapis.com/v1/projects/154064302321/zones/us-central1-f/operations/operation-1475161376626-e6423faf'
 status: StatusValueValuesEnum(DONE, 3)
 statusMessage: u'Timed out waiting for cluster initialization. Cluster API may not be available.'
 targetLink: u'https://test-container.sandbox.googleapis.com/v1/projects/154064302321/zones/us-central1-f/clusters/e2e-gke-agent-pr-31-0'
 zone: u'us-central1-f'>] finished with error: Timed out waiting for cluster initialization. Cluster API may not be available.
2016/09/29 08:24:34 e2e.go:646: Step 'up' finished in 21m42.214576779s
2016/09/29 08:24:34 e2e.go:143: Saved XML output to /workspace/_artifacts/junit_runner.xml.
2016/09/29 08:24:34 e2e.go:160: Something went wrong: starting e2e cluster: error running up: exit status 1

adohe-zz · 2016-09-29T23:40:08Z

pkg/kubectl/cmd/util/helpers.go

+}
+
+// Assumes the flag has a default value.
+func GetFlagInt32(cmd *cobra.Command, flag string) int32 {


any need to keep this function?

@adohe It is no longer needed, I went ahead and removed it 577b5a5

k8s-ci-robot · 2016-09-30T14:44:31Z

Jenkins GKE smoke e2e failed for commit 577b5a548a763086ad4ddf60ef8aab15237e4dc4. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

juanvallejo · 2016-09-30T15:04:47Z

@adohe I don't think I can tell the bot to re-test. I believe the gke e2e test is flaking on #23737

This patch adds the option to set a nodeport when creating a NodePort service. In case of a port allocation error due to a specified port being out of the valid range, the error now includes the valid range. If a `--node-port` value is not specified, it defaults to zero, in which case the allocator will default to its current behavior of assigning an available port. This patch also adds a new helper function in `cmd/util/helpers.go` to retrieve `Int32` cobra flags. **Example** ``` $ kubectl create service nodeport mynodeport --tcp=8080:7777 --node-port=1 The Service "mynodeport" is invalid: spec.ports[0].nodePort: Invalid value: 1: provided port is not in the valid range. Valid ports range from 30000-32767 $ kubectl create service nodeport mynodeport --tcp=8080:7777 --node-port=30000 service "mynodeport" created $ oc describe service mynodeport Name: mynodeport Namespace: default Labels: app=mynodeport Selector: app=mynodeport Type: NodePort IP: 172.30.81.254 Port: 8080-7777 8080/TCP NodePort: 8080-7777 30000/TCP Endpoints: <none> Session Affinity: None No events. ```

pwittrock · 2016-10-04T21:43:27Z

pkg/kubectl/cmd/create_service.go, line 137 at r1 (raw file):

Previously, AdoHe (TonyAdo) wrote…

I agree with @lavalamp , we could use int directly here.

Update?

Comments from Reviewable

juanvallejo · 2016-10-05T16:11:35Z

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions.

pkg/kubectl/cmd/create_service.go, line 137 at r1 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Update?

I went ahead and updated the flag to receive an `int` https://github.com//pull/33319/files#diff-65c6682164bc3185c68546044a5ffb11R137,

I then store it in the generator as an int as well https://github.com/kubernetes/kubernetes/pull/33319/files#diff-dac1b3be3f5aa420a4ada15749324a1eR34

and then store it in a api.ServicePod object as an int32 https://github.com/kubernetes/kubernetes/pull/33319/files#diff-dac1b3be3f5aa420a4ada15749324a1eR186

Comments from Reviewable

k8s-github-robot · 2016-10-05T21:23:47Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-10-05T22:00:32Z

Automatic merge from submit-queue

UPSTREAM: kubernetes/kubernetes#33319 This patch adds the option to set a nodeport when creating a NodePort service. In case of a port allocation error due to a specified port being out of the valid range, the error now includes the valid range. If a `--node-port` value is not specified, it defaults to zero, in which case the allocator will default to its current behavior of assigning an available port. This patch also adds a new helper function in `cmd/util/helpers.go` to retrieve `Int32` cobra flags. **Example** ``` $ oc create service nodeport mynodeport --tcp=8080:7777 --node-port=1 The Service "mynodeport" is invalid: spec.ports[0].nodePort: Invalid value: 1: provided port is not in the valid range. Valid ports range from 30000-32767 $ oc create service nodeport mynodeport --tcp=8080:7777 --node-port=30000 service "mynodeport" created $ oc describe service mynodeport Name: mynodeport Namespace: default Labels: app=mynodeport Selector: app=mynodeport Type: NodePort IP: 172.30.81.254 Port: 8080-7777 8080/TCP NodePort: 8080-7777 30000/TCP Endpoints: <none> Session Affinity: None No events. ```

UPSTREAM: kubernetes#33319 This patch adds the option to set a nodeport when creating a NodePort service. In case of a port allocation error due to a specified port being out of the valid range, the error now includes the valid range. If a `--node-port` value is not specified, it defaults to zero, in which case the allocator will default to its current behavior of assigning an available port. This patch also adds a new helper function in `cmd/util/helpers.go` to retrieve `Int32` cobra flags. **Example** ``` $ oc create service nodeport mynodeport --tcp=8080:7777 --node-port=1 The Service "mynodeport" is invalid: spec.ports[0].nodePort: Invalid value: 1: provided port is not in the valid range. Valid ports range from 30000-32767 $ oc create service nodeport mynodeport --tcp=8080:7777 --node-port=30000 service "mynodeport" created $ oc describe service mynodeport Name: mynodeport Namespace: default Labels: app=mynodeport Selector: app=mynodeport Type: NodePort IP: 172.30.81.254 Port: 8080-7777 8080/TCP NodePort: 8080-7777 30000/TCP Endpoints: <none> Session Affinity: None No events. ``` :100644 100644 8dd3aa6... 5d1d0e1... M pkg/kubectl/cmd/create_service.go :100644 100644 6789920... 24b8ccf... M pkg/kubectl/cmd/util/helpers.go :100644 100644 ee080aa... 9f1c3d2... M pkg/kubectl/service_basic.go :100644 100644 a6d565b... 3a5407d... M pkg/registry/service/portallocator/allocator.go :100644 100644 d060528... 7386172... M pkg/registry/service/portallocator/allocator_test.go :100644 100644 90ebcb1... 1098605... M pkg/registry/service/portallocator/controller/repair.go

googlebot added the cla: yes label Sep 22, 2016

juanvallejo mentioned this pull request Sep 22, 2016

UPSTREAM: 33319: add nodeport option when creating NodePort service openshift/origin#11059

Merged

k8s-github-robot assigned lavalamp Sep 22, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Sep 22, 2016

lavalamp suggested changes Sep 22, 2016

View reviewed changes

lavalamp assigned pwittrock and unassigned lavalamp Sep 22, 2016

juanvallejo force-pushed the jvallejo/add-option-to-set-nodeport branch from df99c20 to 878477b Compare September 22, 2016 21:03

juanvallejo force-pushed the jvallejo/add-option-to-set-nodeport branch from 35a9794 to d63848f Compare September 23, 2016 17:23

pwittrock assigned adohe-zz Sep 26, 2016

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

juanvallejo force-pushed the jvallejo/add-option-to-set-nodeport branch from 24d0993 to dc7f493 Compare September 29, 2016 14:45

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

adohe-zz reviewed Sep 29, 2016

View reviewed changes

juanvallejo added 4 commits October 4, 2016 13:19

make portallocator.ErrNotInRange a type

cfbdcec

update known flags

395f6fd

remove unused GetFlagInt32 func

7d1461b

juanvallejo force-pushed the jvallejo/add-option-to-set-nodeport branch from 577b5a5 to 7d1461b Compare October 4, 2016 17:19

pwittrock added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Oct 5, 2016

k8s-github-robot merged commit 05192d9 into kubernetes:master Oct 5, 2016

juanvallejo deleted the jvallejo/add-option-to-set-nodeport branch October 6, 2016 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to set a service nodeport #33319

Add option to set a service nodeport #33319

juanvallejo commented Sep 22, 2016 •

edited by pwittrock

k8s-ci-robot commented Sep 22, 2016

fabianofranz commented Sep 22, 2016

lavalamp left a comment

lavalamp Sep 22, 2016

adohe-zz Sep 29, 2016

lavalamp Sep 30, 2016

lavalamp Sep 22, 2016

juanvallejo Sep 22, 2016

lavalamp Sep 22, 2016

adohe-zz Sep 28, 2016

lavalamp commented Sep 22, 2016

juanvallejo commented Sep 22, 2016 •

edited

juanvallejo commented Sep 23, 2016

k8s-ci-robot commented Sep 23, 2016

k8s-ci-robot commented Sep 23, 2016

pwittrock commented Sep 26, 2016

adohe-zz commented Sep 27, 2016

pwittrock commented Sep 27, 2016

adohe-zz commented Sep 28, 2016

adohe-zz commented Sep 29, 2016

juanvallejo commented Sep 29, 2016

k8s-ci-robot commented Sep 29, 2016

adohe-zz commented Sep 29, 2016

juanvallejo commented Sep 29, 2016

juanvallejo commented Sep 29, 2016 •

edited

adohe-zz commented Sep 29, 2016

adohe-zz Sep 29, 2016

juanvallejo Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

juanvallejo commented Sep 30, 2016

pwittrock commented Oct 4, 2016

juanvallejo commented Oct 5, 2016

k8s-github-robot commented Oct 5, 2016

k8s-github-robot commented Oct 5, 2016

Add option to set a service nodeport #33319

Add option to set a service nodeport #33319

Conversation

juanvallejo commented Sep 22, 2016 • edited by pwittrock

k8s-ci-robot commented Sep 22, 2016

fabianofranz commented Sep 22, 2016

lavalamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Sep 22, 2016

juanvallejo commented Sep 22, 2016 • edited

juanvallejo commented Sep 23, 2016

k8s-ci-robot commented Sep 23, 2016

k8s-ci-robot commented Sep 23, 2016

pwittrock commented Sep 26, 2016

adohe-zz commented Sep 27, 2016

pwittrock commented Sep 27, 2016

adohe-zz commented Sep 28, 2016

adohe-zz commented Sep 29, 2016

juanvallejo commented Sep 29, 2016

k8s-ci-robot commented Sep 29, 2016

adohe-zz commented Sep 29, 2016

juanvallejo commented Sep 29, 2016

juanvallejo commented Sep 29, 2016 • edited

adohe-zz commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 30, 2016

juanvallejo commented Sep 30, 2016

pwittrock commented Oct 4, 2016

juanvallejo commented Oct 5, 2016

k8s-github-robot commented Oct 5, 2016

k8s-github-robot commented Oct 5, 2016

juanvallejo commented Sep 22, 2016 •

edited by pwittrock

juanvallejo commented Sep 22, 2016 •

edited

juanvallejo commented Sep 29, 2016 •

edited