Question: Why are functions updated when they are deployed?

bmcustodio commented 5 years ago

Expected Behaviour

Running faas deploy multiple times without changing anything doesn't cause a function (and hence its pods) to be re-created.

Current Behaviour

Running faas deploy multiple times without changing anything causes a function (and hence its pods) to be re-created. This is (in part?) due to the value of the UID label being changed everytime, as well as to the fact that environment variables are not sorted:

https://github.com/openfaas/faas-netes/blob/master/handlers/update.go#L80 https://github.com/openfaas/faas-netes/blob/master/handlers/update.go#L71

Possible Solution

I am not sure what the purpose of the UID label is, but maybe it could be removed. If that is not a possibility, then maybe we can provide an option that allows for overriding that value (and hence to keep it constant)? If this is acceptable as a solution, I believe I'll be able to work on a PR.

Steps to Reproduce (for bugs)

Create a function and deploy it using faas deploy.
Run faas deploy again without changing anything.
Observe the pods being re-created.

Context

My current thinking is that running faas deploy should be as idempotent as possible—meaning it is "safe" to run the command multiple times using the same parameters. This would be useful on CI/CD, for instance.

Your Environment

FaaS-CLI version ( Full output from: faas-cli version ):

(...)
CLI:
 commit:  750add9e2a6d1d1f1bfb788a6a85341b4b4140e8
 version: 0.8.22

Docker version docker version (e.g. Docker 17.0.05 ):

Client: Docker Engine - Community
 Version:           19.03.1
 API version:       1.40
 Go version:        go1.12.5
 Git commit:        74b1e89
 Built:             Thu Jul 25 21:18:17 2019
 OS/Arch:           darwin/amd64
 Experimental:      false

Server: Docker Engine - Community
 Engine:
  Version:          19.03.1
  API version:      1.40 (minimum version 1.12)
  Go version:       go1.12.5
  Git commit:       74b1e89
  Built:            Thu Jul 25 21:17:52 2019
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          v1.2.6
  GitCommit:        894b81a4b802e4eb2a91d1ce216b8817763c29fb
 runc:
  Version:          1.0.0-rc8
  GitCommit:        425e105d5a03fabd737a126ad93d62a9eeede87f
 docker-init:
  Version:          0.18.0
  GitCommit:        fec3683

What version and distriubtion of Kubernetes are you using? kubectl version

Client Version: version.Info{Major:"1", Minor:"13", GitVersion:"v1.13.7", GitCommit:"4683545293d792934a7a7e12f2cc47d20b2dd01b", GitTreeState:"clean", BuildDate:"2019-06-06T01:46:52Z", GoVersion:"go1.11.5", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"13+", GitVersion:"v1.13.7-eks-c57ff8", GitCommit:"c57ff8e35590932c652433fab07988da79265d5b", GitTreeState:"clean", BuildDate:"2019-06-07T20:43:03Z", GoVersion:"go1.11.5", Compiler:"gc", Platform:"linux/amd64"}

Operating System and version (e.g. Linux, Windows, MacOS):

MacOS 10.14.5.

Link to your project or a code example to reproduce issue:

N/A.

What network driver are you using and what CIDR? i.e. Weave net / Flannel

I am using AWS EKS.

alexellis commented 5 years ago

Hi Bruno,

This is working as designed.

Many users deploy the same Docker image over and over without changing its tag, and for that reason we cause an annotation to change "UID" in this case, in order for the function to pull in the newer version.

Why are you calling the deploy API multiple times if you're not changing the code?

Cheers,

Alex

alexellis commented 5 years ago

I chatted to the members team about this and suggested that you may benefit from using the Operator and the CRD, if you have some process that is deploying the same config over and over again. The other thing you may want to do is to use an annotation or a state file to decide whether to deploy again.

This behaviour is by design, but there are at least two ways above that you can get what you need. We'd still like to know more about your use-case though as this hasn't been requested before.

Alex

bmcustodio commented 5 years ago

Hi @alexellis, thanks for looking into this! Yeah, I didn't mean to imply this is a bug—I was just saying that it feels a bit strange to me, as I was expecting faas deploy to be idempotent.

I understand what my other options are, and thanks for pointing them out, but I'd really like to get away without the complexity of those if at all possible.

