| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-20 | |||
| 12:22:44 | efried | aspiers: responded. | |
| 12:22:48 | aspiers | ack | |
| 12:28:57 | efried | aspiers: sane? | |
| 12:29:33 | aspiers | efried: so ditch the need to log the exact reason *why* mem encryption has been requested? | |
| 12:29:42 | rdopiera | bauzas: so about https://review.opendev.org/#/c/666394/ | |
| 12:29:43 | aspiers | I thought that was kind of useful | |
| 12:29:53 | efried | aspiers: no, I'm saying log that in the helper | |
| 12:29:57 | efried | not in the filter | |
| 12:30:00 | aspiers | oh, gotcha | |
| 12:30:15 | efried | phrased in helper-speak, not in filter-speak - see other comment | |
| 12:30:21 | aspiers | sure | |
| 12:30:30 | aspiers | so then it gets logged in the API calls too | |
| 12:30:33 | efried | yes! | |
| 12:30:39 | bauzas | rdopiera: yup, so long story there | |
| 12:30:45 | aspiers | even though that's only done for validation | |
| 12:30:49 | efried | dandy | |
| 12:30:52 | aspiers | OK | |
| 12:30:54 | efried | we're at debug, yah? | |
| 12:30:59 | aspiers | I guess so | |
| 12:31:12 | efried | If that really bothers you, add a do_log=False kwarg | |
| 12:31:18 | bauzas | rdopiera: basically, old microversions (<2.29) were forcing the destination when you were providing a target | |
| 12:31:21 | efried | but it doesn't bother me at all. | |
| 12:31:34 | aspiers | efried: nice idea, but yeah I guess it's fine anyway | |
| 12:31:39 | bauzas | rdopiera: (for live-migration and for evacuate) | |
| 12:32:10 | bauzas | rdopiera: that's why by default it's False in Nova | |
| 12:32:16 | bauzas | oops, s/False/True | |
| 12:32:31 | openstackgerrit | sean mooney proposed openstack/nova master: Revert resize: wait for events according to hybrid plug https://review.opendev.org/644881 | |
| 12:33:01 | bauzas | rdopiera: but with 2.30 microversion, we pass a 'False' value unless you have the --force option | |
| 12:34:11 | rdopiera | bauzas: what about >2.67? | |
| 12:37:25 | bauzas | rdopiera: by 2.67, we removed the --force flag | |
| 12:38:23 | bauzas | rdopiera: https://github.com/openstack/nova/commit/7aa2285e724345717a3f333adc13660d7b97dfcd | |
| 12:38:48 | bauzas | in particular https://github.com/openstack/nova/commit/7aa2285e724345717a3f333adc13660d7b97dfcd#diff-6c3b14d3964eb808ade9d3e7c5d4676d | |
| 12:39:21 | bauzas | if api_version_request.is_supported(req, min_version='2.30'): force = body["os-migrateLive"].get("force", False) force = strutils.bool_from_string(force, strict=True) | |
| 12:39:56 | bauzas | rdopiera: so Nova API defaults the force argument to False after 2.30 | |
| 12:40:22 | rdopiera | bauzas: so for >2.67 force=False always? | |
| 12:41:38 | bauzas | rdopiera: it changed a bit but https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/migrate_server.py#L104-L105 | |
| 12:42:05 | bauzas | rdopiera: which does the same indeed https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/migrate_server.py#L168-L174 | |
| 12:42:40 | bauzas | rdopiera: it's just the JSON schema which changed | |
| 12:43:01 | bauzas | after 2.67, it's no longer possible to pass a force field to the API | |
| 12:43:04 | rdopiera | so in plain words, what is your recommendation for handling that in Horizon? | |
| 12:43:15 | bauzas | so it always defaults to False | |
| 12:43:36 | rdopiera | right, so we only need a special case for 2.29? | |
| 12:43:52 | bauzas | rdopiera: well, looking at https://review.opendev.org/#/c/666394/2/openstack_dashboard/api/nova.py | |
| 12:44:12 | bauzas | rdopiera: with 2.30 and later, you don't need to pass a force flag | |
| 12:45:44 | rdopiera | so something like | |
| 12:45:44 | rdopiera | if client.api_version == api_versions.APIVersion('2.29'): | |
| 12:46:29 | rdopiera | os should we just make sure to request a microversion older than 2.29 instead? | |
| 12:46:34 | bauzas | well, you would then change the UX right? | |
| 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: 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 | |