Parse and Resolve the Namespace of Fission function #176

xiekeyang · 2018-07-25T07:31:20Z

This is for closing #160.

This supports to parse and resolve out the function namespace from workflow config file.

User can set specific namespace or left it by default.

This is meaningful to fission functions.

This parses namespace out of all kinds of runtime (fission, native...).The runtimes except fission will drop the namespace value(always default). So that it will not lead to invocation failure.

However, later we should improve it by refactoring framework of parser and resolver, especially auto-resolving.

I didn't add it to this patch, because I want to move the individual parser interface to nested sub to resolver interface. And we might need more discussion about auto-resolver.

@erwinvaneyk WDYT?

erwinvaneyk

Nice PR @xiekeyang 😁 Other than a couple of style-related changes, LGTM. In the meantime, I am going to clone this branch locally to trigger the CI on it

erwinvaneyk · 2018-07-25T08:51:51Z

pkg/fnenv/fission/resolver.go

@@ -15,18 +16,18 @@ func NewResolver(controller *client.Client) *Resolver {
 	return &Resolver{controller}
 }

-func (re *Resolver) Resolve(fnName string) (string, error) {
+func (re *Resolver) Resolve(ref *types.FnRef) (string, error) {


Pass ref by value; it is small enough, and we pass it by value elsewhere in the code.

Same for all other uses of FnRef in this PR ;)

erwinvaneyk · 2018-07-25T08:54:06Z

pkg/fnenv/resolver.go

@@ -182,10 +177,13 @@ func resolveTask(ps Resolver, id string, task *types.TaskSpec, resolvedC chan so
 	if task == nil || resolvedC == nil {
 		return nil
 	}
+
+	// t, err := types.ParseFnRef(task.FunctionRef)


Remove this comment :)

erwinvaneyk · 2018-07-25T08:55:04Z

pkg/types/types.proto

@@ -233,8 +233,12 @@ message FnRef {
    // Runtime is the Function Runtime environment (fnenv) that was used to resolve the function.
    string runtime = 2;

+
+    // Namespace is the namespace of the fission function.
+    string Namespace = 3;


make the namespace lowercase (go-proto makes all defined fields public)

erwinvaneyk · 2018-07-25T08:57:36Z

pkg/types/fnref.go

 )

 var (
-	fnRefReg        = regexp.MustCompile(fmt.Sprintf("^(?:(\\w+)%s)?(\\w+)$", RuntimeDelimiter))
+	fnRefReg        = regexp.MustCompile("^(?:(\\w+)://)?(?:([a-zA-Z0-9][a-zA-Z0-9_-]{1,128})/)?(\\w+)$")


Replace the :// with a reference to the RuntimeDelimiter constant

erwinvaneyk · 2018-07-25T09:05:58Z

pkg/types/fnref.go

+)
+
+const (
+	groupFnRef = iota


I know it wasn't there before either, but could you add a comment above this group enum to explain the purpose of them? maybe: // Capture groups for the FnRef regex

erwinvaneyk · 2018-07-25T09:16:33Z

pkg/fnenv/mock/mock.go

-func (mf *Resolver) Resolve(fnName string) (string, error) {
-	fnID, ok := mf.FnNameIDs[fnName]
+func (mf *Resolver) Resolve(ref *types.FnRef) (string, error) {
+	fnID, ok := mf.FnNameIDs[ref.ID]


When using FnRef as reference, the problem is that we have to be careful with NPEs, such as here. If you turn FnRef into a value, we don't run into this problem.

FYI, cool thing about protobuf generated structs is that they generate getters (GetID()), which have a nil check in them. So even when using pointers, it is safer to use the getters. When using values, this obviously does not matter and directly referencing the field is faster.

erwinvaneyk · 2018-07-25T12:04:59Z

pkg/fnenv/fission/runtime.go

@@ -156,5 +156,9 @@ func createFunctionMeta(fn types.FnRef) *metav1.ObjectMeta {
 }

 func (fe *FunctionEnv) createRouterURL(fn types.FnRef) string {
-	return fe.routerURL + "/fission-function/" + fn.ID
+	if fn.Namespace == metav1.NamespaceDefault {
+		return fe.routerURL + "/fission-function/" + fn.ID


Minor detail, just wondering, is this actually needed? Doesn't $FISSION_ROUTER/fission-function/$FN work the same as $FISSION_ROUTER/fission-function/default/$FN? If it is, we can leave out this if condition (to simplify the code). If it is not, it is a bug (or at least something that should be the case) in Fission and we should file an issue for it.

This check is mandatory. The correct use can refer to here

I doubt that this is a bug.

Okay, I raised an issue for it in the fission repo: fission/fission#819 (it is not really a bug, but it is inconsistent/unexpected behavior). We can keep it like this here :)

erwinvaneyk

LGTM 👍 You mentioned that you wanted to add an example, and you marked this PR as [WIP]. So just ping me if it is ready for a final CI run and merge.

xiekeyang · 2018-07-26T04:49:44Z

@erwinvaneyk I fix the problems as:

Fix the problem in your comments.
Fix the errors in integration tests and unit tests. All are passed through on my local PC.
Present the example in commit 4afcfc7.

PTAL, thanks so much!

erwinvaneyk · 2018-07-26T09:37:20Z

pkg/fnenv/fission/runtime.go

@@ -156,5 +156,9 @@ func createFunctionMeta(fn types.FnRef) *metav1.ObjectMeta {
 }

 func (fe *FunctionEnv) createRouterURL(fn types.FnRef) string {
-	return fe.routerURL + "/fission-function/" + fn.ID
+	if fn.Namespace == metav1.NamespaceDefault {
+		return fe.routerURL + "/fission-function/" + fn.ID


Okay, I raised an issue for it in the fission repo: fission/fission#819 (it is not really a bug, but it is inconsistent/unexpected behavior). We can keep it like this here :)

erwinvaneyk · 2018-07-26T10:16:53Z

@xiekeyang - LGTM 👍 just one test file needs to be fixed (caused by the just merged #177 )

xiekeyang · 2018-07-26T10:36:02Z

@erwinvaneyk Which test file? I have rebased #177 to this PR, and run all tests on my PC. All of them are passed. Maybe I miss some cases?

erwinvaneyk · 2018-07-26T10:58:54Z

It looks like you just missed one small thing:

https://github.com/xiekeyang/fission-workflows/blob/4afcfc7ec0db6b0c6ab9714b37c6e981110fcae0/pkg/fnenv/workflows/workflows_test.go#L134 errors because the function now requires a third argument (the namespace)

xiekeyang · 2018-07-26T11:23:18Z

@erwinvaneyk Thanks for your remind. It is really a missing.

I fix and re-push it: https://github.com/fission/fission-workflows/pull/176/files#diff-bab065f585cfa1d849f4f71fa601b621R134

This parse the function reference from raw string with member of namespace.

This resolve a function reference in tasks which has specific namespace. It is meaningful to fission functions.

xiekeyang · 2018-07-26T13:12:31Z

Rebased #179 , it seems no failed test.

erwinvaneyk · 2018-07-26T13:31:01Z

Verified in https://travis-ci.org/fission/fission-workflows/builds/408488542 - Nice job @xiekeyang! 🎉

erwinvaneyk requested changes Jul 25, 2018

View reviewed changes

xiekeyang changed the title ~~Parse and Resolve the Namespace of Fission function~~ [WIP] Parse and Resolve the Namespace of Fission function Jul 25, 2018

erwinvaneyk reviewed Jul 25, 2018

View reviewed changes

erwinvaneyk approved these changes Jul 25, 2018

View reviewed changes

xiekeyang changed the title ~~[WIP] Parse and Resolve the Namespace of Fission function~~ Parse and Resolve the Namespace of Fission function Jul 26, 2018

erwinvaneyk approved these changes Jul 26, 2018

View reviewed changes

xiekeyang added 5 commits July 26, 2018 20:57

add member namespace to struct FnRef

f9f4818

Parse Function Reference with Customized Namespace

f5f3b3f

This parse the function reference from raw string with member of namespace.

Resolve FnRef in tasks with specific namespace

2aeb9d8

This resolve a function reference in tasks which has specific namespace. It is meaningful to fission functions.

fix errors in integration test

ff86d56

add example access fission function under specific ns

954aba6

erwinvaneyk merged commit 373c76e into fission:master Jul 26, 2018

xiekeyang mentioned this pull request Jul 27, 2018

The namespace of workflow environment can not be configured #160

Closed

xiekeyang deleted the config-namespace-env branch August 6, 2018 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse and Resolve the Namespace of Fission function #176

Parse and Resolve the Namespace of Fission function #176

xiekeyang commented Jul 25, 2018

erwinvaneyk left a comment

erwinvaneyk Jul 25, 2018

erwinvaneyk Jul 25, 2018

erwinvaneyk Jul 25, 2018

erwinvaneyk Jul 25, 2018

erwinvaneyk Jul 25, 2018

erwinvaneyk Jul 25, 2018

erwinvaneyk Jul 25, 2018

erwinvaneyk Jul 25, 2018

xiekeyang Jul 26, 2018

erwinvaneyk Jul 26, 2018

erwinvaneyk left a comment

xiekeyang commented Jul 26, 2018

erwinvaneyk Jul 26, 2018

erwinvaneyk commented Jul 26, 2018

xiekeyang commented Jul 26, 2018

erwinvaneyk commented Jul 26, 2018 •

edited

Loading

xiekeyang commented Jul 26, 2018

xiekeyang commented Jul 26, 2018

erwinvaneyk commented Jul 26, 2018

Parse and Resolve the Namespace of Fission function #176

Parse and Resolve the Namespace of Fission function #176

Conversation

xiekeyang commented Jul 25, 2018

erwinvaneyk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinvaneyk left a comment

Choose a reason for hiding this comment

xiekeyang commented Jul 26, 2018

Choose a reason for hiding this comment

erwinvaneyk commented Jul 26, 2018

xiekeyang commented Jul 26, 2018

erwinvaneyk commented Jul 26, 2018 • edited Loading

xiekeyang commented Jul 26, 2018

xiekeyang commented Jul 26, 2018

erwinvaneyk commented Jul 26, 2018

erwinvaneyk commented Jul 26, 2018 •

edited

Loading