)]}'
{"nova/db/sqlalchemy/api.py":[{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"762f1d6ab4be86d44b44a70407eb236e28744f7d","unresolved":false,"context_lines":[{"line_number":1753,"context_line":"    values - dict containing column values."},{"line_number":1754,"context_line":"    \"\"\""},{"line_number":1755,"context_line":""},{"line_number":1756,"context_line":"    security_group_ensure_default(context)"},{"line_number":1757,"context_line":""},{"line_number":1758,"context_line":"    values \u003d values.copy()"},{"line_number":1759,"context_line":"    values[\u0027metadata\u0027] \u003d _metadata_refs("}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_26a6ef10","line":1756,"updated":"2019-10-14 16:42:05.000000000","message":"based on IRC discussion it seems this uses a separate tranasction because it then can take advantage of the retry decorator, in case another process gets to it first, it should return the value that was just created.  OK. but can we then use the return value right here instaed of calling again down below?\n\nalso why would a \"Security group\" be the kind of thing subject to racing processes trying to create the same one?  seems like an administrator-level concept that would be infrequently created.","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f1ed41cb2381d297932310e9a95c67d87e2059a8","unresolved":false,"context_lines":[{"line_number":1753,"context_line":"    values - dict containing column values."},{"line_number":1754,"context_line":"    \"\"\""},{"line_number":1755,"context_line":""},{"line_number":1756,"context_line":"    security_group_ensure_default(context)"},{"line_number":1757,"context_line":""},{"line_number":1758,"context_line":"    values \u003d values.copy()"},{"line_number":1759,"context_line":"    values[\u0027metadata\u0027] \u003d _metadata_refs("}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_3997f0ee","line":1756,"in_reply_to":"3fa7e38b_26a6ef10","updated":"2019-10-14 17:19:46.000000000","message":"Looking through the history of commits around this, the issue was discovered while running Rally (the performance/stress testing project) which creates lots of resource concurrently, among other things. And so I think they saw issues where a racing server create \"lost\" and failed to the end user, because two requests were trying to create the default security group at the same time.\n\nWhile the default security group is an admin-level concept, because it is \"default\" (i.e. supposed to exist without any intervention from an admin) and because, at the time, these were owned by nova-networking, nova was responsible for creating the default group (not the administrator). And since they are per project, I guess it makes sense that the thinking was to create the default group for a project during instance create, when it is first needed.","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"762f1d6ab4be86d44b44a70407eb236e28744f7d","unresolved":false,"context_lines":[{"line_number":1782,"context_line":"    instance_ref[\u0027extra\u0027].update(values.pop(\u0027extra\u0027, {}))"},{"line_number":1783,"context_line":"    instance_ref.update(values)"},{"line_number":1784,"context_line":""},{"line_number":1785,"context_line":"    def _get_sec_group_models(security_groups):"},{"line_number":1786,"context_line":"        models \u003d []"},{"line_number":1787,"context_line":"        # NOTE(melwitt): We need to call security_group_ensure_default here"},{"line_number":1788,"context_line":"        # instead of _security_group_ensure_default because the default"}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_26cd8fb6","line":1785,"updated":"2019-10-14 16:42:05.000000000","message":"what is the rationale for the code here to be inside a closure?  it seems to suggest there was some some history here, however as someone who has never seen this code it\u0027s not clear why the code is organized this way, assuming im reading the gerrit correctly it looks like it is referenced on line 1806 but could just as well be inline at that point.","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f1ed41cb2381d297932310e9a95c67d87e2059a8","unresolved":false,"context_lines":[{"line_number":1782,"context_line":"    instance_ref[\u0027extra\u0027].update(values.pop(\u0027extra\u0027, {}))"},{"line_number":1783,"context_line":"    instance_ref.update(values)"},{"line_number":1784,"context_line":""},{"line_number":1785,"context_line":"    def _get_sec_group_models(security_groups):"},{"line_number":1786,"context_line":"        models \u003d []"},{"line_number":1787,"context_line":"        # NOTE(melwitt): We need to call security_group_ensure_default here"},{"line_number":1788,"context_line":"        # instead of _security_group_ensure_default because the default"}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_b93d20c5","line":1785,"in_reply_to":"3fa7e38b_26cd8fb6","updated":"2019-10-14 17:19:46.000000000","message":"Good question and I\u0027m afraid I don\u0027t know. I also find it unexpected and seems it could be inline.\n\n(later) I looked up the commit from 2012 where this was added and there\u0027s no review comments around it, so it appears it was just a way to make things look more organized, perhaps?\n\nhttps://review.opendev.org/#/c/8973/2/nova/db/sqlalchemy/api.py@1339","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d4558a005f27c3158152ce0b4a64c26189092f06","unresolved":false,"context_lines":[{"line_number":1782,"context_line":"    instance_ref[\u0027extra\u0027].update(values.pop(\u0027extra\u0027, {}))"},{"line_number":1783,"context_line":"    instance_ref.update(values)"},{"line_number":1784,"context_line":""},{"line_number":1785,"context_line":"    def _get_sec_group_models(security_groups):"},{"line_number":1786,"context_line":"        models \u003d []"},{"line_number":1787,"context_line":"        # NOTE(melwitt): We need to call security_group_ensure_default here"},{"line_number":1788,"context_line":"        # instead of _security_group_ensure_default because the default"}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_17cafb9f","line":1785,"in_reply_to":"3fa7e38b_5c365a05","updated":"2019-10-14 17:58:49.000000000","message":"Ack, will do in the next PS and note it in the commit message.","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"d39d3bf6547e082903213306e30aa4f79c311623","unresolved":false,"context_lines":[{"line_number":1782,"context_line":"    instance_ref[\u0027extra\u0027].update(values.pop(\u0027extra\u0027, {}))"},{"line_number":1783,"context_line":"    instance_ref.update(values)"},{"line_number":1784,"context_line":""},{"line_number":1785,"context_line":"    def _get_sec_group_models(security_groups):"},{"line_number":1786,"context_line":"        models \u003d []"},{"line_number":1787,"context_line":"        # NOTE(melwitt): We need to call security_group_ensure_default here"},{"line_number":1788,"context_line":"        # instead of _security_group_ensure_default because the default"}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_5c365a05","line":1785,"in_reply_to":"3fa7e38b_b93d20c5","updated":"2019-10-14 17:50:12.000000000","message":"very possible, I used to do that all the time for some reason too.   suggest breaking it out, because it contributes to the lack of clarity surrounding the security_group_ensure_default() function as well as it suggests more complex use of this code which is not in fact present.","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"762f1d6ab4be86d44b44a70407eb236e28744f7d","unresolved":false,"context_lines":[{"line_number":1791,"context_line":"        # transaction to be able to read back the newly created default group,"},{"line_number":1792,"context_line":"        # if it was created earlier in the instance_create method."},{"line_number":1793,"context_line":"        # See for more details: https://bugs.launchpad.net/nova/+bug/1824435"},{"line_number":1794,"context_line":"        default_group \u003d security_group_ensure_default(context)"},{"line_number":1795,"context_line":"        if \u0027default\u0027 in security_groups:"},{"line_number":1796,"context_line":"            models.append(default_group)"},{"line_number":1797,"context_line":"            # Generate a new list, so we don\u0027t modify the original"}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_865243dd","line":1794,"updated":"2019-10-14 16:42:05.000000000","message":"how would this default_group have changed from when this function was called at the top of this function? would it be sufficient to just assign default_group at the top when this function was called the first time?","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f1ed41cb2381d297932310e9a95c67d87e2059a8","unresolved":false,"context_lines":[{"line_number":1791,"context_line":"        # transaction to be able to read back the newly created default group,"},{"line_number":1792,"context_line":"        # if it was created earlier in the instance_create method."},{"line_number":1793,"context_line":"        # See for more details: https://bugs.launchpad.net/nova/+bug/1824435"},{"line_number":1794,"context_line":"        default_group \u003d security_group_ensure_default(context)"},{"line_number":1795,"context_line":"        if \u0027default\u0027 in security_groups:"},{"line_number":1796,"context_line":"            models.append(default_group)"},{"line_number":1797,"context_line":"            # Generate a new list, so we don\u0027t modify the original"}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_d9c25c85","line":1794,"in_reply_to":"3fa7e38b_865243dd","updated":"2019-10-14 17:19:46.000000000","message":"This is interesting and off the top of my head, I don\u0027t see why we couldn\u0027t just use the returned group from the earlier security_group_ensure_default call. And, referring back to the original commit that added this code again [1], it looks like originally this was the only place where security_group_ensure_default was called.\n\nA second call was added later to address the case of racing server create requests (as we discussed earlier on IRC [2]):\n\nhttps://review.opendev.org/#/c/122291/6/nova/db/sqlalchemy/api.py\n\nand the later call of _security_group_ensure_default was not revisited and was left as-is.\n\nFinally for completeness, the private _security_group_ensure_default method was added here, before the second security_group_ensure_default call was added:\n\nhttps://review.opendev.org/#/c/115255/2/nova/db/sqlalchemy/api.py\n\nand the purpose of the private method was to make the _security_group_ensure_default use the current database transaction instead of creating a separate one.\n\nBased on this analysis, it looks like this call is just residue left over from earlier refactoring and is not needed. So, I think the best thing to do is to remove this second call and use the returned group from the first call earlier.\n\n[1] https://review.opendev.org/#/c/8973/2/nova/db/sqlalchemy/api.py@1339\n[2] http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2019-10-14.log.html#t2019-10-14T16:31:51","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"d39d3bf6547e082903213306e30aa4f79c311623","unresolved":false,"context_lines":[{"line_number":1791,"context_line":"        # transaction to be able to read back the newly created default group,"},{"line_number":1792,"context_line":"        # if it was created earlier in the instance_create method."},{"line_number":1793,"context_line":"        # See for more details: https://bugs.launchpad.net/nova/+bug/1824435"},{"line_number":1794,"context_line":"        default_group \u003d security_group_ensure_default(context)"},{"line_number":1795,"context_line":"        if \u0027default\u0027 in security_groups:"},{"line_number":1796,"context_line":"            models.append(default_group)"},{"line_number":1797,"context_line":"            # Generate a new list, so we don\u0027t modify the original"}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_dc21eab5","line":1794,"in_reply_to":"3fa7e38b_d9c25c85","updated":"2019-10-14 17:50:12.000000000","message":"+1","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d4558a005f27c3158152ce0b4a64c26189092f06","unresolved":false,"context_lines":[{"line_number":1791,"context_line":"        # transaction to be able to read back the newly created default group,"},{"line_number":1792,"context_line":"        # if it was created earlier in the instance_create method."},{"line_number":1793,"context_line":"        # See for more details: https://bugs.launchpad.net/nova/+bug/1824435"},{"line_number":1794,"context_line":"        default_group \u003d security_group_ensure_default(context)"},{"line_number":1795,"context_line":"        if \u0027default\u0027 in security_groups:"},{"line_number":1796,"context_line":"            models.append(default_group)"},{"line_number":1797,"context_line":"            # Generate a new list, so we don\u0027t modify the original"}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_d7754365","line":1794,"in_reply_to":"3fa7e38b_dc21eab5","updated":"2019-10-14 17:58:49.000000000","message":"Thanks for the fresh eyes on this. I got tunneled with how to fix this after being really confused about the duplicate record creation, putting print statements all over the place, and finally finding what was going on many hours later. xD","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d7f3b42ccc7fb2123c4cde498b6ac1b5744df0b3","unresolved":false,"context_lines":[{"line_number":4105,"context_line":"        # this one is not aborted in case a concurrent one succeeds first"},{"line_number":4106,"context_line":"        # and the unique constraint for security group names is violated"},{"line_number":4107,"context_line":"        # by a concurrent INSERT"},{"line_number":4108,"context_line":"        with get_context_manager(context).writer.independent.using(context):"},{"line_number":4109,"context_line":"            return _security_group_ensure_default(context)"},{"line_number":4110,"context_line":"    except exception.SecurityGroupExists:"},{"line_number":4111,"context_line":"        # NOTE(rpodolyaka): a concurrent transaction has succeeded first,"}],"source_content_type":"text/x-python","patch_set":12,"id":"3fa7e38b_c30fcd34","line":4108,"updated":"2019-10-14 16:12:00.000000000","message":"Note to reviewers: here is where we use a separate database transaction when getting or creating the default security group.","commit_id":"743a0266e99edd05f9e2cac72aeae9c952ce153c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"47906cb2893dd60e67c311a4edabdc7117466dc6","unresolved":false,"context_lines":[{"line_number":1785,"context_line":"    def _get_sec_group_models(security_groups):"},{"line_number":1786,"context_line":"        models \u003d []"},{"line_number":1787,"context_line":"        default_group \u003d _security_group_ensure_default(context)"},{"line_number":1788,"context_line":"        if \u0027default\u0027 in security_groups:"},{"line_number":1789,"context_line":"            models.append(default_group)"},{"line_number":1790,"context_line":"            # Generate a new list, so we don\u0027t modify the original"},{"line_number":1791,"context_line":"            security_groups \u003d [x for x in security_groups if x !\u003d \u0027default\u0027]"}],"source_content_type":"text/x-python","patch_set":13,"id":"3fa7e38b_cb71e646","side":"PARENT","line":1788,"updated":"2019-10-17 20:05:18.000000000","message":"Heh and if this was False the _security_group_ensure_default call in the previous line was pointless.","commit_id":"7c41365f193fd5b08d0174b6cd8e349ac95b7907"}]}