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

Improve wasm errors #3830

Merged
merged 6 commits into from
Feb 23, 2023
Merged

Improve wasm errors #3830

merged 6 commits into from
Feb 23, 2023

Conversation

wleese
Copy link
Contributor

@wleese wleese commented Feb 20, 2023

Currently WASM compiled functions panic when attempting to return an error.
Also, the error returned is often not meaningful without retrieving the resourceList.Results and showing them to the user.

This change builds on this PR GoogleContainerTools/kpt-functions-catalog#978 to benefit from the additional function, introduced to allow retrieving and showing resourceList.Results to the user.
Both that and this PR should solve #3822.

Shortcomings:

  1. No effort is made to match resourceList.Results entries to the entry introduced by the last function. In the PR I assume that entries with Severity "error" result in the function failing, and that all "error" entries are relevant
  2. ...

Previously, output would be like this:

➜ KPT_FN_WASM_RUNTIME=nodejs kpt fn render . --allow-alpha-wasm --allow-exec
Package "kpt2":
[RUNNING] WASM "starlark-wasm"
[FAIL] "starlark-wasm" in 800ms
Error: wrong Node Kind for  expected: MappingNode was ScalarNode: value: {undefined}


➜ kpt fn render . --allow-alpha-wasm --allow-exec
Package "kpt2":
[RUNNING] WASM "starlark-wasm"
panic: ValueOf: invalid value

goroutine 6 [running]:
syscall/js.ValueOf({0xf46c0, 0x1653560})
	/opt/local/lib/go/src/syscall/js/js.go:208 +0xf7
syscall/js.Value.Set({{}, 0x7ff8000100000012, 0x15b0690}, {0x2017f8, 0x6}, {0xf46c0, 0x1653560})
	/opt/local/lib/go/src/syscall/js/js.go:303 +0x8
syscall/js.handleEvent()
	/opt/local/lib/go/src/syscall/js/func.go:95 +0x27
[FAIL] "starlark-wasm" in 0s
Error: invalid resource list output from wasm library; got "s(<nil>"

With these changes, including the PR for the functions:

➜  KPT_FN_WASM_RUNTIME=nodejs kpt fn render . --allow-alpha-wasm --allow-exec
Package "kpt2":
[RUNNING] WASM "starlark-wasm"
[FAIL] "starlark-wasm" in 500ms
Error: wrong Node Kind for  expected: MappingNode was ScalarNode: value: {key "params" not in dict}

➜  kpt fn render . --allow-alpha-wasm --allow-exec
Package "kpt2":
[RUNNING] WASM "starlark-wasm"
[FAIL] "starlark-wasm" in 0s
Error: parsing output resource list with content: "unable to process resource list: error: function failure"
yaml: mapping values are not allowed in this context
&{key "params" not in dict}

The key message here is key "params" not in dict, as in my example I had a starlark function that tries to access params when it is not available.

@wleese wleese requested a review from a team as a code owner February 20, 2023 13:07
@wleese wleese requested a review from justinsb February 20, 2023 13:07
@@ -154,17 +154,33 @@ func (f *WasmtimeFn) Run(r io.Reader, w io.Writer) error {
// Try to parse the output as yaml.
resourceListOutput, err := yaml.Parse(resultStr)
if err != nil {
return fmt.Errorf("error parsing output resource list %q: %w", resultStr, err)
additionalErrorMessage, err2 := retrieveError(f, resourceList)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use a more descriptive name here than err2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave it another shot.

internal/fnruntime/wasmtime.go Outdated Show resolved Hide resolved
Copy link
Contributor

@natasha41575 natasha41575 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for working on this and for providing more detailed context and investigation in the issue as well. This is a great change. I have some small nits below.

One last thing, could you update the PR description to add what the error output looked like before, and what this PR changes it to? (So that someone without context can immediately understand the behavior change.)

internal/fnruntime/wasmtime.go Outdated Show resolved Hide resolved
internal/fnruntime/wasmtime.go Outdated Show resolved Hide resolved
internal/fnruntime/wasmtime.go Outdated Show resolved Hide resolved
Copy link
Contributor

@natasha41575 natasha41575 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Will defer to @mortent to make sure his comments are addressed

Copy link
Contributor

@mortent mortent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for contributing!

@mortent mortent merged commit 2c50be2 into kptdev:main Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants