Dan Kenigsberg has uploaded a new change for review.
Change subject: drop ominous log for libvirt errors ......................................................................
drop ominous log for libvirt errors
Patch http://gerrit.ovirt.org/13990 has introduced a log line whenever a libvirt exception flows through our libvirtconnection.
E.g. Unknown libvirterror: ecode: 9 edom: 19 level: 2 message: operation failed: network 'vdsm-ovirtmgmt' already exists...
The log line appears out of context and is repeated once its traceback is properly caught.
Change-Id: Icd2a53ffee7fb78cb1c8d171093e93e233ed5ad4 Signed-off-by: Dan Kenigsberg danken@redhat.com --- M lib/vdsm/libvirtconnection.py 1 file changed, 0 insertions(+), 4 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/24/17324/1
diff --git a/lib/vdsm/libvirtconnection.py b/lib/vdsm/libvirtconnection.py index cdba57c..9891691 100644 --- a/lib/vdsm/libvirtconnection.py +++ b/lib/vdsm/libvirtconnection.py @@ -95,10 +95,6 @@ if killOnFailure: log.error('taking calling process down.') os.kill(os.getpid(), signal.SIGTERM) - else: - log.debug('Unknown libvirterror: ecode: %d edom: %d ' - 'level: %d message: %s', ecode, edom, - e.get_error_level(), e.get_error_message()) raise wrapper.__name__ = f.__name__ wrapper.__doc__ = f.__doc__
Antoni Segura Puimedon has posted comments on this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Patch Set 1: Looks good to me, but someone else must approve
Yes, a thousand times yes!!!
Dan Kenigsberg has posted comments on this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Patch Set 1: Verified
Michal Skrivanek has posted comments on this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Patch Set 1: I would prefer that you didn't submit this
I'd still be in favor of keeping that, not logged as an error but informatively it's useful...Vinzenz please correct me if I'm wrong, but one reason to add this was that we swallow the error in some places and do not know what's happening
Dan Kenigsberg has posted comments on this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Patch Set 1:
We should never swallow an exception without really good commented reason. Michal, please find *one* place where we do that with libvirt-originating exception, and I will add a log *there*.
Michal Skrivanek has posted comments on this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Patch Set 1:
the point is we do want to see what error code libvirt is returning for debug purposes. We do not handle/capture that in vm.py in few places...probably elsewhere. Again, Vinzenz should comment on that, but even I can tell from my investigations it is helpful.
We do see that often in virt flows because of lookupByUUID, and so I suppose it's a similar case with network flows...but unless we report the errcode in all places when libvirtconnection is used I think we need a debug info about what happened in libvirt. We often do not get complete/all logs so any info is helpful. From this POV it makes sense to me to keep this as a nonimportant debug-level log
oVirt Jenkins CI Server has posted comments on this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/2700/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/3507/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/3591/ : SUCCESS
Vinzenz Feenstra has posted comments on this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Patch Set 1: Code-Review-1
Well as michal said we need this for post mortem debugging.
Dan Kenigsberg has posted comments on this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Patch Set 1:
Was this log line helpful in its 11 months of existence? To me it causes only confusion and lost heart beats.
Vinzenz Feenstra has posted comments on this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Patch Set 1:
If we wouldn't swallow so many exceptions all over the place, I wouldn't have a problem with that. But vdsm is full of except/pass lines where we would never know if anything like this was reported. This is now a debug message and it is harmless in comparison the amount of messages which come from storage for example.
There were moments which would have made our lives enormously easier with having this log line. Especially when libvirt is changing/adding error codes. In those cases it's pretty hard to understand why something that used to work does not work any longer and this is the reason why we put this log in place. I honestly do not understand the aversion of this. There's probably much more less useful stuff logged in vdsm and this is the item I am insisting to keep it.
And it doesn't matter if in the past 11 months there was no such instance happening again. My prime reason is the commit I introduced this for. Look how long it took to realize what the problem is in the bug related to that original commit: https://bugzilla.redhat.com/show_bug.cgi?id=852956
It's more than half a year before we figured out what was the real deal. If we would have had the codes somewhere logged we could have seen some potential difference. Especially the difference between the libvirt version on Fedora and on RHEL. They are quite far apart even if features get backported.
Itamar Heim has posted comments on this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Patch Set 1:
ping
Dan Kenigsberg has abandoned this change.
Change subject: drop ominous log for libvirt errors ......................................................................
Abandoned
I reserve the right to restore this patch, and remove the needless log line.
vdsm-patches@lists.fedorahosted.org