| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-12 | |||
| 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" | |
| 18:35:38 | aspiers | sure :) | |
| 18:36:07 | aspiers | and then there's Instance instances too :) | |
| 18:36:35 | aspiers | OK, gonna take a break or probably stop for today | |
| 18:36:44 | aspiers | thanks a lot all | |
| 18:37:46 | efried | dustinc: Not rechecking the ironicclient part of the sdk series for yesterday's doubled_words snafu - pep8 needs resolving on the lowermost one. | |
| 18:39:44 | dustinc | efried: saw that, once I am done with the spec update I am working on I will take care of it - thanks | |
| 18:40:10 | efried | dustinc: no hurry, just letting you know why it's not going to be included in my "recheck everything" sweep :P | |
| 18:40:39 | efried | dustinc: also lmk if at some point you want to talk through providers.yaml | |
| 18:42:16 | dustinc | efried: providers.yaml: will do..I def. want to at some point | |
| 19:10:13 | openstackgerrit | Eric Fried proposed openstack/nova master: Make RequestContext(instance_lock_checked) fail https://review.opendev.org/665003 | |
| 19:10:25 | efried | mriedem: resolves a TODO of yours ^ | |
| 19:11:38 | openstackgerrit | Merged openstack/nova master: Remove unnecessary setUp methods https://review.opendev.org/663179 | |
| 19:12:15 | mriedem | efried: does it or does it not have to do with SEV | |
| 19:13:37 | efried | does not | |
| 19:14:02 | efried | I was looking into removing a hack for glance using get_endpoint_data through _ContextAuthPlugin (fail) and just happened to notice this. | |
| 19:14:19 | efried | mriedem: what was that bug you opened for the doubled_words hacking test yesterday? Did you mark it as a dup of https://bugs.launchpad.net/nova/+bug/1804062 already? | |
| 19:14:21 | openstack | Launchpad bug 1804062 in OpenStack Compute (nova) "test_hacking fails for python 3.6.7 and newer" [High,In progress] - Assigned to Stephen Finucane (stephenfinucane) | |
| 19:15:24 | mriedem | yes | |
| 19:15:33 | mriedem | https://bugs.launchpad.net/nova/+bug/1832392 | |