On 10/10/18 10:14 AM, Nir Soffer wrote:
Adding devel.
---------- Forwarded message --------- From: *Nir Soffer* <nsoffer@redhat.com <mailto:nsoffer@redhat.com>> Date: Wed, 10 Oct 2018, 11:07 Subject: Re: VDSM API schema inconsistencies To: Piotr Kliczewski <pkliczew@redhat.com <mailto:pkliczew@redhat.com>> Cc: Dan Kenigsberg <danken@redhat.com <mailto:danken@redhat.com>>, Edward Haas <ehaas@redhat.com <mailto:ehaas@redhat.com>>, Milan Zamazal <mzamazal@redhat.com <mailto:mzamazal@redhat.com>>, Tomasz Baranski <tbaransk@redhat.com <mailto:tbaransk@redhat.com>>, Francesco Romani <fromani@redhat.com <mailto:fromani@redhat.com>>, Irit Goihman <igoihman@redhat.com <mailto:igoihman@redhat.com>>, Marcin Sobczyk <msobczyk@redhat.com <mailto:msobczyk@redhat.com>>
On Wed, 10 Oct 2018, 10:21 Piotr Kliczewski, <pkliczew@redhat.com <mailto:pkliczew@redhat.com>> wrote:
+Nir
On Wed, Oct 10, 2018 at 8:39 AM Marcin Sobczyk <msobczyk@redhat.com <mailto:msobczyk@redhat.com>> wrote:
Hi, I would love to get some feedback from you about it on the VDSM call today so if you have a moment, please take a look. Marcin
---------- Forwarded message --------- From: *Marcin Sobczyk* <msobczyk@redhat.com <mailto:msobczyk@redhat.com>> Date: Mon, Oct 8, 2018 at 2:40 PM Subject: VDSM API schema inconsistencies To: devel <devel@ovirt.org <mailto:devel@ovirt.org>>
Hi,
when I was working on BZ#1624306 it turned out, that even if I'd fix the help-printing code in vdsm-client, there's still a lot of inconsistencies in API's schema files.
After some investigation, I found that there's a code for reporting schema inconsistencies, but it's turned off because it bloats the logs.
Right, an it should remain like this. In runtime we never want these warnings.
There's also a "strict" mode that raises an exception each time an inconsistency is found, but it's also off because there's so many of them.
Same.
Both are debugging aids for developers, to make it easy to check the schema.
I think that no one wants to maintain both: the actual code and schema files.
This is not a big issue. We rarely add or change APIs. But it is nice if we don't need to manualy make the schema correct.
My idea would be to make the schema derived from the code at build time as a long-term effort.
This cannot work, since clients depend on the schema, it should not change if when someone makes uintended change in the code.
It should work in the opposite way, you change the schema, and generate new code frome schema.
Threre are existing tools like grpc and thrift that work in this way. For long term we should drop our jsonrpc/stomp stack and move to grpc. Switching to grpc or thrift is a great idea, but that's also a great effort.
For short term we can just fix the schema to reflect actual engine interaction. when engine and schema do not match, engine is usualy right. For responses, if engine can handle the response the code is right.
Please do not make RPC code more complex and inefficient for improving schema issues detection. I would rather remove this code instead.
My goal is quite opposite: * remove as much code as possible * switch to something more standard like JSON Schema for validation * do the validation only in our test suites and not the client code * catch errors ASAP - preferably at build time, without having to run any code at all Maybe my intention was not clear enough, so I've assembled a simple PoC: http://pastebin.test.redhat.com/655651 To run it you'll need to install 'typed-ast' in your venv.
Before we can do that though, we need to address the inconsistencies first.
https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic:vd...
This is a series of patches that helps fixing schema issues. What it does, is it adds a bunch of information from stack to vastly ease the process of fixing them. The logging level is also changed from 'WARNING' to 'DEBUG'. Here's an example of a logged entry:
Host.setKsmTune With message: Required property pages_to_scan is not provided when calling Host.setKsmTune With backtrace: [ [ "_verify_object_type", { "call_arg_keys":[ "merge_across_nodes", "run" ], "schema_type_name":"KsmTuneParams" } ], [ "_verify_complex_type", { "schema_type_type":"object" } ] ]
To make it work, a patch for OST's 'basic-master-suite' is on the way that switches 'schema.inconsistency' logger's logging level do 'DEBUG'. That way, we can find all reported errors in 'vdsm-schema-error.log' files.
An initial report that groups reported errors by namespaces/methods has also been made. Please note, that an implementation that completely avoids error duplicates is really hard and IMHO not worth the effort. You can find the results here:
* Host: https://pastebin.com/YM2XnvTV * Image: https://pastebin.com/GRc1yErL * LVMVolumeGroup: https://pastebin.com/R1276gTu * SDM: https://pastebin.com/P3tfYEDD * StorageDomain: https://pastebin.com/Q0DxKgF5 * StoragePool: https://pastebin.com/VXFWpfRC * VM: https://pastebin.com/tDC60c29 * Volume: https://pastebin.com/TYkr9Zsd * |jobs|: https://pastebin.com/jeXpYyz9 * |virt|: https://pastebin.com/nRREuEub
File a bug per vertical (infra, storage, virt) for these issues. Since we don't know about real user issues with these apis this must be incorrect schema.
Will do so.
Nir
_______________________________________________ Devel mailing list -- devel@ovirt.org To unsubscribe send an email to devel-leave@ovirt.org Privacy Statement: https://www.ovirt.org/site/privacy-policy/ oVirt Code of Conduct: https://www.ovirt.org/community/about/community-guidelines/ List Archives: https://lists.ovirt.org/archives/list/devel@ovirt.org/message/JHTGTSN376HHJH...