CreateNamespace: do not hide error from lower call

[okartau]

This simple change fixes error hiding issue described in #115.
But note that it also changes semantics on failure:
now we return after 1st attempt failure, but original code would try creation in other remaining regions.
If we want to keep original semantics, we have to choose which error to report/return,
in case we try multiple times and there are multiple errors.
Perhaps should add log stmt showing error, in addition to returning error?

[avalluri]

As of i remember, it was intentional, to try with all available regions to serve the creation request. If upper layers are interested in error would call Region.CreateNamespace().

[pohly]

As @avalluri pointed out, this changes the semantic.

Otherwise a shorter way to write this if/else would be
return r.CreateNamespace(opts).

But even better is to wrap errors with errors.Wrap from https://github.com/pkg/errors because then additional information and a stack trace gets added at each level.

gRPC logging however currently doesn't show the stack trace, so that part isn't useful at the moment.

[okartau]

Thanks for Wrap idea, it really seems good solution for multiple/nested errors case. I force-pushed change to use Wrap, fixes the issue without changing semantics.
If there are multiple Regions involved, the output may get crowded, but the most important part is not to miss any information, I think.
There are few wrapping choices here, we can either:

• use empty addition in the loop and add one at the end (current code),
• wrap with added string in the loop, and not string add at the end (can use plain wrappederr var there then)
• add some string in both places (makes output even more messy so I think we should not).

The current code produces this message in case of one Region:

CreateNamespace returned error:Failed to create namespace: : pfn: failed to set namespace

extra colon mark(s) seems result of Wrap() with empty string, but this is just cosmetic problem

[okartau]

another minor thing, should we use import() block instead of multiple import statements

[okartau]

I tried this on a system with 6 Regions were same error happens, and contrary to my expectation (to see 6 times same message repeated) there is just single one:

GRPC error: rpc error: code = Internal desc = CreateVolume: failed to create volume: Failed to create namespace: : pfn: failed to set namespace

Does error.Wrap avoid repeating same msg? or is there something wrong with how Wrap is used here.

[okartau]

what about instead creating more complex error msg manipulation and analyzing there, we just log errors after we get them from lower/layer call, and we return single (new) error, as it was originally. Thats actually much simpler than wrap/analyze. The potential issue with that will be, list of per-Region errors is not returned via RPC but written in log instead. But it's still better than original state where error was not shown at all.

[pohly]

Olev Kartau <[email protected]> writes:

what about instead creating more complex error msg manipulation and analyzing there, we just log errors after we get them from lower/layer call, and we return single (new) error, as it was originally. Thats actually much simpler than wrap/analyze. The potential issue with that will be, list of per-Region errors is not returned via RPC but written in log instead. But it's still better than original state where error was not shown at all.

How easily can the error be discovered in the log? This probably needs to go into an "admin cookbook" section in the README.md.

[okartau]

k8s log info in general is not easy to discover IMHO, but if msg is logged, it can be retrieved, easy or less easy.
So my plan now is: 1. emit error-level log from failed attempts, stating region and error, 2. Wrap most recent error message and return it.

[okartau]

can we merge this now? At least I am happy with how error messages are shown

[okartau]

How easily can the error be discovered in the log? This probably needs to go into an "admin cookbook" section in the README.md.

we don't have "admin cookbook" section in README. Should we create one?

[pohly]

Olev Kartau <[email protected]> writes:

> How easily can the error be discovered in the log? This probably needs to go into an "admin cookbook" section in the README.md. we don't have "admin cookbook" section in README. Should we create one?

Yes.

[okartau]

I change the style towards "less PRs" so that I dont create separate PR for small changes. Thus moving this change into new #133 , closing this PR