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 error handling #61

Merged
merged 2 commits into from
Nov 30, 2023
Merged

Improve error handling #61

merged 2 commits into from
Nov 30, 2023

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented Nov 30, 2023

This PR fixes some errors in error handling. I found it when testing other changes and in fact we have 3 fixes here:

  1. For retrying, disposables where not disposed on errors others than activated check and next initialization would failed due to trying to register same commands (as they need to be disposed). I moved disposable logic to onFailedAttempt which is run every after failed attempt.
  2. Also pRetry does not retry after TypeErrors so I rethrow any activation errors as generic ones (and why it is important see next point).
  3. The initial issue was that during extension initialization (when validation is run) there was TypeError: Cannot read properties of undefined (reading 'addSchema'). When retried, it worked fine (that's way previous point). But more interestingly, this issue is a bit puzzling, it was caused by the fact that when validation was run, it tried to load prometheus schema (K8s schema plugin) via ajv but ajv instance was not initialized yet (thus Cannot read properties of undefined (reading 'addSchema') for this.ajv.addSchema... erro) from here https://github.com/kubeshop/monokle-core/blob/f1c616d6dec28f74d9ca6005be1a8fdade4fc2b3/packages/validation/src/validators/kubernetes-schema/validator.ts#L118). My assumption was that it should load automatically on first validation run (should it?), if yes it seems there might be an issue in core somewhere (will report a task there then) 🤔

Changes

  • None.

Fixes

  • As above.

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@f1ames f1ames requested a review from WitoDelnat November 30, 2023 14:43
await runActivation(context);
} catch (err: any) {
// Rethrow any errors as generic ones so pRetry reacts to it, because it ignores some types of errors as mentioned in the docs:
// > It does not retry on most TypeError's, with the exception of network errors.

Choose a reason for hiding this comment

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

Does it make sense to retry on TypeErrors? Will these not simply happen again in the next attempt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I would say that in most cases yes. However, in the case I encountered it won't because the object ajv was loaded async and next time it was there (at least that's my understanding). That's why I thought it might be worth to have it, but looks a bit edge casey 🤔

Ofc for this particular case we there is dedicated fix so not sure 🤔 What's your take on this?

Choose a reason for hiding this comment

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

It's a bit a smell that we are masking. That said, let's merge this for now if it helps with AJV. There is little harm in doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, let's revisit this with #56.

@f1ames f1ames merged commit f20e8ec into main Nov 30, 2023
3 checks passed
@f1ames f1ames deleted the f1ames/fix/better-error-handling branch November 30, 2023 15:41
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.

2 participants