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

Surface Workload and Jinja Template Errors to Console and Provide Common Solutions #448

Merged

Conversation

IanHoang
Copy link
Collaborator

Description

Users have often made changes to OSB workloads and come across vague and unclear errors. Instead of showing them what the error is or providing them common issues and suggestions on how to fix them, the console wouuld show the following:

The complete workload has been written to '/var/folders/yh/89c2pcg10szgzwc6qj2h04gst115h_/T/tmptoxqvjsr.json' for diagnosis

These files are usually empty and not useful.

This PR adds better error handling when OSB attempts to render workload files and Jinja templates. Users do not have to dig into the OSB logs anymore for workload and jinja template errors or guess as to what went wrong. These changes now surface the error to the console and highlight common issues and solutions.

Issues Resolved

#447

Testing

  • New functionality includes testing

Recreated common errors in workload.json, operations/default.json, and test_procedures/default.json. Ran the tests and encountered more descriptive error handling that either provides the place to fix or recommendations of common issues and solutions.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@IanHoang IanHoang requested a review from gkamat as a code owner January 26, 2024 15:35
@IanHoang IanHoang changed the title Error handling improvements Surface Workload and Jinja Template Errors to Console and Provide Common Solutions Jan 26, 2024
@IanHoang
Copy link
Collaborator Author

Put this together quickly but am open to refactoring this to make it cleaner. Would prefer if we store the error handling messages elsewhere instead of within the except excerpts; However, I decided to place them here since I couldn't think of anywhere better and also since WorkloadSyntaxError is stored in the same file. @gkamat or @rishabh6788 any suggestions are appreciated!

@IanHoang IanHoang requested a review from rishabh6788 January 26, 2024 15:41
Ian Hoang added 2 commits January 26, 2024 09:48
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
Copy link
Collaborator

@gkamat gkamat left a comment

Choose a reason for hiding this comment

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

This is a really helpful PR, that many users will appreciate!

except jinja2.exceptions.TemplateNotFound:
self.logger.exception("Could not load [%s]", workload_spec_file)
raise exceptions.SystemSetupError("Workload {} does not exist".format(workload_name))

except jinja2.exceptions.TemplateSyntaxError as e:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a really good improvement. The implementation is fine for now, but moving the processing to a separate function with a map of error string patterns to suggestions would be helpful. This could be in a separate module and can be updated if the Jinja2 parser changes.

Comment on lines +1034 to +1039
msg = "Could not load '{}'. The complete workload has been written to '{}' for diagnosis. \n\n".format(
workload_spec_file, tmp.name)
console_message = f"Suggestion: Verify that [{workload_name}] workload has correctly formatted JSON files and " + \
"Jinja Templates. For Jinja2 errors, consider using a live Jinja2 parser. " + \
f"See common workload formatting errors:{WorkloadFileReader.COMMON_WORKLOAD_FORMAT_ERRORS}"
msg += console_message
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be helpful to consolidate the repetition into a function, so that possible future changes don't need to made in two places.

@IanHoang IanHoang merged commit 1f5092e into opensearch-project:main Feb 2, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

2 participants