Posted	Nick	Remark
#openstack-nova - 2017-11-30
15:02:28	efried	mriedem http://logs.openstack.org/32/523932/2/check/openstack-tox-functional/05f8149/testr_results.html.gz
15:05:37	openstackgerrit	Jackie Truong proposed openstack/python-novaclient master: Microversion 2.56 - Add trusted_image_certificates https://review.openstack.org/500396
15:06:28	takashin	a patch to create specs directory for Rocky https://review.openstack.org/#/c/514101/
15:09:13	openstackgerrit	Matthew Booth proposed openstack/nova master: Simplify BDM boot index checking https://review.openstack.org/524208
15:10:17	sdague	what's up?
15:10:32	sdague	efried: can you summarize the issue, maybe I can help
15:11:25	efried	sdague If report client tries to ensure a RP exists, and gets any error, it returns None (rather than raising an exception) but nobody is checking for that.
15:11:45	mriedem	efried: these are a single cell test, using the SingleCellSimple
15:11:45	efried	sdague In this test: http://logs.openstack.org/32/523932/2/check/openstack-tox-functional/05f8149/testr_results.html.gz we're hitting that code path
15:12:04	mriedem	efried: looking at HypervisorsSampleJson233Tests, it's creating a 2nd service
15:12:10	mriedem	but not setting unique nodes
15:12:42	efried	sdague As long as we keep the `return None` in there, the test works; but if I raise an exception, the test fails because, as mriedem is saying, it's creating a second service, which creates a second ComputeNode with the same name but different UUID.
15:12:51	mriedem	_IntegratedTestBase, the base class, creates a compute service, and the HypervisorsSampleJson test creates a compute service, but i think they have the same node name
15:13:01	efried	sdague When we try to create that in placement, we get that 409 because the *name* conflicts.
15:13:10	sdague	mriedem: yeh, that's probably something we should prevent on the test side
15:13:21	efried	mriedem Yes, they're both called fake-mini.
15:13:22	mriedem	so you need to put this in HypervisorsSampleJson when creating the 2nd service:
15:13:30	mriedem	fake.set_nodes(['host1'])
15:13:35	mriedem	self.addCleanup(fake.restore_nodes)
15:13:36	efried	I'm confused as to why it's not `host1`
15:13:40	mdbooth	stephenfin: How sure are you of assert(), btw?
15:13:53	mriedem	efried: because the fake driver is pullin the node name from CONF.host
15:13:58	mriedem	if you don't use fake.set_nodes
15:14:09	stephenfin	mdbooth: Not hugely sure. I just recall being told not to use it at some point in the past
15:14:11	efried	But as far as I can see, CONF.host *should* be `host1` and `host2`
15:14:55	mdbooth	stephenfin: I think I'm using it correctly in that patch, but there may be practical considerations. e.g. if we disable it in CI.
15:14:57	mriedem	efried: ala https://github.com/openstack/nova/blob/master/nova/tests/functional/test_servers.py#L1283
15:15:18	mdbooth	If it's disabled in production, we don't really care.
15:15:32	efried	mriedem Okay, thanks, trying...
15:15:59	cdent	efried: I can't remember, is the rp ending up named fake-mini or 'host?' ?
15:16:07	efried	cdent fake-mini
15:16:17	mdbooth	mriedem: Do you happen to remember, btw: all(el + 1 == l[i + 1] for i, el in enumerate(l[:-1])) ?
15:16:25	mriedem	melwitt: so i've got this code now http://paste.openstack.org/show/627877/
15:16:37	mdbooth	You chose to add a comment to it for some reason :)
15:16:44	mriedem	melwitt: which fails with this http://paste.openstack.org/show/627878/
15:16:54	mriedem	so i'm creating host2 in cell2 but when i try to pull host2 out of cell2, it's not found
15:17:06	mriedem	i'm assuming that's because of the CellDatabase fixture defaulting to cell1
15:17:15	mriedem	which is weird since i'm targeting the context specifically to cell2
15:17:26	mriedem	but i can replay this on top of your fixture cleanup patch and see if that fixes it
15:18:04	mriedem	mdbooth: ?
15:18:35	mriedem	mdbooth: if i added a comment it's because i oculdn't understand ndipanov's code and had to figure it out at one point, and left a comment for posterity
15:19:02	mdbooth	mriedem: Not important. I was doing something else and came across it. It made my head hurt :) I submitted this throw-away: https://review.openstack.org/#/c/524208/
15:20:09	mdbooth	I assume this was very X of this patch for some large value of X, and this complexity made sense at some version <X
15:20:16	mdbooth	s/very/version/
15:21:27	mdbooth	I was actually trying to work out where we set the boot index of a root disk to 0 if it wasn't done explicitly in the api call
15:21:30	mdbooth	still haven't found it
15:21:50	openstackgerrit	Matt Riedemann proposed openstack/nova master: Add instance action record for shelve_offload instances https://review.openstack.org/523653
15:23:31	openstackgerrit	Merged openstack/nova stable/pike: Set regex flag on ostestr command for osprofiler tests https://review.openstack.org/510226
15:23:45	mriedem	melwitt: that didn't seem to fix it, still can't find host2 in cell2
15:24:31	tetsuro	Hi all, I'd like you to review https://review.openstack.org/#/c/465160/
15:26:11	tetsuro	Currently, VM with NUMA goes into a silent error if the virt_type doesn't support it.
15:26:21	tetsuro	So, this patch is to raise an error explicitly if NUMA Topology not supported,
15:49:06	mriedem	2017-11-30 10:47:07,831 ERROR [nova.tests.functional.test_servers] Failed to find host host2 in cell cell2
15:49:06	mriedem	2017-11-30 10:47:07,834 ERROR [nova.tests.functional.test_servers] Found host host2 in other cell cell1
15:49:18	mriedem	yeah, so for whatever reason, even if i create host2 in cell2, it only ever comes back from cell1
15:50:29	dansmith	mriedem: I commented on that migration sorting deal
15:51:13	dansmith	I think I deserve a cookie and a nap now
15:52:30	mriedem	heh, thanks, looking
15:52:34	mriedem	i did lose some sleep over this last night
15:52:46	mriedem	this and that god forsaken stray cat
15:53:31	dansmith	hehe
16:00:12	mriedem	dansmith: replied,
16:00:23	mriedem	this doesn't deal with the global/local marker issue either,
16:00:24	efried	mriedem Thanks for the pointer. After adding in the suggested code and finding & fixing the template files, I got the tests passing. <== cdent jaypipes sdague FYI
16:00:31	mriedem	but i'm admittedly a bit lost on the local marker logic and how that all works
16:00:47	mriedem	dansmith: i mentioned the cell-encoded marker idea too, but not entirely sure how we can use that to fix the local marker thing
16:01:05	mriedem	if you look at PS21, he had a near exact copy of the instance list stuff with the global/local marker
16:01:48	dansmith	mriedem: right, which is what I called out with him using the marker uuid in all the cells
16:02:05	ildikov	mriedem: meeting on #openstack-meeting-cp if you have a few minutes
16:03:34	mriedem	ildikov: you must have missed my o/
16:03:54	ildikov	mriedem: you must've done it while I was typing here :)
16:04:20	dansmith	mriedem: the cell encoding in the marker could help in a few cases:
16:04:33	dansmith	mriedem: if we encode the cell, then we can avoid looking in all cells for the marker uuid as the first step
16:04:47	mriedem	yup, i at least understand that part so far
16:05:36	dansmith	mriedem: if we encoded the cell *and* the value we're sorting on of the marker, then we could find the marker in each cell to restart the sort from without having to first look up the global marker in the cell it came from
16:05:51	dansmith	but I think you said none of those are options without retooling the api stuff anyway,
16:05:55	dansmith	so probably not worth it
16:06:23	mdbooth	mriedem: In response, I could change the change id of my patch if you like?
16:06:42	dansmith	I was hoping to not have to use the full instance_list approach if we had a much limited set of sort criteria and stuff,
16:06:54	dansmith	but if we're going to implement the full keys/dirs approach at the bottom, then maybe we need to
16:07:14	mdbooth	I'd forgotten it, tbh, but my patch is a whole lot more thorough. It deals with the transition to mandatory bdm uuid explicitly.
16:07:16	mriedem	mdbooth: ? just cleanup dan's old patches
16:07:16	dansmith	I hate to complicate the instance_list stuff by genericifying the bits we can do, especially if this is the only thing we'll ever need to use it for
16:07:38	mriedem	mdbooth: adding uuids to things is a pretty standard formula now, we've done it in several other objects and talbes
16:07:40	mriedem	*tables
16:07:46	mriedem	so i'm not sure what more needs to be done
16:07:48	dansmith	mdbooth: you can't add the unique constraint in that migration, AFAIK
16:08:03	mdbooth	mriedem: Right, I cribbed off the migration uuid patch.
16:08:08	mriedem	bdm.uuid must start as nullable
16:08:12	mdbooth	mriedem: However, dansmith 's patch is way older than that
16:08:24	mriedem	it can't be that far of
16:08:25	mriedem	*off
16:08:38	mriedem	because dan wrote the migration one, and the services one was based on the aggregates and compute nodes ones, which dan wrote as well
16:08:41	mdbooth	mriedem: Yes, it does. However my patch ensures that uuid is always set on read
16:08:59	mdbooth	Seriously, some bits of my patch are very similar to the old patch
16:09:06	mdbooth	Some are cribbed off the newer stuff
16:09:12	mdbooth	It amounts to the same thing
16:09:21	dansmith	mdbooth: I thought you forgot about the old patch? :)
16:09:29	mdbooth	dansmith: I did :)
16:09:29	dansmith	anyway, keeping the same review is important for historyand context, IMHO

Earlier   Later