Added changes to support container port information while creating/updating service #191

savitaashture · 2019-06-18T06:03:41Z

Issue: #142

Description:
When someone writes application which listens on port other than 8080(default port in knative-serving) then deploy using cli(kn) then there is no way to specify the port.

Current:
Supported flags are

Flags:
      --concurrency-limit int    Hard Limit of concurrent requests to be processed by a single replica.
      --concurrency-target int   Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.
  -e, --env stringArray          Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables.
      --force                    Create service forcefully, replaces existing service if any.
  -h, --help                     help for create
      --image string             Image to run.
      --limits-cpu string        The limits on the requested CPU (e.g., 1000m).
      --limits-memory string     The limits on the requested CPU (e.g., 1024Mi).
      --max-scale int            Maximal number of replicas.
      --min-scale int            Minimal number of replicas.
  -n, --namespace string         List the requested object(s) in given namespace.
      --requests-cpu string      The requested CPU (e.g., 250m).
      --requests-memory string   The requested CPU (e.g., 64Mi).

Expected:
Along with current flags it should support port flag so that user can specify port at which application is running.

ex: something like below

Flags:
   ...
  -p, --port int32               The port that the application should run. (default 8080)
   ...

Usage:

kn service create mysvc --port 80 --image dev.local/ns/image:latest

kn service update mysvc --port 80

knative-prow-robot · 2019-06-18T06:03:56Z

Hi @savitaashture. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

rhuss

Thanks ! Beside some minor nit-picks, I would verify and assert that there is only one port and one container in the template's PodSpec (see comments).

rhuss · 2019-06-18T07:12:55Z

pkg/kn/commands/service/service_update.go

@@ -33,6 +33,9 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
  # Updates a service 'mysvc' with new environment variables
  kn service update mysvc --env KEY1=VALUE1 --env KEY2=VALUE2

+  # Update a service with given port


# Update service "mysvc" to expose port 80

Be more specific, it's an example.

Modified with proper comments

rhuss · 2019-06-18T07:13:47Z

pkg/kn/commands/service/service_create.go

@@ -46,6 +46,10 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
  # Create or replace environment variables of service 's1' using --force flag
  kn service create --force s1 --env KEY1=NEW_VALUE1 --env NEW_KEY2=NEW_VALUE2 --image dev.local/ns/image:v1

+  # Create a service with given port
+  # If port is not specified it will take 8080 as a default port


# Create service 'mysvc' with port 80 # If no --port is given the default port 8080 is used

I would be more specific and more concise in the documentation.

Modified with proper comments

rhuss · 2019-06-18T07:15:25Z

hack/generate-docs.go

@@ -29,9 +29,9 @@ func main() {
 	if len(os.Args) > 1 {
 		dir = os.Args[1]
 	}
-	err := doc.GenMarkdownTree(rootCmd, dir + "/docs/cmd/")
+	err := doc.GenMarkdownTree(rootCmd, dir+"/docs/cmd/")


would revert that as it's not related to this PR.

When i ran go fmt for complete client code its modified i have reverted back now.

rhuss · 2019-06-18T07:18:52Z

pkg/serving/config_changes.go

+		}}
+
+	} else {
+		for i, _ := range container.Ports {


There should be only one. Please either assert that there is only one port (that is what Knative serving allows) and return an error or truncate to take only the first.

Tbh, I would just overwrite unconditionally any existing port.

Overwritten the existing port

rhuss · 2019-06-18T07:20:19Z

pkg/serving/config_changes_test.go

+		t.Fatal(err)
+	}
+	// Verify update is successful or not
+	for p, _ := range template.Spec.Containers {


assert that there are only 1 container and 1 port. Then just assert on template.Spec.Containers[0].Ports[0]

Modified in all the places

rhuss · 2019-06-18T07:21:44Z

pkg/serving/config_changes_test.go

@@ -139,6 +139,43 @@ func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTem
 	}
 }

