| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-20 | |||
| 12:47:03 | rdopiera | I think that was the idea behind the reported issue in the first place | |
| 12:47:13 | bauzas | remember, before 2.30, it wasn't possible to not force a destination | |
| 12:48:03 | bauzas | it was either possible to either pass a destination and then force to it without checking the scheduler *or* not pass a destination and then the scheduler would be giving you some target | |
| 12:48:06 | bauzas | rdopiera: ^ | |
| 12:48:15 | rdopiera | and after 2.67 it's impossible to force it | |
| 12:48:22 | bauzas | correct | |
| 12:48:33 | rdopiera | so there is going to be a change at some point either way | |
| 12:48:44 | bauzas | so, I'd recommend you to not ask for a target with 2.29 and older | |
| 12:49:14 | bauzas | and for 2.30, accept a target by defaulting to not force to it | |
| 12:49:39 | bauzas | that actually depends on what kind of user experience you'd like to provide | |
| 12:50:15 | bauzas | the force flag was just a way to keep the same behaviour for some time | |
| 12:51:30 | rdopiera | personally I don't feel like I have enough knowledge about Nova to decide on user experience, I would rather ask for recommendation from people who designed it | |
| 12:52:53 | rdopiera | you know what, how about I just abandon that patch and close the bug with "current version" explaining that later versions of the api behave as expected? | |
| 13:23:21 | mriedem | o/ | |
| 13:23:30 | sean-k-mooney | o/ | |
| 13:33:10 | bauzas | rdopiera: whoops, missed your question | |
| 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: Pass extra_specs to flavor in vif tests https://review.opendev.org/662556 | |
| 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:14 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add |
|
| 13:56:14 | openstackgerrit | Adam Spiers proposed openstack/nova master: Extract SEV-specific bits on host detection https://review.opendev.org/636334 | |
| 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:15 | openstackgerrit | Adam Spiers proposed openstack/nova master: Allow guest devices to include |
|
| 13:56:16 | openstackgerrit | Adam Spiers proposed openstack/nova master: Enable memory locking if SEV is requested https://review.opendev.org/662558 | |
| 13:56:16 | openstackgerrit | Adam Spiers proposed openstack/nova master: Use |
|
| 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: Use |
|
| 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: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 | 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:11 | jangutter | stephenfin: VNIC_TYPE can in theory be set by the user, where VIF type is behind the scenes. | |
| 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. | |