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

Allow to change CPU arch constraint via commandline argument #82

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@cryptobias
Copy link

commented Jun 6, 2019

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 #18 is implemented.

@hashicorp-cla

This comment has been minimized.

Copy link

commented Jun 6, 2019

CLA assistant check
All committers have signed the CLA.

@acornies
Copy link
Collaborator

left a comment

This configurable default could work in conjunction with overriding it on a function level, implementing #18 . Could you take it one step further and allow this to be overridden in the OpenFaaS function request CreateFunctionRequest.Constraints?

@cryptobias cryptobias force-pushed the cryptobias:allow-to-change-cpu-arch-constraint branch from 6f6392a to ab41155 Jun 9, 2019

@cryptobias

This comment has been minimized.

Copy link
Author

commented Jun 9, 2019

@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:

  • leaving out the attribute (like you would with distinct_hosts)
  • leaving out the value (e.g. with is_set)
  • using multiple operators (like is_set !=)
  • using consecutive white-spaces in the value (they currently will get replaced by just a space)

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.

@acornies
Copy link
Collaborator

left a comment

Thanks for the updated PR! I have one more suggestion that would make it less nuanced to nomad var interpolation: With your change, the function deploy would look something like:

faas-cli store deploy figlet --constraint "${attr.cpu.arch} = arm64"  --gateway http://example:8080

I'm worried that using ${attr.cpu.arch} is too specific. How about simply:

faas-cli store deploy figlet --constraint "arch == arm64"  --gateway http://example:8080

which maps to ${attr.cpu.arch} etc.
This would be more consistent with how the datacenter constraints work now and the OpenFaaS docs on constraints in yaml and cli.

@cryptobias

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

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?:

  • get rid of the job prefix
  • ignore strings containing datacenter in the first field when creating the constraints
  • allow to leave out the ${ and } in the attribute (and automatically add them, if they is missing)
  • allow to use == instead of = for compatibility

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

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

I like your suggestions. Yes please!

@cryptobias cryptobias force-pushed the cryptobias:allow-to-change-cpu-arch-constraint branch 2 times, most recently from c044581 to 1188997 Jun 13, 2019

@cryptobias

This comment has been minimized.

Copy link
Author

commented Jun 13, 2019

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

assert.Equal(t, expectedCpuArchConstraint, *constraints[0])
}

func TestHandlesConstraintsWithoutTwoEqualSigns(t *testing.T) {

This comment has been minimized.

Copy link
@acornies

acornies Jun 13, 2019

Collaborator

WithTwoEqualSigns?

This comment has been minimized.

Copy link
@cryptobias

cryptobias Jun 13, 2019

Author

Yes, of course. Fixed it.

@cryptobias cryptobias force-pushed the cryptobias:allow-to-change-cpu-arch-constraint branch from 1188997 to 7d9db5f Jun 13, 2019

@acornies

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

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.

Allows configuration of CPU arch via argument and job constraint
additionally allows configuration of other job constraints

Squashed commit of the following:

commit 7d9db5f
Author: cryptobias <cryptobias@protonmail.com>
Date:   Thu Jun 13 10:50:41 2019 +0200

    Refactor CreateDataCenters to use Fields

    so it is consistent with CreateConstraints

    There is a small change of behaviour:
    This does not panic if there is no == or no value following it, but
    ignores the entry instead.

commit c6941af
Author: cryptobias <cryptobias@protonmail.com>
Date:   Thu Jun 13 10:44:32 2019 +0200

    Use limit instead of constraint where applicable

commit 714f269
Author: cryptobias <cryptobias@protonmail.com>
Date:   Thu Jun 13 10:31:59 2019 +0200

    Document Nomad job constraints in README.md

commit ddbab0c
Author: cryptobias <cryptobias@protonmail.com>
Date:   Sun Jun 9 16:32:11 2019 +0200

    Allows to specify custom job constraints via the request

    The constraints have to be provided in the format:

    "<attribute> <operator> <value>"

    e.g.

    "${attr.cpu.arch} = arm"

    For compatibility the interpolation notation (`${}`) can be left out and `==` instead of `=` is supported.

    With this implementation the attribute and the operator can not contain
    spaces.

commit e440ccf
Author: cryptobias <cryptobias@protonmail.com>
Date:   Thu Jun 6 10:43:45 2019 +0200

    Allow to change CPU arch constraint via commandline argument

Signed-off-by: cryptobias <cryptobias@protonmail.com>

@cryptobias cryptobias force-pushed the cryptobias:allow-to-change-cpu-arch-constraint branch from 7d9db5f to 69e9b21 Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.