feat!: combine deploy and update commands #152

zroubalik · 2020-10-06T16:38:28Z

Signed-off-by: Zbynek Roubalik zroubali@redhat.com

faas deploy now builds and deploys a Function.

If the Function is already deployed, it is updated.

Fixes: #135

zroubalik · 2020-10-06T17:12:16Z

I am planning to unify (refactor and cleanup) the rest of the commands once this PR is merged.

matejvasek · 2020-10-06T17:39:05Z

knative/deployer.go

+			}
+			return err
+		}
+	} else {
+		// Update the existing Service
+		err = client.UpdateServiceWithRetry(encodedName, updateBuiltTimeStampEnvVar, 3)
+		if err != nil {
+			if !d.Verbose {
+				err = fmt.Errorf("deployer failed to update the service: %v.\nStdOut: %s", err, output.(*bytes.Buffer).String())
+			} else {
+				err = fmt.Errorf("deployer failed to update the service: %v", err)
+			}
+			return err
+		}
 	}
-	c.SetOut(output)
-	args := []string{
-		"service", "create", encodedName,
-		"--image", f.Image,
-		"--env", "VERBOSE=true",
-		"--label", "bosonFunction=true",
+
+	return nil
+}
+
+func generateNewService(name, image string) *servingv1.Service {
+	containers := []corev1.Container{
+		{
+			Image: image,
+			Env: []corev1.EnvVar{
+				{Name: "VERBOSE", Value: "true"},
+			},
+		},
 	}
-	if d.Namespace != "" {
-		args = append(args, "--namespace", d.Namespace)
+
+	return &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: name,
+			Labels: map[string]string{
+				"bosonFunction": "true",
+			},
+		},
+		Spec: v1.ServiceSpec{
+			ConfigurationSpec: v1.ConfigurationSpec{
+				Template: v1.RevisionTemplateSpec{
+					Spec: v1.RevisionSpec{
+						PodSpec: corev1.PodSpec{
+							Containers: containers,
+						},
+					},
+				},
+			},
+		},
 	}
