Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-01-24
15:34:48 cdent if you've deleted an RP why would you need to know it's generation?
15:34:49 cdent it's gone
15:34:55 openstackgerrit Merged openstack/osc-placement master: Add missing runtime requirements https://review.openstack.org/536870
15:35:06 efried cdent Deleted its inventory, not the RP itself.
15:35:26 edleafe efried: what would you love more?
15:35:30 cdent then GET the /rp/{uuid}, that's what it's there for
15:36:09 cdent this notion that it is bad to make requests is unproven. just make requests
15:36:24 efried edleafe Either DELETE returns a payload, which would look like { 'resource_provider_generation': N, 'inventories': {} }, or we always use PUT with {}
15:36:51 efried ...which does that
15:37:11 efried cdent It's not quite that simple.
15:37:26 openstackgerrit Merged openstack/osc-placement master: Address comments from original inventory patch https://review.openstack.org/521578
15:37:27 efried cdent The point of getting the generation in the response is that I know it's the generation right after the inventory was deleted.
15:37:34 openstackgerrit Matt Riedemann proposed openstack/osc-placement master: Usage docs and initial release note for osc-placement https://review.openstack.org/536858
15:37:37 mriedem stephenfin: done ^
15:37:44 cdent If you are depenendent on that, you're not rally making a very resilient system?
15:37:46 efried cdent Whereas if I do a subsequent GET, I don't know if an intervening operation incremented the generation as well.
15:37:55 cdent yes that's _always_ the case
15:38:01 cdent whether you did the the GET or not
15:38:11 efried cdent No, not when I get the generation in the response.
15:38:17 cdent you have no idea, ever, what any other client or thread might be doing with the same resource provider
15:38:37 Roamer` mriedem, yes, I saw it was rax-dfw on the previous runs, too, that's why I haven't complained to -infra (I guessed somebody else with more authority would complain again sooner or later), but thanks for raising it in -infra right now
15:38:41 cdent when you get the generation in the response, something else, immediately, might have changed it
15:38:47 cdent that's always true
15:38:55 cdent what ever mechanism you are using to track generations
15:38:59 efried cdent Yes, but I don't need to care about that until the next time I go to update something.
15:39:12 cdent which should always be the case
15:39:21 cdent you should always be prepared to get a 409
15:39:30 cdent and act accordingly
15:39:39 edleafe gotta agree - I don't see what getting the generation in the response buys you
15:39:49 efried I need to be able to get a 409 reliably, though.
15:39:51 efried Look look.
15:39:57 cdent the generation should be utterly meaningless
15:40:06 cdent sorry, I gotta go fetch the cat, will catch up in 30'
15:40:08 efried That's exactly why I shouldn't be assuming it gets incremented by 1
15:40:18 efried (the meaningless thing, not the cat thing)
15:40:37 edleafe if the delete succeeds, then it will be incremented by 1. You can certainly assume that
15:40:41 mriedem Roamer`: see -cinder
15:40:46 Roamer` mriedem, yes, looking
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 openstackgerrit Stephen Finucane proposed openstack/nova master: objects: remove pagesize from __init__ of InstanceNUMATopology https://review.openstack.org/485553
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:11 openstackgerrit Stephen Finucane proposed openstack/nova master: objects: remove related pinning from __init__ of InstanceNUMATopology https://review.openstack.org/485554
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: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 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:00 efried edleafe Sorry, right, add 1 to all the gen numbers after the first one.
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 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:39 dansmith bauzas: are you around today to address my minor comments here? https://review.openstack.org/#/c/535693/5
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 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:52:30 efried ause the server has gen 7. But I just blew away your inventory update.
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

Earlier   Later