| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-31 | |||
| 15:30:09 | mriedem | on the failed jobs | |
| 15:30:12 | mriedem | anyway i left some comments | |
| 15:30:15 | kashyap | There's an epic comment from mdbooth in the function finish_migration() | |
| 15:30:27 | kashyap | cfriesen: It's to avoid CVEs from format probing | |
| 15:30:28 | mriedem | dansmith: i'm be wary of using the IntegratedHelperMixin | |
| 15:30:44 | dansmith | mriedem: known issue apparently | |
| 15:30:45 | leakypipes | sfinucan: yeah, will try to get to that once I complete a review of jianghuaw's VGPU stuff. | |
| 15:30:48 | mriedem | and I'd use USES_DB_SELF = True with the NoDbTestCase i think so you can setup the db yourself | |
| 15:30:53 | dansmith | mriedem: I was copying the server groups test | |
| 15:30:56 | mriedem | avoiding default cells or something | |
| 15:31:24 | kashyap | cfriesen: Hmm, but wait -- format probing is not the only rationale, because to avoid that, the solution is to explicitly specify the format of the backing file | |
| 15:31:38 | leakypipes | sfinucan: plus, I'm technically snikitin_'s boss so if I don't get to that review today, he can bitch at me all he wants to over the weekend or Monday ;P | |
| 15:31:41 | mriedem | dansmith: fwiw this is how i've been doing them lately https://github.com/openstack/nova/blob/master/nova/tests/functional/regressions/test_bug_1671648.py | |
| 15:31:44 | mriedem | completely self-contained | |
| 15:31:51 | mdbooth | kashyap: IIRC that code is only applicable when the source and destination hosts have different storage configurations | |
| 15:31:53 | kashyap | cfriesen: Can you reproduce it with Git Master? | |
| 15:32:01 | sfinucan | leakypipes: :D | |
| 15:32:02 | mdbooth | As you say, it stops a security bug in that case | |
| 15:32:13 | mdbooth | We really shouldn't be doing it, thoug | |
| 15:32:14 | dansmith | mriedem: ack | |
| 15:32:14 | mdbooth | h | |
| 15:32:20 | cfriesen | kashyap: I think that's different. in our case the conversion is done under pre_live_migration(), wheich eventually calls down to libvirt.utils.fetch_image(), which calls fetch_to_raw() | |
| 15:32:32 | cfriesen | kashyap: let me check master | |
| 15:32:36 | kashyap | mdbooth: But cfriesen seems to imply that we unconditionally convert any raw to qcow2 -- do we do that at all? | |
| 15:32:38 | mdbooth | cfriesen: What's the issue? | |
| 15:32:53 | kashyap | cfriesen: Oh right, fetch_to_raw(), it's from utils.py | |
| 15:32:54 | mdbooth | cfriesen: Are you getting different backing file formats on source and destination? | |
| 15:33:01 | kashyap | mdbooth: See the bug in the scrollback | |
| 15:33:08 | kashyap | mdbooth: https://bugs.launchpad.net/nova/+bug/1484992 | |
| 15:33:08 | openstack | Launchpad bug 1484992 in OpenStack Compute (nova) "Converting image from qcow2 to raw is slow for images with large virtual size" [Wishlist,Confirmed] | |
| 15:33:16 | kashyap | Chris is hitting the above | |
| 15:33:41 | cfriesen | mdbooth: no, we're hitting a performance issue when converting a downloaded qcow2 image to raw for use as a backing file, as part of pre_live_migration() | |
| 15:33:49 | mdbooth | cfriesen: Ah | |
| 15:33:56 | mdbooth | So, you don't actually have to convert those | |
| 15:34:01 | mdbooth | That's a config option | |
| 15:34:19 | mdbooth | However, it's an ongoing issue that changing storage config after deployment is a minefield | |
| 15:34:21 | kashyap | Ah, right, I thought I saw one | |
| 15:35:10 | cfriesen | mdbooth: force_raw_images ? | |
| 15:35:13 | kashyap | 'force_raw_images = True' | |
| 15:35:19 | mdbooth | cfriesen: Sounds about right, yeah | |
| 15:35:28 | kashyap | Yep, that seems to be it - (BoolOpt) Force backing images to raw format | |
| 15:35:38 | mdbooth | cfriesen: You're using LVM? | |
| 15:35:49 | cfriesen | mdbooth: no, qcow2 in this case. | |
| 15:35:56 | cfriesen | mdbooth: lvm in other cases though | |
| 15:36:12 | mdbooth | Ok, because I didn't think live migration worked on lvm... does it? | |
| 15:36:26 | mdbooth | Or is that only cold migration? | |
| 15:36:44 | cfriesen | mdbooth: cold works in our environment, we patched it in. :) | |
| 15:36:50 | mdbooth | Hehe, ok | |
| 15:38:16 | mdbooth | cfriesen: Yeah, if you want to avoid the conversion step, force_raw_images = False is what you want | |
| 15:38:28 | mdbooth | It should probably be the default, tbh | |
| 15:38:43 | openstackgerrit | Sivasathurappan Radhakrishnan proposed openstack/nova master: Return 400 when compute host is not found https://review.openstack.org/447355 | |
| 15:38:43 | openstackgerrit | Sivasathurappan Radhakrishnan proposed openstack/nova master: Reset instance status on invalid host https://review.openstack.org/401009 | |
| 15:39:06 | mriedem | lyarwood: ah i realize now we don't have any unit test coverage on detach or driver_detach in nova.virt.block_device now | |
| 15:39:17 | mriedem | you had the TODOs in test_compute_mgr | |
| 15:39:18 | kashyap | mdbooth: There should surely be some rationale documented as to why it was chosen to be 'True' by default? | |
| 15:39:27 | mdbooth | surely | |
| 15:39:31 | cfriesen | mdbooth: do you know if that has any impact on guest performance? I guess it'd be another level of indirection. | |
| 15:40:10 | mdbooth | cfriesen: You'd have to measure it. My guess would be that it wouldn't be significant, though, over what you've already got. | |
| 15:40:22 | kashyap | cfriesen: If it's a qcow2, "it depends", isn't it -- whether you've fully `fallocate`d it (at that point, it is equivalent to raw), or you've only preallocated the qcow2 metadata, etc | |
| 15:42:22 | cfriesen | mdbooth: hashyap: looks like it was defaulted to False back in "essex", because that was the then-current behaviour. Was added by Vishvananda Ishaya. | |
| 15:43:06 | mdbooth | kashyap: All allocation delay would be on the top layer, regardless of the backing file format. My guess is that for read-only a qcow2 backing file would be very low overhead. | |
| 15:43:39 | lyarwood | mriedem: right, I can follow up with that early next week and move things across I just didn't want the original change to grow out of control tbh | |
| 15:43:45 | kashyap | mdbooth: You are correct. Because the backing file, by definition is read-only | |
| 15:44:00 | kashyap | (Although upstream QEMU seems to be mulling over 'writable' backing files at one point.) | |
| 15:44:37 | mdbooth | kashyap: Multi-writer qcow2 would be awesome, btw | |
| 15:45:37 | mdbooth | qemu has a copy-on-read feature, don't know if you've seen that. You can use it to cache a remote disk image locally as it's read from a remote source. | |
| 15:45:57 | mdbooth | So, e.g. you can boot using the curl backend directly from an iso hosted on a remote webserver | |
| 15:46:08 | kashyap | mdbooth: Yes, I've seen COR | |
| 15:46:17 | mdbooth | You create a local qcow2 with COR and a remote backing file | |
| 15:46:30 | mdbooth | And you can boot it immediately, without waiting for it to finish downloading at all | |
| 15:47:04 | mdbooth | This would be fantastic for this case. Problem is that because the backing file is in the image cache, it has multiple writers | |
| 15:47:08 | mdbooth | And we can't support that :( | |
| 15:47:32 | kashyap | mdbooth: Oh right, probably you should write up an RFE bug on LP before this goes into the void of scrollback | |
| 15:47:46 | cfriesen | mdbooth: kashyap: thanks guys, I've got options to try out now. :) | |
| 15:48:43 | mdbooth | cfriesen: Sadly, I don't think this change will cause existing downloads to break | |
| 15:48:57 | mdbooth | I say sadly, because we do format inspection on backing files :( | |
| 15:49:20 | dansmith | mriedem: you use cast-as-call in your test, for a reason I assume.. do you know why I'm getting NotFound and need that poll loop? | |
| 15:49:32 | kashyap | mdbooth: BTW, do you really mean two users trying to simultaneously to a qcow2 file? | |
| 15:49:37 | mdbooth | I don't know of any current security issues relating to this, but it's still a danger | |
| 15:49:49 | kashyap | s/users/processes/ | |
| 15:50:27 | mdbooth | kashyap: So, you create a qcow2 file with a remote backing file and COR, lets call this cache.qcow | |
| 15:50:42 | mdbooth | kashyap: You then create instance_a.qcow2 which has cache.qcow2 as a backing file | |
| 15:51:03 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Address comments when moving volume detach to block_device.py https://review.openstack.org/452252 | |
| 15:51:04 | mriedem | lyarwood: ^ | |
| 15:51:17 | mriedem | dansmith: i used cast as call because that's what the test did that i copied from :) | |
| 15:51:23 | dansmith | heh, yeah okay | |
| 15:51:25 | mriedem | dansmith: as for the 404 while it's building... | |
| 15:51:26 | mdbooth | When you read a block from instance_a.qcow2, it'll be looked up in cache.qcow2, miss, be downloaded, and stored in cache.qcow before being returned to instance_a.qcow2 | |
| 15:51:55 | mdbooth | So basically, the qemu process writing to instance_a.qcow2 is also writing to cache.qcow2 | |
| 15:51:58 | mriedem | dansmith: could be hitting some window between when the build request and instance exist for the get() method in the compute API? | |
| 15:52:06 | mriedem | dansmith: i thought we closed that in ocata thoguh | |
| 15:52:07 | mriedem | *though | |
| 15:52:08 | mdbooth | Now simultaneously do the same thing with instance_b.qcow2 | |
| 15:52:17 | lyarwood | mriedem: ack thanks, LGTM | |
| 15:52:24 | dansmith | mriedem: well, that's my point.. we shouldn | |
| 15:52:26 | dansmith | mriedem: shouldn't have that problem | |
| 15:52:38 | mdbooth | You've now got 2 separate qemu processes writing to cache.qcow2 without coordination | |
| 15:52:46 | mdbooth | I don't believe this will end well | |
| 15:52:47 | kashyap | mdbooth: Hmm. It's not security issues (about multiple processes trying to write to a qcow2 file live), but image being corrupted -- as we speak, there are patches using POSIX / Linux file-locking to prevent that at QEMU-level | |
| 15:53:02 | mdbooth | kashyap: The security issue is image format detection | |