| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-04-02 | |||
| 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. | |
| 20:58:37 | edleafe | But for a consumer? RPs are long-lived and frequently changed. Consumers rarely get changed, and I can't think of a use case where you would create allocs, and then create some more right away | |
| 21:01:28 | fried_bunny | eh? | |
| 21:01:42 | fried_bunny | Hold on, processing that... | |
| 21:01:43 | melwitt | dansmith: is this your comment on L45? https://etherpad.openstack.org/p/nova-runways-rocky if all non-WIP changes are merged, I think we can bump the blueprint out of the runway and put the next in line in the runway. does that sound cool? | |
| 21:02:08 | dansmith | melwitt: bope not mine | |
| 21:02:10 | dansmith | er, nope | |
| 21:02:32 | fried_bunny | edleafe: But instinctive reaction is: We need to avoid focusing on Nova as the only consumer (again/still). | |
| 21:02:33 | dansmith | melwitt: but yes, the WIP bits are waiting on pieces of placement that don't exist yet, so it's "done" | |
| 21:02:56 | melwitt | oh, guh. I'm really bad at remembering what author color people are | |
| 21:03:08 | fried_bunny | it changes, too :( | |
| 21:03:11 | dansmith | melwitt: maybe want to, at least initially, copy things out of a runway to a log area and put some comments about how it went? | |
| 21:03:19 | edleafe | fried_bunny: I'm not thinking of Nova | |
| 21:03:35 | melwitt | dansmith: sounds like a good idea | |
| 21:03:36 | dansmith | melwitt: for that set, very little review was had for about a month and then it got review, iteration, and then merged quickly whilst in that slot | |
| 21:03:41 | fried_bunny | dansmith, melwitt: ++ to that | |
| 21:03:52 | fried_bunny | (the keeping-logs-of-runways) | |
| 21:04:10 | edleafe | for any placement user: if you are making allocations, you would GET the current state, modify those allocations to add/remove what is needed, and then PUT that back | |
| 21:04:36 | fried_bunny | edleafe: And cache that, so that next time, you don't have to reGET. | |