| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-04-02 | |||
| 19:55:07 | edleafe | fried_bunny: that's how it's supposed to work | |
| 19:55:16 | fried_bunny | jaypipes: Because you *have* to do that when the consumer doesn't exist yet. | |
| 19:56:04 | fried_bunny | Cause we had the discussion about opacity - made sure *that* was in the spec. So you can't send 0. But if you do a GET, you'll get 404 cause the consumer doesn't exist yet. | |
| 19:56:12 | fried_bunny | so you don't have an existing generation to populate your payload with. | |
| 19:56:18 | edleafe | fried_bunny: you will have gotten the generation from GETting existing allocations | |
| 19:56:20 | openstackgerrit | Dan Smith proposed openstack/nova master: Fix cancel_all_events event name parsing https://review.openstack.org/558059 | |
| 19:56:28 | fried_bunny | edleafe: Not when there ain't none yet. | |
| 19:56:28 | edleafe | if there is no consumer, you would get None | |
| 19:56:39 | fried_bunny | you would get 404 | |
| 19:56:43 | fried_bunny | not so? | |
| 19:56:50 | jaypipes | fried_bunny: but the only reason to have this consumer generation at all was for the PUT /allocations/{consumer_uuid} when you wanted to modify an existing allocation. | |
| 19:56:58 | edleafe | you would get an empty list | |
| 19:57:10 | fried_bunny | jaypipes: But how do you know whether you're modifying an existing or creating a new? | |
| 19:57:44 | jaypipes | fried_bunny: when you're trying to modify an existing allocation, it's a totally different call point than when you're creating a new allocation for a new instance. | |
| 19:58:20 | fried_bunny | jaypipes: Maybe for Nova it is. | |
| 19:58:20 | jaypipes | fried_bunny: the scheduler always does claim_resources() which is new consumer (new instance). the conductor will be the only thing modifying existing allocations. | |
| 19:58:24 | edleafe | either way, you're calling set_allocations, no? | |
| 19:58:30 | fried_bunny | ^ | |
| 19:58:41 | jaypipes | edleafe: claim_resources() vs. some new modify_allocations() call. | |
| 19:59:17 | jaypipes | the only reason we need consumer generation is when we're modifying existing allocations. | |
| 19:59:23 | jaypipes | there's no need for it anywhere else. | |
| 19:59:27 | fried_bunny | From an API perspective, though, you're calling PUT /allocations/{c} | |
| 19:59:35 | fried_bunny | (or POST, same-same) | |
| 19:59:55 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: Snapshot https://review.openstack.org/543023 | |
| 19:59:55 | edleafe | ok, I think I see the case fried_bunny is talking about | |
| 20:00:00 | fried_bunny | edleafe: confirmed you get 200 empty list when calling on a consumer that doesn't exist. | |
| 20:00:21 | jaypipes | fried_bunny: then have claim_resources() use microversion 2.11 and have modify_allocations() use microversion 2.42 (or whatever microversion this new change gets) | |
| 20:00:39 | edleafe | it would happen if you created an instance, and then somehow un-allocated all its resources, and then re-allocated them. | |
| 20:00:50 | edleafe | I don't think that's possible | |
| 20:01:02 | fried_bunny | or a race where two agents attempt to create the first alloc for a consumer ID. | |
| 20:01:17 | fried_bunny | Sure, we can say it'll never happen in nova (maybe). | |
| 20:01:35 | edleafe | fried_bunny: yeah, and the second will fail that race | |
| 20:01:38 | edleafe | as designed | |
| 20:01:41 | fried_bunny | But from a pure API perspective, the point of this exercise is to protect against clients racing to PUT /allocations/{c} - whether it's the first or not. | |
| 20:02:13 | fried_bunny | edleafe: That's what I'm saying. If the consumer doesn't exist yet, we *must* have a special case for consumer_generation=None which fails 409 if the consumer exists at all. | |
| 20:02:32 | fried_bunny | We *can't* have it just increment the generation in that case. Or the *first* guy will lose the race, and not know why. | |
| 20:02:48 | edleafe | fried_bunny: let's walk through the race | |
| 20:02:52 | fried_bunny | And, this is a weird behavior, so it needs the shite documented out of it. | |
| 20:02:53 | fried_bunny | okay. | |
| 20:03:03 | fried_bunny | me or you? | |
| 20:03:09 | edleafe | both agents get the current state for the allocations, which is empty | |
| 20:03:12 | edleafe | me | |
| 20:03:31 | edleafe | Both try to PUT their allocations with con_gen=None | |
| 20:03:46 | edleafe | One succeeds, and the generation is then set at 0 | |
| 20:03:55 | edleafe | The second fails with a 409 response. | |
| 20:04:22 | edleafe | The second then refreshes its view, and gets allocations this time, with con_gen=0 | |
| 20:04:43 | edleafe | The second adds its allocations to that, and PUTs with a con_gen of 0 | |
| 20:04:45 | fried_bunny | yeah, the behavior you describe is what needs to happen. | |
| 20:04:52 | fried_bunny | But I don't think that's the way it's coded up. | |
| 20:04:54 | fried_bunny | could be wrong. | |
| 20:04:57 | edleafe | The second succeeds this time, and now con_gen=1 | |
| 20:05:15 | fried_bunny | I think the way it's coded up, you're assuming None means "old microversion, so just increment" | |
| 20:05:32 | edleafe | What I worry about is the second agent posting to an old microversion | |
| 20:05:40 | fried_bunny | That's his fault. | |
| 20:05:45 | edleafe | Which means "preserve the existing behavior" | |
| 20:05:50 | edleafe | which means overwriting | |
| 20:06:50 | fried_bunny | As far as that guy is concerned, the generation field doesn't exist. For the sanity of the first guy, we have to increment it. But if you're trying to use allocations from multiple clients and you're not using the generation-supporting microversion, you're toast anyway. | |
| 20:06:58 | edleafe | It's (very) possible that I missed a case in the code. But the approach is what was designed | |
| 20:07:45 | edleafe | agree that even if there is a post with an old microversion, the generation should be incremented | |
| 20:08:36 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: DiskAdapter parent class https://review.openstack.org/549053 | |
| 20:08:52 | fried_bunny | edleafe: I'm going through the spec again, but I don't recall this case being covered. | |
| 20:09:09 | fried_bunny | It needs to be described explicitly, because it's weird. | |
| 20:10:09 | fried_bunny | "Here's a new microversion where you have to send generation to ensure consistency. Oh, except the first time, when you have to send None (or leave it empty??). But if you do that when the consumer already exists, you'll fail. | |
| 20:10:11 | fried_bunny | " | |
| 20:11:46 | edleafe | it might not be in the spec | |
| 20:12:23 | edleafe | But it's not that weird. Standard practice is to get existing allocations before adding any new ones | |
| 20:12:49 | edleafe | I know it was discussed | |
| 20:12:52 | fried_bunny | okay, so what will you return? | |
| 20:12:55 | fried_bunny | in that first one? | |
| 20:13:00 | fried_bunny | Right now you get {} | |
| 20:13:04 | edleafe | but those discussions happened in several places at various times | |
| 20:13:12 | fried_bunny | Will you return { "consumer_generation": None } ? | |
| 20:13:43 | fried_bunny | or am I responsible for doing result.get('consumer_generation', None) ? | |
| 20:13:44 | edleafe | If there are no allocations for a consumer, then there is no consumer | |
| 20:14:19 | fried_bunny | Cool. So when I do my PUT, do I say "consumer_generation": None, or do I exclude the "consumer_generation" field entirely? | |
| 20:14:50 | edleafe | If you're PUTting to the new microversion, you would have to pass the generation, so the former | |
| 20:15:10 | fried_bunny | In which case the schema needs to tolerate [int or null] | |
| 20:15:29 | edleafe | guess it will have to :) | |
| 20:16:04 | edleafe | I'm just starting the patch that adds the microversion. I think these cases will make for excellent functional tests | |
| 20:16:24 | fried_bunny | I'm gonna mark up the spec review. | |
| 20:16:58 | edleafe | good idea | |
| 20:18:59 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: Localdisk https://review.openstack.org/549300 | |
| 20:21:12 | openstackgerrit | Eric Berglund proposed openstack/nova master: WIP: PowerVM: Cold Migrate & Resize https://review.openstack.org/553583 | |
| 20:21:12 | melwitt | fried_bunny: replied to your comments. this is my remaining open question https://review.openstack.org/#/c/533821/25/nova/tests/functional/api/openstack/placement/test_report_client.py@812 | |
| 20:21:27 | fried_bunny | melwitt: Roger that, looking... | |
| 20:21:35 | fried_bunny | (thought you were bunnying today, melwitt) | |
| 20:22:02 | melwitt | unfortunately no | |
| 20:26:09 | fried_bunny | edleafe: https://review.openstack.org/#/c/556971/ done been marked up. | |
| 20:31:24 | fried_bunny | melwitt: Responded. | |
| 20:37:41 | fried_bunny | edleafe: tbc, you're not intending to do the microversion bump in https://review.openstack.org/#/c/557959/ right? | |
| 20:38:26 | edleafe | fried_bunny: yeah, it will be the next one in that series | |
| 20:38:41 | fried_bunny | rgr | |
| 20:39:54 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: Localdisk https://review.openstack.org/549300 | |
| 20:41:29 | openstackgerrit | Eric Berglund proposed openstack/nova master: WIP: PowerVM: Cold Migrate & Resize https://review.openstack.org/553583 | |
| 20:45:54 | fried_bunny | edleafe: Hum, we need to change the POST /allocations and PUT /allocations/{c} responses to 200 with payload. | |
| 20:46:03 | fried_bunny | they're 204 right now. | |
| 20:49:00 | fried_bunny | starting to look like a mriedem commentary - three separate comment-leavings in a row on the same patch. | |
| 20:56:13 | edleafe | fried_bunny: not sure I get the reasoning behind changing the 204 from PUT/POST | |
| 20:57:08 | fried_bunny | edleafe: It's the same reason we did http://specs.openstack.org/openstack/nova-specs/specs/rocky/approved/generation-from-create-provider.html | |
| 20:58:21 | fried_bunny | edleafe: So clients don't have to do GET, PUT, GET, PUT -- can just do PUT, PUT instead. | |