| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-08-07 | |||
| 08:48:10 | lyarwood | but we'd still need this code to recreate the attachments even if we do remove it | |
| 08:48:23 | lyarwood | as we don't get back up to the API where the initial attachments are created | |
| 08:48:48 | mdbooth | lyarwood: Right, it's just that by getting out of whack we're opening ourselves to potential errors, and we also have to ping cinder to see if that's what we've done | |
| 08:49:06 | mdbooth | That's not a problem in your patch, but it's unfortunate | |
| 08:49:08 | lyarwood | mhen: I don't know of any reason, it's just a legacy choice that I've wanted to remove for ages but can't with existing users | |
| 08:49:51 | lyarwood | mdbooth: yeah, I guess without the attachment_id we would need to do the v2 / v3 checks again in the conductor etc | |
| 08:50:22 | mdbooth | lyarwood: Would you, though? Why wouldn't you just use v3 if conn_info and attachment_id are both unset? | |
| 08:51:07 | lyarwood | mdbooth: iirc the API has a series of compute version checks it goes through before creating the attachments | |
| 08:54:55 | mdbooth | lyarwood: Is it possible to have a reschedule *without* the original 'reservation' attachments having been deleted? | |
| 08:55:12 | mdbooth | e.g. by an early failure which doesn't cause cleanup to go through _shutdown_instance? | |
| 08:56:26 | lyarwood | mdbooth: yeah but that should be fine, the destination compute should just UPDATE the existing attachment at that point and an updated connection_info dict in return from cinder | |
| 08:58:22 | lyarwood | mdbooth: nova/virt/block_device.py -> _volume_attach | |
| 08:59:42 | gibi | dansmith: double checked the legacy allocation handling and we are lucky as the legacy codepath does the right thing for revert. Anyhow filled a bug https://bugs.launchpad.net/nova/+bug/1785776 | |
| 08:59:42 | openstack | Launchpad bug 1785776 in OpenStack Compute (nova) "resize revert still hitting the legacy allocation handling " [Undecided,New] | |
| 09:01:31 | mhen | lyarwood, I assume that the hex representation was chosen to have a predictable and limited set of string characters in order to avoid any problems related to special characters? | |
| 09:01:53 | lyarwood | mhen: yeah that could very well be the case | |
| 09:15:01 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: [placement] api-ref: add description for 1.29 https://review.openstack.org/589407 | |
| 09:18:26 | kosamara | efried: I studied your device-passthrough spec for nova-powervm. Do you plan to propose this or part of it in nova? | |
| 09:42:14 | openstackgerrit | Rajesh Tailor proposed openstack/nova master: Fix host validity check for live-migration https://review.openstack.org/401009 | |
| 09:50:28 | mdbooth | lyarwood: I still think you're missing a test, btw. https://review.openstack.org/#/c/587071/8/nova/tests/unit/conductor/test_conductor.py | |
| 09:50:50 | lyarwood | mdbooth: kk, where it exists? | |
| 09:51:02 | lyarwood | mdbooth: either way the functional tests still aren't happy so I'll respin later today | |
| 09:51:39 | mdbooth | lyarwood: yeah. I just checked the code and there's a reasonably wide window for errors which don't result in attachment deletion. | |
| 11:03:48 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Fix resize revert to use non-legacy alloc handling https://review.openstack.org/589425 | |
| 11:04:26 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Fix resize revert to use non-legacy alloc handling https://review.openstack.org/589425 | |
| 11:09:00 | openstackgerrit | Merged openstack/nova master: Grease some more tests hitting RetryDecorator https://review.openstack.org/588391 | |
| 11:09:06 | openstackgerrit | Merged openstack/nova master: Grease test_try_deallocate_network_retry_direct https://review.openstack.org/588364 | |
| 12:09:28 | openstackgerrit | Zhenyu Zheng proposed openstack/nova master: Update installation guide to be more clear about cellsv2 https://review.openstack.org/584244 | |
| 13:02:00 | mdbooth | lyarwood: FYI, I accidentally started looking at your functional test failure, btw | |
| 13:02:23 | mdbooth | lyarwood: Haven't fixed it yet, but I can continue or not as you like. | |
| 13:05:25 | lyarwood | mdbooth: I've not got back around to it yet but I assume there are multiple attachments in the self.attachments[instance_uuid] list when I've used [0] to delete things from self.volume_to_attachment right? | |
| 13:05:40 | lyarwood | mdbooth: if you already have something feel free to continue and push it up when you're done | |
| 13:05:57 | mdbooth | lyarwood: Ok, will do. | |
| 13:07:34 | mdbooth | lyarwood: What's the thinking here, btw: | |
| 13:07:37 | mdbooth | def fake_get(self_api, context, volume_id, microversion=None): | |
| 13:07:37 | mdbooth | + attachment_id = self.volume_to_attachment.get(volume_id, volume_id) | |
| 13:07:54 | mdbooth | Why would you want fake attachment_id to default to volume_id if not present? | |
| 13:09:10 | mdbooth | Ah... that's what it did before | |
| 13:09:17 | lyarwood | yarp | |
| 13:09:45 | lyarwood | hmm this was working with a previous PS with that in place | |
| 13:10:51 | mdbooth | lyarwood: I'm speculating. Issue is that we end up trying to fetch an attachment id not present. If the attachment id is a volume id ,that might be it. | |
| 13:11:08 | mdbooth | I'm going to make it unique and stash it before returning. | |
| 13:12:16 | lyarwood | mdbooth: https://review.openstack.org/#/c/587013/6..7/nova/tests/fixtures.py@1708 - I bet it's that line | |
| 13:13:13 | mdbooth | Might be in more than 1 place. This is attachment_update | |
| 13:13:32 | mdbooth | But yeah, that looks like another candidate | |
| 13:18:22 | efried | kosamara: Yes, in Stein or the T release. Is this something that interests you? | |
| 13:26:20 | kosamara | efried: yes. I'm working at CERN and I think you had a relevant discussion with Belmiro late June. | |
| 13:27:18 | efried | kosamara: I would be happy to talk through it further. I assume you're interested in doing this with libvirt, or do you have Power systems in your deployment? | |
| 13:27:45 | kosamara | efried: We would like at least a simplified version of what you're developing. With the prospect of working on it, I was looking for any relevant specs, but only came across yours recently. | |
| 13:27:59 | kosamara | efried: libvirt | |
| 13:28:13 | efried | kosamara: Are you aware of the cyborg project? | |
| 13:29:19 | kosamara | efried: yes, but it appears to be doing much more than what we need, so a solution within nova to use GPUs seems better at this point. | |
| 13:30:02 | efried | kosamara: How much have you done with the existing PCI passthrough framework? | |
| 13:30:51 | kosamara | efried: we are already using it in testing and slowly moving to production. | |
| 13:32:16 | kosamara | efried: and only for GPUs | |
| 13:32:18 | s10 | Hello. I've found, that function update_available_resource(), https://github.com/openstack/nova/blob/stable/pike/nova/compute/resource_tracker.py#L694 , driver.get_available_resource(nodename) with Libvirt driver takes 30 seconds to execute on the host with 150 instances on local storage. | |
| 13:32:18 | s10 | Is this behaviour normal or is it a regression introduced by https://github.com/openstack/nova/commit/d88b75e81eabfbd463007f6a4f27e6966a466530 and following commits? Before this commit it was 1-2 seconds for all of them. | |
| 13:32:20 | efried | kosamara: Okay. That's going to be your best bet for the near future. And it should do pretty much everything you need if all you're trying to do is pass through whole GPUs. | |
| 13:33:47 | efried | s10: You've specifically nailed it down to that commit? If you revert it, your performance goes back to normal? | |
| 13:34:41 | kosamara | efried: Whole GPUs is our main use case ATM. "that"? Our main problems with the current pci passthrough in nova are quotas and scheduling without an extra filter. So basically, implementing RPs/RCs for GPUs. | |
| 13:35:59 | s10 | Yes, if I return this function to be like before this commit, performance of this function goes back to normal. But then we will lose all fixes, introduced by this commits and following, like https://github.com/openstack/nova/commit/938c0a745325fa73d098c6d5ddd20b2a599f9624 | |
| 13:37:20 | s10 | efried: if i turn on logging for oslo_concurrency, I see, that a lot of time takes for 300 calls of "qemu-img info" for /var/lib/nova/instances/UUID/disk and disk.config | |
| 13:38:18 | s10 | efried: at least 0.052 for every call. 300 calls - 15 seconds. | |
| 13:40:38 | efried | kosamara: Not sure if we've really started making plans to implement quotas around placement artifacts yet. alex_xu, were you working on that? | |
| 13:41:49 | efried | s10: Let me take a look at what's piled on top of that commit. Trying to figure out what the effect would be of *just* reverting that one fix. | |
| 13:41:50 | s10 | efried: so for 150 instances qemu_img info is being called 600 times. 2 times for dk_size = disk_api.get_allocated_disk_size(path) and 2 times for virt_size = disk_api.get_disk_size(path). | |
| 13:42:12 | dansmith | s10: maybe talk to lyarwood about it | |
| 13:42:39 | efried | s10: Note that we have some work ongoing to cut that in half at least: https://review.openstack.org/#/c/520024/ | |
| 13:43:15 | efried | s10: But that still wouldn't make it okay as performance regressions go. | |
| 13:43:34 | sean-k-mooney | s10: we could proably cache it but is it an apricalble portion of the total boot time | |
| 13:45:20 | sean-k-mooney | there are other cases where we could chache thing in the boot process but dont because it was felt the caching introduced complexity that did not result in a significant perfomace increase when taken as a propotion of the total boot time | |
| 13:46:46 | s10 | efried: I added timer before driver.get_available_resource(nodename) and stopped it after and logged it, so only one call of this function takes 30 seconds. | |
| 13:46:56 | efried | s10: I think your best course of action right now is to open a bug and make sure lyarwood sees it. | |
| 13:47:24 | efried | s10: Do you have the ability to apply a single patch on the fly in your env and reproduce the flow? | |
| 13:47:43 | s10 | efried: yes, I have such ability | |
| 13:48:06 | efried | s10: I can try throwing out a quick revert just to confirm that it does the trick. Stand by. | |
| 13:51:40 | openstackgerrit | Eric Fried proposed openstack/nova stable/pike: DNM: Revert d88b75e https://review.openstack.org/589479 | |
| 13:51:46 | efried | s10: ^ | |
| 13:52:30 | lyarwood | s10: and this is with an images_type of raw? | |
| 13:52:32 | efried | s10: Note: a real revert would be tied to a bug and introduced in master and backported. This is just to confirm. | |
| 13:53:03 | kosamara | efried: alex_xu had proposed a spec for that. The first step is to have pci passthrough GPUs in placement of course. | |
| 13:53:09 | mriedem | yay https://review.openstack.org/#/q/If642e51a4e186833349a8e30b04224a3687f5594 | |
| 13:53:11 | s10 | efried: Before: Took 20.89 seconds to get available resources for nodename. update_available_resource /usr/lib/python2.7/dist-packages/nova/compute/resource_tracker.py:705 | |
| 13:53:29 | s10 | efried: After: Took 11.04 seconds to get available resources for nodename | |
| 13:53:45 | sean-k-mooney | kosamara: the first step to having pcie gpus in placement is having pci devices in placement | |
| 13:53:51 | s10 | If I change virt_size = disk_api.get_disk_size(path) same way, this time reduces to 2 seconds. | |
| 13:54:28 | efried | Okay s10, I'll leave you in lyarwood's capable hands at this point. | |
| 13:54:52 | s10 | lyarwood: yes, image_type is raw | |
| 13:55:08 | mriedem | it's going to be slower b/c it's exec'ing qemu-img info | |
| 13:55:10 | lyarwood | s10: unfortunatley I'm just between calls, could you write this up in a bug and I'll get back to you in ~60mins or so | |
| 13:55:32 | s10 | lyarwood: and preallocate_images=space. I will fill bug report. | |
| 13:58:08 | mriedem | dansmith: want to send https://review.openstack.org/#/q/topic:bug/1784705+(status:open+OR+status:merged)+branch:stable/queens to their maker? | |
| 13:58:55 | dansmith | mriedem: yeah | |
| 14:00:07 | maciejjozefczyk | mriedem: efried hey, about https://review.openstack.org/#/c/520024; we have it on production and it works properly; release is newton | |
| 14:00:52 | efried | maciejjozefczyk: Sweet, thanks for the info. mriedem cdent Ima +2 that sucker. Shall we backport it too? | |
| 14:01:06 | maciejjozefczyk | only one thing is that, as I remember correctly, on nova master was issue with https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L866 def_resource_change() | |
| 14:01:23 | maciejjozefczyk | efried: so could you please take me a moment to confirm that its fiexed on master, or not? | |
| 14:01:58 | cdent | efried: assume maciejjozefczyk's concerns there are okay, I think a backport would be nice but not critical? | |
| 14:02:53 | mriedem | maciejjozefczyk: you said you think https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L866 should be removed yes? | |
| 14:03:42 | maciejjozefczyk | mriedem: afailr I know it says all the time 'False'; so the update is not send to the DB | |