+func TestUpdateContainerPort(t *testing.T) {
+	template, _ := getV1alpha1Config()
+	err := UpdateContainerPort(template, 8080)


Why do you use the default port for the update ? I think for clarities sake its better to use something different.

rhuss · 2019-06-18T07:22:07Z

pkg/serving/config_changes_test.go

+	// update template with container port info
+	for i, _ := range template.Spec.Containers {
+		for j, _ := range template.Spec.Containers[i].Ports {
+			template.Spec.Containers[i].Ports[j].ContainerPort = 9090


there is only 1 container and 1 port. If there are more, than that's an error.

rhuss · 2019-06-18T07:22:51Z

pkg/serving/config_changes_test.go

+	}
+
+	// Verify that given port overrides the existing container port
+	for p, _ := range template.Spec.Containers {


assert again, that there is only 1 container and 1 port after the update (you could put that into an extrac func as its needed twice)

rhuss · 2019-06-18T07:24:16Z

/ok-to-test

navidshaikh · 2019-06-20T11:30:56Z

pkg/serving/config_changes.go

+		return err
+	}
+
+	if len(container.Ports) == 0 {


@savitaashture : I think @rhuss mentioned to worry about the len of container ports, just replace it with container.Ports[0].ContainerPort = port

@navidshaikh this condition i have added because while executing
kn service create --image imagename --port 80
during that time contanier.Ports is nil so iam adding that condition to check and initialize the container.Ports.

I would just then unconditionally set

container.Ports = []corev1.ContainerPort{{ ContainerPort: port, }}

in any case. This makes the code simple to read, and we are sure that only 1 port is set (although as mentioned the slice should have only one entry)

navidshaikh · 2019-06-20T11:34:19Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -102,6 +105,11 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co
 		return err
 	}

+	err = servinglib.UpdateContainerPort(template, p.Port)


Say I have configured my service to run on port 9090, now I want to update my service with an environment variable, this Apply gets called, irrespective of user intent to update the port, this will update the container port to default one (8080).

invoke this operation by flag check
if cmd.Flag().Changed('port') (as done for image above)

navidshaikh · 2019-06-20T11:47:26Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -54,12 +55,14 @@ func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) {
 	command.Flags().IntVar(&p.MaxScale, "max-scale", 0, "Maximal number of replicas.")
 	command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0, "Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.")
 	command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0, "Hard Limit of concurrent requests to be processed by a single replica.")
+	command.Flags().Int32VarP(&p.Port, "port", "p", 8080, "The port that the application should run.")


*The port where application listens on.

maximilien · 2019-06-20T20:35:14Z

pkg/kn/commands/service/configuration_edit_flags.go

 }

 func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) {
 	p.AddUpdateFlags(command)
 	command.Flags().BoolVar(&p.ForceCreate, "force", false, "Create service forcefully, replaces existing service if any.")
 	command.MarkFlagRequired("image")
+


why this extra line?

Added by mistake :) removed now.
Thanku

maximilien

Nice job overall. Needed and welcome addition to kn.

Did you consider also adding to the e2e test suite? Would be easy to change one of the created services update its port. Thanks.

savitaashture · 2019-06-21T05:07:39Z

Nice job overall. Needed and welcome addition to kn.

Did you consider also adding to the e2e test suite? Would be easy to change one of the created services update its port. Thanks.

@maximilien Thank you
I will add e2e testcases.

rhuss · 2019-06-21T08:50:01Z

Build error is the same as in #192 (comment) so let's wait until this if fixed and retest here again.

cppforlife · 2019-06-24T18:27:23Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -102,6 +104,13 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co
 		return err
 	}

