Posted	Nick	Remark
#openstack-nova - 2018-07-10
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	openstack	Launchpad bug 1739482 in Cinder "test_snapshot_backup fails to build backup due to MessagingTimeout" [Medium,Confirmed]
16:13:50	mriedem	dansmith: i found another candidate for your long_rpc_timeout https://bugs.launchpad.net/cinder/+bug/1739482
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	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:09	mriedem	just so callers don't hit NoneType on access?
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)
17:42:42	openstackgerrit	Dan Smith proposed openstack/nova master: Add policy to InstanceGroup object https://review.openstack.org/563375
17:42:43	openstackgerrit	Dan Smith proposed openstack/nova master: Add policy field to ServerGroup notification object https://review.openstack.org/563401
17:42:44	openstackgerrit	Dan Smith proposed openstack/nova master: Change the ServerGroupAntiAffinityFilter to adapt to new policy https://review.openstack.org/571166
17:42:45	openstackgerrit	Dan Smith proposed openstack/nova master: Adapt _validate_instance_group_policy to new policy model https://review.openstack.org/571465
17:42:46	openstackgerrit	Dan Smith proposed openstack/nova master: Microversion 2.64 - Use new format policy in server group https://review.openstack.org/567534
17:42:51	dansmith	okay here we go
17:44:07	mriedem	pep8 brotha
17:45:05	dansmith	come on
17:45:33	mriedem	want me to tag in?
17:45:56	dansmith	oh that's not my fault.. no I can just fix and push easy
17:46:02	mriedem	https://review.openstack.org/#/c/563401/27/nova/notifications/objects/server_group.py@68
17:46:06	mriedem	also ^
17:48:18	dansmith	do I need to re-run tests after snipping that out?
17:48:23	dansmith	the nullable thing
17:48:51	dansmith	ah maybe hash will change?
17:50:20	melwitt	gibi, mriedem: my bug fix got stalled on the functional test. I wasn't able to recreate the bug in a functional test so far
17:53:01	openstackgerrit	Dan Smith proposed openstack/nova master: Add policy to InstanceGroup object https://review.openstack.org/563375
17:53:02	openstackgerrit	Dan Smith proposed openstack/nova master: Add policy field to ServerGroup notification object https://review.openstack.org/563401
17:53:03	openstackgerrit	Dan Smith proposed openstack/nova master: Change the ServerGroupAntiAffinityFilter to adapt to new policy https://review.openstack.org/571166
17:53:04	openstackgerrit	Dan Smith proposed openstack/nova master: Adapt _validate_instance_group_policy to new policy model https://review.openstack.org/571465
17:53:05	openstackgerrit	Dan Smith proposed openstack/nova master: Microversion 2.64 - Use new format policy in server group https://review.openstack.org/567534
18:00:43	mriedem1	i assume bauzas is in his futbol chamber
18:10:11	mriedem	dansmith: ok i'm +2 up that stack to the point of the rest api change, reviewing that during this game
18:11:16	dansmith	mriedem: okay we might want to find someone else to do that policy one since it's like half mine now
18:11:21	dansmith	but I'll hit the rest
18:13:58	dansmith	mriedem: hmm, so I wonder if that notification one should have been squashed into the main servergrouppayload like the objects were?
18:14:23	dansmith	I was just mechanically updating it but hadn't really thought about it
18:14:24	mriedem	i thought about that yesterday
18:14:46	mriedem	and thought the separate payload isn't bad since it's the singular policy and the rules,
18:14:56	mriedem	which in the rest api you just get a singular policy and (eventually) rules
18:15:01	mriedem	so if we squashed,
18:15:10	mriedem	the notification payload would have policies (deprecated list), policy and rules
18:15:20	mriedem	so the nested object for the payload seems ok to me
18:15:27	dansmith	so this mirrors more of what the api does?
18:15:28	mriedem	doesn't matter to me much either way
18:15:38	mriedem	the api will be flat
18:15:52	mriedem	well, sorry, it's not flat
18:15:53	mriedem	https://review.openstack.org/#/c/567534/28/doc/api_samples/os-server-groups/v2.64/server-groups-get-resp.json
18:16:00	mriedem	yes it mirrors the rest api
18:16:12	dansmith	hmm
18:17:21	mriedem	we don't have to model it that way in the notification or api of course

Earlier   Later