Posted	Nick	Remark
#openstack-nova - 2018-07-10
15:33:29	dansmith	that shouldn't be a problem 10 minutes later though
15:33:36	gibi	dansmith: this returns an empty list for the second scheduling https://github.com/openstack/nova/blob/c0350da4a1607d7aa113caceaefb5d29303c7eed/nova/objects/instance_group.py#L422
15:33:51	mdbooth	Just working on this patch: https://review.openstack.org/#/c/578846/ I called it out in the commit message: the cut/paste in there is horrible, but avoids a refactor. Am I going to get that landed as is, or do I need to look at refactoring the driver interface?
15:34:01	mriedem	gibi: and i think that's the bug melwitt is fixing
15:34:01	dansmith	gibi: oh on multi-cell?
15:34:05	dansmith	that needs to stripe
15:34:06	dansmith	yeah
15:34:24	gibi	don't we deploy with multi cell _by default_ ?
15:34:39	mdbooth	It goes without saying that I don't mind refactoring :) However, I'd also like to both land and backport the code.
15:34:55	mriedem	gibi: in devstack yes
15:34:58	dansmith	gibi: yeah, this would hit cell0 if your default is cell0, and always be empty
15:35:14	mriedem	it's not really 'multi cell' so much as superconductor mode in devstack and how the services are configured
15:35:30	gibi	let me rephrase. I our suggested deployment mode (cell_v2) the server group policies are broken
15:35:41	gibi	that is my understanding
15:35:50	dansmith	gibi: because your default connection is to cell0, which will never return the list of instances, yeah
15:36:03	gibi	dansmith: thanks, that explains what I see
15:36:13	dansmith	but striping across cells is what needs to happen, which is that bug afaik
15:36:48	gibi	dansmith: yeah. this is why offered my help to melwitt above on the fix
15:36:59	dansmith	gibi: are we in violent agreement? :)
15:37:06	gibi	dansmith: yes
15:37:48	gibi	I was not sure we are on the same page about the size of the problem. server groups are useless now
15:38:17	dansmith	gibi: if your controllers are pointing at your main cell by default it'll be fine, fwiw
15:38:24	dansmith	but it definitely needs changing
15:38:51	mriedem	gibi: so, if you want to help, we need a functional test in-tree that recreates the regression
15:39:03	mriedem	that's what i was -1 on the change for - the mock based unit tests had logic flaws in them
15:39:54	gibi	dansmith: how can a deployer change the default cell to cell1?
15:40:08	gibi	mriedem: ack. I offered a tempest test :)
15:40:10	dansmith	gibi: [database]/connection :)
15:41:05	gibi	dansmith: in the super conductor conductor conf?
15:41:17	dansmith	gibi: in any controller config
15:41:33	dansmith	gibi: you don't have to be running a superconductor layout with only one non-cell0 cell
15:41:58	mriedem	gibi: yeah, but we can't use that tempest change long-term because of the otherwise known race issue
15:42:28	gibi	dansmith: I see
15:42:32	gibi	dansmith: thanks
15:43:18	gibi	mriedem: is it racy even if the test waits for the first server to go to ACTIVE state before start creating the second one?
15:43:49	mriedem	gibi: in that case maybe not
15:43:59	mriedem	because per scheduling request, we should pull the set of instances per host
15:44:28	mriedem	https://github.com/openstack/nova/blob/master/nova/scheduler/host_manager.py#L781
15:44:49	mriedem	that's if the compute isn't sending it's instance info to the scheduler (which it's not in default devstack b/c we disable that ability)
15:45:38	gibi	OK, so the second scheduling will see the first instance sitting on the host
15:45:41	mriedem	which reminds me of https://review.openstack.org/#/c/569247/ but haven't had good large scale performance testing to see if that's justified
15:45:47	mriedem	gibi: it should yeah
15:46:09	gibi	mriedem: then I don't see how the currently proposed tempest test be racy
15:47:28	openstackgerrit	Jay Pipes proposed openstack/nova master: update project/user for consumer in allocation https://review.openstack.org/581139
15:49:03	mriedem	gibi: ok updated comments
15:50:15	gibi	mriedem: ack, I can cover both single and multi request cases in tempest
15:50:49	gibi	mriedem: if we have the tempest do you still require the functional test in melwitt's patch?
15:51:53	mriedem	maybe not
15:52:06	mriedem	does your tempest test also cover affinity?
15:52:42	mriedem	looks like it doesn't for separate requests
15:52:44	gibi	mriedem: both the affinity and the anti-affinity case fails now
15:53:00	gibi	mriedem: both uses the same helper to create the servers
15:53:50	gibi	mriedem: but in case of affinity we might want to fill the compute to see NoValid host
15:54:04	mriedem	that's what i was going to try with a functional test
15:54:19	mriedem	to set available vcpu inventory to 1 or something
15:54:34	gibi	mriedem: sure we have plenty of such functional tests already but they are not cell aware and passing
15:55:08	gibi	mriedem: https://github.com/openstack/nova/blob/master/nova/tests/functional/test_server_group.py
15:55:31	openstackgerrit	Matt Riedemann proposed openstack/nova master: Document differences and similaries between extra specs and hints https://review.openstack.org/581410
15:55:34	mriedem	right, i plan on writing a multi-cell affinity regression functional test
15:56:18	gibi	mriedem: https://github.com/openstack/nova/blob/master/nova/tests/functional/test_server_group.py#L292
15:57:27	gibi	mriedem: ohh there is one case where functional is needed. multiple non-cell0 cells case
15:57:43	gibi	mriedem: as in tempest we have only cell0 and cell1
15:58:02	mriedem	yes, that's what i've been saying :)
15:58:14	mriedem	when i say 'multi-cell' i mean multiple non-cell0 cells
15:58:15	gibi	mriedem: now I understand ;)
15:58:28	mriedem	i don't consider cell0 a 'real' cell since it doesn't contain compute hosts
16:00:59	gibi	mriedem: OK, I will extend the tempest test tomorrow. Thanks for the explanation
16:05:41	openstackgerrit	Eric Fried proposed openstack/nova master: update project/user for consumer in allocation https://review.openstack.org/581139
16:06:28	openstackgerrit	karim proposed openstack/nova master: Handle rebuild of instances with image traits https://review.openstack.org/569498
16:13:50	mriedem	dansmith: i found another candidate for your long_rpc_timeout https://bugs.launchpad.net/cinder/+bug/1739482
16:13:50	openstack	Launchpad bug 1739482 in Cinder "test_snapshot_backup fails to build backup due to MessagingTimeout" [Medium,Confirmed]
16:14:13	dansmith	mriedem: that's on the cinder side though yeah?
16:14:16	mriedem	yup
16:14:21	mriedem	but i'd do the exact same thing there
16:14:29	mriedem	ala oslo.incubator style
16:14:59	dansmith	aye
16:19:24	mriedem	dansmith: so in https://review.openstack.org/#/c/563375/35/nova/objects/instance_group.py it seems weird that _rules is nullable but we default to {} if it's not set in the db
16:19:38	mriedem	so i can create a group with rules=None but then what i'll get back is group.rules == {}
16:19:52	dansmith	mriedem: so, the reason I left it that way is because he had the first patch passing None from the api
16:20:00	dansmith	but now that I think about it with a fresh head,
16:20:06	dansmith	that can and maybe should just pass {} there
16:20:41	mriedem	why {} over None?
16:21:09	mriedem	just so callers don't hit NoneType on access?
16:21:09	dansmith	I tend to prefer things like dicts to be empty instead of None, just because it means you always have to check to see if the thing is none, then check to see if your thing is in the dict, and then check to see if the value is what you want
16:21:39	dansmith	that's why I was asking for it to be {} on return, but we should be consistent either way
16:22:01	mriedem	so if we just pass in {} we can drop nullable=true
16:22:19	dansmith	yeah i think that was the only place
16:22:26	mriedem	there is one in the request spec as well
16:22:37	mriedem	https://review.openstack.org/#/c/563375/35/nova/objects/request_spec.py
16:22:39	dansmith	I remember de-null-ing it but then flipping it back when the api test failed
16:22:40	dansmith	okay
16:22:54	dansmith	I'll pull it down and make that change and see what breaks
16:23:55	mriedem	this is the only thing (later in the series) i know of that will
16:23:56	mriedem	https://review.openstack.org/#/c/563401/25/doc/notification_samples/common_payloads/ServerGroupPolicyPayload.json@7
16:24:08	mriedem	but that's an easy change
17:14:31	dansmith	mriedem: hmm, I didn't get a failure on that
17:16:23	dansmith	er, wait,
17:16:31	dansmith	maybe that would manifest in the notification patch but not in the last one
17:16:45	dansmith	oh, that's what you linked to
17:20:02	dansmith	hrm

Earlier   Later