| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-04-09 | |||
| 14:48:32 | stephenfin | So you've reverted back to 'deps = -r{toxinidir}/doc/requirements.txt' for those two sections | |
| 14:48:45 | efried | eh? | |
| 14:48:59 | efried | Even though I said envdir= ? | |
| 14:49:05 | efried | okay, I'll twiddle it again. | |
| 14:49:18 | stephenfin | yeah, envdir has no bearing on deps | |
| 14:49:23 | efried | ight | |
| 14:49:24 | mriedem | kashyap: stephenfin: gibi: i've left several comments and questions in this baselineHypervisorCPU spec https://review.openstack.org/#/c/645814/ and i must be missing something that everyone else understands about what this solves | |
| 14:49:46 | kashyap | Will look | |
| 14:49:50 | openstackgerrit | Eric Fried proposed openstack/nova-specs master: Tools & docs for backlog & abandoned spec process https://review.openstack.org/648800 | |
| 14:50:00 | stephenfin | other than if you use the same envdir with different deps, in which case you get the aforementioned rebuilding on environment each time | |
| 14:50:26 | efried | whee | |
| 14:50:38 | efried | done | |
| 14:50:42 | efried | ---^ | |
| 14:50:54 | efried | Thanks for the help stephenfin | |
| 14:53:10 | gibi | mriedem: I think https://review.openstack.org/#/c/645814/ and https://review.openstack.org/#/c/642030/ are connected | |
| 14:53:38 | gibi | mriedem: so when nova will automatically select a specific cpu_model from a list of models it will select a better suited one | |
| 14:54:51 | mriedem | the former doesn't mention anything about the latter | |
| 14:55:16 | stephenfin | efried: +2'd | |
| 14:55:24 | efried | thank you sir. | |
| 14:55:50 | gibi | mriedem: that is true | |
| 14:56:53 | gibi | mriedem: kashyap knows more about the connection | |
| 14:57:25 | kashyap | I did mention about this https://review.openstack.org/#/c/645814/ in https://review.openstack.org/#/c/645814/ | |
| 14:57:49 | mriedem | i assume you meant to link to https://review.openstack.org/#/c/642030/ | |
| 14:57:51 | mriedem | which i haven't read | |
| 14:57:54 | kashyap | mriedem: The "executive summary" is that the two libvirt APIs Nova uses today don't take account what the host KVM, QEMU and libvirt are capable of, when computing CPU models. And the proposed two new APIs will solve a class of issues. | |
| 14:58:16 | kashyap | Among other things, Nova can now ask: "Are CPU flags X and Y supported by KVM, QEMU and libvirt on the host"? | |
| 14:58:20 | kashyap | How does that help operators? | |
| 14:59:21 | kashyap | It gives them more _useful_ (and in some cases _secure_ -- e.g. CPU flags introduced to solve Meltdown/Spectre) guest CPU configuration | |
| 14:59:25 | kashyap | (Not everything is linked to migration.) | |
| 14:59:41 | kashyap | I'll respond on the review, to save some typing. | |
| 14:59:48 | dansmith | kashyap: are you talking about for scheduling or for generating more optimal xml for a guest? | |
| 15:00:51 | mriedem | it's not clear to me if the operator is doing anything here, or if operators today are doing x to workaround limitations in nova's libvirt driver using these older apis | |
| 15:01:15 | mriedem | the spec just isn't clear to me on what the end user visible benefit is | |
| 15:01:21 | mriedem | i'm sure there is one, but it's just not clear to me | |
| 15:01:30 | mriedem | 1. use new apis, 2. ?, 3. profit. | |
| 15:02:02 | kashyap | dansmith: Not scheduling. In some ways you can call it "optimal XML for a guest", but not everything is about XML generation. It is more about how Nova's libvirt driver can take into account multiple important factors (listed in the spec) when coming up with guest CPUs | |
| 15:02:02 | mordred | mriedem: I think either thing is fine from an openstacksdk POV - but I could see the shift from bool to dict being maybe a little more annoying for the more staticly typed folks like go - whereas the flat approach like it is in the notification payload would be potentially friendlier to deal with there | |
| 15:02:05 | mriedem | i should probably re-watch your summit talk that is related to this | |
| 15:02:22 | dansmith | kashyap: okay, I | |
| 15:02:29 | mordred | mriedem: so I think I'd vote, in this case, for flat, mostly because of the existence of the locked key already | |
| 15:02:33 | dansmith | have only skimmed, but.. probably need to be more clear about that | |
| 15:02:34 | kashyap | mriedem: It is not directly a clean one-to-one mapping of: "if you click on this button, elephants will dance on your screen" | |
| 15:03:03 | kashyap | mriedem: The operator doesn't have to do anything much here | |
| 15:03:41 | mriedem | is there a very obvious case of "this is an issue we have today which is a limitation due to using the older apis which would be solved by using the new" | |
| 15:03:59 | mriedem | like an actual example | |
| 15:04:05 | kashyap | Yes, let me link to the code | |
| 15:04:26 | mriedem | mordred: ok, if you can take a sec to comment on the spec that would go a long way i think | |
| 15:04:37 | mordred | sure! | |
| 15:05:12 | kashyap | mriedem: libvirt upstream added the two APIs (partly) based on the need that came out of Nova, noted here: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L3947,#L3958 | |
| 15:05:30 | kashyap | (That comment should be removed, now that the libvirt RFE I filed is fixed.) | |
| 15:05:50 | openstackgerrit | sean mooney proposed openstack/nova-specs master: Libvirt: add vPMU spec for train https://review.openstack.org/651269 | |
| 15:06:51 | mordred | mriedem: done | |
| 15:07:01 | kashyap | mriedem: A practical example is: the existing libvirt APIs can't answer this: Is this combination of 'IvyBridge' CPU model with 'pcid' and 'pdpe1gb' supported by the KVM+QEMU+libvirt on the host? | |
| 15:07:33 | sean-k-mooney | kashyap: isnt that a defct in libvirt | |
| 15:08:07 | kashyap | sean-k-mooney: Yes, it is. That's why I asked libvirt upstream to provide the two new RFEs: https://bugzilla.redhat.com/show_bug.cgi?id=1559832 | |
| 15:08:07 | openstack | bugzilla.redhat.com bug 1559832 in libvirt "[RFE] Fine-grained API to validate if a given CPU model and flags are supported by QEMU / KVM" [Unspecified,Closed: nextrelease] - Assigned to jdenemar | |
| 15:08:11 | sean-k-mooney | it could tell if pdpe1gb was supporte by looking the the host cpu flags | |
| 15:08:21 | kashyap | (To solve the problem for Nova.) | |
| 15:08:27 | mriedem | kashyap: so today operators can just populate cpu_model_extra_flags with ['pcid', 'pdpe1gb'] and we'll blindly throw those into the guest cpu config, which may or may not cause the guest to fail to launch? | |
| 15:08:48 | sean-k-mooney | kashyap: yes but this is somthing that qemu should also be able to do today but it does not provde a cli to ask to question | |
| 15:08:50 | kashyap | sean-k-mooney: I think we're hand-waving; if you see the upstream patch `diff` for libvirt that introduced the APIs, you'll see what is involved | |
| 15:08:59 | kashyap | And also please see the spec I posted. Which compares the two APIs | |
| 15:09:08 | kashyap | mriedem: Yep, exactly | |
| 15:09:39 | kashyap | sean-k-mooney: Here libvirt is taking advantage of the enormous work done in QEMU. (Look up: `query-cpu-model-expansion`) | |
| 15:09:48 | sean-k-mooney | mriedem: yes that why we originally had a whitelist of jsut pcid | |
| 15:09:59 | kashyap | sean-k-mooney: See this blog post from the QEMU's CPU modelling infra maintainer: https://habkost.net/posts/2017/03/qemu-cpu-model-probing-story.html | |
| 15:10:08 | mriedem | ok, so that would be good information for the spec, i.e. today operators have to be careful about how they configure nova-compute for the libvirt driver with regard to cpu_model and cpu_model_extra_flags because the wrong combination can lead to failures to launch a guest - with this change the libvirt driver will automatically figure that out so the operator can set it and forget it | |
| 15:10:39 | kashyap | mriedem: Yep, whatever it takes to make it clear. It's also the "curse of having too much context". I hesitate to add needless detail | |
| 15:10:46 | mriedem | mordred: thanks | |
| 15:10:50 | sean-k-mooney | i dont think we should forget it | |
| 15:11:15 | sean-k-mooney | if you enable somting in the config that is invalid i would prefer if the nova compute agent failed to start | |
| 15:11:17 | kashyap | (Yes, the whitelist was for stable branches) | |
| 15:11:32 | sean-k-mooney | kashyap: not just for stable branches | |
| 15:11:35 | mriedem | kashyap: i realize you're close to the libvirt / qemu details on this stuff, but consider your audience is not so as a nova spec reviewer i understand nova things and need to know how this translates to nova problems and how they are fixed by this spec | |
| 15:12:01 | kashyap | mriedem: I go to painful lengths to be aware of that. | |
| 15:12:06 | sean-k-mooney | kashyap: i reasied the problem of doing the validation on the reviews for you extra flags patches | |
| 15:12:30 | kashyap | mriedem: If you closely read what I write, I make a lot of effort to write with "no assumptions", and with a full-assed beginning, middle and an end :-) | |
| 15:12:53 | kashyap | sean-k-mooney: Well, I remember the change. You're forgetting something: | |
| 15:13:04 | kashyap | sean-k-mooney: Until Rocky we white-listed; after that we lifted it. | |
| 15:13:28 | sean-k-mooney | kashyap: yes and i pushed back on lifting it until we have validation of the extra flags | |
| 15:13:36 | mriedem | kashyap: the writing isn't the problem, i understand the words, they just don't have context for me | |
| 15:13:49 | mriedem | anyway if others are cool with approving this fine | |
| 15:13:51 | mriedem | i didn't -1 it | |
| 15:13:53 | kashyap | mriedem: Noted, I should have also linked the relevant Nova code, from which it came from. | |
| 15:13:53 | sean-k-mooney | i was over ruled but i personally still thing we should not have lifted the whitelist untill we have a way to validate it | |
| 15:13:59 | kashyap | mriedem: Yeah, noticed it. Appreciate the restraint :-) | |
| 15:14:15 | kashyap | mriedem: That spec (and the libvirt work) is derived directly from the problem Nova was facing, BTW | |
| 15:14:26 | kashyap | (Anyway, you see that now. I shold've made it clearer) | |
| 15:15:19 | kashyap | sean-k-mooney: I think there we disagree. Lifting the restrictions then made sense, and now it does. | |
| 15:16:17 | sean-k-mooney | we only lifted the restition because of all the specultive execution vulnerablitys and the fact we tought there would need to be a lot of extra cpus flags in the coming months | |
| 15:16:32 | kashyap | sean-k-mooney: Recall the solid reason *why* we allowed free-form: at that time we had no damn idea what else CPU flags Intel et al will introduce | |
| 15:16:54 | dansmith | sean-k-mooney: are you talking about the cpu flags thing that we added for spectre | |
| 15:16:55 | dansmith | ? | |
| 15:16:58 | kashyap | Yep | |
| 15:17:08 | dansmith | sean-k-mooney: the whitelist was *only* for stable, never intended to have that in master going forward | |
| 15:17:28 | kashyap | Exactly. I think sean-k-mooney got things mixed up. | |
| 15:17:39 | sean-k-mooney | no | |
| 15:17:49 | sean-k-mooney | that was the desicion we came to in the end | |
| 15:18:07 | dansmith | um, what? | |
| 15:18:10 | sean-k-mooney | but i did not think we should add the feature at all without validation logic | |