[WIP] Use rolling update instead of delete/create when function exists #257

ericstoekl · 2017-12-18T07:06:00Z

Description

This change is to perform in-place update of functions rather than delete/create when the function already exists.

If the function does not already exist and we get 404, create the function.

Motivation and Context

I have raised an issue to propose this change (required)
Backlog: prefer rolling update over delete/create #256

How Has This Been Tested?

Testing is improved in proxy/deploy_test.go by adding scaffolding with array-of-struct test cases.

We need to improve testing in this area by adding more test cases (hence why this is WIP).

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

burtonr · 2017-12-18T14:21:40Z

proxy/deploy.go

@@ -79,14 +95,14 @@ func DeployFunction(fprocess string, gateway string, functionName string, image
 	SetAuth(request, gateway)
 	if err != nil {
 		fmt.Println(err)
-		return
+		return -1


Not sure if this matters, but the usage shows that you're returning a status code (in the function call, not the signature), so should this return 500 (http.StatusInternalServerError) instead?

Same comment for line 105 with the other return -1

Purely a nit-pick thing, just thinking about consistency for down the road

Yes what does -1 mean?

alexellis · 2017-12-18T15:33:13Z

proxy/deploy.go

+	constraints []string, update bool, secrets []string, labels map[string]string,
+	functionResourceRequest1 FunctionResourceRequest) {
+
+	statusCode := DeployFunctionImpl(fprocess, gateway, functionName, image, language, replace, envVars, network, constraints, update, secrets, labels, functionResourceRequest1)


Not keen on names like "Impl".. we can just call this Deploy? I saw we broke out a function for this - could we make the change one level higher? Does that even make sense?

By make the change one level higher -- do you mean having the retry logic (retry if 404 error occurs) in commands/deploy.go, in the runDeploy() function?

The reason I'm using the DeployFunction() command to just call DeployFunctionImpl() (renamed to Deploy()) is because in runDeploy(), DeployFunction() is called multiple times in a for loop if a stack.yml file is passed in, otherwise it is called only once with command line parameters. I think it may be better for code reuse/minimizing redundancy if we go with the current structure.

alexellis · 2017-12-27T10:13:04Z

How Has This Been Tested?

@ericstoekl please tell us what end-to-end testing has been done on this.

@burtonr have you played with it yet?

alexellis · 2017-12-27T10:15:19Z

proxy/deploy.go

+
+	statusCode := Deploy(fprocess, gateway, functionName, image, language, replace, envVars, network, constraints, update, secrets, labels, functionResourceRequest1)
+
+	if update == true && statusCode == http.StatusNotFound {


Just a nit here - can we avoid setting update = false and just pass false through the message signature? It will make debugging easier. Or you could create a new local variable as an alternative: updateFunction := false

Just to avoid destructively updating the value

alexellis · 2017-12-27T10:15:44Z

proxy/deploy.go

+	}
+}
+
+// Call FaaS server to deploy a new function


While here let's remove the comment here.

alexellis · 2017-12-27T10:16:05Z

proxy/deploy_test.go

 			[]string{},
 			map[string]string{},
 			FunctionResourceRequest{},
 		)
 	})

-	r := regexp.MustCompile(`(?m:Unexpected status: 404)`)
+	r := regexp.MustCompile(dpt.expectedOutput)


Can we change the name of dpt to something else?

burtonr · 2017-12-27T13:59:36Z

I've tested this by running the tests (all pass) as well as running through the commands as a normal user.

faas-cli deploy -f tester.yml deploys a new function
faas-cli deploy -f tester.yml again, updates the function
faas-cli deploy -f tester.yml --update=false --replace=true replaces the function
All working as expected 👍

However, the faas-cli deploy --help shows incorrect usage. This is not new for this PR, but something that's been lurking for a bit, I just forgot about it as I rarely use that any more.

In commands/deploy.go, lines 85-86, the help text for the deploy function is missing the second flag
Both flags are required as the CLI uses default values and must be explicitly overridden when passing the --replace=true flag

  faas-cli deploy -f ./samples.yml --replace=false
  faas-cli deploy -f ./samples.yml --update=true

should be:

  faas-cli deploy -f ./samples.yml --replace=false --update=true
  faas-cli deploy -f ./samples.yml --replace=true --update=false

or perhaps, remove the flags from the update=true as that's default behavior now.

alexellis · 2017-12-29T10:54:50Z

@ericstoekl can you address the comment from @burtonr please?

ericstoekl · 2017-12-29T16:26:33Z

@alexellis just noticed there's been activity here, I'll have an update for this later today.

Signed-off-by: Eric Stoekl <ems5311@gmail.com>

ericstoekl · 2017-12-30T01:17:23Z

Just updated this, thanks to @burtonr for helping with testing and suggestions. The latest commit addresses the comments.

