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

Add range options to --scale #1070

Closed
wants to merge 39 commits into from
Closed

Add range options to --scale #1070

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
3db3cdb
Add range options to --scale
Oct 22, 2020
7f91820
Updated scale flag so that the docs update with the correct information
Oct 22, 2020
2edf4c4
genereted docs instead of updating manually
Oct 22, 2020
84eb376
Update common github actions (#1072)
knative-automation Oct 26, 2020
abb75e4
fix(tekton e2e): Fix CONTAINER_REGISTRY name generation (#1074)
navidshaikh Oct 26, 2020
2b7631a
Updated --scale docs
Oct 26, 2020
33d9193
Broke up if/else into two if statements in tests
Oct 26, 2020
efa9e27
Added missing ")" in scale docs
Oct 26, 2020
537973f
Generated docs after updating configuration_edit_flags
Oct 26, 2020
ed67482
Add ResolvePodSpec to podspec.go and move the related utilities to po…
Oct 27, 2020
a75cb2c
Resolved setting either value to 0, re-ordered which gets set first f…
Oct 27, 2020
c31eac4
Fixing lint issues
Oct 28, 2020
6d00de6
Update common github actions (#1081)
knative-automation Oct 29, 2020
6ed76ae
uniform multiple writeSink into DescribeSink (#1075)
Oct 29, 2020
6fa4098
Split up helper function, updated wording in help section
Oct 29, 2020
ad4a7b1
Pin deps back to master (#1082)
navidshaikh Oct 30, 2020
d1552ee
updating to use the hack repo (#1084)
Oct 30, 2020
8ca97c7
Feature: "kn service apply" (#964)
rhuss Nov 2, 2020
c60b851
Export test refactor (#1069)
itsmurugappan Nov 2, 2020
8a0067d
upgrade to latest dependencies (#1085)
knative-automation Nov 3, 2020
3614bd6
upgrade to latest dependencies (#1088)
knative-automation Nov 3, 2020
5787e95
upgrade to latest dependencies (#1091)
knative-automation Nov 6, 2020
a0ddad9
Update common github actions (#1090)
knative-automation Nov 6, 2020
4b84de3
got rid of helper functions, cleaned up code so it is easier to read,…
Nov 6, 2020
26867f1
feat: Add channel sink prefix (#1092)
navidshaikh Nov 9, 2020
45ffade
Drop deprecated --async flag support (#1094)
navidshaikh Nov 9, 2020
b72e4be
feat: Add service import command (#1065)
dsimansk Nov 9, 2020
1f0e5f9
Add range options to --scale
Oct 22, 2020
0075a6e
Updated scale flag so that the docs update with the correct information
Oct 22, 2020
629b995
genereted docs instead of updating manually
Oct 22, 2020
a6279dc
Updated --scale docs
Oct 26, 2020
5da8f3c
Broke up if/else into two if statements in tests
Oct 26, 2020
9567f05
Added missing ")" in scale docs
Oct 26, 2020
0ede82b
Generated docs after updating configuration_edit_flags
Oct 26, 2020
b3e962a
Resolved setting either value to 0, re-ordered which gets set first f…
Oct 27, 2020
871ba72
Fixing lint issues
Oct 28, 2020
55c9980
Split up helper function, updated wording in help section
Oct 29, 2020
be8e590
got rid of helper functions, cleaned up code so it is easier to read,…
Nov 6, 2020
3a0c74e
Merge branch 'issue-822-pt3' of github.com:mpetason/client into issue…
Nov 9, 2020
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 docs/cmd/kn_service_create.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ kn service create NAME --image IMAGE
--requests-cpu string DEPRECATED: please use --request instead. The requested CPU (e.g., 250m).
--requests-memory string DEPRECATED: please use --request instead. The requested memory (e.g., 64Mi).
--revision-name string The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
--scale int Minimum and maximum number of replicas.
--scale string Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, or set a range with min/max values, or set either min or max values without specifying the other. Example: --scale 5 (scale-min = 5, scale-max = 5) or --scale 1..5 (scale-min = 1, scale-max = 5) or --scale 1.. (scale-min = 1, scale-max = undefined) or --scale ..5 (scale-min = undefined, scale-max = 5) (default "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 think saying “undefined” isn’t as clear as saying that the current value is unchanged. And there is no default value so I would remove that.

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 think that is is "unchanged" as we are here talking about a create operation. Also for an update operation I still think that --scale 1.. mean min-scale is set to one, but no max-scale is set or removed if set, so that there is no upper bound specified by the user (regardless what was specified before). If you want to change only the minimal scale, you should use --min-scale.

+1 that the default value 1 is wrong as there is supposed to be no default at all, the argument is mandatory.

Copy link
Contributor

Choose a reason for hiding this comment

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

So maybe instead of scale-max = undefined, better use no maximum scale defined) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. @rhuss in the past I think you said you wanted 1.. to mean only the lower bound is set, and upper bound is untouched. That's why I commented the way I did - I thought you still wanted that.

Let's back up a bit... at one point people wanted to deprecate --scale-min and only have --scale - is that still in the cards or is that idea dead? If --scale-min/max will live on, then I'm ok with a missing value in .. meaning unset that value (or in the 'create' case it means just leave it as the default). And yes --scale .. would mean unset both :-)

--scale-init int Initial number of replicas with which a service starts. Can be 0 or a positive integer.
--scale-max int Maximum number of replicas.
--scale-min int Minimum number of replicas.
Expand Down
2 changes: 1 addition & 1 deletion docs/cmd/kn_service_update.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ kn service update NAME
--requests-cpu string DEPRECATED: please use --request instead. The requested CPU (e.g., 250m).
--requests-memory string DEPRECATED: please use --request instead. The requested memory (e.g., 64Mi).
--revision-name string The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
--scale int Minimum and maximum number of replicas.
--scale string Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, or set a range with min/max values, or set either min or max values without specifying the other. Example: --scale 5 (scale-min = 5, scale-max = 5) or --scale 1..5 (scale-min = 1, scale-max = 5) or --scale 1.. (scale-min = 1, scale-max = undefined) or --scale ..5 (scale-min = undefined, scale-max = 5) (default "1")
--scale-init int Initial number of replicas with which a service starts. Can be 0 or a positive integer.
--scale-max int Maximum number of replicas.
--scale-min int Minimum number of replicas.
Expand Down
55 changes: 49 additions & 6 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 @@ -15,6 +15,7 @@
package service

import (
"errors"
"fmt"
"strconv"
"strings"
Expand All @@ -39,7 +40,7 @@ type ConfigurationEditFlags struct {
PodSpecFlags knflags.PodSpecFlags

// Direct field manipulation
Scale int
Scale string
MinScale int
MaxScale int
ConcurrencyTarget int
Expand Down Expand Up @@ -88,7 +89,11 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
command.Flags().MarkHidden("max-scale")
p.markFlagMakesRevision("max-scale")

command.Flags().IntVar(&p.Scale, "scale", 0, "Minimum and maximum number of replicas.")
command.Flags().StringVar(&p.Scale, "scale", "1",
"Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, "+
"or set a range with min/max values, or set either min or max values without specifying the other. "+
"Example: --scale 5 (scale-min = 5, scale-max = 5) or --scale 1..5 (scale-min = 1, scale-max = 5) or --scale "+
"1.. (scale-min = 1, scale-max = undefined) or --scale ..5 (scale-min = undefined, scale-max = 5)")
p.markFlagMakesRevision("scale")

command.Flags().IntVar(&p.MinScale, "scale-min", 0, "Minimum number of replicas.")
Expand Down Expand Up @@ -358,13 +363,21 @@ func (p *ConfigurationEditFlags) Apply(
} else if cmd.Flags().Changed("scale-min") {
return fmt.Errorf("only --scale or --scale-min can be specified")
} else {
err = servinglib.UpdateMaxScale(template, p.Scale)
scaleMin, scaleMax, err := p.scaleConversion(p.Scale)
if err != nil {
return err
}
err = servinglib.UpdateMinScale(template, p.Scale)
if err != nil {
return err
if scaleMin != 0 {
err = servinglib.UpdateMinScale(template, scaleMin)
if err != nil {
return err
}
}
if scaleMax != 0 {
err = servinglib.UpdateMaxScale(template, scaleMax)
if err != nil {
return err
}
}
}
}
Expand Down Expand Up @@ -560,3 +573,33 @@ func (p *ConfigurationEditFlags) AnyMutation(cmd *cobra.Command) bool {
}
return false
}

// Helper function for --scale
func (p *ConfigurationEditFlags) scaleConversion(scale string) (scaleMin int, scaleMax int, err error) {
if len(scale) <= 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be to divide this into smaller private functions for each of the cases... might simply things. Also for each case returning early could also simply code since once case identified the result is there. No need for big if/else if/else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I break up the helper function into multiple private functions it will complicate the cmd.Flags().Changed("scale") section, is that ok? I was trying to minimize how large that code block was going to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on breaking this up next - I should be able to avoid complication in the cmd.Flags().Changed("scale") section.

if !strings.Contains(scale, "..") {
scaleMin, err = strconv.Atoi(scale)
if err != nil {
return 0, 0, err
}
scaleMax = scaleMin
}
} else if strings.Contains(scale, "..") {
scaleParts := strings.Split(scale, "..")
if scaleParts[0] != "" {
scaleMin, err = strconv.Atoi(scaleParts[0])
if err != nil {
return 0, 0, err
}
}
if scaleParts[1] != "" {
scaleMax, err = strconv.Atoi(scaleParts[1])
if err != nil {
return 0, 0, err
}
}
} else {
return 0, 0, errors.New("Scale must be of the format x..y or x")
}
return scaleMin, scaleMax, err
}
127 changes: 121 additions & 6 deletions pkg/kn/commands/service/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,8 +521,9 @@ func TestServiceCreateMaxMinScale(t *testing.T) {

if err != nil {
t.Fatal(err)
} else if !action.Matches("create", "services") {
t.Fatalf("Bad action %v", action)
}
if !action.Matches("create", "services") {
t.Fatal("Bad action ", action)
}

template := &created.Spec.Template
Expand Down Expand Up @@ -555,8 +556,9 @@ func TestServiceCreateScale(t *testing.T) {

if err != nil {
t.Fatal(err)
} else if !action.Matches("create", "services") {
t.Fatalf("Bad action %v", action)
}
if !action.Matches("create", "services") {
t.Fatal("Bad action ", action)
}

template := &created.Spec.Template
Expand All @@ -583,7 +585,7 @@ func TestServiceCreateScaleWithNegativeValue(t *testing.T) {
if err == nil {
t.Fatal(err)
}
expectedErrMsg := "expected 0 <= -1 <= 2147483647: autoscaling.knative.dev/maxScale"
expectedErrMsg := "expected 0 <= -1 <= 2147483647: autoscaling.knative.dev/minScale"
if !strings.Contains(err.Error(), expectedErrMsg) {
t.Errorf("Invalid error output, expected: %s, got : '%s'", expectedErrMsg, err)
}
Expand Down Expand Up @@ -618,6 +620,119 @@ func TestServiceCreateScaleWithMinScaleSet(t *testing.T) {

}

func TestServiceCreateScaleRange(t *testing.T) {
action, created, _, err := fakeServiceCreate([]string{
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
"--scale", "1..5", "--no-wait"}, false)

if err != nil {
t.Fatal(err)
}
if !action.Matches("create", "services") {
t.Fatal("Bad action ", action)
}

template := &created.Spec.Template

actualAnnos := template.Annotations
expectedAnnos := []string{
"autoscaling.knative.dev/minScale", "1",
"autoscaling.knative.dev/maxScale", "5",
}

for i := 0; i < len(expectedAnnos); i += 2 {
anno := expectedAnnos[i]
if actualAnnos[anno] != expectedAnnos[i+1] {
t.Fatalf("Unexpected annotation value for %s : %s (actual) != %s (expected)",
anno, actualAnnos[anno], expectedAnnos[i+1])
}
}
}

func TestServiceCreateScaleRangeOnlyMin(t *testing.T) {
action, created, _, err := fakeServiceCreate([]string{
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
"--scale", "1..", "--no-wait"}, false)

if err != nil {
t.Fatal(err)
}
if !action.Matches("create", "services") {
t.Fatal("Bad action ", action)
}

template := &created.Spec.Template

actualAnnos := template.Annotations
expectedAnnos := []string{
"autoscaling.knative.dev/minScale", "1",
}

for i := 0; i < len(expectedAnnos); i += 2 {
anno := expectedAnnos[i]
if actualAnnos[anno] != expectedAnnos[i+1] {
t.Fatalf("Unexpected annotation value for %s : %s (actual) != %s (expected)",
anno, actualAnnos[anno], expectedAnnos[i+1])
}
}
}

func TestServiceCreateScaleRangeOnlyMax(t *testing.T) {
action, created, _, err := fakeServiceCreate([]string{
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
"--scale", "..5", "--no-wait"}, false)

if err != nil {
t.Fatal(err)
}
if !action.Matches("create", "services") {
t.Fatal("Bad action ", action)
}

template := &created.Spec.Template

actualAnnos := template.Annotations
expectedAnnos := []string{
"autoscaling.knative.dev/maxScale", "5",
}

for i := 0; i < len(expectedAnnos); i += 2 {
anno := expectedAnnos[i]
if actualAnnos[anno] != expectedAnnos[i+1] {
t.Fatalf("Unexpected annotation value for %s : %s (actual) != %s (expected)",
anno, actualAnnos[anno], expectedAnnos[i+1])
}
}
}

func TestServiceCreateScaleRangeOnlyMinWrongSeparator(t *testing.T) {
_, _, _, err := fakeServiceCreate([]string{
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
"--scale", "1--", "--no-wait"}, true)
if err == nil {
t.Fatal(err)
}
expectedErrMsg := "Scale must be of the format x..y or x"
if !strings.Contains(err.Error(), expectedErrMsg) {
t.Errorf("Invalid error output, expected: %s, got : '%s'", expectedErrMsg, err)
}

}

func TestServiceCreateScaleRangeOnlyMaxWrongSeparator(t *testing.T) {
_, _, _, err := fakeServiceCreate([]string{
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
"--scale", "--1", "--no-wait"}, true)
if err == nil {
t.Fatal(err)
}
expectedErrMsg := "Scale must be of the format x..y or x"
if !strings.Contains(err.Error(), expectedErrMsg) {
t.Errorf("Invalid error output, expected: %s, got : '%s'", expectedErrMsg, err)
}

}

func TestServiceCreateRequestsLimitsCPUMemory(t *testing.T) {
action, created, _, err := fakeServiceCreate([]string{
"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
Expand Down Expand Up @@ -1054,5 +1169,5 @@ func TestServiceCreateFromYAMLWithOverrideError(t *testing.T) {
_, _, _, err = fakeServiceCreate([]string{
"service", "create", "foo", "--filename", tempFile, "--scale", "-1"}, false)
assert.Assert(t, err != nil)
assert.Assert(t, util.ContainsAll(err.Error(), "expected", "0", "<=", "2147483647", "autoscaling.knative.dev/maxScale"))
assert.Assert(t, util.ContainsAll(err.Error(), "expected 0 <= -1 <= 2147483647: autoscaling.knative.dev/minScale"))
}
Loading