Posted	Nick	Remark
#openstack-nova - 2017-04-18
15:53:49	mriedem	it might need some tweaking, but the readme says you can run it against a single-node devstack
15:53:52	mriedem	so i'd probably start there
15:54:16	edleafe	mriedem: no, didn't see your reply yet
15:54:33	bauzas	edleafe: I agree with mriedem, I'm pretty sure our infra wouldn't like to have a job faking 1000 nodes for each patch
15:54:46	mriedem	bauzas: ed was working on an in-tree functional test i think
15:54:50	mriedem	which just simulates things in the db
15:54:53	mriedem	and runs the scheduler
15:54:57	edleafe	why wouldn't it fail? If you have inventory/allocations that don't meet the request, shouldn't it fail?
15:55:06	mriedem	edleafe: i mean the test doesn't really fail
15:55:14	mriedem	it's a perf test which is used to compare results
15:55:26	mriedem	so with an actual TestCase, what do you compare it to for the benchmark?
15:55:29	mriedem	that's all i mean
15:55:31	edleafe	And this wasn't going in-tree - it was more of a tool to test our assumptions
15:55:38	mriedem	edleafe: ok, that tool already exists
15:55:39	bauzas	mriedem: not sure I understand the reasoning behind running 1000 nodes
15:55:40	mriedem	we should re-use it
15:55:53	bauzas	and we already have all the infra for running functional tests
15:55:53	edleafe	it does?
15:56:33	bauzas	you can just castascall and run all your services with the fake driver
15:56:45	bauzas	plus all the fixtures
15:56:46	edleafe	bauzas: the question was whether claiming in scheduler or conductor was better when hitting the race situation
15:57:17	bauzas	edleafe: so, that's not needing a functional test - it's different
15:57:34	bauzas	what you need is to run a testbed for verifying some things
15:57:41	edleafe	bauzas: ok, functional "tool"
15:57:51	bauzas	you can write that using the existing fixtures
15:58:06	edleafe	mriedem: which tool are you referring to?
15:58:11	bauzas	but I think it should rather be something for the implementation :)
15:58:38	mriedem	edleafe:
15:58:39	mriedem	http://lists.openstack.org/pipermail/openstack-dev/2017-April/115553.html
15:58:48	bauzas	FWIW, I think discussing about whether the claim post should be in the conductor or the scheduler is an implementation detail that needs verification during the implementation phase
15:58:54	bauzas	I was actually planning to do so
15:59:27	mriedem	edleafe: https://github.com/cyx1231st/nova-scheduler-bench
15:59:37	mriedem	bauzas: that's why this came up
15:59:56	bauzas	honestly, I don't want to defer the spec acceptance by that
16:00:01	edleafe	mriedem: thanks - I'll dig into this
16:00:05	mriedem	i.e. is it better to retry in the scheduler where we already have the hosts, or conductor which requires the full retry
16:00:11	mriedem	bauzas: i don't either
16:00:15	bauzas	having the scheduler or the conductor calling the placement seems an implementation detail
16:00:18	edleafe	bauzas: you could simply add "to be determined"
16:00:37	bauzas	edleafe: I was *on* the spec given I made a promise to mriedem :p
16:00:40	mriedem	bauzas: i'd like to say we're going to start with retry in scheduler, but mention an alternative
16:00:49	bauzas	mriedem: jinxed
16:00:53	mriedem	and say both can be tested
16:01:21	dansmith	mriedem: that pushes the claim to placement into the scheduler too
16:01:27	mriedem	dansmith: i know
16:01:31	dansmith	hmm.
16:01:43	mriedem	that's where we have the list of filtered and weighed hosts
16:01:48	dansmith	I know
16:01:52	mriedem	so if we have to retry, it's faster
16:02:01	mriedem	well, theoretically it's faster
16:02:11	dansmith	or it might be much worse, depending on how you race
16:02:22	dansmith	we could also, you know, return more information from that scheduler interface
16:02:24	mriedem	sure, if host[1] and host[2] in the list are now used up too
16:02:29	dansmith	like "here are the top 5, weighed in order"
16:02:30	mriedem	dansmith: i thought about that yesterday too
16:02:42	mriedem	select_destinations could just return the hosts
16:02:52	dansmith	...yeah
16:03:08	mriedem	i'd return whatever CONF.max_attempts is
16:03:11	bauzas	there are 2 implementation possibilites
16:03:13	dansmith	it already returns multiples now if num_instances is >1, so just return N plus some.. yeah
16:03:32	bauzas	either we put POST in consume_from_request()
16:03:50	bauzas	or, we do POST right after scheduler returns a destination to conductor
16:03:50	openstack	Launchpad bug 1683858 in OpenStack Compute (nova) "Allocation records do not contain overhead information" [High,Triaged]
16:03:50	mriedem	jaypipes: cdent: bauzas: https://bugs.launchpad.net/nova/+bug/1683858
16:04:08	mriedem	bauzas: the post isn't the problem, it's where to retry
16:04:11	jaypipes	mriedem: thx
16:04:19	bauzas	either way, like I discussed with mriedem, we need to DELETE allocations in the conductor on a reschedule/move operation
16:04:40	bauzas	mriedem: like I said to you, the scheduler doesn't know whether it's a move or a single boot
16:05:05	bauzas	for it, it just answers a specific question which is "which host(s) matches my request"
16:05:24	bauzas	*why* we ask the scheduler this question is totally unrelated to it
16:05:41	edleafe	bauzas: either way, the resources on the selected host will have to be claimed
16:06:00	bauzas	edleafe: in the PS I'm writing, I'm decoupling the DELETE and the POST
16:06:56	bauzas	edleafe: in the conductor, I'm planning to GET allocations for that instance and DELETE those if existing, before calling scheduler which would POST the new allocations for that instance
16:07:09	openstackgerrit	Jon Bernard proposed openstack/nova master: Add tempest-dsvm-ceph-rc https://review.openstack.org/456292
16:08:57	efried	sfinucan Got a sec to talk through https://review.openstack.org/#/c/455710/8/nova/api/openstack/placement/handler.py@184 ? (andymccr cdent)
16:09:26	sfinucan	efried: Sure
16:09:40	efried	sfinucan First, assertion that we don't need .get(..., '')
16:09:57	efried	Whyzat? Is something upstack guaranteeing that var will be present (even if empty) in the env?
16:10:55	sfinucan	efried: '.get' will return 'None' even if the key isn't a dictionary
16:11:06	sfinucan	which is different from the dict[key] notation
16:11:18	sfinucan	*isn't in a given dictionary
16:11:38	efried	mm, fair
16:12:04	cdent	this has been through enough iterations I'm not sure if we haven't lost the original point
16:12:09	cdent	or maybe points
16:13:03	andymccr	sfinucan: so if i understand your point correctly, the subsequent .get()'s after line 183 are not required in that format since we are certain by now that the var does exist.
16:13:15	andymccr	although the last one is probably required still since that's for CONTENT_TYPE rather than CONTENT_LENGTH?
16:13:18	efried	I wasn't a fan of leaving the int() to raise TypeError/ValueError, but that was the original behavior that supposedly wasn't wanting to be changed.
16:13:36	efried	andymccr Right, but leave out the , ''
16:13:47	cdent	efried: no we don't want that. we don't want a lose exception to raise
16:13:53	cdent	loose
16:14:04	andymccr	efried: because it would return None anyway- ok cool i understand
16:14:05	efried	cdent Yeah, just thought we might want it to be a better exception
16:14:25	efried	cdent Like HTTPBadRequest
16:14:28	andymccr	i can add in a try/raise if needed
16:15:06	cdent	andymccr: my advice would be to start from a test that demonstrates the bug you're experiencing
16:15:15	cdent	make that, make it fail, then fix it
16:15:40	cdent	you can do that by faking an environ that you pass to the PlacementHandler class
16:15:44	cdent	(as a callable)
16:16:22	efried	Yeah, mahbad for suggesting all the , '' / , 0 stuff. There's some other builtin that raises if you don't use that second var (can't remember which, now) that I was confusing with dict.get().
16:17:32	andymccr	cdent: yeah my plan is to cover a few test cases there. i think it should be doable i just need to carve out some time. but to confirm when i propose the next patch set it would be better to have a try block around the int()

Earlier   Later