diff --git a/pkg/component/component.go b/pkg/component/component.go index 7b58b4e6bb0..e880fcdb303 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -530,9 +530,31 @@ func ValidateComponentCreateRequest(client *occlient.Client, componentSettings c } } + if err := ensureAndLogProperResourceUsage(componentSettings.MinMemory, componentSettings.MaxMemory, "memory"); err != nil { + return err + } + + if err := ensureAndLogProperResourceUsage(componentSettings.MinCPU, componentSettings.MaxCPU, "cpu"); err != nil { + return err + } + return } +func ensureAndLogProperResourceUsage(resourceMin, resourceMax *string, resourceName string) error { + klog.V(4).Infof("Validating configured %v values", resourceName) + + err := fmt.Errorf("`min%s` should accompany `max%s` or use `odo config set %s` to use same value for both min and max", resourceName, resourceName, resourceName) + + if (resourceMin == nil) != (resourceMax == nil) { + return err + } + if (resourceMin != nil && *resourceMin == "") != (resourceMax != nil && *resourceMax == "") { + return err + } + return nil +} + // ApplyConfig applies the component config onto component dc // Parameters: // client: occlient instance diff --git a/pkg/odo/cli/component/create.go b/pkg/odo/cli/component/create.go index 4257e4a65af..00e6eaa404f 100644 --- a/pkg/odo/cli/component/create.go +++ b/pkg/odo/cli/component/create.go @@ -47,18 +47,9 @@ type CreateOptions struct { componentContext string componentPorts []string componentEnvVars []string - memoryMax string - - memoryMin string - - appName string - - memory string - cpuMax string - cpuMin string - cpu string - interactive bool - now bool + appName string + interactive bool + now bool *CommonPushOptions devfileMetadata DevfileMetadata } @@ -142,8 +133,8 @@ Note: When you use odo with experimental mode enabled and create devfile compone # Create new Node.js component with source from remote git repository %[1]s nodejs --git https://github.com/openshift/nodejs-ex.git -# Create new Node.js component with custom ports, additional environment variables and memory and cpu limits -%[1]s nodejs --port 8080,8100/tcp,9100/udp --env key=value,key1=value1 --memory 4Gi --cpu 2 +# Create new Node.js component with custom ports and environment variables +%[1]s nodejs --port 8080,8100/tcp,9100/udp --env key=value,key1=value1 # Create new Node.js component and download the sample project named nodejs-starter %[1]s nodejs --starter=nodejs-starter`) @@ -304,36 +295,6 @@ func createDefaultComponentName(context *genericclioptions.Context, componentTyp return componentName, nil } -func (co *CreateOptions) setResourceLimits() error { - ensureAndLogProperResourceUsage(co.memory, co.memoryMin, co.memoryMax, "memory") - - ensureAndLogProperResourceUsage(co.cpu, co.cpuMin, co.cpuMax, "cpu") - - memoryQuantity, err := util.FetchResourceQuantity(corev1.ResourceMemory, co.memoryMin, co.memoryMax, co.memory) - if err != nil { - return err - } - if memoryQuantity != nil { - minMemory := memoryQuantity.MinQty.String() - maxMemory := memoryQuantity.MaxQty.String() - co.componentSettings.MinMemory = &minMemory - co.componentSettings.MaxMemory = &maxMemory - } - - cpuQuantity, err := util.FetchResourceQuantity(corev1.ResourceCPU, co.cpuMin, co.cpuMax, co.cpu) - if err != nil { - return err - } - if cpuQuantity != nil { - minCPU := cpuQuantity.MinQty.String() - maxCPU := cpuQuantity.MaxQty.String() - co.componentSettings.MinCPU = &minCPU - co.componentSettings.MaxCPU = &maxCPU - } - - return nil -} - // Complete completes create args func (co *CreateOptions) Complete(name string, cmd *cobra.Command, args []string) (err error) { @@ -761,10 +722,6 @@ func (co *CreateOptions) Complete(name string, cmd *cobra.Command, args []string if err != nil { return err } - err = co.setResourceLimits() - if err != nil { - return err - } var portList []string if len(co.componentPorts) > 0 { @@ -1082,33 +1039,6 @@ func (co *CreateOptions) Run() (err error) { return } -// The general cpu/memory is used as a fallback when it's set and both min-cpu/memory max-cpu/memory are not set -// when the only thing specified is the min or max value, we exit the application -func ensureAndLogProperResourceUsage(resource, resourceMin, resourceMax, resourceName string) { - if strings.HasPrefix(resourceMin, "-") { - log.Errorf("min-%s cannot be negative", resource) - os.Exit(1) - } - if strings.HasPrefix(resourceMax, "-") { - log.Errorf("max-%s cannot be negative", resource) - os.Exit(1) - } - if strings.HasPrefix(resource, "-") { - log.Errorf("%s cannot be negative", resource) - os.Exit(1) - } - if resourceMin != "" && resourceMax != "" && resource != "" { - log.Infof("`--%s` will be ignored as `--min-%s` and `--max-%s` has been passed\n", resourceName, resourceName, resourceName) - } - if (resourceMin == "") != (resourceMax == "") && resource != "" { - log.Infof("Using `--%s` %s for min and max limits.\n", resourceName, resource) - } - if (resourceMin == "") != (resourceMax == "") && resource == "" { - log.Errorf("`--min-%s` should accompany `--max-%s` or pass `--%s` to use same value for both min and max or try not passing any of them\n", resourceName, resourceName, resourceName) - os.Exit(1) - } -} - func checkoutProject(sparseCheckoutDir, zipURL, path string) error { if sparseCheckoutDir != "" { @@ -1144,12 +1074,6 @@ func NewCmdCreate(name, fullName string) *cobra.Command { componentCreateCmd.Flags().StringVarP(&co.componentGit, "git", "g", "", "Create a git component using this repository.") componentCreateCmd.Flags().StringVarP(&co.componentGitRef, "ref", "r", "", "Use a specific ref e.g. commit, branch or tag of the git repository (only valid for --git components)") genericclioptions.AddContextFlag(componentCreateCmd, &co.componentContext) - componentCreateCmd.Flags().StringVar(&co.memory, "memory", "", "Amount of memory to be allocated to the component. ex. 100Mi (sets min-memory and max-memory to this value)") - componentCreateCmd.Flags().StringVar(&co.memoryMin, "min-memory", "", "Limit minimum amount of memory to be allocated to the component. ex. 100Mi") - componentCreateCmd.Flags().StringVar(&co.memoryMax, "max-memory", "", "Limit maximum amount of memory to be allocated to the component. ex. 100Mi") - componentCreateCmd.Flags().StringVar(&co.cpu, "cpu", "", "Amount of cpu to be allocated to the component. ex. 100m or 0.1 (sets min-cpu and max-cpu to this value)") - componentCreateCmd.Flags().StringVar(&co.cpuMin, "min-cpu", "", "Limit minimum amount of cpu to be allocated to the component. ex. 100m") - componentCreateCmd.Flags().StringVar(&co.cpuMax, "max-cpu", "", "Limit maximum amount of cpu to be allocated to the component. ex. 1") componentCreateCmd.Flags().StringSliceVarP(&co.componentPorts, "port", "p", []string{}, "Ports to be used when the component is created (ex. 8080,8100/tcp,9100/udp)") componentCreateCmd.Flags().StringSliceVar(&co.componentEnvVars, "env", []string{}, "Environmental variables for the component. For example --env VariableName=Value") diff --git a/pkg/odo/cli/config/set.go b/pkg/odo/cli/config/set.go index b95deaaa81d..b670c8dfe35 100644 --- a/pkg/odo/cli/config/set.go +++ b/pkg/odo/cli/config/set.go @@ -170,6 +170,11 @@ func isValidArgumentList(args []string) error { } switch param { + case "memory", "minmemory", "maxmemory", "cpu", "mincpu", "maxcpu": + err = validation.NonNegativeValidator(args[1]) + if err != nil { + err = errors.Errorf("%s is invalid %v", param, err) + } case "ports", "debugport": err = validation.PortsValidator(args[1]) } diff --git a/pkg/odo/util/validation/validators.go b/pkg/odo/util/validation/validators.go index 36f9bc9d71f..ec3dbf43bff 100644 --- a/pkg/odo/util/validation/validators.go +++ b/pkg/odo/util/validation/validators.go @@ -2,10 +2,11 @@ package validation import ( "fmt" - "github.com/openshift/odo/pkg/util" - "gopkg.in/AlecAivazis/survey.v1" "strconv" "strings" + + "github.com/openshift/odo/pkg/util" + "gopkg.in/AlecAivazis/survey.v1" ) // NameValidator provides a Validator view of the ValidateName function. @@ -40,6 +41,16 @@ func IntegerValidator(ans interface{}) error { return fmt.Errorf("don't know how to convert %v into an integer", ans) } +// NonNegativeValidator validates whether the given value is not negative or not +func NonNegativeValidator(arg interface{}) error { + if s, ok := arg.(string); ok { + if strings.HasPrefix(s, "-") { + return fmt.Errorf("negative value is not acceptable") + } + } + return nil +} + // PathValidator validates whether the given path exists on the file system func PathValidator(path interface{}) error { if s, ok := path.(string); ok { diff --git a/tests/e2escenarios/e2e_images_test.go b/tests/e2escenarios/e2e_images_test.go index e02aa023d89..7df9480cb46 100644 --- a/tests/e2escenarios/e2e_images_test.go +++ b/tests/e2escenarios/e2e_images_test.go @@ -41,7 +41,10 @@ var _ = Describe("odo supported images e2e tests", func() { // create the component helper.CopyExample(filepath.Join("source", srcType), context) - helper.CmdShouldPass("odo", "create", cmpType, srcType+"-app", "--project", project, "--context", context, "--app", appName, "--min-memory", "400Mi", "--max-memory", "700Mi") + helper.CmdShouldPass("odo", "create", cmpType, srcType+"-app", "--project", project, "--context", context, "--app", appName) + + helper.CmdShouldPass("odo", "config", "set", "minmemory", "400Mi", "--context", context) + helper.CmdShouldPass("odo", "config", "set", "maxmemory", "700Mi", "--context", context) // push component and validate helper.CmdShouldPass("odo", "push", "--context", context) diff --git a/tests/integration/cmd_push_test.go b/tests/integration/cmd_push_test.go index bb4a15dbd25..4b184f67b5f 100644 --- a/tests/integration/cmd_push_test.go +++ b/tests/integration/cmd_push_test.go @@ -53,6 +53,72 @@ var _ = Describe("odo push command tests", func() { }) + Context("Check memory and cpu config before odo push", func() { + It("Should work when memory is set..", func() { + + helper.CopyExample(filepath.Join("source", "nodejs"), context) + + helper.CmdShouldPass("odo", "component", "create", "nodejs", cmpName, "--project", project, "--context", context, "--app", appName) + + helper.CmdShouldPass("odo", "config", "set", "Memory", "300Mi", "--context", context) + helper.CmdShouldPass("odo", "push", "--context", context) + }) + + It("Should fail if minMemory is set but maxmemory is not set..", func() { + + helper.CopyExample(filepath.Join("source", "nodejs"), context) + + helper.CmdShouldPass("odo", "component", "create", "nodejs", cmpName, "--project", project, "--context", context, "--app", appName) + + helper.CmdShouldPass("odo", "config", "set", "minmemory", "100Mi", "--context", context) + output := helper.CmdShouldFail("odo", "push", "--context", context) + Expect(output).To(ContainSubstring("`minmemory` should accompany `maxmemory` or use `odo config set memory` to use same value for both min and max")) + }) + + It("should fail if maxmemory is set but minmemory is not set..", func() { + + helper.CopyExample(filepath.Join("source", "nodejs"), context) + + helper.CmdShouldPass("odo", "component", "create", "nodejs", cmpName, "--project", project, "--context", context, "--app", appName) + + helper.CmdShouldPass("odo", "config", "set", "maxmemory", "400Mi", "--context", context) + output := helper.CmdShouldFail("odo", "push", "--context", context) + Expect(output).To(ContainSubstring("`minmemory` should accompany `maxmemory` or use `odo config set memory` to use same value for both min and max")) + }) + + It("Should work when cpu is set", func() { + + helper.CopyExample(filepath.Join("source", "nodejs"), context) + + helper.CmdShouldPass("odo", "component", "create", "nodejs", cmpName, "--project", project, "--context", context, "--app", appName) + + helper.CmdShouldPass("odo", "config", "set", "cpu", "0.4", "--context", context) + helper.CmdShouldPass("odo", "push", "--context", context) + }) + + It("Should fail if mincpu is set but maxcpu is not set..", func() { + + helper.CopyExample(filepath.Join("source", "nodejs"), context) + + helper.CmdShouldPass("odo", "component", "create", "nodejs", cmpName, "--project", project, "--context", context, "--app", appName) + + helper.CmdShouldPass("odo", "config", "set", "mincpu", "0.4", "--context", context) + output := helper.CmdShouldFail("odo", "push", "--context", context) + Expect(output).To(ContainSubstring("`mincpu` should accompany `maxcpu` or use `odo config set cpu` to use same value for both min and max")) + }) + + It("should fail if maxcpu is set but mincpu is not set..", func() { + + helper.CopyExample(filepath.Join("source", "nodejs"), context) + + helper.CmdShouldPass("odo", "component", "create", "nodejs", cmpName, "--project", project, "--context", context, "--app", appName) + + helper.CmdShouldPass("odo", "config", "set", "maxcpu", "0.5", "--context", context) + output := helper.CmdShouldFail("odo", "push", "--context", context) + Expect(output).To(ContainSubstring("`mincpu` should accompany `maxcpu` or use `odo config set cpu` to use same value for both min and max")) + }) + }) + Context("Check for label propagation after pushing", func() { It("Check for labels", func() { diff --git a/tests/integration/component.go b/tests/integration/component.go index 361409c2430..e371295112e 100644 --- a/tests/integration/component.go +++ b/tests/integration/component.go @@ -192,8 +192,8 @@ func componentTests(args ...string) { }) It("should list the component", func() { - helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--git", "https://github.com/openshift/nodejs-ex", "--min-memory", "100Mi", "--max-memory", "300Mi", "--min-cpu", "0.1", "--max-cpu", "2", "--context", context, "--app", "testing")...) - helper.ValidateLocalCmpExist(context, "Type,nodejs", "Name,cmp-git", "Application,testing", "MaxMemory,300Mi") + helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--git", "https://github.com/openshift/nodejs-ex", "--context", context, "--app", "testing")...) + helper.ValidateLocalCmpExist(context, "Type,nodejs", "Name,cmp-git", "Application,testing") helper.CmdShouldPass("odo", append(args, "push", "--context", context)...) cmpList := helper.CmdShouldPass("odo", append(args, "list", "--project", project)...) @@ -207,15 +207,15 @@ func componentTests(args ...string) { }) It("should list the component when it is not pushed", func() { - helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--git", "https://github.com/openshift/nodejs-ex", "--min-memory", "100Mi", "--max-memory", "300Mi", "--min-cpu", "0.1", "--max-cpu", "2", "--context", context, "--app", "testing")...) - helper.ValidateLocalCmpExist(context, "Type,nodejs", "Name,cmp-git", "Application,testing", "MinCPU,100m") + helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--git", "https://github.com/openshift/nodejs-ex", "--context", context, "--app", "testing")...) + helper.ValidateLocalCmpExist(context, "Type,nodejs", "Name,cmp-git", "Application,testing") cmpList := helper.CmdShouldPass("odo", append(args, "list", "--context", context)...) helper.MatchAllInOutput(cmpList, []string{"cmp-git", "Not Pushed"}) helper.CmdShouldPass("odo", append(args, "delete", "-f", "--all", "--context", context)...) }) It("should list the state as unknown for disconnected cluster", func() { - helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--git", "https://github.com/openshift/nodejs-ex", "--min-memory", "100Mi", "--max-memory", "300Mi", "--min-cpu", "0.1", "--max-cpu", "2", "--context", context, "--app", "testing")...) - helper.ValidateLocalCmpExist(context, "Type,nodejs", "Name,cmp-git", "Application,testing", "MinCPU,100m") + helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--git", "https://github.com/openshift/nodejs-ex", "--context", context, "--app", "testing")...) + helper.ValidateLocalCmpExist(context, "Type,nodejs", "Name,cmp-git", "Application,testing") kubeconfigOrig := os.Getenv("KUBECONFIG") os.Setenv("KUBECONFIG", "/no/such/path") cmpList := helper.CmdShouldPass("odo", append(args, "list", "--context", context, "--v", "9")...) @@ -596,12 +596,8 @@ func componentTests(args ...string) { It("should be able to create a git component and update it from local to git", func() { helper.CopyExample(filepath.Join("source", "nodejs"), context) - helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--min-cpu", "0.1", "--max-cpu", "2", "--context", context, "--app", "testing")...) + helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--context", context, "--app", "testing")...) helper.CmdShouldPass("odo", append(args, "push", "--context", context)...) - getCPULimit := oc.MaxCPU("cmp-git", "testing", project) - Expect(getCPULimit).To(ContainSubstring("2")) - getCPURequest := oc.MinCPU("cmp-git", "testing", project) - Expect(getCPURequest).To(ContainSubstring("100m")) helper.CmdShouldPass("odo", "update", "--git", "https://github.com/openshift/nodejs-ex.git", "--context", context) // check the source location and type in the deployment config @@ -612,23 +608,14 @@ func componentTests(args ...string) { }) It("should be able to update a component from git to local", func() { - helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--git", "https://github.com/openshift/nodejs-ex", "--min-memory", "100Mi", "--max-memory", "300Mi", "--context", context, "--app", "testing")...) + helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--git", "https://github.com/openshift/nodejs-ex", "--context", context, "--app", "testing")...) helper.CmdShouldPass("odo", append(args, "push", "--context", context)...) - getMemoryLimit := oc.MaxMemory("cmp-git", "testing", project) - Expect(getMemoryLimit).To(ContainSubstring("300Mi")) - getMemoryRequest := oc.MinMemory("cmp-git", "testing", project) - Expect(getMemoryRequest).To(ContainSubstring("100Mi")) // update the component config according to the git component helper.CopyExample(filepath.Join("source", "nodejs"), context) helper.CmdShouldPass("odo", "update", "--local", "./", "--context", context) - getMemoryLimit = oc.MaxMemory("cmp-git", "testing", project) - Expect(getMemoryLimit).To(ContainSubstring("300Mi")) - getMemoryRequest = oc.MinMemory("cmp-git", "testing", project) - Expect(getMemoryRequest).To(ContainSubstring("100Mi")) - // check the source location and type in the deployment config getSourceLocation := oc.SourceLocationDC("cmp-git", "testing", project) Expect(getSourceLocation).To(ContainSubstring("")) @@ -758,20 +745,6 @@ func componentTests(args ...string) { }) }) - Context("when creating component with improper memory quantities", func() { - JustBeforeEach(func() { - project = helper.CreateRandProject() - }) - JustAfterEach(func() { - helper.DeleteProject(project) - }) - It("should fail gracefully with proper error message", func() { - stdError := helper.CmdShouldFail("odo", append(args, "create", "java:8", "backend", "--memory", "1GB", "--project", project, "--context", context)...) - Expect(stdError).ToNot(ContainSubstring("panic: cannot parse")) - Expect(stdError).To(ContainSubstring("quantities must match the regular expression")) - }) - }) - Context("Creating component using symlink", func() { var symLinkPath string diff --git a/tests/integration/generic_test.go b/tests/integration/generic_test.go index b9b06b0b91f..9d1a1ad79f1 100644 --- a/tests/integration/generic_test.go +++ b/tests/integration/generic_test.go @@ -357,4 +357,63 @@ var _ = Describe("odo generic", func() { helper.CmdShouldPass("oc", "get", "is", dc, "--namespace", project) }) }) + + Context("When using cpu or memory flag with odo create", func() { + var originalDir string + cmpName := "nodejs" + + JustBeforeEach(func() { + context = helper.CreateNewContext() + os.Setenv("GLOBALODOCONFIG", filepath.Join(context, "config.yaml")) + project = helper.CreateRandProject() + originalDir = helper.Getwd() + helper.Chdir(context) + }) + + JustAfterEach(func() { + helper.DeleteProject(project) + helper.Chdir(originalDir) + helper.DeleteDir(context) + os.Unsetenv("GLOBALODOCONFIG") + }) + + It("should not allow using any memory or cpu flag", func() { + + cases := []struct { + paramName string + paramValue string + }{ + { + paramName: "cpu", + paramValue: "0.4", + }, + { + paramName: "mincpu", + paramValue: "0.2", + }, + { + paramName: "maxcpu", + paramValue: "0.4", + }, + { + paramName: "memory", + paramValue: "200Mi", + }, + { + paramName: "minmemory", + paramValue: "100Mi", + }, + { + paramName: "maxmemory", + paramValue: "200Mi", + }, + } + for _, testCase := range cases { + helper.CopyExample(filepath.Join("source", "nodejs"), context) + output := helper.CmdShouldFail("odo", "component", "create", "nodejs", cmpName, "--project", project, "--context", context, "--"+testCase.paramName, testCase.paramValue) + Expect(output).To(ContainSubstring("unknown flag: --" + testCase.paramName)) + } + }) + }) + })