| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-10 | |||
| 15:53:00 | gibi | mriedem: both uses the same helper to create the servers | |
| 15:53:50 | gibi | mriedem: but in case of affinity we might want to fill the compute to see NoValid host | |
| 15:54:04 | mriedem | that's what i was going to try with a functional test | |
| 15:54:19 | mriedem | to set available vcpu inventory to 1 or something | |
| 15:54:34 | gibi | mriedem: sure we have plenty of such functional tests already but they are not cell aware and passing | |
| 15:55:08 | gibi | mriedem: https://github.com/openstack/nova/blob/master/nova/tests/functional/test_server_group.py | |
| 15:55:31 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Document differences and similaries between extra specs and hints https://review.openstack.org/581410 | |
| 15:55:34 | mriedem | right, i plan on writing a multi-cell affinity regression functional test | |
| 15:56:18 | gibi | mriedem: https://github.com/openstack/nova/blob/master/nova/tests/functional/test_server_group.py#L292 | |
| 15:57:27 | gibi | mriedem: ohh there is one case where functional is needed. multiple non-cell0 cells case | |
| 15:57:43 | gibi | mriedem: as in tempest we have only cell0 and cell1 | |
| 15:58:02 | mriedem | yes, that's what i've been saying :) | |
| 15:58:14 | mriedem | when i say 'multi-cell' i mean multiple non-cell0 cells | |
| 15:58:15 | gibi | mriedem: now I understand ;) | |
| 15:58:28 | mriedem | i don't consider cell0 a 'real' cell since it doesn't contain compute hosts | |
| 16:00:59 | gibi | mriedem: OK, I will extend the tempest test tomorrow. Thanks for the explanation | |
| 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 | mriedem | dansmith: i found another candidate for your long_rpc_timeout https://bugs.launchpad.net/cinder/+bug/1739482 | |
| 16:13:50 | openstack | Launchpad bug 1739482 in Cinder "test_snapshot_backup fails to build backup due to MessagingTimeout" [Medium,Confirmed] | |
| 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 | mriedem | just so callers don't hit NoneType on access? | |
| 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: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 | |