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