Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-07-10
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 https://review.openstack.org/#/c/563401/28/nova/notifications/objects/server_group.py@41
18:33:35 mriedem me too
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
18:48:37 openstackgerrit Matt Riedemann proposed openstack/nova stable/ocata: Use ironic-tempest-dsvm-ipa-wholedisk-bios-agent_ipmitool-tinyipa in tree https://review.openstack.org/581445
18:53:02 melwitt looking for a +W on this ironic driver change needed to solve a race during instance creates https://review.openstack.org/563722
18:55:45 efried Looks like jaypipes, dansmith, and johnthetubaguy have reviewed ^ in the past, so I'll stay away for now.
18:56:11 openstackgerrit Matt Riedemann proposed openstack/nova stable/queens: unquiesce instance after quiesce failure https://review.openstack.org/581451
19:04:14 openstackgerrit Eric Fried proposed openstack/nova master: Delete orphan nodes before updating resources https://review.openstack.org/579922

Earlier   Later