| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-31 | |||
| 15:21:11 | kaisers_ | yep, i'll do that | |
| 15:24:14 | sfinucan | leakypipes: Done | |
| 15:24:23 | leakypipes | sfinucan: thanks! | |
| 15:24:37 | leakypipes | sfinucan: or however you say thanks in Irish :P | |
| 15:25:03 | sfinucan | leakypipes: Go raibh maith agat ;) | |
| 15:25:11 | leakypipes | ah, well, that too! :) | |
| 15:25:42 | sfinucan | leakypipes: This is one to stick in your review queue for today :) https://review.openstack.org/#/c/361140/ | |
| 15:26:21 | sfinucan | ...or for next weeks spec review drive | |
| 15:26:28 | cfriesen | kashyap: yep. and in our case it's made worse by the fact that we do the conversion with ionice and no output caching (necessary to minimize impact of the conversion IO load on the instances using the same disk) | |
| 15:26:39 | kashyap | cfriesen: I'm looking at this function _disk_qcow2_to_raw() | |
| 15:26:48 | kashyap | And it's not being used anywhere from my current `grep`ing | |
| 15:27:23 | kashyap | It was introduced as a helper to be used in nova/virt/libvirt -> finish_migration() | |
| 15:27:24 | cfriesen | kashyap: in our case we're hitting it when downloading the backing image file during a block live migration for a qcow2-based instance | |
| 15:28:35 | kashyap | cfriesen: Do you have a full test case? | |
| 15:29:32 | cfriesen | kashyap: boot instance with qcow2 storage, live-migrate it to a compute node that doesn't have the image cached yet. | |
| 15:29:56 | kashyap | cfriesen: I _think_ I know why we do the conversion: | |
| 15:30:06 | mriedem | dansmith: weird the logs are not found for https://review.openstack.org/#/c/452006/ | |
| 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 | |