| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-02 | |||
| 18:37:55 | Sundar | sean-k-mooney: The tempest test waits till the server becomes active: https://review.opendev.org/#/c/667231/11/cyborg_tempest_plugin/tests/scenario/manager.py@155 | |
| 18:39:08 | sean-k-mooney | oh you are using the common tempest function ok | |
| 18:39:08 | dansmith | I don't see that the cyborg instance ever made it to the compute node | |
| 18:40:06 | Sundar | dansmith: The tempest CI test in cyborg-tempest-plugin is called test_server_basic_ops '' https://review.opendev.org/#/c/667231/11/cyborg_tempest_plugin/tests/scenario/test_accelerator_basic_ops.py@45 | |
| 18:40:24 | dansmith | Sundar: right, which makes it hard to distinguish from the other test_server_basic_ops | |
| 18:40:41 | dansmith | I don't see it actually running though: https://8664d62e69400bd89796-9cb1d5e035819d8d5535734f80756cd4.ssl.cf1.rackcdn.com/670999/20/check/tempest-integrated-compute/cdfc846/testr_results.html.gz | |
| 18:40:48 | sean-k-mooney | Sundar: that is an existing class name in tempest | |
| 18:41:00 | dansmith | oh wait, that's the wrong log, ignore me | |
| 18:41:46 | sean-k-mooney | although the module path should be different | |
| 18:41:55 | Sundar | If you just want to mention 'accel' somewhere in the name, that's not a problem. Will do. | |
| 18:42:06 | dansmith | sean-k-mooney: right but the test name is what drives the tempest tenant name I think | |
| 18:42:13 | sean-k-mooney | https://2371b0492dbe3a0c56c0-5469ab4f5c2741453cb8b95135b2c449.ssl.cf2.rackcdn.com/670999/20/check/cyborg-tempest/2124885/testr_results.html.gz | |
| 18:42:19 | sean-k-mooney | that is the result you were looking for | |
| 18:42:28 | dansmith | but they're hardcoding the server name, which helps | |
| 18:42:29 | dansmith | yeah I know | |
| 18:42:58 | dansmith | so for this fake test, we don't actually provide any device to libvirt to attach to the guest right: | |
| 18:43:00 | dansmith | ? | |
| 18:43:17 | Sundar | That's right | |
| 18:43:21 | sean-k-mooney | correct | |
| 18:43:36 | sean-k-mooney | the arq has type fake and its ignored | |
| 18:43:43 | Sundar | The path till creating/binding.getting ARQs is the same as for a real driver | |
| 18:43:49 | dansmith | will there be a real job running on intel ci or something that can actually do this? | |
| 18:44:03 | dansmith | Sundar: sure, it just doesn't cover actually producing a device and getting libvirt to attach it | |
| 18:44:09 | Sundar | dansmith: Yes, we are planning a 3rd party CI for Intel FPGAs | |
| 18:44:22 | sean-k-mooney | we could maybe use some other virtual device for testing in a more robust way too | |
| 18:45:14 | dansmith | sean-k-mooney: yeah I was going to ask if we could pass some harmless pci device through, but wasn't sure if we can do that in the gate without nested | |
| 18:45:19 | Sundar | sean-k-mooney: We have plans to enhance the fake driver to simulate programming -- it will just respons to the programming API within Cyborg with a success/failure | |
| 18:45:35 | sean-k-mooney | dansmith: i was thinking of creating a file on disk and passing it to the guest | |
| 18:45:45 | dansmith | Sundar: yeah, that's good, I just want to see the actual "here's the pci device" stuff tested | |
| 18:45:48 | Sundar | That would cover more paths, including Glance interaction within Cyborg. But it would still not attach a PCI device to the VM | |
| 18:46:02 | dansmith | sean-k-mooney: how does a file on disk simulate a pci device/ | |
| 18:46:42 | dansmith | okay anyway, this log looks good for what we have now and can test now, so that's cool | |
| 18:46:57 | dansmith | I'll want to look at this one Sundar retools the current patches per the pending feedback | |
| 18:47:05 | sean-k-mooney | it does not but it will simulate genarting xml. i was really thinking of a lvm driver for local caching so we could have cyborg provide local storage to a vm. | |
| 18:47:20 | dansmith | Sundar: FWIW, I have this week and next week before I turn to a pumpkin for the rest of the year | |
| 18:47:38 | sean-k-mooney | e.g. have a driver that could be useful in producion but has no hardware depency | |
| 18:47:43 | Sundar | dansmith: I am going to squash https://review.opendev.org/631245 into https://review.opendev.org/631244, for the reasons I mentioned above | |
| 18:47:46 | dansmith | sean-k-mooney: is that a thing? I thought we were focused on pci devices for accelerators right now | |
| 18:48:27 | sean-k-mooney | we are but form a livirt driver point of there is very littel difference between generating the xml for a pci device vs a block device | |
| 18:48:32 | dansmith | Sundar: okay I skimmed over that.. IMHO, keeping the new event definition in its own patch makes sense just because of how much api stuff has to change | |
| 18:49:08 | dansmith | sean-k-mooney: seems fairly different to me.. I understand it's just a chunk of xml, but it's not the same kind of xml we really care about | |
| 18:49:11 | Sundar | dansmith: Agreed. The event definition patch https://review.opendev.org/692707 will stay as is. | |
| 18:49:36 | sean-k-mooney | if we just want a way to create a device (lvm volume) program it (dd image to volume) and validate it in the guset it was a way to do it. | |
| 18:49:41 | dansmith | sean-k-mooney: it's something, it's just not enough | |
| 18:49:52 | dansmith | Sundar: oh sorry I was looking at the wrong two tabs, lemme re-read | |
| 18:50:24 | openstackgerrit | Lee Yarwood proposed openstack/nova master: libvirt: Request and store the decrypted path of an attached encrypted volume https://review.opendev.org/696693 | |
| 18:50:40 | sean-k-mooney | ya i was also looking at ways to fake pci devices in linux and there are some ways but its not trivial | |
| 18:50:47 | Sundar | sean-k-mooney: The code paths within the virt driver are quite different for block devices and PCI devices. So, how does this help? | |
| 18:51:03 | dansmith | Sundar: 631234 all but goes away after you address my feedback | |
| 18:51:22 | dansmith | Sundar: so assuming you mean just bringing forward the cyborg client method, then sure | |
| 18:51:46 | dansmith | Sundar: it would be attaching a block device as an accelerator, so no typical nova block device handling for it | |
| 18:51:50 | sean-k-mooney | its a more complete end to end test that is all. also im not sure how https://review.opendev.org/#/c/689070/ is going to progress and its another way to enable local caching | |
| 18:52:16 | dansmith | sean-k-mooney: I don't think cyborg for block devices is a good way to address the local caching thing | |
| 18:52:24 | dansmith | sean-k-mooney: how is that not going to confuse people? | |
| 18:52:54 | sean-k-mooney | well given samsung are now supporting sriov on there new ssds it might be but sure ill let you get back to it | |
| 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 | |