@alexellis, I have tested this by running the tests as well as manually going through the scenario with a docker swarm deployment.

One thought I have is combining the --update and --replace flags into a single flag. What are your thoughts on this (Burton and/or Alex?)

burtonr · 2017-12-30T01:46:29Z

I agree, I think one flag would be more intuitive since we default one of them to true always and there's no way to check (that I'm aware of) if the flag was actually passed in.

So, just having the --replace flag would make a lot of sense. It updates automatically, but if the --replace flag is present, remove and deploy. Bonus points for also including a -r option :)

Also, I think that should go into a different PR since this one is focused on setting the default behavior.

alexellis · 2017-12-30T21:48:59Z

proxy/deploy_test.go

-	if !r.MatchString(stdout) {
-		t.Fatalf("Output not matched: %s", stdout)
-	}
+var DeployProxyTests = []DeployProxyTest{


The capital letter means this is exported outside the package. Was that the intention?

Was not the intention, I'll fix this.

alexellis · 2017-12-30T21:50:09Z

proxy/deploy_test.go

 	if !r.MatchString(stdout) {
 		t.Fatalf("Output not matched: %s", stdout)
 	}
 }

+func Test_RunDeployProxyTests(t *testing.T) {
+	for _, tst := range DeployProxyTests {
+		t.Run(tst.title, func(t *testing.T) {


Just wanted to ask why you did it this way instead of our usual coding style of test-cases and an in-line struct?

I could change it so the test cases are defined in the Test_RunDeployProxyTests() method, but I like having runDeployProxyTest() as a separate function because it makes it easier to read

Signed-off-by: Eric Stoekl <ems5311@gmail.com>

alexellis · 2018-01-01T19:07:29Z

I think users will find the 404 message confusing. Please could you open a new PR so that we get this output?

When it exists:

Function <name> already exists, attempting rolling-update.

Deployed.
URL: http://192.168.0.66:8080/function/cat

200 OK

When it doesn't:

Deployed.
URL: http://192.168.0.66:8080/function/cat

200 OK

burtonr reviewed Dec 18, 2017

View reviewed changes

alexellis reviewed Dec 18, 2017

View reviewed changes

alexellis reviewed Dec 27, 2017

View reviewed changes

ericstoekl added 3 commits December 29, 2017 17:15

Refactor tests in proxy/deploy_test.go

fa4e4a4

Signed-off-by: Eric Stoekl <ems5311@gmail.com>

Rename function, return 500 ISE on failure

f87ebe7

Signed-off-by: Eric Stoekl <ems5311@gmail.com>

Fix help text and minor updates

678ca92

Signed-off-by: Eric Stoekl <ems5311@gmail.com>

ericstoekl force-pushed the rollingUpdate branch from 6785c6c to 678ca92 Compare December 30, 2017 01:16

alexellis reviewed Dec 30, 2017

View reviewed changes

Refactor proxy/deploy_test.go test cases

490a4b0

Signed-off-by: Eric Stoekl <ems5311@gmail.com>

alexellis merged commit 9a9d784 into openfaas:master Jan 1, 2018

ericstoekl mentioned this pull request Jan 1, 2018

Improve output for rolling updates #272

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use rolling update instead of delete/create when function exists #257

[WIP] Use rolling update instead of delete/create when function exists #257

ericstoekl commented Dec 18, 2017

burtonr Dec 18, 2017

alexellis Dec 18, 2017

alexellis Dec 18, 2017

ericstoekl Dec 18, 2017 •

edited

Loading

alexellis commented Dec 27, 2017

alexellis Dec 27, 2017

alexellis Dec 27, 2017

alexellis Dec 27, 2017

burtonr commented Dec 27, 2017

alexellis commented Dec 29, 2017

ericstoekl commented Dec 29, 2017

ericstoekl commented Dec 30, 2017

burtonr commented Dec 30, 2017

alexellis Dec 30, 2017

ericstoekl Dec 30, 2017

alexellis Dec 30, 2017

ericstoekl Dec 30, 2017

alexellis commented Jan 1, 2018 •

edited

Loading


		statusCode := Deploy(fprocess, gateway, functionName, image, language, replace, envVars, network, constraints, update, secrets, labels, functionResourceRequest1)

		if update == true && statusCode == http.StatusNotFound {

[WIP] Use rolling update instead of delete/create when function exists #257

[WIP] Use rolling update instead of delete/create when function exists #257

Conversation

ericstoekl commented Dec 18, 2017

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstoekl Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

alexellis commented Dec 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burtonr commented Dec 27, 2017

alexellis commented Dec 29, 2017

ericstoekl commented Dec 29, 2017

ericstoekl commented Dec 30, 2017

burtonr commented Dec 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Jan 1, 2018 • edited Loading

ericstoekl Dec 18, 2017 •

edited

Loading

alexellis commented Jan 1, 2018 •

edited

Loading