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

cleanup #3403: removed --memory, --min-memory. --max-memory, --cpu, --min-cpu, --max-cpu flags from 'odo create' command #3475

Merged
merged 3 commits into from
Jul 31, 2020
Merged

cleanup #3403: removed --memory, --min-memory. --max-memory, --cpu, --min-cpu, --max-cpu flags from 'odo create' command #3475

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
808a31e
removed --cpu, --max-cpu,--max-memory,--memory,--max-memory,--min-mem…
devang-gaur Jul 8, 2020
ed11742
add memory and cpu validation for 'config set' and 'push' commands
devang-gaur Jul 15, 2020
f1ff208
Corrected test cases
devang-gaur Jul 28, 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
22 changes: 22 additions & 0 deletions pkg/component/component.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 or try not passing any of them\n", 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
Expand Down
86 changes: 5 additions & 81 deletions pkg/odo/cli/component/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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`)
Expand Down Expand Up @@ -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) {

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These validations might need to happen at push now, as a user can set them using odo config and then push

Copy link
Contributor

Choose a reason for hiding this comment

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

Or better yet when these are set using odo config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a new commit to add validation for push and config set command. Would that suffice?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah

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 != "" {
Expand Down Expand Up @@ -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")

Expand Down
5 changes: 5 additions & 0 deletions pkg/odo/cli/config/set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
Expand Down
15 changes: 13 additions & 2 deletions pkg/odo/util/validation/validators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 4 additions & 1 deletion tests/e2escenarios/e2e_images_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

e2e test stands for broader user specific scenario. These validation can be done through UTs or integration test label. So please remove those lines.

Ofcourse if we find a user specific scenario where we have such hard requirement of adding those validation then we will add these line for sure.

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 don't trim the memory and cpu flags from here then this will fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh...got it. I did not realize it. Sorry for the false alarm.


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)
Expand Down
88 changes: 88 additions & 0 deletions tests/integration/cmd_push_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,94 @@ var _ = Describe("odo push command tests", func() {

})

Context("Check memory and cpu config before odo push", func() {
It("Should work when both minmemory and maxmemory is set..", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove it. It has been covered in config integration test file.


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)
helper.CmdShouldPass("odo", "config", "set", "maxmemory", "200Mi", "--context", context)
helper.CmdShouldPass("odo", "push", "--context", context)
})

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 or try not passing any of them"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Message should be straight forward instead, right.

I would say also in the code implementation make the message pretty straight forward like minmemory should accompany maxmemory, so 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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these validation be done through UTs instead of over crowding the integration test file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it covers multiple packages. Not just config package. It cannot be covered in UTs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The memory and cpu is set in config package but the overall/e2e validation of config parameters occur in component push package


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 or try not passing any of them"))
})

It("Should work when both mincpu and maxcpu is set..", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove it,. It has been covered in config integration test file


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.2", "--context", context)
helper.CmdShouldPass("odo", "config", "set", "maxcpu", "0.5", "--context", context)
helper.CmdShouldPass("odo", "push", "--context", context)
})

It("Should work when cpu is set", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cna it be done through code level validation i mean through UTs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I don't think so, due to the same reason I mentioned above.


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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be done through code level validation i mean through UTs ?


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 or try not passing any of them"))
})

It("should fail if maxcpu is set but mincpu is not set..", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be done through code level validation i mean through UTs ?


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 or try not passing any of them"))
})
})

Context("Check for label propagation after pushing", func() {

It("Check for labels", func() {
Expand Down
Loading