Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-06-11
15:54:08 stephenfin I note that the latest version of pycodestyle does pretty much the same thing too https://github.com/PyCQA/pycodestyle/blob/2.5.0/pycodestyle.py#L2104-L2107
15:54:33 cdent stephenfin: sorry stephenfin, I thought most of what you said above was already understood: and it being a bug on pycodestyle was known
15:54:49 cdent or rather: pycodestyle's continued behavior is the root
15:54:56 stephenfin Um, whut? :)
15:55:02 stephenfin Clearly I didn't read far enough
15:55:37 cdent Or maybe I just reached that conclusion based on the discussion on https://bugs.python.org/issue33899 which if I recall right suggests the same thing
15:56:37 stephenfin Yup, now that I search for pycodestyle I see the line of course. Dang it
15:56:47 stephenfin Their fix is rather odd though https://github.com/PyCQA/pycodestyle/pull/801/files
15:58:06 openstackgerrit Adam Spiers proposed openstack/nova master: Pass extra_specs to flavor in vif tests https://review.opendev.org/662556
16:01:45 aspiers efried: What's supposed to happen when hw:mem_encryption=True but trait:HW_CPU_X86_AMD_SEV=required is missing?
16:02:05 efried aspiers: How could that happen?
16:02:12 efried oh
16:02:18 aspiers efried: Should the filter automatically add it for now, but then later error if we add support for MKTME?
16:02:32 efried No, we should never add the trait.
16:02:40 stephenfin efried: If you apply this to '.tox/py27/lib/python2.7/site-packages/pycodestyle.py' it fixes things, yeah? http://paste.openstack.org/show/752752/
16:03:01 aspiers OK, so then we need to error if it's missing, which somewhat reduces the user-friendliness benefit of hw:mem_encryption
16:03:03 efried aspiers: You're assured of landing on capable hosts because they're the only ones that can satisfy the MEM_ENCRYPTION_CONTEXT inventory.
16:03:05 efried No
16:04:05 efried if I don't care which, I leave the trait out and land on either one.
16:04:05 efried if I care that it's MKTME, I specify the MKTME trait and land on an Intel host
16:04:05 efried if I care that it's SEV, I specify the SEV trait and land on an AMD host
16:04:05 efried if the image/flavor prop is specified, it means I want mem encryption
16:04:21 aspiers Sure, but I'm talking about the last case
16:04:35 aspiers In that case, there needs to be logic *somewhere* in the code which says "OK, the user wants encrypted memory, and the only way to achieve that currently is SEV"
16:04:47 aspiers so that it triggers usage of all the SEV bits
16:04:52 efried I never land on a non-capable host, because only capable hosts have MEM_ENCRYPTION_CONTEXT inventory.
16:04:52 efried No.
16:05:13 efried Today the only hosts exposing that inventory will be SEV-capable hosts, so that falls into place.
16:05:15 aspiers Sure
16:05:22 aspiers but where does that logic go? ^^^
16:05:40 efried It's implicit. It's free. It doesn't "go" anywhere.
16:05:45 aspiers It's not enough to land on the right host
16:05:55 aspiers nova needs to know to add all the extra bits to the guest config
16:06:11 efried extra bits
16:06:21 aspiers yes, like <launchSecurity /> and <locked />
16:06:28 efried always the same if I'm SEV, right?
16:06:28 aspiers and a whole bunch of iommu stuff
16:07:13 aspiers Not sure I understand the question
16:07:28 aspiers That extra config varies depending on a whole bunch of things
16:07:45 efried okay, but the virt driver knows how to twiddle the guest config based on a) mem_encryption flavor/image prop, and b) fact I'm on an AMD host
16:08:21 efried If there are user-/admin-configurable options beyond "yes" or "no", we should talk about where those should go.
16:08:33 efried But my understanding was that from the admin/user pov it was "yes" or "no".
16:08:39 aspiers It is
16:08:52 efried and if there are any variations, it's discovered based on specifics of the system
16:08:53 efried okay
16:08:59 efried so the virt driver knows b)
16:09:04 efried that's how it knows it's doing SEV
16:09:20 aspiers It sounds like you are saying that the logic should go in the virt driver and it should be "if the current context has MEM_ENCRYPTION_CONTEXT=1 and the current host is SEV, then add the SEV bits"
16:09:50 efried okay, yes
16:10:06 aspiers OK, thanks - think I know how to write it now
16:10:51 efried I suppose if you're being careful, you should raise a nice exception if you get MEM_ENCRYPTION_CONTEXT without being SEV-capable (regardless of AMD-ness)
16:11:17 aspiers Yes can do. BTW SEV is AMD-only
16:11:29 aspiers Very much a vendor-specific thing
16:12:00 aspiers You probably knew that and I'm just misinterpreting the point of your last clause in parentheses
16:14:51 efried Yes, what I'm saying is, theoretically there should have been no way you got this far if the host wasn't SEV-capable, but, I don't know, if someone switched off the capability between allocation and spawn, you should fail hard.
16:15:37 aspiers Yep, absolutely
16:15:50 aspiers It was the "(regardless of AMD-ness)" bit I didn't quite get
16:15:57 efried stephenfin: fixes it py27
16:16:07 stephenfin sweet
16:16:18 stephenfin I'll submit that as a fix to pycodestyle
16:17:59 efried stephenfin: fixes in py36 as well
16:18:13 efried stephenfin: why is self.lines[-1] correct?
16:19:38 stephenfin It's not, actually, now that you mention it
16:21:11 stephenfin Or maybe it is. Needs more tests
16:21:34 aspiers efried: bbobrov is on vacation so I'm taking over his SEV patches, and thinking about rebasing them all into a single linear commit series (even though this won't accurately reflect the dependencies between them) - does that make sense?
16:22:03 efried aspiers: Yes it does. I was kind of wondering why they weren't already in one line.
16:23:04 aspiers efried: Well, I gave a few reasons for that in our previous discussions on this, but an additional reason is weak coordination between me and him ... :)
16:23:18 efried ack
16:23:38 aspiers One downside is that if an earlier patch fails CI, it bottlenecks merging of anything higher up
16:24:11 aspiers but if it makes life easier for reviewers then that probably overrules any other considerations
16:24:48 efried It's not just that. It's that ultimately you're going to need a patch that ties everything together. And as previously discussed, you can't have a dep tree that's upside down.
16:24:56 aspiers Sure
16:25:03 aspiers Another downside is that it will significantly increase the number of rebases
16:25:10 aspiers but again I can live with those
16:25:10 efried So yes, the fact that some patches *could* be merged independently if they were separated - it's just something that we live with.
16:25:33 aspiers Definitely a nice future enhancement for Gerrit
16:25:39 aspiers One can dream ...
16:26:29 aspiers I guess I'll rebase all his stuff on top of mine, since it's the next phase
16:26:57 aspiers My stuff is all about feature detection and scheduling; his is about guest config and booting
16:27:17 aspiers Then the series will roughly match the order of work items
16:27:31 aspiers as listed http://specs.openstack.org/openstack/nova-specs/specs/train/approved/amd-sev-libvirt-support.html#work-items
16:29:46 efried So your check in the virt driver should just reuse the same "am I SEV capable" method that it used to expose the MEM_ENCRYPTION_CONTEXT inventory and the HW_CPU_X86_AMD_SEV trait in the first place. Which is why there should be no way for that check to fail - unless something changed in between update_provider_tree and spawn.
16:29:46 efried I just meant "being on AMD isn't enough; other stars must align for SEV to be supported"
16:29:46 efried > It was the "(regardless of AMD-ness)" bit I didn't quite get
16:29:46 efried aspiers:
16:31:06 efried and yes, I agree with your ordering, especially since your patches are already 2/3 with one +2 and the last one very close to same, but the other ones are still awaiting your nod before I even dig into them.
16:37:27 aspiers efried: OK thanks, we're 100% aligned :)
16:40:26 aspiers efried: Oh, one extra detail regarding the above - yes the check can just look at the supports_amd_sev instance variable, but it also needs to check extra_specs for resources:MEM_ENCRYPTION_CONTEXT=1 to know that encrypted memory was requested, right?
16:40:43 aspiers If *both* are true then it should add all the SEV guest config tweaks
16:42:10 efried s/check extra_specs/check the allocation/ but otherwise, ja, genau
16:44:11 openstackgerrit Lee Yarwood proposed openstack/nova master: libvirt: flatten rbd images when unshelving an instance https://review.opendev.org/457886
16:49:06 aspiers efried: OK thanks, I guess that will become self-evident when I look into how to check allocations
16:49:51 efried aspiers: Well, I would rather you check the allocations rather than duplicating the logic you just put in your request filter to check and validate the flavor & image
16:50:12 efried The allocations are passed to spawn. They're just a dict. See the placement API reference for the format.
16:50:26 aspiers Cool, thanks!
16:50:54 aspiers I'm beginning to feel that this is nearing code-completion and maybe can be added to the runway queue soon
16:51:00 efried yup
16:51:15 aspiers I do want to test the whole series on real SEV hardware first though
16:51:48 aspiers (which I have available to me right now)
16:58:05 efried dustinc: sdk release 0.31.0 is in u-c now, so I'd like to respin the series to bump that and fix up that other thing we talked about yesterday. Let me know when you're clear of local edits.

Earlier   Later