The main use-case is to have Terraform deploying a function (using a null_resource and local-exec, maybe) and be able to run the apply plan for that any number of times without causing a redeployment unless there's really the need for that (e.g. change to the function's version or environment).

alexellis commented 5 years ago

I discussed this with @LucasRoesler @stefanprodan @matipan and @martindekov - the feedback was given to you above.

From my perspective, I would tend to agree with the team, but I also want faas-netes to suit users of Terraform.

Personally I think I would accept a PR for an environment variable called UPDATE_UID which you could set to 0 via the helm chart for the faas-netes container. When the value would be set to 0, it would not update the UID label upon redeployment and so your Pods would not be re-created. That would mean you could call the update endpoint multiple times without seeing Pods being re-created.

@LucasRoesler / @stefanprodan - would you be OK with us adding this feature for Form3?

Alex

LucasRoesler commented 5 years ago

This behavior can currently be overridden by setting the uid label on a function to a static value. It will override the generated value set by faas-netes. Once the value is statically set, it should stop k8s from seeing the function changing on every deploy.

If we don't want to make any code changes, we could document this behavior (and close #229 as won't fix).

If we make a change, I think I would prefer to avoid another installation configuration option like this. Especially because I could see wanting to change this behavior on the fly. It seems slightly cleaner to remove the UID generation from faas-netes and let the client control that label. This provides maximum flexibility to the client and integrators. We can maintain this behavior for existing users by generating the UID label in the CLI.

alexellis commented 5 years ago

Thanks for your input Lucas.

@bmcstdio please can you try setting a static UID label as suggested by @LucasRoesler ? It sounds like a good balance between changing the code and achieving your goals. Perhaps you can set it to a value by convention like the endpoint name?

Alex

LucasRoesler commented 5 years ago

@alexellis i tested this in the KinD test env using

faas-cli deploy  --gateway localhost:31112 --fprocess=cat -e write_debug=true --label "uid=empty" --image=functions/alpine --name echo-me

The resulting function pod had these labels

  labels:
    faas_function: echo-me
    pod-template-hash: 94d59cdbb
    uid: empty

And subsequent calls to deploy with the same values was accepted by the API but did not result in a new pod in k8s.

I also re-deployed while updating that uid value and saw it do the expected rolling deploy and apply the new uid label.

TL;DR: the work around works as expected.

LucasRoesler commented 5 years ago

@bmcstdio Once you verify this behavior with the static uid, do you think this will work for your use case and we can close it?

bmcustodio commented 5 years ago

@alexellis @LucasRoesler thank you for looking into this and for your feedback. However, and as I've mentioned, the UID label is not the only thing causing pods to be re-created: the fact that the environment variables are not sorted—and hence that the actual value of .spec.template.spec.containers[0].env changes between invocations of faas deploy—is enough to trigger the re-creation of pods (in case there's more than one environment variable, of course). Hence, and in order to work around this behaviour, we should make sure that they are sorted in some well-known order (alphabetically?). We should also check what other fields of the PodSpec can change in unpredictable ways between invocations and try to make those changes predictable as well. I think I could work on a pull request for this, provided we agree on that being a good thing to do.

LucasRoesler commented 5 years ago

@alexellis and @bmcstdio i just tested this with multiple envvars using

faas-cli deploy  --gateway localhost:31112 --fprocess=cat -e write_debug=true -e a_env=yes -e b_env=no --label "uid=empty" --image=functions/alpine --name echo-me

As @bmcstdio said, after multiple deploys, you can see the deployment be re-created because the env variable order will change.

I think env vars are the only remaining issue for this, because it goes from a map in the Function request, to an array in the Deploymentspec. The map is unordered. The other values from the function request that are also maps (Labels and Annotations) are also maps in the Deployment spec, so I do not expect those to cause problems, since k8s will already handle the fact that they are unordered.

Sorting the env vars alphabetically seems reasonable to me.

alexellis commented 5 years ago

@LucasRoesler given your comment "Sorting the env vars alphabetically seems reasonable to me." and the hypothesis that this single change is enough to resolve @bmcstdio's issue, I will go ahead with a fix and submit a PR.

Alex

bmcustodio commented 5 years ago

@alexellis I think this can be closed now.

alexellis commented 5 years ago

/close

alexellis commented 5 years ago

Thank you Bruno. Let us know if you need anything.

openfaas / faas-netes