omit_data_loss config variable #4001
Conversation
richtja
force-pushed
the
omit_data_loss
branch
from
c11406e
to
ab30e55
Compare
|
Hi @smitterl, can you please test this in your workflow and let me know if this solution works for you? Thanks. |
|
Hi @richtja, I used your PR in migration job. In the test results, there are still the following errors: 
Could you help to check this issue? Thank you. |
|
Hi @cliping, IMO this should be ok. The idea is to keep the info about the error in the logs, but just omit it from the test result. So if the overall result is Of course, we can discuss how to remove them from logs as well. |
|
Hi @richtja, the final test result is FAIL. I don't know why this error is not omitted. 
|
|
@cliping Ok, so that is something which needs to be fixed. Thanks for letting me know, I'm going to look into that. |
|
@richtja any updates? There are a lot of failures(~20%) in recent runs due this OSError, it will be appreciated if you can fix the problem, thx! |
|
Agree with the others that the test must pass and if configured we don't want to see warnings or errors in the logs. As called out by @Yingshun we have a high rate of failures due to this. This also makes me assume we will configure this and likely never find the time to look further into the data loss. The instances that I've seen in our test runs don't deliver a compelling argument that these errors can't be skipped. But I guess that's avocado-vt specific and avocado will cover other situations. IIUC, you guys are worried about the data loss. Is there a way to avoid this data loss and write the data out into a file in the test log dir? |
|
Hi @smitterl, @Yingshun. I don't have any progress on this right now, but I will look into this in this week. So IIUIC your CI is scanyng for ERRORs and WARNINGs to find failures. How this scan work? Would be be possible to just change the message type from or do we need to remove the message completly from debug.log file? |
| "avocado.utils.process utility during clean up. This can be used " | ||
| "when this error would cause false positive failures of a test." | ||
| ) | ||
| add_option( |
There was a problem hiding this comment.
if introduce new option in avocado run (here --vt-omit-data-loss), it will bring additional efforts to update our CI to enable that. I wonder whether we can introduce one gating_variable e.g omit_data_loss in avocado_vt/conf.d/vt.conf, if omit_data_loss is set true, it enable it implicitly .
There was a problem hiding this comment.
With this PR, you can enable it with config option as well by setting vt.omit_data_loss to true. see https://github.com/avocado-framework/avocado-vt/pull/4001/files#diff-64c70a40c0700575186086930678d178f26a5b3fbe71dfe9d1ebd20d4d5e7752R135
But I am not sure if we should enable this implicitly, because it can hide real failures by skipping error messages. AFAIK this issue is related only to your CI and I think we shouldn't affect others by this.
avocado_vt/test.py
Outdated
| @@ -150,6 +150,11 @@ def setUp(self): | |||
| except exceptions.TestSkipError: | |||
| self.__exc_info = sys.exc_info() | |||
| raise # This one has to be raised in setUp | |||
| except OSError as e: | |||
| if not self._config.get("vt.omit_data_loss"): | |||
There was a problem hiding this comment.
since we need this solution urgently, if in short term, we can not have complete solution. Maybe we can just output the error message, but never raise exception in any situation. But surely we can open one issue to track limitation, and further work out complete solution
There was a problem hiding this comment.
I have just sent an update via force-push which keeps the messages, but it should stop raising the exceptions.
In tp_libvrt testing, We've seen many occurrences where "Bad file descriptor" OSError was raised during clean up phase when an external command is executed via `avocado.utils.process` utility. It happens when `SubProcess` class wants to flush the stdout and stderr of the external command after finishes. These kinds of errors might lead to stdout and stderr data loss, but during the tp_libvrt testing it leads to false positive failures, which makes test evaluation harder. This commit introduces a new config variable `omit_data_loss` which, when is enabled, will omit these errors, and they won't affect the overall test result. Reference: avocado-framework/avocado#6019 Signed-off-by: Jan Richter <[email protected]>
richtja
force-pushed
the
omit_data_loss
branch
from
ab30e55
to
6e40e56
Compare
|
Hi @cliping, @Yingshun, @smitterl, @chunfuwen I have just sent new version via About removing the messages from logs. Unfortunately, it looks like that it will be harder than I thought, because the logging happens at different places than the exception raise. |
|
Thank you all for your efforts on this issue! As chunfu mentioned, the results are good for now and we are waiting for further testing, so merge it. |
In tp_libvrt testing, We've seen many occurrences where "Bad file descriptor" OSError was raised during clean up phase when an external command is executed via
avocado.utils.processutility. It happens whenSubProcessclass wants to flush the stdout and stderr of the external command after finishes. These kinds of errors might lead to stdout and stderr data loss, but during the tp_libvrt testing it leads to false positive failures, which makes test evaluation harder.This commit introduces a new config variable
omit_data_losswhich, when is enabled, will omit these errors, and they won't affect the overall test result.Reference: avocado-framework/avocado#6019