| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-11 | |||
| 15:35:24 | efried | dansmith: mind unblocking the gate ^ | |
| 15:45:26 | openstackgerrit | Adam Spiers proposed openstack/nova master: Add extra spec parameter and image property for memory encryption https://review.opendev.org/664420 | |
| 15:45:39 | aspiers | efried: vastly improved patchset based on your feedback ^^^ | |
| 15:47:31 | openstackgerrit | Sylvain Bauza proposed openstack/nova-specs master: Proposes NUMA affinity for vGPUs https://review.opendev.org/650963 | |
| 15:48:26 | efried | aspiers: looking. | |
| 15:49:51 | stephenfin | efried, cdent: Here's what's happening | |
| 15:50:12 | stephenfin | Because of https://bugs.python.org/issue33899, we now insert a NEWLINE token at the end of all lines | |
| 15:50:26 | stephenfin | that manifests itself in pycodestyle | |
| 15:50:45 | stephenfin | which is interating through all the tokens on the test line | |
| 15:50:46 | stephenfin | https://github.com/PyCQA/pycodestyle/blob/2.0.0/pycodestyle.py#L1616 | |
| 15:51:45 | stephenfin | previously, when we got to the end of the line we were falling through to this case https://github.com/PyCQA/pycodestyle/blob/2.0.0/pycodestyle.py#L1650-L1652 | |
| 15:52:09 | stephenfin | but now, because we see this newline token, we hit this case instead https://github.com/PyCQA/pycodestyle/blob/2.0.0/pycodestyle.py#L1632-L1635 | |
| 15:52:26 | stephenfin | note that that case doesn't include a call to 'check_physical' | |
| 15:52:33 | stephenfin | so our test never gets run | |
| 15:53:06 | stephenfin | I don't know how to fix that. It feels like this is now a bug in pycodestyle | |
| 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: | |