)]}'
{"neutron/api/v2/attributes.py":[{"author":{"_account_id":6670,"name":"Amir Sadoughi","email":"amir.sadoughi@rackspace.com","username":"amir-sadoughi"},"change_message_id":"07bde034eefb434f4632ff851358211f3d36425a","unresolved":false,"context_lines":[{"line_number":676,"context_line":"                       \u0027default\u0027: ATTR_NOT_SPECIFIED,"},{"line_number":677,"context_line":"                       \u0027validate\u0027: {\u0027type:ip_address_or_none\u0027: None},"},{"line_number":678,"context_line":"                       \u0027is_visible\u0027: True},"},{"line_number":679,"context_line":"        #TODO(salvatore-orlando): Enable PUT on allocation_pools"},{"line_number":680,"context_line":"        \u0027allocation_pools\u0027: {\u0027allow_post\u0027: True, \u0027allow_put\u0027: True,"},{"line_number":681,"context_line":"                             \u0027default\u0027: ATTR_NOT_SPECIFIED,"},{"line_number":682,"context_line":"                             \u0027validate\u0027: {\u0027type:ip_pools\u0027: None},"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F%2Fw9c%3D","line":679,"updated":"2014-01-08 18:33:47.000000000","message":"TODO no longer applies. Please remove.","commit_id":"f70a29a876546ddb164fd1547c420b1e3a84d260"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"2f74402b37b1ff4ac7bd2f3fa96657494ea6056c","unresolved":false,"context_lines":[{"line_number":676,"context_line":"                       \u0027default\u0027: ATTR_NOT_SPECIFIED,"},{"line_number":677,"context_line":"                       \u0027validate\u0027: {\u0027type:ip_address_or_none\u0027: None},"},{"line_number":678,"context_line":"                       \u0027is_visible\u0027: True},"},{"line_number":679,"context_line":"        #TODO(salvatore-orlando): Enable PUT on allocation_pools"},{"line_number":680,"context_line":"        \u0027allocation_pools\u0027: {\u0027allow_post\u0027: True, \u0027allow_put\u0027: True,"},{"line_number":681,"context_line":"                             \u0027default\u0027: ATTR_NOT_SPECIFIED,"},{"line_number":682,"context_line":"                             \u0027validate\u0027: {\u0027type:ip_pools\u0027: None},"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F%2FSkc%3D","line":679,"in_reply_to":"AAAAUH%2F%2Fw9c%3D","updated":"2014-01-15 15:57:51.000000000","message":"thx! Done","commit_id":"f70a29a876546ddb164fd1547c420b1e3a84d260"}],"neutron/db/db_base_plugin_v2.py":[{"author":{"_account_id":6072,"name":"Eugene Nikanorov","email":"enikanorov@mirantis.com","username":"enikanorov"},"change_message_id":"6c6598ddb6aa18f790c1e5fdf936640f05df0ede","unresolved":false,"context_lines":[{"line_number":1264,"context_line":"                new_pools_dict \u003d s[\u0027allocation_pools\u0027]"},{"line_number":1265,"context_line":"                #dbase session remove old add new:"},{"line_number":1266,"context_line":"                for pool in old_pools:"},{"line_number":1267,"context_line":"                    context.session.delete(pool)"},{"line_number":1268,"context_line":"                for pool_dict in new_pools_dict:"},{"line_number":1269,"context_line":"                    pool \u003d models_v2.IPAllocationPool("},{"line_number":1270,"context_line":"                                                first_ip\u003dpool_dict[\u0027start\u0027],"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9MoM%3D","line":1267,"updated":"2013-12-14 13:21:15.000000000","message":"This is just not safe both for concurrent db access and for the case when there are an allocations in existing allocation pools.\n\nI think the first step that should be made is to ensure that updating allocation pools is safe.\nThen everything related to the pool should be locked, and then pool is updated.\n\nThis t doesn\u0027t seem that simple to me.","commit_id":"de04ee2c511dfae0ca1664b20a1c283ca61df59d"},{"author":{"_account_id":6670,"name":"Amir Sadoughi","email":"amir.sadoughi@rackspace.com","username":"amir-sadoughi"},"change_message_id":"07bde034eefb434f4632ff851358211f3d36425a","unresolved":false,"context_lines":[{"line_number":1278,"context_line":"                ip_allocs \u003d (__session.query(models_v2.IPAllocation)."},{"line_number":1279,"context_line":"                             filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1280,"context_line":"                             with_lockmode(\u0027update\u0027)).all()"},{"line_number":1281,"context_line":"                for ip_alloc in ip_allocs:"},{"line_number":1282,"context_line":"                    #is from any of the current allocation_pools?"},{"line_number":1283,"context_line":"                    cidrs \u003d _get_pool_cidrs(old_pools)"},{"line_number":1284,"context_line":"                    ip \u003d ip_alloc.ip_address"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F%2Fw78%3D","line":1281,"updated":"2014-01-08 18:33:47.000000000","message":"Nit: I think it would be a lot cleaner to use IPSet\u0027s to derive the intersection of pools and allocations, then iterating only across the intersection.","commit_id":"f70a29a876546ddb164fd1547c420b1e3a84d260"},{"author":{"_account_id":6670,"name":"Amir Sadoughi","email":"amir.sadoughi@rackspace.com","username":"amir-sadoughi"},"change_message_id":"f1e00e9e972a2a5b7442f2e9d2ec4f9a70e82a78","unresolved":false,"context_lines":[{"line_number":1278,"context_line":"                ip_allocs \u003d (__session.query(models_v2.IPAllocation)."},{"line_number":1279,"context_line":"                             filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1280,"context_line":"                             with_lockmode(\u0027update\u0027)).all()"},{"line_number":1281,"context_line":"                for ip_alloc in ip_allocs:"},{"line_number":1282,"context_line":"                    #is from any of the current allocation_pools?"},{"line_number":1283,"context_line":"                    cidrs \u003d _get_pool_cidrs(old_pools)"},{"line_number":1284,"context_line":"                    ip \u003d ip_alloc.ip_address"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F%2FRak%3D","line":1281,"in_reply_to":"AAAAUH%2F%2FSiU%3D","updated":"2014-01-15 16:21:12.000000000","message":"The IPSet I was referring to is part of netaddr. It\u0027s up to you, more of a coding style choice.","commit_id":"f70a29a876546ddb164fd1547c420b1e3a84d260"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"2f74402b37b1ff4ac7bd2f3fa96657494ea6056c","unresolved":false,"context_lines":[{"line_number":1278,"context_line":"                ip_allocs \u003d (__session.query(models_v2.IPAllocation)."},{"line_number":1279,"context_line":"                             filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1280,"context_line":"                             with_lockmode(\u0027update\u0027)).all()"},{"line_number":1281,"context_line":"                for ip_alloc in ip_allocs:"},{"line_number":1282,"context_line":"                    #is from any of the current allocation_pools?"},{"line_number":1283,"context_line":"                    cidrs \u003d _get_pool_cidrs(old_pools)"},{"line_number":1284,"context_line":"                    ip \u003d ip_alloc.ip_address"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F%2FSiU%3D","line":1281,"in_reply_to":"AAAAUH%2F%2Fw78%3D","updated":"2014-01-15 15:57:51.000000000","message":"thx, is that from IPy? is it worth importing a new module for? netaddr is already present. i can change it if people feel strongly for it,\n\nthanks!","commit_id":"f70a29a876546ddb164fd1547c420b1e3a84d260"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1209,"context_line":"                new_dns_addr_set \u003d set(s[\"dns_nameservers\"])"},{"line_number":1210,"context_line":"                old_dns_addr_set \u003d set([dns[\u0027address\u0027]"},{"line_number":1211,"context_line":"                                        for dns in old_dns_list])"},{"line_number":1212,"context_line":""},{"line_number":1213,"context_line":"                new_dns \u003d list(new_dns_addr_set)"},{"line_number":1214,"context_line":"                for dns_addr in old_dns_addr_set - new_dns_addr_set:"},{"line_number":1215,"context_line":"                    for dns in old_dns_list:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0gI%3D","side":"PARENT","line":1212,"updated":"2014-01-21 19:55:29.000000000","message":"This change has nothing to do with this patch.  Avoid gratuitous changes, even just in the whitespace.","commit_id":"a253a8c9b7c34a614ee4c15ac46857c575717942"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"bc182f6f3cbcc68ab29bd4c149833a774e0b399e","unresolved":false,"context_lines":[{"line_number":1209,"context_line":"                new_dns_addr_set \u003d set(s[\"dns_nameservers\"])"},{"line_number":1210,"context_line":"                old_dns_addr_set \u003d set([dns[\u0027address\u0027]"},{"line_number":1211,"context_line":"                                        for dns in old_dns_list])"},{"line_number":1212,"context_line":""},{"line_number":1213,"context_line":"                new_dns \u003d list(new_dns_addr_set)"},{"line_number":1214,"context_line":"                for dns_addr in old_dns_addr_set - new_dns_addr_set:"},{"line_number":1215,"context_line":"                    for dns in old_dns_list:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BqGg%3D","side":"PARENT","line":1212,"in_reply_to":"AAAAUH%2F%2B0gI%3D","updated":"2014-01-23 16:53:58.000000000","message":"Done (sorry didn\u0027t even notice this one)","commit_id":"a253a8c9b7c34a614ee4c15ac46857c575717942"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1204,"context_line":"            self._validate_gw_out_of_pools(s[\"gateway_ip\"], allocation_pools)"},{"line_number":1205,"context_line":"        #get session for safely dealing with allocation pools:"},{"line_number":1206,"context_line":"        __session \u003d context.session"},{"line_number":1207,"context_line":"        with __session.begin(subtransactions\u003dTrue):"},{"line_number":1208,"context_line":"            if \"dns_nameservers\" in s:"},{"line_number":1209,"context_line":"                changed_dns \u003d True"},{"line_number":1210,"context_line":"                old_dns_list \u003d self._get_dns_by_subnet(context, id)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0Sc%3D","line":1207,"updated":"2014-01-21 19:55:29.000000000","message":"This temporary adds little to no value.  You could revert this block of the change entirely.","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"bc182f6f3cbcc68ab29bd4c149833a774e0b399e","unresolved":false,"context_lines":[{"line_number":1204,"context_line":"            self._validate_gw_out_of_pools(s[\"gateway_ip\"], allocation_pools)"},{"line_number":1205,"context_line":"        #get session for safely dealing with allocation pools:"},{"line_number":1206,"context_line":"        __session \u003d context.session"},{"line_number":1207,"context_line":"        with __session.begin(subtransactions\u003dTrue):"},{"line_number":1208,"context_line":"            if \"dns_nameservers\" in s:"},{"line_number":1209,"context_line":"                changed_dns \u003d True"},{"line_number":1210,"context_line":"                old_dns_list \u003d self._get_dns_by_subnet(context, id)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BqGk%3D","line":1207,"in_reply_to":"AAAAUH%2F%2B0Sc%3D","updated":"2014-01-23 16:53:58.000000000","message":"Done","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1256,"context_line":"                del s[\"host_routes\"]"},{"line_number":1257,"context_line":""},{"line_number":1258,"context_line":"            def _get_pool_cidrs(pools):"},{"line_number":1259,"context_line":"                cidrs \u003d []"},{"line_number":1260,"context_line":"                for pool in pools:"},{"line_number":1261,"context_line":"                    first \u003d pool.first_ip"},{"line_number":1262,"context_line":"                    last \u003d pool.last_ip"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0f8%3D","line":1259,"updated":"2014-01-21 19:55:29.000000000","message":"Maybe use an IPSet here instead.","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"bc182f6f3cbcc68ab29bd4c149833a774e0b399e","unresolved":false,"context_lines":[{"line_number":1256,"context_line":"                del s[\"host_routes\"]"},{"line_number":1257,"context_line":""},{"line_number":1258,"context_line":"            def _get_pool_cidrs(pools):"},{"line_number":1259,"context_line":"                cidrs \u003d []"},{"line_number":1260,"context_line":"                for pool in pools:"},{"line_number":1261,"context_line":"                    first \u003d pool.first_ip"},{"line_number":1262,"context_line":"                    last \u003d pool.last_ip"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BqFs%3D","line":1259,"in_reply_to":"AAAAUH%2F%2B0f8%3D","updated":"2014-01-23 16:53:58.000000000","message":"thanks will do, in fact Amir suggested same on rev2 but I was mistaken in thinking it meant a new import...","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1260,"context_line":"                for pool in pools:"},{"line_number":1261,"context_line":"                    first \u003d pool.first_ip"},{"line_number":1262,"context_line":"                    last \u003d pool.last_ip"},{"line_number":1263,"context_line":"                    cidrs.append(netaddr.iprange_to_cidrs(first, last))"},{"line_number":1264,"context_line":"                #flatten"},{"line_number":1265,"context_line":"                cidrs \u003d list(itertools.chain(*cidrs))"},{"line_number":1266,"context_line":"                return cidrs"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0ck%3D","line":1263,"updated":"2014-01-21 19:55:29.000000000","message":"ipset |\u003d netaddr.IPSet(netaddr.iprange_to_cidrs(first, last))","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"bc182f6f3cbcc68ab29bd4c149833a774e0b399e","unresolved":false,"context_lines":[{"line_number":1260,"context_line":"                for pool in pools:"},{"line_number":1261,"context_line":"                    first \u003d pool.first_ip"},{"line_number":1262,"context_line":"                    last \u003d pool.last_ip"},{"line_number":1263,"context_line":"                    cidrs.append(netaddr.iprange_to_cidrs(first, last))"},{"line_number":1264,"context_line":"                #flatten"},{"line_number":1265,"context_line":"                cidrs \u003d list(itertools.chain(*cidrs))"},{"line_number":1266,"context_line":"                return cidrs"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BqEU%3D","line":1263,"in_reply_to":"AAAAUH%2F%2B0ck%3D","updated":"2014-01-23 16:53:58.000000000","message":"thanks!","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1262,"context_line":"                    last \u003d pool.last_ip"},{"line_number":1263,"context_line":"                    cidrs.append(netaddr.iprange_to_cidrs(first, last))"},{"line_number":1264,"context_line":"                #flatten"},{"line_number":1265,"context_line":"                cidrs \u003d list(itertools.chain(*cidrs))"},{"line_number":1266,"context_line":"                return cidrs"},{"line_number":1267,"context_line":""},{"line_number":1268,"context_line":"            if \"allocation_pools\" in s:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0eE%3D","line":1265,"updated":"2014-01-21 19:55:29.000000000","message":"return ipset","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"bc182f6f3cbcc68ab29bd4c149833a774e0b399e","unresolved":false,"context_lines":[{"line_number":1262,"context_line":"                    last \u003d pool.last_ip"},{"line_number":1263,"context_line":"                    cidrs.append(netaddr.iprange_to_cidrs(first, last))"},{"line_number":1264,"context_line":"                #flatten"},{"line_number":1265,"context_line":"                cidrs \u003d list(itertools.chain(*cidrs))"},{"line_number":1266,"context_line":"                return cidrs"},{"line_number":1267,"context_line":""},{"line_number":1268,"context_line":"            if \"allocation_pools\" in s:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BqEQ%3D","line":1265,"in_reply_to":"AAAAUH%2F%2B0eE%3D","updated":"2014-01-23 16:53:58.000000000","message":"Done","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1278,"context_line":"                ip_allocs \u003d (__session.query(models_v2.IPAllocation)."},{"line_number":1279,"context_line":"                             filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1280,"context_line":"                             with_lockmode(\u0027update\u0027)).all()"},{"line_number":1281,"context_line":"                for ip_alloc in ip_allocs:"},{"line_number":1282,"context_line":"                    #is from any of the current allocation_pools?"},{"line_number":1283,"context_line":"                    cidrs \u003d _get_pool_cidrs(old_pools)"},{"line_number":1284,"context_line":"                    ip \u003d ip_alloc.ip_address"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0aM%3D","line":1281,"updated":"2014-01-21 19:55:29.000000000","message":"This might look a bit cleaner as a set operation.\n\nallocations \u003d IPSet(...)\nold_pools \u003d IPSet(...)\nnew_pools \u003d IPSet(...)\n\npotential_problem_allocations \u003d allocations \u0026 (old_pools - new_pools)\n\nSee line 421 here:  https://review.openstack.org/#/c/58017/17/neutron/db/db_base_plugin_v2.py","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1279,"context_line":"                             filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1280,"context_line":"                             with_lockmode(\u0027update\u0027)).all()"},{"line_number":1281,"context_line":"                for ip_alloc in ip_allocs:"},{"line_number":1282,"context_line":"                    #is from any of the current allocation_pools?"},{"line_number":1283,"context_line":"                    cidrs \u003d _get_pool_cidrs(old_pools)"},{"line_number":1284,"context_line":"                    ip \u003d ip_alloc.ip_address"},{"line_number":1285,"context_line":"                    if not netaddr.all_matching_cidrs(ip, cidrs):"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0WI%3D","line":1282,"updated":"2014-01-21 19:55:29.000000000","message":"Don\u0027t check allocations that are in the intersection of the old pools and the new pools.  For example, what if we\u0027re just making the existing pools bigger.  There is no reason to throw an error in this case.","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"bc182f6f3cbcc68ab29bd4c149833a774e0b399e","unresolved":false,"context_lines":[{"line_number":1279,"context_line":"                             filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1280,"context_line":"                             with_lockmode(\u0027update\u0027)).all()"},{"line_number":1281,"context_line":"                for ip_alloc in ip_allocs:"},{"line_number":1282,"context_line":"                    #is from any of the current allocation_pools?"},{"line_number":1283,"context_line":"                    cidrs \u003d _get_pool_cidrs(old_pools)"},{"line_number":1284,"context_line":"                    ip \u003d ip_alloc.ip_address"},{"line_number":1285,"context_line":"                    if not netaddr.all_matching_cidrs(ip, cidrs):"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BpX4%3D","line":1282,"in_reply_to":"AAAAUH%2F%2B0WI%3D","updated":"2014-01-23 16:53:58.000000000","message":"Done","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1280,"context_line":"                             with_lockmode(\u0027update\u0027)).all()"},{"line_number":1281,"context_line":"                for ip_alloc in ip_allocs:"},{"line_number":1282,"context_line":"                    #is from any of the current allocation_pools?"},{"line_number":1283,"context_line":"                    cidrs \u003d _get_pool_cidrs(old_pools)"},{"line_number":1284,"context_line":"                    ip \u003d ip_alloc.ip_address"},{"line_number":1285,"context_line":"                    if not netaddr.all_matching_cidrs(ip, cidrs):"},{"line_number":1286,"context_line":"                        continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0aA%3D","line":1283,"updated":"2014-01-21 19:55:29.000000000","message":"This is run every time through the loop.  Same result each time, right?","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"bc182f6f3cbcc68ab29bd4c149833a774e0b399e","unresolved":false,"context_lines":[{"line_number":1280,"context_line":"                             with_lockmode(\u0027update\u0027)).all()"},{"line_number":1281,"context_line":"                for ip_alloc in ip_allocs:"},{"line_number":1282,"context_line":"                    #is from any of the current allocation_pools?"},{"line_number":1283,"context_line":"                    cidrs \u003d _get_pool_cidrs(old_pools)"},{"line_number":1284,"context_line":"                    ip \u003d ip_alloc.ip_address"},{"line_number":1285,"context_line":"                    if not netaddr.all_matching_cidrs(ip, cidrs):"},{"line_number":1286,"context_line":"                        continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BpX8%3D","line":1283,"in_reply_to":"AAAAUH%2F%2B0aA%3D","updated":"2014-01-23 16:53:58.000000000","message":"indeed thanks","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1290,"context_line":"                        #deleting the allocation (and port by cascade)"},{"line_number":1291,"context_line":"                        #or just \u0027continue\u0027 here? The whole pool is gonna"},{"line_number":1292,"context_line":"                        #get nuked and replaced if update successful here"},{"line_number":1293,"context_line":"                        NeutronDbPluginV2._delete_ip_allocation("},{"line_number":1294,"context_line":"                            context, db_subnet.network_id,"},{"line_number":1295,"context_line":"                            id, ip_alloc.ip_address)"},{"line_number":1296,"context_line":"                    else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0YM%3D","line":1293,"updated":"2014-01-21 19:55:29.000000000","message":"I think \"continue\" would be a better choice.  It isn\u0027t the end of the world if there is an allocation leftover from the old pool.  It is likely that this port is still in use by the DHCP server or whatever.","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"bc182f6f3cbcc68ab29bd4c149833a774e0b399e","unresolved":false,"context_lines":[{"line_number":1290,"context_line":"                        #deleting the allocation (and port by cascade)"},{"line_number":1291,"context_line":"                        #or just \u0027continue\u0027 here? The whole pool is gonna"},{"line_number":1292,"context_line":"                        #get nuked and replaced if update successful here"},{"line_number":1293,"context_line":"                        NeutronDbPluginV2._delete_ip_allocation("},{"line_number":1294,"context_line":"                            context, db_subnet.network_id,"},{"line_number":1295,"context_line":"                            id, ip_alloc.ip_address)"},{"line_number":1296,"context_line":"                    else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BpPg%3D","line":1293,"in_reply_to":"AAAAUH%2F%2B0YM%3D","updated":"2014-01-23 16:53:58.000000000","message":"right... the AUTO_DELETE_PORT_OWNERS is just dhcp... ok i will remove this for now and until someone speaks up :)","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1294,"context_line":"                            context, db_subnet.network_id,"},{"line_number":1295,"context_line":"                            id, ip_alloc.ip_address)"},{"line_number":1296,"context_line":"                    else:"},{"line_number":1297,"context_line":"                        raise q_exc.IpAllocationConflict("},{"line_number":1298,"context_line":"                            ip_address\u003dip_alloc.ip_address,"},{"line_number":1299,"context_line":"                            port_id\u003dip_alloc.port_id)"},{"line_number":1300,"context_line":"                #at this point safe to delete the old alloc pools:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0Xw%3D","line":1297,"updated":"2014-01-21 19:55:29.000000000","message":"I want to float the question that maybe it is okay if there are outstanding ip allocations in the old pools.  What do others think?  They might have been manually allocated or some other special case.\n\nIf I\u0027m the operator and I get an error from this the first thing I\u0027m going to want is a \"force\" option so that I can force change the allocation pools and deal with the stragglers later.","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1295,"context_line":"                            id, ip_alloc.ip_address)"},{"line_number":1296,"context_line":"                    else:"},{"line_number":1297,"context_line":"                        raise q_exc.IpAllocationConflict("},{"line_number":1298,"context_line":"                            ip_address\u003dip_alloc.ip_address,"},{"line_number":1299,"context_line":"                            port_id\u003dip_alloc.port_id)"},{"line_number":1300,"context_line":"                #at this point safe to delete the old alloc pools:"},{"line_number":1301,"context_line":"                for old_pool in old_pools:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0W0%3D","line":1298,"updated":"2014-01-21 19:55:29.000000000","message":"Let\u0027s say there are many problem allocations.  This just reports 1.  So, the API caller has to iterate.\n\n1.  Try modifying pools\n2.  It fails and reports a problem allocation that is preventing the operation.\n3.  Fix up the problem and retry starting over at #1.\n\nWith the set operation I suggest, all problems could be reported at once.  That will be very helpful to the caller.","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"bc182f6f3cbcc68ab29bd4c149833a774e0b399e","unresolved":false,"context_lines":[{"line_number":1295,"context_line":"                            id, ip_alloc.ip_address)"},{"line_number":1296,"context_line":"                    else:"},{"line_number":1297,"context_line":"                        raise q_exc.IpAllocationConflict("},{"line_number":1298,"context_line":"                            ip_address\u003dip_alloc.ip_address,"},{"line_number":1299,"context_line":"                            port_id\u003dip_alloc.port_id)"},{"line_number":1300,"context_line":"                #at this point safe to delete the old alloc pools:"},{"line_number":1301,"context_line":"                for old_pool in old_pools:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BpJ8%3D","line":1298,"in_reply_to":"AAAAUH%2F%2B0W0%3D","updated":"2014-01-23 16:53:58.000000000","message":"ok have reworked to try and gather all problematic allocations before raising","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1300,"context_line":"                #at this point safe to delete the old alloc pools:"},{"line_number":1301,"context_line":"                for old_pool in old_pools:"},{"line_number":1302,"context_line":"                    __session.delete(old_pool)"},{"line_number":1303,"context_line":"                new_pools_dict \u003d s[\u0027allocation_pools\u0027]"},{"line_number":1304,"context_line":"                #dbase session remove old add new:"},{"line_number":1305,"context_line":"                for pool_dict in new_pools_dict:"},{"line_number":1306,"context_line":"                    pool \u003d models_v2.IPAllocationPool("}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0Tc%3D","line":1303,"updated":"2014-01-21 19:55:29.000000000","message":"Inconsistent use of temporary (i.e. you refer to s[\u0027allocation_pools\u0027] directly in the next for loop.)  Just remove the temporary.  There is no benefit.","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"41af76cbaf82cd1f73130d0a10a76c286064bdd5","unresolved":false,"context_lines":[{"line_number":1309,"context_line":"                    __session.add(pool)"},{"line_number":1310,"context_line":"                #Gather new pools for result:"},{"line_number":1311,"context_line":"                new_pools \u003d []"},{"line_number":1312,"context_line":"                for pool_dict in s[\u0027allocation_pools\u0027]:"},{"line_number":1313,"context_line":"                    new_pools.append({\u0027start\u0027: pool_dict[\u0027start\u0027],"},{"line_number":1314,"context_line":"                                      \u0027end\u0027: pool_dict[\u0027end\u0027]})"},{"line_number":1315,"context_line":"                del s[\u0027allocation_pools\u0027]"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B0T4%3D","line":1312,"updated":"2014-01-21 19:55:29.000000000","message":"Combine this with the previous for loop?","commit_id":"1bbb29e06730886d3a769033fe84dbf48a6d6e61"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"5a8bac0ab936e36671b55dae73cdc4bd2a40381c","unresolved":false,"context_lines":[{"line_number":1201,"context_line":"            allocation_pools \u003d [{\u0027start\u0027: p[\u0027first_ip\u0027], \u0027end\u0027: p[\u0027last_ip\u0027]}"},{"line_number":1202,"context_line":"                                for p in db_subnet.allocation_pools]"},{"line_number":1203,"context_line":"            self._validate_gw_out_of_pools(s[\"gateway_ip\"], allocation_pools)"},{"line_number":1204,"context_line":""},{"line_number":1205,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"},{"line_number":1206,"context_line":"            if \"dns_nameservers\" in s:"},{"line_number":1207,"context_line":"                changed_dns \u003d True"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bo6g%3D","side":"PARENT","line":1204,"updated":"2014-01-23 16:58:56.000000000","message":"damn.. seems i did it again ;) will fix with the inevitable next rev.","commit_id":"a253a8c9b7c34a614ee4c15ac46857c575717942"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1201,"context_line":"            allocation_pools \u003d [{\u0027start\u0027: p[\u0027first_ip\u0027], \u0027end\u0027: p[\u0027last_ip\u0027]}"},{"line_number":1202,"context_line":"                                for p in db_subnet.allocation_pools]"},{"line_number":1203,"context_line":"            self._validate_gw_out_of_pools(s[\"gateway_ip\"], allocation_pools)"},{"line_number":1204,"context_line":""},{"line_number":1205,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"},{"line_number":1206,"context_line":"            if \"dns_nameservers\" in s:"},{"line_number":1207,"context_line":"                changed_dns \u003d True"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm8I%3D","side":"PARENT","line":1204,"in_reply_to":"AAAAUH%2F%2Bo6g%3D","updated":"2014-01-23 23:04:31.000000000","message":";)","commit_id":"a253a8c9b7c34a614ee4c15ac46857c575717942"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1254,"context_line":"                         \u0027nexthop\u0027: route_str.partition(\"_\")[2]})"},{"line_number":1255,"context_line":"                del s[\"host_routes\"]"},{"line_number":1256,"context_line":""},{"line_number":1257,"context_line":"            def _get_pool_cidrs(pools):"},{"line_number":1258,"context_line":"                ipset \u003d netaddr.IPSet()"},{"line_number":1259,"context_line":"                for pool in pools:"},{"line_number":1260,"context_line":"                    first \u003d pool.first_ip"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm4M%3D","line":1257,"updated":"2014-01-23 23:04:31.000000000","message":"The name of this method doesn\u0027t quite fit anymore.  Maybe something like _convert_pools_to_ipset.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1254,"context_line":"                         \u0027nexthop\u0027: route_str.partition(\"_\")[2]})"},{"line_number":1255,"context_line":"                del s[\"host_routes\"]"},{"line_number":1256,"context_line":""},{"line_number":1257,"context_line":"            def _get_pool_cidrs(pools):"},{"line_number":1258,"context_line":"                ipset \u003d netaddr.IPSet()"},{"line_number":1259,"context_line":"                for pool in pools:"},{"line_number":1260,"context_line":"                    first \u003d pool.first_ip"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F9%2BB0%3D","line":1257,"in_reply_to":"AAAAUH%2F%2Bm4M%3D","updated":"2014-01-31 16:32:43.000000000","message":"Done","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1257,"context_line":"            def _get_pool_cidrs(pools):"},{"line_number":1258,"context_line":"                ipset \u003d netaddr.IPSet()"},{"line_number":1259,"context_line":"                for pool in pools:"},{"line_number":1260,"context_line":"                    first \u003d pool.first_ip"},{"line_number":1261,"context_line":"                    last \u003d pool.last_ip"},{"line_number":1262,"context_line":"                    ipset |\u003d netaddr.IPSet("},{"line_number":1263,"context_line":"                        netaddr.iprange_to_cidrs(first, last))"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm34%3D","line":1260,"updated":"2014-01-23 23:04:31.000000000","message":"These temporary variables have little value.  Consider inlining them.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1257,"context_line":"            def _get_pool_cidrs(pools):"},{"line_number":1258,"context_line":"                ipset \u003d netaddr.IPSet()"},{"line_number":1259,"context_line":"                for pool in pools:"},{"line_number":1260,"context_line":"                    first \u003d pool.first_ip"},{"line_number":1261,"context_line":"                    last \u003d pool.last_ip"},{"line_number":1262,"context_line":"                    ipset |\u003d netaddr.IPSet("},{"line_number":1263,"context_line":"                        netaddr.iprange_to_cidrs(first, last))"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F9%2BAw%3D","line":1260,"in_reply_to":"AAAAUH%2F%2Bm34%3D","updated":"2014-01-31 16:32:43.000000000","message":"Done","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1263,"context_line":"                        netaddr.iprange_to_cidrs(first, last))"},{"line_number":1264,"context_line":"                return ipset"},{"line_number":1265,"context_line":""},{"line_number":1266,"context_line":"            if \"allocation_pools\" in s:"},{"line_number":1267,"context_line":"                #before committing to anything check sanity"},{"line_number":1268,"context_line":"                self._validate_allocation_pools(s[\u0027allocation_pools\u0027],"},{"line_number":1269,"context_line":"                                                s[\u0027cidr\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm5w%3D","line":1266,"updated":"2014-01-23 23:04:31.000000000","message":"A few lines of whitespace might help break up this long block of code and make it easier to digest.  (I encourage well-placed whitespace lines in new code that you add.  :)","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1263,"context_line":"                        netaddr.iprange_to_cidrs(first, last))"},{"line_number":1264,"context_line":"                return ipset"},{"line_number":1265,"context_line":""},{"line_number":1266,"context_line":"            if \"allocation_pools\" in s:"},{"line_number":1267,"context_line":"                #before committing to anything check sanity"},{"line_number":1268,"context_line":"                self._validate_allocation_pools(s[\u0027allocation_pools\u0027],"},{"line_number":1269,"context_line":"                                                s[\u0027cidr\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F9%2BAk%3D","line":1266,"in_reply_to":"AAAAUH%2F%2Bm5w%3D","updated":"2014-01-31 16:32:43.000000000","message":"pep8 won\u0027t let me do it ./neutron/db/db_base_plugin_v2.py:1266:13: E303 too many blank lines (2)","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1264,"context_line":"                return ipset"},{"line_number":1265,"context_line":""},{"line_number":1266,"context_line":"            if \"allocation_pools\" in s:"},{"line_number":1267,"context_line":"                #before committing to anything check sanity"},{"line_number":1268,"context_line":"                self._validate_allocation_pools(s[\u0027allocation_pools\u0027],"},{"line_number":1269,"context_line":"                                                s[\u0027cidr\u0027])"},{"line_number":1270,"context_line":"                changed_allocation_pools \u003d True"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm4k%3D","line":1267,"updated":"2014-01-23 23:04:31.000000000","message":"This comment doesn\u0027t add value.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1264,"context_line":"                return ipset"},{"line_number":1265,"context_line":""},{"line_number":1266,"context_line":"            if \"allocation_pools\" in s:"},{"line_number":1267,"context_line":"                #before committing to anything check sanity"},{"line_number":1268,"context_line":"                self._validate_allocation_pools(s[\u0027allocation_pools\u0027],"},{"line_number":1269,"context_line":"                                                s[\u0027cidr\u0027])"},{"line_number":1270,"context_line":"                changed_allocation_pools \u003d True"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F9%2BAU%3D","line":1267,"in_reply_to":"AAAAUH%2F%2Bm4k%3D","updated":"2014-01-31 16:32:43.000000000","message":"Done","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1268,"context_line":"                self._validate_allocation_pools(s[\u0027allocation_pools\u0027],"},{"line_number":1269,"context_line":"                                                s[\u0027cidr\u0027])"},{"line_number":1270,"context_line":"                changed_allocation_pools \u003d True"},{"line_number":1271,"context_line":"                #Get the pools for this subnet and lock them"},{"line_number":1272,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1273,"context_line":"                old_pools \u003d pools_qry.filter_by(subnet_id\u003did).with_lockmode("},{"line_number":1274,"context_line":"                    \u0027update\u0027).all()"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm4g%3D","line":1271,"updated":"2014-01-23 23:04:31.000000000","message":"This comment doesn\u0027t add value.  Reading the code is straight-forward and says the same thing.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1268,"context_line":"                self._validate_allocation_pools(s[\u0027allocation_pools\u0027],"},{"line_number":1269,"context_line":"                                                s[\u0027cidr\u0027])"},{"line_number":1270,"context_line":"                changed_allocation_pools \u003d True"},{"line_number":1271,"context_line":"                #Get the pools for this subnet and lock them"},{"line_number":1272,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1273,"context_line":"                old_pools \u003d pools_qry.filter_by(subnet_id\u003did).with_lockmode("},{"line_number":1274,"context_line":"                    \u0027update\u0027).all()"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F9%2BAM%3D","line":1271,"in_reply_to":"AAAAUH%2F%2Bm4g%3D","updated":"2014-01-31 16:32:43.000000000","message":"Done","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1273,"context_line":"                old_pools \u003d pools_qry.filter_by(subnet_id\u003did).with_lockmode("},{"line_number":1274,"context_line":"                    \u0027update\u0027).all()"},{"line_number":1275,"context_line":"                #Check existing allocations for this subnet"},{"line_number":1276,"context_line":"                #gather existing ip allocations with ports"},{"line_number":1277,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1278,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1279,"context_line":"                               with_lockmode(\u0027update\u0027))"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm4U%3D","line":1276,"updated":"2014-01-23 23:04:31.000000000","message":"Same thing about the comment.  Comments are great but if it is just telling me something that I can easily see from the code then it detracts from readability.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1273,"context_line":"                old_pools \u003d pools_qry.filter_by(subnet_id\u003did).with_lockmode("},{"line_number":1274,"context_line":"                    \u0027update\u0027).all()"},{"line_number":1275,"context_line":"                #Check existing allocations for this subnet"},{"line_number":1276,"context_line":"                #gather existing ip allocations with ports"},{"line_number":1277,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1278,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1279,"context_line":"                               with_lockmode(\u0027update\u0027))"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F9%2Bj0%3D","line":1276,"in_reply_to":"AAAAUH%2F%2Bm4U%3D","updated":"2014-01-31 16:32:43.000000000","message":"yeah sure... i didn\u0027t intentionally write them as doc... i use comments like that to help me implement. Usually they come first and then just hang around, sometimes in the wrong places :) \n\nwill trim, thanks","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1280,"context_line":"                allocated_ips \u003d netaddr.IPSet("},{"line_number":1281,"context_line":"                    [netaddr.ipaddress(i[\u0027ip_address\u0027])"},{"line_number":1282,"context_line":"                        for i in alloc_query.all()])"},{"line_number":1283,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1284,"context_line":"                #create IP sets for old and new pools:"},{"line_number":1285,"context_line":"                old_pools_ips \u003d _get_pool_cidrs(old_pools)"},{"line_number":1286,"context_line":"                #create array of new pools for generation of IP set"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm3Q%3D","line":1283,"updated":"2014-01-23 23:04:31.000000000","message":"This code runs alloc_query.all() twice.  Move this line before line 1280 and then use the result in the list comprehension rather than running the query again.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1280,"context_line":"                allocated_ips \u003d netaddr.IPSet("},{"line_number":1281,"context_line":"                    [netaddr.ipaddress(i[\u0027ip_address\u0027])"},{"line_number":1282,"context_line":"                        for i in alloc_query.all()])"},{"line_number":1283,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1284,"context_line":"                #create IP sets for old and new pools:"},{"line_number":1285,"context_line":"                old_pools_ips \u003d _get_pool_cidrs(old_pools)"},{"line_number":1286,"context_line":"                #create array of new pools for generation of IP set"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F99%2FY%3D","line":1283,"in_reply_to":"AAAAUH%2F%2Bm3Q%3D","updated":"2014-01-31 16:32:43.000000000","message":"good catch, thanks","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1282,"context_line":"                        for i in alloc_query.all()])"},{"line_number":1283,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1284,"context_line":"                #create IP sets for old and new pools:"},{"line_number":1285,"context_line":"                old_pools_ips \u003d _get_pool_cidrs(old_pools)"},{"line_number":1286,"context_line":"                #create array of new pools for generation of IP set"},{"line_number":1287,"context_line":"                new_pools \u003d []"},{"line_number":1288,"context_line":"                for pool_dict in s[\u0027allocation_pools\u0027]:"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm14%3D","line":1285,"updated":"2014-01-23 23:04:31.000000000","message":"Consider inlining this temporary.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1282,"context_line":"                        for i in alloc_query.all()])"},{"line_number":1283,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1284,"context_line":"                #create IP sets for old and new pools:"},{"line_number":1285,"context_line":"                old_pools_ips \u003d _get_pool_cidrs(old_pools)"},{"line_number":1286,"context_line":"                #create array of new pools for generation of IP set"},{"line_number":1287,"context_line":"                new_pools \u003d []"},{"line_number":1288,"context_line":"                for pool_dict in s[\u0027allocation_pools\u0027]:"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F99%2B0%3D","line":1285,"in_reply_to":"AAAAUH%2F%2Bm14%3D","updated":"2014-01-31 16:32:43.000000000","message":"Done","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1285,"context_line":"                old_pools_ips \u003d _get_pool_cidrs(old_pools)"},{"line_number":1286,"context_line":"                #create array of new pools for generation of IP set"},{"line_number":1287,"context_line":"                new_pools \u003d []"},{"line_number":1288,"context_line":"                for pool_dict in s[\u0027allocation_pools\u0027]:"},{"line_number":1289,"context_line":"                    pool \u003d models_v2.IPAllocationPool("},{"line_number":1290,"context_line":"                        first_ip\u003dpool_dict[\u0027start\u0027], last_ip\u003dpool_dict[\u0027end\u0027],"},{"line_number":1291,"context_line":"                        subnet_id\u003did)"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2BmvU%3D","line":1288,"updated":"2014-01-23 23:04:31.000000000","message":"Consider a list comprehension.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1290,"context_line":"                        first_ip\u003dpool_dict[\u0027start\u0027], last_ip\u003dpool_dict[\u0027end\u0027],"},{"line_number":1291,"context_line":"                        subnet_id\u003did)"},{"line_number":1292,"context_line":"                    new_pools.append(pool)"},{"line_number":1293,"context_line":"                new_pools_ips \u003d _get_pool_cidrs(new_pools)"},{"line_number":1294,"context_line":"                problem_allocations \u003d allocated_ips \u0026 \\"},{"line_number":1295,"context_line":"                    (old_pools_ips - new_pools_ips)"},{"line_number":1296,"context_line":"                conflicts \u003d \"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm10%3D","line":1293,"updated":"2014-01-23 23:04:31.000000000","message":"Consider inlining this temporary.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1290,"context_line":"                        first_ip\u003dpool_dict[\u0027start\u0027], last_ip\u003dpool_dict[\u0027end\u0027],"},{"line_number":1291,"context_line":"                        subnet_id\u003did)"},{"line_number":1292,"context_line":"                    new_pools.append(pool)"},{"line_number":1293,"context_line":"                new_pools_ips \u003d _get_pool_cidrs(new_pools)"},{"line_number":1294,"context_line":"                problem_allocations \u003d allocated_ips \u0026 \\"},{"line_number":1295,"context_line":"                    (old_pools_ips - new_pools_ips)"},{"line_number":1296,"context_line":"                conflicts \u003d \"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F99%2Bg%3D","line":1293,"in_reply_to":"AAAAUH%2F%2Bm10%3D","updated":"2014-01-31 16:32:43.000000000","message":"Done","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1294,"context_line":"                problem_allocations \u003d allocated_ips \u0026 \\"},{"line_number":1295,"context_line":"                    (old_pools_ips - new_pools_ips)"},{"line_number":1296,"context_line":"                conflicts \u003d \"\""},{"line_number":1297,"context_line":"                for alloc in ip_allocs_with_ports:"},{"line_number":1298,"context_line":"                    ip \u003d alloc.ip_address"},{"line_number":1299,"context_line":"                    if ((ip in problem_allocations) and not"},{"line_number":1300,"context_line":"                        (alloc.ports.device_owner in AUTO_DELETE_PORT_OWNERS)):"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bmv8%3D","line":1297,"updated":"2014-01-23 23:04:31.000000000","message":"I wish there were a good way to only iterate over the problem_allocations.  But, I don\u0027t see one.  So, you can ignore this comment.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1294,"context_line":"                problem_allocations \u003d allocated_ips \u0026 \\"},{"line_number":1295,"context_line":"                    (old_pools_ips - new_pools_ips)"},{"line_number":1296,"context_line":"                conflicts \u003d \"\""},{"line_number":1297,"context_line":"                for alloc in ip_allocs_with_ports:"},{"line_number":1298,"context_line":"                    ip \u003d alloc.ip_address"},{"line_number":1299,"context_line":"                    if ((ip in problem_allocations) and not"},{"line_number":1300,"context_line":"                        (alloc.ports.device_owner in AUTO_DELETE_PORT_OWNERS)):"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F9%2Bis%3D","line":1297,"in_reply_to":"AAAAUH%2F%2Bmv8%3D","updated":"2014-01-31 16:32:43.000000000","message":"hehe :) will revisit when implementing the rest","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1303,"context_line":"                            ip, alloc.port_id)"},{"line_number":1304,"context_line":"                if conflicts:"},{"line_number":1305,"context_line":"                        raise q_exc.IpAllocationConflict(conflicts\u003dconflicts)"},{"line_number":1306,"context_line":"                #at this point safe to delete the old alloc pools:"},{"line_number":1307,"context_line":"                for old_pool in old_pools:"},{"line_number":1308,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1309,"context_line":"                #add new pools:"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm2g%3D","line":1306,"updated":"2014-01-23 23:04:31.000000000","message":"This might be a good comment if it is reasonable to expect that it isn\u0027t clear that it is safe to delete the old alloc pools.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1303,"context_line":"                            ip, alloc.port_id)"},{"line_number":1304,"context_line":"                if conflicts:"},{"line_number":1305,"context_line":"                        raise q_exc.IpAllocationConflict(conflicts\u003dconflicts)"},{"line_number":1306,"context_line":"                #at this point safe to delete the old alloc pools:"},{"line_number":1307,"context_line":"                for old_pool in old_pools:"},{"line_number":1308,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1309,"context_line":"                #add new pools:"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F99%2Bc%3D","line":1306,"in_reply_to":"AAAAUH%2F%2Bm2g%3D","updated":"2014-01-31 16:32:43.000000000","message":"Done","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1310,"context_line":"                for pool in new_pools:"},{"line_number":1311,"context_line":"                    context.session.add(pool)"},{"line_number":1312,"context_line":"                #Gather new pools for result:"},{"line_number":1313,"context_line":"                new_pools \u003d []"},{"line_number":1314,"context_line":"                for pool_dict in s[\u0027allocation_pools\u0027]:"},{"line_number":1315,"context_line":"                    new_pools.append({\u0027start\u0027: pool_dict[\u0027start\u0027],"},{"line_number":1316,"context_line":"                                      \u0027end\u0027: pool_dict[\u0027end\u0027]})"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bm0w%3D","line":1313,"updated":"2014-01-23 23:04:31.000000000","message":"This is an overloaded use of the variable \"new_pools\".  Maybe call this one new_pools_result or something.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9a0d6f65a523bf5ca3bd082e51ddeb7bada9cc4","unresolved":false,"context_lines":[{"line_number":1311,"context_line":"                    context.session.add(pool)"},{"line_number":1312,"context_line":"                #Gather new pools for result:"},{"line_number":1313,"context_line":"                new_pools \u003d []"},{"line_number":1314,"context_line":"                for pool_dict in s[\u0027allocation_pools\u0027]:"},{"line_number":1315,"context_line":"                    new_pools.append({\u0027start\u0027: pool_dict[\u0027start\u0027],"},{"line_number":1316,"context_line":"                                      \u0027end\u0027: pool_dict[\u0027end\u0027]})"},{"line_number":1317,"context_line":"                del s[\u0027allocation_pools\u0027]"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2Bmxc%3D","line":1314,"updated":"2014-01-23 23:04:31.000000000","message":"Consider a list comprehension.","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"c347d0a7f69448ebc183fbef0ac6e4fd3fe815cc","unresolved":false,"context_lines":[{"line_number":1311,"context_line":"                    context.session.add(pool)"},{"line_number":1312,"context_line":"                #Gather new pools for result:"},{"line_number":1313,"context_line":"                new_pools \u003d []"},{"line_number":1314,"context_line":"                for pool_dict in s[\u0027allocation_pools\u0027]:"},{"line_number":1315,"context_line":"                    new_pools.append({\u0027start\u0027: pool_dict[\u0027start\u0027],"},{"line_number":1316,"context_line":"                                      \u0027end\u0027: pool_dict[\u0027end\u0027]})"},{"line_number":1317,"context_line":"                del s[\u0027allocation_pools\u0027]"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F998M%3D","line":1314,"in_reply_to":"AAAAUH%2F%2Bmxc%3D","updated":"2014-01-31 16:32:43.000000000","message":"Done","commit_id":"4e88f2411e9cfac76f6cf198efdb11ae0f458505"},{"author":{"_account_id":2592,"name":"Mark McClain","email":"mark@mcclain.xyz","username":"markmcclain"},"change_message_id":"a66fa372611c5b72f7e9b0ea163a62ddb1a45a82","unresolved":false,"context_lines":[{"line_number":1279,"context_line":"                new_pools \u003d [models_v2.IPAllocationPool("},{"line_number":1280,"context_line":"                    first_ip\u003dp[\u0027start\u0027], last_ip\u003dp[\u0027end\u0027],"},{"line_number":1281,"context_line":"                    subnet_id\u003did) for p in s[\u0027allocation_pools\u0027]]"},{"line_number":1282,"context_line":"                problem_allocations \u003d allocated_ips \u0026 \\"},{"line_number":1283,"context_line":"                    ((_pools_to_ipset(old_pools)) -"},{"line_number":1284,"context_line":"                    (_pools_to_ipset(new_pools)))"},{"line_number":1285,"context_line":"                conflicts \u003d \"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUH%2F97Sg%3D","line":1282,"updated":"2014-02-01 00:19:46.000000000","message":"line continuations are not allowed per our hacking guidelines.  Please wrap this in parenthesis.\n\nproblem_allocations \u003d (\n    allocated_ips \u0026 _pools_to_ipset(old_pools) - \n    _pools_to_ipset(new_pools))","commit_id":"799b5348d6b1d86c68ca99de986cc0710da60049"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"0ab0207053a2a370e96b6d72feac700affdb7502","unresolved":false,"context_lines":[{"line_number":1279,"context_line":"                new_pools \u003d [models_v2.IPAllocationPool("},{"line_number":1280,"context_line":"                    first_ip\u003dp[\u0027start\u0027], last_ip\u003dp[\u0027end\u0027],"},{"line_number":1281,"context_line":"                    subnet_id\u003did) for p in s[\u0027allocation_pools\u0027]]"},{"line_number":1282,"context_line":"                problem_allocations \u003d allocated_ips \u0026 \\"},{"line_number":1283,"context_line":"                    ((_pools_to_ipset(old_pools)) -"},{"line_number":1284,"context_line":"                    (_pools_to_ipset(new_pools)))"},{"line_number":1285,"context_line":"                conflicts \u003d \"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUH%2F9iNU%3D","line":1282,"in_reply_to":"AAAAUH%2F97Sg%3D","updated":"2014-02-06 08:43:34.000000000","message":"Done","commit_id":"799b5348d6b1d86c68ca99de986cc0710da60049"},{"author":{"_account_id":2592,"name":"Mark McClain","email":"mark@mcclain.xyz","username":"markmcclain"},"change_message_id":"a66fa372611c5b72f7e9b0ea163a62ddb1a45a82","unresolved":false,"context_lines":[{"line_number":1287,"context_line":"                    ip \u003d alloc.ip_address"},{"line_number":1288,"context_line":"                    if ((ip in problem_allocations) and not"},{"line_number":1289,"context_line":"                        (alloc.ports.device_owner in AUTO_DELETE_PORT_OWNERS)):"},{"line_number":1290,"context_line":"                        #collect all errors so they can be reported at once"},{"line_number":1291,"context_line":"                        conflicts +\u003d \" addr %s (port %s), \" % ("},{"line_number":1292,"context_line":"                            ip, alloc.port_id)"},{"line_number":1293,"context_line":"                if conflicts:"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUH%2F97Sc%3D","line":1290,"updated":"2014-02-01 00:19:46.000000000","message":"Why using string concatenation here.  I\u0027d collection the items in a conflicts list.  Replace 1285-1292 with:\n\nconflicts \u003d [\n    (alloc.ip_address, alloc_port_id)\n    for alloc in ip_allocs_with_ports\n    if (alloc.ip_addres in problem_allocations and not \n        (alloc.ports.device_owner in AUTO_DELETE_PORT_OWNERS))\n]","commit_id":"799b5348d6b1d86c68ca99de986cc0710da60049"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"0ab0207053a2a370e96b6d72feac700affdb7502","unresolved":false,"context_lines":[{"line_number":1287,"context_line":"                    ip \u003d alloc.ip_address"},{"line_number":1288,"context_line":"                    if ((ip in problem_allocations) and not"},{"line_number":1289,"context_line":"                        (alloc.ports.device_owner in AUTO_DELETE_PORT_OWNERS)):"},{"line_number":1290,"context_line":"                        #collect all errors so they can be reported at once"},{"line_number":1291,"context_line":"                        conflicts +\u003d \" addr %s (port %s), \" % ("},{"line_number":1292,"context_line":"                            ip, alloc.port_id)"},{"line_number":1293,"context_line":"                if conflicts:"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUH%2F9iMg%3D","line":1290,"in_reply_to":"AAAAUH%2F97Sc%3D","updated":"2014-02-06 08:43:34.000000000","message":"thanks!","commit_id":"799b5348d6b1d86c68ca99de986cc0710da60049"},{"author":{"_account_id":2592,"name":"Mark McClain","email":"mark@mcclain.xyz","username":"markmcclain"},"change_message_id":"a66fa372611c5b72f7e9b0ea163a62ddb1a45a82","unresolved":false,"context_lines":[{"line_number":1291,"context_line":"                        conflicts +\u003d \" addr %s (port %s), \" % ("},{"line_number":1292,"context_line":"                            ip, alloc.port_id)"},{"line_number":1293,"context_line":"                if conflicts:"},{"line_number":1294,"context_line":"                        raise q_exc.IpAllocationConflict(conflicts\u003dconflicts)"},{"line_number":1295,"context_line":"                for old_pool in old_pools:"},{"line_number":1296,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1297,"context_line":"                for pool in new_pools:"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUH%2F97SY%3D","line":1294,"updated":"2014-02-01 00:19:46.000000000","message":"raise.q_exc.IpAllocationConflict(\n   conflicts\u003d\u0027, \u0027.join(\u0027addr %s, (port %s\u0027 % c for c in conflicts)\n)","commit_id":"799b5348d6b1d86c68ca99de986cc0710da60049"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"0ab0207053a2a370e96b6d72feac700affdb7502","unresolved":false,"context_lines":[{"line_number":1291,"context_line":"                        conflicts +\u003d \" addr %s (port %s), \" % ("},{"line_number":1292,"context_line":"                            ip, alloc.port_id)"},{"line_number":1293,"context_line":"                if conflicts:"},{"line_number":1294,"context_line":"                        raise q_exc.IpAllocationConflict(conflicts\u003dconflicts)"},{"line_number":1295,"context_line":"                for old_pool in old_pools:"},{"line_number":1296,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1297,"context_line":"                for pool in new_pools:"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUH%2F9iMc%3D","line":1294,"in_reply_to":"AAAAUH%2F97SY%3D","updated":"2014-02-06 08:43:34.000000000","message":"Done","commit_id":"799b5348d6b1d86c68ca99de986cc0710da60049"},{"author":{"_account_id":6072,"name":"Eugene Nikanorov","email":"enikanorov@mirantis.com","username":"enikanorov"},"change_message_id":"8ae5eea55b8c3a7b5d50bbe10d77d9a4a63eaace","unresolved":false,"context_lines":[{"line_number":1270,"context_line":"                old_pools \u003d pools_qry.filter_by(subnet_id\u003did).with_lockmode("},{"line_number":1271,"context_line":"                    \u0027update\u0027).all()"},{"line_number":1272,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1273,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1274,"context_line":"                               with_lockmode(\u0027update\u0027))"},{"line_number":1275,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1276,"context_line":"                allocated_ips \u003d netaddr.IPSet("}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F9cX4%3D","line":1273,"updated":"2014-02-06 21:38:32.000000000","message":"Apparently there are no integration tests for these lines of code.\nFrom my experience it seems that with_lockmode(\u0027update\u0027) will not work for postgresql, so this query will just fail.\nOn the other hand, why do we need additional lock after we got one in line 1271?","commit_id":"d0a0e93d2ea2a886b45487e13b6716ae55fef49e"},{"author":{"_account_id":6072,"name":"Eugene Nikanorov","email":"enikanorov@mirantis.com","username":"enikanorov"},"change_message_id":"a3977d6d4afce4e978b8fd3dbba240f6a35425a6","unresolved":false,"context_lines":[{"line_number":1270,"context_line":"                old_pools \u003d pools_qry.filter_by(subnet_id\u003did).with_lockmode("},{"line_number":1271,"context_line":"                    \u0027update\u0027).all()"},{"line_number":1272,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1273,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1274,"context_line":"                               with_lockmode(\u0027update\u0027))"},{"line_number":1275,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1276,"context_line":"                allocated_ips \u003d netaddr.IPSet("}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F9cWs%3D","line":1273,"in_reply_to":"AAAAUH%2F9cX4%3D","updated":"2014-02-06 21:39:30.000000000","message":"I meant that with_lockmode(\u0027update\u0027) will not work for joins in  postgresql","commit_id":"d0a0e93d2ea2a886b45487e13b6716ae55fef49e"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"219a9c2fc2ab9c532c2f02f6cc42364f63e6141e","unresolved":false,"context_lines":[{"line_number":1274,"context_line":"                               with_lockmode(\u0027update\u0027))"},{"line_number":1275,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1276,"context_line":"                allocated_ips \u003d netaddr.IPSet("},{"line_number":1277,"context_line":"                    [netaddr.ipaddress(i[\u0027ip_address\u0027])"},{"line_number":1278,"context_line":"                        for i in ip_allocs_with_ports])"},{"line_number":1279,"context_line":"                new_pools \u003d [models_v2.IPAllocationPool("},{"line_number":1280,"context_line":"                    first_ip\u003dp[\u0027start\u0027], last_ip\u003dp[\u0027end\u0027],"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F9ZEA%3D","line":1277,"updated":"2014-02-07 08:31:35.000000000","message":"netaddr.IPAddress please :)","commit_id":"d0a0e93d2ea2a886b45487e13b6716ae55fef49e"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"66b72a0e0d3028c5ef83d2a59c25af6b222193b5","unresolved":false,"context_lines":[{"line_number":1278,"context_line":"                        for i in ip_allocs_with_ports])"},{"line_number":1279,"context_line":"                new_pools \u003d [models_v2.IPAllocationPool("},{"line_number":1280,"context_line":"                    first_ip\u003dp[\u0027start\u0027], last_ip\u003dp[\u0027end\u0027],"},{"line_number":1281,"context_line":"                    subnet_id\u003did) for p in s[\u0027allocation_pools\u0027]]"},{"line_number":1282,"context_line":"                problem_allocations \u003d ("},{"line_number":1283,"context_line":"                    allocated_ips \u0026 (_pools_to_ipset(old_pools) -"},{"line_number":1284,"context_line":"                                     _pools_to_ipset(new_pools)))"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F9Y%2Fc%3D","line":1281,"updated":"2014-02-07 08:45:23.000000000","message":"This code needs to also create IPAvailabilityRange objects, or the (potentially new) subnets are not usable...","commit_id":"d0a0e93d2ea2a886b45487e13b6716ae55fef49e"},{"author":{"_account_id":6072,"name":"Eugene Nikanorov","email":"enikanorov@mirantis.com","username":"enikanorov"},"change_message_id":"8ae5eea55b8c3a7b5d50bbe10d77d9a4a63eaace","unresolved":false,"context_lines":[{"line_number":1289,"context_line":"                        (alloc.ports.device_owner in AUTO_DELETE_PORT_OWNERS))"},{"line_number":1290,"context_line":"                ]"},{"line_number":1291,"context_line":"                if conflicts:"},{"line_number":1292,"context_line":"                        raise q_exc.IpAllocationConflict(conflicts\u003d\u0027, \u0027.join("},{"line_number":1293,"context_line":"                            \u0027addr %s, port %s\u0027 % c for c in conflicts))"},{"line_number":1294,"context_line":"                for old_pool in old_pools:"},{"line_number":1295,"context_line":"                    context.session.delete(old_pool)"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F9cXs%3D","line":1292,"updated":"2014-02-06 21:38:32.000000000","message":"nit: looks like the line is overindented","commit_id":"d0a0e93d2ea2a886b45487e13b6716ae55fef49e"},{"author":{"_account_id":6072,"name":"Eugene Nikanorov","email":"enikanorov@mirantis.com","username":"enikanorov"},"change_message_id":"a7e000e1e22630526398f68fc2c200db03ecfc96","unresolved":false,"context_lines":[{"line_number":1269,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1270,"context_line":"                old_pools \u003d pools_qry.filter_by(subnet_id\u003did).with_lockmode("},{"line_number":1271,"context_line":"                    \u0027update\u0027).all()"},{"line_number":1272,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1273,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1274,"context_line":"                               with_lockmode(\u0027update\u0027))"},{"line_number":1275,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAUH%2F9YsQ%3D","line":1272,"updated":"2014-02-07 09:31:51.000000000","message":"please see my comment to patchset #6","commit_id":"ca5dd747948e9f95b04333f5cae8aa5136827ccb"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a1973dcfcd4861f988e1851aa4b1b3d1ba99962f","unresolved":false,"context_lines":[{"line_number":1270,"context_line":"                old_pools \u003d pools_qry.filter_by(subnet_id\u003did).with_lockmode("},{"line_number":1271,"context_line":"                    \u0027update\u0027).all()"},{"line_number":1272,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1273,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1274,"context_line":"                               with_lockmode(\u0027update\u0027))"},{"line_number":1275,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1276,"context_line":"                allocated_ips \u003d netaddr.IPSet("}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAAUX%2F%2Bx8M%3D","line":1273,"updated":"2014-02-19 04:38:32.000000000","message":"Please address Eugene\u0027s comments about this join from patch set 6.  Contact him on irc if necessary.  I have seen the problem with join and lock mode update in post gres too.","commit_id":"7918ff71079239ffab9c16a6c65be3a876cbbdb0"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"df8b09c093dfffa7667c2272b1494b303d9f8610","unresolved":false,"context_lines":[{"line_number":1270,"context_line":"                old_pools \u003d pools_qry.filter_by(subnet_id\u003did).with_lockmode("},{"line_number":1271,"context_line":"                    \u0027update\u0027).all()"},{"line_number":1272,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1273,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1274,"context_line":"                               with_lockmode(\u0027update\u0027))"},{"line_number":1275,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1276,"context_line":"                allocated_ips \u003d netaddr.IPSet("}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAAU3%2F%2F4os%3D","line":1273,"in_reply_to":"AAAAUX%2F%2Bx8M%3D","updated":"2014-02-24 10:33:07.000000000","message":"thanks - if i understand enikanorov\u0027s comments and his fix from I148d37f2a1c2a340327d36c240eb8173aee9219a /#/c/43305/ - apparently the problem is with the query to IPAllocationPool, because of incompatibility between the left outer join and the \u0027update\u0027 lockmode (join is from \u0027eager loading\u0027 of the relationship between IPAllocationPool and the IPAvailabilityRange model through the \u0027available_ranges\u0027 attribute)*\n\nwill fix\n\n*I tried to find more info about this issue and failed - does any one know more? Is it indeed specific to postgresql? Do we need to document this somewhere about queries to IPAllocationPool?","commit_id":"7918ff71079239ffab9c16a6c65be3a876cbbdb0"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a1973dcfcd4861f988e1851aa4b1b3d1ba99962f","unresolved":false,"context_lines":[{"line_number":1296,"context_line":"                for pool in new_pools:"},{"line_number":1297,"context_line":"                    context.session.add(pool)"},{"line_number":1298,"context_line":"                    context.session.flush()"},{"line_number":1299,"context_line":"                    # Create IPAllocation Ranges for the new pools."},{"line_number":1300,"context_line":"                    pool_set \u003d netaddr.IPSet("},{"line_number":1301,"context_line":"                        netaddr.iprange_to_cidrs(pool.first_ip, pool.last_ip))"},{"line_number":1302,"context_line":"                    avail_ips \u003d pool_set - allocated_ips"}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAAUX%2F%2Bx94%3D","line":1299,"updated":"2014-02-19 04:38:32.000000000","message":"Rebase this commit and then see the _rebuild_availability_ranges method.   I don\u0027t think any of this code is needed anymore.  Deleting the old pools should cascade delete the availability ranges.  Then, they will be rebuilt when a new ip is to be allocated.","commit_id":"7918ff71079239ffab9c16a6c65be3a876cbbdb0"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"df8b09c093dfffa7667c2272b1494b303d9f8610","unresolved":false,"context_lines":[{"line_number":1296,"context_line":"                for pool in new_pools:"},{"line_number":1297,"context_line":"                    context.session.add(pool)"},{"line_number":1298,"context_line":"                    context.session.flush()"},{"line_number":1299,"context_line":"                    # Create IPAllocation Ranges for the new pools."},{"line_number":1300,"context_line":"                    pool_set \u003d netaddr.IPSet("},{"line_number":1301,"context_line":"                        netaddr.iprange_to_cidrs(pool.first_ip, pool.last_ip))"},{"line_number":1302,"context_line":"                    avail_ips \u003d pool_set - allocated_ips"}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAAU3%2F%2F4UU%3D","line":1299,"in_reply_to":"AAAAUX%2F%2Bx94%3D","updated":"2014-02-24 10:33:07.000000000","message":"i\u0027ll send out an update to address the earlier lockmode issue, will try and find some time to look at the _rebuild_availability_ranges wrt this code. I\u0027ll ping lifeless too since he wrote this part. I\u0027ll thus mark the next review as WIP.","commit_id":"7918ff71079239ffab9c16a6c65be3a876cbbdb0"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"665f24e8740cadeab01b2f832ceb30350ced0151","unresolved":false,"context_lines":[{"line_number":1225,"context_line":"                #see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1226,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1227,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1228,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1229,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1230,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1231,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAVn%2F%2F3d8%3D","line":1228,"updated":"2014-03-03 18:30:05.000000000","message":"There may be still a problem here.  Any pool that happens to not have an availability range associated (i.e. no availability) will not be returned in an inner join.  This needs to be accounted for as it is a valid state.","commit_id":"dbaeaa9e4f27a4a7d48e298b9b5d9dd23c89027d"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"3e2a372c1e3db8b36553a83c844cc781da6759ea","unresolved":false,"context_lines":[{"line_number":1225,"context_line":"                #see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1226,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1227,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1228,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1229,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1230,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1231,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAVn%2F%2Fh4k%3D","line":1228,"in_reply_to":"AAAAVn%2F%2F3d8%3D","updated":"2014-03-05 15:39:39.000000000","message":"If there\u0027s no availability range, we don\u0027t need to fixup the  IPAllocation for it, right?","commit_id":"dbaeaa9e4f27a4a7d48e298b9b5d9dd23c89027d"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"efe31b875556b0686d8be2a7d49345f16319e2c1","unresolved":false,"context_lines":[{"line_number":1225,"context_line":"                #see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1226,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1227,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1228,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1229,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1230,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1231,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAVn%2F%2FOf8%3D","line":1228,"in_reply_to":"AAAAVn%2F%2FWsU%3D","updated":"2014-03-07 13:46:03.000000000","message":"I see. OK I have added an extra merge for the next revision. Since we are dealing with the \u0027lockmode\u0027 issue safely already, I\u0027m hoping it is safe to just use the existing pools_qry from above and filter by subnet_id. I guess it\u0027ll be easier to see/check this in the code! thanks","commit_id":"dbaeaa9e4f27a4a7d48e298b9b5d9dd23c89027d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"2abd2a797cf5bde194946b12ce025815ccc54e15","unresolved":false,"context_lines":[{"line_number":1225,"context_line":"                #see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1226,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1227,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1228,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1229,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1230,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1231,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAVn%2F%2FWsU%3D","line":1228,"in_reply_to":"AAAAVn%2F%2FaSY%3D","updated":"2014-03-06 17:08:26.000000000","message":"This may actually be common.  If the old pool has been completely allocated then there will be no availability ranges.  If someone is trying to modify the pools then it is probably likely he is doing it because the old pool is completely allocated.","commit_id":"dbaeaa9e4f27a4a7d48e298b9b5d9dd23c89027d"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"7cd4dd094c4ea1f4462ee3ab3458228d273bd891","unresolved":false,"context_lines":[{"line_number":1225,"context_line":"                #see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1226,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1227,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1228,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1229,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1230,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1231,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAVn%2F%2FaSY%3D","line":1228,"in_reply_to":"AAAAVn%2F%2FgO4%3D","updated":"2014-03-06 09:00:46.000000000","message":"I can\u0027t see where this situation would arise. AFAICS from the models and create_subnet, allocation_pools will be autoallocated on create (and we claim this in the v2 API too). I can stick another db query here to see if there are in fact any subnets with null allocation pools, but I\u0027m not convinced it is necessary.","commit_id":"dbaeaa9e4f27a4a7d48e298b9b5d9dd23c89027d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"ab0946870ae04fe3077fe8469d2ad1325c5b8ce8","unresolved":false,"context_lines":[{"line_number":1225,"context_line":"                #see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1226,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1227,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1228,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1229,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1230,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1231,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAVn%2F%2FgO4%3D","line":1228,"in_reply_to":"AAAAVn%2F%2Fh4k%3D","updated":"2014-03-05 18:13:53.000000000","message":"We need to delete the old pools.  See lines 1252/3.","commit_id":"dbaeaa9e4f27a4a7d48e298b9b5d9dd23c89027d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"665f24e8740cadeab01b2f832ceb30350ced0151","unresolved":false,"context_lines":[{"line_number":1229,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1230,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1231,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1232,"context_line":"                               with_lockmode(\u0027update\u0027))"},{"line_number":1233,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1234,"context_line":"                allocated_ips \u003d netaddr.IPSet("},{"line_number":1235,"context_line":"                    [netaddr.IPAddress(i[\u0027ip_address\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAVn%2F%2F3dQ%3D","line":1232,"updated":"2014-03-03 18:30:05.000000000","message":"Will this have the same problem with the join and lockmode update?","commit_id":"dbaeaa9e4f27a4a7d48e298b9b5d9dd23c89027d"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"7cd4dd094c4ea1f4462ee3ab3458228d273bd891","unresolved":false,"context_lines":[{"line_number":1229,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1230,"context_line":"                alloc_query \u003d (context.session.query(models_v2.IPAllocation)."},{"line_number":1231,"context_line":"                               filter_by(subnet_id\u003did).join(models_v2.Port)."},{"line_number":1232,"context_line":"                               with_lockmode(\u0027update\u0027))"},{"line_number":1233,"context_line":"                ip_allocs_with_ports \u003d alloc_query.all()"},{"line_number":1234,"context_line":"                allocated_ips \u003d netaddr.IPSet("},{"line_number":1235,"context_line":"                    [netaddr.IPAddress(i[\u0027ip_address\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAVn%2F%2FaPs%3D","line":1232,"in_reply_to":"AAAAVn%2F%2F3dQ%3D","updated":"2014-03-06 09:00:46.000000000","message":"Well, IDK, but I don\u0027t think so. In enikanorov\u0027s review at I148d37f2a1c2a340327d36c240eb8173aee9219a the info seems to be specific to the \u0027eager loading\u0027 of the available_ranges of db.IPAllocationPool model. Looking at db.IPAllocation there are no equivalent orm.relationships (just standard foreign keys on port_id, subnet and network ids).\n\nI tried to find more info about this issue and failed - does any one know more? Is it indeed specific to postgresql? Do we need to document this somewhere about queries to IPAllocationPool? Do/can we change IPAllocationPool \u0027available_ranges\u0027 attribute to something other than orm.relationship? IMO all these things can/should be addressed in a different review.","commit_id":"dbaeaa9e4f27a4a7d48e298b9b5d9dd23c89027d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"665f24e8740cadeab01b2f832ceb30350ced0151","unresolved":false,"context_lines":[{"line_number":1240,"context_line":"                problem_allocations \u003d ("},{"line_number":1241,"context_line":"                    allocated_ips \u0026 (_pools_to_ipset(old_pools) -"},{"line_number":1242,"context_line":"                                     _pools_to_ipset(new_pools)))"},{"line_number":1243,"context_line":"                conflicts \u003d ["},{"line_number":1244,"context_line":"                    (alloc.ip_address, alloc.port_id)"},{"line_number":1245,"context_line":"                    for alloc in ip_allocs_with_ports"},{"line_number":1246,"context_line":"                    if (alloc.ip_address in problem_allocations and not"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAVn%2F%2F3Wo%3D","line":1243,"updated":"2014-03-03 18:30:05.000000000","message":"Should we reconsider how safe we need to be here?  I\u0027m still not quite convinced that we really need to check that these exist and fail if they do.  I don\u0027t know.  On one hand, it is making the DB queries very difficult to get right.","commit_id":"dbaeaa9e4f27a4a7d48e298b9b5d9dd23c89027d"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"7cd4dd094c4ea1f4462ee3ab3458228d273bd891","unresolved":false,"context_lines":[{"line_number":1240,"context_line":"                problem_allocations \u003d ("},{"line_number":1241,"context_line":"                    allocated_ips \u0026 (_pools_to_ipset(old_pools) -"},{"line_number":1242,"context_line":"                                     _pools_to_ipset(new_pools)))"},{"line_number":1243,"context_line":"                conflicts \u003d ["},{"line_number":1244,"context_line":"                    (alloc.ip_address, alloc.port_id)"},{"line_number":1245,"context_line":"                    for alloc in ip_allocs_with_ports"},{"line_number":1246,"context_line":"                    if (alloc.ip_address in problem_allocations and not"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAVn%2F%2FaMM%3D","line":1243,"in_reply_to":"AAAAVn%2F%2F3Wo%3D","updated":"2014-03-06 09:00:46.000000000","message":"IF the general consensus is we don\u0027t need this check and it\u0027s \u0027up to the admin to deal with this kind of stuff\u0027 then so be it; for one will make the method much simpler.\n\nIMO we can and should try and catch these obvious cases of conflicts. It seems reasonable to me that if there are existing IP allocations from the old subnet allocation_pools that aren\u0027t inside the new allocation_pools there will be explosions.","commit_id":"dbaeaa9e4f27a4a7d48e298b9b5d9dd23c89027d"},{"author":{"_account_id":261,"name":"Salvatore Orlando","email":"salv.orlando@gmail.com","username":"salvatore-orlando"},"change_message_id":"05e6686f93d8f4a107208578248283ab8743df68","unresolved":false,"context_lines":[{"line_number":1200,"context_line":"                changed_allocation_pools \u003d True"},{"line_number":1201,"context_line":"                #deal with eagerload issue for postgres and update lockmode"},{"line_number":1202,"context_line":"                #see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1203,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1204,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1205,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1206,"context_line":"                             with_lockmode(\u0027update\u0027))"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAWH%2F%2FMWE%3D","line":1203,"updated":"2014-03-19 15:23:55.000000000","message":"you can add filter_by(subnet_id\u003did) here","commit_id":"54468bd7cae5e9b9db565aa479d92382b0db36f0"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"a5b7838fe09d732b63b5e2115a0d282cc474a965","unresolved":false,"context_lines":[{"line_number":1200,"context_line":"                changed_allocation_pools \u003d True"},{"line_number":1201,"context_line":"                #deal with eagerload issue for postgres and update lockmode"},{"line_number":1202,"context_line":"                #see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1203,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1204,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1205,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1206,"context_line":"                             with_lockmode(\u0027update\u0027))"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAWH%2F9Qto%3D","line":1203,"in_reply_to":"AAAAWH%2F%2FMWE%3D","updated":"2014-04-07 15:39:13.000000000","message":"Done","commit_id":"54468bd7cae5e9b9db565aa479d92382b0db36f0"},{"author":{"_account_id":261,"name":"Salvatore Orlando","email":"salv.orlando@gmail.com","username":"salvatore-orlando"},"change_message_id":"05e6686f93d8f4a107208578248283ab8743df68","unresolved":false,"context_lines":[{"line_number":1203,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1204,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1205,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1206,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1207,"context_line":"                #also make sure we get any pools with no available IPs"},{"line_number":1208,"context_line":"                old_pools \u003d (old_pools.all() +"},{"line_number":1209,"context_line":"                             pools_qry.filter_by(subnet_id\u003did).all())"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAWH%2F%2FMWI%3D","line":1206,"updated":"2014-03-19 15:23:55.000000000","message":"I\u0027m not sure if you need to lock available ranges as well since you\u0027re locking IPAllocations","commit_id":"54468bd7cae5e9b9db565aa479d92382b0db36f0"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"a5b7838fe09d732b63b5e2115a0d282cc474a965","unresolved":false,"context_lines":[{"line_number":1203,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1204,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1205,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1206,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1207,"context_line":"                #also make sure we get any pools with no available IPs"},{"line_number":1208,"context_line":"                old_pools \u003d (old_pools.all() +"},{"line_number":1209,"context_line":"                             pools_qry.filter_by(subnet_id\u003did).all())"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAWH%2F9Qq4%3D","line":1206,"in_reply_to":"AAAAWH%2F%2BXcY%3D","updated":"2014-04-07 15:39:13.000000000","message":"thanks, though i am confused now. I *thought* that\u0027s what I did by using the innerjoin as you did for your review @ I148d37f2a1c2a340327d36c240eb8173aee9219a\n\nI discussed/referred to this in a few places in earlier versions of this review, see for example revision 12 https://review.openstack.org/#/c/62042/12/neutron/db/db_base_plugin_v2.py\n\nOK, so what you\u0027re saying is if I just jock the IPAllocations, I don\u0027t need to lock the IPAllocationPools at all? will do this for the next revision please shout if it isn\u0027t what you meant","commit_id":"54468bd7cae5e9b9db565aa479d92382b0db36f0"},{"author":{"_account_id":6072,"name":"Eugene Nikanorov","email":"enikanorov@mirantis.com","username":"enikanorov"},"change_message_id":"21d78e2da92e8850642bd8f9c88799bed5220b75","unresolved":false,"context_lines":[{"line_number":1203,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1204,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1205,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1206,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1207,"context_line":"                #also make sure we get any pools with no available IPs"},{"line_number":1208,"context_line":"                old_pools \u003d (old_pools.all() +"},{"line_number":1209,"context_line":"                             pools_qry.filter_by(subnet_id\u003did).all())"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAWH%2F%2BXcY%3D","line":1206,"in_reply_to":"AAAAWH%2F%2FMWI%3D","updated":"2014-03-27 10:44:06.000000000","message":"+1 to that.\nAlso, please take a look to the comments that I\u0027ve left for patchset #6","commit_id":"54468bd7cae5e9b9db565aa479d92382b0db36f0"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"a5b7838fe09d732b63b5e2115a0d282cc474a965","unresolved":false,"context_lines":[{"line_number":1203,"context_line":"                pools_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":1204,"context_line":"                old_pools \u003d (pools_qry.filter_by(subnet_id\u003did).options(orm."},{"line_number":1205,"context_line":"                             joinedload(\u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1206,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1207,"context_line":"                #also make sure we get any pools with no available IPs"},{"line_number":1208,"context_line":"                old_pools \u003d (old_pools.all() +"},{"line_number":1209,"context_line":"                             pools_qry.filter_by(subnet_id\u003did).all())"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAWH%2F9QqM%3D","line":1206,"in_reply_to":"AAAAWH%2F%2FMWI%3D","updated":"2014-04-07 15:39:13.000000000","message":"thanks, see below after enikanorov comment","commit_id":"54468bd7cae5e9b9db565aa479d92382b0db36f0"},{"author":{"_account_id":261,"name":"Salvatore Orlando","email":"salv.orlando@gmail.com","username":"salvatore-orlando"},"change_message_id":"05e6686f93d8f4a107208578248283ab8743df68","unresolved":false,"context_lines":[{"line_number":1228,"context_line":"                ]"},{"line_number":1229,"context_line":"                if conflicts:"},{"line_number":1230,"context_line":"                    raise n_exc.IpAllocationConflict(conflicts\u003d\u0027, \u0027.join("},{"line_number":1231,"context_line":"                        \u0027addr %s, port %s\u0027 % c for c in conflicts))"},{"line_number":1232,"context_line":"                for old_pool in old_pools:"},{"line_number":1233,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1234,"context_line":"                for pool in new_pools:"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAWH%2F%2FMUA%3D","line":1231,"updated":"2014-03-19 15:23:55.000000000","message":"I reckon that perhaps there is no need to raise an IpAllocationConflict exception. If you agree, we shall be able to simplify the logic of this method as well.\n\nThe reason is that Neutron allows allocating IP addresses outside of pools. When the allocation pools are updated, those IPs which currently end up in your conflicts list will be IPs outside of their pools, which are still allowed by Neutron.","commit_id":"54468bd7cae5e9b9db565aa479d92382b0db36f0"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"a5b7838fe09d732b63b5e2115a0d282cc474a965","unresolved":false,"context_lines":[{"line_number":1228,"context_line":"                ]"},{"line_number":1229,"context_line":"                if conflicts:"},{"line_number":1230,"context_line":"                    raise n_exc.IpAllocationConflict(conflicts\u003d\u0027, \u0027.join("},{"line_number":1231,"context_line":"                        \u0027addr %s, port %s\u0027 % c for c in conflicts))"},{"line_number":1232,"context_line":"                for old_pool in old_pools:"},{"line_number":1233,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1234,"context_line":"                for pool in new_pools:"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAWH%2F9Qno%3D","line":1231,"in_reply_to":"AAAAWH%2F%2FMUA%3D","updated":"2014-04-07 15:39:13.000000000","message":"I see, do you think a LOG message will be enough then? Since we already have the logic in place to determine the conflicts? I will do that for next revision and you can let me know what you think","commit_id":"54468bd7cae5e9b9db565aa479d92382b0db36f0"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"8b5f0fdbd2aae65aa89819f51bc2ae0fe54f67aa","unresolved":false,"context_lines":[{"line_number":1228,"context_line":"                ]"},{"line_number":1229,"context_line":"                if conflicts:"},{"line_number":1230,"context_line":"                    raise n_exc.IpAllocationConflict(conflicts\u003d\u0027, \u0027.join("},{"line_number":1231,"context_line":"                        \u0027addr %s, port %s\u0027 % c for c in conflicts))"},{"line_number":1232,"context_line":"                for old_pool in old_pools:"},{"line_number":1233,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1234,"context_line":"                for pool in new_pools:"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAWn%2F%2F7a8%3D","line":1231,"in_reply_to":"AAAAWH%2F9P9U%3D","updated":"2014-04-08 12:57:42.000000000","message":"well, ok... I understand that Neutron allows IP allocation outside of subnet allocation pools. Now we could document the fact that when updating an allocation pool, admins should be aware that pre-update allocations may then fall outside the updated pool range. I guess I just don\u0027t see the harm in logging this fact when it does occur, e.g. accidentally.\n\nI will remove the logic entirely from my next revision...","commit_id":"54468bd7cae5e9b9db565aa479d92382b0db36f0"},{"author":{"_account_id":261,"name":"Salvatore Orlando","email":"salv.orlando@gmail.com","username":"salvatore-orlando"},"change_message_id":"f603579bded4dd3c039dc8dc203c85d9d9d1ac8a","unresolved":false,"context_lines":[{"line_number":1228,"context_line":"                ]"},{"line_number":1229,"context_line":"                if conflicts:"},{"line_number":1230,"context_line":"                    raise n_exc.IpAllocationConflict(conflicts\u003d\u0027, \u0027.join("},{"line_number":1231,"context_line":"                        \u0027addr %s, port %s\u0027 % c for c in conflicts))"},{"line_number":1232,"context_line":"                for old_pool in old_pools:"},{"line_number":1233,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1234,"context_line":"                for pool in new_pools:"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAAWH%2F9P9U%3D","line":1231,"in_reply_to":"AAAAWH%2F9Qno%3D","updated":"2014-04-07 16:58:27.000000000","message":"If there is a valuable use case in logging conflicts, I am happy to keep this logic.\nOtherwise, we can skip it.","commit_id":"54468bd7cae5e9b9db565aa479d92382b0db36f0"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"68096ff53ddab7455e3e8a92e1637ba3103f1a4e","unresolved":false,"context_lines":[{"line_number":1267,"context_line":"                        {\u0027destination\u0027: route_str.partition(\"_\")[0],"},{"line_number":1268,"context_line":"                         \u0027nexthop\u0027: route_str.partition(\"_\")[2]})"},{"line_number":1269,"context_line":"                del s[\"host_routes\"]"},{"line_number":1270,"context_line":""},{"line_number":1271,"context_line":"            def _pools_to_ipset(pools):"},{"line_number":1272,"context_line":"                ipset \u003d netaddr.IPSet()"},{"line_number":1273,"context_line":"                for pool in pools:"}],"source_content_type":"text/x-python","patch_set":16,"id":"AAAAXX%2F%2Fyls%3D","line":1270,"updated":"2014-04-27 13:36:19.000000000","message":"Unused function?","commit_id":"71c815ac4ed28eb4123cb329ce505049779d1108"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"7acf3065366fd0c8c10f42a3df9674334b24e7f0","unresolved":false,"context_lines":[{"line_number":1267,"context_line":"                        {\u0027destination\u0027: route_str.partition(\"_\")[0],"},{"line_number":1268,"context_line":"                         \u0027nexthop\u0027: route_str.partition(\"_\")[2]})"},{"line_number":1269,"context_line":"                del s[\"host_routes\"]"},{"line_number":1270,"context_line":""},{"line_number":1271,"context_line":"            def _pools_to_ipset(pools):"},{"line_number":1272,"context_line":"                ipset \u003d netaddr.IPSet()"},{"line_number":1273,"context_line":"                for pool in pools:"}],"source_content_type":"text/x-python","patch_set":16,"id":"AAAAXX%2F%2Fs6Y%3D","line":1270,"in_reply_to":"AAAAXX%2F%2Fyls%3D","updated":"2014-04-28 15:32:01.000000000","message":"indeed! nice catch, sorry it is from an earlier version of the review","commit_id":"71c815ac4ed28eb4123cb329ce505049779d1108"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"ab9bd08b6419cff0e83d8b6c138e44aa09e4f13f","unresolved":false,"context_lines":[{"line_number":1282,"context_line":"                self._validate_allocation_pools(s[\u0027allocation_pools\u0027],"},{"line_number":1283,"context_line":"                                                s[\u0027cidr\u0027])"},{"line_number":1284,"context_line":"                changed_allocation_pools \u003d True"},{"line_number":1285,"context_line":"                # deal with eagerload issue for postgres and update lockmode"},{"line_number":1286,"context_line":"                # see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1287,"context_line":"                pools_qry \u003d (context.session.query(models_v2.IPAllocationPool)."},{"line_number":1288,"context_line":"                             filter_by(subnet_id\u003did))"}],"source_content_type":"text/x-python","patch_set":17,"id":"bad4c104_43817b8e","line":1285,"updated":"2014-04-29 21:53:13.000000000","message":"I did not catch this before.  I\u0027m sorry that I did not follow the progression of this patch as closely as I should have.\n\nThere is no more lockmode(\u0027update\u0027) here.  So, we don\u0027t need to work around postgres anymore with an innerjoin and a second query.\n\nThat got me thinking that this could be even simpler.  In the current revision of this patch, the only reason we need to know \"old_pools\" is to delete them.  In regular old SQL that would be silly because we\u0027d be doing two queries to the db when the following would do one:\n\nDELETE FROM IPAllocationPool WHERE subnet_id \u003d id;\n\nThere must be a way to accomplish this with sqlalchemy.  You\u0027d probably have to make sure to delete the availability ranges as well.","commit_id":"679c514a15e7c6204c16b10b5f272c9545d70392"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"7ed68d6046c42685a9f274fed2e2987e5032816b","unresolved":false,"context_lines":[{"line_number":1282,"context_line":"                self._validate_allocation_pools(s[\u0027allocation_pools\u0027],"},{"line_number":1283,"context_line":"                                                s[\u0027cidr\u0027])"},{"line_number":1284,"context_line":"                changed_allocation_pools \u003d True"},{"line_number":1285,"context_line":"                # deal with eagerload issue for postgres and update lockmode"},{"line_number":1286,"context_line":"                # see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1287,"context_line":"                pools_qry \u003d (context.session.query(models_v2.IPAllocationPool)."},{"line_number":1288,"context_line":"                             filter_by(subnet_id\u003did))"}],"source_content_type":"text/x-python","patch_set":17,"id":"9ad9bd40_1304137a","line":1285,"in_reply_to":"bad4c104_43817b8e","updated":"2014-05-01 13:09:46.000000000","message":"thanks Carl... hmm... so actually I think I need to add the lockmode here again. It was removed because there was a subnsequent query and lock on IPAllocations (see [1] ). - when we were trying to handle allocation conflicts here. In which case I\u0027ll leave the code as is and add the lockmode (unless you think we don\u0027t need it at all?). \n\nAlso apologies but am not entirely clear what you mean about simplification. Are you referring to lines 1296-1299 below (delete old, add new pools)? \n\n[1] https://review.openstack.org/#/c/62042/13/neutron/db/db_base_plugin_v2.py","commit_id":"679c514a15e7c6204c16b10b5f272c9545d70392"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"ab9bd08b6419cff0e83d8b6c138e44aa09e4f13f","unresolved":false,"context_lines":[{"line_number":1297,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1298,"context_line":"                for pool in new_pools:"},{"line_number":1299,"context_line":"                    context.session.add(pool)"},{"line_number":1300,"context_line":"                NeutronDbPluginV2._rebuild_availability_ranges(context, [s])"},{"line_number":1301,"context_line":"                #Gather new pools for result:"},{"line_number":1302,"context_line":"                result_pools \u003d [{\u0027start\u0027: pool[\u0027start\u0027],"},{"line_number":1303,"context_line":"                                 \u0027end\u0027: pool[\u0027end\u0027]}"}],"source_content_type":"text/x-python","patch_set":17,"id":"9ad9bd40_1fffe302","line":1300,"updated":"2014-04-29 21:53:13.000000000","message":"It might not be necessary to run this.  _rebuild_availability_ranges will be run on the next call to allocate an IP address.\n\nI\u0027ll leave it up to you but if it isn\u0027t necessary to run it it would make the patch even simpler.","commit_id":"679c514a15e7c6204c16b10b5f272c9545d70392"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"3dc20e32d694b11f903ead3c2b17dd1e515f4783","unresolved":false,"context_lines":[{"line_number":1297,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1298,"context_line":"                for pool in new_pools:"},{"line_number":1299,"context_line":"                    context.session.add(pool)"},{"line_number":1300,"context_line":"                NeutronDbPluginV2._rebuild_availability_ranges(context, [s])"},{"line_number":1301,"context_line":"                #Gather new pools for result:"},{"line_number":1302,"context_line":"                result_pools \u003d [{\u0027start\u0027: pool[\u0027start\u0027],"},{"line_number":1303,"context_line":"                                 \u0027end\u0027: pool[\u0027end\u0027]}"}],"source_content_type":"text/x-python","patch_set":17,"id":"9ad9bd40_7349dfcf","line":1300,"in_reply_to":"9ad9bd40_1fffe302","updated":"2014-05-01 13:11:12.000000000","message":"I think it makes sense to invoke it at this time since we\u0027re changing the pools (but didn\u0027t realise that\u0027s when it is invoked so thanks)","commit_id":"679c514a15e7c6204c16b10b5f272c9545d70392"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"b885d766452a5d8528d53262212b39961200f848","unresolved":false,"context_lines":[{"line_number":1296,"context_line":"                    subnet_id\u003did) for p in s[\u0027allocation_pools\u0027]]"},{"line_number":1297,"context_line":"                for old_pool in old_pools:"},{"line_number":1298,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1299,"context_line":"                for pool in new_pools:"},{"line_number":1300,"context_line":"                    context.session.add(pool)"},{"line_number":1301,"context_line":"                NeutronDbPluginV2._rebuild_availability_ranges(context, [s])"},{"line_number":1302,"context_line":"                #Gather new pools for result:"}],"source_content_type":"text/x-python","patch_set":18,"id":"5adbc538_4387da61","line":1299,"updated":"2014-05-24 00:44:03.000000000","message":"context.session.add_all(pools)","commit_id":"ff2550f157f0768f762918abf535b669f7a67ee6"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"1816b3ce24ac8f9483e8f1e0c726175fdc883939","unresolved":false,"context_lines":[{"line_number":1296,"context_line":"                    subnet_id\u003did) for p in s[\u0027allocation_pools\u0027]]"},{"line_number":1297,"context_line":"                for old_pool in old_pools:"},{"line_number":1298,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1299,"context_line":"                for pool in new_pools:"},{"line_number":1300,"context_line":"                    context.session.add(pool)"},{"line_number":1301,"context_line":"                NeutronDbPluginV2._rebuild_availability_ranges(context, [s])"},{"line_number":1302,"context_line":"                #Gather new pools for result:"}],"source_content_type":"text/x-python","patch_set":18,"id":"5adbc538_f1c35036","line":1299,"in_reply_to":"5adbc538_4387da61","updated":"2014-05-24 07:47:24.000000000","message":"Thanks!","commit_id":"ff2550f157f0768f762918abf535b669f7a67ee6"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"e3684de87f8ec1b2337b0447c3cc5bfba7600729","unresolved":false,"context_lines":[{"line_number":1213,"context_line":""},{"line_number":1214,"context_line":"        return self._make_subnet_dict(subnet)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def update_subnet(self, context, id, subnet):"},{"line_number":1217,"context_line":"        \"\"\"Update the subnet with new info."},{"line_number":1218,"context_line":""},{"line_number":1219,"context_line":"        The change however will not be realized until the client renew the"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ae8d1ca_efdb1b19","line":1216,"updated":"2014-05-28 22:29:46.000000000","message":"This method is getting huge! Is there any chance we can take this as an opportunity to break it down in manageable chunks?","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"1014629e7ca3122616701f16a06223d3a50a10bb","unresolved":false,"context_lines":[{"line_number":1213,"context_line":""},{"line_number":1214,"context_line":"        return self._make_subnet_dict(subnet)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def update_subnet(self, context, id, subnet):"},{"line_number":1217,"context_line":"        \"\"\"Update the subnet with new info."},{"line_number":1218,"context_line":""},{"line_number":1219,"context_line":"        The change however will not be realized until the client renew the"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ae8d1ca_9377003a","line":1216,"in_reply_to":"3ae8d1ca_8df283d0","updated":"2014-05-29 18:02:40.000000000","message":"True, but the counter-argument is that a refactoring make it easier to review.","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"af26ec406210f9abb49b1408bf4c1e60594baf01","unresolved":false,"context_lines":[{"line_number":1213,"context_line":""},{"line_number":1214,"context_line":"        return self._make_subnet_dict(subnet)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def update_subnet(self, context, id, subnet):"},{"line_number":1217,"context_line":"        \"\"\"Update the subnet with new info."},{"line_number":1218,"context_line":""},{"line_number":1219,"context_line":"        The change however will not be realized until the client renew the"}],"source_content_type":"text/x-python","patch_set":19,"id":"1ae5cdf2_4fd53d42","line":1216,"in_reply_to":"3ae8d1ca_9377003a","updated":"2014-05-29 22:43:20.000000000","message":"Here\u0027s how I would do it...  I would refactor the original code and submit a patch.  Then, I would add this code as a dependent patch.  Possibly the other way around.  I think splitting refactoring from new code also makes it easier to review.","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"01e2f7b257d4f9b17a406e5e4d71399ad0c67f1c","unresolved":false,"context_lines":[{"line_number":1213,"context_line":""},{"line_number":1214,"context_line":"        return self._make_subnet_dict(subnet)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def update_subnet(self, context, id, subnet):"},{"line_number":1217,"context_line":"        \"\"\"Update the subnet with new info."},{"line_number":1218,"context_line":""},{"line_number":1219,"context_line":"        The change however will not be realized until the client renew the"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ae8d1ca_b8e30232","line":1216,"in_reply_to":"3ae8d1ca_ad3669e5","updated":"2014-05-29 14:32:31.000000000","message":"will do","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"768c0a10d4c2c6adafceb5441754e588e1d20bde","unresolved":false,"context_lines":[{"line_number":1213,"context_line":""},{"line_number":1214,"context_line":"        return self._make_subnet_dict(subnet)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def update_subnet(self, context, id, subnet):"},{"line_number":1217,"context_line":"        \"\"\"Update the subnet with new info."},{"line_number":1218,"context_line":""},{"line_number":1219,"context_line":"        The change however will not be realized until the client renew the"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ae8d1ca_8df283d0","line":1216,"in_reply_to":"3ae8d1ca_b8e30232","updated":"2014-05-29 17:13:13.000000000","message":"I probably would have done the refactoring in a different patch to keep this one focused on its intended purpose.  Patch set 20 will now require a bit more work to review because it includes the refactoring.","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":261,"name":"Salvatore Orlando","email":"salv.orlando@gmail.com","username":"salvatore-orlando"},"change_message_id":"875769f53a2546cb20c7f2774a01698a03e05f8c","unresolved":false,"context_lines":[{"line_number":1213,"context_line":""},{"line_number":1214,"context_line":"        return self._make_subnet_dict(subnet)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def update_subnet(self, context, id, subnet):"},{"line_number":1217,"context_line":"        \"\"\"Update the subnet with new info."},{"line_number":1218,"context_line":""},{"line_number":1219,"context_line":"        The change however will not be realized until the client renew the"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ae8d1ca_ad3669e5","line":1216,"in_reply_to":"3ae8d1ca_efdb1b19","updated":"2014-05-29 12:54:41.000000000","message":"well, it was already large enough before. If this patch\u0027s author is ok with a bit of refactoring then it will be welcome.","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":261,"name":"Salvatore Orlando","email":"salv.orlando@gmail.com","username":"salvatore-orlando"},"change_message_id":"875769f53a2546cb20c7f2774a01698a03e05f8c","unresolved":false,"context_lines":[{"line_number":1299,"context_line":"                             filter_by(subnet_id\u003did))"},{"line_number":1300,"context_line":"                old_pools \u003d (pools_qry.options(orm.joinedload("},{"line_number":1301,"context_line":"                             \u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1302,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1303,"context_line":"                # also make sure we get any pools with no available IPs"},{"line_number":1304,"context_line":"                old_pools \u003d (old_pools.all() + pools_qry.all())"},{"line_number":1305,"context_line":"                new_pools \u003d [models_v2.IPAllocationPool("}],"source_content_type":"text/x-python","patch_set":19,"id":"3ae8d1ca_6dcf01b1","line":1302,"updated":"2014-05-29 12:54:41.000000000","message":"is it important to lock here?\nThese records are going to be deleted anyway.\nIt is interesting to see how we should treat concurrent IP allocations in this case. Is it ok to allocate from old pools, and then have those allocation as out-of-pool once the pools are updated?\n\nIs it important they use the new pools?\nWhat if the request which ends up allocating an IP was submitted before the subnet update - should it use the old or the new pools?\n\nThese might be dumb questions, but an answer might really help.","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"38e65de93fe9613f26d2a7f5de76ec710c8b2294","unresolved":false,"context_lines":[{"line_number":1299,"context_line":"                             filter_by(subnet_id\u003did))"},{"line_number":1300,"context_line":"                old_pools \u003d (pools_qry.options(orm.joinedload("},{"line_number":1301,"context_line":"                             \u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1302,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1303,"context_line":"                # also make sure we get any pools with no available IPs"},{"line_number":1304,"context_line":"                old_pools \u003d (old_pools.all() + pools_qry.all())"},{"line_number":1305,"context_line":"                new_pools \u003d [models_v2.IPAllocationPool("}],"source_content_type":"text/x-python","patch_set":19,"id":"1ae5cdf2_9f13a15b","line":1302,"in_reply_to":"3ae8d1ca_4d279b12","updated":"2014-06-02 11:46:40.000000000","message":"OK, see comment on ver 20 @ May 30 2:58 PM","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"768c0a10d4c2c6adafceb5441754e588e1d20bde","unresolved":false,"context_lines":[{"line_number":1299,"context_line":"                             filter_by(subnet_id\u003did))"},{"line_number":1300,"context_line":"                old_pools \u003d (pools_qry.options(orm.joinedload("},{"line_number":1301,"context_line":"                             \u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1302,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1303,"context_line":"                # also make sure we get any pools with no available IPs"},{"line_number":1304,"context_line":"                old_pools \u003d (old_pools.all() + pools_qry.all())"},{"line_number":1305,"context_line":"                new_pools \u003d [models_v2.IPAllocationPool("}],"source_content_type":"text/x-python","patch_set":19,"id":"3ae8d1ca_4d279b12","line":1302,"in_reply_to":"3ae8d1ca_552647cf","updated":"2014-05-29 17:13:13.000000000","message":"I wondered about the same thing back in patch set 17 when the lock had actually been removed.  Marios added the lock back in and I decided that I was okay with that at the time.\n\nI don\u0027t think it is important to lock the pools for the reasons that Salvatore stated.\n\nI still wonder about the cascade delete to the available_ranges table.  What we don\u0027t want is for the available_ranges to somehow be rebuilt from the old pools after they are deleted.  Does this lock even help there?  I can\u0027t seem to work in my head out how the transaction, lock for update, cascading delete and all that work together and whether there is a possible race condition that could leave available_ranges in a bad state.","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"01e2f7b257d4f9b17a406e5e4d71399ad0c67f1c","unresolved":false,"context_lines":[{"line_number":1299,"context_line":"                             filter_by(subnet_id\u003did))"},{"line_number":1300,"context_line":"                old_pools \u003d (pools_qry.options(orm.joinedload("},{"line_number":1301,"context_line":"                             \u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1302,"context_line":"                             with_lockmode(\u0027update\u0027))"},{"line_number":1303,"context_line":"                # also make sure we get any pools with no available IPs"},{"line_number":1304,"context_line":"                old_pools \u003d (old_pools.all() + pools_qry.all())"},{"line_number":1305,"context_line":"                new_pools \u003d [models_v2.IPAllocationPool("}],"source_content_type":"text/x-python","patch_set":19,"id":"3ae8d1ca_552647cf","line":1302,"in_reply_to":"3ae8d1ca_6dcf01b1","updated":"2014-05-29 14:32:31.000000000","message":"Right. So we are locking on the available ranges of the allocation pool, which hopefully prevents these being changed (i.e. address allocation) during the update.\n\nAs you mention, a related issue is what to do with conflicts. I originally had some conflict detection logic here - initially exploding e.g. rev 14 [1], then logging e.g. rev 15 [2]. Actually it was on your suggestion that I removed that code after discussion in rev 13 [3]. Do you think this would help, at least logging of conflicts. \n\nWRT the \u0027out-of-pool\u0027 allocations, I think that should be fine (i.e. existing assigned addresses that are now outside allocation range), allocation already occurs outside of allocation_pools right?\n\n[1] https://review.openstack.org/#/c/62042/14/neutron/db/db_base_plugin_v2.py\n\n[2] https://review.openstack.org/#/c/62042/15/neutron/db/db_base_plugin_v2.py\n\n[3] https://review.openstack.org/#/c/62042/13/neutron/db/db_base_plugin_v2.py","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"316c1729e27c512a1f90f03c261a5471e3d84c3d","unresolved":false,"context_lines":[{"line_number":1306,"context_line":"                    first_ip\u003dp[\u0027start\u0027], last_ip\u003dp[\u0027end\u0027],"},{"line_number":1307,"context_line":"                    subnet_id\u003did) for p in s[\u0027allocation_pools\u0027]]"},{"line_number":1308,"context_line":"                for old_pool in old_pools:"},{"line_number":1309,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1310,"context_line":"                context.session.add_all(new_pools)"},{"line_number":1311,"context_line":"                NeutronDbPluginV2._rebuild_availability_ranges(context, [s])"},{"line_number":1312,"context_line":"                #Gather new pools for result:"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ae8d1ca_c059d3ff","line":1309,"updated":"2014-05-28 20:25:46.000000000","message":"Assaf, all of the old pools are deleted.","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"316c1729e27c512a1f90f03c261a5471e3d84c3d","unresolved":false,"context_lines":[{"line_number":1308,"context_line":"                for old_pool in old_pools:"},{"line_number":1309,"context_line":"                    context.session.delete(old_pool)"},{"line_number":1310,"context_line":"                context.session.add_all(new_pools)"},{"line_number":1311,"context_line":"                NeutronDbPluginV2._rebuild_availability_ranges(context, [s])"},{"line_number":1312,"context_line":"                #Gather new pools for result:"},{"line_number":1313,"context_line":"                result_pools \u003d [{\u0027start\u0027: pool[\u0027start\u0027],"},{"line_number":1314,"context_line":"                                 \u0027end\u0027: pool[\u0027end\u0027]}"}],"source_content_type":"text/x-python","patch_set":19,"id":"3ae8d1ca_6051dfd7","line":1311,"updated":"2014-05-28 20:25:46.000000000","message":"Assaf, this call resets the availability ranges.  This code does set calculations to specifically exclude any IPs that have already been allocated.","commit_id":"a74e3bea4efa3f36804f29e0ec4aa89e97d698ba"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d8f8b6c09f0a9b3449a42aecb9985f549fb25576","unresolved":false,"context_lines":[{"line_number":1213,"context_line":""},{"line_number":1214,"context_line":"        return self._make_subnet_dict(subnet)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def _update_subnet_dns_nameservers(self, context, id, s):"},{"line_number":1217,"context_line":"        old_dns_list \u003d self._get_dns_by_subnet(context, id)"},{"line_number":1218,"context_line":"        new_dns_addr_set \u003d set(s[\"dns_nameservers\"])"},{"line_number":1219,"context_line":"        old_dns_addr_set \u003d set([dns[\u0027address\u0027]"}],"source_content_type":"text/x-python","patch_set":20,"id":"3ae8d1ca_edcb071c","line":1216,"updated":"2014-05-29 17:21:01.000000000","message":"I\u0027d prefer that this patch *not* touch this code.  It isn\u0027t related to the stated purpose of the patch.\n\nImagine that we have a problem with this patch and we have to revert.  A clean revert takes the refactor with it which may not be necessary.\n\nSince this refactoring is independent of the patch, I\u0027d prefer you submit it as a follow-on.\n\nTo be clear, I don\u0027t argue that this refactoring should not be done.  I only argue that it shouldn\u0027t be included with this patch.","commit_id":"13ccb9970c1181186fadd15258e68b300240cc10"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"cd22d156bebd1ed3690d66094123fda4c3a6907e","unresolved":false,"context_lines":[{"line_number":1213,"context_line":""},{"line_number":1214,"context_line":"        return self._make_subnet_dict(subnet)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def _update_subnet_dns_nameservers(self, context, id, s):"},{"line_number":1217,"context_line":"        old_dns_list \u003d self._get_dns_by_subnet(context, id)"},{"line_number":1218,"context_line":"        new_dns_addr_set \u003d set(s[\"dns_nameservers\"])"},{"line_number":1219,"context_line":"        old_dns_addr_set \u003d set([dns[\u0027address\u0027]"}],"source_content_type":"text/x-python","patch_set":20,"id":"1ae5cdf2_9f3ac1cb","line":1216,"in_reply_to":"3ae8d1ca_edcb071c","updated":"2014-06-02 11:46:49.000000000","message":"OK see comment on ver 20  @ May 30 2:58 PM","commit_id":"13ccb9970c1181186fadd15258e68b300240cc10"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d8f8b6c09f0a9b3449a42aecb9985f549fb25576","unresolved":false,"context_lines":[{"line_number":1232,"context_line":"        del s[\"dns_nameservers\"]"},{"line_number":1233,"context_line":"        return new_dns"},{"line_number":1234,"context_line":""},{"line_number":1235,"context_line":"    def _update_subnet_host_routes(self, context, id, s):"},{"line_number":1236,"context_line":""},{"line_number":1237,"context_line":"        def _combine(ht):"},{"line_number":1238,"context_line":"            return ht[\u0027destination\u0027] + \"_\" + ht[\u0027nexthop\u0027]"}],"source_content_type":"text/x-python","patch_set":20,"id":"3ae8d1ca_6dd7f7bf","line":1235,"updated":"2014-05-29 17:21:01.000000000","message":"Same with this refactor.","commit_id":"13ccb9970c1181186fadd15258e68b300240cc10"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d8f8b6c09f0a9b3449a42aecb9985f549fb25576","unresolved":false,"context_lines":[{"line_number":1265,"context_line":"        del s[\"host_routes\"]"},{"line_number":1266,"context_line":"        return new_routes"},{"line_number":1267,"context_line":""},{"line_number":1268,"context_line":"    def _update_subnet_allocation_pools(self, context, id, s):"},{"line_number":1269,"context_line":"        # deal with eagerload issue for postgres and update lockmode"},{"line_number":1270,"context_line":"        # see I148d37f2a1c2a340327d36c240eb8173aee9219a"},{"line_number":1271,"context_line":"        pools_qry \u003d (context.session.query(models_v2.IPAllocationPool)."}],"source_content_type":"text/x-python","patch_set":20,"id":"3ae8d1ca_2d5faf73","line":1268,"updated":"2014-05-29 17:21:01.000000000","message":"Breaking out this method is fine and good since this is the new code.","commit_id":"13ccb9970c1181186fadd15258e68b300240cc10"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d8f8b6c09f0a9b3449a42aecb9985f549fb25576","unresolved":false,"context_lines":[{"line_number":1272,"context_line":"                     filter_by(subnet_id\u003did))"},{"line_number":1273,"context_line":"        old_pools \u003d (pools_qry.options(orm.joinedload("},{"line_number":1274,"context_line":"                     \u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1275,"context_line":"                     with_lockmode(\u0027update\u0027))"},{"line_number":1276,"context_line":"        # also make sure we get any pools with no available IPs"},{"line_number":1277,"context_line":"        old_pools \u003d (old_pools.all() + pools_qry.all())"},{"line_number":1278,"context_line":"        new_pools \u003d [models_v2.IPAllocationPool("}],"source_content_type":"text/x-python","patch_set":20,"id":"3ae8d1ca_6dac5736","line":1275,"updated":"2014-05-29 17:21:01.000000000","message":"I added some comments to PS 19 in reply to Salvatore\u0027s.  I\u0027m tending to think that the lockmode update doesn\u0027t with the cascade delete.  Maybe you can explain to me how this will work in the DB.\n\nIf we decide to go without the lock here, please revisit my comments in PS17 about simplifying the whole thing.","commit_id":"13ccb9970c1181186fadd15258e68b300240cc10"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"43eefb28017ddc0dcfeb3d0a02ff3c8186d7ecd3","unresolved":false,"context_lines":[{"line_number":1222,"context_line":"                     \u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1223,"context_line":"                     with_lockmode(\u0027update\u0027))"},{"line_number":1224,"context_line":"        # also make sure we get any pools with no available IPs"},{"line_number":1225,"context_line":"        old_pools \u003d (old_pools.all() + pools_qry.all())"},{"line_number":1226,"context_line":"        new_pools \u003d [models_v2.IPAllocationPool("},{"line_number":1227,"context_line":"            first_ip\u003dp[\u0027start\u0027], last_ip\u003dp[\u0027end\u0027],"},{"line_number":1228,"context_line":"            subnet_id\u003did) for p in s[\u0027allocation_pools\u0027]]"}],"source_content_type":"text/x-python","patch_set":22,"id":"1ae5cdf2_c0b43237","line":1225,"updated":"2014-06-03 23:04:02.000000000","message":"I might misunderstand this line... anyway here is my thinking:\nOn line 1222 why do you need a joinedload? You never use that information.\n\nI know this problem was raised before anyway let me repeat. I don\u0027t think a lock is needed at line 1223, the IPAllocationPool table is only modified by this method and by create_subnet. The delete and update operations for IPAllocationPool will have to acquire a lock anyway to make this transaction succeed.\n\nThe only problem I can imagine is the following. During a possible context switch after the query at line 1225, another subnet_update request arrives for the same subnet. This second update request acquires the lock before this thread and it\u0027s able to commit the changes. It\u0027s not very likely but in that case we would use stale data instead of the fresh data in the DB.\nA possible fix would be to issue the delete query immediately, Something like this, remove lines 1219-1225 and 1229-1230 and insert:\n\ncontext.session.query(\n    models_v2.IPAllocationPool).filter_by(subnet_id\u003did).delete()\n\n\nNo new IP can be generated for this subnet until the IPAllocationPool table is updated since the table is locked and that lock is required by the code for generating a new IP. Same goes for  ipavailabilityranges table.","commit_id":"b96f7803ccbbe2d5fa827d52041b21b5abcfebe8"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"4c0d99139c5ee30e1c63791c41b8296a52526b4b","unresolved":false,"context_lines":[{"line_number":1222,"context_line":"                     \u0027available_ranges\u0027, innerjoin\u003dTrue))."},{"line_number":1223,"context_line":"                     with_lockmode(\u0027update\u0027))"},{"line_number":1224,"context_line":"        # also make sure we get any pools with no available IPs"},{"line_number":1225,"context_line":"        old_pools \u003d (old_pools.all() + pools_qry.all())"},{"line_number":1226,"context_line":"        new_pools \u003d [models_v2.IPAllocationPool("},{"line_number":1227,"context_line":"            first_ip\u003dp[\u0027start\u0027], last_ip\u003dp[\u0027end\u0027],"},{"line_number":1228,"context_line":"            subnet_id\u003did) for p in s[\u0027allocation_pools\u0027]]"}],"source_content_type":"text/x-python","patch_set":22,"id":"1ae5cdf2_34cdcf66","line":1225,"in_reply_to":"1ae5cdf2_c0b43237","updated":"2014-06-05 15:16:11.000000000","message":"The joinedload is explained in the linked changeid (was discussed back in v 10/12 or thereabouts). I see that in fact that has been reverted now :)\n\nAbout the locking:\navailability_ranges are also changed during IP allocation, not only create and here. I\u0027m kind of torn... \n\nI really like the elegance of your immediate deletion (we have long ago established we don\u0027t care about old_pools... we aren\u0027t looking for conflicts etc) so just deleting them makes sense. BUT, I still feel we can (try our best) to prevent IP allocation from an availability pool which we are currently updating; that involves the lock, complete with joinedload hack. \n\nYou are right that I don\u0027t use that info... I can just use \u0027noload\u0027 to include the lock without loading the ranges like: \n\n       pool_qry \u003d context.session.query(                                      \n            models_v2.IPAllocationPool).options(                                 \n                orm.noload(\u0027available_ranges\u0027)).with_lockmode(\u0027update\u0027)     \n\nAnyway. You are the third person advocating for nolock, so I\u0027ll take the hint. I will simplify it for next v. Let\u0027s see what others think.","commit_id":"b96f7803ccbbe2d5fa827d52041b21b5abcfebe8"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"43eefb28017ddc0dcfeb3d0a02ff3c8186d7ecd3","unresolved":false,"context_lines":[{"line_number":1227,"context_line":"            first_ip\u003dp[\u0027start\u0027], last_ip\u003dp[\u0027end\u0027],"},{"line_number":1228,"context_line":"            subnet_id\u003did) for p in s[\u0027allocation_pools\u0027]]"},{"line_number":1229,"context_line":"        for old_pool in old_pools:"},{"line_number":1230,"context_line":"            context.session.delete(old_pool)"},{"line_number":1231,"context_line":"        context.session.add_all(new_pools)"},{"line_number":1232,"context_line":"        NeutronDbPluginV2._rebuild_availability_ranges(context, [s])"},{"line_number":1233,"context_line":"        #Gather new pools for result:"}],"source_content_type":"text/x-python","patch_set":22,"id":"1ae5cdf2_40f742f5","line":1230,"updated":"2014-06-03 23:04:02.000000000","message":"Because of the cascade relationship this line will cause the deletion of all the ipavailabilityranges associated with the pools that we are deleting. The new ones are going to be created inside the method _rebuild_availability_ranges . Correct?","commit_id":"b96f7803ccbbe2d5fa827d52041b21b5abcfebe8"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"4c0d99139c5ee30e1c63791c41b8296a52526b4b","unresolved":false,"context_lines":[{"line_number":1227,"context_line":"            first_ip\u003dp[\u0027start\u0027], last_ip\u003dp[\u0027end\u0027],"},{"line_number":1228,"context_line":"            subnet_id\u003did) for p in s[\u0027allocation_pools\u0027]]"},{"line_number":1229,"context_line":"        for old_pool in old_pools:"},{"line_number":1230,"context_line":"            context.session.delete(old_pool)"},{"line_number":1231,"context_line":"        context.session.add_all(new_pools)"},{"line_number":1232,"context_line":"        NeutronDbPluginV2._rebuild_availability_ranges(context, [s])"},{"line_number":1233,"context_line":"        #Gather new pools for result:"}],"source_content_type":"text/x-python","patch_set":22,"id":"1ae5cdf2_d40aeb88","line":1230,"in_reply_to":"1ae5cdf2_40f742f5","updated":"2014-06-05 15:16:11.000000000","message":"right","commit_id":"b96f7803ccbbe2d5fa827d52041b21b5abcfebe8"}],"neutron/tests/unit/plumgrid/test_plumgrid_plugin.py":[{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"b885d766452a5d8528d53262212b39961200f848","unresolved":false,"context_lines":[{"line_number":79,"context_line":"        self.skipTest(\"Plugin does not support Neutron allocation process\")"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"class TestPlumgridPluginSubnetsV2(test_plugin.TestSubnetsV2,"},{"line_number":83,"context_line":"                                  PLUMgridPluginV2TestCase):"},{"line_number":84,"context_line":"    def test_create_subnet_default_gw_conflict_allocation_pool_returns_409("},{"line_number":85,"context_line":"            self):"}],"source_content_type":"text/x-python","patch_set":18,"id":"5adbc538_43319aa1","line":82,"updated":"2014-05-24 00:44:03.000000000","message":"replace 84 to 103 with:\n\ndef setUp(self):\n    unsupported \u003d (\n \u0027test_create_subnet_default_gw_conflict_allocation_pool_returns_409\u0027,\n \u0027test_create_subnet_defaults\u0027, \u0027test_create_subnet_gw_values\u0027,\n \u0027test_update_subnet_gateway_in_allocation_pool_returns_409\u0027,\n \u0027test_update_subnet_allocation_pools\u0027,\n \u0027test_update_subnet_allocation_pools_invalid_pool_for_cidr\u0027)\n  if self._testMethodName in unsupported:\n      self.skipTest(\"Plugin does not support Neutron allocation process\")\n  super(TestPlumgridPluginSubnetsV2, self).setUp()","commit_id":"ff2550f157f0768f762918abf535b669f7a67ee6"},{"author":{"_account_id":8449,"name":"Marios Andreou","email":"marios.andreou@gmail.com","username":"marios"},"change_message_id":"1816b3ce24ac8f9483e8f1e0c726175fdc883939","unresolved":false,"context_lines":[{"line_number":79,"context_line":"        self.skipTest(\"Plugin does not support Neutron allocation process\")"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"class TestPlumgridPluginSubnetsV2(test_plugin.TestSubnetsV2,"},{"line_number":83,"context_line":"                                  PLUMgridPluginV2TestCase):"},{"line_number":84,"context_line":"    def test_create_subnet_default_gw_conflict_allocation_pool_returns_409("},{"line_number":85,"context_line":"            self):"}],"source_content_type":"text/x-python","patch_set":18,"id":"5adbc538_11c19c2e","line":82,"in_reply_to":"5adbc538_43319aa1","updated":"2014-05-24 07:47:24.000000000","message":"Nice","commit_id":"ff2550f157f0768f762918abf535b669f7a67ee6"}],"neutron/tests/unit/test_db_plugin.py":[{"author":{"_account_id":2592,"name":"Mark McClain","email":"mark@mcclain.xyz","username":"markmcclain"},"change_message_id":"a66fa372611c5b72f7e9b0ea163a62ddb1a45a82","unresolved":false,"context_lines":[{"line_number":3179,"context_line":"    #TODO(marios): how can i do this from here?i.e. query existing allocations"},{"line_number":3180,"context_line":"    #as there is no api request for that"},{"line_number":3181,"context_line":"    def test_update_subnet_allocation_pools_existing_allocation(self):"},{"line_number":3182,"context_line":"        pass"},{"line_number":3183,"context_line":""},{"line_number":3184,"context_line":"    #updating alloc pool to something outside subnet.cidr"},{"line_number":3185,"context_line":"    def test_update_subnet_allocation_pools_invalid_pool_for_cidr(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUH%2F97SI%3D","line":3182,"updated":"2014-02-01 00:19:46.000000000","message":"We should either define this test or drop it versus adding an empty test.","commit_id":"799b5348d6b1d86c68ca99de986cc0710da60049"},{"author":{"_account_id":6601,"name":"James Carey","email":"jecarey@yerac.com","username":"jecarey"},"change_message_id":"4f3df202d1176a36862f591bf150dfd71ec36971","unresolved":false,"context_lines":[{"line_number":3232,"context_line":"                self.assertEqual(res.status_int,"},{"line_number":3233,"context_line":"                                 webob.exc.HTTPConflict.code)"},{"line_number":3234,"context_line":""},{"line_number":3235,"context_line":"\u003c\u003c\u003c\u003c\u003c\u003c\u003c HEAD"},{"line_number":3236,"context_line":"    def test_update_subnet_ipv6_attributes(self):"},{"line_number":3237,"context_line":"        with self.subnet(ip_version\u003d6, cidr\u003d\u0027fe80::/80\u0027,"},{"line_number":3238,"context_line":"                         ipv6_ra_mode\u003dconstants.IPV6_SLAAC,"}],"source_content_type":"text/x-python","patch_set":14,"id":"AAAAWH%2F9Qe8%3D","line":3235,"updated":"2014-04-07 15:52:27.000000000","message":"Needs cleanup after conflict","commit_id":"019bcb9cdef21b01f730e99d81d8f5a879310d69"},{"author":{"_account_id":2592,"name":"Mark McClain","email":"mark@mcclain.xyz","username":"markmcclain"},"change_message_id":"df99dbf424ea78a00a80402708d852337ee108dc","unresolved":false,"context_lines":[{"line_number":3333,"context_line":"                self.assertEqual(res.status_int,"},{"line_number":3334,"context_line":"                                 webob.exc.HTTPClientError.code)"},{"line_number":3335,"context_line":""},{"line_number":3336,"context_line":"    #Test: can succesfully update alloc_pool with sane params"},{"line_number":3337,"context_line":"    def test_update_subnet_allocation_pools(self):"},{"line_number":3338,"context_line":"        allocation_pools \u003d [{\u0027start\u0027: \u0027192.168.0.2\u0027, \u0027end\u0027: \u0027192.168.0.254\u0027}]"},{"line_number":3339,"context_line":"        with self.network() as network:"}],"source_content_type":"text/x-python","patch_set":22,"id":"1ae5cdf2_2a5483c8","line":3336,"updated":"2014-06-05 17:48:54.000000000","message":"seems like this should be a proper docstring for the method","commit_id":"b96f7803ccbbe2d5fa827d52041b21b5abcfebe8"},{"author":{"_account_id":2592,"name":"Mark McClain","email":"mark@mcclain.xyz","username":"markmcclain"},"change_message_id":"df99dbf424ea78a00a80402708d852337ee108dc","unresolved":false,"context_lines":[{"line_number":3357,"context_line":"                for pool_val in [\u002710\u0027, \u002720\u0027, \u002730\u0027, \u002740\u0027]:"},{"line_number":3358,"context_line":"                    self.assertTrue(\u0027192.168.0.%s\u0027 % (pool_val) in res_vals)"},{"line_number":3359,"context_line":""},{"line_number":3360,"context_line":"    #updating alloc pool to something outside subnet.cidr"},{"line_number":3361,"context_line":"    def test_update_subnet_allocation_pools_invalid_pool_for_cidr(self):"},{"line_number":3362,"context_line":"        allocation_pools \u003d [{\u0027start\u0027: \u0027192.168.0.2\u0027, \u0027end\u0027: \u0027192.168.0.254\u0027}]"},{"line_number":3363,"context_line":"        with self.network() as network:"}],"source_content_type":"text/x-python","patch_set":22,"id":"1ae5cdf2_4a4f0f34","line":3360,"updated":"2014-06-05 17:48:54.000000000","message":"same as above","commit_id":"b96f7803ccbbe2d5fa827d52041b21b5abcfebe8"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"9cc28ba33511d252de6725b3dfc954c95b7a1277","unresolved":false,"context_lines":[{"line_number":3333,"context_line":"                self.assertEqual(res.status_int,"},{"line_number":3334,"context_line":"                                 webob.exc.HTTPClientError.code)"},{"line_number":3335,"context_line":""},{"line_number":3336,"context_line":"    #Test: can succesfully update alloc_pool with sane params"},{"line_number":3337,"context_line":"    def test_update_subnet_allocation_pools(self):"},{"line_number":3338,"context_line":"        allocation_pools \u003d [{\u0027start\u0027: \u0027192.168.0.2\u0027, \u0027end\u0027: \u0027192.168.0.254\u0027}]"},{"line_number":3339,"context_line":"        with self.network() as network:"}],"source_content_type":"text/x-python","patch_set":23,"id":"1ae5cdf2_26a62ac2","line":3336,"updated":"2014-06-09 15:35:45.000000000","message":"You forgot to address Mark\u0027s comment on patch 22 :)","commit_id":"19effb9fb3e9020b24d4e9f778bd98a66ee83261"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"136e9d669c774ef54dc8fe17e67eba285aa4c416","unresolved":false,"context_lines":[{"line_number":3333,"context_line":"                self.assertEqual(res.status_int,"},{"line_number":3334,"context_line":"                                 webob.exc.HTTPClientError.code)"},{"line_number":3335,"context_line":""},{"line_number":3336,"context_line":"    #Test: can succesfully update alloc_pool with sane params"},{"line_number":3337,"context_line":"    def test_update_subnet_allocation_pools(self):"},{"line_number":3338,"context_line":"        allocation_pools \u003d [{\u0027start\u0027: \u0027192.168.0.2\u0027, \u0027end\u0027: \u0027192.168.0.254\u0027}]"},{"line_number":3339,"context_line":"        with self.network() as network:"}],"source_content_type":"text/x-python","patch_set":23,"id":"1ae5cdf2_fa49328f","line":3336,"in_reply_to":"1ae5cdf2_26a62ac2","updated":"2014-06-09 16:22:52.000000000","message":"+1","commit_id":"19effb9fb3e9020b24d4e9f778bd98a66ee83261"}]}