| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-10 | |||
| 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 | |
| 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 | |