Posted	Nick	Remark
#openstack-nova - 2018-02-13
20:01:05	dansmith	but for this inventory thing, nic bandwidth is probably a good example
20:01:06	mriedem	dansmith: yeah that's what i had to do in my poc patch for the capabilities thing
20:01:10	dansmith	yeah
20:01:29	efried	So I agree that we don't want to make it a rule that virt blows away children it doesn't recognize.
20:01:32	dansmith	so maybe for this we could get the inventory from the vif modeling somehow?
20:01:44	mriedem	trying to balance the stance we've had in the past against things like metrics providers in-tree saying that's all best served outside of nova,
20:01:54	efried	But the design (and imple) is flexible enough that we don't need to make that rule at this level.
20:02:00	efried	s/impl/implementation/
20:02:03	mriedem	and now we appear to have something that's outside of nova for external services, but we're saying we'll overwrite what they do
20:02:05	efried	vay
20:02:25	efried	(Sorry, that was /me frustrated at own inability to spell, twice)
20:02:37	efried	Okay, I get the concern.
20:02:50	dansmith	efried: so you're saying I can have a child of compute node and the update_tree() won't blow those away, just inveentory for the level I'm updating, yeah?
20:02:58	openstackgerrit	Merged openstack/nova master: Invalid query parameter could lead to HTTP 500 https://review.openstack.org/539164
20:03:32	efried	dansmith: We'll make placement look *exactly* like whatever comes out the other side of update_provider_tree.
20:03:41	efried	But I still think that's okay.
20:04:03	dansmith	efried: not if that means we blow away stuff we didn't return.. so now I'm confused
20:04:10	efried	Because 1) virt gets to be the (primary) source of truth for the provider tree rooted at the compute node.
20:04:33	efried	And 2) virt kinda needs to know whether there's some other entity "out there" that's allowed to mess with some level of its tree.
20:04:49	dansmith	well, that's what I'm saying,
20:04:51	efried	If it knows that, then it can preserve those pieces of the tree unchanged.
20:04:53	dansmith	if we go that route,
20:05:07	efried	Because it receives them as part of the tree it gets on input.
20:05:15	dansmith	then we need to have the virt drvier capable of collecting external things we support, like bandwidth on a nic
20:05:20	efried	nononono
20:05:22	dansmith	oh,
20:05:31	efried	It doesn't need to be able to collect them, cause we already gave it to... yeah.
20:05:32	dansmith	you're saying ProviderTree already has the child things,
20:05:37	efried	yes, exactly.
20:05:43	dansmith	that's the confusion though:
20:05:45	efried	It has the whole picture as Placement knows about it right now.
20:05:58	efried	...at least the picture that's rooted at the compute host RP.
20:06:20	dansmith	does ProviderTree.update(<not including things that were in the child>) blow those away? or are you saying the virt driver needs to look and include those things in its update?
20:07:24	efried	ProviderTree.update_[thingy](rp_uuid) for thingy in (inventory, traits, aggregates) will replace thingy for just the RP indicated by rp_uuid.
20:07:50	efried	E.g. if you replace inventory via that method, but not traits, then traits stay whatever they were before.
20:08:02	efried	Also, those update_* methods don't muck with children.
20:08:06	dansmith	yeah but that's not the question
20:08:12	dansmith	okay, _that_ is the quesation
20:08:19	dansmith	whether update() touches the children
20:08:22	dansmith	um
20:08:26	dansmith	"affects the children"
20:08:28	efried	To muck with hierarchy, you have to ProviderTree.remove(), .new_root(), .new_child()...
20:08:33	efried	There is no update().
20:08:39	dansmith	I know, I'm saving letters
20:08:45	dansmith	I think that's his question
20:08:54	dansmith	so I had assumed it would only affect the level you're on, and was replying,
20:09:04	dansmith	but then re-read the bit of your text he's taking exception to and wasn't sure
20:09:10	dansmith	so I think you just need to tighten that up a smidge
20:09:42	efried	Okay. That's a true statement ("...only affects the level you're on"). *except* that if you .remove(rp), you also remove all of rp's descendants. That's what the sentence is saying.
20:09:48	dansmith	sure
20:09:50	dansmith	and,
20:10:15	dansmith	I was going to say that if the compute node thinks it needs to remove its provider, then it _should_ affect the external thing that still thinks it is reporting resources for that provider
20:10:23	dansmith	i.e. compute is authoritative for the existence of itself
20:10:29	efried	right.
20:11:07	efried	It'll be a possibly-not-enforced-but-highly-recommended rule that these entities that share control of a tree never share control of a single RP within that tree.
20:11:22	efried	that way lies madness.
20:11:29	dansmith	not sure how you would enforce tat, but agree, that should be the assumption
20:11:33	dansmith	*that
20:11:35	mriedem	stephenfin: still around?
20:12:19	efried	Okay, I'll post an edit. Thanks for the discussion dansmith, mriedem
20:21:30	openstackgerrit	Matt Riedemann proposed openstack/nova master: Cleanup the manage-volumes admin doc https://review.openstack.org/544066
20:25:13	jaypipes	efried, dansmith, mriedem: just finished reading back... yes, agree that the spec could use some clarity/language around the update_provider_tree() method implementations not calling ProviderTree.remove() for nodes in the tree that it doesn't manage.
20:25:24	dansmith	aye
20:25:30	efried	Roger wilco.
20:26:19	jaypipes	efried: I still don't understand this from tetsuro though: "Note that at least libvirt driver today removes CUSTOM_RCs which users set manually via placement API for a compute host when the periodic updating task happens."
20:26:21	openstackgerrit	Matt Riedemann proposed openstack/nova master: Cleanup the manage-volumes admin doc https://review.openstack.org/544066
20:26:58	efried	jaypipes: I think he's saying libvirt removes *inventory* in custom RCs it didn't create itself.
20:27:13	efried	I don't think we're talking about removing the actual resource classes from placement.
20:27:26	jaypipes	efried: is he just saying that if an operator manually calls POST /resource_providers/{cn_uuid}/inventories and adds a CUSTOM_XXX resource class, that it will be removed by the libvirt virt driver's get_inventory() method?
20:27:33	efried	I think so, yes.
20:27:44	jaypipes	efried: gotcha. ok. and yes, that is definitely by design...
20:27:47	efried	yup.
20:28:23	mriedem	that is eventually due to this check? https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L831
20:28:31	jaypipes	efried: since clearly the virt driver owns the resources on the compute node. but it's worth saying that update_provider_tree() implementations will *not* remove a *node* from the tree of providers if it doesn't know about the node.
20:28:37	jaypipes	mriedem: yea
20:28:57	jaypipes	mriedem: oh, no...
20:29:26	efried	That's just an optimization that'll short-circuit if nothing has changed.
20:29:29	jaypipes	mriedem: that check is seeing if what we got from get_inventory() is the same as what is cached in the reportclient's provider tree.
20:29:36	jaypipes	what efried said.
20:29:57	mriedem	but if the driver reports some inventory change, for VCPU, MEMORY_MB and DISK_GB,
20:30:07	mriedem	and some external service had set some CUSTOM_FOO inventory,
20:30:12	mriedem	that's the code that's going to blast it away right?
20:30:34	jaypipes	mriedem: that conditional will be true if the operator set some CUSTOM_XXX resource class inventory and libvirt doesn't report that resource class.
20:30:52	jaypipes	mriedem: it's just that inv_data is always exactly and only what the virt driver sets it to.
20:30:53	mriedem	yeah ok
20:31:04	jaypipes	mriedem: and that will overwrite anything the operator manually set.
20:31:07	mriedem	yup
20:32:57	jaypipes	mriedem: right, but again, that's get_inventory(). the update_provider_tree() method will blow away inventory for the specific (root) compute node provider and set it to whatever it thinks it should be. but it won't delete child nodes it doesn't manage. so if the operator (or external agent) set inventory for, say, NET_EFRESS_BYTES_PER_SECOND on a child provider node, the update_provider_tree() method won't delete that node and its inventory.
20:33:22	jaypipes	EGRESS...
20:33:50	mriedem	EFRIED_BYTES
20:33:54	mriedem	jaypipes: got it
20:34:07	jaypipes	mriedem: but if an operator manually set the VCPU inventory on the compute node resource provider to something, the virt driver absolutely will overwrite that inventory to what it thinks is correct.
20:34:13	jaypipes	mriedem: ok, sorry for babbling
20:34:19	mriedem	yup makes sense
20:35:39	efried	jaypipes: tbc, we have no way (other than code reviews) to enforce that update_provider_tree doesn't muck with child nodes it doesn't manage.
20:36:09	mriedem	functional tests?
20:36:33	jaypipes	efried: correct.
20:36:48	efried	mriedem: Functional tests that would have to know something about the expected structure of the tree for the individual virt driver.
20:36:59	efried	structure & management
20:38:08	jaypipes	efried: we can't programmatically determine resource ownership and policy for the various virt drivers and potential external agents ahead of time. all we can do is docstring the heck out of update_provider_tree() and explain the ramifications of calling ProviderTree.remove().

Earlier   Later