| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-24 | |||
| 15:40:50 | efried | edleafe No way. | |
| 15:40:51 | Roamer` | I mean watching | |
| 15:40:54 | mriedem | looks like cinder made a change in the last 24 hours that is slowing down volume backup | |
| 15:41:01 | edleafe | What you can't assume is that the next time you do something, it won't have been changed by something else | |
| 15:41:07 | efried | edleafe That's not part of the contract. It just happens to be the way placement is implemented. | |
| 15:41:23 | efried | This is exactly my point | |
| 15:41:24 | Roamer` | mriedem, ah, you even figured out which one! I just mentioned it a couple of times there, but it seemed nobody was awake yet... | |
| 15:41:57 | efried | It is crucial that the generation I have in my local cache matches the data I have in my local cache. | |
| 15:42:03 | edleafe | efried: do you mean if generation was a hash of the current state, you wouldn't be able to make any assumption? | |
| 15:42:24 | efried | edleafe Or any other implementation that's not a monotonically increasing integer. | |
| 15:42:52 | efried | edleafe I mean that the format of the generation is not part of the API contract at all, and the client side should be assuming absolutely nothing about it. | |
| 15:43:07 | edleafe | So what would getting the generation buy you? It *still* could have been changed by the time you go to use it | |
| 15:43:35 | efried | Yes, in which case I will legitimately get a 409 and need to refresh/redrive my change. That's the good path. | |
| 15:43:41 | efried | The bad path is this: | |
| 15:45:07 | edleafe | efried: https://github.com/openstack/nova/blob/master/nova/api/openstack/placement/schemas/inventory.py#L20-L25 | |
| 15:45:53 | efried | I have generation 5. I do my DELETE. Then you do a GET (so you have generation 5) and a PUT to create some inventory. You're now at gen 6. Now I do my GET as cdent suggested and find out the gen is 6. So I think gen 6 corresponds with empty inventory when in fact it does not. Now I decide to add some inventory back. I do a PUT with that new inventory and gen 6, which will *succeed* because the server has gen 6. But | |
| 15:45:54 | efried | I just blew away your inventory update. | |
| 15:46:10 | mriedem | Roamer`: https://bugs.launchpad.net/cinder/+bug/1745168 for rechecks i guess | |
| 15:46:11 | openstack | Launchpad bug 1745168 in Cinder "volume backup tests timing out since 1/23" [Undecided,New] | |
| 15:46:21 | mriedem | melwitt: dansmith: et al ^ if you see gate timeouts | |
| 15:46:31 | dansmith | ack | |
| 15:47:10 | efried | edleafe Great, it's defined as an integer, in the schema, for the current microversion. There's still nothing in the contract that tells me it's monotonically increasing starting from zero. | |
| 15:47:10 | openstackgerrit | Stephen Finucane proposed openstack/nova master: objects: remove pagesize from __init__ of InstanceNUMATopology https://review.openstack.org/485553 | |
| 15:47:11 | openstackgerrit | Stephen Finucane proposed openstack/nova master: objects: remove cpuset_reserved from __init__ of InstanceNUMATopology https://review.openstack.org/466030 | |
| 15:47:11 | openstackgerrit | Stephen Finucane proposed openstack/nova master: objects: remove related pinning from __init__ of InstanceNUMATopology https://review.openstack.org/485554 | |
| 15:47:22 | edleafe | efried: "Then you do a GET (so you have generation 5)" - that's not correct. You'd get 6 in that case, since the DELETE succeeded | |
| 15:48:00 | efried | edleafe Sorry, right, add 1 to all the gen numbers after the first one. | |
| 15:48:00 | Roamer` | mriedem, thanks, I'll use it if needed... let's hope it's not needed much longer, because there's not much time left till... what, tomorrow? :) | |
| 15:48:11 | efried | edleafe Or whatever. The logic is the same. | |
| 15:48:33 | mriedem | Roamer`: eod tomorrow yeah | |
| 15:48:46 | edleafe | efried: so you'd think you're at 6, but meanwhile someone did an update to move it to 7. You then PUT the inventory at 6 (since you don't know it's been changed), and get a 409 | |
| 15:49:01 | edleafe | Nothing blown away | |
| 15:49:16 | Roamer` | mriedem, aaaaand if failed again.... and I can't do a recheck before it finishes failing... | |
| 15:49:22 | efried | edleafe No, I don't think I'm at 6 unless I assume the generation increments by 1 | |
| 15:49:39 | dansmith | bauzas: are you around today to address my minor comments here? https://review.openstack.org/#/c/535693/5 | |
| 15:49:39 | efried | edleafe I'll think I'm at 7 because I did my GET (to discover the "correct" generation) after you did your PUT. | |
| 15:49:53 | dansmith | bauzas: if not I can do it, but I'd like to retain my impartiality to be able to vote on it :) | |
| 15:50:04 | edleafe | efried: Huh? I thought you said "Then you do a GET" after the delete | |
| 15:50:11 | mriedem | Roamer`: i'll recheck it throughout the day | |
| 15:50:14 | edleafe | Where are you assuming anything | |
| 15:50:17 | edleafe | ? | |
| 15:50:36 | efried | edleafe Just so. | |
| 15:50:44 | Roamer` | mriedem, so yeah, I'll keep on rechecking... oh? You will? Thanks a lot! (again... it seems that I'm thanking you on every other line...) | |
| 15:51:06 | efried | edleafe If I'm not assuming the generation increments by 1, and I have to do a GET to discover what the generation is after I do my DELETE, that's broken. | |
| 15:51:48 | edleafe | efried: I still don't see the issue | |
| 15:52:01 | efried | Here it is again with the correct generations: | |
| 15:52:28 | edleafe | efried: you can make the assumption, and if something else changes it, you get a 409 and *then* do the GET to see what the current state is | |
| 15:52:30 | efried | ause the server has gen 7. But I just blew away your inventory update. | |
| 15:52:30 | efried | I have generation 5. I do my DELETE. Server has gen 6, I have gen 5 still. Then you do a GET (so you have generation 6) and a PUT to create some inventory. You and server are now at gen 7. Now I do my GET and find out the gen is 7. So I think gen 7 corresponds with empty inventory when in fact it does not. Now I decide to add some inventory back. I do a PUT with that new inventory and gen 7, which will *succeed* bec | |
| 15:53:33 | efried | edleafe Perhaps the disconnect is where I'm doing that GET to discover the generation. At that point I'm only doing a GET /resource_provider/{uuid} - I really *don't* want to go getting its traits, aggs, and inventories as well. | |
| 15:53:45 | edleafe | efried: if you do a GET and ignore that there is now inventory and then proceed to blow it away, that's not placement's fault | |
| 15:54:00 | efried | Then there's no point having a cache. | |
| 15:54:43 | efried | We have this cache that we're using successfully for most of the operations, but if we run any DELETE API, we have to blow it away and start over? That seems not okay. | |
| 15:54:47 | edleafe | efried: especially if you have two different systems claiming to be authoritative on what the inventory for an RP is | |
| 15:55:35 | efried | That's the entire reason we have this generation business in the first place. To manage sync issues when more than one thread claims authority over a provider. | |
| 15:56:14 | efried | And I'm not saying it's placement's fault that this happens. It's a matter of cache management by the client. Which IMO we're not doing correctly in this scenario. | |
| 15:56:49 | edleafe | efried: yes, but it is to prevent *accidental* conflicts. If you know you have a conflict and proceed ahead anyway, there isn't a system that will make that OK | |
| 15:57:01 | efried | But I don't know I have a conflict. | |
| 15:57:14 | efried | Put another way, a client shouldn't use DELETE for inventories (or traits or aggs if it comes to that) if there's no response containing the generation. | |
| 15:57:55 | edleafe | efried: your cache has RP, generation, and inventory. You go to update that and get a 409. It is now important that you invalidate the cache before you proceed | |
| 15:58:17 | efried | I don't get a 409. That's the crux of the problem. | |
| 15:58:40 | edleafe | efried: you don't do the GET *before* trying to update | |
| 15:59:12 | efried | How else do I discover the "new" generation? | |
| 15:59:28 | efried | (the one I think is post-DELETE, but which is in fact post-my-DELETE-and-your-PUT) | |
| 15:59:32 | edleafe | You update based on what the cache says. If the cache is current, the generation it has will work. If something else has changed in placement, you invalidate the cache, and do a GET to refresh it | |
| 15:59:51 | efried | and how do I know "something else has changed in placement"? | |
| 16:00:02 | edleafe | that's where the assumption that after the delete, the generation is original gen + 1 | |
| 16:00:38 | efried | Right. Which works, today, because we're dirtily exploiting what we know to be the internals of the placement service. | |
| 16:01:25 | edleafe | "dirtily" - heh | |
| 16:01:29 | edleafe | it's documented | |
| 16:01:44 | edleafe | Give me a second to write up the flow as I understand it. | |
| 16:01:52 | efried | edleafe Where is it documented? | |
| 16:04:49 | efried | edleafe In the API ref, all we get is: "A consistent view marker that assists with the management of concurrent resource provider updates." and "409 Conflict if the resource_provider_generation doesn’t match with the server side." <== note "match" | |
| 16:05:07 | efried | And it's not mentioned in the overview doc (https://docs.openstack.org/nova/latest/user/placement.html) at all | |
| 16:05:44 | edleafe | efried: I'll have to do some doc digging, but I'm sure it's there | |
| 16:06:04 | edleafe | efried: here's the flow I see for your inventory delete/update: | |
| 16:06:06 | edleafe | You have a cached RP with generation 5. You call DELETE for the inventory, and it succeeds. You now update your cache with generation 6, and no inventory. | |
| 16:06:10 | edleafe | You now want to PUT some inventory. So you send the PUT with the data and generation 6. Something else has changed the RP in the meantime, so your PUT returns 409. At that point, your cache is not accurate. You don't know what has changed, so you get rid of anything for that RP, and call GET with the rp_uuid to refresh everything in your cache for that RP. You then compare the inventory you | |
| 16:06:16 | edleafe | expected with what is there (which you would have done before the original PUT), and if it is still correct to update inventory, you re-PUT the inventory with the new generation. Rinse and repeat. | |
| 16:06:56 | efried | edleafe That's a good flow, but it's not the one I described. | |
| 16:07:13 | efried | edleafe But before we go on, how does "You now update your cache with generation 6" happen? | |
| 16:07:43 | edleafe | that was the assume that if your DELETE succeeds at gen5, it is now at gen6 | |
| 16:07:50 | efried | Assuming gen+1? | |
| 16:08:13 | melwitt | mriedem: ack | |
| 16:08:27 | edleafe | efried: yes. | |
| 16:08:54 | efried | Okay, then I agree it works, but is illegal API consumption. Because the API doesn't tell us we can assuming monotonically increasing generation counter per operation. | |
| 16:09:22 | edleafe | efried: if it doesn't, then that's a doc bug | |
| 16:09:49 | efried | This is how we do it today. Which is (part of) the reason we don't have a "bug" per se. (The other part is that we don't actually have any paths that do concurrent updates yet.) | |
| 16:10:01 | efried | edleafe I don't agree with that. | |
| 16:11:40 | edleafe | efried: let's get jaypipes and cdent to weigh in on this | |
| 16:11:49 | efried | edleafe But maybe. We should find out what Jay thinks. If we add words to those DELETE 204s along the lines of, "You may now assume the resource provider generation has been incremented by 1," I'll stfu. | |
| 16:11:56 | efried | yeah, what you said. | |
| 16:11:58 | edleafe | I think we understand each other, and are not in agreement | |
| 16:12:08 | efried | Does dansmith have skin in this game? | |
| 16:12:27 | dansmith | um | |
| 16:12:29 | edleafe | I think dansmith is pretty busy with other things :) | |
| 16:12:37 | efried | k | |
| 16:13:07 | edleafe | efried: the assumption is that after anything that modifies anything about a RP, the generation is +1 from what it was | |
| 16:13:14 | edleafe | not just DELETE | |
| 16:14:19 | dansmith | client-side ever predicting the generation is wrong | |