fprocess cuts out command options which contain '='

[omrishtam]

Setting a command to fprocess which is containing a '=' character is being cut out after the '=' character.

Expected Behaviour

When setting fprocess="node --max_old_space_size=4096 index.js" it should execute the index.js script with the given --max_old_space_size option set to whatever value is given to it (4096 in this example).

Current Behaviour

When setting fprocess="node --max_old_space_size=4096 index.js" it fails to execute the command and gives the following output when sending a request to the function:

$ fprocess="node --max_old_space_size=4096 index.js" ./of-watchdog
2018/12/28 15:29:40 OperationalMode: streaming
2018/12/28 15:29:40 Writing lock-file to: /tmp/.lock
2018/12/28 15:29:45 Running node
2018/12/28 15:29:45 Started logging stderr from function.
2018/12/28 15:29:45 stderr: Error: missing value for flag --max_old_space_size of type int
Try --help for options
node: bad option: --max_old_space_size

2018/12/28 15:29:45 Error reading stderr: read |0: file already closed
2018/12/28 15:29:45 Took 0.008821 secs
2018/12/28 15:29:45 exit status 9

Cause of the issue

The issue is caused by the mapEnv function in config.go which is called with a slice of strings of the environment variables in key=value form. It splits the fprocess environmemnt variable with a '=' sign separator to map fprocess as a key to the command as a value. When the command is containing a '=' sign it splits the string to unknown number of substrings, but the mapped value (the command which is ran by of-watchdog) is only based on the first substring. This results to fprocess=node --max_old_space_size=4096 index.js being mapped as mapped["fprocess"]="node --max_old_space_size" which causes the error.

Possible Solution

Possible fixes for the mapEnv function in https://github.com/openfaas-incubator/of-watchdog/blob/85505a7210cf413e455f8a03d74ba94d9a9fcd30/config/config.go#L89-L101

1. strings.Join in line 97:

mapped[parts[0]] = strings.Join(parts[1:], "=")

2. strings.SplitN in line 93:

parts := strings.SplitN(val, "=", 2)

Both would result to the mapping correctly as mapped["fprocess"]="node --max_old_space_size=4096 index.js".

3. setting an environment variable for the function (temporal fix):

It's possible to set environment variables in the function's YAML file as described here: https://github.com/openfaas-incubator/node10-express-template For example node can set NODE_OPTIONS="--max-old-space-size=4096" to use the option, though this may not be suitable for all use cases.

Benchmarks

Benchmark for solution 1 (strings.Join):

func BenchmarkJoin(b *testing.B) {
    val := "fprocess=node --max_old_space_size=4096 index.js"
    parts := strings.Split(val, "=")
    for n := 0; n < b.N; n++ {
        strings.Join(parts[1:], "=")
    }
}

Result:

Running tool: C:\Go\bin\go.exe test -benchmem -run=^$ benchmarks -bench ^(BenchmarkJoin)$

goos: windows
goarch: amd64
pkg: benchmarks
BenchmarkJoin-8     30000000            45.8 ns/op        48 B/op          1 allocs/op
PASS
ok      benchmarks  1.557s
Success: Benchmarks passed.

Benchmark for solution 2 (strings.SplitN):

func BenchmarkSplitn(b *testing.B) {
    val := "fprocess=node --max_old_space_size=4096 index.js"
    for n := 0; n < b.N; n++ {
        strings.SplitN(val, "=", 2)
    }
}

Result:

Running tool: C:\Go\bin\go.exe test -benchmem -run=^$ benchmarks -bench ^(BenchmarkSplitn)$

goos: windows
goarch: amd64
pkg: benchmarks
BenchmarkSplitn-8       20000000            63.0 ns/op        32 B/op          1 allocs/op
PASS
ok      benchmarks  1.495s
Success: Benchmarks passed.

Overall the use of strings.SplitN (solution 2) to solve this issue seems better.

Steps to Reproduce (for bugs)

1. git clone https://github.com/openfaas-incubator/of-watchdog.git
2. go get -u github.com/openfaas-incubator/of-watchdog/config
3. go get -u github.com/openfaas-incubator/of-watchdog/executor
4. go build
5. fprocess="node --max_old_space_size=4096 index.js" ./of-watchdog

Context

I've deployed a nodeJS function based on https://github.com/openfaas-incubator/node10-express-template which required an increase of the maximum memory allocation for the function to run correctly. I encountered this issue and its temporal fix (solution 3) by setting the --max_old_space_size=4096 option which caused the issue and forced me to set it as an environment variable for the function in the YAML file.

Your Environment

• docker version 18.06

• I'm using Docker Swarm (FaaS-netes)

• Operating System and version (e.g. Linux, Windows, MacOS): Ubuntu 16.04, Windows 10 Build 17134

[alexellis]

Hi if you are changing the fprocess then you must have your own template.

Why don't you create an entrypoint.sh bash script that runs node with all the various options you care about? An alternative is to look at which of these variables are available as environmental variables and to set them that way.

I do this for debugging Node.js

Hope these two solutions are useful for you.

Alex

[alexellis]

Who is @omerzamir? Here are two viable solutions within 30 minutes of the issue being created.

[alexellis]

@omrishtam I've taken a look at the code and written some unit tests to cover the scenario you raised.

I'll be pushing a fix to address the parsing of the arguments array from fprocess, but using the environmental variable is what I would recommend for you so that you don't have to hack on or fork the templates and then maintain them out of tree from upstream.

You can add NODE_OPTIONS easily without changing any templates by adding it in the environment section of your YAML file or by adding an external environment_file - this currently has to be a local file, but I would accept a PR to allow this to be a HTTP/s URL if that made things easier for you.

Thanks for your thorough suggestions on a fix - I think in this case the benchmarks won't be as relevant since the code only runs once per of-watchdog start.

If you and @omerzamir would like to contribute please join Slack and check out the contributors' getting started guide -> https://docs.openfaas.com/contributing/get-started/

Alex

[omrishtam]

Hi 😄, Thanks for the quick response, I'll be using the environment section in my YAML file for now, until a fix is released.

Omri

[alexellis]

I've published a new release as: https://github.com/openfaas-incubator/of-watchdog/releases/tag/0.4.6

Alex

[alexellis]

We welcome contributions to the project, an initial contribution could be to update the of-watchdog version in some of all of the incubator templates listed in the template store?

https://github.com/openfaas/store/blob/master/templates.json

Would either of you like this or should I find someone else? (please note you have to type in git commit -s rather than git commit or editing in the UI for changes to be accepted by the Developer-Certificate of Origin that OpenFaaS uses.

Regards,

Alex

[omrishtam]

Sure! How do you want us to do this? Should we clone each repo and branch from master and make the change for the Dockerfiles and open a PR for the commit?

Omri

[alexellis]

That's basically the pattern right now. We could do with thinking about whether it can be automated in the future.