Posted	Nick	Remark
#openstack-nova - 2019-11-26
18:40:31	efried	hahaha
18:41:24	openstackgerrit	Dan Smith proposed openstack/nova master: Add a way to exit early from a wait_for_instance_event() https://review.opendev.org/695985
18:51:08	gmann	johnthetubaguy: these test change is due to base class assert on error message. these tests will be changed only where granularity is added which should not be many or we can remove the error message checks . - https://review.opendev.org/#/c/648480/20/nova/tests/unit/policies/test_services.py@63
18:54:44	openstackgerrit	Merged openstack/nova stable/train: Replace time.sleep(10) with service forced_down in tests https://review.opendev.org/696088
19:05:42	openstackgerrit	Ghanshyam Mann proposed openstack/nova master: Add new default rules and mapping in policy base class https://review.opendev.org/645452
19:05:58	openstackgerrit	Ghanshyam Mann proposed openstack/nova master: Suppress policy deprecated warnings in tests https://review.opendev.org/676670
19:07:49	openstackgerrit	Ghanshyam Mann proposed openstack/nova master: Add new default roles in os-services API policies https://review.opendev.org/648480
19:08:02	efried	Can I get you to look at the vTPM spec again please?
19:08:02	efried	dansmith: +2, thanks.
19:08:15	efried	https://review.opendev.org/#/c/686804/
19:08:41	dansmith	no
19:09:57	efried	mriedem: if you have a minute, would you please take a look at https://review.opendev.org/695985 (lets us fix races to wait_for_instance_event())
19:12:33	dansmith	efried: mriedem: just left a comment about _how_ we'll use this with cyborg to close that race: https://review.opendev.org/#/c/631244/46/nova/compute/manager.py@2627
19:14:32	efried	dansmith: ++ makes perfect sense. Do you have a strong opinion about whether the bind should be kicked off from conductor instead of compute at this point?
19:14:53	dansmith	efried: I think it'd be better if we did it from conductor for all the reasons I originally thought that
19:15:35	dansmith	so unless there's some new revelation about why that's bad, I have the same opinion
19:16:01	efried	I think it might have had something to do with how easy/hard it is to clean up. But I can't think why it would matter.
19:16:15	efried	otherwise it's just a question of having to redo the code moar.
19:16:49	dansmith	well, the major benefit is the overlap of work getting done to build the accelerator before we get down to the last minute
19:17:13	sean-k-mooney	the only issue i was aware of was it would require use to pass the arq info as part of the spawn call to the compute
19:17:20	dansmith	if you do it from conductor, you're letting cyborg know about things before you take the trip over rabbit to the compute, wait in the parallel build queue, set up volumes, networking, etc, etc
19:17:21	sean-k-mooney	but we may already need to do that
19:17:49	dansmith	the current code is not passing it, AFAICT, and building all of that into the compute,
19:17:55	dansmith	which is another good reason not to do it on the compute
19:18:12	dansmith	the more the compute implies about the request from the state of the system, the harder it is to effect an upgrade that changes those implications later
19:18:19	sean-k-mooney	right but i think they choose to build it on the compute initally to avoid the rpc chagne to pass it
19:18:29	sean-k-mooney	i prefer doing it in the conductor too by the way
19:18:39	dansmith	right now, all the computes are examining the flavor directly and doing what they think they need to
19:18:39	sean-k-mooney	i just think that is why the current code does not
19:19:16	dansmith	if we change something about how that works, conductor gets upgraded first and can change the behavior, just passing the arq to the compute, which just needs to wait and hook it up
19:39:12	mriedem	efried: the bind from compute/conductor thing was in the ML awhile back, probably worth dredging that up if we're changing our minds now
19:40:12	mriedem	from what i remember, i think i also pushed for conductor but then after lots of wah wah it's easier from compute without a bunch of rpc interface changes i relented
19:40:26	dansmith	mriedem: we had originally said it would be from conductor, then the spec changed
19:40:46	dansmith	mriedem: the spec seemed to call out the eventing as the primary reason why it had to happen from compute
19:40:57	dansmith	which is both not correct, and also laughably terrible in the implementation currently proposed
19:41:00	mriedem	i don't remember the details, i just know it came up in the ML
19:41:22	sean-k-mooney	it also came up in the denver and maybe dubling ptg
19:41:28	sean-k-mooney	its been a while
19:41:55	sean-k-mooney	i think long term we all agreed teh conductor was better.
19:42:00	mriedem	ok, let me rephrase: the last time i remember there being thoughtful and documented takes on that was in the ML
19:42:11	mriedem	rather than some undocumented stuff from dubling
19:42:20	sean-k-mooney	i think at some point there was a request to avoid the rpc chagne but i dont really rememebr
19:42:33	mriedem	"i don't really remember" is why i refer back to the ML
19:42:53	sean-k-mooney	ya fair
19:42:57	dansmith	I don't remember a thread on the ML, but there was discussion and documentation on the spec, which seemed to be the most fresh to me
19:43:54	dansmith	https://review.opendev.org/#/c/603955/11/specs/train/approved/nova-cyborg-interaction.rst@250
19:44:01	dansmith	this ^
19:44:23	mriedem	wacka wacka http://lists.openstack.org/pipermail/openstack-discuss/2019-June/thread.html#6979
19:44:33	dansmith	the discussion basically says either can do it, but it's hard to do it from conductor and then catch the event from compute
19:44:34	dansmith	that is not a problem
19:46:01	dansmith	mriedem: looks like that ended with you still opting for as much in conductor as possible
19:46:16	dansmith	but no real summarizing agreement that it should be in compute
19:46:49	mriedem	in http://lists.openstack.org/pipermail/openstack-discuss/2019-June/007013.html i'm saying please create arqs in conductor and bind in compute, like john was trying to do with ports at one time
19:47:09	mriedem	bind generates the event if that's what is important
19:47:25	sean-k-mooney	isnt that what we agreed to do
19:47:33	dansmith	right, but bind is the thing that takes a long time,
19:47:41	dansmith	so doing that in conductor is what gets us the most gain
19:47:55	dansmith	the original email states the reason for moving is because of the eventing being hard, which is is not
19:48:03	mriedem	having not really any skin in this game, it doesn't really matter to me anyone re this cyborg series
19:48:13	mriedem	i agree the original email is mostly excuses
19:48:25	mriedem	minimal change to get a thing to sort of work as a poc
19:48:42	mriedem	to which i said essentially, yeah that's why we have a lot of turdy spaghetti code we've wanted to refactor for years
19:48:51	mriedem	and why artom calls nova fugly
19:49:19	dansmith	yeah, understand.. the reason for moving to compute based on the spec and that original email seemed to be excuses around eventing, which are not valid,
19:49:42	dansmith	so I'd prefer we revert back to the original plan which had good reasoning, which is to overlap the programming with all the other things we can do in parallel,
19:49:47	dansmith	which means bind at conductor
19:50:09	sean-k-mooney	they baically need to do the equvalent of double check locking in the driver. check if its bound start waiting for event, check if its bound again incase you mised it and if so cancel waiting if not wait
19:50:17	mriedem	does that include building a new interface into conductor so when the api receives the external event it can route it to conductor rather than compute?
19:50:40	dansmith	sean-k-mooney: did you see the explanation I linked above about how to do the check and event without the race?
19:50:47	dansmith	sean-k-mooney: they do not need to check twice
19:50:56	dansmith	mriedem: no it does not
19:51:16	dansmith	read this: https://review.opendev.org/#/c/631244/46/nova/compute/manager.py@2627
19:51:18	sean-k-mooney	no but i trust you when you say there is a way to do that
19:51:24	sean-k-mooney	but neighter woudl be hard to do
19:51:36	dansmith	neighter?
19:51:53	sean-k-mooney	neither
19:51:58	dansmith	oh
19:52:07	sean-k-mooney	ya that was more off then normal
19:52:12	dansmith	the double check doesn't actually help, AFAICT, it would have to be N checks i.e. polling
19:52:47	sean-k-mooney	well once you have register to wait for the event if you check the status and its not complete you can just wait
19:53:04	dansmith	sean-k-mooney: right, that's one check and that's what I mention in the above comment
19:53:19	dansmith	it's just that you need a new thing to be able to exit the wait after you have started if you know it is not necessary which is not easy today
19:53:41	dansmith	you said "equivalent of double check" so, I see what you mean now
19:53:46	sean-k-mooney	ah ok i was suggesting checking that api once ot skip that incase its already done but that is not needed
19:53:51	dansmith	yes
19:53:58	dansmith	skip is the thing we can't do without the patch I proposed
19:54:34	sean-k-mooney	gotch ya
19:56:06	mriedem	so tl;dr create and bind the arq in conductor, cast to compute to build, compute grabs the event lock and checks to see if it should wait based on the cyborg api - if so, wait, if not, cancel the wait since the event taint comin
19:56:23	dansmith	basically yeah
19:57:40	mriedem	i suppose it's out of the question waiting for the cyborg series to be rebased and integrated with said wait-or-cancel function
19:58:05	openstackgerrit	Matt Riedemann proposed openstack/nova stable/stein: Replace time.sleep(10) with service forced_down in tests https://review.opendev.org/696175
19:59:06	dansmith	mriedem: I've already proposed the patch, efried has already asked you to +W, and yes, the cyborg set will need to use it to get my +2 :)
20:00:55	dansmith	mriedem: or did you mean waiting to merge the new skip thing until the cyborg set is using it?
20:02:12	mriedem	i was hoping to see the latter before having to dig into the new thing, to show something using it successfully outside of unit tests - but i realize that might be awhile
20:02:46	dansmith	mriedem: oh that's fine, I don't care that much, but I think it's probably good to get that just landed ahead of time so the set doesn't get too much bigger..
20:03:00	dansmith	I'd also propose we land the test base refactor, once it gets unfscked
20:03:18	mriedem	i'm not sure why that test base refactor is even needed
20:03:32	mriedem	it's just noise imo
20:03:39	dansmith	me either, I haven't gotten that far yet, but someone else had a similar one up too Ithought
20:03:48	dansmith	was it mdbooth you said?

Earlier   Later