<br><br><div class="gmail_quote">On Sat, Mar 9, 2013 at 4:54 PM, Michael J Fork <span dir="ltr"><<a href="mailto:mjfork@us.ibm.com" target="_blank">mjfork@us.ibm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<p><tt><font>John Griffith <<a href="mailto:john.griffith@solidfire.com" target="_blank">john.griffith@solidfire.com</a>> wrote on 03/09/2013 12:09:41 AM:<br>
<br>
> From: John Griffith <<a href="mailto:john.griffith@solidfire.com" target="_blank">john.griffith@solidfire.com</a>></font></tt><br>
<tt><font>> To: OpenStack Development Mailing List <<a href="mailto:openstack-dev@lists.openstack.org" target="_blank">openstack-dev@lists.openstack.org</a>>, </font></tt><br>
<tt><font>> Date: 03/09/2013 12:22 AM</font></tt><br>
<tt><font>> Subject: Re: [openstack-dev] [Cinder] test_preattach_status_volume <br>
> test failure</font></tt></p><div><div class="h5"><br>
<tt><font>> <br>
> <br>
</font></tt><br>
<tt><font>> On Fri, Mar 8, 2013 at 1:07 AM, Michael J Fork <<a href="mailto:mjfork@us.ibm.com" target="_blank">mjfork@us.ibm.com</a>> wrote:</font></tt><br>
<tt><font>> After updating the Cinder oslo rpc libraries to the latest (see <br>
> <a href="https://review.openstack.org/#/c/23822/" target="_blank">https://review.openstack.org/#/c/23822/</a>), Cinder <br>
> test_preattach_status_volume fails with the following:<br>
> <br>
> ======================================================================<br>
> FAIL: Ensure volume goes into pre-attaching state<br>
> ----------------------------------------------------------------------<br>
> Traceback (most recent call last):<br>
> File "/mnt/home/openstack/dev/cinder/cinder/tests/test_volume.py",<br>
> line 308, in test_preattach_status_volume<br>
> self.assertEqual(vol['status'], "available")<br>
> <br>
> and the test<br>
> <br>
> def test_preattach_status_volume(self):<br>
> """Ensure volume goes into pre-attaching state"""<br>
> instance_uuid = '12345678-1234-5678-1234-567812345678'<br>
> mountpoint = "/dev/sdf"<br>
> volume = db.volume_create(self.context, {'size': 1,<br>
> 'status': 'available'})<br>
> volume_id = volume['id']<br>
> <br>
> volume_api = cinder.volume.api.API()<br>
> volume_api.attach(self.context, volume, instance_uuid, mountpoint)<br>
> <br>
> vol = db.volume_get(self.context, volume_id)<br>
> self.assertEqual(vol['status'], "available")<br>
> self.assertEqual(vol['attach_status'], None)<br>
> self.assertEqual(vol['instance_uuid'], None)<br>
> <br>
> I would expect volume_api.attach call to set the state to "in-use" <br>
> (as the error shows it was) vs still being "available". Is this a <br>
> legitimate bug that needs fixed or is the error caused by something <br>
> in the oslo update?<br>
> <br>
> Thanks.<br>
> <br>
> Michael<br>
> <br>
> -------------------------------------------------<br>
> Michael Fork<br>
> Architect, OpenStack Development<br>
> IBM Systems & Technology Group</font></tt><br>
<tt><font>> <br>
> _______________________________________________<br>
> OpenStack-dev mailing list<br>
> <a href="mailto:OpenStack-dev@lists.openstack.org" target="_blank">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>
</font></tt><br>
<tt><font>> There's a bug in here (although I suspect it's a bug in the unit <br>
> test code), just haven't figured out how the oslo change exposed it.<br>
> The status should be "in-use" as you pointed out, if you run the <br>
> test individually or even just test_volume:VolumeTestCase, it <br>
> passes. So there's something wonky in the test setup somewhere.</font></tt><br>
<br>
</div></div><tt><font>I did some digging, and it looks like a subsequent refactoring (<a href="https://github.com/openstack/cinder/commit/2331d3336a6adf4fc13a3b187e91a5d1b1f7c723" target="_blank">https://github.com/openstack/cinder/commit/2331d3336a6adf4fc13a3b187e91a5d1b1f7c723</a>) of the original commit (<a href="https://github.com/openstack/cinder/commit/e27b171883c1b87b8d2fdf0994947d4b93e640d9" target="_blank">https://github.com/openstack/cinder/commit/e27b171883c1b87b8d2fdf0994947d4b93e640d9</a>) modified the code path here but didn't retain the original purpose of this test (which was to ensure the "attaching" state was set with an instance UUID on the way to being "attached"). However, I don't know the right way to fix. Looking at attach_volume in </font></tt><tt><font>cinder/volume/manager.py, I can see the volume_update call right before attach)volume on the driver is called. This test needs to be inserted right after volume_update. How is that best done?</font></tt><div class="im">
<br>
<tt><font> <br>
> _______________________________________________<br>
> OpenStack-dev mailing list<br>
> <a href="mailto:OpenStack-dev@lists.openstack.org" target="_blank">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>
</font></tt><br>
<font face="sans-serif">Michael<br>
<br>
-------------------------------------------------<br>
Michael Fork<br>
Architect, OpenStack Development<br>
IBM Systems & Technology Group</font><br>
</div><p></p></div><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>
<br></blockquote></div>Hey Michael,<div><br></div><div>Thanks for following up on this, I tracked down the commit as you did and started looking at things. What is interesting here though (and what distracted me a bit) is the fact that if you run the individual test it makes it's way through the manager code path and works as the test is written. Same holds true if you run the entire file "test_volume".</div>
<div><br></div><div>It seems that something isn't getting cleaned up properly by another test somewhere or an assumption was made when this test was written. I have not figured out the root cause of the issue. This is one of my gripes about a large percentage of the unit tests, they're a bit convoluted and don't necessarily really test what one would expect them to.</div>
<div><br></div><div>I'll leave it up to you, if you'd like to disable that test we can log a bug and note that it's expected to be a "unit test" issue and not an issue in the code and I can revisit later. </div>
<div><br></div><div>Thanks,</div><div>John</div><div><br></div><div><br></div>