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.

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

Jump to bottom

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

Merged
merged 1 commit into from
Jun 28, 2019
Merged

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

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
4 changes: 4 additions & 0 deletions docs/cmd/kn_service_create.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ kn service create NAME --image IMAGE [flags]
# 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 service 'mysvc' with port 80
kn service create mysvc --port 80 --image dev.local/ns/image:latest

# Create or replace default resources of a service 's1' using --force flag
# (earlier configured resource requests and limits will be replaced with default)
# (earlier configured environment variables will be cleared too if any)
Expand All @@ -47,6 +50,7 @@ kn service create NAME --image IMAGE [flags]
--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.
-p, --port int32 The port where application listens on.
--requests-cpu string The requested CPU (e.g., 250m).
--requests-memory string The requested CPU (e.g., 64Mi).
```
Expand Down
4 changes: 4 additions & 0 deletions docs/cmd/kn_service_update.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ kn service update NAME [flags]
# Updates a service 'mysvc' with new environment variables
kn service update mysvc --env KEY1=VALUE1 --env KEY2=VALUE2

# Update a service 'mysvc' with new port
kn service update mysvc --port 80

# Updates a service 'mysvc' with new requests and limits parameters
kn service update mysvc --requests-cpu 500m --limits-memory 1024Mi
```
Expand All @@ -34,6 +37,7 @@ kn service update NAME [flags]
--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.
-p, --port int32 The port where application listens on.
--requests-cpu string The requested CPU (e.g., 250m).
--requests-memory string The requested CPU (e.g., 64Mi).
```
Expand Down
9 changes: 9 additions & 0 deletions pkg/kn/commands/service/configuration_edit_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type ConfigurationEditFlags struct {
MaxScale int
ConcurrencyTarget int
ConcurrencyLimit int
Port int32
}

type ResourceFlags struct {
Expand All @@ -54,6 +55,7 @@ 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", 0, "The port where application listens on.")
}

func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) {
Expand Down Expand Up @@ -102,6 +104,13 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co
return err
}

if cmd.Flags().Changed("port") {
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

err = servinglib.UpdateContainerPort(template, p.Port)
if err != nil {
return err
}
}

servinglib.UpdateConcurrencyConfiguration(template, p.MinScale, p.MaxScale, p.ConcurrencyTarget, p.ConcurrencyLimit)

return nil
Expand Down
3 changes: 3 additions & 0 deletions pkg/kn/commands/service/service_create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ 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 service 'mysvc' with port 80
kn service create mysvc --port 80 --image dev.local/ns/image:latest

# Create or replace default resources of a service 's1' using --force flag
# (earlier configured resource requests and limits will be replaced with default)
# (earlier configured environment variables will be cleared too if any)
Expand Down
3 changes: 3 additions & 0 deletions pkg/kn/commands/service/service_update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 'mysvc' with new port
kn service update mysvc --port 80

# Updates a service 'mysvc' with new requests and limits parameters
kn service update mysvc --requests-cpu 500m --limits-memory 1024Mi`,
RunE: func(cmd *cobra.Command, args []string) (err error) {
Expand Down
12 changes: 12 additions & 0 deletions pkg/serving/config_changes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,18 @@ func UpdateImage(template *servingv1alpha1.RevisionTemplateSpec, image string) e
return nil
}

// UpdateContainerPort updates container with a give port
func UpdateContainerPort(template *servingv1alpha1.RevisionTemplateSpec, port int32) error {
container, err := extractContainer(template)
if err != nil {
return err
}
container.Ports = []corev1.ContainerPort{{
ContainerPort: port,
}}
return nil
}

func UpdateResources(template *servingv1alpha1.RevisionTemplateSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error {
container, err := extractContainer(template)
if err != nil {
Expand Down
24 changes: 24 additions & 0 deletions pkg/serving/config_changes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,30 @@ func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTem
}
}

func TestUpdateContainerPort(t *testing.T) {
template, _ := getV1alpha1Config()
err := UpdateContainerPort(template, 8888)
if err != nil {
t.Fatal(err)
}
// Verify update is successful or not
checkPortUpdate(t, template, 8888)
// update template with container port info
template.Spec.Containers[0].Ports[0].ContainerPort = 9090
err = UpdateContainerPort(template, 80)
if err != nil {
t.Fatal(err)
}
// Verify that given port overrides the existing container port
checkPortUpdate(t, template, 80)
}

func checkPortUpdate(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, port int32) {
if len(template.Spec.Containers) != 1 || template.Spec.Containers[0].Ports[0].ContainerPort != port {
t.Error("Failed to update the container port")
}
}

func TestUpdateEnvVarsBoth(t *testing.T) {
template, container := getV1alpha1RevisionTemplateWithOldFields()
testUpdateEnvVarsBoth(t, template, container)
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/basic_workflow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func TestBasicWorkflow(t *testing.T) {
testServiceCreate(t, k, "hello")
testServiceList(t, k, "hello")
testServiceDescribe(t, k, "hello")
testServiceUpdate(t, k, "hello", []string{"--env", "TARGET=kn"})
testServiceUpdate(t, k, "hello", []string{"--env", "TARGET=kn", "--port", "8888"})
testServiceCreate(t, k, "svc2")
testRevisionListForService(t, k, "hello")
testRevisionListForService(t, k, "svc2")
Expand Down