-	c.SetArgs(args)
-	err = c.Execute()
-	if err != nil {
-		if !d.Verbose {
-			err = fmt.Errorf("failed to deploy the service: %v.\nStdOut: %s", err, output.(*bytes.Buffer).String())
-		} else {
-			err = fmt.Errorf("failed to deploy the service: %v", err)
+}
+
+func updateBuiltTimeStampEnvVar(service *servingv1.Service) (*servingv1.Service, error) {
+	envs := service.Spec.Template.Spec.Containers[0].Env
+
+	builtEnvVarName := "BUILT"
+
+	builtEnvVar := findEnvVar(builtEnvVarName, envs)
+	if builtEnvVar == nil {
+		envs = append(envs, corev1.EnvVar{Name: "VERBOSE", Value: "true"})
+		builtEnvVar = &envs[len(envs)-1]
+	}
+
+	builtEnvVar.Value = time.Now().Format("20060102T150405")
+
+	sort.SliceStable(envs, func(i, j int) bool {
+		return envs[i].Name <= envs[j].Name
+	})
+	service.Spec.Template.Spec.Containers[0].Env = envs
+
+	return service, nil
+}
+
+func findEnvVar(name string, envs []corev1.EnvVar) *corev1.EnvVar {
+	var result *corev1.EnvVar = nil
+	for i, envVar := range envs {
+		if envVar.Name == name {
+			result = &envs[i]
+			break
 		}
-		return
 	}
-	// TODO: use the KN service client noted above, such that we can return the
-	// final path/route of the final deployed Function.  While it can be assumed
-	// due to being deterministic, new users would be aided by having it echoed.
-	return
+	return result


@lance @rhuss this will let cluster to generate revision name so this: knative/serving#9544 won't affect us, right?

I think so? I'm starting to get confused about the different approaches. Is it correct to use the commands or the client as in this case?

My impression was, that we should use the client, but I might be wrong.

@rhuss wdyt?

I recommend using the client APIs. However not the commands but the client API which comes as an interface as here --> https://github.com/knative/client/blob/master/pkg/serving/v1/client.go (this is not the serving API but a facade to it)

Please don't use the commands. They can change anytime in incompatible ways, there are no precautions to prevent this.

Also we are planning to expose the client API (not the commands!), the way how to parse options, how to parse sink prefixes, ... and generally anything that can be useful for implementing an plugin into a separate repository (knative/client-pkg as working title), so that you don't even have a direct dependency to knative/client anymore (which then also prevents you technically to access the cobra kn commands)

I tested this and K_SINK is injected, unlike before this change 👍 .

lance

Looks great! A few minor comments. And one big one about client vs command in kn.

lance · 2020-10-06T21:59:11Z

cmd/deploy.go

 	deployCmd.Flags().StringP("namespace", "n", "", "Override namespace into which the Function is deployed (on supported platforms).  Default is to use currently active underlying platform setting - $FAAS_NAMESPACE")
 	deployCmd.Flags().StringP("path", "p", cwd(), "Path to the function project directory - $FAAS_PATH")
+	deployCmd.Flags().StringP("repository", "r", "", "Repository for built images, ex 'docker.io/myuser' or just 'myuser'.  - $FAAS_REPOSITORY")


We will need to remember to change this in #156 - assuming this PR lands first. If #156 lands first, this should change to "registry".

cmd/deploy.go

lance · 2020-10-06T22:04:23Z

cmd/deploy.go

 `,
 	SuggestFor: []string{"delpoy", "deplyo"},
-	PreRunE:    bindEnv("namespace", "path", "confirm"),
+	PreRunE:    bindEnv("image", "namespace", "path", "repository", "confirm"),


Just noting that this is affected by #156 as well.

docs/commands.md

lance · 2020-10-06T22:18:24Z

knative/deployer.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/client/pkg/wait"
+	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
+	v1 "knative.dev/serving/pkg/apis/serving/v1"


You and @dsimansk are taking different approaches with this file. I know that you are also dealing with a lot more logic, but I wonder which is right - using the client or using a command?

🤷‍♂️

We have briefly discussed with @rhuss that it might be beneficial to avoid direct usage of serving/eventing client and rather go with kn abstraction.
For this particular case it'd mean a lot less mangling with service object directly. And in addition we can expect that some level of test coverage is done on kn side already.

@dsimansk btw, in this PR we are not using serving/eventing client, but the one provided by knative/client: https://github.com/boson-project/faas/pull/152/files#diff-0cff654ab7f6b7619dd3ebd4a8855f70R11-R37
But I suppose that doesn't change anything from your suggestion, right?

I strongly recommend choosing either/or. However, if you use the kn API that is offered in front of eventing/serving you still have to import serving/eventing APIs like this, since the signature of the kn client API references the serving objects directly.

For consistencies sake and from an architectural POV though I would definitely recommend to use the client API if it offers all that you need. If not, we should fix this in the client.

@rhuss I am starting to be confused a little bit, sorry for that, but I am getting lost in the nomenclature, since the word client has multiple meanings in this context 😅

So your suggestion is to use go client provided byt knative/client? eg:

client, err := p.NewServingClient(namespace) ... client.CreateService(service *servingv1.Service)

or to use the kn abstraction as @dsimansk mentioned? eg:

c := service.NewServiceCreateCommand(&params) args := []string{ encodedName, "--image", f.Image, "--env", "VERBOSE=true", "--label", "bosonFunction=true", } c.SetArgs(args) err = c.Execute()

The former. No reference to any cobra commands, but just the services. There is also a better way to create an instance of the service that does not goes over the parameter as in the example above.

Mostly to avoid cyclic references, and keep a hierarchical dependency tree.

Thanks for the clarification, that's was my original understanding of this.

lance · 2020-10-06T22:20:03Z

knative/deployer.go

+			}
+			return err
+		}
+	} else {
+		// Update the existing Service
+		err = client.UpdateServiceWithRetry(encodedName, updateBuiltTimeStampEnvVar, 3)
+		if err != nil {
+			if !d.Verbose {
+				err = fmt.Errorf("deployer failed to update the service: %v.\nStdOut: %s", err, output.(*bytes.Buffer).String())
+			} else {
+				err = fmt.Errorf("deployer failed to update the service: %v", err)
+			}
+			return err
+		}
 	}
-	c.SetOut(output)
-	args := []string{
-		"service", "create", encodedName,
-		"--image", f.Image,
-		"--env", "VERBOSE=true",
-		"--label", "bosonFunction=true",
+
+	return nil
+}
+
+func generateNewService(name, image string) *servingv1.Service {
+	containers := []corev1.Container{
+		{
+			Image: image,
+			Env: []corev1.EnvVar{
+				{Name: "VERBOSE", Value: "true"},
+			},
+		},
 	}
-	if d.Namespace != "" {
-		args = append(args, "--namespace", d.Namespace)
+
+	return &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: name,
+			Labels: map[string]string{
+				"bosonFunction": "true",
+			},
+		},
+		Spec: v1.ServiceSpec{
+			ConfigurationSpec: v1.ConfigurationSpec{
+				Template: v1.RevisionTemplateSpec{
+					Spec: v1.RevisionSpec{
+						PodSpec: corev1.PodSpec{
+							Containers: containers,
+						},
+					},
+				},
+			},
+		},
 	}
-	c.SetArgs(args)
-	err = c.Execute()
-	if err != nil {
-		if !d.Verbose {
-			err = fmt.Errorf("failed to deploy the service: %v.\nStdOut: %s", err, output.(*bytes.Buffer).String())
-		} else {
-			err = fmt.Errorf("failed to deploy the service: %v", err)
+}
+
+func updateBuiltTimeStampEnvVar(service *servingv1.Service) (*servingv1.Service, error) {
+	envs := service.Spec.Template.Spec.Containers[0].Env
+
+	builtEnvVarName := "BUILT"
+
+	builtEnvVar := findEnvVar(builtEnvVarName, envs)
+	if builtEnvVar == nil {
+		envs = append(envs, corev1.EnvVar{Name: "VERBOSE", Value: "true"})
+		builtEnvVar = &envs[len(envs)-1]
+	}
+
+	builtEnvVar.Value = time.Now().Format("20060102T150405")
+
+	sort.SliceStable(envs, func(i, j int) bool {
+		return envs[i].Name <= envs[j].Name
+	})
+	service.Spec.Template.Spec.Containers[0].Env = envs
+
+	return service, nil
+}
+
+func findEnvVar(name string, envs []corev1.EnvVar) *corev1.EnvVar {
+	var result *corev1.EnvVar = nil
+	for i, envVar := range envs {
+		if envVar.Name == name {
+			result = &envs[i]
+			break
 		}
-		return
 	}
-	// TODO: use the KN service client noted above, such that we can return the
-	// final path/route of the final deployed Function.  While it can be assumed
-	// due to being deterministic, new users would be aided by having it echoed.
-	return
+	return result


I think so? I'm starting to get confused about the different approaches. Is it correct to use the commands or the client as in this case?

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>

Co-authored-by: Lance Ball <lball@redhat.com>

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>

rhuss · 2020-10-07T09:18:27Z

cmd/deploy.go

-Deploys the Function project in the current directory. A path to the project
-directory may be provided using the --path or -p flag. The image to be deployed
-must have already been created using the "build" command.
+Builds and Deploys the Function project in the current directory. 


From this description it looks like that faas build and faas deploy overlap, so that deploy is a superset of build. This is fine, but then it would be better to include the build only mode as an option to faas deploy, like in faas deploy --build-only. This would have several benefits:

Reducing duplication in options that are the same for deploy and build

Less commands, smaller UI interface

Drawbacks are:

Maybe not easy to find out which options of faas deploy are relevant for build and which for creating the service.

Build can be considered as a high-level concern, so a dedicated command might reflect this more prominently

I'm not sure what's the best way to go. From my experience with serverless.com integration, I can say that "Faas" platforms don't expose the "build" aspect so prominently as many FaaS platform don't have that stage anyway (think AWS Lambda). So it's just an aspect of the one single, most important operation: How to get my local code running in the cloud ? Aka "deploy". From that angle, I'd recommend to fold "build" as just another aspect of "deploy" and enable this with a dedicated option that influence the flow (its a bit similar to --dry-run option that is used by many commands)

On the other hand, if we really want to expose the build as a first level concept, which e.g. is also used without deploying (eg for local development with containers), then this separate commands make definitely sense.

zroubalik · 2020-10-07T15:15:24Z

@lance @matejvasek Is this ready to be merged? So we can move forward with the follow up PRs, that need to be rebased.

lance · 2020-10-07T15:19:43Z

@lance @matejvasek Is this ready to be merged? So we can move forward with the follow up PRs, that need to be rebased.

Based on our call today, yes I think so.

zroubalik added the source/commands label Oct 6, 2020

zroubalik requested review from lance, matejvasek and lkingland and removed request for lance October 6, 2020 16:42

matejvasek reviewed Oct 6, 2020

View reviewed changes

lance reviewed Oct 6, 2020

View reviewed changes

Zbynek Roubalik and others added 5 commits October 7, 2020 09:12

feat!: combine deploy and update commands

498d97a

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>

fix lint problems

5879e18

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>

Update cmd/deploy.go

32dc73a

Co-authored-by: Lance Ball <lball@redhat.com>

Apply suggestions from code review

1ac540d

Co-authored-by: Lance Ball <lball@redhat.com>

wrap to 80 columns

dc99a11

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>

zroubalik force-pushed the deployupdate branch from 7e2a57b to dc99a11 Compare October 7, 2020 07:16

functionWithOverrides() fix description

123c0d8

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>

rhuss reviewed Oct 7, 2020

View reviewed changes

matejvasek approved these changes Oct 7, 2020

View reviewed changes

zroubalik merged commit d5839ea into knative:main Oct 7, 2020

zroubalik mentioned this pull request Oct 7, 2020

feat(kn): Enable faas to be integrated as plugin to kn #155

Merged

This was referenced Oct 7, 2020

chore: release 1.0.0 #158

Closed

chore: release 0.8.0 #171

Merged

github-actions bot mentioned this pull request Jan 11, 2021

chore: release 1.0.0 lkingland/func#2

Closed

github-actions bot mentioned this pull request Mar 29, 2021

chore: release 1.0.0 lkingland/func#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: combine deploy and update commands #152

feat!: combine deploy and update commands #152

zroubalik commented Oct 6, 2020

zroubalik commented Oct 6, 2020 •

edited

Loading

matejvasek Oct 6, 2020

lance Oct 6, 2020

zroubalik Oct 7, 2020

rhuss Oct 7, 2020 •

edited

Loading

matejvasek Oct 7, 2020

lance left a comment

lance Oct 6, 2020

lance Oct 6, 2020

lance Oct 6, 2020

zroubalik Oct 7, 2020

dsimansk Oct 7, 2020

zroubalik Oct 7, 2020

rhuss Oct 7, 2020

zroubalik Oct 7, 2020 •

edited

Loading

rhuss Oct 7, 2020

rhuss Oct 7, 2020

zroubalik Oct 7, 2020

lance Oct 6, 2020

rhuss Oct 7, 2020

zroubalik commented Oct 7, 2020

lance commented Oct 7, 2020

feat!: combine deploy and update commands #152

feat!: combine deploy and update commands #152

Conversation

zroubalik commented Oct 6, 2020

zroubalik commented Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance left a comment

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

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

zroubalik Oct 7, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zroubalik commented Oct 7, 2020

lance commented Oct 7, 2020

zroubalik commented Oct 6, 2020 •

edited

Loading

rhuss Oct 7, 2020 •

edited

Loading

zroubalik Oct 7, 2020 •

edited

Loading