<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 12, 2014 at 12:23 AM, Mark McLoughlin <span dir="ltr"><<a href="mailto:markmc@redhat.com" target="_blank">markmc@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, 2014-08-11 at 15:25 -0700, Joe Gordon wrote:<br>
><br>
><br>
><br>
> On Sun, Aug 10, 2014 at 11:59 PM, Mark McLoughlin <<a href="mailto:markmc@redhat.com">markmc@redhat.com</a>><br>
> wrote:<br>
> On Fri, 2014-08-08 at 09:06 -0400, Russell Bryant wrote:<br>
> > On 08/07/2014 08:06 PM, Michael Still wrote:<br>
> > > It seems to me that the tension here is that there are<br>
> groups who<br>
> > > would really like to use features in newer libvirts that<br>
> we don't CI<br>
> > > on in the gate. Is it naive to think that a possible<br>
> solution here is<br>
> > > to do the following:<br>
> > ><br>
> > > - revert the libvirt version_cap flag<br>
> ><br>
> > I don't feel strongly either way on this. It seemed useful<br>
> at the time<br>
> > for being able to decouple upgrading libvirt and enabling<br>
> features that<br>
> > come with that.<br>
><br>
><br>
> Right, I suggested the flag as a more deliberate way of<br>
> avoiding the<br>
> issue that was previously seen in the gate with live<br>
> snapshots. I still<br>
> think it's a pretty elegant and useful little feature, and<br>
> don't think<br>
> we need to use it as proxy battle over testing requirements<br>
> for new<br>
> libvirt features.<br>
><br>
><br>
> Mark,<br>
><br>
><br>
> I am not sure if I follow. The gate issue with live snapshots has<br>
> been worked around by turning it off [0], so presumably this patch is<br>
> forward facing. I fail to see how this patch is needed to help the<br>
> gate in the future.<br>
<br>
</div></div>On the live snapshot issue specifically, we disabled it by requiring<br>
1.3.0 for the feature. With the version cap set to 1.2.2, we won't<br>
automatically enable this code path again if we update to 1.3.0. No<br>
question that's a bit of a mess, though.<br></blockquote><div><br></div><div>Agreed</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The point was a more general one - we learned from the live snapshot<br>
issue that having a libvirt upgrade immediately enable new code paths<br>
was a bad idea. The patch is a simple, elegant way of avoiding that.<br>
<div class=""><br>
> Wouldn't it just delay the issues until we change the version_cap?<br>
<br>
</div>Yes, that's the idea. Rather than having to scramble when the new<br>
devstack-gate image shows up, we'd be able to work on any issues in the<br>
context of a patch series to bump the version_cap.<br></blockquote><div><br></div><div>So the version_cap flag only possibly help for bugs in libvirt that are triggered by new nova code paths, and not bugs that are triggered by existing nova code paths that trigger a libvirt regression. Furthermore it can only catch libvirt bugs that trigger frequently enough to be caught on the patch to bump the version_cap, and we commonly have bugs that are 1 in a 1000 these days. This sounds like a potential solution for a very specific case when I would rather see a more general solution.</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
> The issue I see with the libvirt version_cap [1] is best captured in<br>
> its commit message: "The end user can override the limit if they wish<br>
> to opt-in to use of untested features via the 'version_cap' setting in<br>
> the 'libvirt' group." This goes against the very direction nova has<br>
> been moving in for some time now. We have been moving away from<br>
> merging untested (re: no integration testing) features. This patch<br>
> changes the very direction the project is going in over testing<br>
> without so much as a discussion. While I think it may be time that we<br>
> revisited this discussion, the discussion needs to happen before any<br>
> patches are merged.<br>
<br>
</div>You put it well - some apparently see us moving towards a zero-tolerance<br>
policy of not having any code which isn't functionally tested in the<br>
gate. That obviously is not the case right now.<br>
<br>
The sentiment is great, but any zero-tolerance policy is dangerous. I'm<br>
very much in favor of discussing this further. We should have some<br>
principles and goals around this, but rather than argue this in the<br>
abstract we should be open to discussing the tradeoffs involved with<br>
individual patches.<br></blockquote><div><br></div><div>To bad the mid-cycle just passed this would have been a great discussion for it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
> I am less concerned about the contents of this patch, and more<br>
> concerned with how such a big de facto change in nova policy (we<br>
> accept untested code sometimes) without any discussion or consensus.<br>
> In your comment on the revert [2], you say the 'whether not-CI-tested<br>
> features should be allowed to be merged' debate is 'clearly<br>
> unresolved.' How did you get to that conclusion? This was never<br>
> brought up in the mid-cycles as a unresolved topic to be discussed. In<br>
> our specs template we say "Is this untestable in gate given current<br>
> limitations (specific hardware / software configurations available)?<br>
> If so, are there mitigation plans (3rd party testing, gate<br>
> enhancements, etc)" [3]. We have been blocking untested features for<br>
> some time now.<br>
<br>
</div>Asking "is this tested" in a spec template makes a tonne of sense.<br>
Requiring some thought to be put into mitigation where a feature is<br>
untestable in the gate makes sense. Requiring that the code is tested<br>
where possible makes sense. It's a zero-tolerance "get your code<br>
functionally tested or GTFO" policy that I'm concerned about.<br>
<div class=""><br>
> I am further perplexed by what Daniel Berrange, the patch author,<br>
> meant when he commented [2] "Regardless of the outcome of the testing<br>
> discussion we believe this is a useful feature to have." Who is 'we'?<br>
> Because I don't see how that can be nova-core or even nova-specs-core,<br>
> especially considering how many members of those groups are +2 on the<br>
> revert. So if 'we' is neither of those groups then who is 'we'?<br>
<br>
</div>That's for Dan to answer, but I think you're either nitpicking or have a<br>
very serious concern.<br>
<br>
If nitpicking, Dan could just be using the "Royal 'We'" :) Or he could<br>
just mean the nova-core reviewers who +2ed the patch.<br>
<br>
On the more serious front, I'm conscious that Dan, Russell and I all<br>
work for Red Hat and maybe you're really asking whether "we" means "Red<br>
Hat" - i.e. some sort of corporate agenda. Absolutely not. The debate<br>
kicked off here:<br>
<br>
<a href="https://review.openstack.org/103923" target="_blank">https://review.openstack.org/103923</a><br>
<br>
I think it's pretty clear everyone is coming at this with good-faith,<br>
genuine personal opinions and concerns.<br>
<div class="HOEnZb"><div class="h5"><br>
Mark.<br>
<br>
<br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div><br></div></div>