<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jan 10, 2016 at 2:56 PM, Oved Ourfali <span dir="ltr">&lt;<a href="mailto:oourfali@redhat.com" target="_blank">oourfali@redhat.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><p dir="ltr"><br>
On Jan 10, 2016 3:37 PM, &quot;Nir Soffer&quot; &lt;<a href="mailto:nsoffer@redhat.com" target="_blank">nsoffer@redhat.com</a>&gt; wrote:<br>
&gt;<br>
&gt; On Sun, Jan 10, 2016 at 2:57 PM, Oved Ourfali &lt;<a href="mailto:oourfali@redhat.com" target="_blank">oourfali@redhat.com</a>&gt; wrote:<br>
&gt; &gt; Thanks for the summary!<br>
&gt; &gt; See one comment inline.<br>
&gt; &gt;<br>
&gt; &gt; On Sun, Jan 10, 2016 at 2:49 PM, Yaniv Bronheim &lt;<a href="mailto:ybronhei@redhat.com" target="_blank">ybronhei@redhat.com</a>&gt; wrote:<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; (fromani, nsoffer, ybronhei, alitke)<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt;  - Removing xmlrpc for good - who should accept it? where do we stand with<br>
&gt; &gt;&gt; full jsonrpc client ? (we didn&#39;t get to any conclusions and said that we&#39;ll<br>
&gt; &gt;&gt; reraise this topic next week with pioter)<br>
&gt; &gt;&gt;<br>
&gt; &gt;<br>
&gt; &gt; With regards to that, in order to move to 3.6 cluster level, you MUST have<br>
&gt; &gt; all hosts in jsonrpc protocol. So, we just need to make sure no piece of<br>
&gt; &gt; code uses that explicitly, and if so move that to jsonrpc as well.<br>
&gt;<br>
&gt; I don&#39;t remember that this was discussed here, and storage never<br>
&gt; approved this change<br>
&gt; for 3.6.  We need to keep the xmlrpc option in 3.6, as a backup for<br>
&gt; jsonrpc issues.<br>
&gt;</p>
</span><p dir="ltr">No we don&#39;t. And we won&#39;t. We had it around for one version for this reason, but no need for more. <br>
We had a bug about that, and communicated it to whomever is relevant. <br>
We want to get rid of it entirely in 4.0, so 3.6 cluster won&#39;t work with it. <br>
If you know of any issue then please fix it now, or open a bug about it. </p><span class="">
<p dir="ltr">&gt; We just fixed couple of jsonrpc verbs that were returning True instead of<br>
&gt; the correct return value (caused by incorrect schema).<br>
&gt; - <a href="https://github.com/oVirt/vdsm/commit/0ca680700596564b4d6b0ef01ed4b0ae7c488de7" target="_blank">https://github.com/oVirt/vdsm/commit/0ca680700596564b4d6b0ef01ed4b0ae7c488de7</a><br>
&gt; - <a href="https://gerrit.ovirt.org/40402" target="_blank">https://gerrit.ovirt.org/40402</a> VM.getDiskAlignment cannot be used in jsonrpc<br>
&gt;</p>
</span><p dir="ltr">That&#39;s great. As said earlier, if you know of others please open bugs. </p><div><div class="h5">
<p dir="ltr">&gt; However this is not the topic of the discussion, we are discussion the next<br>
&gt; version (3.7/4.0). We are adding lot of new verbs as part of removing the spm,<br>
&gt; and we don&#39;t want to invest time in adding xmlrpc and vdsClient support.<br>
&gt;<br>
&gt; Example new verb merged recently:<br>
&gt; <a href="https://github.com/oVirt/vdsm/commit/bbbb72a192d8b54d21c8d65f6a10278404a966db" target="_blank">https://github.com/oVirt/vdsm/commit/bbbb72a192d8b54d21c8d65f6a10278404a966db</a><br>
&gt;<br>
&gt; In the new verbs, we cleaned up the api, so integer values are passed<br>
&gt; as integers,<br>
&gt; not as strings. Previously we use to require strings since xmlrpc did<br>
&gt; not support large<br>
&gt; numbers (&gt; 2**31 - 1).<br>
&gt;<br>
&gt; So in the schema, we require now a uint:<br>
&gt;<br>
&gt; +##<br>
&gt; +# @CreateVolumeInfo:<br>
&gt; +#<br>
&gt; ...<br>
&gt; +# @virtual_size: The Volume size in bytes<br>
&gt; ...<br>
&gt; +# @initial_size: #optional If specified, the initial allocated size of volume<br>
&gt; +# in bytes. Allowed only when creating a thinly provisioned<br>
&gt; +# volume on block storage.<br>
&gt; +#<br>
&gt; +# Since: 4.18<br>
&gt; +##<br>
&gt; +{&#39;type&#39;: &#39;CreateVolumeInfo&#39;,<br>
&gt; + &#39;data&#39;: {&#39;sd_id&#39;: &#39;UUID&#39;, &#39;img_id&#39;: &#39;UUID&#39;, &#39;vol_id&#39;: &#39;UUID&#39;,<br>
&gt; + &#39;virtual_size&#39;: &#39;uint&#39;, &#39;vol_format&#39;: &#39;VolumeFormat&#39;,<br>
&gt; + &#39;disk_type&#39;: &#39;DiskType&#39;, &#39;description&#39;: &#39;str&#39;,<br>
&gt; + &#39;*parent_img_id&#39;: &#39;UUID&#39;, &#39;*parent_vol_id&#39;: &#39;UUID&#39;,<br>
&gt; + &#39;*initial_size&#39;: &#39;uint&#39;}}<br>
&gt;<br>
&gt; To support xmlrpc, we added this ugly code in bindingxmlrpc.py:<br>
&gt;<br>
&gt; + def sdm_create_volume(self, args):<br>
&gt; + validateArgTypes(args, [str, parse_json_obj])<br>
&gt; +<br>
&gt; + # Convert large integers to strings. The server&#39;s xmlrpc binding will<br>
&gt; + # restore them to their proper int types.<br>
&gt; + vol_info = args[1]<br>
&gt; + for param in &#39;virtual_size&#39;, &#39;initial_size&#39;:<br>
&gt; + if param in vol_info:<br>
&gt; + vol_info[param] = str(vol_info[param])<br>
&gt; +<br>
&gt; + res = self.s.sdm_create_volume(*args)<br>
&gt; + if res[&#39;status&#39;][&#39;code&#39;]:<br>
&gt; + return res[&#39;status&#39;][&#39;code&#39;], res[&#39;status&#39;][&#39;message&#39;]<br>
&gt; +<br>
&gt; + return 0, &#39;&#39;<br>
&gt; +<br>
&gt;<br>
&gt; To support vdsClient, we added this:<br>
&gt;<br>
&gt; + def sdm_create_volume(self, job_id, vol_info):<br>
&gt; + sdm = API.SDM()<br>
&gt; +<br>
&gt; + # As a workaround for the 32bit signed integer limitation of xmlrpc,<br>
&gt; + # allow large integers to be passed as strings. We convert them back<br>
&gt; + # to the correct type here.<br>
&gt; + for param in &#39;virtual_size&#39;, &#39;initial_size&#39;:<br>
&gt; + if param in vol_info:<br>
&gt; + vol_info[param] = int(vol_info[param])<br>
&gt; +<br>
&gt; + return sdm.create_volume(job_id, vol_info)<br>
&gt; +<br>
&gt;<br>
&gt; All this work is waste effort on our side.<br>
&gt;<br>
&gt; We should make a decision now - do we support xmlrpc in vdsm?<br>
&gt;<br>
&gt; I think we should not support it, after two version we support both<br>
&gt; xmlrpc and jsonrpc.</p>
</div></div><p dir="ltr">I agree. But again, 3.6 clusters will require jsonrpc. That&#39;s why in 4.0 xmlrpc will no longer be relevant. </p><div class="HOEnZb"><div class="h5">
<p dir="ltr">&gt;<br>
&gt; If we stop supporting xmlrpc, we must have a replacement for vdsClient,<br>
&gt;<br>
&gt; We need to have commnad line client, both for development, and for sos<br>
&gt; plugin. I think this patch, owned now by Piotr, is the best direction:<br>
&gt; <a href="https://gerrit.ovirt.org/35181/" target="_blank">https://gerrit.ovirt.org/35181/</a><br>
&gt;<br>
&gt; But we need to give this high priority, as having a command line client is<br>
&gt; a must for developing vdsm.<br>
&gt;<br>
&gt; Adding Aharon - supporting both xmlrpc and jsonrpc means we need to test<br>
&gt; everything twice,  I don&#39;t think Aharion will like to do that.<br>
&gt;<br>
&gt; Piotr, Francesco, Dan, Adam: your thoughts?<br>
&gt;<br></p></div></div></blockquote><div>I would love to get rid of it but we need to make sure that none of the code is using<br></div><div>xmlrpc. I think that we need to prioritize tasks before we can stop using it.<br><br></div><div>It would be great to change format of the schema for new client and drop/replace<br></div><div>code which is still using xmlrpc.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><p dir="ltr">
&gt; &gt;&gt;<br>
&gt; &gt;&gt;  - Moving from nose to pytest - generally good approach to achieve. It<br>
&gt; &gt;&gt; requires some changes in current testlib.py code. must be an item for next<br>
&gt; &gt;&gt; major version (nir already managed to run most of the tests with it, and<br>
&gt; &gt;&gt; stated few gaps)<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt;  - Exception patches - still on progress, please review<br>
&gt; &gt;&gt; (<a href="https://gerrit.ovirt.org/48868" target="_blank">https://gerrit.ovirt.org/48868</a>)<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt;  - python3 effort to cover all asyncProc usage, and allowing utils import<br>
&gt; &gt;&gt; without having python3-cpopen - <a href="https://gerrit.ovirt.org/51421" target="_blank">https://gerrit.ovirt.org/51421</a><br>
&gt; &gt;&gt; <a href="https://gerrit.ovirt.org/49441" target="_blank">https://gerrit.ovirt.org/49441</a> . still under review<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; We didn&#39;t take notes during that talk, so if I forgot to mention something<br>
&gt; &gt;&gt; I apologize. Feel free to reply and raise it<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; Greetings,<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; --<br>
&gt; &gt;&gt; Yaniv Bronhaim.<br>
&gt; &gt;&gt;<br>
&gt; &gt;&gt; _______________________________________________<br>
&gt; &gt;&gt; Devel mailing list<br>
&gt; &gt;&gt; <a href="mailto:Devel@ovirt.org" target="_blank">Devel@ovirt.org</a><br>
&gt; &gt;&gt; <a href="http://lists.ovirt.org/mailman/listinfo/devel" target="_blank">http://lists.ovirt.org/mailman/listinfo/devel</a><br>
&gt; &gt;<br>
&gt; &gt;<br>
&gt; &gt;<br>
&gt; &gt; _______________________________________________<br>
&gt; &gt; Devel mailing list<br>
&gt; &gt; <a href="mailto:Devel@ovirt.org" target="_blank">Devel@ovirt.org</a><br>
&gt; &gt; <a href="http://lists.ovirt.org/mailman/listinfo/devel" target="_blank">http://lists.ovirt.org/mailman/listinfo/devel</a><br>
</p>
</div></div></blockquote></div><br></div></div>