<div class="gmail_quote">On Tue Nov 25 2014 at 7:20:00 AM Sean Dague <<a href="mailto:sean@dague.net">sean@dague.net</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 11/25/2014 10:07 AM, Jim Rollenhagen wrote:<br>
> On Tue, Nov 25, 2014 at 08:02:56AM -0500, Sean Dague wrote:<br>
>> When at Summit I discovered that check-tempest-dsvm-ironic-pxe_<u></u>ssh is<br>
>> now voting on Nova check queue. The reasons given is that the Nova team<br>
>> ignored the interface contract that was being provided to Ironic, broke<br>
>> them, so the Ironic team pushed for co-gating (which basically means the<br>
>> interface contract is now enforced by a 3rd party outside of Nova / Ironic).<br>
>><br>
>> However, this was all in vague term, and I think is exactly the kind of<br>
>> thing we don't want to do. Which is use the gate as a proxy fight over<br>
>> teams breaking contracts with other teams.<br>
>><br>
>> So I'd like to dive into what changes happened and what actually broke,<br>
>> so that we can get back to doing this smarter.<br>
>><br>
>> Because if we are going to continue to grow as a community, we have to<br>
>> co-gate less. It has become a crutch to not think about interfaces and<br>
>> implications of changes, and is something we need to be doing a lot less of.<br></blockquote><div><br></div><div>Definitely -- and we're already co-gating less than other projects :)</div><div><br></div><div>You might notice that ironic jobs aren't running in the check queues for any other Integrated project, even though Ironic depends on interacting directly with Glance, Neutron, Keystone, and (for some drivers) Swift.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> Completely agree here; I would love to not gate Nova on Ironic.<br>
><br>
> The major problem is that Nova's driver API is explicitly not stable.[0]<br>
> If the driver API becomes stable and properly versioned, Nova should be<br>
> able to change the driver API without breaking Ironic.<br>
><br>
> Now, this won't fix all cases where Nova could break Ironic. The<br>
> resource tracker or scheduler could change in a breaking way. However, I<br>
> think the driver API has been the most common cause of Ironic breakage,<br>
> so I think it's a great first step.<br>
><br>
> // jim<br>
><br>
> [0] <a href="http://docs.openstack.org/developer/nova/devref/policies.html#public-contractual-apis" target="_blank">http://docs.openstack.org/<u></u>developer/nova/devref/<u></u>policies.html#public-<u></u>contractual-apis</a><br>
<br>
So we actually had a test in tree for that part of the contract...<br>
<br></blockquote><div><br></div><div>We did. It was added here:</div><div>  <a href="https://review.openstack.org/#/c/98201">https://review.openstack.org/#/c/98201</a></div><div><br></div><div>.. and removed here:</div><div>  <a href="https://review.openstack.org/#/c/111425/">https://review.openstack.org/#/c/111425/</a></div><div> </div><div>.. because there is now unit test coverage for the internal API usage by in-tree virt drivers via assertPublicAPISignatures() here:</div><div>  <a href="https://git.openstack.org/cgit/openstack/nova/tree/nova/test.py#n370">https://git.openstack.org/cgit/openstack/nova/tree/nova/test.py#n370</a></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
We don't need the surface to be public contractual, we just need to know<br>
what things Ironic is depending on and realize that can't be changed<br>
without some compatibility code put in place.</blockquote><div><br></div><div>There is the virt driver API, which is the largest and most obvious one. Then there's the HostManager, ResourceTracker, and SchedulerFilter classes (or sublcasses thereof). None of these are "public contractual APIs" as defined by Nova. Without any guarantee of stability in those interfaces, I believe co-gating is a reasonable thing to do between the projects.</div><div><br></div><div>Also, there have been discussions [2] at and leading up to the Paris summit (some ongoing for many cycles now) regarding changing every one of those interfaces. Until those interfaces are refactored / split out / otherwise deemed stable, I would like to continue running Ironic's functional tests on all Nova changes. If you think we don't need to co-gate while that work is underway, I'd like to understand why, and what that would look like.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Again... without knowing exactly what happened (I was on leave) it's<br>
hard to come up with a solution. However, I think the co-gate was an<br>
elephant gun that we don't actually want.<br></blockquote><div><br></div><div>Apologies, but I don't recall exactly what time period you were on leave for, so you may have already seen some or all of these.</div><div><br></div><div>I have looked up up several cases of asymmetrical breaks that happened (due to changes in multiple projects) during Juno (before Ironic was voting in Nova's check queue). At least one of these was the result of a change in Nova after the Ironic driver merged. Links at the bottom for reference [1].</div><div><br></div><div>Here is a specific example where a patch introduced subtle behavior changes within the resource tracker that were not caught by any of Nova's tests, and would not have been caught by the API contract test, even if that had been in place at the time, nor any other API contract test, since it did not, in fact, change the API. It changed a behavior of the resource tracker which, it turns out, libvirt does not use (at least within the devstack-gate environment).</div><div><br></div><div><a href="https://review.openstack.org/#/c/71557/33/nova/compute/resource_tracker.py">https://review.openstack.org/#/c/71557/33/nova/compute/resource_tracker.py</a><br></div><div><br></div><div>The problem is at line 385, where the supplied 'stats' are overwritten. None of the libvirt-based tests touched this code path, though.</div><div><br></div><div><div>> def _write_ext_resources(self, resources):</div><div>>    resources['stats'] = {}        ## right here</div><div>>    resources['stats'].update(self.stats)</div><div>>    self.ext_resources_handler.write_resources(resources)</div></div><div><br></div><div><br></div><div><br></div><div>-Devananda</div><div><br></div><div><br></div><div>[1]</div><div><a href="https://review.openstack.org/#/c/120857/">https://review.openstack.org/#/c/120857/</a><br></div><div><div><a href="https://review.openstack.org/#/c/111538/">https://review.openstack.org/#/c/111538/</a></div><div><a href="https://review.openstack.org/#/c/107562/">https://review.openstack.org/#/c/107562/</a></div><div><a href="https://review.openstack.org/#/c/105599/">https://review.openstack.org/#/c/105599/</a><br></div><div><a href="https://review.openstack.org/#/c/71557/">https://review.openstack.org/#/c/71557/</a></div></div><div><a href="https://review.openstack.org/#/c/68942/">https://review.openstack.org/#/c/68942/</a><br></div><div><br></div><div>[2]</div><div>splitting out virt drivers<br></div><div><a href="http://lists.openstack.org/pipermail/openstack-dev/2014-September/044872.html">http://lists.openstack.org/pipermail/openstack-dev/2014-September/044872.html</a><br></div><div><br></div><div>splitting out the scheduler and resource tracker</div><div><a href="https://etherpad.openstack.org/p/kilo-nova-scheduler-rt">https://etherpad.openstack.org/p/kilo-nova-scheduler-rt</a><br></div><div><br></div><div>support for clustered hypervisors // why Ironic subclasses ComputeManager</div><div><a href="https://etherpad.openstack.org/p/juno-nova-clustered-hypervisor-support">https://etherpad.openstack.org/p/juno-nova-clustered-hypervisor-support</a><br></div></div>