<div dir="ltr"><div>This series is part of the priority feature libvirt-instance-storage[1]. At</div><div>first glance it may not be immediately apparent why, so I'll work backwards for</div><div>context.</div><div><br></div><div>The purpose of the feature is to create an unambiguous, canonical source of</div><div>metadata about 'local' storage. 'Local' here is defined to be storage which is</div><div>directly managed by Nova, so non-volume root, ephemeral, swap, and config</div><div>disks. This storage may not actually be local (eg Rbd), but it's managed</div><div>locally, so that's how we treat it. The justification for wanting to do this is</div><div>in the spec, so I won't repeat it here.</div><div><br></div><div>In order to solve this problem, we first had to fix the interface between</div><div>libvirt.driver and libvirt.imagebackend. As it stands, the code currently calls</div><div>Image.cache(), passing a callback function which will write data to a target.</div><div>The problem is, we need this context in order to know in advance how the disk</div><div>will be persisted. The lack of this context also results in some tortured</div><div>layering violations, such as the SUPPORTS_CLONE interface, where we essentially</div><div>push Rbd-specific logic into driver. There are many examples of this. The</div><div>solution to this is the Image.create_from_image and Image.create_from_func</div><div>interfaces, which provide the backend with all the relevant context required to</div><div>do backend-specific special handling in the backend.</div><div><br></div><div>We initially created a series of patches which first added the relevant new</div><div>interfaces for each of the 5[2] storage backends, and followed this up with a</div><div>patch which updated driver to call the new interfaces. 2 things came back from</div><div>early review feedback which made us re-examine this:</div><div><br></div><div>* The 'big bang' approach of switching all backends on in a single commit</div><div>  was tolerable, but not popular. We were asked to look into making the</div><div>  switch-over incremental.</div><div><br></div><div>* Feodor Tersin (but not tempest or the unit tests) noticed that we broke</div><div>  resize.</div><div><br></div><div>The resize problem it turns out was hard. The problem is that as well as</div><div>providing no context to the backend, the Image.cache() interface also does 'all</div><div>the things', which incidentally doesn't always involve the cache. Additionally,</div><div>it was called via _create_image(), which also does 'all the things'. So there</div><div>were 2 levels of 'all the things'. We expressly did not want to turn our create</div><div>functions into new 'all the things' methods. Because both cache() and</div><div>_create_image() both do so many things that nobody can remember why they do</div><div>them all, and which ones interact badly in which contexts, they had become</div><div>magnets for hacks, workarounds, and bugs[3]. Rather than add another layer to</div><div>_create_image(), we decided to finally pull it apart.</div><div><br></div><div><div>This resulting series achieves both of the above goals. We add a shim layer</div><div>which implements the new Image interfaces in terms of the existing cache()</div><div>interface. Specific backend implementations override the shim layer with</div><div>a backend-specific implementation. Once all backends have been updated we can</div><div>start to remove additional assumptions from driver, but the switch-over will</div><div>happen incrementally.</div><div><br></div><div>We also deconstruct both _create_image and _create_images_and_backing, and</div><div>provide helper methods for callers in driver to easily implement only the</div><div>functionality they require. This both makes it obvious to the reader what is</div><div>happening for a particular caller, and removes interactions between unexpected</div><div>behaviours (by eliminating them).</div><div><br></div><div>The change to _create_images_and_backing also makes it use common code with</div><div>regular backend disk creation. Although it wasn't an explicit goal of this</div><div>series, I believe this also fixes a problem which would have prevented live</div><div>migration between Lvm-backed instances.</div><div><br></div><div>In creating this series we have made good test coverage a high priority.</div><div>Specifically, we want to ensure that tests:</div><div><br></div><div>* Validate the behaviour we are interested in</div><div><br></div><div>* Provide assurance that the refactoring is not introducing regressions</div><div><br></div><div>For the latter reason, we have pulled any test changes we can to the front of</div><div>the series, before making any actual changes. This allows us to see that the</div><div>tests ran successfully both before and after the interface change. After the</div><div>interface change, we update the tests to test the new interfaces. As they have</div><div>more context, these are also easier to test more thoroughly.</div><div><br></div><div>The series can be viewed here:</div><div><br></div><div>  <a href="https://review.openstack.org/#/q/topic:libvirt-imagebackend">https://review.openstack.org/#/q/topic:libvirt-imagebackend</a></div><div><br></div><div>Note that the series is a single, dependent chain. Yes, you really do have to</div><div>click next to see it all, because there are 35 patches. Yes, this is a total</div><div>pita to manage.</div><div><br></div><div>The last patch in the series is currently:</div><div><br></div><div>  <a href="https://review.openstack.org/#/c/320610/">https://review.openstack.org/#/c/320610/</a></div><div><br></div><div>This is the patch which adds the Qcow2 backend, which is currently the only</div><div>backend patch we've rebased on to the new series. We picked this because it's</div><div>the default for most tests, so a green here gives us a moderately confident</div><div>feeling. The others will follow.</div></div><div><br></div><div><div>To the greatest extent practical the patches are all single purpose, which is</div><div>why there are so many. I'll highlight the 'crux' patches here, in order:</div><div><br></div><div>* libvirt: Improve mocking of imagebackend disks</div><div>  <a href="https://review.openstack.org/#/c/333242/">https://review.openstack.org/#/c/333242/</a></div><div><br></div><div>This provides a single, common way for driver tests to mock imagebackend. There</div><div>were a few existing approaches to this which had various deficiencies. This new</div><div>one replaces them all, and allows easy inspection by driver of what went on.</div><div>The series relies on this interface a lot, which is why I highlight it here.</div><div><br></div><div>* libvirt: Add create_from_image and create_from_func to Backend</div><div>  <a href="https://review.openstack.org/#/c/333244/">https://review.openstack.org/#/c/333244/</a></div><div><br></div><div>This is the requested 'shim' layer. It updates the driver to call the new</div><div>interfaces, and implements these new interfaces in terms of the old interfaces.</div><div>Note that we don't make any significant changes to existing tests in this</div><div>change, which validates that the shim layer continues to call the old</div><div>interface in the same way as before.</div><div><br></div><div>The immediately following change updates the tests to use the new interfaces.</div><div><br></div><div>* libvirt: Add DiskFromImage and DiskFromFunc</div><div>  <a href="https://review.openstack.org/#/c/333522/">https://review.openstack.org/#/c/333522/</a></div><div><br></div><div>This is the first part of the change to deconstruct _create_image(). These</div><div>interfaces allow us to return a 'creatable' disk, which has not actually been</div><div>created. The caller can then decide whether the disk should be created.</div><div><br></div><div>Over the next few changes we create some helper methods which return iterators</div><div>over creatable disks.</div><div><br></div><div>* libvirt: Don't call _create_image from finish_migration</div><div>  <a href="https://review.openstack.org/#/c/337160/">https://review.openstack.org/#/c/337160/</a></div><div><br></div><div>This removes the problematic caller of _create_image. finish_migration doesn't</div><div>create disks, so supporting it with _create_image and Image.cache() was</div><div>tortured. The new method is explicit about what needs to happen, and to which</div><div>disks.</div><div><br></div><div>* libvirt: Replace _create_images_and_backing in pre_live_migration</div><div>  <a href="https://review.openstack.org/#/c/342224/">https://review.openstack.org/#/c/342224/</a></div><div><br></div><div>This is a big change to pre_live_migration. The purpose is to ensure that disks</div><div>created for live migration are created identically to how they were originally</div><div>created.</div><div><br></div><div>* libvirt: Add create_from_image & create_from_func for Qcow2</div><div>  <a href="https://review.openstack.org/#/c/320610/">https://review.openstack.org/#/c/320610/</a></div><div><br></div><div>The native implementation of the new backend interfaces for the Qcow2 backend.</div></div><div><br></div><div><div>I'd like to apologise in advance for dropping this immediately before heading</div><div>out on holiday for a week. I will be back at work on Monday 1st August, and I</div><div>obviously won't be able to respond to review feedback before then. In my</div><div>absence, Diana Clarke may be able to respond.</div><div><br></div><div>This is a big series, though, so I'd greatly appreciate eyes on all of it, even</div><div>if some of the earlier patches receive -1s. If we could knock off even some of</div><div>the earlier test patches, the patch bombs I keep dropping on gerrit will get a</div><div>bit smaller ;)</div><div><br></div><div>Thanks,</div><div><br></div><div>Matt</div><div><br></div><div>[1] <a href="https://blueprints.launchpad.net/nova/+spec/libvirt-instance-storage">https://blueprints.launchpad.net/nova/+spec/libvirt-instance-storage</a></div><div>[2] Flat, Qcow2, Lvm, Rbd, Ploop</div><div>[3] For recent examples see stable libvirt rescue, and device tagging.</div></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><span style="font-size:12.8px">Matthew Booth</span><br></div><div>Red Hat Engineering, Virtualisation Team</div><div><br></div><div>Phone: +442070094448 (UK)</div><div><br></div></div></div></div></div>
</div>