xenserver / status-report

Program that gathers data for xenserver host diagnostics
GNU Lesser General Public License v2.1
1 stars 9 forks source link

Migrate from broad Exception handling in the filter and collection function to wrapping them into an Exception handled call #72

Open bernhardkaindl opened 7 months ago

bernhardkaindl commented 7 months ago

In #67, @edwintorok had a very good observation and good proposals to implement - my @bernhardkaindl comments are inline:

If the real reason is to avoid broad try-excepts, can we do that here? There are only a limited number of exceptions that 'open' can raise, and KeyError is certainly not one we should catch, and I understand the desire to not mask these errors.

Yes, and:

I think the reason for catching broad errors in status-report may be a good one though: if one of the status report gathering functions fail you don't want to give up on the entire script, there may still be valuable information you can gather from other files.

Yes, this was the intention why the broad Exceptoins are used, Ross told me.

Perhaps a better approach (that could be addressed in a future PR) would be:

  • have some kind of top-level try/catch handler whenever we invoke a function to collect something for capability "X", and if that fails we log the exception somewhere (general xen-bugtool.out or the file that was supposed to contain our output, etc.), but keep going and capture other things. Then the broad try/except would only wrap a very small number of lines (i.e. 1 or 2, just invoke the gathering function for that capability)

Yes, the filter functions like this and the data retrieval functions (those called from func_output) are called from these mid-level functions and we can wrap the calls to these functions into appropiate exception handling and log the exceptions.

  • you've already added a check on whether the file exists which avoids logging errors unnecessarily when they are not errors

Yep, that's done.

  • status-report could keep a count of how many of these broad try/except it has caught and logged, and exit non-zero at the end, which should allow XenRT to detect this and fail (we should whether it'd still store the already created bugtool in that case, as that is still very useful in debugging what went wrong)

Yep, if the count of exceptions is higher than expected, we should still finish but give a warning at the end and exit with an error exit status.

In a second step, once the script is cleaned up, if we ever get such exceptions, then we don't need the count anymore and can indicate the exceptions using a message and an error exit status always.