Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-07-10
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
17:20:03 dansmith no fail
17:20:33 dansmith maybe I'll push this up and you can look/comment
17:20:49 openstackgerrit Dan Smith proposed openstack/nova master: Add policy to InstanceGroup object https://review.openstack.org/563375
17:20:50 openstackgerrit Dan Smith proposed openstack/nova master: Add policy field to ServerGroup notification object https://review.openstack.org/563401
17:20:51 openstackgerrit Dan Smith proposed openstack/nova master: Change the ServerGroupAntiAffinityFilter to adapt to new policy https://review.openstack.org/571166
17:20:52 openstackgerrit Dan Smith proposed openstack/nova master: Adapt _validate_instance_group_policy to new policy model https://review.openstack.org/571465
17:20:53 openstackgerrit Dan Smith proposed openstack/nova master: Microversion 2.64 - Use new format policy in server group https://review.openstack.org/567534
17:26:04 mriedem dansmith: did you run just unit tests?
17:26:09 mriedem the notification sample will fail functional
17:27:12 dansmith I ran functional, but maybe not on the actual notification patch, looking at my history
17:28:40 openstackgerrit Matt Riedemann proposed openstack/nova master: Heal allocations with incomplete consumer information https://review.openstack.org/574488
17:28:41 openstackgerrit Matt Riedemann proposed openstack/nova master: Refactor _heal_instances_in_cell https://review.openstack.org/577896
17:28:42 openstackgerrit Matt Riedemann proposed openstack/nova master: Use consumer generation in _heal_allocations_for_instance https://review.openstack.org/577905
17:29:30 dansmith mriedem: yeah, I did and just re-ran with "notification" regex and all passed
17:29:40 dansmith and it's running notification sample tests...
17:30:00 dansmith nova.tests.functional.notification_sample_tests.test_server_group.TestServerGroupNotificationSample.test_server_group_add_member
17:30:33 dansmith (as an example)
17:32:38 mriedem hmm
17:33:09 mriedem oh i see why
17:33:15 mriedem https://review.openstack.org/#/c/563375/35..36/nova/objects/instance_group.py@142
17:33:27 mriedem that should be removed or changed to {}
17:33:40 dansmith oh
17:33:55 dansmith sho nuf
17:34:16 mriedem and then we can make the rules field on the payload object no longer nullable as well https://review.openstack.org/#/c/563401/26/nova/notifications/objects/server_group.py@68
17:35:38 dansmith lemme write a test for that behavior
17:36:11 mriedem yeah that group = objects.InstanceGroup(rules={}) && self.assertEqual({}, group.rules)

Earlier   Later