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.

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

Feature: Add memory/cpu configuration to "store deploy" #670

Open
martindekov opened this issue Aug 10, 2019 · 12 comments
Open

Feature: Add memory/cpu configuration to "store deploy" #670

martindekov opened this issue Aug 10, 2019 · 12 comments

Comments

@martindekov
Copy link
Contributor

If I want to apply a memory limit, or CPU limit I would need to create YAML file and deploy my function.

Expected Behaviour

I would like to directly apply the CPU/memory limit of a function.

Current Behaviour

You need to define them in stack the yaml.

Possible Solution

Add two new flags to the deploy command in here my suggestion would be --cpu 10m --memory 20m which will overwrite the place above, the priority should be discussed I would go with command>yaml

Steps to Reproduce (for bugs)

It is not a bug, per se

Context

Requested

Your Environment

  • FaaS-CLI version ( Full output from: faas-cli version ):
    N/A
  • Docker version ( Full output from: docker version ):
    N/A
  • Are you using Docker Swarm (FaaS-swarm ) or Kubernetes (FaaS-netes)?
    N/A
  • Operating System and version (e.g. Linux, Windows, MacOS):
    N/A
  • Link to your project or a code example to reproduce issue:
    N/A
@alexellis
Copy link
Member

Still looking for a volunteer for this.

@martindekov
Copy link
Contributor Author

I can take a shot at this 👍

@rajibmitra
Copy link

I would love to work with you @martindekov

@alexellis alexellis changed the title Feature: Add memory configuration to the cli Feature: Add memory/cpu configuration to "store deploy" Aug 21, 2019
@alexellis
Copy link
Member

The deploy command has this already:

https://github.com/openfaas/faas-cli/blob/master/commands/deploy.go

The store deploy command is lacking this:

https://github.com/openfaas/faas-cli/blob/master/commands/store_deploy.go

The task would be to:

  • Add the new parameters/flags
  • Build a dev version of the CLI
  • Test that the new flags work by using kubectl describe -n openfaas-fn <pod-name>
  • Update any broken tests by running build_redist.sh to check
  • Check that the existing behaviour of no limits by default still works
  • Send a PR with a signed-off commit (simply: git commit -s)

@alexellis
Copy link
Member

It seems like I was wrong. We don't have the flags on either of the commands yet.

So this task is going to be a bit bigger, but not a lot.

please can you make some suggestions for the flags that you think we should use? Cc @rgee0 (if you have an opinion)

There is both a request value and a limit value that can be added and I think these should closely mirror the names in the YAML.

@rajibmitra
Copy link

@alexellis I like the flags options mentioned by @martindekov --cpu 10m --memory 20m

@alexellis
Copy link
Member

I don't know if you are aware, but they are not simply "--cpu" - it's CPU request and CPU limit, so we will need 4 flags.

https://docs.openfaas.com/reference/yaml/#function-memorycpu-limits

@alexellis
Copy link
Member

Perhaps --cpu-limit --cpu-request ?

If think it is a smart idea, as a follow-up PR (afterwards) we could add --cpu where it populates both fields?

@rgee0
Copy link
Contributor

rgee0 commented Aug 21, 2019

I'll always advocate consistency between the YAML and the CLI - for example build_options and build-options drive me crazy. I'm torn here because --cpu-limit is more idiomatic but the YAML path would switch them over (with good reason).

I'm interested in how the feature behaves where a stack contains multiple functions. Potentially the MO will be different to that of the YAML. Then there's how this behaves with a command > yaml precedence. If a user wants to deploy a suite of functions where some are less important/needy than others, then should the command honour the YAML and use the command only for functions which don't have explicit values defined?

I'd suggest we stick with the original scope of this issue - store deploy - as this is a 1:1 scenario. The can then keep moving while we think a little more about whether and how deploy should work.

@alexellis
Copy link
Member

Related, but different feature/work - #675

@alexellis
Copy link
Member

build_options and build-options drive me crazy

This is YAML vs JSON?

@alexellis
Copy link
Member

On second thoughts I think @rgee0 is right.. let's keep this scoped to store deploy and not update the deploy command which can also use YAML as input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants