Posted	Nick	Remark
#openstack-nova - 2017-03-09
15:06:01	mdbooth	dansmith: That was another reason I didn't want to go all the way down this rabbit hole. I expect there are a bunch of tricky cases.
15:06:12	dansmith	mdbooth: there are a ton
15:06:31	mdbooth	dansmith: However, I was hoping that by removing the whole side-effect of changing the global context there might be significantly less
15:06:41	dansmith	mdbooth: I think you might be surprised
15:07:03	sfinucan	jaypipes: I thought the point of those tests was to fake out libvirt?
15:07:12	mdbooth	dansmith: Ok, I'll limit to get_all() initially and see how it looks.
15:07:14	mdbooth	Thanks for looking.
15:07:28	jaypipes	sfinucan: see the functional test failure :)
15:07:30	cdent	coreycb: I think I've got another one. Is it considered a bug for the service.conf files to have uncommented config settings?
15:08:11	coreycb	cdent, not necessarily. some of the packages set defaults.
15:08:24	sfinucan	jaypipes: Well that doesn't look right :D
15:08:36	cdent	coreycb: in this case it is that os_region_name in the [placement] section is being set
15:08:50	sfinucan	Guess we can mock out that version check too
15:08:55	dansmith	mdbooth: fwiw, we already had a discussion about whether or not to fork the context in the objects that are tied to cells and decided on this approach to start
15:09:09	dansmith	mdbooth: one thing to remember is that if you send an object over rpc, it resets the context on all the objects in the stack
15:09:25	dansmith	mdbooth: so if you make a call over to conductor, for anything like boot or resize, contexts get reset anyway
15:09:33	coreycb	cdent, that may not need to be set. we set defaults so that the service will can start after the package is installed.
15:10:13	openstackgerrit	Dan Smith proposed openstack/nova master: WIP: Temporarily untarget context when deleting from cell0 https://review.openstack.org/443403
15:10:21	cdent	coreycb: the reason I noticed was because the docs I was testing changed it, i made all those changs at the top of the section, and then realized it was going to conflict with something that's set further below
15:10:47	cdent	coreycb: ah, okay, yeah, that makes sense then
15:10:53	cdent	it won't start without that
15:10:57	cdent	(there's no default)
15:11:42	mriedem	dansmith: ftr, i did give you the business, yes
15:11:44	mriedem	:)
15:11:55	dansmith	mriedem: yeah, I just totally wasn't paying attention when I wrote that :)
15:11:59	dansmith	mriedem: see my rev
15:12:00	dansmith	?
15:13:02	mriedem	yes
15:13:08	mriedem	wait, what?
15:13:26	mriedem	oh i see
15:13:53	dansmith	actually, I have to fix target_cell too
15:13:58	mriedem	dansmith: you missed on
15:13:59	mriedem	*one
15:14:00	dansmith	oh no
15:14:01	dansmith	it works
15:14:07	mriedem	you missed one in the exception block
15:14:09	dansmith	ah yeah
15:14:13	dansmith	just a sec
15:14:31	openstackgerrit	Dan Smith proposed openstack/nova master: WIP: Temporarily untarget context when deleting from cell0 https://review.openstack.org/443403
15:14:48	openstackgerrit	Balazs Gibizer proposed openstack/nova master: Using max api version in notificationt sample test https://review.openstack.org/443686
15:15:32	coreycb	cdent, ok. we're considering loosening the restriction of having to start services by default because it can cause issues particularly with deployment tools.
15:15:54	dansmith	mriedem: so we should be able to write a functional test for this that checks the quota in each after delete right?
15:15:56	coreycb	cdent, but we haven't done that yet. if we did i think we'd just use the default config files.
15:16:20	cdent	coreycb: it's a reasonable thing to do for machines being directly used by humans but not so much in this case :)
15:16:40	mriedem	dansmith: yes
15:16:55	mriedem	that's the plan
15:17:00	mriedem	the tricky thing is going to be,
15:17:00	coreycb	cdent, yeah :)
15:17:16	mriedem	we only need one of these fixes for the backport to ocata
15:17:24	mriedem	so the functional test won't pass until the top patch in pike
15:17:29	mriedem	but it will work for the bottom patch in ocata
15:17:42	mriedem	but re-ordering the changes in master doesn't make sense to me
15:17:49	dansmith	okay
15:18:02	mriedem	so i'm thinking maybe the functional test is in a separate patch
15:18:03	mriedem	not sure
15:18:09	dansmith	yeah
15:22:06	mriedem	so i think the easy thing i can do first is create a functional regression patch that goes at the bottom of the series and recreates the failure, and asserts the error,
15:22:10	mriedem	then the fix patches on top of that,
15:22:31	mriedem	and the top fix patch changes the functional test to show it working
15:22:35	mriedem	that was my plan anyway
15:22:39	dansmith	I was just looking at the tests
15:22:44	dansmith	give me a few minutes
15:23:00	dansmith	we don't really have a way to query into the reservation stuff to look, AFAICT, but I think I can hack something in at least to check
15:23:09	mriedem	well,
15:23:16	mriedem	for a functional test i think this is pretty straightforward,
15:23:19	mriedem	we have cell1 and cell0,
15:23:25	mdbooth	dansmith: Incidentally, https://review.openstack.org/#/c/443403/ is pretty much what I was talking about with the side-effect thing.
15:23:29	mriedem	we disable nova-compute or do something to make scheduling fail,
15:23:39	mriedem	create the server and show it goes into ERROR state,
15:23:47	dansmith	mdbooth: it's not at all, since this is a problem across multiple requests
15:23:49	mriedem	assert we have quota of instances=9 (default-1)
15:23:55	mriedem	then delete the instance,
15:24:03	mriedem	and assert that quota of instances=10 (but it won't be until the fix)
15:24:12	mriedem	it's basically the same recreate steps as in the bug report
15:24:35	dansmith	mriedem: I guess I'm lost in the db api on terminology, knowing which things are returning limits and which are calculated by limit and usage
15:24:44	dansmith	mriedem: but yeah, that's what I'm doing
15:24:48	mriedem	the functional test just hits the rest api
15:24:54	mriedem	like novaclient would
15:25:04	mriedem	i really don't want to be going into the actual databases
15:25:32	mriedem	i was going to do this in nova/tests/functional/regressions
15:25:54	dansmith	mriedem: well, you're not really validating that exactly what you think is happening really is, but okay
15:26:09	dansmith	I'll leave it to you then
15:26:36	mriedem	as long as the regression test asserts the failure (the recreate), and the fix has to change the test to show it now passing, i think that's what we care about
15:27:02	dansmith	I just want to see them in the wrong database, but okay
15:27:33	dansmith	the quota stuff is so opaque to me that I'd want to see that we're out of sync for the reason we think and not one of the other reasons quotas gets out of sync
15:27:46	dansmith	but that's fine, you're far more familiar, so go for it
15:28:04	mriedem	why did you change set_target_cell?
15:28:09	mriedem	we're calling target_cell and passing None
15:28:35	dansmith	target_cell calls set_target_cell
15:29:42	mriedem	gotcha
15:32:05	johnthetubaguy	mdbooth: I am looking at your comments on migrate_volume_completion
15:32:30	mdbooth	johnthetubaguy: I spent half a day or so reading the cinder side code for that, btw
15:32:32	johnthetubaguy	mdbooth: I don't feel like I am understanding what that is trying to do, and if it should die/get replaced in this new model
15:32:43	mdbooth	I'll never get that back
15:32:50	johnthetubaguy	yeah, I did that about 6-8 months ago, and its wondered out my head again
15:33:36	mdbooth	johnthetubaguy: I think it needs to continue to exist, because cinder needs to know that the copy is complete so it can cleanup the old volume.
15:34:01	johnthetubaguy	mdbooth: yeah, we totally need that bit
15:34:04	mdbooth	However, I suspect it could similarly exist in a new form
15:34:09	johnthetubaguy	yeah, that
15:34:56	mdbooth	johnthetubaguy: It would be great if the new form included a flag for 'we didn't do the copy'

Earlier   Later