)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"75ea816cfd64733fd15165a53a6ddefaffdf37d8","unresolved":false,"context_lines":[{"line_number":16,"context_line":"_validate_instance_group_policy to return a context manager. The check"},{"line_number":17,"context_line":"is done on entry, and cleanup accounting is done on exit."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Closes-Bug: #1763181"},{"line_number":20,"context_line":"Change-Id: Ie53bcd745e6748697f3b6a13e752d76933ccc669"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"3f79a3b5_3b9d9bba","line":19,"range":{"start_line":19,"start_character":0,"end_line":19,"end_character":20},"updated":"2018-10-02 13:32:29.000000000","message":"This appears to be untrue:\n\nhttp://logs.openstack.org/36/605436/4/check/nova-tox-functional-py35/66bf4c1/testr_results.html.gz","commit_id":"f37ea991794837f8d894ca9d23ea2c872e121d8e"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d2a77b71c82942baec9ce016c6f1098d3a87e511","unresolved":false,"context_lines":[{"line_number":16,"context_line":"_validate_instance_group_policy to return a context manager. The check"},{"line_number":17,"context_line":"is done on entry, and cleanup accounting is done on exit."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Closes-Bug: #1763181"},{"line_number":20,"context_line":"Change-Id: Ie53bcd745e6748697f3b6a13e752d76933ccc669"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"3f79a3b5_7b4a3324","line":19,"range":{"start_line":19,"start_character":0,"end_line":19,"end_character":20},"in_reply_to":"3f79a3b5_3b9d9bba","updated":"2018-10-02 13:37:01.000000000","message":"I\u0027m told it actually does fix this specific bug, but introduces another that causes the same test to fail in a very similar but distinct way.\n\np\u0027tayta p\u0027tahta.","commit_id":"f37ea991794837f8d894ca9d23ea2c872e121d8e"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"a8dd3129802917e26a5cc5ef95edafa93993122d","unresolved":false,"context_lines":[{"line_number":16,"context_line":"_validate_instance_group_policy to return a context manager. The check"},{"line_number":17,"context_line":"is done on entry, and cleanup accounting is done on exit."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Closes-Bug: #1763181"},{"line_number":20,"context_line":"Change-Id: Ie53bcd745e6748697f3b6a13e752d76933ccc669"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"3f79a3b5_5b4f3734","line":19,"range":{"start_line":19,"start_character":0,"end_line":19,"end_character":20},"in_reply_to":"3f79a3b5_3b9d9bba","updated":"2018-10-02 13:37:04.000000000","message":"It is true, but this patch breaks an assumption in that test which causes it to fail differently. Fix coming shortly.","commit_id":"f37ea991794837f8d894ca9d23ea2c872e121d8e"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"9766151bea28b2d1169ee36c81abb9587019a3a6","unresolved":false,"context_lines":[{"line_number":16,"context_line":"_validate_instance_group_policy to return a context manager. The check"},{"line_number":17,"context_line":"is done on entry, and cleanup accounting is done on exit."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Closes-Bug: #1763181"},{"line_number":20,"context_line":"Change-Id: Ie53bcd745e6748697f3b6a13e752d76933ccc669"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"3f79a3b5_21a00ca5","line":19,"range":{"start_line":19,"start_character":0,"end_line":19,"end_character":20},"in_reply_to":"3f79a3b5_5b4f3734","updated":"2018-10-02 14:46:11.000000000","message":"Specifically, the functional test previously assumed that both rebuilds would execute _do_rebuild_instance, because the validation happens after that. However, this patch changes that because we now validate before calling _do_rebuild_instance. The fake call (fake_rebuild) previously arbitrarily made the instance on host1 wait, but because this now happens after validation and therefore only on 1 host this became a race.\n\nHowever, I can\u0027t actually work out what this delay is for. The comment says:\n\n  # Simulate that the rebuild request of one of the instances\n  # reaches the target compute manager significantly later ***so the\n  # rebuild of the other instance can finish before the late\n  # validation of the first rebuild***.\n\n(*** around what appears to be the justification). However, I don\u0027t understand why that\u0027s important, or how this delay achieves it. Specifically I don\u0027t understand:\n\n* why it\u0027s important that one rebuild is completely finished before the other is validated.\n\n* why, if we want to wait for a rebuild to finish, we\u0027re waiting on rebuild.start.\n\nHowever, I do think it\u0027s important to have 1 thread delay here if we wanted to guarantee correctness, but for an entirely different reason: we need to guarantee the scheduler race. We want to guarantee that the scheduler will send both instances to host3, thus requiring the late validation check that we\u0027re testing here. This means that we don\u0027t want one rebuild to finish so completely that the next rebuild isn\u0027t sent there in the first place.\n\nNote that the existing code doesn\u0027t guarantee that either because the instance on host2 will never delay. Therefore it could, theoretically, win the scheduling race so completely that the instance on host1 is never sent to host3. In practise this seems extremely unlikely, and in 250 local runs of this test it hasn\u0027t happened.\n\nHowever, for the sake of correctness I\u0027ll fix it anyway rather than just removing the delay entirely.","commit_id":"f37ea991794837f8d894ca9d23ea2c872e121d8e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"3b952ad98bc27bbf66a2a54f467937c3d041f04f","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There was a race in the late server group policy check because we"},{"line_number":10,"context_line":"were updating instance.host after _validate_instance_group_policy and"},{"line_number":11,"context_line":"without locking."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This change ensures that we account both for servers which are already"},{"line_number":14,"context_line":"on the host and servers which are building by keeping a list of"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"3f79a3b5_6407d61e","line":11,"updated":"2018-10-03 17:07:00.000000000","message":"We set the instance.host in instance_claim() before we return the claim and then enter that context, where we do the late check, right? In what way are we setting it after the late check?","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"e1f3dba683acd12aa252c574014f8683502f6cf7","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There was a race in the late server group policy check because we"},{"line_number":10,"context_line":"were updating instance.host after _validate_instance_group_policy and"},{"line_number":11,"context_line":"without locking."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This change ensures that we account both for servers which are already"},{"line_number":14,"context_line":"on the host and servers which are building by keeping a list of"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"3f79a3b5_3ac9f277","line":11,"in_reply_to":"3f79a3b5_6407d61e","updated":"2018-10-03 19:03:46.000000000","message":"In the case of evacuate, we run ComputeManager.rebuild_instance(), which does a rebuild_claim, which doesn\u0027t set instance.host.  We then call _do_rebuild_instance_with_claim() (which in turn calls _validate_instance_group_policy()) and if that passes we set instance.host.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"3b952ad98bc27bbf66a2a54f467937c3d041f04f","unresolved":false,"context_lines":[{"line_number":10,"context_line":"were updating instance.host after _validate_instance_group_policy and"},{"line_number":11,"context_line":"without locking."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This change ensures that we account both for servers which are already"},{"line_number":14,"context_line":"on the host and servers which are building by keeping a list of"},{"line_number":15,"context_line":"builders. Tracking builders requires updating"},{"line_number":16,"context_line":"_validate_instance_group_policy to return a context manager. The check"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"3f79a3b5_5f6383dd","line":13,"range":{"start_line":13,"start_character":25,"end_line":13,"end_character":52},"updated":"2018-10-03 17:07:00.000000000","message":"Based on the above, the servers that are building should just fail their check when they go to do it. Since we synchronize the method that actually sets instance.host, no two instances will get their host set at the same time. If the concern is just that there is a gap between setting hosts and doing the check, couldn\u0027t we just yield out of instance_claim() while holding the RT lock so that we do the check stuff while locked?","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"445759782087fdf4beb18bcb25085af8bf502b5b","unresolved":false,"context_lines":[{"line_number":10,"context_line":"were updating instance.host after _validate_instance_group_policy and"},{"line_number":11,"context_line":"without locking."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This change ensures that we account both for servers which are already"},{"line_number":14,"context_line":"on the host and servers which are building by keeping a list of"},{"line_number":15,"context_line":"builders. Tracking builders requires updating"},{"line_number":16,"context_line":"_validate_instance_group_policy to return a context manager. The check"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"3f79a3b5_7c9f5b64","line":13,"range":{"start_line":13,"start_character":25,"end_line":13,"end_character":52},"in_reply_to":"3f79a3b5_5f6383dd","updated":"2018-10-04 08:46:37.000000000","message":"...what Chris said above: we do this differently for build and evacuate. Both cases previously had problems: build was safe but sub-optimal when given multiple concurrent builds[1], evacuate had this bug.\n\nThe purpose of this patch is to fix evacuate, but in doing so we also fix the problem in build.\n\n[1] If you got 2 builds concurrently, both would take a claim, then both would fail the late validation check, leaving neither building on the host.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"6f289bae4490059d05f14c9f588cf0a9979812c3","unresolved":false,"context_lines":[{"line_number":20,"context_line":"\u0027in-progress\u0027 accounting in the check itself it is no longer necessary"},{"line_number":21,"context_line":"that instance.host be set first. By flipping the order we prevent a"},{"line_number":22,"context_line":"situation where multiple builds take a claim, then all of them fail the"},{"line_number":23,"context_line":"validation check, leaving none of the building. With this change we will"},{"line_number":24,"context_line":"always allow the maximum permitted number of builds to proceed."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"TODO: some additional unit tests"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"3f79a3b5_c0841ab5","line":23,"range":{"start_line":23,"start_character":34,"end_line":23,"end_character":37},"updated":"2018-10-09 20:57:42.000000000","message":"s/the/them","commit_id":"0b8ac0ba9146b76bdde4de8124e57f95e86fd026"}],"nova/compute/manager.py":[{"author":{"_account_id":15888,"name":"Zhenyu Zheng","email":"zheng.zhenyu@outlook.com","username":"Kevin_Zheng"},"change_message_id":"9a6cf060c6be536d395d383a85a78ee6c2e7daa2","unresolved":false,"context_lines":[{"line_number":1415,"context_line":"        if context_manager:"},{"line_number":1416,"context_line":"            return group_policy_context()"},{"line_number":1417,"context_line":"        else:"},{"line_number":1418,"context_line":"            _do_validation()"},{"line_number":1419,"context_line":""},{"line_number":1420,"context_line":"    def _log_original_error(self, exc_info, instance_uuid):"},{"line_number":1421,"context_line":"        LOG.error(\u0027Error: %s\u0027, exc_info[1], instance_uuid\u003dinstance_uuid,"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_d82ad88f","line":1418,"range":{"start_line":1418,"start_character":12,"end_line":1418,"end_character":28},"updated":"2018-09-27 02:13:10.000000000","message":"shouldn\u0027t we still keep the original lock for this case?","commit_id":"fc54d286383627f50b063bbd7c87251942f524fb"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bcd868c6c6165661f0d648711ccc77d2b58134e8","unresolved":false,"context_lines":[{"line_number":1415,"context_line":"        if context_manager:"},{"line_number":1416,"context_line":"            return group_policy_context()"},{"line_number":1417,"context_line":"        else:"},{"line_number":1418,"context_line":"            _do_validation()"},{"line_number":1419,"context_line":""},{"line_number":1420,"context_line":"    def _log_original_error(self, exc_info, instance_uuid):"},{"line_number":1421,"context_line":"        LOG.error(\u0027Error: %s\u0027, exc_info[1], instance_uuid\u003dinstance_uuid,"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_83514cdc","line":1418,"range":{"start_line":1418,"start_character":12,"end_line":1418,"end_character":28},"in_reply_to":"3f79a3b5_d82ad88f","updated":"2018-09-27 08:48:06.000000000","message":"No. It wasn\u0027t locking anything.","commit_id":"fc54d286383627f50b063bbd7c87251942f524fb"},{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"356d321c71204a76f358396aed70abc7d78e4294","unresolved":false,"context_lines":[{"line_number":1463,"context_line":"                # host in this anti-affinity group, which we assume will"},{"line_number":1464,"context_line":"                # succeed. This check allows us to fail early without waiting"},{"line_number":1465,"context_line":"                # for the in-progress build to complete."},{"line_number":1466,"context_line":"                if not lock.acquire(False):"},{"line_number":1467,"context_line":"                    return False"},{"line_number":1468,"context_line":""},{"line_number":1469,"context_line":"                try:"},{"line_number":1470,"context_line":"                    instances_uuids \u003d objects.InstanceList.get_uuids_by_host("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_b6f599ce","line":1467,"range":{"start_line":1466,"start_character":16,"end_line":1467,"end_character":32},"updated":"2018-09-27 16:42:56.000000000","message":"Unfortunately I don\u0027t think we can do this.  Suppose we have max_server_per_host\u003d2, zero instances on this node to start, and two instances being checked at the same time.  In that case, both should pass.\n\nThe ugly solution would be for the second to block waiting for the first to give up the lock.\n\nA faster but complicated solution would be to keep track of the number of in-progress operations on this node for each group and factor that into the check against \"max_server_per_host\".","commit_id":"3ceb5db37ad5c58452ab53506d0e9aefed3fb1a9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ea890c4ff172c2cda9276992f4045b449d8a415b","unresolved":false,"context_lines":[{"line_number":1463,"context_line":"                # host in this anti-affinity group, which we assume will"},{"line_number":1464,"context_line":"                # succeed. This check allows us to fail early without waiting"},{"line_number":1465,"context_line":"                # for the in-progress build to complete."},{"line_number":1466,"context_line":"                if not lock.acquire(False):"},{"line_number":1467,"context_line":"                    return False"},{"line_number":1468,"context_line":""},{"line_number":1469,"context_line":"                try:"},{"line_number":1470,"context_line":"                    instances_uuids \u003d objects.InstanceList.get_uuids_by_host("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_245aa21d","line":1467,"range":{"start_line":1466,"start_character":16,"end_line":1467,"end_character":32},"in_reply_to":"3f79a3b5_0df0f065","updated":"2018-09-28 11:59:42.000000000","message":"Turns out it even makes it a bit simpler :)","commit_id":"3ceb5db37ad5c58452ab53506d0e9aefed3fb1a9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"709b32cc51630fa3aa2205498692132cc3819555","unresolved":false,"context_lines":[{"line_number":1463,"context_line":"                # host in this anti-affinity group, which we assume will"},{"line_number":1464,"context_line":"                # succeed. This check allows us to fail early without waiting"},{"line_number":1465,"context_line":"                # for the in-progress build to complete."},{"line_number":1466,"context_line":"                if not lock.acquire(False):"},{"line_number":1467,"context_line":"                    return False"},{"line_number":1468,"context_line":""},{"line_number":1469,"context_line":"                try:"},{"line_number":1470,"context_line":"                    instances_uuids \u003d objects.InstanceList.get_uuids_by_host("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_0df0f065","line":1467,"range":{"start_line":1466,"start_character":16,"end_line":1467,"end_character":32},"in_reply_to":"3f79a3b5_b6f599ce","updated":"2018-09-28 08:49:05.000000000","message":"Good point. My first solution did actually wait. Tracking the number of in-progress builds would not be complex.","commit_id":"3ceb5db37ad5c58452ab53506d0e9aefed3fb1a9"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"31c8d1077ffe15c843388911f2795a111ff84d5b","unresolved":false,"context_lines":[{"line_number":2080,"context_line":"                                                        request_spec)"},{"line_number":2081,"context_line":"            rt \u003d self._get_resource_tracker()"},{"line_number":2082,"context_line":"            with rt.instance_claim(context, instance, node, limits):"},{"line_number":2083,"context_line":"                # NOTE(russellb) It\u0027s important that this validation be done"},{"line_number":2084,"context_line":"                # *after* the resource tracker instance claim, as that is where"},{"line_number":2085,"context_line":"                # the host is set on the instance."},{"line_number":2086,"context_line":"                self._validate_instance_group_policy(context, instance,"},{"line_number":2087,"context_line":"                                                     scheduler_hints)"},{"line_number":2088,"context_line":"                image_meta \u003d objects.ImageMeta.from_dict(image)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_c4dff509","side":"PARENT","line":2085,"range":{"start_line":2083,"start_character":16,"end_line":2085,"end_character":50},"updated":"2018-10-02 00:37:44.000000000","message":"Why kill this comment? Is it not still true, with regard to the ordering of the nested_contexts?","commit_id":"41ac87f812f239e38ad3b72c2f2108523ad3444d"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"35e0ce9cd6670f899a48a1c3c195feb68efb27b0","unresolved":false,"context_lines":[{"line_number":2080,"context_line":"                                                        request_spec)"},{"line_number":2081,"context_line":"            rt \u003d self._get_resource_tracker()"},{"line_number":2082,"context_line":"            with rt.instance_claim(context, instance, node, limits):"},{"line_number":2083,"context_line":"                # NOTE(russellb) It\u0027s important that this validation be done"},{"line_number":2084,"context_line":"                # *after* the resource tracker instance claim, as that is where"},{"line_number":2085,"context_line":"                # the host is set on the instance."},{"line_number":2086,"context_line":"                self._validate_instance_group_policy(context, instance,"},{"line_number":2087,"context_line":"                                                     scheduler_hints)"},{"line_number":2088,"context_line":"                image_meta \u003d objects.ImageMeta.from_dict(image)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_6612a163","side":"PARENT","line":2085,"range":{"start_line":2083,"start_character":16,"end_line":2085,"end_character":50},"in_reply_to":"3f79a3b5_c4dff509","updated":"2018-10-02 12:08:44.000000000","message":"No, it\u0027s not true any more, and in fact I explicitly flipped the order if you look at (new) line 2147.\n\nThe build behaviour (as opposed to rebuild) has always been safe, but sub-optimal. When we took the claim we updated instance.host to the current host, so when we later ran the check the accounting was accurate *assuming all builders would succeed*. That meant that if multiple builders all took a claim, they could then *all* subsequently fail the check.\n\nBecause we now account for in-progress builders in the check itself this is no longer required, and in fact either order is still safe. However, if we flip the order to do the check first then we also fix the problem of failing all concurrent builders. Now if we get N concurrent build requests, when the check runs they haven\u0027t updated instance.host yet so we\u0027re only accounting for the ones which passed the check. That means we now allow the correct number to proceed up to the max.\n\nTL;DR Previously the old order was required for safety. Now it\u0027s not, and I flipped the order to handle multiple builds more efficiently, as well as consistency with rebuild.","commit_id":"41ac87f812f239e38ad3b72c2f2108523ad3444d"},{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"3ba4ea2273e4c81bc13eb6859075e4c8d55332a7","unresolved":false,"context_lines":[{"line_number":1452,"context_line":""},{"line_number":1453,"context_line":"                builders.add(instance.uuid)"},{"line_number":1454,"context_line":""},{"line_number":1455,"context_line":"            yield"},{"line_number":1456,"context_line":""},{"line_number":1457,"context_line":"            with self._group_anti_affinity_check_lock:"},{"line_number":1458,"context_line":"                builders.remove(instance.uuid)"},{"line_number":1459,"context_line":""},{"line_number":1460,"context_line":"        if \u0027affinity\u0027 \u003d\u003d group.policy:"},{"line_number":1461,"context_line":"            return _validate_affinity()"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_79227b03","line":1458,"range":{"start_line":1455,"start_character":0,"end_line":1458,"end_character":46},"updated":"2018-09-28 16:39:39.000000000","message":"I think maybe the yield and builders.remove() could use comments, to minimize confusion for the casual reader.","commit_id":"76c62b13549ad14f64c7b5eee7ee57efaa3870e6"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"35e0ce9cd6670f899a48a1c3c195feb68efb27b0","unresolved":false,"context_lines":[{"line_number":1452,"context_line":""},{"line_number":1453,"context_line":"                builders.add(instance.uuid)"},{"line_number":1454,"context_line":""},{"line_number":1455,"context_line":"            yield"},{"line_number":1456,"context_line":""},{"line_number":1457,"context_line":"            with self._group_anti_affinity_check_lock:"},{"line_number":1458,"context_line":"                builders.remove(instance.uuid)"},{"line_number":1459,"context_line":""},{"line_number":1460,"context_line":"        if \u0027affinity\u0027 \u003d\u003d group.policy:"},{"line_number":1461,"context_line":"            return _validate_affinity()"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_0cfe6f04","line":1458,"range":{"start_line":1455,"start_character":0,"end_line":1458,"end_character":46},"in_reply_to":"3f79a3b5_4450c55e","updated":"2018-10-02 12:08:44.000000000","message":"I\u0027ve added a better comment at the top, and some context here.","commit_id":"76c62b13549ad14f64c7b5eee7ee57efaa3870e6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"31c8d1077ffe15c843388911f2795a111ff84d5b","unresolved":false,"context_lines":[{"line_number":1452,"context_line":""},{"line_number":1453,"context_line":"                builders.add(instance.uuid)"},{"line_number":1454,"context_line":""},{"line_number":1455,"context_line":"            yield"},{"line_number":1456,"context_line":""},{"line_number":1457,"context_line":"            with self._group_anti_affinity_check_lock:"},{"line_number":1458,"context_line":"                builders.remove(instance.uuid)"},{"line_number":1459,"context_line":""},{"line_number":1460,"context_line":"        if \u0027affinity\u0027 \u003d\u003d group.policy:"},{"line_number":1461,"context_line":"            return _validate_affinity()"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_4450c55e","line":1458,"range":{"start_line":1455,"start_character":0,"end_line":1458,"end_character":46},"in_reply_to":"3f79a3b5_79227b03","updated":"2018-10-02 00:37:44.000000000","message":"+1, it would be nice to just make it easier for readers of this code.","commit_id":"76c62b13549ad14f64c7b5eee7ee57efaa3870e6"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"56a241ef875fa949f657253ff8fc0b7454d21a77","unresolved":false,"context_lines":[{"line_number":294,"context_line":"    return decorated_function"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":""},{"line_number":297,"context_line":"def nested_contexts(*ctxts):"},{"line_number":298,"context_line":"    \"\"\"Return a context manager which is the composition of all given context"},{"line_number":299,"context_line":"    managers applied in order, except any which are None."},{"line_number":300,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_9617e19d","line":297,"range":{"start_line":297,"start_character":4,"end_line":297,"end_character":19},"updated":"2018-10-02 16:14:17.000000000","message":"nit: a better name might be nested_optional_contexts.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f201851c7e54165ff8635f1bf46cb3e8d340972a","unresolved":false,"context_lines":[{"line_number":294,"context_line":"    return decorated_function"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":""},{"line_number":297,"context_line":"def nested_contexts(*ctxts):"},{"line_number":298,"context_line":"    \"\"\"Return a context manager which is the composition of all given context"},{"line_number":299,"context_line":"    managers applied in order, except any which are None."},{"line_number":300,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_74546b54","line":297,"range":{"start_line":297,"start_character":0,"end_line":297,"end_character":28},"updated":"2018-10-03 11:17:23.000000000","message":"you should proably just factor this\nhttps://github.com/openstack/nova/blob/957f4818b0d89cd71552effb8b5a5b231a68de7b/nova/test.py#L79-L85\n\ninto a common shared moduel and use that instead.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"e5d90d9baed7ed80fc17465f9cc226fea376f90d","unresolved":false,"context_lines":[{"line_number":294,"context_line":"    return decorated_function"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":""},{"line_number":297,"context_line":"def nested_contexts(*ctxts):"},{"line_number":298,"context_line":"    \"\"\"Return a context manager which is the composition of all given context"},{"line_number":299,"context_line":"    managers applied in order, except any which are None."},{"line_number":300,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_6adb267c","line":297,"range":{"start_line":297,"start_character":0,"end_line":297,"end_character":28},"in_reply_to":"3f79a3b5_4fdafce5","updated":"2018-10-03 12:46:37.000000000","message":"\u003e $ git grep test\\.nested | wc -l\n \u003e 348\n\nNoted :)","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"563a16cd2c7bee94577adafa2231628676124060","unresolved":false,"context_lines":[{"line_number":294,"context_line":"    return decorated_function"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":""},{"line_number":297,"context_line":"def nested_contexts(*ctxts):"},{"line_number":298,"context_line":"    \"\"\"Return a context manager which is the composition of all given context"},{"line_number":299,"context_line":"    managers applied in order, except any which are None."},{"line_number":300,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_cfb5ac7e","line":297,"range":{"start_line":297,"start_character":0,"end_line":297,"end_character":28},"in_reply_to":"3f79a3b5_4fdafce5","updated":"2018-10-03 12:09:08.000000000","message":"ok i ill do this myself later as a followup patch.\nyou are right about the added chrun that is not needed.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"89239ff912f9f3687376a3419fc2d1f2ed105ba0","unresolved":false,"context_lines":[{"line_number":294,"context_line":"    return decorated_function"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":""},{"line_number":297,"context_line":"def nested_contexts(*ctxts):"},{"line_number":298,"context_line":"    \"\"\"Return a context manager which is the composition of all given context"},{"line_number":299,"context_line":"    managers applied in order, except any which are None."},{"line_number":300,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_39c4a791","line":297,"range":{"start_line":297,"start_character":0,"end_line":297,"end_character":28},"in_reply_to":"3f79a3b5_6adb267c","updated":"2018-10-03 14:55:30.000000000","message":"git grep??? git grep?!?!! \u003cmind blown\u003e","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d7020c10bed30824916c4dc53e1ea5b2b2184ba6","unresolved":false,"context_lines":[{"line_number":294,"context_line":"    return decorated_function"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":""},{"line_number":297,"context_line":"def nested_contexts(*ctxts):"},{"line_number":298,"context_line":"    \"\"\"Return a context manager which is the composition of all given context"},{"line_number":299,"context_line":"    managers applied in order, except any which are None."},{"line_number":300,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_afcb901a","line":297,"range":{"start_line":297,"start_character":0,"end_line":297,"end_character":28},"in_reply_to":"3f79a3b5_74546b54","updated":"2018-10-03 12:02:07.000000000","message":"Feel free... the churn from moving test.nested isn\u0027t something I want to do here. Also I\u0027d still have to implement the filter.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f0e9b675dda286797f48f4a115af2b5f92ddc02c","unresolved":false,"context_lines":[{"line_number":294,"context_line":"    return decorated_function"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":""},{"line_number":297,"context_line":"def nested_contexts(*ctxts):"},{"line_number":298,"context_line":"    \"\"\"Return a context manager which is the composition of all given context"},{"line_number":299,"context_line":"    managers applied in order, except any which are None."},{"line_number":300,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_4fdafce5","line":297,"range":{"start_line":297,"start_character":0,"end_line":297,"end_character":28},"in_reply_to":"3f79a3b5_afcb901a","updated":"2018-10-03 12:04:00.000000000","message":"$ git grep test\\.nested | wc -l\n348","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"043a6e2f07596a6693b5354092a33347ad341327","unresolved":false,"context_lines":[{"line_number":318,"context_line":"        except StopIteration:"},{"line_number":319,"context_line":"            yield"},{"line_number":320,"context_line":""},{"line_number":321,"context_line":"    return _nested()"},{"line_number":322,"context_line":""},{"line_number":323,"context_line":""},{"line_number":324,"context_line":"class InstanceEvents(object):"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_7d4bcb85","line":321,"updated":"2018-10-04 17:41:02.000000000","message":"This is neat, but I feel like it might be a little overkill and thus more obscure than it needs to be just because it\u0027s generic.\n\nIf it were me, I would just make a context manager that specifically takes the two contexts that we care about, and grabs them in the same order. It would make this smaller, and very much more readable.\n\nJust something stupid like:\n\n with gp_ctxt:\n     with claim_ctxt:\n         yield\n\nwould do the trick. It also has the benefit of helping to make sure that future use or refactoring of this code does not switch the order of arguments so the build pulls the contexts in one order and rebuild pulls them in the other, leaving us with a deadlock whenever they do compete.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"13d7c66c47476fadff19cc5634e759b3611c02a3","unresolved":false,"context_lines":[{"line_number":318,"context_line":"        except StopIteration:"},{"line_number":319,"context_line":"            yield"},{"line_number":320,"context_line":""},{"line_number":321,"context_line":"    return _nested()"},{"line_number":322,"context_line":""},{"line_number":323,"context_line":""},{"line_number":324,"context_line":"class InstanceEvents(object):"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_6f03030e","line":321,"in_reply_to":"3f79a3b5_7d4bcb85","updated":"2018-10-05 09:58:34.000000000","message":"If I messed with this, I think I\u0027d prefer to go with Sean\u0027s suggestion of pulling test.nested into utils and just using that. It would also be cumbersome to write a simple, hardcoded solution which also took account of gp_ctxt being possibly None. I\u0027ll have a think about that, though.\n\nThe order thing is interesting. Because we don\u0027t hold a lock during execution I don\u0027t think we can deadlock, but the order is still important to correctness.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"043a6e2f07596a6693b5354092a33347ad341327","unresolved":false,"context_lines":[{"line_number":564,"context_line":"        # migration object and Future for the queued live migration."},{"line_number":565,"context_line":"        self._waiting_live_migrations \u003d {}"},{"line_number":566,"context_line":"        self._group_anti_affinity_builders \u003d collections.defaultdict(set)"},{"line_number":567,"context_line":"        self._group_anti_affinity_check_lock \u003d threading.Lock()"},{"line_number":568,"context_line":""},{"line_number":569,"context_line":"        super(ComputeManager, self).__init__(service_name\u003d\"compute\","},{"line_number":570,"context_line":"                                             *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_fdd35b62","line":567,"range":{"start_line":567,"start_character":47,"end_line":567,"end_character":63},"updated":"2018-10-04 17:41:02.000000000","message":"This should be something in lockutils.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a1d713e0ba2dcc8ef10cee86847e7a7aa97e8630","unresolved":false,"context_lines":[{"line_number":564,"context_line":"        # migration object and Future for the queued live migration."},{"line_number":565,"context_line":"        self._waiting_live_migrations \u003d {}"},{"line_number":566,"context_line":"        self._group_anti_affinity_builders \u003d collections.defaultdict(set)"},{"line_number":567,"context_line":"        self._group_anti_affinity_check_lock \u003d threading.Lock()"},{"line_number":568,"context_line":""},{"line_number":569,"context_line":"        super(ComputeManager, self).__init__(service_name\u003d\"compute\","},{"line_number":570,"context_line":"                                             *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_1433b714","line":567,"range":{"start_line":567,"start_character":7,"end_line":567,"end_character":63},"updated":"2018-10-03 12:02:06.000000000","message":"so here we create a lock object but dont aquire it","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"13d7c66c47476fadff19cc5634e759b3611c02a3","unresolved":false,"context_lines":[{"line_number":564,"context_line":"        # migration object and Future for the queued live migration."},{"line_number":565,"context_line":"        self._waiting_live_migrations \u003d {}"},{"line_number":566,"context_line":"        self._group_anti_affinity_builders \u003d collections.defaultdict(set)"},{"line_number":567,"context_line":"        self._group_anti_affinity_check_lock \u003d threading.Lock()"},{"line_number":568,"context_line":""},{"line_number":569,"context_line":"        super(ComputeManager, self).__init__(service_name\u003d\"compute\","},{"line_number":570,"context_line":"                                             *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_e9015309","line":567,"range":{"start_line":567,"start_character":47,"end_line":567,"end_character":63},"in_reply_to":"3f79a3b5_fdd35b62","updated":"2018-10-05 09:58:34.000000000","message":"I thought about that, but it just seemed like unnecessary indirection. The lockutils still is useful for multi-process locks, and locks used in multiple places which form a kind of \u0027internal api\u0027. However, the only interface exposed is a decorator, which is super kludgy when you don\u0027t need that. \n\nThis is about the simplest possible use of a lock.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a1d713e0ba2dcc8ef10cee86847e7a7aa97e8630","unresolved":false,"context_lines":[{"line_number":1442,"context_line":"            # This is not a race, as the set arithmetic ensures we always count"},{"line_number":1443,"context_line":"            # it exactly once."},{"line_number":1444,"context_line":""},{"line_number":1445,"context_line":"            with self._group_anti_affinity_check_lock:"},{"line_number":1446,"context_line":"                # Get group instances currently on this host from the db"},{"line_number":1447,"context_line":"                instances_uuids \u003d objects.InstanceList.get_uuids_by_host("},{"line_number":1448,"context_line":"                    context, self.host)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_8f21f4fe","line":1445,"range":{"start_line":1445,"start_character":11,"end_line":1445,"end_character":54},"updated":"2018-10-03 12:02:06.000000000","message":"then we aquire the lock here because eventlets\nmay context switch us in this block.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a1d713e0ba2dcc8ef10cee86847e7a7aa97e8630","unresolved":false,"context_lines":[{"line_number":1444,"context_line":""},{"line_number":1445,"context_line":"            with self._group_anti_affinity_check_lock:"},{"line_number":1446,"context_line":"                # Get group instances currently on this host from the db"},{"line_number":1447,"context_line":"                instances_uuids \u003d objects.InstanceList.get_uuids_by_host("},{"line_number":1448,"context_line":"                    context, self.host)"},{"line_number":1449,"context_line":"                ins_on_host \u003d set(instances_uuids)"},{"line_number":1450,"context_line":"                members \u003d set(group.members)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_af7dd0e6","line":1447,"range":{"start_line":1447,"start_character":35,"end_line":1447,"end_character":73},"updated":"2018-10-03 12:02:06.000000000","message":"specifcally we could yeild here.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bc81b2e5a7c1e27b7a295d6cc220c4d9674c9adc","unresolved":false,"context_lines":[{"line_number":1444,"context_line":""},{"line_number":1445,"context_line":"            with self._group_anti_affinity_check_lock:"},{"line_number":1446,"context_line":"                # Get group instances currently on this host from the db"},{"line_number":1447,"context_line":"                instances_uuids \u003d objects.InstanceList.get_uuids_by_host("},{"line_number":1448,"context_line":"                    context, self.host)"},{"line_number":1449,"context_line":"                ins_on_host \u003d set(instances_uuids)"},{"line_number":1450,"context_line":"                members \u003d set(group.members)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_2a30cee8","line":1447,"range":{"start_line":1447,"start_character":35,"end_line":1447,"end_character":73},"in_reply_to":"3f79a3b5_af7dd0e6","updated":"2018-10-03 12:22:42.000000000","message":"To clarify, sean is pointing out that eventlet could cause a context switch here.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"043a6e2f07596a6693b5354092a33347ad341327","unresolved":false,"context_lines":[{"line_number":1455,"context_line":"                members_on_host \u003d ins_on_host \u0026 members - set([instance.uuid])"},{"line_number":1456,"context_line":""},{"line_number":1457,"context_line":"                # Add in any which are currently building"},{"line_number":1458,"context_line":"                builders \u003d self._group_anti_affinity_builders[group_hint]"},{"line_number":1459,"context_line":"                members_on_host |\u003d builders"},{"line_number":1460,"context_line":""},{"line_number":1461,"context_line":"                rules \u003d group.rules"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_7d78abf6","line":1458,"range":{"start_line":1458,"start_character":62,"end_line":1458,"end_character":72},"updated":"2018-10-04 17:41:02.000000000","message":"You\u0027re leaking memory here. It\u0027s small, but it\u0027s a leak. Since the user chooses this value and it can be 255 bytes long, I can trivially consume 255 bytes (plus the overhead of an empty set) of compute node memory at a time, over and over.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"13d7c66c47476fadff19cc5634e759b3611c02a3","unresolved":false,"context_lines":[{"line_number":1455,"context_line":"                members_on_host \u003d ins_on_host \u0026 members - set([instance.uuid])"},{"line_number":1456,"context_line":""},{"line_number":1457,"context_line":"                # Add in any which are currently building"},{"line_number":1458,"context_line":"                builders \u003d self._group_anti_affinity_builders[group_hint]"},{"line_number":1459,"context_line":"                members_on_host |\u003d builders"},{"line_number":1460,"context_line":""},{"line_number":1461,"context_line":"                rules \u003d group.rules"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_69b22399","line":1458,"range":{"start_line":1458,"start_character":62,"end_line":1458,"end_character":72},"in_reply_to":"3f79a3b5_7d78abf6","updated":"2018-10-05 09:58:34.000000000","message":"Hmm, I\u0027d mentioned this myself in a previous version, but it hadn\u0027t occurred to me that these are user-defined. Yeah, that\u0027s bad. It\u0027s also much simpler to clean up in this version, as you point out below.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"043a6e2f07596a6693b5354092a33347ad341327","unresolved":false,"context_lines":[{"line_number":1473,"context_line":"                # Check passed"},{"line_number":1474,"context_line":"                builders.add(instance.uuid)"},{"line_number":1475,"context_line":""},{"line_number":1476,"context_line":"            yield"},{"line_number":1477,"context_line":""},{"line_number":1478,"context_line":"            # Build has completed: success or failure. Either way, ensure it is"},{"line_number":1479,"context_line":"            # removed from the in-progress set."}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_ddc9bf7a","line":1476,"range":{"start_line":1476,"start_character":12,"end_line":1476,"end_character":17},"updated":"2018-10-04 17:41:02.000000000","message":"If the thing we\u0027re yielding to raises an exception, we will never remove the thing from builders, which means we might make poor decisions later (and or this could grow unbounded with enough time).\n\nYou need to do something like:\n\n try:\n     yield\n finally:\n     # Build has completed: success or failure. Either way, ensure it is\n     # removed from the in-progress set.\n     with self._group_anti_affinity_check_lock:\n         builders.remove(instance.uuid)","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"13d7c66c47476fadff19cc5634e759b3611c02a3","unresolved":false,"context_lines":[{"line_number":1473,"context_line":"                # Check passed"},{"line_number":1474,"context_line":"                builders.add(instance.uuid)"},{"line_number":1475,"context_line":""},{"line_number":1476,"context_line":"            yield"},{"line_number":1477,"context_line":""},{"line_number":1478,"context_line":"            # Build has completed: success or failure. Either way, ensure it is"},{"line_number":1479,"context_line":"            # removed from the in-progress set."}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_29ac2b35","line":1476,"range":{"start_line":1476,"start_character":12,"end_line":1476,"end_character":17},"in_reply_to":"3f79a3b5_ddc9bf7a","updated":"2018-10-05 09:58:34.000000000","message":"D\u0027oh.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"043a6e2f07596a6693b5354092a33347ad341327","unresolved":false,"context_lines":[{"line_number":1478,"context_line":"            # Build has completed: success or failure. Either way, ensure it is"},{"line_number":1479,"context_line":"            # removed from the in-progress set."},{"line_number":1480,"context_line":"            with self._group_anti_affinity_check_lock:"},{"line_number":1481,"context_line":"                builders.remove(instance.uuid)"},{"line_number":1482,"context_line":""},{"line_number":1483,"context_line":"        if \u0027affinity\u0027 \u003d\u003d group.policy:"},{"line_number":1484,"context_line":"            return _validate_affinity()"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_fd635b44","line":1481,"updated":"2018-10-04 17:41:02.000000000","message":"if not builders:\n     self._group_anti_affinity_builders.pop(group_hint)","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"13d7c66c47476fadff19cc5634e759b3611c02a3","unresolved":false,"context_lines":[{"line_number":1478,"context_line":"            # Build has completed: success or failure. Either way, ensure it is"},{"line_number":1479,"context_line":"            # removed from the in-progress set."},{"line_number":1480,"context_line":"            with self._group_anti_affinity_check_lock:"},{"line_number":1481,"context_line":"                builders.remove(instance.uuid)"},{"line_number":1482,"context_line":""},{"line_number":1483,"context_line":"        if \u0027affinity\u0027 \u003d\u003d group.policy:"},{"line_number":1484,"context_line":"            return _validate_affinity()"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_69678326","line":1481,"in_reply_to":"3f79a3b5_fd635b44","updated":"2018-10-05 09:58:34.000000000","message":"+1","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"043a6e2f07596a6693b5354092a33347ad341327","unresolved":false,"context_lines":[{"line_number":1483,"context_line":"        if \u0027affinity\u0027 \u003d\u003d group.policy:"},{"line_number":1484,"context_line":"            return _validate_affinity()"},{"line_number":1485,"context_line":"        elif \u0027anti-affinity\u0027 \u003d\u003d group.policy:"},{"line_number":1486,"context_line":"            return _validate_anti_affinity()"},{"line_number":1487,"context_line":""},{"line_number":1488,"context_line":"    def _log_original_error(self, exc_info, instance_uuid):"},{"line_number":1489,"context_line":"        LOG.error(\u0027Error: %s\u0027, exc_info[1], instance_uuid\u003dinstance_uuid,"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_1d35f7f6","line":1486,"updated":"2018-10-04 17:41:02.000000000","message":"I would expect to see new unit test coverage for this method that pokes all of the potential failure points, like the raised-an-exception case mentioned above.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"13d7c66c47476fadff19cc5634e759b3611c02a3","unresolved":false,"context_lines":[{"line_number":1483,"context_line":"        if \u0027affinity\u0027 \u003d\u003d group.policy:"},{"line_number":1484,"context_line":"            return _validate_affinity()"},{"line_number":1485,"context_line":"        elif \u0027anti-affinity\u0027 \u003d\u003d group.policy:"},{"line_number":1486,"context_line":"            return _validate_anti_affinity()"},{"line_number":1487,"context_line":""},{"line_number":1488,"context_line":"    def _log_original_error(self, exc_info, instance_uuid):"},{"line_number":1489,"context_line":"        LOG.error(\u0027Error: %s\u0027, exc_info[1], instance_uuid\u003dinstance_uuid,"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_6c40f160","line":1486,"in_reply_to":"3f79a3b5_1d35f7f6","updated":"2018-10-05 09:58:34.000000000","message":"Ok.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"}],"nova/tests/functional/regressions/test_bug_1735407.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a1d713e0ba2dcc8ef10cee86847e7a7aa97e8630","unresolved":false,"context_lines":[{"line_number":86,"context_line":""},{"line_number":87,"context_line":"            # Try (non-blocking) to take the scheduled_first lock. Only the"},{"line_number":88,"context_line":"            # winner will get it. The loser will proceed without waiting."},{"line_number":89,"context_line":"            if scheduled_first.acquire(False):"},{"line_number":90,"context_line":"                try:"},{"line_number":91,"context_line":"                    fake_notifier.wait_for_versioned_notifications("},{"line_number":92,"context_line":"                        \u0027instance.rebuild.start\u0027, n_events\u003d1)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_d49c5f0e","line":89,"range":{"start_line":89,"start_character":11,"end_line":89,"end_character":46},"updated":"2018-10-03 12:02:06.000000000","message":"im not sure why you are making this atomic just to intentionally write a race.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"043a6e2f07596a6693b5354092a33347ad341327","unresolved":false,"context_lines":[{"line_number":86,"context_line":""},{"line_number":87,"context_line":"            # Try (non-blocking) to take the scheduled_first lock. Only the"},{"line_number":88,"context_line":"            # winner will get it. The loser will proceed without waiting."},{"line_number":89,"context_line":"            if scheduled_first.acquire(False):"},{"line_number":90,"context_line":"                try:"},{"line_number":91,"context_line":"                    fake_notifier.wait_for_versioned_notifications("},{"line_number":92,"context_line":"                        \u0027instance.rebuild.start\u0027, n_events\u003d1)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_ddfd3f3d","line":89,"range":{"start_line":89,"start_character":11,"end_line":89,"end_character":46},"in_reply_to":"3f79a3b5_6fb0188c","updated":"2018-10-04 17:41:02.000000000","message":"I definitely don\u0027t understand why we need this and why we can\u0027t just use a mock to determine which order things happened in.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bc81b2e5a7c1e27b7a295d6cc220c4d9674c9adc","unresolved":false,"context_lines":[{"line_number":86,"context_line":""},{"line_number":87,"context_line":"            # Try (non-blocking) to take the scheduled_first lock. Only the"},{"line_number":88,"context_line":"            # winner will get it. The loser will proceed without waiting."},{"line_number":89,"context_line":"            if scheduled_first.acquire(False):"},{"line_number":90,"context_line":"                try:"},{"line_number":91,"context_line":"                    fake_notifier.wait_for_versioned_notifications("},{"line_number":92,"context_line":"                        \u0027instance.rebuild.start\u0027, n_events\u003d1)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_6fb0188c","line":89,"range":{"start_line":89,"start_character":11,"end_line":89,"end_character":46},"in_reply_to":"3f79a3b5_d49c5f0e","updated":"2018-10-03 12:22:42.000000000","message":"If it wasn\u0027t atomic then it would be possible for both threads to acquire it, and then this would be a deadlock.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"13d7c66c47476fadff19cc5634e759b3611c02a3","unresolved":false,"context_lines":[{"line_number":86,"context_line":""},{"line_number":87,"context_line":"            # Try (non-blocking) to take the scheduled_first lock. Only the"},{"line_number":88,"context_line":"            # winner will get it. The loser will proceed without waiting."},{"line_number":89,"context_line":"            if scheduled_first.acquire(False):"},{"line_number":90,"context_line":"                try:"},{"line_number":91,"context_line":"                    fake_notifier.wait_for_versioned_notifications("},{"line_number":92,"context_line":"                        \u0027instance.rebuild.start\u0027, n_events\u003d1)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_8cf22d05","line":89,"range":{"start_line":89,"start_character":11,"end_line":89,"end_character":46},"in_reply_to":"3f79a3b5_ddfd3f3d","updated":"2018-10-05 09:58:34.000000000","message":"See a missive I wrote about this on an earlier version of the patch. The previous version of this was very broken, and I\u0027ve essentially tried here to reverse engineer the original purpose and write that.\n\nWe don\u0027t care what order things happen here; we care about timing. It\u0027s no good just to check mock later to see what order things happened, because we don\u0027t care. We care that whichever thread is scheduled first, it doesn\u0027t complete until the second thread is also scheduled here.\n\nIn the actual test, we do:\n\n        # evacuate both instances\n        post \u003d {\u0027evacuate\u0027: {}}\n        self.api.post_server_action(server1[\u0027id\u0027], post)\n        self.api.post_server_action(server2[\u0027id\u0027], post)\n\nIn order to ensure that the race we\u0027re trying to test actually occurs, we need to guarantee that the first evacuate doesn\u0027t complete entirely before the second evacuate runs.\n\nIncidentally, this code is what caused the recent uptick in the incidence of this test in CI. Before the fix to fake_notifier we *never* waited here, so the race almost never occurred, so we weren\u0027t testing it. After the fix to fake_notifier, we only tested the race if the instance on host1 landed first, hence the approximately 50% failure rate.\n\nThis code guarantees that we always exercise the race by setting using a flag. We use an atomic flag because we\u0027re multi-threaded[1].\n\n[1] (For Sean ;)): I know you can do this without a lock if you assume eventlet and encode implicit assumptions about the conditions under which eventlet will context switch, but that way lies insanity and really-hard-to-find bugs. E.g. when you later add a new context switching method into the middle of what you forgot was a critical section, or when we ditch eventlet and all assumptions change. It\u0027s way simpler just to assume a context switch can happen anywhere and write defensive code.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bc81b2e5a7c1e27b7a295d6cc220c4d9674c9adc","unresolved":false,"context_lines":[{"line_number":91,"context_line":"                    fake_notifier.wait_for_versioned_notifications("},{"line_number":92,"context_line":"                        \u0027instance.rebuild.start\u0027, n_events\u003d1)"},{"line_number":93,"context_line":"                finally:"},{"line_number":94,"context_line":"                    scheduled_first.release()"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"            original_rebuild(_self, context, instance, *args, **kwargs)"},{"line_number":97,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_8a3a42c1","line":94,"updated":"2018-10-03 12:22:42.000000000","message":"Incidentally, this line isn\u0027t strictly necessary as we never re-acquire the lock and it\u0027ll just go safely out of scope. It\u0027s tidier, though.","commit_id":"4188ebf7f5d87bfa9d38718af98b1c77ca39f438"}]}