faas-cli icon indicating copy to clipboard operation
faas-cli copied to clipboard

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

Open martindekov opened this issue 5 years ago • 12 comments

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

martindekov avatar Aug 10 '19 17:08 martindekov

Still looking for a volunteer for this.

alexellis avatar Aug 21 '19 07:08 alexellis

I can take a shot at this 👍

martindekov avatar Aug 21 '19 07:08 martindekov

I would love to work with you @martindekov

rajibmitra avatar Aug 21 '19 07:08 rajibmitra

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 avatar Aug 21 '19 07:08 alexellis

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.

alexellis avatar Aug 21 '19 16:08 alexellis

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

rajibmitra avatar Aug 21 '19 18:08 rajibmitra

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 avatar Aug 21 '19 18:08 alexellis

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?

alexellis avatar Aug 21 '19 18:08 alexellis

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.

rgee0 avatar Aug 21 '19 19:08 rgee0

Related, but different feature/work - https://github.com/openfaas/faas-cli/pull/675

alexellis avatar Aug 21 '19 21:08 alexellis

build_options and build-options drive me crazy

This is YAML vs JSON?

alexellis avatar Aug 21 '19 21:08 alexellis

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.

alexellis avatar Aug 21 '19 21:08 alexellis