Posted	Nick	Remark
#openstack-nova - 2021-08-27
14:45:01	gibi	we do
14:45:05	sean-k-mooney	oh ok
14:45:28	gibi	you can change QoS policy on a bound port, neutron will try to amend the instance allocation according to the new policy
14:45:35	sean-k-mooney	so ya change the bandwith or pps qos amount on a bound port would be the only time neutron could modify the allocation i think
14:45:43	gibi	yepp I think too
14:45:44	dansmith	okay, but this was for nested A-C, meaning another allocation against another provider coming back from placement, not any sort of actual nesting of allocations as stored right?
14:46:20	sean-k-mooney	yes i dont think there is really a concept of nested allocations
14:46:27	gibi	dansmith: right, we dont have sub allocations we have multiple RPs in an allocaiton
14:46:31	sean-k-mooney	jsut a singel allcoation with resouce form multple RPs today
14:47:04	dansmith	gibi: so you wrote this initial patch to move from delete to put specifically for "benefit" of generations.. are you thinking that was wrong now?
14:48:03	gibi	dansmith: I tried to figure out this mornign the reason why I had that patch
14:48:43	gibi	the only thing I know is that bumping the placemenet api to use nested a_cs meant that we got generations as well
14:48:51	gibi	so we had to do something with generations
14:49:29	gibi	i.e. the nested a_c support was in a bigger placement microversion than the generation support
14:50:26	gibi	1.28 and 1.29
14:51:53	gibi	if the request from the user is to delete the instance but that deletion fails due to a parallel operation then I think now that the delete operation should be the one that wins
14:52:47	dansmith	I don't think this patch was required just because we did generations and nested a_c, because you're updating the version you pass there,
14:52:48	gibi	in the other hand if the user requested restore of soft deleted instance and the timer on the soft delete -> hard delete is racing with that request then the restore should win
14:52:53	dansmith	and each call uses whatever version it wants right?
14:53:00	sean-k-mooney	gibi: microversion are per cal though
14:53:03	dansmith	right
14:53:24	gibi	right
14:54:30	gibi	I think the warning at the end of https://docs.openstack.org/placement/latest/placement-api-microversion-history.html#consumer-generation-support shows that at that time we was pretty serious about using generations strictly
14:55:44	sean-k-mooney	gibi: for the soft delete case we are expectign that this will help because? resotre will update allocation generation and the perodici will fail?
14:56:23	sean-k-mooney	for soft delete i assume we continue to claim the resouce with the allocation until the actul hard delete happens
14:56:34	sean-k-mooney	so would there be a need to update the allocation at all
14:56:40	sean-k-mooney	for either soft delete or restore
14:56:49	gibi	sean-k-mooney: if you are right then there could no be race between restore and soft -> hard delete
14:56:50	sean-k-mooney	e.g. will the generation actuly cahnge
14:57:01	gibi	btw the microversion history doc states
14:57:01	dansmith	gibi: I haven't mapped it out, but is there some reason we can't specifically opt-into the nuke-and-destroy behavior when we're deleting an instance and still fail for other cases?
14:57:02	gibi	Passing an empty allocations object along with a consumer_generation makes PUT /allocations/{consumer_uuid} a safe way to delete allocations for a consumer.
14:57:57	gibi	dansmith: you mean the user should be able to opt into the force?
14:58:03	dansmith	no
14:58:06	sean-k-mooney	dansmith: am i think the reason is that technially deletes are not allowd to have request bodies and we did not want to use a query arg
14:58:08	dansmith	you're force=True by default
14:58:22	sean-k-mooney	oh you mean the force default
14:58:27	dansmith	yes
14:58:31	gibi	dansmith: so just inverting the default
14:58:37	gibi	dansmith: that is OK to me
14:58:40	sean-k-mooney	we can set that to false and jsut pass true for a user delete
14:58:50	sean-k-mooney	ya im fine with inverting too
14:59:08	gibi	I think at some point melwitt stated that she used force=True as default as that was the more frequent call
14:59:09	dansmith	right, I think that's better signaling, and also better for whoever comes along later and uses that to delete an allocation
14:59:33	dansmith	I think making the dangerous activity explicit is worth the verbosity
14:59:39	sean-k-mooney	i tihnk its pretty close either way
15:00:01	sean-k-mooney	in terms of calls
15:00:21	dansmith	ack
15:40:03	dansmith	gibi: sean-k-mooney: do we know why the consumer types thing suddenly means we need to do this btw? I imagine the answer is just "more chance for the generation to change" but.. why specifically?
15:42:45	gibi	dansmith: exactly the consumer_types series merged the two transaction handling the allocation update into one, causing a longer transaction and hence a longer race window
15:43:40	dansmith	really? I wouldn't expect that to be meaningfully long
15:44:15	dansmith	as in, I would expect people running this in a larger cloud with lots of stuff going on would have hit this by now and not just in a gate run with few instances at any one point
15:44:29	dansmith	I saw that analysis from melwitt in the bug and wasn't so sure
15:44:58	dansmith	but I guess if it's really related to that change only then that has to be it
15:45:47	gibi	there is a correlation with the consumer_type series landed and the uptick in the conflict failures in the gate
15:46:20	gibi	I know correlation is not causation but still it feel relevant
15:46:34	gibi	and the consumer_type series had this transaction logic change
15:46:36	dansmith	hrm
15:46:50	gibi	i have no better explanation at the moment
15:47:21	dansmith	ack
15:53:21	sean-k-mooney	dansmith: sorry was on internal call
15:54:17	sean-k-mooney	ah am no i know there is a collation but not why
16:11:39	opendevreview	Balazs Gibizer proposed openstack/nova master: Support boot with extended resource request https://review.opendev.org/c/openstack/nova/+/800086
16:12:39	opendevreview	Ghanshyam proposed openstack/nova master: Convert features not supported error to HTTPBadRequest https://review.opendev.org/c/openstack/nova/+/806294
16:13:01	opendevreview	Balazs Gibizer proposed openstack/nova master: Support move ops with extended resource request https://review.opendev.org/c/openstack/nova/+/800087
16:14:27	opendevreview	Balazs Gibizer proposed openstack/nova master: [func test] refactor interface attach with qos https://review.opendev.org/c/openstack/nova/+/800088
16:15:49	opendevreview	Balazs Gibizer proposed openstack/nova master: Support interface attach / detach with new resource request format https://review.opendev.org/c/openstack/nova/+/800089
16:17:10	opendevreview	Balazs Gibizer proposed openstack/nova master: [func test] move unshelve test to the proper place https://review.opendev.org/c/openstack/nova/+/793621
16:18:32	opendevreview	Balazs Gibizer proposed openstack/nova master: [nova-manage]support extended resource request https://review.opendev.org/c/openstack/nova/+/802060
16:19:26	opendevreview	sean mooney proposed openstack/nova master: Remove module level caching https://review.opendev.org/c/openstack/nova/+/806394
16:19:27	opendevreview	sean mooney proposed openstack/nova master: db: Handle parameters in DB strings https://review.opendev.org/c/openstack/nova/+/805663
16:19:54	opendevreview	Balazs Gibizer proposed openstack/nova master: Reno for qos-minimum-guaranteed-packet-rate https://review.opendev.org/c/openstack/nova/+/805046
16:30:31	opendevreview	Balazs Gibizer proposed openstack/nova master: Add force kwarg to delete_allocation_for_instance https://review.opendev.org/c/openstack/nova/+/688802
16:30:39	gibi	dansmith, sean-k-mooney, melwitt, lyarwood: based on the above disucssion I inverted the force default value in ^^
16:31:09	gibi	lyarwood, stephenfin: I fixed up the nits in the pps series. thanks for the valuable feedback
16:33:39	gibi	and with that I end my week. o/
16:42:15	sean-k-mooney	gibi: https://review.opendev.org/c/openstack/project-config/+/787523 has now merged so we shoudl have the review priority lable avaiable just an fyi
16:45:37	gmann	finally :)
16:46:14	sean-k-mooney	i think https://review.opendev.org/q/project:openstack/nova+Review-Priority:1 is how to use it
16:46:30	sean-k-mooney	but we dont have any patches with it set
16:46:36	sean-k-mooney	https://review.opendev.org/q/project:openstack/nova+Review-Priority:0 get hits however
16:56:07	gmann	or event this, https://review.opendev.org/q/project:openstack/nova+Review-Priority:1+label:Verified%253D1++NOT+label:Workflow%253C%253D-1
16:56:08	gmann	sean-k-mooney: ^^
16:56:29	gmann	*even
16:57:15	sean-k-mooney	perhaps we will need at lest one patch marked as a prioroty to test it
16:57:22	gmann	it can be appended with NOT+owner:self
16:57:56	gmann	sean-k-mooney: you can test with 0 priority to test query
16:58:52	melwitt	gibi: ack
17:22:16	melwitt	dansmith: I didn't think changing to a single transaction would make that much of a timing difference either but the consumer types patch series hit that bug far more often than anything else did. I didn't see anything else in those patches that could possibly be related, so I guessed about the db transaction change. my guess might be wrong
17:22:38	dansmith	I'm not saying you're wrong, I'm just surprised
17:23:05	dansmith	and surprised lots of real deployments aren't having trouble with much busier systems before the consumer types patch made the transaction longer
17:23:19	melwitt	I was surprised too and thought maybe it was a coincidence but once it merged everything was hitting the bug
17:23:26	dansmith	and if that's really it, I sure hope we're not in for crazy pain if that made the transaction like waaaay longer or something
17:24:17	melwitt	yeah, same. I haven't seen something like this before
17:37:31	sean-k-mooney	i know that i have seen conflict in ci jobs form update avaiable resouces before the consome types change
17:38:01	sean-k-mooney	so there was already some conficts happening
17:38:18	sean-k-mooney	i dont think it caused test failures but i have seen it in the logs
17:43:15	dansmith	oh definitely
17:57:57	opendevreview	Ghanshyam proposed openstack/nova master: Convert features not supported error to HTTPBadRequest https://review.opendev.org/c/openstack/nova/+/806294
18:28:37	melwitt	yeah, I know it was occurring prior to consumer types, just saying that on the consumer types series it hit the bug often and after it merged it's happening more often

Earlier   Later