| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-04-02 | |||
| 19:49:18 | fried_bunny | Then what do you send down as your generation value the first time you create such an allocation? I guess None has to be legal at that point. | |
| 19:49:54 | jaypipes | fried_bunny: yeah | |
| 19:50:10 | fried_bunny | edleafe: ^ how is this handled? | |
| 19:50:54 | fried_bunny | Under the new microversion, None has to be special | |
| 19:51:10 | fried_bunny | special-cased to mean "Fail 409 if the consumer exists" | |
| 19:52:09 | jaypipes | fried_bunny: the previous behaviour is essentially an implicit consumer_generation of None. | |
| 19:52:20 | fried_bunny | jaypipes: Not the same thing at all, though. | |
| 19:52:28 | jaypipes | fried_bunny: how so? | |
| 19:53:29 | fried_bunny | With the new code in place, you can still use the old microversion, which is consumer_generation of None, and it'll increment the existing generation, whatever it happens to be. | |
| 19:53:39 | fried_bunny | If you use the *new* microversion and send None, we can't do that. | |
| 19:54:51 | jaypipes | fried_bunny: why would you send the new microversion and send None, though?> | |
| 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. | |