| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-11-26 | |||
| 17:08:18 | mriedem | and would just fail later, e.g. attaching sriov ports (not supported), rebuilding volume-backed instances with a new image (not supported) | |
| 17:08:56 | gmann | mriedem: yes but here we are changing the behavior. user does not permissions to GET project and project exist is right request and nvoa is 200 currently which will be 400. | |
| 17:09:40 | mriedem | gmann: the "user" in this case is an admin | |
| 17:09:46 | gmann | yes | |
| 17:10:10 | dansmith | tssurya: -1.9 | |
| 17:10:11 | mriedem | i'm agreeing that re-raising the 403 from keystone would be a behavior change | |
| 17:10:17 | tssurya | I would prefer not to have a microversion to fix this, but I don't know if its "okay" to do it | |
| 17:10:24 | mriedem | but i'm not agreeing it requires a microversion | |
| 17:10:37 | mriedem | we didn't add the project id validation in a new microversion either, because it was stopping a leaky bug | |
| 17:10:54 | tssurya | dansmith: bad idea ? | |
| 17:11:02 | mriedem | iow, i shouldn't have to request a microversion to tell nova to properly do validation | |
| 17:11:05 | dansmith | tssurya: can't get past the typo in the commit message ;P | |
| 17:11:15 | dansmith | tssurya: I added two people that should look at it | |
| 17:11:19 | tssurya | dansmith: haha :P | |
| 17:11:23 | mriedem | she said "lightly review" | |
| 17:11:24 | tssurya | thanks :) | |
| 17:11:31 | dansmith | mriedem: true story | |
| 17:11:53 | gmann | mriedem: but we cannot say that this nova API is only for users having GET project permission ? | |
| 17:12:04 | tssurya | mriedem, dansmith: by "light" I mean the idea of removing them, those parts of code are grey areas for me | |
| 17:12:30 | mriedem | i usually ask dan to lightly review my stuff when i want him to just rubberstamp it :) | |
| 17:12:32 | dansmith | tssurya: I'm sure you're right that it needs to happen.. I'm not super strong in that area of the code as well | |
| 17:12:40 | gmann | i mean we cannot say in current code behavior or doc | |
| 17:13:10 | tssurya | dansmith: ack, I'll wait for the experts you tagged to respond | |
| 17:13:12 | mriedem | gmann: i'm lost in what you're asking and what you're proposing | |
| 17:13:16 | tssurya | mriedem:hehe | |
| 17:13:37 | mriedem | gmann: because earlier you said "(10:52:19 AM) gmann: tssurya: mriedem i think we should only proceed the request for 2xx case and rest all return 400 with error message of what we get from keystone" | |
| 17:13:38 | gmann | option 1 (telling via document for right permissions for this API) options 2 do the same via code. I like later one but that need microversion change IMO | |
| 17:13:54 | gmann | yeah but IMO it need microverison change | |
| 17:14:00 | mriedem | and i disagree | |
| 17:14:18 | gmann | on microverison only right ? | |
| 17:14:32 | mriedem | 1. request at 2.1 -> blast off my foot, ouch....reads docs.... 2. oh request at 2.97 - 403 (it works!) | |
| 17:15:26 | mriedem | i am a disagreeable troll on most everything but on this point yes just the microversion thing | |
| 17:17:11 | sean-k-mooney | it might be worth updating the "when do you need a micorverions bump" doc to note its not required if you are blocking knonw broken behavior using an error code that could previously be retruned form teh api | |
| 17:17:15 | gmann | it break the success cases from 200->400. because GET project permission is not actually required for this API only things is how nova check the project exist or not? | |
| 17:18:13 | gmann | sean-k-mooney: i think that is covered, broken behavior is ok to fix without microversion. | |
| 17:18:40 | tssurya | gmann: so you are fine to go ahead without microversion ? | |
| 17:18:53 | mriedem | maybe y'all duke it out in the mailing list and let others weigh in | |
| 17:19:00 | gmann | but this is little tricky where it is mix of success and broken case because keystone 403 does not mean project does not exist always | |
| 17:20:17 | gmann | tssurya: no, i am saying it need microversion if we change the keystone's 403 case to failure case. | |
| 17:20:37 | tssurya | I'll post it on ML and we can see if there are people who care about this | |
| 17:20:47 | gmann | or we request keystone with no auth way ? do not know how | |
| 17:20:53 | gmann | +1 | |
| 17:34:08 | openstackgerrit | Merged openstack/nova master: compute: Take an instance.uuid lock when rebooting https://review.opendev.org/673463 | |
| 18:14:42 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/train: compute: Take an instance.uuid lock when rebooting https://review.opendev.org/696151 | |
| 18:15:52 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: compute: Take an instance.uuid lock when rebooting https://review.opendev.org/696152 | |
| 18:16:18 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/rocky: compute: Take an instance.uuid lock when rebooting https://review.opendev.org/696153 | |
| 18:16:47 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/queens: compute: Take an instance.uuid lock when rebooting https://review.opendev.org/696154 | |
| 18:35:51 | efried | dansmith: I'm confused by some of the new wording, which might just be me, but pretty sure the test has a mistake https://review.opendev.org/#/c/695985/ | |
| 18:37:36 | dansmith | efried: ah, frack, that was supposed to get dedented | |
| 18:37:43 | efried | :) | |
| 18:37:54 | efried | dansmith: but please add the self.fail() as well | |
| 18:38:18 | dansmith | efried: you mean a self.fail after the early exit to make sure we don't get there right? | |
| 18:38:23 | efried | correct | |
| 18:38:26 | dansmith | sure | |
| 18:38:32 | efried | self.fail('this code should have been unreachable!') | |
| 18:39:12 | dansmith | self.fail('I could totally take eric in a a street fight') | |
| 18:39:20 | dansmith | we'll never run that code, so we'll never know | |
| 18:40:31 | efried | hahaha | |
| 18:41:24 | openstackgerrit | Dan Smith proposed openstack/nova master: Add a way to exit early from a wait_for_instance_event() https://review.opendev.org/695985 | |
| 18:51:08 | gmann | johnthetubaguy: these test change is due to base class assert on error message. these tests will be changed only where granularity is added which should not be many or we can remove the error message checks . - https://review.opendev.org/#/c/648480/20/nova/tests/unit/policies/test_services.py@63 | |
| 18:54:44 | openstackgerrit | Merged openstack/nova stable/train: Replace time.sleep(10) with service forced_down in tests https://review.opendev.org/696088 | |
| 19:05:42 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Add new default rules and mapping in policy base class https://review.opendev.org/645452 | |
| 19:05:58 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Suppress policy deprecated warnings in tests https://review.opendev.org/676670 | |
| 19:07:49 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Add new default roles in os-services API policies https://review.opendev.org/648480 | |
| 19:08:02 | efried | Can I get you to look at the vTPM spec again please? | |
| 19:08:02 | efried | dansmith: +2, thanks. | |
| 19:08:15 | efried | https://review.opendev.org/#/c/686804/ | |
| 19:08:41 | dansmith | no | |
| 19:09:57 | efried | mriedem: if you have a minute, would you please take a look at https://review.opendev.org/695985 (lets us fix races to wait_for_instance_event()) | |
| 19:12:33 | dansmith | efried: mriedem: just left a comment about _how_ we'll use this with cyborg to close that race: https://review.opendev.org/#/c/631244/46/nova/compute/manager.py@2627 | |
| 19:14:32 | efried | dansmith: ++ makes perfect sense. Do you have a strong opinion about whether the bind should be kicked off from conductor instead of compute at this point? | |
| 19:14:53 | dansmith | efried: I think it'd be better if we did it from conductor for all the reasons I originally thought that | |
| 19:15:35 | dansmith | so unless there's some new revelation about why that's bad, I have the same opinion | |
| 19:16:01 | efried | I think it might have had something to do with how easy/hard it is to clean up. But I can't think why it would matter. | |
| 19:16:15 | efried | otherwise it's just a question of having to redo the code moar. | |
| 19:16:49 | dansmith | well, the major benefit is the overlap of work getting done to build the accelerator before we get down to the last minute | |
| 19:17:13 | sean-k-mooney | the only issue i was aware of was it would require use to pass the arq info as part of the spawn call to the compute | |
| 19:17:20 | dansmith | if you do it from conductor, you're letting cyborg know about things before you take the trip over rabbit to the compute, wait in the parallel build queue, set up volumes, networking, etc, etc | |
| 19:17:21 | sean-k-mooney | but we may already need to do that | |
| 19:17:49 | dansmith | the current code is not passing it, AFAICT, and building all of that into the compute, | |
| 19:17:55 | dansmith | which is another good reason not to do it on the compute | |
| 19:18:12 | dansmith | the more the compute implies about the request from the state of the system, the harder it is to effect an upgrade that changes those implications later | |
| 19:18:19 | sean-k-mooney | right but i think they choose to build it on the compute initally to avoid the rpc chagne to pass it | |
| 19:18:29 | sean-k-mooney | i prefer doing it in the conductor too by the way | |
| 19:18:39 | dansmith | right now, all the computes are examining the flavor directly and doing what they think they need to | |
| 19:18:39 | sean-k-mooney | i just think that is why the current code does not | |
| 19:19:16 | dansmith | if we change something about how that works, conductor gets upgraded first and can change the behavior, just passing the arq to the compute, which just needs to wait and hook it up | |
| 19:39:12 | mriedem | efried: the bind from compute/conductor thing was in the ML awhile back, probably worth dredging that up if we're changing our minds now | |
| 19:40:12 | mriedem | from what i remember, i think i also pushed for conductor but then after lots of wah wah it's easier from compute without a bunch of rpc interface changes i relented | |
| 19:40:26 | dansmith | mriedem: we had originally said it would be from conductor, then the spec changed | |
| 19:40:46 | dansmith | mriedem: the spec seemed to call out the eventing as the primary reason why it had to happen from compute | |
| 19:40:57 | dansmith | which is both not correct, and also laughably terrible in the implementation currently proposed | |
| 19:41:00 | mriedem | i don't remember the details, i just know it came up in the ML | |
| 19:41:22 | sean-k-mooney | it also came up in the denver and maybe dubling ptg | |
| 19:41:28 | sean-k-mooney | its been a while | |
| 19:41:55 | sean-k-mooney | i think long term we all agreed teh conductor was better. | |
| 19:42:00 | mriedem | ok, let me rephrase: the last time i remember there being thoughtful and documented takes on that was in the ML | |
| 19:42:11 | mriedem | rather than some undocumented stuff from dubling | |
| 19:42:20 | sean-k-mooney | i think at some point there was a request to avoid the rpc chagne but i dont really rememebr | |
| 19:42:33 | mriedem | "i don't really remember" is why i refer back to the ML | |