+	if cmd.Flags().Changed("port") {


i think it's a bit weird for us to not set container port if we are going to specify it as a default in the above flag definition.

Right! Apply gets called for create as well update both.

@savitaashture: We'll probably need to identify the command in Apply definition.
cmd.Name() returns the command name, we can update this check to not set the port when cmd.Name() == "update" && !cmd.Flags().Changed("port") and set for all other cases.

WDYT ?

@cppforlife:

i think it's a bit weird for us to not set container port if we are going to specify it as a default in the above flag definition.

Do you mean, we should update the port value to default one for service update, even if user doesnt specify -p port for service update command ?

@savitaashture : or we just remove the default value of port and the mentioned conditional should suffice.

yes we can just remove default port value 8080 from the flag and if cmd.Flags().Changed("port") { condition will work for all the scenario.

i think it's best to rely on the server default and not specify default in the cli (shows up in -h). if user does specify the port configuration then it should be applied.

yes i have removed saying default value from (shows up in -h) and if user specify it will be applied.

csantanapr · 2019-06-25T16:53:44Z

Glad to see that this is being added. I just hit this problem where the container I want to use doesn't listen on port 8080

navidshaikh · 2019-06-27T06:39:00Z

/test pull-knative-client-integration-tests

rhuss

Except for some minor feature request the PR lgtm.

…dating service

knative-metrics-robot · 2019-06-27T15:55:32Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/configuration_edit_flags.go	82.4%	78.6%	-3.8
pkg/serving/config_changes.go	66.1%	67.2%	1.0

rhuss

👍

knative-prow-robot · 2019-06-27T16:23:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss, savitaashture

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhuss · 2019-06-27T16:24:42Z

@savitaashture Thanks a lot, looks good to me. I'm on the integration-test issue, and if this is resolved (it unrelated to this PR), lets merge it.

savitaashture · 2019-06-27T16:26:18Z

@rhuss Thank you :)

rhuss · 2019-06-28T16:16:30Z

/retest pull-knative-client-integration-tests

rhuss · 2019-06-28T16:24:04Z

/retest

rhuss · 2019-06-28T17:23:43Z

/lgtm

…dating service (knative#191)

Proof: https://prow.knative.dev/log?job=ci-knative-build-latency&id=1051941890796032000

* [release-v1.9] Add kn-plugin-source-kafka v1.9.1 * Update vendor dir

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 18, 2019

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 18, 2019

knative-prow-robot requested review from cppforlife and mattmoor June 18, 2019 06:03

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 18, 2019

rhuss requested changes Jun 18, 2019

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 18, 2019

rhuss self-assigned this Jun 18, 2019

savitaashture force-pushed the issue-142 branch 2 times, most recently from a97594e to 1bf28e6 Compare June 18, 2019 18:37

navidshaikh requested changes Jun 20, 2019

View reviewed changes

savitaashture force-pushed the issue-142 branch 2 times, most recently from a359585 to 911d1ca Compare June 20, 2019 16:01

maximilien reviewed Jun 20, 2019

View reviewed changes

mattmoor removed their request for review June 21, 2019 03:17

savitaashture force-pushed the issue-142 branch from 911d1ca to f6cb688 Compare June 21, 2019 05:04

savitaashture force-pushed the issue-142 branch from f6cb688 to 2b62a68 Compare June 21, 2019 05:23

savitaashture force-pushed the issue-142 branch 2 times, most recently from 93617ae to 3512d0c Compare June 24, 2019 17:07

cppforlife reviewed Jun 24, 2019

View reviewed changes

savitaashture force-pushed the issue-142 branch 3 times, most recently from e3d3cfd to 1134b3e Compare June 26, 2019 12:06

rhuss requested changes Jun 27, 2019

View reviewed changes

savitaashture force-pushed the issue-142 branch from 1134b3e to fac4d90 Compare June 27, 2019 15:52

Added changes to support container port information while creating/up…

6e421ae

…dating service

savitaashture force-pushed the issue-142 branch from fac4d90 to 6e421ae Compare June 27, 2019 15:53

rhuss approved these changes Jun 27, 2019

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2019

knative-prow-robot merged commit 4cc5c84 into knative:master Jun 28, 2019

maximilien pushed a commit to maximilien/client that referenced this pull request Jul 1, 2019

Added changes to support container port information while creating/up…

12de316

…dating service (knative#191)

navidshaikh mentioned this pull request Jul 10, 2019

No flags to support container port information while creating service #142

Closed

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

$(JOB_NAME) is invalid, use raw name (knative#191)

89f53d3

Proof: https://prow.knative.dev/log?job=ci-knative-build-latency&id=1051941890796032000

dsimansk added a commit to dsimansk/client that referenced this pull request Jul 3, 2023

[release-v1.9] Add kn-plugin-source-kafka v1.9.1 (knative#191)

d0d131f

* [release-v1.9] Add kn-plugin-source-kafka v1.9.1 * Update vendor dir

Added changes to support container port information while creating/updating service #191

Added changes to support container port information while creating/updating service #191

Conversation

savitaashture commented Jun 18, 2019

knative-prow-robot commented Jun 18, 2019

rhuss 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savitaashture Jun 18, 2019 • edited Loading

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

rhuss commented Jun 18, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

savitaashture commented Jun 21, 2019

rhuss commented Jun 21, 2019

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

csantanapr commented Jun 25, 2019

navidshaikh commented Jun 27, 2019

rhuss left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jun 27, 2019

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jun 27, 2019

rhuss commented Jun 27, 2019

savitaashture commented Jun 27, 2019

rhuss commented Jun 28, 2019

rhuss commented Jun 28, 2019

rhuss commented Jun 28, 2019

savitaashture Jun 18, 2019 •

edited

Loading