| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-12 | |||
| 18:15:56 | efried | but jumped right to "go look at the flavor" | |
| 18:16:09 | efried | so that seems like the path | |
| 18:16:16 | aspiers | What about https://review.opendev.org/#/c/664420/4/nova/scheduler/request_filter.py@223 | |
| 18:16:20 | sean-k-mooney | i would emulate this https://github.com/openstack/nova/blob/master/nova/virt/hardware.py#L259 | |
| 18:16:40 | aspiers | alex_xu seems to have a reasonable point there | |
| 18:16:56 | sean-k-mooney | or maybe this is a little less scary https://github.com/openstack/nova/blob/master/nova/virt/hardware.py#L171 | |
| 18:17:08 | efried | aspiers, alex_xu: How is this different from what's happening in require_tenant_aggregate ? | |
| 18:17:15 | aspiers | sean-k-mooney: BTW you know that we have gitea running on opendev now :) | |
| 18:17:40 | sean-k-mooney | aspiers: i do and the last time i used it it nova almost crashed it | |
| 18:17:43 | efried | aspiers: Commenting on the review | |
| 18:17:57 | aspiers | sean-k-mooney: interesting, gitea seems a lot more lightweight on my browser than github | |
| 18:18:04 | aspiers | efried: thanks | |
| 18:18:07 | sean-k-mooney | that was because they messed up a tempelate which is now fixed but still | |
| 18:18:53 | aspiers | efried: right, I copied the raise RequestFilterFailed() from that | |
| 18:19:39 | sean-k-mooney | anyway im feeling really tired today so im going to go lie down. o/ talk to people tomorow | |
| 18:19:40 | aspiers | sean-k-mooney: so what exception should I throw if the SEV extra spec and image property conflict with each other? | |
| 18:19:52 | sean-k-mooney | am | |
| 18:20:09 | sean-k-mooney | it depend on where you are thowing it | |
| 18:20:23 | aspiers | well, you are suggesting to put it in hardware.py? | |
| 18:20:26 | sean-k-mooney | alex asked you to add some validation in the api as well | |
| 18:20:27 | aspiers | IIUC | |
| 18:20:35 | aspiers | it needs to be in a shared library, right? | |
| 18:20:58 | sean-k-mooney | ya so nova.virt.hardware.py is called everywhere | |
| 18:20:59 | aspiers | so that it can be called from the request_filter, from the driver, and maybe also from the api | |
| 18:21:07 | sean-k-mooney | its in the virt dirver schduler and api already | |
| 18:21:43 | aspiers | I'm talking about the exception currently at https://review.opendev.org/#/c/664420/4/nova/scheduler/request_filter.py@223 | |
| 18:21:52 | aspiers | if I move it to a shared library, the exception needs to be more generic | |
| 18:22:06 | aspiers | InvalidRequest? | |
| 18:22:06 | sean-k-mooney | we have generally beein add new excpetion as needed for each check | |
| 18:22:18 | aspiers | OK | |
| 18:22:25 | sean-k-mooney | like this | |
| 18:22:27 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/virt/hardware.py#L1192-L1198 | |
| 18:23:02 | aspiers | that sounds fine to me | |
| 18:23:06 | aspiers | I can add one to exceptions.py | |
| 18:23:14 | aspiers | I mean exception.py | |
| 18:23:27 | aspiers | efried: does that work for you? | |
| 18:23:47 | openstackgerrit | Eric Fried proposed openstack/nova master: update constraints url https://review.opendev.org/664771 | |
| 18:24:40 | dansmith | efried: fwiw, my unmattering vote would be for just checking image+flavor for the sev thing and not doing the virtapi bit until we come up with a good reason | |
| 18:24:45 | dansmith | this being, not a good enough reason | |
| 18:24:54 | dansmith | in the future, when said reason exists, that's how we do it, IMHO | |
| 18:24:55 | efried | dansmith: ack, that's where we're going | |
| 18:25:00 | efried | thanks for the input sir | |
| 18:25:00 | dansmith | cool | |
| 18:25:02 | aspiers | wfm | |
| 18:25:43 | efried | aspiers: Yes, the centralized method in hardware.py should raise some kind of generic conflict exception, which should be caught and converted to RequestFilterFailed (or whatever it's called today) in the request filter. | |
| 18:26:15 | aspiers | efried: sean-k-mooney suggested making a new exception. ACK about the conversion to RFF | |
| 18:26:26 | efried | (by "generic" I mean not request filter specific; it should still have the lovely and hard-fought message you've already composed) | |
| 18:26:42 | aspiers | oh right, gotcha | |
| 18:26:46 | sean-k-mooney | aspiers: right just copy this one and change the wording https://github.com/openstack/nova/blob/master/nova/exception.py#L2054 | |
| 18:26:59 | efried | aspiers: you could use this: https://review.opendev.org/#/c/663365/2/nova/exception.py | |
| 18:27:19 | sean-k-mooney | efried: oh ya you were adding a reusable one | |
| 18:27:22 | sean-k-mooney | efried: that works too | |
| 18:27:28 | aspiers | oh perfect | |
| 18:27:45 | aspiers | Well, I might inherit from that one :) | |
| 18:27:50 | aspiers | since my message is more informative | |
| 18:28:13 | aspiers | class SEVFlavorImageConflict(FlavorImageConflict) | |
| 18:28:16 | efried | you should introduce the exception (class name) in your own patch, cause there's no telling if/when ^ will merge | |
| 18:28:24 | efried | and whoever comes second can resolve conflict. | |
| 18:28:42 | sean-k-mooney | aspiers: non of the other conflcit ones inherit for efried new excetion at teh moment | |
| 18:28:44 | efried | but yeah, make the exception name generic, and you can override the message in your method. | |
| 18:29:10 | sean-k-mooney | they inherit for Invalid or Forbidden | |
| 18:29:31 | aspiers | and NotFound | |
| 18:29:47 | efried | Those have HTTP semantic baggage, swhy I avoided them. | |
| 18:29:49 | sean-k-mooney | Forbidden when its disallows by policy and Invalid when the did not specify it correctly | |
| 18:30:11 | sean-k-mooney | e.g mem_encyption_context=yes instead or true | |
| 18:30:17 | aspiers | efried: why introduce the same new exception in two patches? can't we just land yours sooner? | |
| 18:30:26 | aspiers | is it because you can +2 my patches but not your own? ;-) | |
| 18:30:36 | efried | Your API invocation of new centralized message should convert generic exception to something that inherits from Invalid | |
| 18:31:03 | efried | aspiers: I can't +2 my patches, no, and also, there doesn't seem to be an overwhelming consensus that that patch of mine is a good thing to do at all. | |
| 18:31:15 | aspiers | efried: even if it's needed for SEV? | |
| 18:31:17 | efried | so I don't have high confidence that it will land soon/ever. | |
| 18:31:20 | efried | it's not | |
| 18:31:24 | efried | only the exception is | |
| 18:31:29 | aspiers | I'm confused | |
| 18:31:30 | efried | so it should be extracted. | |
| 18:31:34 | aspiers | ohhhhh | |
| 18:31:35 | sean-k-mooney | efried: i think i found a bug in our vgpu code today that might use your patch | |
| 18:31:47 | aspiers | I missed that that patch does other stuff | |
| 18:31:49 | aspiers | OK | |
| 18:32:08 | sean-k-mooney | e.g. without looking too close i think we dont handel stop followed by start proerly | |
| 18:32:11 | efried | yeah, the patch I pointed to has nothing to do with SEV. It has to do with "hide hypervisor ID to make NVIDIA drivers work | |
| 18:32:13 | efried | " | |
| 18:32:20 | aspiers | yeah, got it now | |
| 18:32:37 | aspiers | yikes, feels like I need another 40k spec just to remember all the plans we made in the last hour | |
| 18:32:52 | efried | You don't need to update the spec with any of this. | |
| 18:32:53 | aspiers | thank god for IRC logs | |
| 18:33:13 | aspiers | I know, was just being silly :) | |
| 18:33:32 | aspiers | it's a lot of detail for a newbie to remember, but I think I'll be ok | |
| 18:33:49 | sean-k-mooney | anyway this time im really going to go and rest. o/ | |
| 18:33:52 | aspiers | I guess my right to play the newbie card is gradually fading though | |
| 18:33:58 | aspiers | cya sean-k-mooney, thanks for your help o/ | |
| 18:33:59 | efried | - create two consolidated methods in hardware.py: a) parse/validate flavor/image for SEV yes-or-no; b) introspect host for SEV capability | |
| 18:33:59 | efried | - ^ methods raise exceptions | |
| 18:33:59 | efried | - existing usages of ^ logic cut over to using ^, converting exceptions to contextually-appropriate ones. | |
| 18:34:26 | aspiers | I think b) and callers of b) do not need to change | |
| 18:34:29 | aspiers | it's only a) | |
| 18:34:36 | efried | ight | |
| 18:34:41 | aspiers | b) is all in the driver | |
| 18:35:00 | aspiers | and available via the supports_amd_sev instance variable | |
| 18:35:21 | efried | "instance" == compute driver instance, not "instance" == "nova server" | |