)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"676ca322b958757ec528165d383b687a2cd2d2f8","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Optimize the number of database calls made during the DVR router"},{"line_number":10,"context_line":"updates to eliminate unnecessary duplication of calls which return"},{"line_number":11,"context_line":"the same data.  Also only process floating ips on a router that"},{"line_number":12,"context_line":"are relevant to the agent hosting the router (don\u0027t process floating"},{"line_number":13,"context_line":"ips assigned to a router if the associated vm is not hosted on the"},{"line_number":14,"context_line":"compute node requesting the router sync)"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1a930d6b_10f2bd6f","line":11,"updated":"2015-01-27 05:27:59.000000000","message":"Does this \"also\" indicate that this patch could be two independent patches?","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"487d9dc4fb48edf17a02e89f0419ec95c25e6f4a","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Optimize the number of database calls made during the DVR router"},{"line_number":10,"context_line":"updates to eliminate unnecessary duplication of calls which return"},{"line_number":11,"context_line":"the same data.  Also only process floating ips on a router that"},{"line_number":12,"context_line":"are relevant to the agent hosting the router (don\u0027t process floating"},{"line_number":13,"context_line":"ips assigned to a router if the associated vm is not hosted on the"},{"line_number":14,"context_line":"compute node requesting the router sync)"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1a930d6b_ba98d16a","line":11,"in_reply_to":"1a930d6b_10f2bd6f","updated":"2015-01-27 15:02:44.000000000","message":"Maybe I can word it a little better.  The database lookup optimizations are predicated on only processing floating ips on routers that are relevant to the agent.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"}],"neutron/db/l3_dvr_db.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"676ca322b958757ec528165d383b687a2cd2d2f8","unresolved":false,"context_lines":[{"line_number":367,"context_line":"        return routers_dict"},{"line_number":368,"context_line":""},{"line_number":369,"context_line":"    def _process_floating_ips(self, context, routers_dict,"},{"line_number":370,"context_line":"                              floating_ips, host\u003dNone, agent\u003dNone):"},{"line_number":371,"context_line":"        if not agent:"},{"line_number":372,"context_line":"            super(L3_NAT_with_dvr_db_mixin,"},{"line_number":373,"context_line":"                  self)._process_floating_ips(context,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_8dae9d2c","line":370,"updated":"2015-01-27 05:27:59.000000000","message":"I don\u0027t really like overloading this method with extra arguments.  I\u0027d rather use a different name to make it clear that the super-class\u0027s version cannot be called with these extra arguments.\n\n  def _process_floating_ips_dvr(...):","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"487d9dc4fb48edf17a02e89f0419ec95c25e6f4a","unresolved":false,"context_lines":[{"line_number":367,"context_line":"        return routers_dict"},{"line_number":368,"context_line":""},{"line_number":369,"context_line":"    def _process_floating_ips(self, context, routers_dict,"},{"line_number":370,"context_line":"                              floating_ips, host\u003dNone, agent\u003dNone):"},{"line_number":371,"context_line":"        if not agent:"},{"line_number":372,"context_line":"            super(L3_NAT_with_dvr_db_mixin,"},{"line_number":373,"context_line":"                  self)._process_floating_ips(context,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_ba25315d","line":370,"in_reply_to":"1a930d6b_8dae9d2c","updated":"2015-01-27 15:02:44.000000000","message":"Okay","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"676ca322b958757ec528165d383b687a2cd2d2f8","unresolved":false,"context_lines":[{"line_number":369,"context_line":"    def _process_floating_ips(self, context, routers_dict,"},{"line_number":370,"context_line":"                              floating_ips, host\u003dNone, agent\u003dNone):"},{"line_number":371,"context_line":"        if not agent:"},{"line_number":372,"context_line":"            super(L3_NAT_with_dvr_db_mixin,"},{"line_number":373,"context_line":"                  self)._process_floating_ips(context,"},{"line_number":374,"context_line":"                                              routers_dict,"},{"line_number":375,"context_line":"                                              floating_ips)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_0d80edbe","line":372,"updated":"2015-01-27 05:27:59.000000000","message":"should this \"return super(...)...?\"\n\nIt would look more normal to me if it did.  It would avoid indenting the code below (no \"else:\" needed) and the change scope here would appear smaller.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"487d9dc4fb48edf17a02e89f0419ec95c25e6f4a","unresolved":false,"context_lines":[{"line_number":369,"context_line":"    def _process_floating_ips(self, context, routers_dict,"},{"line_number":370,"context_line":"                              floating_ips, host\u003dNone, agent\u003dNone):"},{"line_number":371,"context_line":"        if not agent:"},{"line_number":372,"context_line":"            super(L3_NAT_with_dvr_db_mixin,"},{"line_number":373,"context_line":"                  self)._process_floating_ips(context,"},{"line_number":374,"context_line":"                                              routers_dict,"},{"line_number":375,"context_line":"                                              floating_ips)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_da283d35","line":372,"in_reply_to":"1a930d6b_0d80edbe","updated":"2015-01-27 15:02:44.000000000","message":"Good suggestion, I\u0027ll make it so.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"676ca322b958757ec528165d383b687a2cd2d2f8","unresolved":false,"context_lines":[{"line_number":374,"context_line":"                                              routers_dict,"},{"line_number":375,"context_line":"                                              floating_ips)"},{"line_number":376,"context_line":"        else:"},{"line_number":377,"context_line":"            floatingip_agent_intfs \u003d self.get_fip_sync_interfaces(context,"},{"line_number":378,"context_line":"                                                                  agent.id)"},{"line_number":379,"context_line":"            LOG.debug(\"FIP Agent ports: %s\", floatingip_agent_intfs)"},{"line_number":380,"context_line":"            for floating_ip in floating_ips:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_edcac119","line":377,"updated":"2015-01-27 05:27:59.000000000","message":"This looks odd.  I would expect this:\n\n  fip_sync_interfaces \u003d self.get_fip_sync_interfaces(...)\n\nI guess part of it is that \"intfs\" doesn\u0027t jump out at me as an abbreviation for interfaces.  Maybe \"fip_agent_interfaces?\"\n\nSince it was named this in the previous code, I guess you could leave it.  However, since all of the code is changing anyway, you could take the opportunity to rename it if you want.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"487d9dc4fb48edf17a02e89f0419ec95c25e6f4a","unresolved":false,"context_lines":[{"line_number":374,"context_line":"                                              routers_dict,"},{"line_number":375,"context_line":"                                              floating_ips)"},{"line_number":376,"context_line":"        else:"},{"line_number":377,"context_line":"            floatingip_agent_intfs \u003d self.get_fip_sync_interfaces(context,"},{"line_number":378,"context_line":"                                                                  agent.id)"},{"line_number":379,"context_line":"            LOG.debug(\"FIP Agent ports: %s\", floatingip_agent_intfs)"},{"line_number":380,"context_line":"            for floating_ip in floating_ips:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_fa225950","line":377,"in_reply_to":"1a930d6b_edcac119","updated":"2015-01-27 15:02:44.000000000","message":"I will rename it.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"676ca322b958757ec528165d383b687a2cd2d2f8","unresolved":false,"context_lines":[{"line_number":376,"context_line":"        else:"},{"line_number":377,"context_line":"            floatingip_agent_intfs \u003d self.get_fip_sync_interfaces(context,"},{"line_number":378,"context_line":"                                                                  agent.id)"},{"line_number":379,"context_line":"            LOG.debug(\"FIP Agent ports: %s\", floatingip_agent_intfs)"},{"line_number":380,"context_line":"            for floating_ip in floating_ips:"},{"line_number":381,"context_line":"                router \u003d routers_dict.get(floating_ip[\u0027router_id\u0027])"},{"line_number":382,"context_line":"                if router:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_30ced92e","line":379,"updated":"2015-01-27 05:27:59.000000000","message":"I still need to look through this diff more carefully.  I\u0027m hoping that my suggestion on L372 will make the whole diff much more readable by avoiding changing the indentation level.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6d8e9b47c8c93ce2591f39dd4a6b515fcaacc582","unresolved":false,"context_lines":[{"line_number":369,"context_line":"    def _process_floating_ips_dvr(self, context, routers_dict,"},{"line_number":370,"context_line":"                              floating_ips, host, agent):"},{"line_number":371,"context_line":"        fip_sync_interfaces \u003d self.get_fip_sync_interfaces(context,"},{"line_number":372,"context_line":"                                                           agent.id)"},{"line_number":373,"context_line":"        LOG.debug(\"FIP Agent ports: %s\", fip_sync_interfaces)"},{"line_number":374,"context_line":"        for floating_ip in floating_ips:"},{"line_number":375,"context_line":"            router \u003d routers_dict.get(floating_ip[\u0027router_id\u0027])"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a930d6b_59bff260","line":372,"updated":"2015-01-27 18:44:44.000000000","message":"nit:  The above should be able to fit on one line.","commit_id":"1ebf207a74f68cd87d9081c2a53ac9a104f79945"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6d8e9b47c8c93ce2591f39dd4a6b515fcaacc582","unresolved":false,"context_lines":[{"line_number":375,"context_line":"            router \u003d routers_dict.get(floating_ip[\u0027router_id\u0027])"},{"line_number":376,"context_line":"            if router:"},{"line_number":377,"context_line":"                router_floatingips \u003d router.get(l3_const.FLOATINGIP_KEY,"},{"line_number":378,"context_line":"                                                [])"},{"line_number":379,"context_line":"                if router[\u0027distributed\u0027]:"},{"line_number":380,"context_line":"                    if floating_ip[\u0027host\u0027] !\u003d host:"},{"line_number":381,"context_line":"                        continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a930d6b_b96ff6fa","line":378,"updated":"2015-01-27 18:44:44.000000000","message":"ditto","commit_id":"1ebf207a74f68cd87d9081c2a53ac9a104f79945"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6d8e9b47c8c93ce2591f39dd4a6b515fcaacc582","unresolved":false,"context_lines":[{"line_number":376,"context_line":"            if router:"},{"line_number":377,"context_line":"                router_floatingips \u003d router.get(l3_const.FLOATINGIP_KEY,"},{"line_number":378,"context_line":"                                                [])"},{"line_number":379,"context_line":"                if router[\u0027distributed\u0027]:"},{"line_number":380,"context_line":"                    if floating_ip[\u0027host\u0027] !\u003d host:"},{"line_number":381,"context_line":"                        continue"},{"line_number":382,"context_line":"                    LOG.debug(\"Floating IP host: %s\", floating_ip[\u0027host\u0027])"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a930d6b_594d3299","line":379,"updated":"2015-01-27 18:44:44.000000000","message":"aside:  weird that this is highlighted as a change.","commit_id":"1ebf207a74f68cd87d9081c2a53ac9a104f79945"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"08941ee50b5047b5f82ec565ba74d0c7949ad26b","unresolved":false,"context_lines":[{"line_number":309,"context_line":"        return router_interface_info"},{"line_number":310,"context_line":""},{"line_number":311,"context_line":"    def get_snat_sync_interfaces(self, context, router_ids):"},{"line_number":312,"context_line":"        \"\"\"Query router interfaces that relate to list of router_ids.\"\"\""},{"line_number":313,"context_line":"        if not router_ids:"},{"line_number":314,"context_line":"            return []"},{"line_number":315,"context_line":"        qry \u003d context.session.query(l3_db.RouterPort)"}],"source_content_type":"text/x-python","patch_set":7,"id":"da86d52c_692fb388","side":"PARENT","line":312,"updated":"2015-02-04 22:35:32.000000000","message":"This looks like you\u0027re mixing refactoring with other changes.  I\u0027d like to split them in to two patches so that I can more easily see what is refactoring and what is the optimization that you\u0027re after.","commit_id":"33ac5fa18068d43f21d94134e3ecd3afed5ccdb2"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"82a4fa33ded8e918063054ac32577868fc66e9ea","unresolved":false,"context_lines":[{"line_number":309,"context_line":"        return router_interface_info"},{"line_number":310,"context_line":""},{"line_number":311,"context_line":"    def get_snat_sync_interfaces(self, context, router_ids):"},{"line_number":312,"context_line":"        \"\"\"Query router interfaces that relate to list of router_ids.\"\"\""},{"line_number":313,"context_line":"        if not router_ids:"},{"line_number":314,"context_line":"            return []"},{"line_number":315,"context_line":"        qry \u003d context.session.query(l3_db.RouterPort)"}],"source_content_type":"text/x-python","patch_set":7,"id":"da86d52c_74d4d1bc","side":"PARENT","line":312,"in_reply_to":"da86d52c_692fb388","updated":"2015-02-05 15:08:58.000000000","message":"Yes, there has been a little bit of \u0027creep\u0027 introduced. I\u0027ll split it out.","commit_id":"33ac5fa18068d43f21d94134e3ecd3afed5ccdb2"},{"author":{"_account_id":1038,"name":"Jack McCann","username":"jack-mccann"},"change_message_id":"69c20e27d2aea07f9c1eabf822ebc1273bc9123b","unresolved":false,"context_lines":[{"line_number":320,"context_line":"        if not router_ids:"},{"line_number":321,"context_line":"            return []"},{"line_number":322,"context_line":"        filters \u003d {\u0027device_owner\u0027: [DEVICE_OWNER_DVR_SNAT],"},{"line_number":323,"context_line":"                   \u0027device_id\u0027: router_ids}"},{"line_number":324,"context_line":"        return self._core_plugin.get_ports(context, filters)"},{"line_number":325,"context_line":""},{"line_number":326,"context_line":"    def _build_routers_list(self, context, routers, gw_ports):"}],"source_content_type":"text/x-python","patch_set":7,"id":"da86d52c_26da4ae8","line":323,"updated":"2015-02-04 22:37:14.000000000","message":"are normal tenants still able to modify the device owner and device id fields of ports?  if so, what might happen here if somebody set a random port device owner and device id to match a dvr snat port?","commit_id":"cdc2d0182aa6d36670c4e04b0ab7ca61660ad931"},{"author":{"_account_id":1038,"name":"Jack McCann","username":"jack-mccann"},"change_message_id":"69c20e27d2aea07f9c1eabf822ebc1273bc9123b","unresolved":false,"context_lines":[{"line_number":400,"context_line":"            context, router_ids\u003drouter_ids, active\u003dactive,"},{"line_number":401,"context_line":"            device_owners\u003dl3_const.ROUTER_INTERFACE_OWNERS)"},{"line_number":402,"context_line":"        port_dict \u003d dict()"},{"line_number":403,"context_line":"        ports \u003d self._core_plugin.get_ports(context)"},{"line_number":404,"context_line":"        for port in ports:"},{"line_number":405,"context_line":"            port_dict.update({port[\u0027id\u0027]: port})"},{"line_number":406,"context_line":"        # Add the port binding host to the floatingip dictionary"}],"source_content_type":"text/x-python","patch_set":7,"id":"da86d52c_291f7b09","line":403,"updated":"2015-02-04 22:37:14.000000000","message":"shouldn\u0027t there be some sort of filter on this get_ports call?","commit_id":"cdc2d0182aa6d36670c4e04b0ab7ca61660ad931"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"82a4fa33ded8e918063054ac32577868fc66e9ea","unresolved":false,"context_lines":[{"line_number":400,"context_line":"            context, router_ids\u003drouter_ids, active\u003dactive,"},{"line_number":401,"context_line":"            device_owners\u003dl3_const.ROUTER_INTERFACE_OWNERS)"},{"line_number":402,"context_line":"        port_dict \u003d dict()"},{"line_number":403,"context_line":"        ports \u003d self._core_plugin.get_ports(context)"},{"line_number":404,"context_line":"        for port in ports:"},{"line_number":405,"context_line":"            port_dict.update({port[\u0027id\u0027]: port})"},{"line_number":406,"context_line":"        # Add the port binding host to the floatingip dictionary"}],"source_content_type":"text/x-python","patch_set":7,"id":"da86d52c_546d0d03","line":403,"in_reply_to":"da86d52c_291f7b09","updated":"2015-02-05 15:08:58.000000000","message":"I agree, some sort of filter should be applied, but I want to also make sure that we get all possible ports \u0027of interest\u0027 with one call instead of the previous approach which was to make a \u0027get_port\u0027 lookup for each floating ip port_id.","commit_id":"cdc2d0182aa6d36670c4e04b0ab7ca61660ad931"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"52c7f25725d7121d6323078905fca78aebccd84b","unresolved":false,"context_lines":[{"line_number":366,"context_line":"                router[SNAT_ROUTER_INTF_KEY] \u003d snat_router_intfs"},{"line_number":367,"context_line":"        return routers_dict"},{"line_number":368,"context_line":""},{"line_number":369,"context_line":"    def _process_floating_ips(self, context, routers_dict, floating_ips):"},{"line_number":370,"context_line":"        for floating_ip in floating_ips:"},{"line_number":371,"context_line":"            router \u003d routers_dict.get(floating_ip[\u0027router_id\u0027])"},{"line_number":372,"context_line":"            if router:"}],"source_content_type":"text/x-python","patch_set":9,"id":"da86d52c_60f6b85e","side":"PARENT","line":369,"updated":"2015-02-05 22:55:27.000000000","message":"I\u0027m not sure what the consequences of renaming this method might be.  The reason I say this is that _process_floating_ips may still be called.  Before this patch, a call to _process_floating_ips would get this version by virtue of polymorphism.  Now, since the name has changed, the base class version will be called which is similar but not the same.\n\nI\u0027m trying to decide if this matters or not.  I wonder if we should leave the originals just to be safe (potentially refactoring them to avoid the code duplication from the base class.","commit_id":"1e36581c2e00abb26873df137ad764121304f12f"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"52c7f25725d7121d6323078905fca78aebccd84b","unresolved":false,"context_lines":[{"line_number":370,"context_line":"                                  floating_ips, host, agent):"},{"line_number":371,"context_line":"        fip_sync_interfaces \u003d self.get_fip_sync_interfaces(context, agent.id)"},{"line_number":372,"context_line":"        LOG.debug(\"FIP Agent ports: %s\", fip_sync_interfaces)"},{"line_number":373,"context_line":"        for floating_ip in floating_ips:"},{"line_number":374,"context_line":"            router \u003d routers_dict.get(floating_ip[\u0027router_id\u0027])"},{"line_number":375,"context_line":"            if router:"},{"line_number":376,"context_line":"                router_floatingips \u003d router.get(l3_const.FLOATINGIP_KEY, [])"}],"source_content_type":"text/x-python","patch_set":9,"id":"da86d52c_bdf1bc6c","line":373,"updated":"2015-02-05 22:55:27.000000000","message":"aside:  It saddens me that the base class\u0027s version of this method was copied to form the basis for this method instead of making a call to super.  I know this comment has nothing to do with this patch.  Just sayin\u0027.  It would have been better to refactor in a way that properly separated concerns.","commit_id":"9b2d09a25acf58417aa23387204132e3b72b660d"},{"author":{"_account_id":9380,"name":"Sayaji Patil","email":"sayaji15@gmail.com","username":"Sayaji"},"change_message_id":"f34b856f2bbf808c223ee744bdb605d8fc44a844","unresolved":false,"context_lines":[{"line_number":378,"context_line":"                    if floating_ip[\u0027host\u0027] !\u003d host:"},{"line_number":379,"context_line":"                        continue"},{"line_number":380,"context_line":"                    LOG.debug(\"Floating IP host: %s\", floating_ip[\u0027host\u0027])"},{"line_number":381,"context_line":"                    LOG.debug(\"FIP Agent : %s \", agent.id)"},{"line_number":382,"context_line":"                router_floatingips.append(floating_ip)"},{"line_number":383,"context_line":"                router[l3_const.FLOATINGIP_KEY] \u003d router_floatingips"},{"line_number":384,"context_line":"                router[l3_const.FLOATINGIP_AGENT_INTF_KEY] \u003d ("}],"source_content_type":"text/x-python","patch_set":9,"id":"da86d52c_6e007034","line":381,"updated":"2015-02-05 19:41:47.000000000","message":"Nit: Both debugs can be combined into one","commit_id":"9b2d09a25acf58417aa23387204132e3b72b660d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"52c7f25725d7121d6323078905fca78aebccd84b","unresolved":false,"context_lines":[{"line_number":404,"context_line":"            context, router_ids\u003drouter_ids, active\u003dactive,"},{"line_number":405,"context_line":"            device_owners\u003dl3_const.ROUTER_INTERFACE_OWNERS)"},{"line_number":406,"context_line":"        port_dict \u003d dict()"},{"line_number":407,"context_line":"        ports \u003d self._core_plugin.get_ports(context)"},{"line_number":408,"context_line":"        for port in ports:"},{"line_number":409,"context_line":"            port_dict.update({port[\u0027id\u0027]: port})"},{"line_number":410,"context_line":"        # Add the port binding host to the floatingip dictionary"}],"source_content_type":"text/x-python","patch_set":9,"id":"da86d52c_9d64f7b9","line":407,"updated":"2015-02-05 22:55:27.000000000","message":"I agree with Jack\u0027s comment that we need to apply a filter here.  What do we know about the context at this point?  How about building a list of port_ids like this:\n\n port_ids \u003d set(fip[\u0027port_id\u0027] for fip in floating_ips)\n\nNow, we know the comprehensive list of port_ids that we\u0027ll encounter on L412.  Could we use this list as part of the filter?\n\nWe also know the host.  Could we exclude ports which do not match the host since ultimately we\u0027ll be skipping them anyway in _process_floating_ips_dvr?","commit_id":"9b2d09a25acf58417aa23387204132e3b72b660d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"52c7f25725d7121d6323078905fca78aebccd84b","unresolved":false,"context_lines":[{"line_number":405,"context_line":"            device_owners\u003dl3_const.ROUTER_INTERFACE_OWNERS)"},{"line_number":406,"context_line":"        port_dict \u003d dict()"},{"line_number":407,"context_line":"        ports \u003d self._core_plugin.get_ports(context)"},{"line_number":408,"context_line":"        for port in ports:"},{"line_number":409,"context_line":"            port_dict.update({port[\u0027id\u0027]: port})"},{"line_number":410,"context_line":"        # Add the port binding host to the floatingip dictionary"},{"line_number":411,"context_line":"        for fip in floating_ips:"}],"source_content_type":"text/x-python","patch_set":9,"id":"da86d52c_fdd09320","line":408,"updated":"2015-02-05 22:55:27.000000000","message":"nit:  You could use a \"dict comprehension\" here.\n\nport_dict \u003d dict((port[\u0027id\u0027], port) for port in ports)","commit_id":"9b2d09a25acf58417aa23387204132e3b72b660d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"52c7f25725d7121d6323078905fca78aebccd84b","unresolved":false,"context_lines":[{"line_number":410,"context_line":"        # Add the port binding host to the floatingip dictionary"},{"line_number":411,"context_line":"        for fip in floating_ips:"},{"line_number":412,"context_line":"            vm_port \u003d port_dict[fip[\u0027port_id\u0027]]"},{"line_number":413,"context_line":"            fip[\u0027host\u0027] \u003d self.get_vm_port_hostid(context, fip[\u0027port_id\u0027],"},{"line_number":414,"context_line":"                                                  port\u003dvm_port)"},{"line_number":415,"context_line":"        routers_dict \u003d self._process_routers(context, routers)"},{"line_number":416,"context_line":"        self._process_floating_ips_dvr(context, routers_dict,"}],"source_content_type":"text/x-python","patch_set":9,"id":"da86d52c_6045d841","line":413,"updated":"2015-02-05 22:55:27.000000000","message":"Are all of these ports going to pass \"is_dvr_serviced\" in this method call?  I suspect they will.\n\nIf so, could we just skip straight to the statement below to avoid awkward code reuse (see L424)?\n\n    fip[\u0027host\u0027] \u003d vm_port[portbindings.HOST_ID]","commit_id":"9b2d09a25acf58417aa23387204132e3b72b660d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"52c7f25725d7121d6323078905fca78aebccd84b","unresolved":false,"context_lines":[{"line_number":421,"context_line":"    def get_vm_port_hostid(self, context, port_id, port\u003dNone):"},{"line_number":422,"context_line":"        \"\"\"Return the portbinding host_id.\"\"\""},{"line_number":423,"context_line":"        vm_port_db \u003d port or self._core_plugin.get_port(context, port_id)"},{"line_number":424,"context_line":"        device_owner \u003d vm_port_db[\u0027device_owner\u0027] if vm_port_db else \"\""},{"line_number":425,"context_line":"        if (n_utils.is_dvr_serviced(device_owner) or"},{"line_number":426,"context_line":"            device_owner \u003d\u003d DEVICE_OWNER_AGENT_GW):"},{"line_number":427,"context_line":"            return vm_port_db[portbindings.HOST_ID]"}],"source_content_type":"text/x-python","patch_set":9,"id":"da86d52c_dda10f47","line":424,"updated":"2015-02-05 22:55:27.000000000","message":"aside:  Maybe in L, I\u0027ll tackle refactoring some of this (I\u0027m kidding, where would I find the time).  I find optional arguments that change the path through the code like port\u003dNone here very confusing.  I would have written two methods:\n\n    def get_vm_port_hostid(self, context, port_id):\n        vm_port_db \u003d self._core_plugin.get_port(context, port_id)\n        return self.get_vm_port_hostid_from_port(vm_port_id)\n\n    def get_vm_port_hostid_from_port(self, port_db):\n        device_owner \u003d port_db[\u0027device_owner\u0027] if port_db else \"\"\n        if (n_utils.is_dvr_serviced(device_owner) or\n            device_owner \u003d\u003d DEVICE_OWNER_AGENT_GW):\n            return vm_port_db[portbindings.HOST_ID]\n\nIsn\u0027t that just more straight-forward?  L413 above would call the second method directly for a more straight-forward path through the code.\n\nBonus points for creating a patch to refactor this.","commit_id":"9b2d09a25acf58417aa23387204132e3b72b660d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"98f2b7d04b9ce660671adfcf561230db8a3ca609","unresolved":false,"context_lines":[{"line_number":388,"context_line":""},{"line_number":389,"context_line":"    def _process_floating_ips_dvr(self, context, routers_dict,"},{"line_number":390,"context_line":"                                  floating_ips, host, agent):"},{"line_number":391,"context_line":"        fip_sync_interfaces \u003d self.get_fip_sync_interfaces(context, agent.id)"},{"line_number":392,"context_line":"        LOG.debug(\"FIP Agent ports: %s\", fip_sync_interfaces)"},{"line_number":393,"context_line":"        for floating_ip in floating_ips:"},{"line_number":394,"context_line":"            router \u003d routers_dict.get(floating_ip[\u0027router_id\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"ba7be1f8_ae9ec352","line":391,"updated":"2015-02-26 20:06:46.000000000","message":"nit:  Consider making this call lazy.  It looks like the value here may never be used if L405 is never reached.  Is that possible?","commit_id":"3b712afc0cda495c0ce843e3b9fe44263af3f31e"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"6785859ed8ca1a41a66eaed38d3e7cc592f7edad","unresolved":false,"context_lines":[{"line_number":388,"context_line":""},{"line_number":389,"context_line":"    def _process_floating_ips_dvr(self, context, routers_dict,"},{"line_number":390,"context_line":"                                  floating_ips, host, agent):"},{"line_number":391,"context_line":"        fip_sync_interfaces \u003d self.get_fip_sync_interfaces(context, agent.id)"},{"line_number":392,"context_line":"        LOG.debug(\"FIP Agent ports: %s\", fip_sync_interfaces)"},{"line_number":393,"context_line":"        for floating_ip in floating_ips:"},{"line_number":394,"context_line":"            router \u003d routers_dict.get(floating_ip[\u0027router_id\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"ba7be1f8_bcaa79d0","line":391,"in_reply_to":"ba7be1f8_ae9ec352","updated":"2015-02-26 20:50:14.000000000","message":"It is possible that L405 is never reached, if floating_ip[\u0027host\u0027] does not equal host.  I will see about making this call lazy.","commit_id":"3b712afc0cda495c0ce843e3b9fe44263af3f31e"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"98f2b7d04b9ce660671adfcf561230db8a3ca609","unresolved":false,"context_lines":[{"line_number":419,"context_line":"    def get_dvr_sync_data(self, context, host, agent, router_ids\u003dNone,"},{"line_number":420,"context_line":"                          active\u003dNone):"},{"line_number":421,"context_line":"        requesting_agent \u003d agent or self._get_agent_by_type_and_host("},{"line_number":422,"context_line":"            context, l3_const.AGENT_TYPE_L3, host)"},{"line_number":423,"context_line":"        routers, interfaces, floating_ips \u003d self._get_router_info_list("},{"line_number":424,"context_line":"            context, router_ids\u003drouter_ids, active\u003dactive,"},{"line_number":425,"context_line":"            device_owners\u003dl3_const.ROUTER_INTERFACE_OWNERS)"}],"source_content_type":"text/x-python","patch_set":12,"id":"ba7be1f8_8edbbf7c","line":422,"updated":"2015-02-26 20:06:46.000000000","message":"nit:  I\u0027d prefer to require that the caller provide the agent rather than make it optional and fill it in here.  I think there is only place where None is passed for agent.  Could you refactor so that the caller that now passes None here gets the agent using this call first?","commit_id":"3b712afc0cda495c0ce843e3b9fe44263af3f31e"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"6785859ed8ca1a41a66eaed38d3e7cc592f7edad","unresolved":false,"context_lines":[{"line_number":419,"context_line":"    def get_dvr_sync_data(self, context, host, agent, router_ids\u003dNone,"},{"line_number":420,"context_line":"                          active\u003dNone):"},{"line_number":421,"context_line":"        requesting_agent \u003d agent or self._get_agent_by_type_and_host("},{"line_number":422,"context_line":"            context, l3_const.AGENT_TYPE_L3, host)"},{"line_number":423,"context_line":"        routers, interfaces, floating_ips \u003d self._get_router_info_list("},{"line_number":424,"context_line":"            context, router_ids\u003drouter_ids, active\u003dactive,"},{"line_number":425,"context_line":"            device_owners\u003dl3_const.ROUTER_INTERFACE_OWNERS)"}],"source_content_type":"text/x-python","patch_set":12,"id":"ba7be1f8_dc600dd2","line":422,"in_reply_to":"ba7be1f8_8edbbf7c","updated":"2015-02-26 20:50:14.000000000","message":"I will look to refactor.","commit_id":"3b712afc0cda495c0ce843e3b9fe44263af3f31e"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"98f2b7d04b9ce660671adfcf561230db8a3ca609","unresolved":false,"context_lines":[{"line_number":423,"context_line":"        routers, interfaces, floating_ips \u003d self._get_router_info_list("},{"line_number":424,"context_line":"            context, router_ids\u003drouter_ids, active\u003dactive,"},{"line_number":425,"context_line":"            device_owners\u003dl3_const.ROUTER_INTERFACE_OWNERS)"},{"line_number":426,"context_line":"        port_filter \u003d {portbindings.HOST_ID: [host]}"},{"line_number":427,"context_line":"        ports \u003d self._core_plugin.get_ports(context, port_filter)"},{"line_number":428,"context_line":"        port_dict \u003d dict((port[\u0027id\u0027], port) for port in ports)"},{"line_number":429,"context_line":"        # Add the port binding host to the floatingip dictionary"}],"source_content_type":"text/x-python","patch_set":12,"id":"ba7be1f8_aec5e33e","line":426,"updated":"2015-02-26 20:06:46.000000000","message":"Yay!","commit_id":"3b712afc0cda495c0ce843e3b9fe44263af3f31e"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"98f2b7d04b9ce660671adfcf561230db8a3ca609","unresolved":false,"context_lines":[{"line_number":433,"context_line":"                fip[\u0027host\u0027] \u003d self.get_vm_port_hostid(context, fip[\u0027port_id\u0027],"},{"line_number":434,"context_line":"                                                      port\u003dvm_port)"},{"line_number":435,"context_line":"            else:"},{"line_number":436,"context_line":"                fip[\u0027host\u0027] \u003d \" \""},{"line_number":437,"context_line":"        routers_dict \u003d self._process_routers(context, routers)"},{"line_number":438,"context_line":"        self._process_floating_ips_dvr(context, routers_dict,"},{"line_number":439,"context_line":"                                       floating_ips, host, requesting_agent)"}],"source_content_type":"text/x-python","patch_set":12,"id":"ba7be1f8_cbe3e9a2","line":436,"updated":"2015-02-26 20:06:46.000000000","message":"I can\u0027t imagine what it means to assign \" \" (a single space, right?) as the fip host here.  I smell a hack and it isn\u0027t clear to me from this context what is going on here.  Ping me to chat.  Maybe you can catch me up on why this is necessary.","commit_id":"3b712afc0cda495c0ce843e3b9fe44263af3f31e"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"6785859ed8ca1a41a66eaed38d3e7cc592f7edad","unresolved":false,"context_lines":[{"line_number":433,"context_line":"                fip[\u0027host\u0027] \u003d self.get_vm_port_hostid(context, fip[\u0027port_id\u0027],"},{"line_number":434,"context_line":"                                                      port\u003dvm_port)"},{"line_number":435,"context_line":"            else:"},{"line_number":436,"context_line":"                fip[\u0027host\u0027] \u003d \" \""},{"line_number":437,"context_line":"        routers_dict \u003d self._process_routers(context, routers)"},{"line_number":438,"context_line":"        self._process_floating_ips_dvr(context, routers_dict,"},{"line_number":439,"context_line":"                                       floating_ips, host, requesting_agent)"}],"source_content_type":"text/x-python","patch_set":12,"id":"ba7be1f8_1c7385b5","line":436,"in_reply_to":"ba7be1f8_cbe3e9a2","updated":"2015-02-26 20:50:14.000000000","message":"Upon reflection, this is entirely unnecessary since fip[\u0027host\u0027] being unset still will pass the check in L398 which prevents processing of any fip which is not associated with a port on host.  I will remove this.","commit_id":"3b712afc0cda495c0ce843e3b9fe44263af3f31e"},{"author":{"_account_id":8976,"name":"shihanzhang","email":"ayshihanzhang@gmail.com","username":"shihanzhang"},"change_message_id":"2b63f69a19f624c9e49ab778c89149aac3fd6b3a","unresolved":false,"context_lines":[{"line_number":398,"context_line":"                    if floating_ip.get(\u0027host\u0027, None) !\u003d host:"},{"line_number":399,"context_line":"                        continue"},{"line_number":400,"context_line":"                    LOG.debug(\"Floating IP host: %s\", floating_ip[\u0027host\u0027])"},{"line_number":401,"context_line":"                    LOG.debug(\"FIP Agent : %s \", agent.id)"},{"line_number":402,"context_line":"                router_floatingips.append(floating_ip)"},{"line_number":403,"context_line":"                router[l3_const.FLOATINGIP_KEY] \u003d router_floatingips"},{"line_number":404,"context_line":"                if not fip_sync_interfaces:"}],"source_content_type":"text/x-python","patch_set":13,"id":"ba7be1f8_17207baf","line":401,"updated":"2015-02-28 08:04:51.000000000","message":"why divided into two sentences?","commit_id":"3ea2d78c90d9583db13d816ed773a99dac528977"},{"author":{"_account_id":13667,"name":"ZongKai LI","email":"onionpiece@163.com","username":"lzklibj"},"change_message_id":"6b06181ee911fdbd6b8fe911581cea1dda90e170","unresolved":false,"context_lines":[{"line_number":428,"context_line":"                    LOG.debug(\"Floating IP host: %s\", floating_ip[\u0027host\u0027])"},{"line_number":429,"context_line":"                router_floatingips.append(floating_ip)"},{"line_number":430,"context_line":"                router[l3_const.FLOATINGIP_KEY] \u003d router_floatingips"},{"line_number":431,"context_line":"                if not fip_sync_interfaces:"},{"line_number":432,"context_line":"                    fip_sync_interfaces \u003d self.get_fip_sync_interfaces("},{"line_number":433,"context_line":"                        context, agent.id)"},{"line_number":434,"context_line":"                    LOG.debug(\"FIP Agent ports: %s\", fip_sync_interfaces)"}],"source_content_type":"text/x-python","patch_set":15,"id":"1a6ced46_d76b8c83","line":431,"updated":"2015-03-22 07:46:53.000000000","message":"Seems this is based on Carl\u0027s comments in patch set 12. Why not directly L432~L434 without L419 and L431.","commit_id":"b96a22661290ce2ea747537512eab2fb767679e6"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"dfa0c4c5001b543124a19a0c992ecdfdb907c497","unresolved":false,"context_lines":[{"line_number":428,"context_line":"                    LOG.debug(\"Floating IP host: %s\", floating_ip[\u0027host\u0027])"},{"line_number":429,"context_line":"                router_floatingips.append(floating_ip)"},{"line_number":430,"context_line":"                router[l3_const.FLOATINGIP_KEY] \u003d router_floatingips"},{"line_number":431,"context_line":"                if not fip_sync_interfaces:"},{"line_number":432,"context_line":"                    fip_sync_interfaces \u003d self.get_fip_sync_interfaces("},{"line_number":433,"context_line":"                        context, agent.id)"},{"line_number":434,"context_line":"                    LOG.debug(\"FIP Agent ports: %s\", fip_sync_interfaces)"}],"source_content_type":"text/x-python","patch_set":15,"id":"da9b358b_41f2129c","line":431,"in_reply_to":"1a6ced46_d76b8c83","updated":"2015-03-27 22:59:36.000000000","message":"Your suggestion would call get_fip_sync_interfaces once each time this section of code is reached in the loop.  It is not an efficient method call and so we want to avoid running it more than once.  I believe that what Erik has here fits the bill.","commit_id":"b96a22661290ce2ea747537512eab2fb767679e6"},{"author":{"_account_id":13667,"name":"ZongKai LI","email":"onionpiece@163.com","username":"lzklibj"},"change_message_id":"f3bbc054d455d2759a778f556cd529420bf9e015","unresolved":false,"context_lines":[{"line_number":428,"context_line":"                    LOG.debug(\"Floating IP host: %s\", floating_ip[\u0027host\u0027])"},{"line_number":429,"context_line":"                router_floatingips.append(floating_ip)"},{"line_number":430,"context_line":"                router[l3_const.FLOATINGIP_KEY] \u003d router_floatingips"},{"line_number":431,"context_line":"                if not fip_sync_interfaces:"},{"line_number":432,"context_line":"                    fip_sync_interfaces \u003d self.get_fip_sync_interfaces("},{"line_number":433,"context_line":"                        context, agent.id)"},{"line_number":434,"context_line":"                    LOG.debug(\"FIP Agent ports: %s\", fip_sync_interfaces)"}],"source_content_type":"text/x-python","patch_set":15,"id":"da9b358b_47fc623e","line":431,"in_reply_to":"da9b358b_41f2129c","updated":"2015-03-28 00:47:50.000000000","message":"Yeah, thanks, I get it now.","commit_id":"b96a22661290ce2ea747537512eab2fb767679e6"}],"neutron/db/l3_dvrscheduler_db.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"676ca322b958757ec528165d383b687a2cd2d2f8","unresolved":false,"context_lines":[{"line_number":326,"context_line":"                l3agent_sch_db.RouterL3AgentBinding.router_id.in_(router_ids))"},{"line_number":327,"context_line":"        router_ids \u003d [item[0] for item in query]"},{"line_number":328,"context_line":"        if router_ids:"},{"line_number":329,"context_line":"            if n_utils.is_extension_supported(self,"},{"line_number":330,"context_line":"                                              q_const.L3_HA_MODE_EXT_ALIAS):"},{"line_number":331,"context_line":"                return self.get_ha_sync_data_for_host(context, host,"},{"line_number":332,"context_line":"                                                      router_ids\u003drouter_ids,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_f0dff1d3","line":329,"updated":"2015-01-27 05:27:59.000000000","message":"I think I\u0027d like the DVR guys to take a look.  I\u0027ll add them as reviewers.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"676ca322b958757ec528165d383b687a2cd2d2f8","unresolved":false,"context_lines":[{"line_number":331,"context_line":"                return self.get_ha_sync_data_for_host(context, host,"},{"line_number":332,"context_line":"                                                      router_ids\u003drouter_ids,"},{"line_number":333,"context_line":"                                                      active\u003dTrue)"},{"line_number":334,"context_line":"            else:"},{"line_number":335,"context_line":"                return self.get_dvr_sync_data(context, host, agent,"},{"line_number":336,"context_line":"                                              router_ids\u003drouter_ids,"},{"line_number":337,"context_line":"                                              active\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_f070d141","line":334,"updated":"2015-01-27 05:27:59.000000000","message":"nit:  else is not necessary here or below.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"487d9dc4fb48edf17a02e89f0419ec95c25e6f4a","unresolved":false,"context_lines":[{"line_number":331,"context_line":"                return self.get_ha_sync_data_for_host(context, host,"},{"line_number":332,"context_line":"                                                      router_ids\u003drouter_ids,"},{"line_number":333,"context_line":"                                                      active\u003dTrue)"},{"line_number":334,"context_line":"            else:"},{"line_number":335,"context_line":"                return self.get_dvr_sync_data(context, host, agent,"},{"line_number":336,"context_line":"                                              router_ids\u003drouter_ids,"},{"line_number":337,"context_line":"                                              active\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_5ab8cd10","line":334,"in_reply_to":"1a930d6b_f070d141","updated":"2015-01-27 15:02:44.000000000","message":"I\u0027ll remove.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6d8e9b47c8c93ce2591f39dd4a6b515fcaacc582","unresolved":false,"context_lines":[{"line_number":309,"context_line":"            self.bind_dvr_router_servicenode("},{"line_number":310,"context_line":"                context, router_id, chosen_agent)"},{"line_number":311,"context_line":""},{"line_number":312,"context_line":"    def list_active_sync_routers_on_active_l3_agent("},{"line_number":313,"context_line":"             self, context, host, router_ids):"},{"line_number":314,"context_line":"        agent \u003d self._get_agent_by_type_and_host(context,"},{"line_number":315,"context_line":"                                                 q_const.AGENT_TYPE_L3,"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa81d914_aae30f04","line":312,"updated":"2015-01-27 18:44:44.000000000","message":"I should have picked up on the fact that this is almost identical to the agentschedulers version last night.  That\u0027s what I get for reviewing in to the night.  :(","commit_id":"1ebf207a74f68cd87d9081c2a53ac9a104f79945"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6d8e9b47c8c93ce2591f39dd4a6b515fcaacc582","unresolved":false,"context_lines":[{"line_number":331,"context_line":"                return self.get_ha_sync_data_for_host(context, host,"},{"line_number":332,"context_line":"                                                      router_ids\u003drouter_ids,"},{"line_number":333,"context_line":"                                                      active\u003dTrue)"},{"line_number":334,"context_line":"            return self.get_dvr_sync_data(context, host, agent,"},{"line_number":335,"context_line":"                                          router_ids\u003drouter_ids,"},{"line_number":336,"context_line":"                                          active\u003dTrue)"},{"line_number":337,"context_line":"        LOG.debug(\u0027returning empty router_ids list\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa81d914_0a95fbe3","line":334,"updated":"2015-01-27 18:44:44.000000000","message":"This line here is the only significant difference from the version of this method in the base class.  It calls \"get_dvr_sync_data\" instead of \"get_sync_data\" and it passes two extra arguments:  host and agent.\n\nThis could easily get in to a maintenance problem.  I think some refactoring is in order.  It would be easiest to review if you posted a pure refactoring patch and then made this patch depend on it to change the behavior.","commit_id":"1ebf207a74f68cd87d9081c2a53ac9a104f79945"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6d8e9b47c8c93ce2591f39dd4a6b515fcaacc582","unresolved":false,"context_lines":[{"line_number":334,"context_line":"            return self.get_dvr_sync_data(context, host, agent,"},{"line_number":335,"context_line":"                                          router_ids\u003drouter_ids,"},{"line_number":336,"context_line":"                                          active\u003dTrue)"},{"line_number":337,"context_line":"        LOG.debug(\u0027returning empty router_ids list\u0027)"},{"line_number":338,"context_line":"        return []"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa81d914_aaacef36","line":337,"updated":"2015-01-27 18:44:44.000000000","message":"This debug statement is also added from the base class but I wouldn\u0027t call that significant.","commit_id":"1ebf207a74f68cd87d9081c2a53ac9a104f79945"},{"author":{"_account_id":13667,"name":"ZongKai LI","email":"onionpiece@163.com","username":"lzklibj"},"change_message_id":"6b06181ee911fdbd6b8fe911581cea1dda90e170","unresolved":false,"context_lines":[{"line_number":317,"context_line":""},{"line_number":318,"context_line":"    def _get_active_l3_agent_routers_sync_data(self, context, host, agent,"},{"line_number":319,"context_line":"                                               router_ids):"},{"line_number":320,"context_line":"        if n_utils.is_extension_supported(self, q_const.L3_HA_MODE_EXT_ALIAS):"},{"line_number":321,"context_line":"            return self.get_ha_sync_data_for_host(context, host,"},{"line_number":322,"context_line":"                                                  router_ids\u003drouter_ids,"},{"line_number":323,"context_line":"                                                  active\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":15,"id":"1a6ced46_d702acb3","line":320,"updated":"2015-03-22 07:46:53.000000000","message":"Please ignore this comments if you feel it doesn\u0027t make any sense.\n\nSeems dvr+ha router is not supported yet, so if it\u0027s trying to fetch sync data for dvr router, why not directly call get_dvr_sync_data?","commit_id":"b96a22661290ce2ea747537512eab2fb767679e6"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"dfa0c4c5001b543124a19a0c992ecdfdb907c497","unresolved":false,"context_lines":[{"line_number":317,"context_line":""},{"line_number":318,"context_line":"    def _get_active_l3_agent_routers_sync_data(self, context, host, agent,"},{"line_number":319,"context_line":"                                               router_ids):"},{"line_number":320,"context_line":"        if n_utils.is_extension_supported(self, q_const.L3_HA_MODE_EXT_ALIAS):"},{"line_number":321,"context_line":"            return self.get_ha_sync_data_for_host(context, host,"},{"line_number":322,"context_line":"                                                  router_ids\u003drouter_ids,"},{"line_number":323,"context_line":"                                                  active\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":15,"id":"da9b358b_81d13afe","line":320,"in_reply_to":"1a6ced46_d702acb3","updated":"2015-03-27 22:59:36.000000000","message":"It may be subtle but there is a difference between having a router that is dvr+ha (which we don\u0027t support yet) and having dvr and ha extensions loaded simultaneous (which is supported).  There could be dvr routers and ha routers in the same deployment.","commit_id":"b96a22661290ce2ea747537512eab2fb767679e6"},{"author":{"_account_id":13667,"name":"ZongKai LI","email":"onionpiece@163.com","username":"lzklibj"},"change_message_id":"9747f1952df5702c2157fda6b0cc1b688855cbb6","unresolved":false,"context_lines":[{"line_number":317,"context_line":""},{"line_number":318,"context_line":"    def _get_active_l3_agent_routers_sync_data(self, context, host, agent,"},{"line_number":319,"context_line":"                                               router_ids):"},{"line_number":320,"context_line":"        if n_utils.is_extension_supported(self, q_const.L3_HA_MODE_EXT_ALIAS):"},{"line_number":321,"context_line":"            return self.get_ha_sync_data_for_host(context, host,"},{"line_number":322,"context_line":"                                                  router_ids\u003drouter_ids,"},{"line_number":323,"context_line":"                                                  active\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":15,"id":"da9b358b_8ae4aa0d","line":320,"in_reply_to":"da9b358b_81d13afe","updated":"2015-03-28 08:24:47.000000000","message":"Thanks, I get it now.","commit_id":"b96a22661290ce2ea747537512eab2fb767679e6"}],"neutron/db/l3_hamode_db.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6d8e9b47c8c93ce2591f39dd4a6b515fcaacc582","unresolved":false,"context_lines":[{"line_number":463,"context_line":"    def get_ha_sync_data_for_host(self, context, host\u003dNone, router_ids\u003dNone,"},{"line_number":464,"context_line":"                                  active\u003dNone):"},{"line_number":465,"context_line":"        if n_utils.is_extension_supported(self,"},{"line_number":466,"context_line":"                                          constants.L3_DISTRIBUTED_EXT_ALIAS):"},{"line_number":467,"context_line":"            # DVR has to be handled differently"},{"line_number":468,"context_line":"            sync_data \u003d self.get_dvr_sync_data(context, host, None, router_ids,"},{"line_number":469,"context_line":"                                               active)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a930d6b_94d6814d","line":466,"updated":"2015-01-27 18:44:44.000000000","message":"I\u0027m not sure it is appropriate for this method to check for dvr.  It seems a bit tangled up.  I\u0027m not familiar enough with this code to suggest a better alternative though.  I\u0027d like for Swami and Michael who are working on getting ha and dvr to work together better to comment here.","commit_id":"1ebf207a74f68cd87d9081c2a53ac9a104f79945"}],"neutron/tests/unit/db/test_l3_dvr_db.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"676ca322b958757ec528165d383b687a2cd2d2f8","unresolved":false,"context_lines":[{"line_number":293,"context_line":"    def test_floatingip_on_port_no_host(self):"},{"line_number":294,"context_line":"        router, fip \u003d self._floatingip_on_port_test_setup(None)"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":"        self.assertTrue(self.mixin.get_vm_port_hostid.called)"},{"line_number":297,"context_line":"        self.assertFalse(self.mixin._get_agent_by_type_and_host.called)"},{"line_number":298,"context_line":"        self.assertFalse(self.mixin.get_fip_sync_interfaces.called)"},{"line_number":299,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_700f8188","side":"PARENT","line":296,"updated":"2015-01-27 05:27:59.000000000","message":"It isn\u0027t clear to me why these asserts are being removed.  Maybe because it is a little bit late.","commit_id":"fcdb2a88fc3eb7827d71d5ebb7344132f220ad68"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"12bbb43b4da05f824f8f510627d185f5d7c577c7","unresolved":false,"context_lines":[{"line_number":293,"context_line":"    def test_floatingip_on_port_no_host(self):"},{"line_number":294,"context_line":"        router, fip \u003d self._floatingip_on_port_test_setup(None)"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":"        self.assertTrue(self.mixin.get_vm_port_hostid.called)"},{"line_number":297,"context_line":"        self.assertFalse(self.mixin._get_agent_by_type_and_host.called)"},{"line_number":298,"context_line":"        self.assertFalse(self.mixin.get_fip_sync_interfaces.called)"},{"line_number":299,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_797f4ee0","side":"PARENT","line":296,"in_reply_to":"1a930d6b_37c31c39","updated":"2015-01-27 18:09:42.000000000","message":"Thanks.","commit_id":"fcdb2a88fc3eb7827d71d5ebb7344132f220ad68"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"487d9dc4fb48edf17a02e89f0419ec95c25e6f4a","unresolved":false,"context_lines":[{"line_number":293,"context_line":"    def test_floatingip_on_port_no_host(self):"},{"line_number":294,"context_line":"        router, fip \u003d self._floatingip_on_port_test_setup(None)"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":"        self.assertTrue(self.mixin.get_vm_port_hostid.called)"},{"line_number":297,"context_line":"        self.assertFalse(self.mixin._get_agent_by_type_and_host.called)"},{"line_number":298,"context_line":"        self.assertFalse(self.mixin.get_fip_sync_interfaces.called)"},{"line_number":299,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_37c31c39","side":"PARENT","line":296,"in_reply_to":"1a930d6b_700f8188","updated":"2015-01-27 15:02:44.000000000","message":"The assertFalse for _get_agent_by_type_and_host.called is removed since that call has been removed altogether from the _process_floating_ips method and hence it demonstrates nothing to assert that it isn\u0027t called.\n\nThe assertFalse for the get_fip_sync_interfaces.called is removed because with the optimization refactoring, it is called for all instances, so asserting that it isn\u0027t called is just wrong.","commit_id":"fcdb2a88fc3eb7827d71d5ebb7344132f220ad68"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"676ca322b958757ec528165d383b687a2cd2d2f8","unresolved":false,"context_lines":[{"line_number":267,"context_line":"    def _floatingip_on_port_test_setup(self, hostid):"},{"line_number":268,"context_line":"        router \u003d {\u0027id\u0027: \u0027foo_router_id\u0027, \u0027distributed\u0027: True}"},{"line_number":269,"context_line":"        if hostid:"},{"line_number":270,"context_line":"            floatingip \u003d {"},{"line_number":271,"context_line":"                \u0027id\u0027: _uuid(),"},{"line_number":272,"context_line":"                \u0027port_id\u0027: _uuid(),"},{"line_number":273,"context_line":"                \u0027router_id\u0027: \u0027foo_router_id\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_b0a1e995","line":270,"updated":"2015-01-27 05:27:59.000000000","message":"I think this would work for either case since hostid is None in the else: clause on L277.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"12bbb43b4da05f824f8f510627d185f5d7c577c7","unresolved":false,"context_lines":[{"line_number":267,"context_line":"    def _floatingip_on_port_test_setup(self, hostid):"},{"line_number":268,"context_line":"        router \u003d {\u0027id\u0027: \u0027foo_router_id\u0027, \u0027distributed\u0027: True}"},{"line_number":269,"context_line":"        if hostid:"},{"line_number":270,"context_line":"            floatingip \u003d {"},{"line_number":271,"context_line":"                \u0027id\u0027: _uuid(),"},{"line_number":272,"context_line":"                \u0027port_id\u0027: _uuid(),"},{"line_number":273,"context_line":"                \u0027router_id\u0027: \u0027foo_router_id\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_79e3ced8","line":270,"in_reply_to":"1a930d6b_37487ccb","updated":"2015-01-27 18:09:42.000000000","message":"I was thinking this but I\u0027m not going to fuss over it:\n\n  floatingip \u003d {\n      ...\n      \u0027host\u0027: hostid\n  }\n  if not hostid:\n      hostid \u003d \u0027not_my_host_id\u0027","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"94dfc1aeaa938a3a0b23cd87b2c97074c4e08c35","unresolved":false,"context_lines":[{"line_number":267,"context_line":"    def _floatingip_on_port_test_setup(self, hostid):"},{"line_number":268,"context_line":"        router \u003d {\u0027id\u0027: \u0027foo_router_id\u0027, \u0027distributed\u0027: True}"},{"line_number":269,"context_line":"        if hostid:"},{"line_number":270,"context_line":"            floatingip \u003d {"},{"line_number":271,"context_line":"                \u0027id\u0027: _uuid(),"},{"line_number":272,"context_line":"                \u0027port_id\u0027: _uuid(),"},{"line_number":273,"context_line":"                \u0027router_id\u0027: \u0027foo_router_id\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_f498a5c1","line":270,"in_reply_to":"1a930d6b_79e3ced8","updated":"2015-01-27 18:22:50.000000000","message":"Oh, I misunderstood your original comment.  If another round of patches is called for I will fix it there.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"487d9dc4fb48edf17a02e89f0419ec95c25e6f4a","unresolved":false,"context_lines":[{"line_number":267,"context_line":"    def _floatingip_on_port_test_setup(self, hostid):"},{"line_number":268,"context_line":"        router \u003d {\u0027id\u0027: \u0027foo_router_id\u0027, \u0027distributed\u0027: True}"},{"line_number":269,"context_line":"        if hostid:"},{"line_number":270,"context_line":"            floatingip \u003d {"},{"line_number":271,"context_line":"                \u0027id\u0027: _uuid(),"},{"line_number":272,"context_line":"                \u0027port_id\u0027: _uuid(),"},{"line_number":273,"context_line":"                \u0027router_id\u0027: \u0027foo_router_id\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_37487ccb","line":270,"in_reply_to":"1a930d6b_b0a1e995","updated":"2015-01-27 15:02:44.000000000","message":"It doesn\u0027t work for both cases since I am using the hostid is None to distinguish the test case where the floating ip port is on the host from the case where the floating ip is not on the host, which necessitates that the \u0027host\u0027 in the floatingip is different from hostid, which is why I set the hostid to \u0027not_my_host_id\u0027 when it is passed in as None.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"676ca322b958757ec528165d383b687a2cd2d2f8","unresolved":false,"context_lines":[{"line_number":298,"context_line":"            return_value\u003d\u0027fip_interface\u0027)"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        self.mixin._process_floating_ips(self.ctx, routers, [floatingip],"},{"line_number":301,"context_line":"                                         hostid, agentview(fipagent))"},{"line_number":302,"context_line":"        return (router, floatingip)"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"    def test_floatingip_on_port_not_host(self):"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_7038613d","line":301,"updated":"2015-01-27 05:27:59.000000000","message":"I think I\u0027d go with a mock...  It is just more straight-forward.\n\n  agent \u003d mock.Mock()\n  agent.id \u003d fipagent[\u0027id\u0027]","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"487d9dc4fb48edf17a02e89f0419ec95c25e6f4a","unresolved":false,"context_lines":[{"line_number":298,"context_line":"            return_value\u003d\u0027fip_interface\u0027)"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        self.mixin._process_floating_ips(self.ctx, routers, [floatingip],"},{"line_number":301,"context_line":"                                         hostid, agentview(fipagent))"},{"line_number":302,"context_line":"        return (router, floatingip)"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"    def test_floatingip_on_port_not_host(self):"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a930d6b_37e4fcaa","line":301,"in_reply_to":"1a930d6b_7038613d","updated":"2015-01-27 15:02:44.000000000","message":"No problem, I\u0027ll go ahead and change it.","commit_id":"16aebb500902730cb335dbba41b8fade49a923e2"},{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"364fce2d9398709d7883aad3eba93af122428307","unresolved":false,"context_lines":[{"line_number":293,"context_line":"            \u0027router_id\u0027: \u0027foo_router_id\u0027,"},{"line_number":294,"context_line":"            \u0027host\u0027: hostid"},{"line_number":295,"context_line":"        }"},{"line_number":296,"context_line":"        if not hostid:"},{"line_number":297,"context_line":"            hostid \u003d \u0027not_my_host_id\u0027"},{"line_number":298,"context_line":"        routers \u003d {"},{"line_number":299,"context_line":"            \u0027foo_router_id\u0027: router"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a80dd14_2f327210","line":296,"updated":"2015-03-05 18:53:27.000000000","message":"Why do you need this check in here?","commit_id":"7d02cba050a3c148351a51dd4ee899045696dfe6"},{"author":{"_account_id":10652,"name":"Erik Colnick","email":"erikcolnick@gmail.com","username":"ecolnick"},"change_message_id":"2f11919e7819420eca21ac83d6adb835a7dfe10c","unresolved":false,"context_lines":[{"line_number":293,"context_line":"            \u0027router_id\u0027: \u0027foo_router_id\u0027,"},{"line_number":294,"context_line":"            \u0027host\u0027: hostid"},{"line_number":295,"context_line":"        }"},{"line_number":296,"context_line":"        if not hostid:"},{"line_number":297,"context_line":"            hostid \u003d \u0027not_my_host_id\u0027"},{"line_number":298,"context_line":"        routers \u003d {"},{"line_number":299,"context_line":"            \u0027foo_router_id\u0027: router"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a80dd14_8bc6f147","line":296,"in_reply_to":"9a80dd14_2f327210","updated":"2015-03-05 19:02:22.000000000","message":"To support the two separate test cases test_floatingip_on_port_not_host and test_floatingip_on_port_with_host","commit_id":"7d02cba050a3c148351a51dd4ee899045696dfe6"}]}