| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-20 | |||
| 13:33:38 | efried | kashyap: "Book will arrive tomorrow" --Renae (aka wife aka personal shopper) | |
| 13:33:41 | bauzas | rdopiera: I think yeah it's a reasonable approach, as I wouldn't recommend to change an expected behaviour | |
| 13:34:04 | kashyap | efried: Ow, you trusted me. Thank you :-) | |
| 13:34:12 | bauzas | rdopiera: but this is kind of a tribal knowledge, so again it really depends on what the horizon community would like to see | |
| 13:34:31 | bauzas | and when I see community, I obviously mean devs *and* ops | |
| 13:34:38 | bauzas | (or rather users) | |
| 13:45:16 | mriedem | alex_xu: i replied to you here https://review.opendev.org/#/c/610727/ - i'm not sure if i misunderstood your comment | |
| 13:49:03 | mriedem | sean-k-mooney: is https://review.opendev.org/#/c/644881/ ready for re-review? i saw you went from PS33 to PS37 :) | |
| 13:51:02 | sean-k-mooney | it should be. i had to fix the unit tests and then i had to put it to grammerly to fix my spelling in the commit message | |
| 13:52:37 | sean-k-mooney | the changes are not really that big but i think i have adressed everything. ill respin if there are anything else you would like changed. | |
| 13:53:03 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add extra spec parameter and image property for memory encryption https://review.opendev.org/664420 | |
| 13:53:11 | aspiers | efried: ^^^ should address all your feedback | |
| 13:53:19 | efried | ack | |
| 13:56:07 | aspiers | rebasing ... | |
| 13:56:13 | openstackgerrit | Adam Spiers proposed openstack/nova master: Use fake flavor instead of empty dict in test https://review.opendev.org/662555 | |
| 13:56:13 | openstackgerrit | Adam Spiers proposed openstack/nova master: Pass extra_specs to flavor in vif tests https://review.opendev.org/662556 | |
| 13:56:14 | openstackgerrit | Adam Spiers proposed openstack/nova master: Extract SEV-specific bits on host detection https://review.opendev.org/636334 | |
| 13:56:14 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add |
|
| 13:56:15 | openstackgerrit | Adam Spiers proposed openstack/nova master: Allow guest devices to include |
|
| 13:56:15 | openstackgerrit | Adam Spiers proposed openstack/nova master: Detect that SEV is required and enable iommu for devices https://review.opendev.org/644565 | |
| 13:56:16 | openstackgerrit | Adam Spiers proposed openstack/nova master: Use |
|
| 13:56:16 | openstackgerrit | Adam Spiers proposed openstack/nova master: Enable memory locking if SEV is requested https://review.opendev.org/662558 | |
| 14:01:19 | aspiers | efried: the way bbobrov added the code to ensure virtio devices have IOMMU enabled when SEV is requested, it was inserted within each chunk of device config creation. I'm wondering if it might be better to have a single method which is called after all device config is created if SEV is required, and iterates over each device, enabling IOMMU where required? | |
| 14:02:17 | aspiers | efried: similar to how the tests do it: https://review.opendev.org/#/c/644565/9/nova/tests/unit/virt/libvirt/test_driver.py@2609 | |
| 14:02:57 | efried | aspiers: I don't really understand how libvirt works. Kind of relying on folks like kashyap to validate those top patches. | |
| 14:03:02 | aspiers | maybe not, since other code might want to insist on (say) disabling IOMMU for a specific type of device | |
| 14:03:36 | efried | aspiers: IMO if the flavor/image requested encryption and some other piece of code thinks that shouldn't happen, that should be an error. | |
| 14:03:39 | kashyap | On a long call; back in a few, folks | |
| 14:03:54 | aspiers | maybe better to keep each device's IOMMU decision local to the code which creates its config | |
| 14:04:11 | efried | We should never turn off requested encryption based on something discovered internally "later on". | |
| 14:04:18 | aspiers | efried: sure, that's not quite the scenario I mean though | |
| 14:04:22 | efried | okay | |
| 14:04:44 | aspiers | let's say some other extra spec requires a particular storage device to be virtio but have IOMMU disabled | |
| 14:04:54 | aspiers | and there's also an extra spec or image property requiring memory encryption | |
| 14:05:32 | aspiers | these are mutually incompatible since SEV requires IOMMU on all virtio devices | |
| 14:05:36 | aspiers | now ideally, this would be caught earlier on than at instance creation time | |
| 14:05:52 | aspiers | but as a last resort, the chunk of code building the XML for that device should spot the conflict | |
| 14:06:33 | aspiers | whereas if I followed my previous suggestion of moving the "turn IOMMU on for all virtio devices" code to a separate SEV method, it couldn't | |
| 14:07:25 | aspiers | so in summary, maybe the general approach in places like https://review.opendev.org/#/c/644565/9/nova/virt/libvirt/driver.py@1626 is fine | |
| 14:07:35 | aspiers | well | |
| 14:07:42 | aspiers | fine, except for your valid point about explicit turn-off | |
| 14:07:46 | aspiers | which I am still replying to | |
| 14:18:38 | openstackgerrit | Adam Spiers proposed openstack/nova master: Detect that SEV is required and enable iommu for devices https://review.opendev.org/644565 | |
| 14:18:38 | openstackgerrit | Adam Spiers proposed openstack/nova master: Use |
|
| 14:18:39 | openstackgerrit | Adam Spiers proposed openstack/nova master: Enable memory locking if SEV is requested https://review.opendev.org/662558 | |
| 14:22:56 | aspiers | efried: regarding the need for some kind of master switch: I don't think I can float the MEM_ENCRYPTION_CONTEXT patch to the top - at very least, that's the one which bumps os-resource-classes to 0.4.0 to provide the new class | |
| 14:23:43 | aspiers | efried: if I combined the top three patches, that would be the master switch really, since none of the others change the way guests are created | |
| 14:27:55 | openstackgerrit | Merged openstack/nova master: Log disk transfer stats in live migration monitor. https://review.opendev.org/619395 | |
| 14:33:04 | mriedem | melwitt: replied inline on https://review.opendev.org/#/c/633227/ | |
| 14:33:07 | mriedem | thanks for thinking through that | |
| 14:39:59 | kashyap | aspiers: Back here for 15 min. So on the approach to enable IOMMU for virtio devices 'if-SEV-is-requested': I need a fresher mind than the pear-shaped one I have now, afraid. Not to mention a chunky block of time. Let's work it out tomm? | |
| 14:40:18 | aspiers | kashyap: sure but it's simpler than you think | |
| 14:40:40 | aspiers | e.g. https://review.opendev.org/#/c/644565/11/nova/virt/libvirt/driver.py@1626 | |
| 14:40:56 | aspiers | the only question I'm asking is whether this is the right place to do it | |
| 14:40:57 | kashyap | aspiers: The "single method" thing does make sense to me | |
| 14:41:19 | aspiers | except the single method approach has the issue described above | |
| 14:41:31 | aspiers | (35 mins ago) | |
| 14:41:47 | kashyap | Hm, "maybe not, since other code might want to insist on (say) disabling IOMMU for a specific type of device" | |
| 14:42:43 | kashyap | aspiers: Did you audit what other code will insist disabling IOMMU? Let me quickly check with the RHT virt folks | |
| 14:43:02 | aspiers | kashyap: no, that was a potentially hypothetical future scenario | |
| 14:43:04 | kashyap | As in, for potential gotchas here | |
| 14:44:36 | kashyap | aspiers: So SEV requires IOMMU unconditionally on _all_ virtio devices? | |
| 14:46:12 | aspiers | kashyap: yes, http://specs.openstack.org/openstack/nova-specs/specs/train/approved/amd-sev-libvirt-support.html#proposed-change | |
| 14:46:43 | kashyap | aspiers: So, I looked at the "enable IOMMU at the time of each device configuration" code -- where the check is: *if* bus is 'virtio', then enable IOMMU... | |
| 14:53:13 | efried | aspiers: My problem with "none of the others change the way guests are created" is that as of patch #3: the inventory is being exposed, the flavor/image syntax is being accepted, and the allocation is being created. The only thing that's *not* happening is... the actual feature. That's bad. | |
| 14:53:42 | aspiers | efried: OK. Totally open to suggestions for how to fix that | |
| 14:53:44 | efried | aspiers: You can put the os-traits bump in whatever patch first needs it. That's fine. | |
| 14:53:58 | stephenfin | jangutter, sean-k-mooney, ralonsoh: I've royally confused myself again (every damn time) - either of you care to remind me of the difference between VIF_TYPE_foo and VNIC_TYPE_foo? | |
| 14:54:24 | efried | aspiers: I'm saying that exposing the inventory is a nice master switch, because without it, there's zero expectation of anything else working. | |
| 14:54:26 | aspiers | efried: o-r-c not o-t, but yeah sure, I can split the o-r-c patch in two | |
| 14:54:35 | stephenfin | I think one refers to the backend that handles the VIF, while the other determines how it's actually attached to the instance. Right? | |
| 14:54:44 | aspiers | efried: wfm | |
| 14:55:11 | jangutter | stephenfin: VNIC_TYPE can in theory be set by the user, where VIF type is behind the scenes. | |
| 14:55:11 | efried | aspiers: As far as I'm concerned, we can *always* bump ot and orc to latest. (In other projects we've implemented canary tests that will fail as soon as a new release comes through upper-constraints.) | |
| 14:55:25 | aspiers | efried: yup, makes sense | |
| 14:55:43 | efried | aspiers: So for my money, you could do that in its own patch on the bottom, and not even have to tag it against this blueprint. | |
| 14:55:53 | jangutter | stephenfin: VNIC is an _attribute_ of the VIF. | |
| 14:56:43 | Sundar | sean-k-mooney, mriedem, gibi, johnthetubaguy: There has been further discussion on Line 276 of https://review.opendev.org/#/c/603955/14. That seems to be the sole point to address before this spec is merged. Please take a look. Thanks. | |
| 14:57:09 | stephenfin | jangutter: So in theory any VIF type could use one of the...six VNIC types we provide? | |
| 14:57:45 | jangutter | stephenfin: there are two checks I think. Neutron has to allow it, and Nova has to translate it right from Neutron. | |
| 14:58:23 | stephenfin | Yeah, in theory but not in practice | |
| 14:58:35 | jangutter | stephenfin: there's no easy way to answer the question: "does this cloud support this VNIC type" without trying to spawn an instance. | |
| 14:58:56 | openstackgerrit | Matt Riedemann proposed openstack/os-traits master: Add COMPUTE_SAME_HOST_COLD_MIGRATE trait https://review.opendev.org/666604 | |
| 14:59:32 | jangutter | stephenfin: your first NACK might be neutron saying "this combination of driver + vnic type is not valid". But it could go right up to Nova failing to find a free PCI device conforming to the NUMA topology. | |
| 15:00:06 | stephenfin | How did I ever get dragged into this stuff? :D | |
| 15:00:09 | kashyap | aspiers: I need to get back to this, afraid. Saddled with another meeting | |
| 15:00:29 | aspiers | kashyap: np | |
| 15:00:43 | stephenfin | jangutter: That does clarify some things | |
| 15:00:47 | stephenfin | ...though | |
| 15:00:49 | jangutter | stephenfin: in them olden days, I think there was a field that you could set that could select e1000 or realtek or something. | |
| 15:01:05 | jangutter | stephenfin: confusingly that was called vif type. | |
| 15:01:14 | kashyap | aspiers: But I _did_ ask the question to libvirt folks if there's any other way than injecting the "IOMMU on" at device config time | |
| 15:01:20 | kashyap | [If the device is 'virtio'] | |
| 15:01:20 | aspiers | kashyap: thanks for that | |
| 15:01:20 | gibi | Sundar: do you mean L376 ? | |
| 15:01:23 | kashyap | I'll keep you posted | |
| 15:02:14 | aspiers | kashyap: I don't expect there to be. I was more wondering whether it should be done device by device, or near the end of instance config time, after all devices have already been constructed | |
| 15:02:15 | Sundar | gibi: Yes, sorry for the typo | |
| 15:02:26 | aspiers | kashyap: but this is just a matter of how to construct the guest XML | |
| 15:02:29 | gibi | Sundar: OK, reading the discussion | |