Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-01-24
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
16:14:26 dansmith you would never do gen+=1 locally
16:14:30 efried ++
16:14:37 dansmith you have no idea if your generation 6 is the same as the server side
16:14:46 efried https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L927
16:14:51 dansmith you think so because you posted with 5 and it succeeded, but you don't know
16:15:44 edleafe dansmith: which is why you always need to expect a 409
16:16:00 edleafe otherwise, what's the point of caching anything?
16:16:59 efried Which is actually another good point. DELETE /rp/{u}/inventories doesn't accept a generation (because doesn't accept a payload).
16:17:25 edleafe efried: ah, now *that's* a problem
16:19:21 efried Here's the other place we're making assumptions: https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L552 -- that a newly-created provider has gen 0.
16:20:39 efried Because the POST /resource_providers responds 201 with no content.
16:21:48 edleafe efried: that's the same assumption as earlier. generation starts at 0, and each change increases it by one
16:21:59 edleafe if you just created it, it's at zero
16:22:19 efried edleafe Right. Which isn't a good assumption, unless it's a documented part of the API, which today it ain't.
16:22:47 edleafe It's sounding more and more like a doc bug
16:23:17 efried If in fact we want it to be part of the API. Which I'm not convinced of.
16:29:13 efried I'm gonna say we definitely have a doc shortage with respect to how to *use* the generation in any case. Other than that one spot in the API ref, there's not even anything that tells you you're supposed to send down the same generation you GET.
16:35:23 cdent efried, edleafe: I definitely agree (with dansmith) that _in the presence of the provider tree caching mechanism_ we should never gen += 1 on the client side. Instead we should always be prepared to invalidate the _entire_ cache when a 409 happens. This is the reason for creating the PlacementAPIConflict base class: so any gen conflict can cause a complete reset.
16:35:52 openstackgerrit Matt Riedemann proposed openstack/osc-placement master: CLI for resource classes (v1.2) https://review.openstack.org/511182
16:35:53 openstackgerrit Matt Riedemann proposed openstack/osc-placement master: RP delete inventories (v1.5) https://review.openstack.org/514642
16:35:53 openstackgerrit Matt Riedemann proposed openstack/osc-placement master: RP list: member_of and resources parameters (v1.3, v1.4) https://review.openstack.org/511183

Earlier   Later