| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-02 | |||
| 18:53:18 | dansmith | well, I guess we can have that discussion, but I definitely don't want to merge that with this effort at the moment | |
| 18:53:34 | dansmith | and since PCI is different enough, I still want to see PCI working for this | |
| 18:53:41 | sean-k-mooney | oh sure. its totally seperate | |
| 18:53:48 | sean-k-mooney | ya | |
| 18:53:57 | sean-k-mooney | i just have not figured out how to fake it yet | |
| 18:54:23 | sean-k-mooney | the kernel netdevsim module was the close i came up with in the past | |
| 18:54:25 | dansmith | yeah, well, as long as we'll have intel ci so we can see it working, that's something | |
| 18:54:30 | Sundar | dansmith: 631234 is a typo, right? Quoting myself from above: I think we need to squash https://review.opendev.org/631245 into the previous patch https://review.opendev.org/631244. Because once the get_resolved_arqs() moves to previous patch, the only things left behind are the virt driver changes. It would arguably be easier to understand those | |
| 18:54:31 | Sundar | changes together with the changes to get the arqs in compute/manager.py. | |
| 18:54:55 | dansmith | Sundar: I meant 631245 yes, sorry | |
| 18:55:01 | sean-k-mooney | ya. the fake driver should allow us to test the api flow. the intel ci should validate it works with real hardware | |
| 18:55:09 | Sundar | Cool, np | |
| 18:55:24 | dansmith | Sundar: the virt changes will be insulated from the cyborg client method is my point | |
| 18:56:08 | dansmith | Sundar: if I were you I would have one patch that adds the support into the manager, all the build_resources changes to capture the ARQs and pass them to the virt driver, | |
| 18:56:17 | Sundar | dansmith: Sure, will try to get back ASAP with the changes | |
| 18:56:17 | dansmith | Sundar: then a separate patch to implement things for libvirt | |
| 18:56:33 | Sundar | Oh, separate patch for libvirt? | |
| 18:56:40 | dansmith | yes definitely | |
| 18:56:47 | Sundar | That would be 63124 today | |
| 18:56:58 | Sundar | *631245 | |
| 18:57:21 | dansmith | Sundar: so when hyperv people come along, we point them at the libvirt patch and say "implement this part for your driver and you will be good"... separate from the plumbing in compute to wire everything up | |
| 18:58:04 | Sundar | I see, fine with that too | |
| 18:58:06 | dansmith | Sundar: in the compute manager patch, you just assume that any virt driver below you will ignore the ARQs you pass to it, which will be true of libvirt, until the next patch in the series where that becomes untrue for libvirt, but remains true for all the others | |
| 18:59:11 | openstack | bug 1830747 in OpenStack Compute (nova) pike "Error 500 trying to migrate an instance after wrong request_spec" [High,In progress] https://launchpad.net/bugs/1830747 - Assigned to Matt Riedemann (mriedem) | |
| 18:59:11 | openstackgerrit | Merged openstack/nova stable/pike: Add regression recreate test for bug 1830747 https://review.opendev.org/663124 | |
| 18:59:15 | openstackgerrit | Merged openstack/nova stable/pike: Workaround missing RequestSpec.instance_group.uuid https://review.opendev.org/663125 | |
| 18:59:27 | Sundar | dansmith: In the compute manager patch, the signature for libvirt driver's spawn() will have tio change to include the arqs as a parameter. | |
| 18:59:35 | dansmith | Sundar: does this method go away if you pass the stuff from compute manager to virt? https://review.opendev.org/#/c/631245/39/nova/accelerator/cyborg.py | |
| 18:59:49 | dansmith | Sundar: that would leave the virt patch you have as just the libvirt config bits | |
| 19:00:47 | dansmith | Sundar: correct, spawn() will need to take the ARQs, just like it takes network and block device info | |
| 19:01:51 | dansmith | Sundar: https://github.com/openstack/nova/blob/56d3cd7aa7f4d3be01fd2a5c10903fb548c49458/nova/compute/manager.py#L2491-L2499 | |
| 19:02:08 | dansmith | Sundar: just like block and network, you should pull out arqs from resources and pass to spawn | |
| 19:02:26 | dansmith | arguably we could just be passing in resources to spawn to avoid splitting it out, but let's not go down that hole in this set | |
| 19:03:06 | dansmith | Sundar: and since I will have a strong opinion on that later, let me go ahead and say, it should be called: | |
| 19:03:17 | dansmith | accel_info = resources['accel_info'] | |
| 19:03:20 | dansmith | :) | |
| 19:03:28 | Sundar | dansmith: That method would remain, albeit it would move to the compute manager patch. So that the waiting logic can call it once to decide whether to quit early. The virt driver patch 631245 will slim down to just the libvirt driver changes. The change in the signature for virt driver base class and the libvirt driver (to include arqs as a | |
| 19:03:29 | Sundar | parameter) will now be in the compute manager patch. | |
| 19:03:52 | dansmith | Sundar: ah right, forgot that would be the check, so yes, move it thusly | |
| 19:04:01 | Sundar | dansmith: What you said about resources['accel...'] is exactly what I said in the gerrit review :) | |
| 19:04:06 | dansmith | ack | |
| 19:04:35 | dansmith | Sundar: except not the same names | |
| 19:04:46 | dansmith | Sundar: I was giving you a freebie pre -1 on the *name* :) | |
| 19:05:02 | Sundar | What name? | |
| 19:05:13 | dansmith | accel_info instead of accelerator_requests | |
| 19:05:35 | Sundar | I see. To match network_info etc. | |
| 19:05:38 | dansmith | yes | |
| 19:05:49 | Sundar | Ok, I'm not too hung up on names | |
| 19:05:49 | dansmith | because I have issues. my therapist and I are working on them. | |
| 19:05:54 | dansmith | good :) | |
| 19:08:23 | Sundar | sean-k-mooney: still intrigued by your idea for a fake device, block or PCI. Do you have a way to simulate a PCI device? It can be *quite* difficult AFAICS with linux PCI subsystem. | |
| 19:08:56 | dansmith | so I was thinking about something else, like grab the last usb host and pass that through | |
| 19:09:28 | dansmith | Sundar: I would focus on getting the patches back into order, and let sean-k-mooney pull something out of his butt for faking the PCI stuff... :) | |
| 19:10:25 | Sundar | Yup, that's what I plan to do :) but a 'real fake driver' (heck, I'll grab the award for today's oxymoron) would be very useful for the future. | |
| 19:10:26 | dansmith | he pulled off some fancy stuff for the numa live migration stuff, so we should see if he can do it again here :) | |
| 19:10:31 | sean-k-mooney | Sundar: the closest i have come up with is the netdevsim kernel module wich does create sriov capable fake pci device for testing and developing the kernel ebpf code | |
| 19:10:50 | sean-k-mooney | Sundar: how ever i dont think you can actully pass those device to a qemu instace. i have not tried however | |
| 19:11:08 | sean-k-mooney | i did try to use it for sriov testing in the past but i could not get it to work | |
| 19:11:56 | sean-k-mooney | i was able to create the fake device but i could not assing the vfs the pf might have worked but we dont really want to have to compile kernel module in the gate if we can avoid it | |
| 19:12:20 | Sundar | sean-k-mooney: It would be useful to have a libvirt patch where the domain XML for hostdev can have an attribute that indicates 'do not pass this to qemu'. | |
| 19:12:35 | Sundar | That would be useful outside of OpenStack too, I would guess. | |
| 19:12:37 | dansmith | sean-k-mooney: that's why i was thinking pick something on the host we could pass through, like one of those USB hosts | |
| 19:12:56 | sean-k-mooney | dansmith: maybe ya | |
| 19:12:56 | dansmith | Sundar: can't imagine the libvirt folks would go for that | |
| 19:14:11 | sean-k-mooney | there was talk and patchs for adding the abilty to emulate sriov device in qemu | |
| 19:14:27 | sean-k-mooney | if that ever gets merged upstream we might be able to use that | |
| 19:14:42 | sean-k-mooney | but for now it think focusing on getting the patches in order is better | |
| 19:15:06 | sean-k-mooney | if i come up with a way to fake sriov/pci deivces ill let ye know | |
| 19:15:24 | Sundar | We're all agreed on that :). Time for me to get back to work. | |
| 19:16:13 | sean-k-mooney | one thing i have been meaning to test for a while is if i can use a neutron port as a pci device for a guest | |
| 19:16:38 | sean-k-mooney | they dont support vfs put i might be able to do a PF passthough | |
| 19:18:13 | sean-k-mooney | im pretty sure we are missing a few things to make that work however espacally in the upstream ci. | |
| 19:38:52 | openstackgerrit | Eric Fried proposed openstack/nova master: Use Placement 1.34 (string suffixes & mappings) https://review.opendev.org/696418 | |
| 19:38:53 | openstackgerrit | Eric Fried proposed openstack/nova master: WIP: Tie requester_id to RequestGroup suffix https://review.opendev.org/696946 | |
| 19:40:02 | efried | gibi: 696418 ^ now just does the 1.34 cutover, will continue with the rest in subsequent patches. | |
| 19:51:42 | openstackgerrit | Merged openstack/nova master: Process requested_resources in ResourceRequest init https://review.opendev.org/696354 | |
| 20:04:18 | efried | [2] https://github.com/openstack/nova/blob/f1382651dc3b7b945b69a2af0bc05aa472f26b28/nova/objects/request_spec.py#L1071 | |
| 20:04:18 | efried | [1] https://github.com/openstack/nova/blob/f1382651dc3b7b945b69a2af0bc05aa472f26b28/nova/objects/request_spec.py#L1061 | |
| 20:04:18 | efried | dansmith: is nullable=True (e.g. [1]) made moot by having a default and using obj_set_defaults() in __init__ (e.g. [2])? | |
| 20:04:28 | dansmith | no | |
| 20:04:40 | dansmith | efried: nullable=True *only* means "can be set to None" | |
| 20:04:55 | efried | ah fusk, will I *ever* get that right? | |
| 20:05:04 | dansmith | probably not | |
| 20:05:05 | efried | I thought it meant "can be absent" (in terms of `in`) | |
| 20:05:16 | dansmith | no, | |
| 20:05:23 | dansmith | all fields can be absent all the time | |
| 20:05:29 | dansmith | also unrelated to defaults | |
| 20:05:49 | efried | dansmith: oh; so does obj_set_defaults() mean I don't have to do the `in` check? | |
| 20:05:55 | dansmith | no | |
| 20:06:14 | dansmith | if you haven't set defaults on the object, then 'in' will fail if the field is unset | |
| 20:06:29 | dansmith | you're connecting three things which are unrelated | |
| 20:06:44 | dansmith | __contains__ means nothing more than "is it set right now" | |
| 20:06:51 | dansmith | nullable means "can be set to none" | |
| 20:07:05 | dansmith | and default= means "if they call obj_set_defaults(), set this field to this value" | |
| 20:07:21 | efried | right, so forget nullable; I'm really just confirming whether gibi is correct here https://review.opendev.org/#/c/696418/1/nova/scheduler/utils.py@327 | |
| 20:07:45 | dansmith | technically he is not right | |
| 20:07:52 | dansmith | practically, he is. | |
| 20:07:53 | efried | because the obj could be loaded from cold storage | |
| 20:07:57 | dansmith | right | |
| 20:07:59 | efried | where it was unset | |