Posted	Nick	Remark
#openstack-nova - 2018-01-24
16:52:08	mriedem	stephenfin: ok updated the docs one more time to fix a link and to include the resource class stuff which is approved ^
16:52:27	efried	cdent Beyond that, they *either* need to say, "We start at zero and increment by 1 whenever something changes about the provider," *or*, "You can't count on how we deal with the generation; it's an opaque value that you need to turn around and send back to us."
16:52:44	efried	cdent Yes, I agree that point does not have agreement.
16:53:00	efried	If we document the former, then the code as it sits is (almost) okay.
16:53:03	cdent	efried why would we want to say the first point above? I think of it as the second.
16:53:10	efried	cdent I'm with you.
16:53:18	efried	edleafe is on the other side.
16:53:27	efried	We don't know what jaypipes thinks
16:53:36	cdent	I think edleafe is being pragmatic, more than anything? edleafe ?
16:53:40	efried	And I *think* dansmith is also on the "opaque" side.
16:53:43	edleafe	if our code makes that assumption, we should document that
16:54:16	efried	whoah, we should document something about the API because of something our client code assumes? That's cray-cray.
16:54:20	dansmith	efried: yes
16:55:09	dansmith	efried: in fact, we could use some random sequence of numbers if we had one and that should be fine. or a uuid. "here is my proof that I'm making a change to the most recent copy" is all we should assume from the client side about the meaning of the generation field, IMHO
16:55:11	efried	I'd be willing to bet the client assumptions were made out of expediency to get function merged. At the time it didn't matter because a) single point of control, b) no nested, c) no traits/aggs to complicate.
16:55:18	edleafe	efried: the code uses it because it was understood to be the case. If it is going to be the case, it should be documented. If it isn't, the code that assumes that should change
16:55:20	efried	dansmith ++
16:55:30	efried	edleafe ++
16:55:53	cdent	efried: yes, expediency is what I said above
16:56:14	cdent	generation are ints because jay likes ints in databases
16:56:18	edleafe	We went with a monotonic integer scheme, even though there were better options available. I don't recall why it won out
16:56:25	cdent	but that they are ints is incidental
16:56:43	efried	So we need to make a call. Course, Jay should be included in the "vote" (if it's a democracy in the first place).
16:57:26	efried	"opaque" will require some nontrivial redesign and rework
16:57:26	cdent	Is the call as narrow as "should we += 1 in the client" or something more broad than that?
16:58:44	efried	cdent The call is, do we document +1 behavior or do we document opacity. What flows from the former is fairly narrow. What flows from the latter is broad.
16:59:38	efried	And not to distract, but we still definitely have a problem with DELETE not sending a generation down, no matter which way we decide to go.
17:00:13	dansmith	efried: I'm not sure we fully have a problem with delete
17:00:31	dansmith	efried: I should be able to delete allocations without a generation without being in violation of anything
17:00:39	dansmith	because I know the thing I'm deleting for is never coming back
17:00:58	dansmith	what else do we delete for? inventory maybe, but I bet the same argument works for that
17:01:51	cdent	efried: I think we can continue to fake on +1 in the ProviderTree if that's what we need to do, and _not_ document it. Other (to be developed) clients won't need to care and should respond to documentation that says what it already says "A consistent view marker"
17:01:52	efried	dansmith Inventory, yeah, and agree it's the same argument.
17:02:16	dansmith	efried: same argument, but you think we _do_ need a generation on delete right?
17:02:40	efried	dansmith Considering the multiple-points-of-control thing, if I think I'm ready to delete the RP's inventory, but you think you're happily editing it...
17:03:01	dansmith	efried: but inventory is and should only be managed by the thing owning the inventory
17:03:11	efried	dansmith If that's the case, we don't need generations at all.
17:03:28	dansmith	efried: I guess the only case that we might hit would be:
17:03:38	efried	dansmith I think the problem comes in when deleting the whole inventory is actually a result of "I want to remove inventory in this resource class, but that happens to be the last resource class".
17:03:41	dansmith	efried: I'm going to _allocate_ against a RP, right when the RP is deleting the inventory
17:04:19	efried	dansmith I think yours is okay. The DELETE will bounce if the allocations happened first; and the allocations will bounce if the DELETE happened first.
17:05:28	dhellmann	sdague : are you interested in this testing patch for the nova doc redirects? https://review.openstack.org/#/c/516385/
17:05:48	efried	dansmith But for mine: let's say for whatever reason two threads own inventory in different resource classes on the same RP. Thread A decides there's no longer any CUSTOM_FOO, so he goes to remove all the CUSTOM_FOO. He does a GET and sees that the inventory comprises *only* CUSTOM_FOO, so he gets ready to do a DELETE.
17:06:14	cdent	efried: I thought the point of the provider tree itself was to lock over that kind of operation?
17:06:26	cdent	the tree can only be manipulated by one thread at a time
17:06:26	efried	dansmith Meanwhile, thread B decides it's time to add some CUSTOM_BAR. He does his GET, sees some CUSTOM_FOO in there, adds his CUSTOM_BAR, and PUTs the whole shebang back. Now on the server, there's CUSTOM_FOO and CUSTOM_BAR.
17:06:40	dansmith	efried: but nothing other than the thing that owns the inventory should collapse a PUT to a DELETE because it thinks it's the last thing, right?
17:07:02	dansmith	efried: and the PUT in your case would have included a generation which is wrong now because the DELETE bumped it, no?
17:07:22	efried	No, because the PUT happens first.
17:07:27	efried	That's the point.
17:07:44	dansmith	no I meant the second case
17:08:25	dansmith	in the first case, the non-owner is collapsing a PUT to a DELETE which isn't legit It hink
17:08:26	cdent	efried: is [t 1zzs] not true?
17:08:26	purplerbot	<cdent> the tree can only be manipulated by one thread at a time [2018-01-24 17:06:26.123234] [n 1zzs]
17:08:38	dansmith	cdent: it doesn't matter,
17:08:44	efried	dansmith I don't disagree. But that's what the code is doing today.
17:08:46	dansmith	there could be multiple trees on multiple hosts I think is his point
17:09:02	dansmith	efried: I'm saying that's probably wrong, for the non-owner
17:09:06	cdent	dansmith: I though we were concluding that the compute node owns the inventory
17:09:08	efried	dansmith https://github.com/openstack/nova/blob/b214dfc41928d9e05199263301f8e5b23555c170/nova/scheduler/client/report.py#L982-L985
17:09:14	cdent	but I guess some things might share nested bits
17:09:16	cdent	which is
17:09:17	cdent	awkward
17:10:13	dansmith	cdent: we do, I'm just saying I think efried is talking about a more general case of multiple external actors needing to interact with one set of inventory or allocations, using generations as the locking mechanism
17:10:21	efried	sean-k-mooney has been coming up with all sorts of interesting use cases where it may very well be the case that e.g. nova owns the VFs, neutron owns the VIFs, and some other thing owns the bandwidth - all on the same RP.
17:10:31	dansmith	I'm only half paying attention to this conversation so maybe I should keep quiet :)
17:11:00	dansmith	efried: I'm not sure I agree that such a case is legit
17:11:00	efried	dansmith So far you've only been reaffirming my position, so by all means continue half paying attention.
17:11:03	cdent	efried: in which case a cache that is local the compute node is going to fraught with challenges beyond the one you have identified today
17:11:12	efried	dansmith Once you start disagreeing, then yeah...
17:11:16	dansmith	hah
17:12:07	openstackgerrit	Stephen Finucane proposed openstack/nova master: Don't filter out sibling sets with one core https://review.openstack.org/537361
17:12:07	openstackgerrit	Stephen Finucane proposed openstack/nova master: trivial: Update tests to set 'NUMACell.siblings' https://review.openstack.org/537362
17:12:08	openstackgerrit	Stephen Finucane proposed openstack/nova master: Ensure emulator threads are always calculated https://review.openstack.org/537363
17:12:08	openstackgerrit	Stephen Finucane proposed openstack/nova master: Always pass 'NUMACell.siblings' to _pack_instance_onto_cores' https://review.openstack.org/537364
17:17:10	cdent	efried: are you blocked in the immediate sense, or "merely" the medium term sense?
17:18:02	efried	cdent I'm not blocked, other than the fact that I'm discussing and writing up this issue rather than working on the update_from_provider_tree patch.
17:18:32	efried	I think we're broken long-term, but again, it's not going to matter until we have real-world situations that can do concurrent updates to the same RPs.
17:18:36	efried	Which won't be Q.
17:19:29	cdent	is "broken if we want a cache" or "broken"?
17:19:51	cdent	What I mean is, if later we decided "screw it, let's strip out the cache", would that be a thing?
17:20:06	cdent	(just trying to make sure I'm grokking)
17:20:15	mriedem	alex_xu: some comments in https://review.openstack.org/#/c/536085/ which can be addressed in the follow up nit cleanup patch - and also a request for a functional test, which would be done in a follow up patch
17:20:58	efried	cdent If we get rid of the cache, and never DELETE allocations/inventories unless we know we're the sole owner thereof, then I think we would be okay.
17:21:47	efried	cdent I don't think the placement API is broken. I just think it has some holes for consumers wishing to do good caching.
17:22:01	efried	cdent And certainly some holes in documentation.
17:22:20	cdent	for allocations that's (sole owner thereof, when deleting by consumer id) supposed to be true, but for inventories there's probably work to do
17:22:58	mriedem	"And certainly some holes in documentation." - i will say, all things aside to this discussion, the placement stuff has a lot of good documentation compared to what we've historically had for stuff in nova
17:23:05	cdent	From my perspective placement wasn't designed for caching. It was designed to be capable of tell you the truth, right now.
17:23:37	cdent	efried: if it can't do that fast enough, that's the bug
17:23:56	cdent	fast -> effectively, efficiently, low latency, reliably
17:24:03	efried	cdent I continue to be vehemently not on board with the theory that it's okay to do lots of calls to placement if that can be avoided.
17:24:33	cdent	we can agree to disagree on that and I respect your opinion.
17:24:34	efried	cdent Because I don't care how fast we get it, it's still a "call over the wire" (even if localhost).
17:24:56	efried	...which is always going to be an order of magnitude higher overhead than a local cache index.
17:25:06	cdent	efried: if you want to avoid calls over the wire I'd suggest looking at RPC :)
17:25:12	cdent	which is ripe for adjustments
17:25:14	efried	Even RPC
17:25:20	efried	oh, that's not what you meant.
17:25:26	edleafe	and what good is a local cache if you aren't sure it's current?

Earlier   Later