hashicorp / faas-nomad

OpenFaaS plugin for Nomad
https://www.openfaas.com
MIT License
255 stars 45 forks source link

Allow to change CPU arch constraint via commandline argument #82

Closed cryptobias closed 5 years ago

cryptobias commented 5 years ago

I was able to start up faas-nomad on my RPi after some fiddling with the configuration, but I unfortunately was not able to run a function without patching the hard-coded CPU architecture constraint.

This might suffice as an alternative to use OpenFaas with Nomad purely on one architecture (e.g. a RPi cluster) until https://github.com/hashicorp/faas-nomad/issues/18 is implemented.

hashicorp-cla commented 5 years ago

CLA assistant check
All committers have signed the CLA.

cryptobias commented 5 years ago

@acornies Instead of adding a custom constraint name for the CPU architecture I went with a more generic approach. This unfortunately does not support some of the configurations possible with Nomad's constraints, but it might be enough for most use-cases.

Things that are not working are:

I am also not sure if it is still necessary to restrict the architecture by default. If you run a mixed setup you will know and can adjust it via the function constraints.

cryptobias commented 5 years ago

It would even have to be: faas-cli store deploy figlet --constraint "job ${attr.cpu.arch} = arm64" --gateway http://example:8080 (notice the job)

I added this part to distinguish this constraint from the datacenter constraint and maybe allow group and task constraints later on, if applicable.

I chose the above approach exactly because it was more based on how Nomad treats it's constraints. This way you can use it to set constraints on more than just the CPU architecture.

The documentation says:

Constraints are passed directly to the underlying container orchestrator.

The examples in the documentation are based on constraints for Docker Swarm. So I think it could be OK to go with a orchestrator based syntax.

The datacenter constraint is a bit special, since it does not get turned into a constraint, but a special property on the job. Maybe it could even be replaced by a ${node.datacenter} = dc1 constraint.

I see however that someone might have a different expectation from reading the documentation and/or experience with other orchestrators.

What would you think of the following changes?:

That way the constraint could be written as attr.cpu.arch == amd64.

Additionally I would adjust the readme for this repository and give an example for setting constraints other than data center and potentially communicate the limitations mentioned in my previous comment.

acornies commented 5 years ago

I like your suggestions. Yes please!

cryptobias commented 5 years ago

@acornies please have another look. I implemented the suggested changes and did a little refactoring for consistency.

acornies commented 5 years ago

One more thing @cryptobias can you rebase these 5 commits down to 1 using git reset --soft HEAD~5 and sign your commit with --signoff? I'll create a 0.4.3-rc1 release after that.

cryptobias commented 5 years ago

@acornies I combined commits into one and signed it.