Posted	Nick	Remark
#openstack-nova - 2018-07-10
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
18:17:27	mriedem	could all be flat
18:18:04	mriedem	gibi wanted both the old policies field and new policy payload in the notification for compat since notification consumers can't request a specific versoin of the notification payload
18:18:08	mriedem	but that's not the same in the rest api
18:18:19	dansmith	yeah, I dunno
18:18:37	dansmith	just not sure I see the point of the nesting in either the api or the notification object
18:18:43	dansmith	it's not bad, it just seems unnecessary
18:18:50	dansmith	just another object and hash to track
18:19:29	dansmith	anyway, if you want to just steam on I'll play along
18:26:02	mriedem	i'm ok with making them flat if we want
18:26:37	mriedem	thinking about stuff we nest in the rest api for servers, those are things like security groups, volumes, ports, etc - things that have their own resources in the api
18:26:50	mriedem	server group policies wouldn't count like that since they aren't separate resources
18:26:55	dansmith	yeah
18:27:26	mriedem	gibi is probably the only other person that would have an opinion and he's probably gone by now
18:28:14	mriedem	should we just wait and ask yikun what he thinks? if he doesn't care, then we can flatify tomorrow
18:28:28	dansmith	if you're cool with that
18:28:33	mriedem	yeah i'm fine with it
18:28:49	mriedem	need to review Kevin_Zheng's abort queued live migration stuff today anyway
18:29:06	mriedem	i'll drop my +2 on yikun's notification patch
18:30:27	dansmith	aight
18:30:32	dansmith	I'll comment
18:31:54	openstackgerrit	Eric Fried proposed openstack/nova master: Tighten up ReportClient use of generation https://review.openstack.org/556669
18:33:35	mriedem	me too
18:33:35	mriedem	https://review.openstack.org/#/c/563401/28/nova/notifications/objects/server_group.py@41
18:33:42	mriedem	gibi: fyi ^
18:46:05	openstackgerrit	Matt Riedemann proposed openstack/nova stable/pike: Use ironic-tempest-dsvm-ipa-wholedisk-bios-agent_ipmitool-tinyipa in tree https://review.openstack.org/581444

Earlier   Later