)]}'
{".pylintrc":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"50a80ff7f5cc4fd8648eb3f2f3c00ffbd64fe510","unresolved":false,"context_lines":[{"line_number":69,"context_line":" too-many-public-methods,"},{"line_number":70,"context_line":" too-many-return-statements,"},{"line_number":71,"context_line":" too-many-statements,"},{"line_number":72,"context_line":" bad-super-call"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"[BASIC]"},{"line_number":75,"context_line":"# Variable names can be 1 to 31 characters long, with lowercase and underscores"}],"source_content_type":"application/octet-stream","patch_set":45,"id":"9a1a9d01_734ca070","line":72,"updated":"2015-09-25 23:17:48.000000000","message":"Yikes:  Is this really what we want to be doing?","commit_id":"cf360f7308dcf305854ac351fe2274fe508fc4b5"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"850b72bdebd2281b20885b3b15539a57aad07192","unresolved":false,"context_lines":[{"line_number":69,"context_line":" too-many-public-methods,"},{"line_number":70,"context_line":" too-many-return-statements,"},{"line_number":71,"context_line":" too-many-statements,"},{"line_number":72,"context_line":" bad-super-call"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"[BASIC]"},{"line_number":75,"context_line":"# Variable names can be 1 to 31 characters long, with lowercase and underscores"}],"source_content_type":"application/octet-stream","patch_set":45,"id":"9a1a9d01_c29486e3","line":72,"in_reply_to":"9a1a9d01_734ca070","updated":"2015-09-28 17:30:55.000000000","message":"I understand your concern. I am not sure either. John and I added this because pylint seems to actually have a problem (defect?) \nif you make a call  to super(classname, self)  and classname is not the name of the current class, it will flag it as an error.  \nin other words... if you have something like: \n\nclass mychildclass(mom, dad, object). \n   .... \n   def someMethod(self):\n         super(dad, self).someMethod()\n\n which is a valid call, pylint will flag it as an error because \"dad\" is not the name of the current class.  \nIf you do some googling on the subject you\u0027ll  find arguments for and against pylint on this. \nThis type of calls are pretty common in the code. But I share your hesitation. The problem is that if we remove this then we have to duplicate lots of code, because we can\u0027t make super calls to other ancestor classes, because  pylint flaging it as an error under tox -e pep8 test. \n\nI am good either way, but we\u0027ll have to duplicate more code if we decide to take it out.","commit_id":"cf360f7308dcf305854ac351fe2274fe508fc4b5"}],"/COMMIT_MSG":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":1,"context_line":"Parent:     b10e3bc4 (Merge \"SimpleInterfaceMonitor: get rid of self.data_received flag\")"},{"line_number":2,"context_line":"Author:     Artur Korzeniewski \u003cartur.korzeniewski@intel.com\u003e"},{"line_number":3,"context_line":"AuthorDate: 2015-09-30 14:22:45 +0200"},{"line_number":4,"context_line":"Commit:     Artur Korzeniewski \u003cartur.korzeniewski@intel.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2015-09-30 14:22:45 +0200"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":46,"id":"9a1a9d01_c005fd13","line":2,"updated":"2015-09-30 17:55:43.000000000","message":"Artur, how did the author get reset since PS45?  Normally, git requires an extra option to reset the author with an --amend.  If there is something in your patch amend work flow that did this, you need to fix that.  If you would like your contribution recognized, it is appropriate to add yourself to the list of co-authors.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f57bd04956c692096b53a023ecaa1b7067da549a","unresolved":false,"context_lines":[{"line_number":1,"context_line":"Parent:     b10e3bc4 (Merge \"SimpleInterfaceMonitor: get rid of self.data_received flag\")"},{"line_number":2,"context_line":"Author:     Artur Korzeniewski \u003cartur.korzeniewski@intel.com\u003e"},{"line_number":3,"context_line":"AuthorDate: 2015-09-30 14:22:45 +0200"},{"line_number":4,"context_line":"Commit:     Artur Korzeniewski \u003cartur.korzeniewski@intel.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2015-09-30 14:22:45 +0200"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":46,"id":"9a1a9d01_49457a40","line":2,"in_reply_to":"9a1a9d01_c005fd13","updated":"2015-10-01 13:01:35.000000000","message":"Done","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"119820ae935745a365f4376242903a211f47699c","unresolved":false,"context_lines":[{"line_number":1,"context_line":"Parent:     b10e3bc4 (Merge \"SimpleInterfaceMonitor: get rid of self.data_received flag\")"},{"line_number":2,"context_line":"Author:     Artur Korzeniewski \u003cartur.korzeniewski@intel.com\u003e"},{"line_number":3,"context_line":"AuthorDate: 2015-09-30 14:22:45 +0200"},{"line_number":4,"context_line":"Commit:     Artur Korzeniewski \u003cartur.korzeniewski@intel.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2015-09-30 14:22:45 +0200"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":46,"id":"9a1a9d01_7405ca11","line":2,"in_reply_to":"9a1a9d01_c005fd13","updated":"2015-09-30 18:57:30.000000000","message":"Sorry, I have by mistake push my local branch (dvr_ha2) instead of topic bugs/196893 - I think this changed the author. I\u0027m terribly sorry about that - I did not have intention to override the original author of this patch. I will fix it tomorrow.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"}],"neutron/agent/l3/agent.py":[{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"e9f9c7bfbc8de2d254b71b5c89b4b672d4aabe1b","unresolved":false,"context_lines":[{"line_number":348,"context_line":"            return"},{"line_number":349,"context_line":""},{"line_number":350,"context_line":"        registry.notify(resources.ROUTER, events.BEFORE_DELETE,"},{"line_number":351,"context_line":"                        self, router\u003dri)"},{"line_number":352,"context_line":"        ri.delete(self)"},{"line_number":353,"context_line":"        del self.router_info[router_id]"},{"line_number":354,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_cc067ecd","line":351,"updated":"2015-07-09 22:10:45.000000000","message":"change: not related to this patch.","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"e234fb1f950bfcec2a24963542c0ab6159b905bc","unresolved":false,"context_lines":[{"line_number":348,"context_line":"            return"},{"line_number":349,"context_line":""},{"line_number":350,"context_line":"        registry.notify(resources.ROUTER, events.BEFORE_DELETE,"},{"line_number":351,"context_line":"                        self, router\u003dri)"},{"line_number":352,"context_line":"        ri.delete(self)"},{"line_number":353,"context_line":"        del self.router_info[router_id]"},{"line_number":354,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_833cc0c0","line":351,"in_reply_to":"ba3cc151_cc067ecd","updated":"2015-07-10 00:05:44.000000000","message":"Done","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"4cfab84824045c9dab01938ece5629b8c33071e0","unresolved":false,"context_lines":[{"line_number":301,"context_line":"            kwargs[\u0027state_change_callback\u0027] \u003d self.enqueue_state_change"},{"line_number":302,"context_line":"            return dvr_edge_ha_router.DvrEdgeHaRouter(*args, **kwargs)"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"        elif router.get(\u0027distributed\u0027):"},{"line_number":305,"context_line":"            kwargs[\u0027agent\u0027] \u003d self"},{"line_number":306,"context_line":"            kwargs[\u0027host\u0027] \u003d self.host"},{"line_number":307,"context_line":"            if self.conf.agent_mode \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT:"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_793a769a","line":304,"updated":"2015-07-10 12:25:02.000000000","message":"nit: This line, L312, and L316 don\u0027t need to be changed as the if clauses preceding them will return. Can leave the original L301, 309, 313 as is.","commit_id":"870ef2a3442e8c420d84e41f135a84868fd55473"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"d2c13d1f70106b5ee716203cccef80f95ccab0c8","unresolved":false,"context_lines":[{"line_number":301,"context_line":"            kwargs[\u0027state_change_callback\u0027] \u003d self.enqueue_state_change"},{"line_number":302,"context_line":"            return dvr_edge_ha_router.DvrEdgeHaRouter(*args, **kwargs)"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"        elif router.get(\u0027distributed\u0027):"},{"line_number":305,"context_line":"            kwargs[\u0027agent\u0027] \u003d self"},{"line_number":306,"context_line":"            kwargs[\u0027host\u0027] \u003d self.host"},{"line_number":307,"context_line":"            if self.conf.agent_mode \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT:"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a50d1a3_2989cd26","line":304,"in_reply_to":"ba3cc151_793a769a","updated":"2015-07-21 18:45:53.000000000","message":"Done","commit_id":"870ef2a3442e8c420d84e41f135a84868fd55473"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"477f5136adb4c9b7596532148ff7302304a99c18","unresolved":false,"context_lines":[{"line_number":284,"context_line":"            \u0027use_ipv6\u0027: self.use_ipv6,"},{"line_number":285,"context_line":"            \u0027agent_conf\u0027: self.conf,"},{"line_number":286,"context_line":"            \u0027interface_driver\u0027: self.driver,"},{"line_number":287,"context_line":"        }"},{"line_number":288,"context_line":""},{"line_number":289,"context_line":"        if router.get(\u0027distributed\u0027) and router.get(\u0027ha\u0027):"},{"line_number":290,"context_line":"            kwargs[\u0027agent\u0027] \u003d self"}],"source_content_type":"text/x-python","patch_set":19,"id":"fa1b9901_76a3229f","line":287,"updated":"2015-08-24 17:43:36.000000000","message":"Instead of duplicating all the kwargs additions below for both cases, it can all be done here:\n\n        if router.get(\u0027distributed\u0027):\n            kwargs[\u0027agent\u0027] \u003d self\n            kwargs[\u0027host\u0027] \u003d self.host\n\n        if router.get(\u0027ha\u0027):\n            kwargs[\u0027state_change_callback\u0027] \u003d self.enqueue_state_change","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"2fe4153b80da6e03d4a8556456d67544cad7b4f5","unresolved":false,"context_lines":[{"line_number":284,"context_line":"            \u0027use_ipv6\u0027: self.use_ipv6,"},{"line_number":285,"context_line":"            \u0027agent_conf\u0027: self.conf,"},{"line_number":286,"context_line":"            \u0027interface_driver\u0027: self.driver,"},{"line_number":287,"context_line":"        }"},{"line_number":288,"context_line":""},{"line_number":289,"context_line":"        if router.get(\u0027distributed\u0027) and router.get(\u0027ha\u0027):"},{"line_number":290,"context_line":"            kwargs[\u0027agent\u0027] \u003d self"}],"source_content_type":"text/x-python","patch_set":19,"id":"fa1b9901_a4b3c009","line":287,"in_reply_to":"fa1b9901_76a3229f","updated":"2015-08-24 18:17:44.000000000","message":"Thank you ! \nwill add asap. will be in next sp","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"4215ce7bf1291f3d79c8bc45f9060cca3c4aa638","unresolved":false,"context_lines":[{"line_number":309,"context_line":"                kwargs[\u0027state_change_callback\u0027] \u003d self.enqueue_state_change"},{"line_number":310,"context_line":"                return dvr_edge_ha_router.DvrEdgeHaRouter(*args, **kwargs)"},{"line_number":311,"context_line":"            else:"},{"line_number":312,"context_line":"                return dvr_local_router.DvrLocalRouter(*args, **kwargs)"},{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        if router.get(\u0027distributed\u0027):"},{"line_number":315,"context_line":"            if self.conf.agent_mode \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT:"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_39d5a2b8","line":312,"updated":"2015-08-27 20:30:02.000000000","message":"nit: this line is the same as 318, this if statement online 306 could be improved to also  check for self.conf.agent_mode \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT that would allow you to restructure this code in a cleaner way.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":309,"context_line":"                kwargs[\u0027state_change_callback\u0027] \u003d self.enqueue_state_change"},{"line_number":310,"context_line":"                return dvr_edge_ha_router.DvrEdgeHaRouter(*args, **kwargs)"},{"line_number":311,"context_line":"            else:"},{"line_number":312,"context_line":"                return dvr_local_router.DvrLocalRouter(*args, **kwargs)"},{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        if router.get(\u0027distributed\u0027):"},{"line_number":315,"context_line":"            if self.conf.agent_mode \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT:"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_c8176943","line":312,"in_reply_to":"da20952f_39d5a2b8","updated":"2015-08-31 19:15:25.000000000","message":"Done","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"060cdeb6de3c255713fe883608f0a6a4880a36cb","unresolved":false,"context_lines":[{"line_number":309,"context_line":"                kwargs[\u0027state_change_callback\u0027] \u003d self.enqueue_state_change"},{"line_number":310,"context_line":"                return dvr_edge_ha_router.DvrEdgeHaRouter(*args, **kwargs)"},{"line_number":311,"context_line":"            else:"},{"line_number":312,"context_line":"                return dvr_local_router.DvrLocalRouter(*args, **kwargs)"},{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        if router.get(\u0027distributed\u0027):"},{"line_number":315,"context_line":"            if self.conf.agent_mode \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT:"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_4f0684cf","line":312,"in_reply_to":"da20952f_39d5a2b8","updated":"2015-08-27 21:29:38.000000000","message":"You could actually remove L311 and L312 and it would work just fine.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":309,"context_line":"                kwargs[\u0027state_change_callback\u0027] \u003d self.enqueue_state_change"},{"line_number":310,"context_line":"                return dvr_edge_ha_router.DvrEdgeHaRouter(*args, **kwargs)"},{"line_number":311,"context_line":"            else:"},{"line_number":312,"context_line":"                return dvr_local_router.DvrLocalRouter(*args, **kwargs)"},{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        if router.get(\u0027distributed\u0027):"},{"line_number":315,"context_line":"            if self.conf.agent_mode \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT:"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_480b5924","line":312,"in_reply_to":"da20952f_4f0684cf","updated":"2015-08-31 19:15:25.000000000","message":"Going with Carl\u0027s suggestion. Thanks.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":311,"context_line":"            kwargs[\u0027agent\u0027] \u003d self"},{"line_number":312,"context_line":"            kwargs[\u0027host\u0027] \u003d self.host"},{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        if router.get(\u0027distributed\u0027) and router.get(\u0027ha\u0027):"},{"line_number":315,"context_line":"            if self.conf.agent_mode \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT:"},{"line_number":316,"context_line":"                kwargs[\u0027state_change_callback\u0027] \u003d self.enqueue_state_change"},{"line_number":317,"context_line":"                return dvr_edge_ha_router.DvrEdgeHaRouter(*args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_2731a8ed","line":314,"updated":"2015-09-23 21:58:56.000000000","message":"Note to self: What happens if a DVR+HA router is scheduled to a \u0027dvr\u0027 node? (And not dvr-snat).","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":16934,"name":"Martin Roy","email":"lindycoder@gmail.com","username":"lindycoder"},"change_message_id":"dba3c4a91999fc8cb6a9ca77b1341249aa1c2553","unresolved":false,"context_lines":[{"line_number":311,"context_line":"            kwargs[\u0027agent\u0027] \u003d self"},{"line_number":312,"context_line":"            kwargs[\u0027host\u0027] \u003d self.host"},{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        if router.get(\u0027distributed\u0027) and router.get(\u0027ha\u0027):"},{"line_number":315,"context_line":"            if self.conf.agent_mode \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT:"},{"line_number":316,"context_line":"                kwargs[\u0027state_change_callback\u0027] \u003d self.enqueue_state_change"},{"line_number":317,"context_line":"                return dvr_edge_ha_router.DvrEdgeHaRouter(*args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_c4a43a4c","line":314,"updated":"2015-09-30 13:53:50.000000000","message":"i think you could rework this whole if section from line 310 to 323 to something like that :\n\nif router.get(\u0027distributed\u0027):\n   update kwargs...\n   \n   if agent_mode \u003d\u003d DVR_SNAT:\n       if router.get(\u0027ha\u0027):\n          update kwargs...\n          return edge ha router\n       else:\n          return edge router\n    else:\n       return local","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":311,"context_line":"            kwargs[\u0027agent\u0027] \u003d self"},{"line_number":312,"context_line":"            kwargs[\u0027host\u0027] \u003d self.host"},{"line_number":313,"context_line":""},{"line_number":314,"context_line":"        if router.get(\u0027distributed\u0027) and router.get(\u0027ha\u0027):"},{"line_number":315,"context_line":"            if self.conf.agent_mode \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT:"},{"line_number":316,"context_line":"                kwargs[\u0027state_change_callback\u0027] \u003d self.enqueue_state_change"},{"line_number":317,"context_line":"                return dvr_edge_ha_router.DvrEdgeHaRouter(*args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_19404c33","line":314,"in_reply_to":"9a1a9d01_c4a43a4c","updated":"2015-09-30 17:55:43.000000000","message":"+1  I wouldn\u0027t require you to change this but it is a slight improvement.  Use your judgement.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"}],"neutron/agent/l3/dvr_edge_ha_router.py":[{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"e9f9c7bfbc8de2d254b71b5c89b4b672d4aabe1b","unresolved":false,"context_lines":[{"line_number":35,"context_line":"    def get_snat_ns_name(self):"},{"line_number":36,"context_line":"        return self.snat_namespace.name"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    # This can\u0027t be inherited as both DvrEdgeRouter and"},{"line_number":39,"context_line":"    # HaRouter classes implement it and we don\u0027t want"},{"line_number":40,"context_line":"    # to execute the one from HaRouter, so we can\u0027t call"},{"line_number":41,"context_line":"    # super on it. Classic diamond inheritance problem."}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_6ce4120c","line":38,"updated":"2015-07-09 22:10:45.000000000","message":"This comment needs some change, does\u0027t sound right.","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"e234fb1f950bfcec2a24963542c0ab6159b905bc","unresolved":false,"context_lines":[{"line_number":35,"context_line":"    def get_snat_ns_name(self):"},{"line_number":36,"context_line":"        return self.snat_namespace.name"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    # This can\u0027t be inherited as both DvrEdgeRouter and"},{"line_number":39,"context_line":"    # HaRouter classes implement it and we don\u0027t want"},{"line_number":40,"context_line":"    # to execute the one from HaRouter, so we can\u0027t call"},{"line_number":41,"context_line":"    # super on it. Classic diamond inheritance problem."}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_43b47834","line":38,"in_reply_to":"ba3cc151_6ce4120c","updated":"2015-07-10 00:05:44.000000000","message":"removed comment. should be obvious from code","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"4cfab84824045c9dab01938ece5629b8c33071e0","unresolved":false,"context_lines":[{"line_number":25,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"class DvrEdgeHaRouter(DvrEdgeRouter, HaRouter):"},{"line_number":29,"context_line":"    def __init__(self, agent, host, *args, **kwargs):"},{"line_number":30,"context_line":"        super(DvrEdgeHaRouter, self).__init__(agent, host,"},{"line_number":31,"context_line":"                                              *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_99c65a67","line":28,"updated":"2015-07-10 12:25:02.000000000","message":"It would be really helpful to readers (especially those unfamiliar with this code), to have a docstring for the class. Would be nice to add to other classes as well, as the naming is hard to understand (I didn\u0027t initially realize that the edge router was the centralized router with SNAT).   It would be great if we could add some summary for each class.","commit_id":"870ef2a3442e8c420d84e41f135a84868fd55473"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"d2c13d1f70106b5ee716203cccef80f95ccab0c8","unresolved":false,"context_lines":[{"line_number":25,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"class DvrEdgeHaRouter(DvrEdgeRouter, HaRouter):"},{"line_number":29,"context_line":"    def __init__(self, agent, host, *args, **kwargs):"},{"line_number":30,"context_line":"        super(DvrEdgeHaRouter, self).__init__(agent, host,"},{"line_number":31,"context_line":"                                              *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a50d1a3_09ebb1b1","line":28,"in_reply_to":"ba3cc151_99c65a67","updated":"2015-07-21 18:45:53.000000000","message":"Will do","commit_id":"870ef2a3442e8c420d84e41f135a84868fd55473"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"03665db030be100630f2dd3c0e72e870e74c3511","unresolved":false,"context_lines":[{"line_number":100,"context_line":"            ex_gw_port, interface_name)"},{"line_number":101,"context_line":"        for port in self.internal_ports:"},{"line_number":102,"context_line":"            for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":103,"context_line":"                self._remvoe_vip(ip_cidr)"},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"},{"line_number":106,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_updated("}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_46199f87","line":103,"updated":"2015-08-04 17:21:58.000000000","message":"_remove_vip()?\n\nBegs the question of whether there is a UT that tests this change?","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":100,"context_line":"            ex_gw_port, interface_name)"},{"line_number":101,"context_line":"        for port in self.internal_ports:"},{"line_number":102,"context_line":"            for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":103,"context_line":"                self._remvoe_vip(ip_cidr)"},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"},{"line_number":106,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_updated("}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_ce5aa12a","line":103,"in_reply_to":"1a4dcd0f_46199f87","updated":"2015-08-04 19:13:41.000000000","message":"*Puts his testing lieutenant hat on*\n\nTsk tsk!","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":100,"context_line":"            ex_gw_port, interface_name)"},{"line_number":101,"context_line":"        for port in self.internal_ports:"},{"line_number":102,"context_line":"            for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":103,"context_line":"                self._remvoe_vip(ip_cidr)"},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"},{"line_number":106,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_updated("}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_d260c0d1","line":103,"in_reply_to":"1a4dcd0f_46199f87","updated":"2015-08-06 19:36:01.000000000","message":"will add to ut","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"e1ec6ac8adb1f0090014cb0a61576e1144b0afe1","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                               bridge\u003dself.agent_conf.external_network_bridge,"},{"line_number":118,"context_line":"                               namespace\u003dself.snat_namespace.name,"},{"line_number":119,"context_line":"                               prefix\u003drouter.EXTERNAL_DEV_PREFIX)"},{"line_number":120,"context_line":"    #self.snat_namespace.delete()"},{"line_number":121,"context_line":"    #self.snat_namespace \u003d None"},{"line_number":122,"context_line":"        for port in self.internal_ports:"},{"line_number":123,"context_line":"            for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_fc7697a2","line":120,"updated":"2015-08-10 12:23:02.000000000","message":"remove dead code","commit_id":"8eb7fec6ea238254d53fb6d97149bebb19de7e54"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"0e856833f2431af70c5990c18a9ba6fecdc57f96","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                               bridge\u003dself.agent_conf.external_network_bridge,"},{"line_number":118,"context_line":"                               namespace\u003dself.snat_namespace.name,"},{"line_number":119,"context_line":"                               prefix\u003drouter.EXTERNAL_DEV_PREFIX)"},{"line_number":120,"context_line":"    #self.snat_namespace.delete()"},{"line_number":121,"context_line":"    #self.snat_namespace \u003d None"},{"line_number":122,"context_line":"        for port in self.internal_ports:"},{"line_number":123,"context_line":"            for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_fa34e9de","line":120,"in_reply_to":"1a4dcd0f_fc7697a2","updated":"2015-08-11 03:04:10.000000000","message":"Done","commit_id":"8eb7fec6ea238254d53fb6d97149bebb19de7e54"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"e1ec6ac8adb1f0090014cb0a61576e1144b0afe1","unresolved":false,"context_lines":[{"line_number":121,"context_line":"    #self.snat_namespace \u003d None"},{"line_number":122,"context_line":"        for port in self.internal_ports:"},{"line_number":123,"context_line":"            for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":124,"context_line":"                self._remove_vip(ip_cidr)"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"},{"line_number":127,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_updated("}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_6e43646d","line":124,"updated":"2015-08-10 12:23:02.000000000","message":"Do we have a UT that checks that this is called, when external_gateway_removed() is invoked?","commit_id":"8eb7fec6ea238254d53fb6d97149bebb19de7e54"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"0e856833f2431af70c5990c18a9ba6fecdc57f96","unresolved":false,"context_lines":[{"line_number":121,"context_line":"    #self.snat_namespace \u003d None"},{"line_number":122,"context_line":"        for port in self.internal_ports:"},{"line_number":123,"context_line":"            for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":124,"context_line":"                self._remove_vip(ip_cidr)"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"},{"line_number":127,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_updated("}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_ba4d0174","line":124,"in_reply_to":"1a4dcd0f_6e43646d","updated":"2015-08-11 03:04:10.000000000","message":"done in ps 14","commit_id":"8eb7fec6ea238254d53fb6d97149bebb19de7e54"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"b1bae201f12b861c53bec88f8407f7b03de52042","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                               bridge\u003dself.agent_conf.external_network_bridge,"},{"line_number":118,"context_line":"                               namespace\u003dself.snat_namespace.name,"},{"line_number":119,"context_line":"                               prefix\u003drouter.EXTERNAL_DEV_PREFIX)"},{"line_number":120,"context_line":"    #self.snat_namespace.delete()"},{"line_number":121,"context_line":"    #self.snat_namespace \u003d None"},{"line_number":122,"context_line":"        for port in self.internal_ports:"},{"line_number":123,"context_line":"            for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"}],"source_content_type":"text/x-python","patch_set":14,"id":"1a4dcd0f_14406533","line":120,"updated":"2015-08-11 17:51:13.000000000","message":"See PS12 comment. dead code still there.","commit_id":"fe758c91c1a0c8c7f4103be38bcf3cb7f319b811"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"aeea5b602e97452d8d4f95ab01360ef632788600","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        if not self._is_this_snat_host():"},{"line_number":113,"context_line":"            # no centralized SNAT gateway for this node/agent"},{"line_number":114,"context_line":"            LOG.debug(\"not hosting snat for router: %s\", self.router[\u0027id\u0027])"},{"line_number":115,"context_line":"        else:"},{"line_number":116,"context_line":"            self.driver.unplug(interface_name,"},{"line_number":117,"context_line":"                               bridge\u003dself.agent_conf.external_network_bridge,"},{"line_number":118,"context_line":"                               namespace\u003dself.snat_namespace.name,"}],"source_content_type":"text/x-python","patch_set":19,"id":"fa1b9901_35f615d8","line":115,"updated":"2015-08-24 11:57:27.000000000","message":"At this point we\u0027re already sure that the external gateway is removed and that this is an snat host. In this case, shouldn\u0027t we also delete the \"sg-\" device (which currently isn\u0027t cleaned up at all until the router is deleted?)\n\nI\u0027ve written a code snippet to delete \"sg-\" devices ([1]), and inside this \u0027else\u0027 bracket I\u0027ve inserted this loop:\n\n for port in self.snat_ports:\n     self._dvr_external_network_removed(port)\n\n[1]: http://pastebin.com/ZPFjSpeb","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"2fe4153b80da6e03d4a8556456d67544cad7b4f5","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        if not self._is_this_snat_host():"},{"line_number":113,"context_line":"            # no centralized SNAT gateway for this node/agent"},{"line_number":114,"context_line":"            LOG.debug(\"not hosting snat for router: %s\", self.router[\u0027id\u0027])"},{"line_number":115,"context_line":"        else:"},{"line_number":116,"context_line":"            self.driver.unplug(interface_name,"},{"line_number":117,"context_line":"                               bridge\u003dself.agent_conf.external_network_bridge,"},{"line_number":118,"context_line":"                               namespace\u003dself.snat_namespace.name,"}],"source_content_type":"text/x-python","patch_set":19,"id":"fa1b9901_c4283436","line":115,"in_reply_to":"fa1b9901_35f615d8","updated":"2015-08-24 18:17:44.000000000","message":"I like it thank you. \nBut I think this might be addressed by the fix to bug (which also affects dvr) by itself.\nhttps://bugs.launchpad.net/neutron/+bug/1479130\nwhich is being addressed by:\nhttps://review.openstack.org/#/c/215329/\n\nIf it is the same reason ( I am 90% sure it is) merging this patch will fix it. I will test manually to verify and make sure patch 215329 fixes what you point out John. \n\nThanks.","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"2fef6d19bcb371b279f0753e9b47eb130fa27673","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        if not self._is_this_snat_host():"},{"line_number":113,"context_line":"            # no centralized SNAT gateway for this node/agent"},{"line_number":114,"context_line":"            LOG.debug(\"not hosting snat for router: %s\", self.router[\u0027id\u0027])"},{"line_number":115,"context_line":"        else:"},{"line_number":116,"context_line":"            self.driver.unplug(interface_name,"},{"line_number":117,"context_line":"                               bridge\u003dself.agent_conf.external_network_bridge,"},{"line_number":118,"context_line":"                               namespace\u003dself.snat_namespace.name,"}],"source_content_type":"text/x-python","patch_set":19,"id":"da20952f_701bbfca","line":115,"in_reply_to":"fa1b9901_c4283436","updated":"2015-08-31 10:44:02.000000000","message":"Said patch only fixes the case of deleting an interface, not a gateway.","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"aeea5b602e97452d8d4f95ab01360ef632788600","unresolved":false,"context_lines":[{"line_number":178,"context_line":"        return (self.agent_conf.agent_mode"},{"line_number":179,"context_line":"                \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"    def _dvr_internal_network_removed(self, port):"},{"line_number":182,"context_line":"        super(DvrEdgeHaRouter, self)._dvr_internal_network_removed(port)"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"        if not self.ex_gw_port:"}],"source_content_type":"text/x-python","patch_set":19,"id":"fa1b9901_423aea99","line":181,"updated":"2015-08-24 11:57:27.000000000","message":"When removing an internal interface (but when we still have the external gateway), we also want to remove the \"sg-\" device. However, this super() call doesn\u0027t do so because self.get_snat_port_for_internal_port() ([1]) returns an empty list (since the router dict referenced by self.router no longer has the expected ports since it was deleted server side and update from it during the sync).\n\nThis results in: 1. an error message saying \"DVR: no map match_port found!\" and 2. the \"sg-\" device is not deleted in this case.\n\nI\u0027m thinking we want to either add something like [3] to the code, to make sure it\u0027s aware of snat ports which were recently deleted, or explicitly call the snippet from my previous comment as well [2].\n\n[1]: https://github.com/openstack/neutron/blob/master/neutron/agent/l3/dvr_edge_router.py#L104\nhttps://github.com/openstack/neutron/blob/master/neutron/agent/l3/dvr_router_base.py#L29\n\n[2]: http://pastebin.com/ZPFjSpeb\n\n[3]:\n def get_snat_interfaces(self):\n     return (self.router.get(l3_constants.SNAT_ROUTER_INTF_KEY, []) or self.snat_ports)","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"2fe4153b80da6e03d4a8556456d67544cad7b4f5","unresolved":false,"context_lines":[{"line_number":178,"context_line":"        return (self.agent_conf.agent_mode"},{"line_number":179,"context_line":"                \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"    def _dvr_internal_network_removed(self, port):"},{"line_number":182,"context_line":"        super(DvrEdgeHaRouter, self)._dvr_internal_network_removed(port)"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"        if not self.ex_gw_port:"}],"source_content_type":"text/x-python","patch_set":19,"id":"fa1b9901_44ec24f3","line":181,"in_reply_to":"fa1b9901_423aea99","updated":"2015-08-24 18:17:44.000000000","message":"yes. Again, I will look at this to see if it fixes the bug in question: \nhttps://bugs.launchpad.net/neutron/+bug/1479130\n\nIf it does not I will take you helpful code and implement it. \n\nThanks again.","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"b72ea3f0f1374a2c6e37a944d5d46ea6625069b0","unresolved":false,"context_lines":[{"line_number":176,"context_line":""},{"line_number":177,"context_line":"    def _is_this_snat_host(self):"},{"line_number":178,"context_line":"        return (self.agent_conf.agent_mode"},{"line_number":179,"context_line":"                \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"    def _dvr_internal_network_removed(self, port):"},{"line_number":182,"context_line":"        super(DvrEdgeHaRouter, self)._dvr_internal_network_removed(port)"}],"source_content_type":"text/x-python","patch_set":27,"id":"da20952f_2dec808e","line":179,"updated":"2015-08-27 19:24:02.000000000","message":"nit: don\u0027t need the () here","commit_id":"b9dfa6fd85c06db9dcd9911cf23272e7ab43296e"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"060cdeb6de3c255713fe883608f0a6a4880a36cb","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright (c) 2015 Openstack Foundation"},{"line_number":2,"context_line":"#"},{"line_number":3,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":4,"context_line":"#    not use this file except in compliance with the License. You may obtain"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_4f2d2445","line":1,"updated":"2015-08-27 21:29:38.000000000","message":"Either use your company here or leave this line out entirely.  Don\u0027t use Openstack Foundation.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright (c) 2015 Openstack Foundation"},{"line_number":2,"context_line":"#"},{"line_number":3,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":4,"context_line":"#    not use this file except in compliance with the License. You may obtain"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_a5758032","line":1,"in_reply_to":"da20952f_4f2d2445","updated":"2015-08-31 19:15:25.000000000","message":"Done","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"060cdeb6de3c255713fe883608f0a6a4880a36cb","unresolved":false,"context_lines":[{"line_number":75,"context_line":"            self.ns_name, network_id, port_id, fixed_ips, mac_address,"},{"line_number":76,"context_line":"            interface_name, INTERNAL_DEV_PREFIX)"},{"line_number":77,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name,"},{"line_number":78,"context_line":"                                                   self.ns_name)"},{"line_number":79,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":80,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"},{"line_number":81,"context_line":""}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_6f7d871b","line":78,"updated":"2015-08-27 21:29:38.000000000","message":"Why should the HA stuff ever touch self.ns_name?  I would imagine that HA only applies to the snat part of a DVR router.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":75,"context_line":"            self.ns_name, network_id, port_id, fixed_ips, mac_address,"},{"line_number":76,"context_line":"            interface_name, INTERNAL_DEV_PREFIX)"},{"line_number":77,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name,"},{"line_number":78,"context_line":"                                                   self.ns_name)"},{"line_number":79,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":80,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"},{"line_number":81,"context_line":""}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_02a22299","line":78,"in_reply_to":"da20952f_6f7d871b","updated":"2015-08-31 19:15:25.000000000","message":"The interface in question here is qr-  which is in the ns namespace. \nThe curent DVREdgeRouter (ancestor) does not disable the ipv6 on the qr- interfaces, but HA requres ipv6 addressing to be disabled. Look at ancestor ha_router.py  (l268 - l281).  So it has to be done here, once ha_router does not require ipv6 to be disabled, then we can probably remove this line.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"060cdeb6de3c255713fe883608f0a6a4880a36cb","unresolved":false,"context_lines":[{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        self._add_gateway_vip(ex_gw_port, interface_name)"},{"line_number":98,"context_line":"        self._disable_ipv6_addressing_on_interface("},{"line_number":99,"context_line":"            interface_name, self.snat_namespace.name)"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":102,"context_line":"        self._clear_vips(interface_name)"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_afa5df04","line":99,"updated":"2015-08-27 21:29:38.000000000","message":"self.snat_namespace.name or self.snat_ns_name?","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"3cd9b2304df23fda2ad2d4f224e178a7cd90f2c8","unresolved":false,"context_lines":[{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        self._add_gateway_vip(ex_gw_port, interface_name)"},{"line_number":98,"context_line":"        self._disable_ipv6_addressing_on_interface("},{"line_number":99,"context_line":"            interface_name, self.snat_namespace.name)"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":102,"context_line":"        self._clear_vips(interface_name)"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_03cba973","line":99,"in_reply_to":"da20952f_a219f623","updated":"2015-09-02 08:54:54.000000000","message":"But according to \u0027initialize\u0027, snat_ns_name \u003d\u003d self.snat_namespace.name.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        self._add_gateway_vip(ex_gw_port, interface_name)"},{"line_number":98,"context_line":"        self._disable_ipv6_addressing_on_interface("},{"line_number":99,"context_line":"            interface_name, self.snat_namespace.name)"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":102,"context_line":"        self._clear_vips(interface_name)"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_a219f623","line":99,"in_reply_to":"da20952f_afa5df04","updated":"2015-08-31 19:15:25.000000000","message":"self.snat_namespace.name. The external gateway port is added on the snat namespace, so the ipv6 addressing has to be disabled. there for the interface.\nif you notice above, the internal_network_added does it on the ns_name, because that method deals with the qr- interface, this method deals with the sg- interface","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"4215ce7bf1291f3d79c8bc45f9060cca3c4aa638","unresolved":false,"context_lines":[{"line_number":150,"context_line":""},{"line_number":151,"context_line":"        # Update ex_gw_port and enable_snat on the router info cache"},{"line_number":152,"context_line":"        self.ex_gw_port \u003d self.get_ex_gw_port()"},{"line_number":153,"context_line":"        self.snat_ports \u003d self.router.get("},{"line_number":154,"context_line":"            l3_constants.SNAT_ROUTER_INTF_KEY, [])"},{"line_number":155,"context_line":"        self.enable_snat \u003d self.router.get(\u0027enable_snat\u0027)"},{"line_number":156,"context_line":"        if self.ha_port:"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_794eba97","line":153,"updated":"2015-08-27 20:30:02.000000000","message":"ditto with self.snat_ports this seems unused","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":150,"context_line":""},{"line_number":151,"context_line":"        # Update ex_gw_port and enable_snat on the router info cache"},{"line_number":152,"context_line":"        self.ex_gw_port \u003d self.get_ex_gw_port()"},{"line_number":153,"context_line":"        self.snat_ports \u003d self.router.get("},{"line_number":154,"context_line":"            l3_constants.SNAT_ROUTER_INTF_KEY, [])"},{"line_number":155,"context_line":"        self.enable_snat \u003d self.router.get(\u0027enable_snat\u0027)"},{"line_number":156,"context_line":"        if self.ha_port:"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_a51ca06a","line":153,"in_reply_to":"da20952f_794eba97","updated":"2015-08-31 19:15:25.000000000","message":"took out.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"4215ce7bf1291f3d79c8bc45f9060cca3c4aa638","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        self.ex_gw_port \u003d self.get_ex_gw_port()"},{"line_number":153,"context_line":"        self.snat_ports \u003d self.router.get("},{"line_number":154,"context_line":"            l3_constants.SNAT_ROUTER_INTF_KEY, [])"},{"line_number":155,"context_line":"        self.enable_snat \u003d self.router.get(\u0027enable_snat\u0027)"},{"line_number":156,"context_line":"        if self.ha_port:"},{"line_number":157,"context_line":"            self.enable_keepalived()"},{"line_number":158,"context_line":""}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_f9ac0a37","line":155,"updated":"2015-08-27 20:30:02.000000000","message":"Where is self.enabled_snat used? This looks unused to me.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        self.ex_gw_port \u003d self.get_ex_gw_port()"},{"line_number":153,"context_line":"        self.snat_ports \u003d self.router.get("},{"line_number":154,"context_line":"            l3_constants.SNAT_ROUTER_INTF_KEY, [])"},{"line_number":155,"context_line":"        self.enable_snat \u003d self.router.get(\u0027enable_snat\u0027)"},{"line_number":156,"context_line":"        if self.ha_port:"},{"line_number":157,"context_line":"            self.enable_keepalived()"},{"line_number":158,"context_line":""}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_6529a889","line":155,"in_reply_to":"da20952f_f9ac0a37","updated":"2015-08-31 19:15:25.000000000","message":"took out","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"    def internal_network_added(self, port):"},{"line_number":67,"context_line":"        # Both master and backup routers get the q-router interface fully"},{"line_number":68,"context_line":"        # configured with l3"},{"line_number":69,"context_line":"        network_id \u003d port[\u0027network_id\u0027]"},{"line_number":70,"context_line":"        port_id \u003d port[\u0027id\u0027]"},{"line_number":71,"context_line":"        fixed_ips \u003d port[\u0027fixed_ips\u0027]"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_e33eb528","line":68,"updated":"2015-09-02 11:46:46.000000000","message":"With the exception that HaRouter\u0027s also calls self._add_vip for fixed_ips, starting here and ending in line 78 this entire chunk of code is very similar to:\n\n super(HaRouter, self).internal_network_added(port)\n\nI\u0027m thinking we can probably get away with invoking super() instead of having this 10-lines of code duplication.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"    def internal_network_added(self, port):"},{"line_number":67,"context_line":"        # Both master and backup routers get the q-router interface fully"},{"line_number":68,"context_line":"        # configured with l3"},{"line_number":69,"context_line":"        network_id \u003d port[\u0027network_id\u0027]"},{"line_number":70,"context_line":"        port_id \u003d port[\u0027id\u0027]"},{"line_number":71,"context_line":"        fixed_ips \u003d port[\u0027fixed_ips\u0027]"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_bbc9af5c","line":68,"in_reply_to":"da20952f_e33eb528","updated":"2015-09-09 18:41:47.000000000","message":"yes. super(HaRouter, self).internal_network_added(port) will do the trick. \nthe only thing that needed to be left was the \"interface_name\" line, in order to make the _disable_ipv6_addressing_on_interface call. Don\u0027t see an easy way around that one. it just adds one line.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":68,"context_line":"        # configured with l3"},{"line_number":69,"context_line":"        network_id \u003d port[\u0027network_id\u0027]"},{"line_number":70,"context_line":"        port_id \u003d port[\u0027id\u0027]"},{"line_number":71,"context_line":"        fixed_ips \u003d port[\u0027fixed_ips\u0027]"},{"line_number":72,"context_line":"        mac_address \u003d port[\u0027mac_address\u0027]"},{"line_number":73,"context_line":"        interface_name \u003d self.get_internal_device_name(port_id)"},{"line_number":74,"context_line":"        super(DvrEdgeHaRouter, self)._internal_network_added("}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_033c092e","line":71,"updated":"2015-09-02 11:46:46.000000000","message":"fixed_ips is not used in this function.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":68,"context_line":"        # configured with l3"},{"line_number":69,"context_line":"        network_id \u003d port[\u0027network_id\u0027]"},{"line_number":70,"context_line":"        port_id \u003d port[\u0027id\u0027]"},{"line_number":71,"context_line":"        fixed_ips \u003d port[\u0027fixed_ips\u0027]"},{"line_number":72,"context_line":"        mac_address \u003d port[\u0027mac_address\u0027]"},{"line_number":73,"context_line":"        interface_name \u003d self.get_internal_device_name(port_id)"},{"line_number":74,"context_line":"        super(DvrEdgeHaRouter, self)._internal_network_added("}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_5bdf330c","line":71,"in_reply_to":"da20952f_033c092e","updated":"2015-09-09 18:41:47.000000000","message":"removed these lines","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"c9f390b462b40349ee03573985a915667f4c784e","unresolved":false,"context_lines":[{"line_number":71,"context_line":"        fixed_ips \u003d port[\u0027fixed_ips\u0027]"},{"line_number":72,"context_line":"        mac_address \u003d port[\u0027mac_address\u0027]"},{"line_number":73,"context_line":"        interface_name \u003d self.get_internal_device_name(port_id)"},{"line_number":74,"context_line":"        super(DvrEdgeHaRouter, self)._internal_network_added("},{"line_number":75,"context_line":"            self.ns_name, network_id, port_id, fixed_ips, mac_address,"},{"line_number":76,"context_line":"            interface_name, INTERNAL_DEV_PREFIX)"},{"line_number":77,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name,"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_1cc27e85","line":74,"updated":"2015-09-07 08:37:28.000000000","message":"_internal_network_added() is plugging the \u0027qr-\u0027 device. Then, on line 89 we call a function which, again, plugs the \u0027qr-\u0027 device. Is this expected?","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":71,"context_line":"        fixed_ips \u003d port[\u0027fixed_ips\u0027]"},{"line_number":72,"context_line":"        mac_address \u003d port[\u0027mac_address\u0027]"},{"line_number":73,"context_line":"        interface_name \u003d self.get_internal_device_name(port_id)"},{"line_number":74,"context_line":"        super(DvrEdgeHaRouter, self)._internal_network_added("},{"line_number":75,"context_line":"            self.ns_name, network_id, port_id, fixed_ips, mac_address,"},{"line_number":76,"context_line":"            interface_name, INTERNAL_DEV_PREFIX)"},{"line_number":77,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name,"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_9645a244","line":74,"in_reply_to":"da20952f_1cc27e85","updated":"2015-09-09 18:41:47.000000000","message":"if you mean: \n self.add_ha_snat_port_for_internal_port(port)\n\nthat actually plugs qs port, not qr-. It just uses \"port\" to look up the qs-port that needs to be plugged, and rules are added to redirect traffic from qr- to qs- to provide snat services. \ntake a look at L42 - L76","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"b5e94da705ea93794241c5421ea2fbf20481c22c","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        super(DvrEdgeHaRouter, self)._internal_network_added("},{"line_number":75,"context_line":"            self.ns_name, network_id, port_id, fixed_ips, mac_address,"},{"line_number":76,"context_line":"            interface_name, INTERNAL_DEV_PREFIX)"},{"line_number":77,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name,"},{"line_number":78,"context_line":"                                                   self.ns_name)"},{"line_number":79,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":80,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_db602cd2","line":77,"updated":"2015-09-09 13:12:18.000000000","message":"This call here ultimately adds a vip to keepalived for the \u0027qr-\u0027 device. However, the \u0027qr-\u0027 device is not in the \u0027self.ns_name\u0027 (snat-) namespace, but rather in the qrouter- namespace. This results in a bad configuration pushed to keepalived and an error message to /var/log/messages, as can be seen in [1].\n\nThis does not occur in a normal HA router (where all interfaces, including qr-, ARE in the qrouter- namespace), so it\u0027s a regression from this patch and should be fixed here.\n\n[1]: http://pastebin.com/5JY4WLXb","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"22037037ef4f9f0f1574de580ee8fa126cda84c5","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        super(DvrEdgeHaRouter, self)._internal_network_added("},{"line_number":75,"context_line":"            self.ns_name, network_id, port_id, fixed_ips, mac_address,"},{"line_number":76,"context_line":"            interface_name, INTERNAL_DEV_PREFIX)"},{"line_number":77,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name,"},{"line_number":78,"context_line":"                                                   self.ns_name)"},{"line_number":79,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":80,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_69257f08","line":77,"in_reply_to":"da20952f_96be02ad","updated":"2015-09-10 06:38:06.000000000","message":"Call to _disable_ipv6 _addressing_on_interface is not needed for the qr interfaces, so removing it. That solves the problem john pointed out.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        super(DvrEdgeHaRouter, self)._internal_network_added("},{"line_number":75,"context_line":"            self.ns_name, network_id, port_id, fixed_ips, mac_address,"},{"line_number":76,"context_line":"            interface_name, INTERNAL_DEV_PREFIX)"},{"line_number":77,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name,"},{"line_number":78,"context_line":"                                                   self.ns_name)"},{"line_number":79,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":80,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_96be02ad","line":77,"in_reply_to":"da20952f_db602cd2","updated":"2015-09-09 18:41:47.000000000","message":"ns_name IS the qrouter- namespace. \nsnat_ns_name is the snat namespace. Sorry I did not pick the namespace names ns_name is what it is called everywhere else (parent classes).  so if the call here is using ns_name \u003d snat_namespace, the problem is somewhere else. ns_name should have qrouter-namespace. \nwe need to check why we are using the wrong value for ns_name. This file never sets it, it inherits from above.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":95,"context_line":"            self.add_ha_snat_port_for_internal_port(port)"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        self._add_gateway_vip(ex_gw_port, interface_name)"},{"line_number":98,"context_line":"        self._disable_ipv6_addressing_on_interface("},{"line_number":99,"context_line":"            interface_name, self.snat_namespace.name)"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_638e2529","line":98,"updated":"2015-09-02 11:46:46.000000000","message":"As per patchset #29, this should be changed to \u0027self.snat_ns_name\u0027.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":102,"context_line":"        self._clear_vips(interface_name)"},{"line_number":103,"context_line":"        self.process_floating_ip_nat_rules()"},{"line_number":104,"context_line":"        if self.fip_ns:"},{"line_number":105,"context_line":"            to_fip_interface_name \u003d ("},{"line_number":106,"context_line":"                self.get_external_device_interface_name(ex_gw_port))"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_b2b564d1","line":103,"updated":"2015-09-02 11:46:46.000000000","message":"The following lines of this functions can be replaced by the following code:\n\n super(DvrEdgeRouter, self).external_gateway_removed(...)\n\nLines 119-122 (defined in DvrEdgeRouter.external_gateway_removed),\n\nLines 103-111 (defined in DvrLocalRouter.external_gateway_removed, called by DvrEdgeRouter...)\n\nSo basically we can reduce about 20 lines from this function in favor of a simple super() call :)\n\n super(DvrEdgeRouter, self).external_gateway_removed(ex_gw_port, interface_name)","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"336b64db2b67c2eef1704e5b4ff345c075fe069c","unresolved":false,"context_lines":[{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":102,"context_line":"        self._clear_vips(interface_name)"},{"line_number":103,"context_line":"        self.process_floating_ip_nat_rules()"},{"line_number":104,"context_line":"        if self.fip_ns:"},{"line_number":105,"context_line":"            to_fip_interface_name \u003d ("},{"line_number":106,"context_line":"                self.get_external_device_interface_name(ex_gw_port))"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_00adac87","line":103,"in_reply_to":"da20952f_adfdf0b2","updated":"2015-09-10 08:25:47.000000000","message":"So I see your point about lines 119-122, but lines 103-111 definitely has to go in favor of:\n\n super(DvrLocalRouter, self).external_gateway_removed(...)","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":102,"context_line":"        self._clear_vips(interface_name)"},{"line_number":103,"context_line":"        self.process_floating_ip_nat_rules()"},{"line_number":104,"context_line":"        if self.fip_ns:"},{"line_number":105,"context_line":"            to_fip_interface_name \u003d ("},{"line_number":106,"context_line":"                self.get_external_device_interface_name(ex_gw_port))"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_adfdf0b2","line":103,"in_reply_to":"da20952f_b2b564d1","updated":"2015-09-09 18:41:47.000000000","message":"Look at the last lines of  DvrEdgeRouter.external_gateway_removed(...)\n\n        self.snat_namespace.delete()\n        self.snat_namespace \u003d None\n\nthat destroys the snat namespace. \nWe don\u0027t want that. At least my memory tells me we don\u0027t. I\u0027ll have to check where the snat namespace is created. I believe it only happens on router creation.\n\nAlso see below, I had to add certain lines which will make it not possible to use super call.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":114,"context_line":"            LOG.debug(\"not hosting snat for router: %s\", self.router[\u0027id\u0027])"},{"line_number":115,"context_line":"        else:"},{"line_number":116,"context_line":"            snat_ports \u003d self.get_snat_interfaces()"},{"line_number":117,"context_line":"            for port in snat_ports:"},{"line_number":118,"context_line":"                self._dvr_external_network_removed(port)"},{"line_number":119,"context_line":"            self.driver.unplug(interface_name,"},{"line_number":120,"context_line":"                               bridge\u003dself.agent_conf.external_network_bridge,"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_32ce3484","line":117,"updated":"2015-09-02 11:46:46.000000000","message":"We can remove the \u0027snat_ports\u0027 variable by iterating directly on self.get_snat_interfaces()","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":114,"context_line":"            LOG.debug(\"not hosting snat for router: %s\", self.router[\u0027id\u0027])"},{"line_number":115,"context_line":"        else:"},{"line_number":116,"context_line":"            snat_ports \u003d self.get_snat_interfaces()"},{"line_number":117,"context_line":"            for port in snat_ports:"},{"line_number":118,"context_line":"                self._dvr_external_network_removed(port)"},{"line_number":119,"context_line":"            self.driver.unplug(interface_name,"},{"line_number":120,"context_line":"                               bridge\u003dself.agent_conf.external_network_bridge,"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_4db9b4eb","line":117,"in_reply_to":"da20952f_32ce3484","updated":"2015-09-09 18:41:47.000000000","message":"done.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":121,"context_line":"                               namespace\u003dself.snat_namespace.name,"},{"line_number":122,"context_line":"                               prefix\u003drouter.EXTERNAL_DEV_PREFIX)"},{"line_number":123,"context_line":"        for port in self.internal_ports:"},{"line_number":124,"context_line":"            for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":125,"context_line":"                self._remove_vip(ip_cidr)"},{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_f292fcf5","line":124,"updated":"2015-09-02 11:46:46.000000000","message":"I\u0027m not seeing other external_gateway_removed implementation remove their vips (floating ips). This leads me to think that, iirc, removing an external gateway is not allowed if there are still vips attached to it. If that is the case, this for block can be deleted. Am I right?","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":121,"context_line":"                               namespace\u003dself.snat_namespace.name,"},{"line_number":122,"context_line":"                               prefix\u003drouter.EXTERNAL_DEV_PREFIX)"},{"line_number":123,"context_line":"        for port in self.internal_ports:"},{"line_number":124,"context_line":"            for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":125,"context_line":"                self._remove_vip(ip_cidr)"},{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_4ddc5418","line":124,"in_reply_to":"da20952f_f292fcf5","updated":"2015-09-09 18:41:47.000000000","message":"vips !\u003d fips. \nbut I think I get your point. \n_remove_vips simply removes the vip given from the keepalive conf file. the call shoudl be to remove the vip from the qs- interface (self.get_snat_interfaces())","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":125,"context_line":"                self._remove_vip(ip_cidr)"},{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"},{"line_number":128,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_updated("},{"line_number":129,"context_line":"            ex_gw_port, interface_name)"},{"line_number":130,"context_line":"        fixed_ips \u003d self.ex_gw_port[\u0027fixed_ips\u0027]"},{"line_number":131,"context_line":"        for old_gateway_cidr in common_utils.fixed_ip_cidrs(fixed_ips):"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_fd1c0d6c","line":128,"updated":"2015-09-02 11:46:46.000000000","message":"IF my assumption re: deleting gateway when floating ips are still there is correct, then this can also be simplified greatly?","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":125,"context_line":"                self._remove_vip(ip_cidr)"},{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"},{"line_number":128,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_updated("},{"line_number":129,"context_line":"            ex_gw_port, interface_name)"},{"line_number":130,"context_line":"        fixed_ips \u003d self.ex_gw_port[\u0027fixed_ips\u0027]"},{"line_number":131,"context_line":"        for old_gateway_cidr in common_utils.fixed_ip_cidrs(fixed_ips):"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_20b6c7f9","line":128,"in_reply_to":"da20952f_fd1c0d6c","updated":"2015-09-09 18:41:47.000000000","message":"floating ips and snat services are separate in dvr. \nAlso, I think again you are confusing fips with vips. \nvips are ip addresses in vrrp (as in the keepalive process). they refer to the ip address being programmed by the keepalive process. \n\nLook at ha_router.py you will see lots of methods dealing with vips, not fips. \nin the case of ha_router fips ~\u003d vips because they are part of the same process. that is not the case in dvr. in dvr fips are treated completely different and don\u0027t really have naything to do with the ha part of dvrha... in dvr, fips are already ha.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":140,"context_line":"        self.snat_ns_name \u003d self.snat_namespace.name"},{"line_number":141,"context_line":"        super(DvrEdgeHaRouter, self).initialize(process_monitor)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    def process(self, agent):"},{"line_number":144,"context_line":"        ex_gw_port \u003d self.get_ex_gw_port()"},{"line_number":145,"context_line":"        if ex_gw_port:"},{"line_number":146,"context_line":"            self.fip_ns \u003d agent.get_fip_ns(ex_gw_port[\u0027network_id\u0027])"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_ddd4a948","line":143,"updated":"2015-09-02 11:46:46.000000000","message":"This entire function can be removed. This is since if the function is not present, the super() copies is automatically called - in this case lines 144-156 can be invoked by DvrEdgeRouter.process(), and lines 157-158 can be invoked by HaRouter.process() - both are in the inheritance tree and will be invoked automatically.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"22037037ef4f9f0f1574de580ee8fa126cda84c5","unresolved":false,"context_lines":[{"line_number":140,"context_line":"        self.snat_ns_name \u003d self.snat_namespace.name"},{"line_number":141,"context_line":"        super(DvrEdgeHaRouter, self).initialize(process_monitor)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    def process(self, agent):"},{"line_number":144,"context_line":"        ex_gw_port \u003d self.get_ex_gw_port()"},{"line_number":145,"context_line":"        if ex_gw_port:"},{"line_number":146,"context_line":"            self.fip_ns \u003d agent.get_fip_ns(ex_gw_port[\u0027network_id\u0027])"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_d8aed680","line":143,"in_reply_to":"da20952f_ddd4a948","updated":"2015-09-10 06:38:06.000000000","message":"That would be nice, but unfortunately  this is a classic diamond inheritance problem. \nBoth dvredgerouter and harouter make a call to their super in process(). that would cause the  router_info.process() to be called twice. we don\u0027t really want that.\nbut we can make a call to super(DvrEdgeRouter, self).process() and add lines 157 to 158","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":162,"context_line":"        if self.snat_namespace:"},{"line_number":163,"context_line":"            self.snat_namespace.delete()"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":166,"context_line":"        return set([addr[\u0027cidr\u0027] for addr in device.addr.list()])"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    def _external_gateway_added(self, ex_gw_port, interface_name,"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_dd93494e","line":165,"updated":"2015-09-02 11:46:46.000000000","message":"This can be replaced with:\n\n return super(DvrEdgeRouter, self).get_router_cidrs(device)","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"336b64db2b67c2eef1704e5b4ff345c075fe069c","unresolved":false,"context_lines":[{"line_number":162,"context_line":"        if self.snat_namespace:"},{"line_number":163,"context_line":"            self.snat_namespace.delete()"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":166,"context_line":"        return set([addr[\u0027cidr\u0027] for addr in device.addr.list()])"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    def _external_gateway_added(self, ex_gw_port, interface_name,"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_20feb0ae","line":165,"in_reply_to":"da20952f_40edfb8c","updated":"2015-09-10 08:25:47.000000000","message":"You forgot to remove this code :)","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":162,"context_line":"        if self.snat_namespace:"},{"line_number":163,"context_line":"            self.snat_namespace.delete()"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":166,"context_line":"        return set([addr[\u0027cidr\u0027] for addr in device.addr.list()])"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    def _external_gateway_added(self, ex_gw_port, interface_name,"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_40edfb8c","line":165,"in_reply_to":"da20952f_dd93494e","updated":"2015-09-09 18:41:47.000000000","message":"Actually, I think we can take this out all copletely. we inherit from DvrEdge, so we should get it from router_base","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":170,"context_line":"        self._plug_external_gateway(ex_gw_port, interface_name, ns_name)"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def _internal_network_added(self, ns_name, network_id, port_id,"},{"line_number":173,"context_line":"                                fixed_ips, mac_address,"},{"line_number":174,"context_line":"                                interface_name, prefix):"},{"line_number":175,"context_line":"        self.driver.plug(network_id, port_id, interface_name, mac_address,"},{"line_number":176,"context_line":"                         namespace\u003dns_name,"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_a3f0bdca","line":173,"updated":"2015-09-02 11:46:46.000000000","message":"fixed_ips is never used in this function and can be removed as an argument.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"22037037ef4f9f0f1574de580ee8fa126cda84c5","unresolved":false,"context_lines":[{"line_number":170,"context_line":"        self._plug_external_gateway(ex_gw_port, interface_name, ns_name)"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def _internal_network_added(self, ns_name, network_id, port_id,"},{"line_number":173,"context_line":"                                fixed_ips, mac_address,"},{"line_number":174,"context_line":"                                interface_name, prefix):"},{"line_number":175,"context_line":"        self.driver.plug(network_id, port_id, interface_name, mac_address,"},{"line_number":176,"context_line":"                         namespace\u003dns_name,"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_7829ca9a","line":173,"in_reply_to":"da20952f_2314914b","updated":"2015-09-10 06:38:06.000000000","message":"recoded and instead overode _create_dvr_gateway(), which was the original reason for this method. so added _create_dvr_gateway() and remvoed _internal_network_added.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":170,"context_line":"        self._plug_external_gateway(ex_gw_port, interface_name, ns_name)"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def _internal_network_added(self, ns_name, network_id, port_id,"},{"line_number":173,"context_line":"                                fixed_ips, mac_address,"},{"line_number":174,"context_line":"                                interface_name, prefix):"},{"line_number":175,"context_line":"        self.driver.plug(network_id, port_id, interface_name, mac_address,"},{"line_number":176,"context_line":"                         namespace\u003dns_name,"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_2314914b","line":173,"in_reply_to":"da20952f_a3f0bdca","updated":"2015-09-09 18:41:47.000000000","message":"hmm. I am thinking we can remove this function completely. The reason for the fixed_ips was because the call to this method comes from old (ancestor classes) which don\u0027t use key\u003dvalue calls. (router_info.py, all the dvr...router.py, etc...) and they call it with fixed_ips. \n But after looking at this part of the code I am not sure we even need to create this one here. \nwill test and correct accordingly.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"890fcab445916a8d420bc69128bd62a2c20a8e95","unresolved":false,"context_lines":[{"line_number":42,"context_line":""},{"line_number":43,"context_line":"    def add_ha_snat_port_for_internal_port(self, port):"},{"line_number":44,"context_line":"        \"\"\""},{"line_number":45,"context_line":"        Plugs the corresponding qs- interface and maps traffic from"},{"line_number":46,"context_line":"        corresponding qr- interface to the qs- interface to provide"},{"line_number":47,"context_line":"        snat functionality."},{"line_number":48,"context_line":"        :param port:"}],"source_content_type":"text/x-python","patch_set":32,"id":"da20952f_8059bc90","line":45,"updated":"2015-09-10 08:25:18.000000000","message":"What is the \u0027qs-\u0027 device? Did you mean \u0027qg-\u0027?","commit_id":"32b4058b1dd72606e00baad606a2b77d657c16e3"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"bed679187aa56063688392edba525d4d371dd60c","unresolved":false,"context_lines":[{"line_number":42,"context_line":""},{"line_number":43,"context_line":"    def add_ha_snat_port_for_internal_port(self, port):"},{"line_number":44,"context_line":"        \"\"\""},{"line_number":45,"context_line":"        Plugs the corresponding qs- interface and maps traffic from"},{"line_number":46,"context_line":"        corresponding qr- interface to the qs- interface to provide"},{"line_number":47,"context_line":"        snat functionality."},{"line_number":48,"context_line":"        :param port:"}],"source_content_type":"text/x-python","patch_set":32,"id":"da20952f_8030c608","line":45,"in_reply_to":"da20952f_8059bc90","updated":"2015-09-10 18:26:59.000000000","message":"sorry I meant sg- (snat interface that lives in the snat namepsace).","commit_id":"32b4058b1dd72606e00baad606a2b77d657c16e3"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"890fcab445916a8d420bc69128bd62a2c20a8e95","unresolved":false,"context_lines":[{"line_number":67,"context_line":"                         snat_interface_name,"},{"line_number":68,"context_line":"                         sn_port[\u0027mac_address\u0027],"},{"line_number":69,"context_line":"                         namespace\u003dself.snat_ns_name,"},{"line_number":70,"context_line":"                         prefix\u003dINTERNAL_DEV_PREFIX)"},{"line_number":71,"context_line":"        self._disable_ipv6_addressing_on_interface(snat_interface_name,"},{"line_number":72,"context_line":"                                                   self.snat_ns_name)"},{"line_number":73,"context_line":""}],"source_content_type":"text/x-python","patch_set":32,"id":"da20952f_40dba4ed","line":70,"updated":"2015-09-10 08:25:18.000000000","message":"INTERNAL_DEV_PREFIX \u003d \u0027qr-\u0027, so I\u0027m not clear on why this actually plugs anything other than the qr- port. Can you clarify this for me?","commit_id":"32b4058b1dd72606e00baad606a2b77d657c16e3"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"bed679187aa56063688392edba525d4d371dd60c","unresolved":false,"context_lines":[{"line_number":67,"context_line":"                         snat_interface_name,"},{"line_number":68,"context_line":"                         sn_port[\u0027mac_address\u0027],"},{"line_number":69,"context_line":"                         namespace\u003dself.snat_ns_name,"},{"line_number":70,"context_line":"                         prefix\u003dINTERNAL_DEV_PREFIX)"},{"line_number":71,"context_line":"        self._disable_ipv6_addressing_on_interface(snat_interface_name,"},{"line_number":72,"context_line":"                                                   self.snat_ns_name)"},{"line_number":73,"context_line":""}],"source_content_type":"text/x-python","patch_set":32,"id":"da20952f_7473f7b4","line":70,"in_reply_to":"da20952f_40dba4ed","updated":"2015-09-10 18:26:59.000000000","message":"that is a mistake. I should be dvr_snat_ns.SNAT_INT_DEV_PREFIX. \nshould not be rewriting functions so late in the night. sorry.","commit_id":"32b4058b1dd72606e00baad606a2b77d657c16e3"},{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"5fc077e5d16a2875efa58219211aaaaa0873aaf5","unresolved":false,"context_lines":[{"line_number":49,"context_line":"        \"\"\""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"        # \"port\" already has a qr- interface. Here, a corresponding"},{"line_number":52,"context_line":"        # sg- interface is obtain so we cam map traffic which needs to"},{"line_number":53,"context_line":"        # be redirected from qr- to sg- in order to provide snat services"},{"line_number":54,"context_line":"        sn_port \u003d self.get_snat_port_for_internal_port(port)"},{"line_number":55,"context_line":"        if not sn_port:"}],"source_content_type":"text/x-python","patch_set":34,"id":"da20952f_7ea3ceb1","line":52,"updated":"2015-09-10 20:43:04.000000000","message":"nit: typo: \u0027cam map\" to \u0027can map\"","commit_id":"1efbf11944864039040df37f936dbe18d99aa58e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"fda133806242acf939887b067f8826c631634618","unresolved":false,"context_lines":[{"line_number":49,"context_line":"        \"\"\""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"        # \"port\" already has a qr- interface. Here, a corresponding"},{"line_number":52,"context_line":"        # sg- interface is obtain so we cam map traffic which needs to"},{"line_number":53,"context_line":"        # be redirected from qr- to sg- in order to provide snat services"},{"line_number":54,"context_line":"        sn_port \u003d self.get_snat_port_for_internal_port(port)"},{"line_number":55,"context_line":"        if not sn_port:"}],"source_content_type":"text/x-python","patch_set":34,"id":"da20952f_c647a34a","line":52,"in_reply_to":"da20952f_7ea3ceb1","updated":"2015-09-11 05:34:14.000000000","message":"Done","commit_id":"1efbf11944864039040df37f936dbe18d99aa58e"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":107,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_added("},{"line_number":108,"context_line":"            ex_gw_port, interface_name)"},{"line_number":109,"context_line":"        for port in self.internal_ports:"},{"line_number":110,"context_line":"            self.add_ha_snat_port_for_internal_port(port)"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"        self._add_gateway_vip(ex_gw_port, interface_name)"},{"line_number":113,"context_line":"        self._disable_ipv6_addressing_on_interface("}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_95544fc6","line":110,"updated":"2015-09-23 21:58:56.000000000","message":"This call is incorrect.\n\nself._snat_redirect_add and self.driver.plug were already called during super(DvrEdgeHaRouter, self).external_gateway_added(). This method just needs to add the SNAT ports VIPs and disable ipv6 lladdrs.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":188,"context_line":"                                ns_name, preserve_ips):"},{"line_number":189,"context_line":"        self._plug_external_gateway(ex_gw_port, interface_name, ns_name)"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    def _add_internal_network_without_l3(self, ns_name, network_id, port_id,"},{"line_number":192,"context_line":"                                         mac_address,"},{"line_number":193,"context_line":"                                         interface_name, prefix):"},{"line_number":194,"context_line":"        self.driver.plug(network_id, port_id, interface_name, mac_address,"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_df04287f","line":191,"updated":"2015-09-23 21:58:56.000000000","message":"This method is unused.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":199,"context_line":"        return (self.agent_conf.agent_mode"},{"line_number":200,"context_line":"                \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT)"},{"line_number":201,"context_line":""},{"line_number":202,"context_line":"    def is_this_snat_host(self):"},{"line_number":203,"context_line":"        return self._is_this_snat_host()"},{"line_number":204,"context_line":""},{"line_number":205,"context_line":"    def _dvr_internal_network_removed(self, port):"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_44212f4c","line":202,"updated":"2015-09-23 21:58:56.000000000","message":"This method is unused.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":210,"context_line":"        for ip_cidr in common_utils.fixed_ip_cidrs(sn_port[\u0027fixed_ips\u0027]):"},{"line_number":211,"context_line":"            self._remove_vip(ip_cidr)"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":"    def _create_dvr_gateway(self, ex_gw_port, gw_interface_name):"},{"line_number":214,"context_line":"        \"\"\"Create SNAT namespace.\"\"\""},{"line_number":215,"context_line":"        snat_ns \u003d self._create_snat_namespace()"},{"line_number":216,"context_line":"        # connect snat_ports to br_int from SNAT namespace"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_ffe24cd4","line":213,"updated":"2015-09-23 21:58:56.000000000","message":"The only difference between this implementation and DvrEdgeRouter\u0027s, is that in line 220 this method uses plug, while DvrEdgeRouter uses _internal_network_added (Which calls plug then adds an IP address to the device). I think that instead of duplicating the entire method, we can add a method to DvrEdgeRouter that plugs an SNAT interface. In the base class it would call _internal_network_added, and here it would call plug.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":229,"context_line":"        # kicks the FW Agent to add rules for the snat namespace"},{"line_number":230,"context_line":"        self.agent.process_router_add(self)"},{"line_number":231,"context_line":""},{"line_number":232,"context_line":"    def _init_keepalived_manager(self, process_monitor):"},{"line_number":233,"context_line":"        self.keepalived_manager \u003d keepalived.KeepalivedManager("},{"line_number":234,"context_line":"            self.router[\u0027id\u0027],"},{"line_number":235,"context_line":"            keepalived.KeepalivedConf(),"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_6449d330","line":232,"updated":"2015-09-23 21:58:56.000000000","message":"Removed as per comment in line 329.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":258,"context_line":""},{"line_number":259,"context_line":"        config.add_instance(instance)"},{"line_number":260,"context_line":""},{"line_number":261,"context_line":"    def ha_network_added(self):"},{"line_number":262,"context_line":"        interface_name \u003d self.get_ha_device_name()"},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"        self.driver.plug(self.ha_port[\u0027network_id\u0027],"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_a1ffdd64","line":261,"updated":"2015-09-23 21:58:56.000000000","message":"Removed as per comment in line 329.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":272,"context_line":"                            namespace\u003dself.snat_ns_name,"},{"line_number":273,"context_line":"                            preserve_ips\u003d[self._get_primary_vip()])"},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"    def ha_network_removed(self):"},{"line_number":276,"context_line":"        self.driver.unplug(self.get_ha_device_name(),"},{"line_number":277,"context_line":"                           namespace\u003dself.snat_ns_name,"},{"line_number":278,"context_line":"                           prefix\u003dHA_DEV_PREFIX)"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_e1e2b5d4","line":275,"updated":"2015-09-23 21:58:56.000000000","message":"Removed as per comment in line 329.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":278,"context_line":"                           prefix\u003dHA_DEV_PREFIX)"},{"line_number":279,"context_line":"        self.ha_port \u003d None"},{"line_number":280,"context_line":""},{"line_number":281,"context_line":"    def _add_default_gw_virtual_route(self, ex_gw_port, interface_name):"},{"line_number":282,"context_line":"        default_gw_rts \u003d []"},{"line_number":283,"context_line":"        gateway_ips \u003d self._get_external_gw_ips(ex_gw_port)"},{"line_number":284,"context_line":"        for gw_ip in gateway_ips:"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_41cb6153","line":281,"updated":"2015-09-23 21:58:56.000000000","message":"This is just a straight up copy/paste from HaRouter. It can be deleted.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":288,"context_line":"                                  default_gw, gw_ip, interface_name))"},{"line_number":289,"context_line":"        instance.virtual_routes.gateway_routes \u003d default_gw_rts"},{"line_number":290,"context_line":""},{"line_number":291,"context_line":"    def _get_state_change_monitor_process_manager(self):"},{"line_number":292,"context_line":"        return external_process.ProcessManager("},{"line_number":293,"context_line":"            self.agent_conf,"},{"line_number":294,"context_line":"            \u0027%s.monitor\u0027 % self.router_id,"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_01aa4971","line":291,"updated":"2015-09-23 21:58:56.000000000","message":"Same comment as below.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":295,"context_line":"            self.snat_ns_name,"},{"line_number":296,"context_line":"            default_cmd_callback\u003dself._get_state_change_monitor_callback())"},{"line_number":297,"context_line":""},{"line_number":298,"context_line":"    def _get_state_change_monitor_callback(self):"},{"line_number":299,"context_line":"        ha_device \u003d self.get_ha_device_name()"},{"line_number":300,"context_line":"        ha_cidr \u003d self._get_primary_vip()"},{"line_number":301,"context_line":""}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_41a4c1a4","line":298,"updated":"2015-09-23 21:58:56.000000000","message":"Same comment as below.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":315,"context_line":""},{"line_number":316,"context_line":"        return callback"},{"line_number":317,"context_line":""},{"line_number":318,"context_line":"    def update_initial_state(self, callback):"},{"line_number":319,"context_line":"        ha_device \u003d ip_lib.IPDevice("},{"line_number":320,"context_line":"            self.get_ha_device_name(),"},{"line_number":321,"context_line":"            self.snat_ns_name)"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_e1d87555","line":318,"updated":"2015-09-23 21:58:56.000000000","message":"Same comment as below.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":326,"context_line":"        self.ha_state \u003d state"},{"line_number":327,"context_line":"        callback(self.router_id, state)"},{"line_number":328,"context_line":""},{"line_number":329,"context_line":"    def _disable_ipv6_addressing_on_interface(self, interface_name, namespace):"},{"line_number":330,"context_line":"        \"\"\"Disable IPv6 link local addressing on the device and add it as"},{"line_number":331,"context_line":"        a VIP to keepalived. This means that the IPv6 link local address"},{"line_number":332,"context_line":"        will only be present on the master."}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_cb0552c2","line":329,"updated":"2015-09-23 21:58:56.000000000","message":"This is nearly direct copy/paste from HaRouter. The only difference is the addition of the \u0027namespace\u0027 parameter.\n\nWhat we need to do is add a method called something like:\n@property\ndef ha_namespace():\n    return self.ns_name\n\nAnd here you re-implement it with:\n    return self.snat_ns_name\n\nThen change HaRouter to use get_ha_namespace wherever appropriate. Then any method like this one that only differs by the namespace name will just be deleted. This is very similar to what you did in for example PS 20. There you use self.snat_ns_name in HaRouter and overrode the value in this class. What I\u0027m suggesting will have the same effect, just a bit more polished.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":37,"context_line":"        self.enable_snat \u003d None"},{"line_number":38,"context_line":"        self.snat_ports \u003d None"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    def add_ha_snat_port_for_internal_port(self, port):"},{"line_number":41,"context_line":"        \"\"\""},{"line_number":42,"context_line":"        Plugs the corresponding sg- (dvr_snat_ns.SNAT_INT_DEV_PREFI)"},{"line_number":43,"context_line":"        interface and maps traffic from corresponding qr- interface to the sg-"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_1583f5cd","line":40,"updated":"2015-09-24 23:40:08.000000000","message":"It\u0027s possible to simplify this code and internal_network_added below by calling DvrLocal\u0027s version of internal_network_added, plugging the SNAT device and adding the VIP.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"ea2e009d41e94e281b6c34f610da15e61d13fd05","unresolved":false,"context_lines":[{"line_number":37,"context_line":"        self.enable_snat \u003d None"},{"line_number":38,"context_line":"        self.snat_ports \u003d None"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    def add_ha_snat_port_for_internal_port(self, port):"},{"line_number":41,"context_line":"        \"\"\""},{"line_number":42,"context_line":"        Plugs the corresponding sg- (dvr_snat_ns.SNAT_INT_DEV_PREFI)"},{"line_number":43,"context_line":"        interface and maps traffic from corresponding qr- interface to the sg-"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_480554c0","line":40,"in_reply_to":"9a1a9d01_1583f5cd","updated":"2015-09-25 00:12:23.000000000","message":"OK, the way to reuse code is a bit different, but it\u0027s possible.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":98,"context_line":"        self._add_gateway_vip(ex_gw_port, interface_name)"},{"line_number":99,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name)"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":102,"context_line":"        self._clear_vips(interface_name)"},{"line_number":103,"context_line":"        self.process_floating_ip_nat_rules()"},{"line_number":104,"context_line":"        if self.fip_ns:"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_1edd5714","line":101,"updated":"2015-09-24 23:40:08.000000000","message":"This can be replaced by a super calling, then call _clear_vips.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":125,"context_line":"                self._remove_vip(ip_cidr)"},{"line_number":126,"context_line":""},{"line_number":127,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"},{"line_number":128,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_updated("},{"line_number":129,"context_line":"            ex_gw_port, interface_name)"},{"line_number":130,"context_line":"        fixed_ips \u003d self.ex_gw_port[\u0027fixed_ips\u0027]"},{"line_number":131,"context_line":"        for old_gateway_cidr in common_utils.fixed_ip_cidrs(fixed_ips):"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_1e63171c","line":128,"updated":"2015-09-24 23:40:08.000000000","message":"I\u0027m not sure this is correct. This will call external_gateway_updated, which should always call _external_gateway_added in the case of DvrEdgeHaRouter. That calls router_info._external_gateway_added, which is not relevant for HA routers (It adds IP addresses etc). I think that all that\u0027s needed in this case is calling HaRouter.external_gateway_updated.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":132,"context_line":"            self._remove_vip(old_gateway_cidr)"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"        self._add_gateway_vip(ex_gw_port, interface_name)"},{"line_number":135,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name)"},{"line_number":136,"context_line":""},{"line_number":137,"context_line":"    def initialize(self, process_monitor):"},{"line_number":138,"context_line":"        self._create_snat_namespace()"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_3ec13be5","line":135,"updated":"2015-09-24 23:40:08.000000000","message":"I don\u0027t think that calling self._disable_ipv6_addressing_on_interface is needed here? external_gateway_updated is only called after interface_name was created by external_gateway_added before (Which called _disabled_ipv6_addressing).","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":139,"context_line":"        self.snat_ns_name \u003d self.snat_namespace.name"},{"line_number":140,"context_line":"        super(DvrEdgeHaRouter, self).initialize(process_monitor)"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"    def process(self, agent):"},{"line_number":143,"context_line":"        ex_gw_port \u003d self.get_ex_gw_port()"},{"line_number":144,"context_line":"        if ex_gw_port:"},{"line_number":145,"context_line":"            self.fip_ns \u003d agent.get_fip_ns(ex_gw_port[\u0027network_id\u0027])"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_831d94bf","line":142,"updated":"2015-09-24 23:40:08.000000000","message":"This is line for line copy/paste of the \u0027process\u0027 function of DvrLocal.process, then HaRouter.process. We can just call those instead. We\u0027ll never be able to maintain this code if it\u0027s just duplicated here. This can be replaced with:\n\n    def process(self, agent):\n        super(DvrEdgeHaRouter, self).process(agent)\n        if self.ha_port:\n            self.enable_keepalived()","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":182,"context_line":"        if not sn_port:"},{"line_number":183,"context_line":"            return"},{"line_number":184,"context_line":"        for ip_cidr in common_utils.fixed_ip_cidrs(sn_port[\u0027fixed_ips\u0027]):"},{"line_number":185,"context_line":"            self._remove_vip(ip_cidr)"},{"line_number":186,"context_line":""},{"line_number":187,"context_line":"    def _plug_snat_port(self, port):"},{"line_number":188,"context_line":"        \"\"\"Used by _create_dvr_gateway in DvrEdgeRouter.\"\"\""}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_9562a5d7","line":185,"updated":"2015-09-24 23:40:08.000000000","message":"Oops, this doesn\u0027t remove vroutes on the port\u0027s interface.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"50a80ff7f5cc4fd8648eb3f2f3c00ffbd64fe510","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def internal_network_added(self, port):"},{"line_number":39,"context_line":"        # Call RouterInfo\u0027s internal_network_added (Plugs the port, adds IP)"},{"line_number":40,"context_line":"        super(HaRouter, self).internal_network_added(port)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":45,"id":"9a1a9d01_33906843","line":40,"updated":"2015-09-25 23:17:48.000000000","message":"I don\u0027t know how much I like this.  I know that what it accomplishes in this code is what you say in the comment.  But, I\u0027m wary of condoning this pattern.\n\n- it depends on the order of evaluating super classes and the order the super classes are listed on L26.  This could make it fragile.  It doesn\u0027t help that the mro algorithm has changed twice in python [1].\n- \"the order of calls is determined at runtime, because that order adapts to changes in the class hierarchy, and because that order can include sibling classes that are unknown prior to runtime\" [2]  This makes it difficult to foresee what effect changes in the hierarchy in the future may have.\n- At first glance, I bet most reviewers will balk at this.  It is just unexpected and requires significant thought to trust it.\n\n [1] http://python-history.blogspot.com/2010/06/method-resolution-order.html\n [2] https://docs.python.org/2/library/functions.html#super","commit_id":"cf360f7308dcf305854ac351fe2274fe508fc4b5"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"37d1078697ecbc79d44ccfedc64aea797b93d82c","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def internal_network_added(self, port):"},{"line_number":39,"context_line":"        # Call RouterInfo\u0027s internal_network_added (Plugs the port, adds IP)"},{"line_number":40,"context_line":"        super(HaRouter, self).internal_network_added(port)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":45,"id":"9a1a9d01_b6999821","line":40,"in_reply_to":"9a1a9d01_33906843","updated":"2015-09-28 16:07:16.000000000","message":"The use of super was advised in PS 31 review by John Schwarz, do you suggest we should copy-paste the code from parent class instead of calling super?","commit_id":"cf360f7308dcf305854ac351fe2274fe508fc4b5"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3e654dbe304eb40acde563bef23e872bb5f3213a","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def internal_network_added(self, port):"},{"line_number":39,"context_line":"        # Call RouterInfo\u0027s internal_network_added (Plugs the port, adds IP)"},{"line_number":40,"context_line":"        super(HaRouter, self).internal_network_added(port)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":45,"id":"9a1a9d01_f00e213a","line":40,"in_reply_to":"9a1a9d01_b6999821","updated":"2015-09-29 02:24:10.000000000","message":"I doubt that\u0027s what Carl meant. There\u0027s other solutions though, like renaming RouterInfo.internal_network_added, and having both the new Router.internal_network_added and this method call it (That just bypasses the issue).\n\n@Carl: What do you think about what I suggested here? Generally though consider that many other methods both in this class and in other router classes assume the hierarchy ordering. There\u0027s a ton of super calls everywhere.","commit_id":"cf360f7308dcf305854ac351fe2274fe508fc4b5"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"850b72bdebd2281b20885b3b15539a57aad07192","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def internal_network_added(self, port):"},{"line_number":39,"context_line":"        # Call RouterInfo\u0027s internal_network_added (Plugs the port, adds IP)"},{"line_number":40,"context_line":"        super(HaRouter, self).internal_network_added(port)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":45,"id":"9a1a9d01_76277bb6","line":40,"in_reply_to":"9a1a9d01_b6999821","updated":"2015-09-28 17:30:55.000000000","message":"this is six of one, half of the other. If we don\u0027t make this call we have to copy and paste, either way we are getting something and giving something up.","commit_id":"cf360f7308dcf305854ac351fe2274fe508fc4b5"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"4dbeb7c3eaff35dae26bbd158626ce829ae471eb","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def internal_network_added(self, port):"},{"line_number":39,"context_line":"        # Call RouterInfo\u0027s internal_network_added (Plugs the port, adds IP)"},{"line_number":40,"context_line":"        super(HaRouter, self).internal_network_added(port)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":45,"id":"9a1a9d01_f9055845","line":40,"in_reply_to":"9a1a9d01_f00e213a","updated":"2015-09-30 16:55:47.000000000","message":"There are lots of super calls but this one stands out because it passes the name of a base class in order to skip to that class\u0027s base class.  Many code readers will miss the fact that if L26 were different, it would actually skip to a sibling in the hierarchy rather than its base class.\n\nThe others use the much more common pattern of passing the current class as the first argument to super to defer to the next super class in line.  There may be some hidden assumptions elsewhere and I\u0027ll take a quick look to see if there is anything else that concerns me.\n\nIn general, I think use of super should normally stick to a non-conditional call in the method (often at the beginning or end) and should be called (or not called) consistently up and down the class hierarchy.  When the use of super starts getting weird or heavily conditional, the code gets much more difficult to understand, especially with multiple inheritance.","commit_id":"cf360f7308dcf305854ac351fe2274fe508fc4b5"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def internal_network_added(self, port):"},{"line_number":39,"context_line":"        # Call RouterInfo\u0027s internal_network_added (Plugs the port, adds IP)"},{"line_number":40,"context_line":"        self.internal_network_added_base(port)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_b9bda003","line":40,"updated":"2015-09-30 17:55:43.000000000","message":"For the record, I like this better.  It is straight-forward and doesn\u0027t send my brain off thinking about the intricacies of python\u0027s super and mro algorithm.\n\nYou might consider making the method \"private\" by pre-pending _.\n\nI might even be okay with just calling this directly here similar to L83:\n\n  RouterInfo.internal_network_added(self, port)","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f57bd04956c692096b53a023ecaa1b7067da549a","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def internal_network_added(self, port):"},{"line_number":39,"context_line":"        # Call RouterInfo\u0027s internal_network_added (Plugs the port, adds IP)"},{"line_number":40,"context_line":"        self.internal_network_added_base(port)"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_ffa0bcf5","line":40,"in_reply_to":"9a1a9d01_b9bda003","updated":"2015-10-01 13:01:35.000000000","message":"Done","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":41,"context_line":""},{"line_number":42,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"},{"line_number":44,"context_line":"        self._snat_redirect_add_from_port(port)"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"        if not self.get_ex_gw_port() or not self._is_this_snat_host():"},{"line_number":47,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_cbc8b957","line":44,"updated":"2015-09-30 17:55:43.000000000","message":"If we could break the DvrLocalRouter class away from this hierarchy, I don\u0027t think we\u0027d need to worry about calling this.  I wonder if this whole thing would be a bit simpler.\n\nThis redirect only needs to be added to dvr qrouter namespaces which should only be handled by the DvrLocalRouter class.  The fact that it is mixed up in this hierarchy makes this code more complex than it needs to be I think.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"82349f47633434d8b4642a9467b46ec7f21a206c","unresolved":false,"context_lines":[{"line_number":41,"context_line":""},{"line_number":42,"context_line":"        for subnet in port[\u0027subnets\u0027]:"},{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"},{"line_number":44,"context_line":"        self._snat_redirect_add_from_port(port)"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"        if not self.get_ex_gw_port() or not self._is_this_snat_host():"},{"line_number":47,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":46,"id":"7a2fa921_d7524c13","line":44,"in_reply_to":"9a1a9d01_cbc8b957","updated":"2015-10-06 14:49:47.000000000","message":"I took a crack at this and it was very intrusive.  I\u0027m going to have to think about it and take another crack later.  Don\u0027t hold this patch set up for this.  Sorry if I held it up too long.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":16934,"name":"Martin Roy","email":"lindycoder@gmail.com","username":"lindycoder"},"change_message_id":"dba3c4a91999fc8cb6a9ca77b1341249aa1c2553","unresolved":false,"context_lines":[{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"},{"line_number":44,"context_line":"        self._snat_redirect_add_from_port(port)"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"        if not self.get_ex_gw_port() or not self._is_this_snat_host():"},{"line_number":47,"context_line":"            return"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"        sn_port \u003d self.get_snat_port_for_internal_port(port)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_c7451c43","line":46,"updated":"2015-09-30 13:53:50.000000000","message":"that method name doesn\u0027t have to contain the word \"this\" it is already called on self, so self._is_snat_host should be good","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":43,"context_line":"            self._set_subnet_arp_info(subnet[\u0027id\u0027])"},{"line_number":44,"context_line":"        self._snat_redirect_add_from_port(port)"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"        if not self.get_ex_gw_port() or not self._is_this_snat_host():"},{"line_number":47,"context_line":"            return"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"        sn_port \u003d self.get_snat_port_for_internal_port(port)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_03069f51","line":46,"in_reply_to":"9a1a9d01_c7451c43","updated":"2015-09-30 17:55:43.000000000","message":"This method existed under its current name before this patch set.  It would be inappropriate for this patch set to rename it.  It should be consistent with the one defined in dvr_edge_router.py.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":58,"context_line":"    def external_gateway_added(self, ex_gw_port, interface_name):"},{"line_number":59,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_added("},{"line_number":60,"context_line":"            ex_gw_port, interface_name)"},{"line_number":61,"context_line":"        for port in self.get_snat_interfaces():"},{"line_number":62,"context_line":"            snat_interface_name \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":63,"context_line":"            self._disable_ipv6_addressing_on_interface(snat_interface_name)"},{"line_number":64,"context_line":"            self._add_vips("}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_cfec46bd","line":61,"updated":"2015-09-30 17:55:43.000000000","message":"These are internal ports.  Seems strange to set them up in external_gateway_added instead of internal_network_added.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"21c88928b3e1728abcdadf8e6bb3c14e230d968d","unresolved":false,"context_lines":[{"line_number":58,"context_line":"    def external_gateway_added(self, ex_gw_port, interface_name):"},{"line_number":59,"context_line":"        super(DvrEdgeHaRouter, self).external_gateway_added("},{"line_number":60,"context_line":"            ex_gw_port, interface_name)"},{"line_number":61,"context_line":"        for port in self.get_snat_interfaces():"},{"line_number":62,"context_line":"            snat_interface_name \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":63,"context_line":"            self._disable_ipv6_addressing_on_interface(snat_interface_name)"},{"line_number":64,"context_line":"            self._add_vips("}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_7492aaf7","line":61,"in_reply_to":"9a1a9d01_cfec46bd","updated":"2015-09-30 19:24:24.000000000","message":"snat ports are only used when an external network is in place or added. I belive , the snat ports have to be set here in case the internal network is already there and then the external network is added afterwards.  I don\u0027t believe internal_network_added gets called in that scenario.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":69,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name)"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":72,"context_line":"        for port in self.snat_ports:"},{"line_number":73,"context_line":"            snat_interface \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":74,"context_line":"            self.driver.unplug(snat_interface,"},{"line_number":75,"context_line":"                               namespace\u003dself.ha_namespace,"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_af48f2cc","line":72,"updated":"2015-09-30 17:55:43.000000000","message":"ditto (L61)","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"21c88928b3e1728abcdadf8e6bb3c14e230d968d","unresolved":false,"context_lines":[{"line_number":69,"context_line":"        self._disable_ipv6_addressing_on_interface(interface_name)"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":72,"context_line":"        for port in self.snat_ports:"},{"line_number":73,"context_line":"            snat_interface \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":74,"context_line":"            self.driver.unplug(snat_interface,"},{"line_number":75,"context_line":"                               namespace\u003dself.ha_namespace,"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_5f53ff16","line":72,"in_reply_to":"9a1a9d01_af48f2cc","updated":"2015-09-30 19:24:24.000000000","message":"just as before, in some scenarios, weather or not snat ports need to be \"plummed\",  depends on external gateway addition or deletion.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":80,"context_line":"        self._clear_vips(interface_name)"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"},{"line_number":83,"context_line":"        HaRouter.external_gateway_updated(self, ex_gw_port, interface_name)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    def initialize(self, process_monitor):"},{"line_number":86,"context_line":"        self._create_snat_namespace()"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_af76b203","line":83,"updated":"2015-09-30 17:55:43.000000000","message":"I notice here a direct call to a specific class in the hierarchy.  At least this is straight-forward.  I might be okay with this.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"21c88928b3e1728abcdadf8e6bb3c14e230d968d","unresolved":false,"context_lines":[{"line_number":80,"context_line":"        self._clear_vips(interface_name)"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"    def external_gateway_updated(self, ex_gw_port, interface_name):"},{"line_number":83,"context_line":"        HaRouter.external_gateway_updated(self, ex_gw_port, interface_name)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    def initialize(self, process_monitor):"},{"line_number":86,"context_line":"        self._create_snat_namespace()"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_3f9e4bb9","line":83,"in_reply_to":"9a1a9d01_af76b203","updated":"2015-09-30 19:24:24.000000000","message":"agreed. it looks cleaner anyways, and its more obvious what is being done.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":106,"context_line":""},{"line_number":107,"context_line":"    def _is_this_snat_host(self):"},{"line_number":108,"context_line":"        return (self.agent_conf.agent_mode"},{"line_number":109,"context_line":"                \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    def _dvr_internal_network_removed(self, port):"},{"line_number":112,"context_line":"        super(DvrEdgeHaRouter, self)._dvr_internal_network_removed(port)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_0f247ec2","line":109,"updated":"2015-09-30 17:55:43.000000000","message":"I\u0027m not sure this is the proper way to do this.  The agent_mode has always been a bit funny but this could be a dvr_snat agent but still not be the snat host for this router.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"21c88928b3e1728abcdadf8e6bb3c14e230d968d","unresolved":false,"context_lines":[{"line_number":106,"context_line":""},{"line_number":107,"context_line":"    def _is_this_snat_host(self):"},{"line_number":108,"context_line":"        return (self.agent_conf.agent_mode"},{"line_number":109,"context_line":"                \u003d\u003d l3_constants.L3_AGENT_MODE_DVR_SNAT)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    def _dvr_internal_network_removed(self, port):"},{"line_number":112,"context_line":"        super(DvrEdgeHaRouter, self)._dvr_internal_network_removed(port)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_f480faf8","line":109,"in_reply_to":"9a1a9d01_0f247ec2","updated":"2015-09-30 19:24:24.000000000","message":"You hit the problem on the nose. if this host is a \"backup\" for the HA it will not be listed as the  snat host for the router. but it still has to do all the setup, so we have to evaluate this as true.\nAll dvr_snat hosts will play either master or backup to the router.  The only difference is that the master agent, will also be the host listed as the snat_host in the database ( and router info sent from server to agent). Even then who becomes active is up to the keepalived process so its not even sure that the host listed is the actual \"active\" host.\nThe scheduler is the one that makes the decision as to what agents \"instantiates\" a router and who does not. So, If this code is reached it means the router has been instantiated on this agent and this function should evaluate to true.\nIt would be nicer to have all the agents, master and backups, listed in the \"snat_host\" field of the info sent to agent, but that would require modifying  the database and server.  Then the agent could check the name of the host against that list of hosts.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":116,"context_line":"        self._clear_vips(self._get_snat_int_device_name(sn_port[\u0027id\u0027]))"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _plug_snat_port(self, port):"},{"line_number":119,"context_line":"        \"\"\"Used by _create_dvr_gateway in DvrEdgeRouter.\"\"\""},{"line_number":120,"context_line":"        interface_name \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":121,"context_line":"        self.driver.plug(port[\u0027network_id\u0027], port[\u0027id\u0027],"},{"line_number":122,"context_line":"                         interface_name, port[\u0027mac_address\u0027],"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_cb38b9d5","line":119,"updated":"2015-09-30 17:55:43.000000000","message":"It isn\u0027t clear to me why this is overridden here.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f57bd04956c692096b53a023ecaa1b7067da549a","unresolved":false,"context_lines":[{"line_number":116,"context_line":"        self._clear_vips(self._get_snat_int_device_name(sn_port[\u0027id\u0027]))"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _plug_snat_port(self, port):"},{"line_number":119,"context_line":"        \"\"\"Used by _create_dvr_gateway in DvrEdgeRouter.\"\"\""},{"line_number":120,"context_line":"        interface_name \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":121,"context_line":"        self.driver.plug(port[\u0027network_id\u0027], port[\u0027id\u0027],"},{"line_number":122,"context_line":"                         interface_name, port[\u0027mac_address\u0027],"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_3ab3a24e","line":119,"in_reply_to":"9a1a9d01_cb38b9d5","updated":"2015-10-01 13:01:35.000000000","message":"I\u0027m not sure either, it looks like the only diff is not executing the driver.init_router_port and ip_lib.send_ip_addr_adv_notif from RouterInfo._internal_network_added...","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    @property"},{"line_number":127,"context_line":"    def ha_namespace(self):"},{"line_number":128,"context_line":"        return self.snat_ns_name"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_2fe702ad","line":128,"updated":"2015-09-30 17:55:43.000000000","message":"nit:  I think I\u0027d just write this like the following and skip the extra variable (self.snat_ns_name):\n\n  if self.snat_namespace:\n      return self.snat_namespace.name","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f57bd04956c692096b53a023ecaa1b7067da549a","unresolved":false,"context_lines":[{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    @property"},{"line_number":127,"context_line":"    def ha_namespace(self):"},{"line_number":128,"context_line":"        return self.snat_ns_name"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_5f70c80a","line":128,"in_reply_to":"9a1a9d01_2fe702ad","updated":"2015-10-01 13:01:35.000000000","message":"Done","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"1995d47a5f1a6a5c6b299b7db679e6319ba13626","unresolved":false,"context_lines":[{"line_number":122,"context_line":"                         namespace\u003dself.snat_namespace.name,"},{"line_number":123,"context_line":"                         prefix\u003ddvr_snat_ns.SNAT_INT_DEV_PREFIX)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"    @property"},{"line_number":126,"context_line":"    def ha_namespace(self):"},{"line_number":127,"context_line":"        if self.snat_namespace:"},{"line_number":128,"context_line":"            return self.snat_namespace.name"}],"source_content_type":"text/x-python","patch_set":49,"id":"7a2fa921_359f0305","line":125,"updated":"2015-10-06 12:18:56.000000000","message":"nit: can this function be moved further up, to be after the __init__ and before the other functions (to match the comment on ha_router.py)?","commit_id":"58df349a2cf2bea2ba84527e014288536d151135"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"ddcc5493a95b55fa17d020f7f41a6663afc837e9","unresolved":false,"context_lines":[{"line_number":122,"context_line":"                         namespace\u003dself.snat_namespace.name,"},{"line_number":123,"context_line":"                         prefix\u003ddvr_snat_ns.SNAT_INT_DEV_PREFIX)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"    @property"},{"line_number":126,"context_line":"    def ha_namespace(self):"},{"line_number":127,"context_line":"        if self.snat_namespace:"},{"line_number":128,"context_line":"            return self.snat_namespace.name"}],"source_content_type":"text/x-python","patch_set":49,"id":"7a2fa921_5d941d8a","line":125,"in_reply_to":"7a2fa921_359f0305","updated":"2015-10-06 15:27:02.000000000","message":"Done","commit_id":"58df349a2cf2bea2ba84527e014288536d151135"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"1995d47a5f1a6a5c6b299b7db679e6319ba13626","unresolved":false,"context_lines":[{"line_number":124,"context_line":""},{"line_number":125,"context_line":"    @property"},{"line_number":126,"context_line":"    def ha_namespace(self):"},{"line_number":127,"context_line":"        if self.snat_namespace:"},{"line_number":128,"context_line":"            return self.snat_namespace.name"}],"source_content_type":"text/x-python","patch_set":49,"id":"7a2fa921_9565b746","line":127,"updated":"2015-10-06 12:18:56.000000000","message":"Something here seems off to me. If self.snat_namespace is None (or empty or whatever), then None is returned.\n\n1. Shouldn\u0027t self.snat_namespace always point to something non-empty? In this case, the \u0027if\u0027 block can be removed.\n\n2. If not, and if self.snat_namespace is empty, then HaRouter.ha_namespace should be returned - that is, an \u0027else\u0027 block should be put in.","commit_id":"58df349a2cf2bea2ba84527e014288536d151135"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"ddcc5493a95b55fa17d020f7f41a6663afc837e9","unresolved":false,"context_lines":[{"line_number":124,"context_line":""},{"line_number":125,"context_line":"    @property"},{"line_number":126,"context_line":"    def ha_namespace(self):"},{"line_number":127,"context_line":"        if self.snat_namespace:"},{"line_number":128,"context_line":"            return self.snat_namespace.name"}],"source_content_type":"text/x-python","patch_set":49,"id":"7a2fa921_5dc2dd86","line":127,"in_reply_to":"7a2fa921_9565b746","updated":"2015-10-06 15:27:02.000000000","message":"Looks like from other code that snat_namespace is *almost* always checked if it\u0027s not None. I\u0027ve therefor opted for the second option, and this will now explicitly return (calling super() might return the wrong namespace, which can be argued to be worse than having the code crash).","commit_id":"58df349a2cf2bea2ba84527e014288536d151135"}],"neutron/agent/l3/dvr_edge_router.py":[{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":182,"context_line":"        super(DvrEdgeRouter, self).update_routing_table(operation, route,"},{"line_number":183,"context_line":"                                                        namespace\u003dns_name)"},{"line_number":184,"context_line":""},{"line_number":185,"context_line":"    def _dvr_external_network_removed(self, port):"},{"line_number":186,"context_line":"            snat_interface \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":187,"context_line":"            ns_name \u003d self.snat_namespace.name"},{"line_number":188,"context_line":"            prefix \u003d dvr_snat_ns.SNAT_INT_DEV_PREFIX"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_6dbb7476","line":185,"updated":"2015-09-02 11:46:46.000000000","message":"This function is not used at all by this class. Why not move it to dvr_edge_ha_router instead?","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":182,"context_line":"        super(DvrEdgeRouter, self).update_routing_table(operation, route,"},{"line_number":183,"context_line":"                                                        namespace\u003dns_name)"},{"line_number":184,"context_line":""},{"line_number":185,"context_line":"    def _dvr_external_network_removed(self, port):"},{"line_number":186,"context_line":"            snat_interface \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":187,"context_line":"            ns_name \u003d self.snat_namespace.name"},{"line_number":188,"context_line":"            prefix \u003d dvr_snat_ns.SNAT_INT_DEV_PREFIX"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_9ee61443","line":185,"in_reply_to":"da20952f_6dbb7476","updated":"2015-09-09 18:41:47.000000000","message":"because this method is relevant to dvredgerouters not just dvrha routers. I could move it to dvr_ha_router but that just means in the future it will have to be moved here when somene else somewhere else needs the functionaltiy. Easier to start in the right place than to refactor later. Remmeber, this was your code that fixes the problem with external networks being removed. That problem is particular to everything not just dvrha.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"336b64db2b67c2eef1704e5b4ff345c075fe069c","unresolved":false,"context_lines":[{"line_number":182,"context_line":"        super(DvrEdgeRouter, self).update_routing_table(operation, route,"},{"line_number":183,"context_line":"                                                        namespace\u003dns_name)"},{"line_number":184,"context_line":""},{"line_number":185,"context_line":"    def _dvr_external_network_removed(self, port):"},{"line_number":186,"context_line":"            snat_interface \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":187,"context_line":"            ns_name \u003d self.snat_namespace.name"},{"line_number":188,"context_line":"            prefix \u003d dvr_snat_ns.SNAT_INT_DEV_PREFIX"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_e09d18bc","line":185,"in_reply_to":"da20952f_9ee61443","updated":"2015-09-10 08:25:47.000000000","message":"Actually the issue with the \u0027sg-\u0027 device not being cleaned up is specific to DVR+HA routers (your patches), and does not occur in DVR/HA/Legacy routers.\n\nI agree that one day someone might need to use this function, but currently we can\u0027t even see who will want to use this and why, so I still think that it should be moved to dvr_edge_ha_router instead. In other words, it *might* be right in the future but it\u0027s not right at this time.\n\nLook at it this way: one day, if someone will need to use this function in dvr_edge_router, he\u0027ll need to write a patch that calls the function anyway - so he might as well make the move in the same patch.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":180,"context_line":"        super(DvrEdgeRouter, self).update_routing_table(operation, route,"},{"line_number":181,"context_line":"                                                        namespace\u003dns_name)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"    def _dvr_external_network_removed(self, port):"},{"line_number":184,"context_line":"            snat_interface \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":185,"context_line":"            ns_name \u003d self.snat_namespace.name"},{"line_number":186,"context_line":"            prefix \u003d dvr_snat_ns.SNAT_INT_DEV_PREFIX"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_a731385b","line":183,"updated":"2015-09-23 21:58:56.000000000","message":"Nit: The contents of the function is indented one times too many.\n\nI\u0027ll have to examine why was this function added here.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":183,"context_line":"        super(DvrEdgeRouter, self).update_routing_table(operation, route,"},{"line_number":184,"context_line":"                                                        namespace\u003dns_name)"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def _dvr_external_network_removed(self, port):"},{"line_number":187,"context_line":"        snat_interface \u003d self._get_snat_int_device_name(port[\u0027id\u0027])"},{"line_number":188,"context_line":"        ns_name \u003d self.snat_namespace.name"},{"line_number":189,"context_line":"        prefix \u003d dvr_snat_ns.SNAT_INT_DEV_PREFIX"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_92207bf9","line":186,"updated":"2015-09-24 23:40:08.000000000","message":"I\u0027m removing this method following the changes I made to external_gateway_removed in DvrEdgeHaRouter. By doing so I\u0027ll bring DvrEdgeHaRouter and DvrEdgeRouter to the same level. I believe DvrEdgeRouter has a bug (On master), where if you:\n\n  1) create a DVR router\n  2) add an internal interface\n  3) Set router as gateway (SNAT namespace and devices will be created)\n  4) Clear router gateway (SNAT namespace will be deleted,\n     thus deleting the SNAT devices from the IP stack,\n     BUT unplug was never called, so the devices remain in the OVS DB).","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":16934,"name":"Martin Roy","email":"lindycoder@gmail.com","username":"lindycoder"},"change_message_id":"dba3c4a91999fc8cb6a9ca77b1341249aa1c2553","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":74,"context_line":"        self._external_gateway_removed(ex_gw_port, interface_name)"},{"line_number":75,"context_line":"        if self.snat_namespace:"},{"line_number":76,"context_line":"            self.snat_namespace.delete()"},{"line_number":77,"context_line":"            self.snat_namespace \u003d None"},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_673a109b","line":75,"updated":"2015-09-30 13:53:50.000000000","message":"Seems like this code could only be added at the end of the original external_gateway_removed method,\n\nmaybe make a little private function called _clean_snat_namespace and call it at the end instead of bridging","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f57bd04956c692096b53a023ecaa1b7067da549a","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":"    def external_gateway_removed(self, ex_gw_port, interface_name):"},{"line_number":74,"context_line":"        self._external_gateway_removed(ex_gw_port, interface_name)"},{"line_number":75,"context_line":"        if self.snat_namespace:"},{"line_number":76,"context_line":"            self.snat_namespace.delete()"},{"line_number":77,"context_line":"            self.snat_namespace \u003d None"},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_dd4b7b5e","line":75,"in_reply_to":"9a1a9d01_673a109b","updated":"2015-10-01 13:01:35.000000000","message":"The _external_gateway_removed method is needed in DvrEdgeHaRouter.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"}],"neutron/agent/l3/dvr_router_base.py":[{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        else:"},{"line_number":42,"context_line":"            LOG.error(_LE(\u0027DVR: no map match_port found!\u0027))"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":45,"context_line":"        return super(DvrRouterBase, self).get_router_cidrs(device)"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_39e34d35","line":44,"updated":"2015-08-04 19:13:41.000000000","message":"Why is this method needed?","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        else:"},{"line_number":42,"context_line":"            LOG.error(_LE(\u0027DVR: no map match_port found!\u0027))"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":45,"context_line":"        return super(DvrRouterBase, self).get_router_cidrs(device)"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_299bba73","line":44,"in_reply_to":"1a4dcd0f_39e34d35","updated":"2015-08-06 19:36:01.000000000","message":"coding for \"coprative multiple inheritnace\"\n\nDvrEdgeHaRouter inherits from both DvrEdgeRouter and HaRouter: \nclass DvrEdgeHaRouter(DvrEdgeRouter, HaRouter):\n\nThe way MRO  works, If this method is not here then, the class incorrectly inherits get_router_cidrs from HaRouter instead of from RouterInfo (which is what is needed).","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"b72ea3f0f1374a2c6e37a944d5d46ea6625069b0","unresolved":false,"context_lines":[{"line_number":42,"context_line":"            LOG.error(_LE(\u0027DVR: no map match_port found!\u0027))"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":45,"context_line":"        return super(DvrRouterBase, self).get_router_cidrs(device)"}],"source_content_type":"text/x-python","patch_set":27,"id":"da20952f_60f27f71","line":45,"updated":"2015-08-27 19:24:02.000000000","message":"Why is this necessary?  Don\u0027t inheritance rules take care of this?","commit_id":"b9dfa6fd85c06db9dcd9911cf23272e7ab43296e"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"060cdeb6de3c255713fe883608f0a6a4880a36cb","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        else:"},{"line_number":42,"context_line":"            LOG.error(_LE(\u0027DVR: no map match_port found!\u0027))"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":45,"context_line":"        return super(DvrRouterBase, self).get_router_cidrs(device)"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_0f9acc63","line":44,"updated":"2015-08-27 21:29:38.000000000","message":"Trying to figure out why this is needed.  I know that multiple inheritance gets funny with calls to super, but I still don\u0027t see the scenario that requires this since this method inherits from only one class.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        else:"},{"line_number":42,"context_line":"            LOG.error(_LE(\u0027DVR: no map match_port found!\u0027))"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":45,"context_line":"        return super(DvrRouterBase, self).get_router_cidrs(device)"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_28b3850b","line":44,"in_reply_to":"da20952f_0f9acc63","updated":"2015-08-31 19:15:25.000000000","message":"yes this method does. But its descendants  might not.  dvr_edge_ha router does not:)  That one inherits from this  one (dvr_router_base) and  ha_router. \nSo If this is not here, the inheritance \"jumps\" to ha_router  instead of going to router_info which is what is intended, and probably what any future descendants of dvr_router_base would want.\nThe option is to copy and paste get_router_cidrs to dvr_edge_ha class and I think that would be trouble in the future. \nAs a matter of fact with all the refactoring that has gone on, this class should make a super call to all the functions  from router_info it expects any of its descendants to use, but I think that is better left to be added on as needed basis.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":52,"context_line":"                                  \u0027snat_list\u0027: snat_ports,"},{"line_number":53,"context_line":"                                  \u0027int_p\u0027: int_port})"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":56,"context_line":"        return super(DvrRouterBase, self).get_router_cidrs(device)"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_abf49ef0","line":55,"updated":"2015-09-23 21:58:56.000000000","message":"This is not needed.\n\nYou can remove this, then from DvrEdgeHaRouter.get_router_cidrs call:\nreturn DvrEdgeRouter.get_router_cidrs(self, device).\n\nThat will explicitly select DvrEdgeRouter\u0027s implementation of get_router_cidrs and won\u0027t use the MRO to travel all the way to HaRouter\u0027s implementation.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":16934,"name":"Martin Roy","email":"lindycoder@gmail.com","username":"lindycoder"},"change_message_id":"dba3c4a91999fc8cb6a9ca77b1341249aa1c2553","unresolved":false,"context_lines":[{"line_number":52,"context_line":"                                  \u0027snat_list\u0027: snat_ports,"},{"line_number":53,"context_line":"                                  \u0027int_p\u0027: int_port})"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":56,"context_line":"        return super(DvrRouterBase, self).get_router_cidrs(device)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_47110c03","line":55,"updated":"2015-09-30 13:53:50.000000000","message":"this override seems useless","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f57bd04956c692096b53a023ecaa1b7067da549a","unresolved":false,"context_lines":[{"line_number":52,"context_line":"                                  \u0027snat_list\u0027: snat_ports,"},{"line_number":53,"context_line":"                                  \u0027int_p\u0027: int_port})"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":56,"context_line":"        return super(DvrRouterBase, self).get_router_cidrs(device)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_900defe9","line":55,"in_reply_to":"9a1a9d01_3f2e4bfb","updated":"2015-10-01 13:01:35.000000000","message":"If I\u0027m seeing right, the get_router_cidrs is already copied from RouterInfo to DvrEdgeHaRouter... we can drop this override and use RouterInfo.get_router_cidrs directly like we did for internal_network_added...\nI have removed it in PS48.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":52,"context_line":"                                  \u0027snat_list\u0027: snat_ports,"},{"line_number":53,"context_line":"                                  \u0027int_p\u0027: int_port})"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":56,"context_line":"        return super(DvrRouterBase, self).get_router_cidrs(device)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_1e711276","line":55,"in_reply_to":"9a1a9d01_47110c03","updated":"2015-09-30 17:55:43.000000000","message":"+1.  Good point.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"21c88928b3e1728abcdadf8e6bb3c14e230d968d","unresolved":false,"context_lines":[{"line_number":52,"context_line":"                                  \u0027snat_list\u0027: snat_ports,"},{"line_number":53,"context_line":"                                  \u0027int_p\u0027: int_port})"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    def get_router_cidrs(self, device):"},{"line_number":56,"context_line":"        return super(DvrRouterBase, self).get_router_cidrs(device)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_3f2e4bfb","line":55,"in_reply_to":"9a1a9d01_47110c03","updated":"2015-09-30 19:24:24.000000000","message":"actually if this overide is not here, the get_rouer_cidr from ha_router(ancestor class) gets called instead of the one from router_info(ancestor class), when calling from dvr_edge_ha (child class)\nSo it has to be here or we call the wrong one.\ndvr_router_base should expose any of the calls from router_info which are needed by its children. Another option is to copy7 and paste from router_info to router_edge_ha_router.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"}],"neutron/agent/l3/ha.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":138,"context_line":"            if not ri.is_v6_gateway_set(gateway_ips):"},{"line_number":139,"context_line":"                interface_name \u003d ri.get_external_device_name(ex_gw_port_id)"},{"line_number":140,"context_line":"                if ri.router.get(\u0027distributed\u0027, False):"},{"line_number":141,"context_line":"                    namespace \u003d ri.snat_ns_name"},{"line_number":142,"context_line":"                else:"},{"line_number":143,"context_line":"                    namespace \u003d ri.ns_name"},{"line_number":144,"context_line":"                ri.driver.configure_ipv6_ra(namespace, interface_name)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_0b37914d","line":141,"updated":"2015-09-30 17:55:43.000000000","message":"Yikes!  This leaks details a bit.  Let me think about this a bit.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"21c88928b3e1728abcdadf8e6bb3c14e230d968d","unresolved":false,"context_lines":[{"line_number":138,"context_line":"            if not ri.is_v6_gateway_set(gateway_ips):"},{"line_number":139,"context_line":"                interface_name \u003d ri.get_external_device_name(ex_gw_port_id)"},{"line_number":140,"context_line":"                if ri.router.get(\u0027distributed\u0027, False):"},{"line_number":141,"context_line":"                    namespace \u003d ri.snat_ns_name"},{"line_number":142,"context_line":"                else:"},{"line_number":143,"context_line":"                    namespace \u003d ri.ns_name"},{"line_number":144,"context_line":"                ri.driver.configure_ipv6_ra(namespace, interface_name)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_9f00b703","line":141,"in_reply_to":"9a1a9d01_0b37914d","updated":"2015-09-30 19:24:24.000000000","message":"yes. unfortunately the was the code for L3NATAgent is laid out its a bit difficult to introduce this change somewhere else without having to write lots of more code. But if you can figure out a better way that would be great.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f57bd04956c692096b53a023ecaa1b7067da549a","unresolved":false,"context_lines":[{"line_number":138,"context_line":"            if not ri.is_v6_gateway_set(gateway_ips):"},{"line_number":139,"context_line":"                interface_name \u003d ri.get_external_device_name(ex_gw_port_id)"},{"line_number":140,"context_line":"                if ri.router.get(\u0027distributed\u0027, False):"},{"line_number":141,"context_line":"                    namespace \u003d ri.snat_ns_name"},{"line_number":142,"context_line":"                else:"},{"line_number":143,"context_line":"                    namespace \u003d ri.ns_name"},{"line_number":144,"context_line":"                ri.driver.configure_ipv6_ra(namespace, interface_name)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_5acd3185","line":141,"in_reply_to":"9a1a9d01_9f00b703","updated":"2015-10-01 13:01:35.000000000","message":"Since I have removed the snat_ns_name, I have replaced it with ha_namespace (returns the same as snat_ns_name was previously)","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"}],"neutron/agent/l3/ha_router.py":[{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"e9f9c7bfbc8de2d254b71b5c89b4b672d4aabe1b","unresolved":false,"context_lines":[{"line_number":95,"context_line":"            keepalived.KeepalivedConf(),"},{"line_number":96,"context_line":"            process_monitor,"},{"line_number":97,"context_line":"            conf_path\u003dself.agent_conf.ha_confs_path,"},{"line_number":98,"context_line":"            namespace\u003dself.snat_ns_name)"},{"line_number":99,"context_line":"        config \u003d self.keepalived_manager.config"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"        interface_name \u003d self.get_ha_device_name()"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_acb45af0","line":98,"updated":"2015-07-09 22:10:45.000000000","message":"unrelated change. - blank line","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"e234fb1f950bfcec2a24963542c0ab6159b905bc","unresolved":false,"context_lines":[{"line_number":95,"context_line":"            keepalived.KeepalivedConf(),"},{"line_number":96,"context_line":"            process_monitor,"},{"line_number":97,"context_line":"            conf_path\u003dself.agent_conf.ha_confs_path,"},{"line_number":98,"context_line":"            namespace\u003dself.snat_ns_name)"},{"line_number":99,"context_line":"        config \u003d self.keepalived_manager.config"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"        interface_name \u003d self.get_ha_device_name()"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_0602fe09","line":98,"in_reply_to":"ba3cc151_acb45af0","updated":"2015-07-10 00:05:44.000000000","message":"Done","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"e9f9c7bfbc8de2d254b71b5c89b4b672d4aabe1b","unresolved":false,"context_lines":[{"line_number":353,"context_line":"        super(HaRouter, self).delete(agent)"},{"line_number":354,"context_line":""},{"line_number":355,"context_line":"    def process(self, agent):"},{"line_number":356,"context_line":"        super(HaRouter, self).process(agent)"},{"line_number":357,"context_line":"        if self.ha_port:"},{"line_number":358,"context_line":"            self.enable_keepalived()"},{"line_number":359,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_cca2fead","line":356,"updated":"2015-07-09 22:10:45.000000000","message":"unrelated change - blank line.","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"e234fb1f950bfcec2a24963542c0ab6159b905bc","unresolved":false,"context_lines":[{"line_number":353,"context_line":"        super(HaRouter, self).delete(agent)"},{"line_number":354,"context_line":""},{"line_number":355,"context_line":"    def process(self, agent):"},{"line_number":356,"context_line":"        super(HaRouter, self).process(agent)"},{"line_number":357,"context_line":"        if self.ha_port:"},{"line_number":358,"context_line":"            self.enable_keepalived()"},{"line_number":359,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_4301d805","line":356,"in_reply_to":"ba3cc151_cca2fead","updated":"2015-07-10 00:05:44.000000000","message":"Done","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":95,"context_line":"            keepalived.KeepalivedConf(),"},{"line_number":96,"context_line":"            process_monitor,"},{"line_number":97,"context_line":"            conf_path\u003dself.agent_conf.ha_confs_path,"},{"line_number":98,"context_line":"            namespace\u003dself.snat_ns_name)"},{"line_number":99,"context_line":"        config \u003d self.keepalived_manager.config"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"        interface_name \u003d self.get_ha_device_name()"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_99f3810a","line":98,"updated":"2015-08-04 19:13:41.000000000","message":"What\u0027s the point of all of the renames?","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":95,"context_line":"            keepalived.KeepalivedConf(),"},{"line_number":96,"context_line":"            process_monitor,"},{"line_number":97,"context_line":"            conf_path\u003dself.agent_conf.ha_confs_path,"},{"line_number":98,"context_line":"            namespace\u003dself.snat_ns_name)"},{"line_number":99,"context_line":"        config \u003d self.keepalived_manager.config"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"        interface_name \u003d self.get_ha_device_name()"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_c962563e","line":98,"in_reply_to":"1a4dcd0f_99f3810a","updated":"2015-08-06 19:36:01.000000000","message":"In order to support ha in dvr by inheriting this class, some of  the functions in this class need to be able to accept a different namespace than the qrouter namespace (ns_name). i.e. _init_keepalived(self, process_montior), becomes _init_keepalived(self, process_monitor, namespace), etc... \nThat would require a change of name anyways, *and* changing the declaration of the function. \nInstead its cleaner to just add one more namespace variable to the class, just like ns_name, but call it snat_ns_name, then for this class those two are the same: self.snat_ns_name \u003d self.ns_name, then this class still works as before (not even the memory footprint sould change, since self.ns_name and self.snat_ns_name should point to the same location in memory. \n\nAny class inherinting this just sets self.snat_namespace to something different if it wants to use those functions in an different namespace. \nThat is what DvrEdgeHaRouter does. All those functions will act on a different namespace (snat-xxxx ) instead of (qrouter--xxx)\n\nThe other option is to copy and paste all the stuff from this class to DvrEdgeHa, I don\u0027t have a preference but I think duplicating code is not very well liked...","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"060cdeb6de3c255713fe883608f0a6a4880a36cb","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        self.ha_port \u003d None"},{"line_number":39,"context_line":"        self.keepalived_manager \u003d None"},{"line_number":40,"context_line":"        self.snat_ns_name \u003d self.ns_name"},{"line_number":41,"context_line":"        self.state_change_callback \u003d state_change_callback"},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"    @property"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_afc0802b","line":40,"updated":"2015-08-27 21:29:38.000000000","message":"Is this leaking a bit too much detail of DVR in to the HA class?  I see the need to differentiate between the namespace used for snat and the qrouter namespace but I wonder if there is a better that leaves HA mostly ignorant of the distinction.\n\nIt begs the question:  why does the HA part of the router need to know anything about a qrouter namespace in a DVR router?  It should only need to know about the snat part of it, right?","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        self.ha_port \u003d None"},{"line_number":39,"context_line":"        self.keepalived_manager \u003d None"},{"line_number":40,"context_line":"        self.snat_ns_name \u003d self.ns_name"},{"line_number":41,"context_line":"        self.state_change_callback \u003d state_change_callback"},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"    @property"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_93c76663","line":40,"in_reply_to":"da20952f_afc0802b","updated":"2015-08-31 19:15:25.000000000","message":"It turned out that the least disruptive way of modifying this class to accomodate any descendants that want to use the functions in here that apply to ha.\nMost of the methods dealing with ha in this class take a namespace, and that namespace was hardcoded. This is actually the cleanest least invasive way of allowing for that namespace to be configurable by the caller. \nI don\u0027t think  this addition actually show anything about dvr or other routers, it just adds the flexibility of the ha stuff to be executed on any chosen namespace. In the case of this class the snat namespace and the ns namespace are the same. so it supports the legacy expected behavior. \nAny descendant can simply inherit this class and change the snat_ns_name to whereever they way to use the ha functions. \nNotice that at no point is dvr or the \"reason\" as to why you would want to use a different namespace stated. It just gives descendants the option to do so. \nI would be willing to bet that in this way, not even the memory footprint is changed because the self.snat_ns_name and self.ns_name variables both point to the same memory location, which contains the namespace name. \n\nOne  option would be to copy and paste all the wanted methods to the new class dvr_edge_ha class, which I am not opposed to, but I did not want to create duplicate code, but perhaps it is worth the trouble.\n\nA third option would be to re-write this class to make it more fexible so you can pass in the namespace, but that would be really not a good idea ( it think). It would required rewriting all the callers of this classes methods. I don\u0027t\u0027 really want to do that.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"060cdeb6de3c255713fe883608f0a6a4880a36cb","unresolved":false,"context_lines":[{"line_number":95,"context_line":"            keepalived.KeepalivedConf(),"},{"line_number":96,"context_line":"            process_monitor,"},{"line_number":97,"context_line":"            conf_path\u003dself.agent_conf.ha_confs_path,"},{"line_number":98,"context_line":"            namespace\u003dself.snat_ns_name)"},{"line_number":99,"context_line":"        config \u003d self.keepalived_manager.config"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"        interface_name \u003d self.get_ha_device_name()"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_cf27f410","line":98,"updated":"2015-08-27 21:29:38.000000000","message":"Add the next line back in.  This patch shouldn\u0027t remove it.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":95,"context_line":"            keepalived.KeepalivedConf(),"},{"line_number":96,"context_line":"            process_monitor,"},{"line_number":97,"context_line":"            conf_path\u003dself.agent_conf.ha_confs_path,"},{"line_number":98,"context_line":"            namespace\u003dself.snat_ns_name)"},{"line_number":99,"context_line":"        config \u003d self.keepalived_manager.config"},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"        interface_name \u003d self.get_ha_device_name()"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_f3cca28a","line":98,"in_reply_to":"da20952f_cf27f410","updated":"2015-08-31 19:15:25.000000000","message":"Done","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"8d2c3d21180c15105e933fad1e79d4ef3e561adf","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        self.ha_port \u003d None"},{"line_number":39,"context_line":"        self.keepalived_manager \u003d None"},{"line_number":40,"context_line":"        self.snat_ns_name \u003d self.ns_name"},{"line_number":41,"context_line":"        self.state_change_callback \u003d state_change_callback"},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"    @property"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_a81c1a6b","line":40,"updated":"2015-09-02 11:46:46.000000000","message":"In continuation to what you and Carl were talking about in patchset #29; It is unclear to me why this specific renaming is correct - for a simple HA router (which is not a DVR) we don\u0027t create an snat namespace, so why is the code saying that we do?\n\nI appreciate the need to feed specific namespaces to some functions (this is good coding), but the variable should be named \u0027ns_name\u0027 and not \u0027snat_ns_name\u0027.\n\nHow about reversing the situation: in the 2 functions that actually refer self.ns_name (internal_network_added and _disable_ipv6_addressing_on_interace), use a new variable called \u0027qrouter_ns_name\u0027 (which defualts to \u0027ns_name\u0027). Then descendants can change the ns_name, which is the namespace the HA interfaces should actually live in, and also reference the qrouter_ns_name - with clear distinction between the two.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7b4a18dc3a2ddc403ddba44c2ea67f75e19c2f7c","unresolved":false,"context_lines":[{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        self.ha_port \u003d None"},{"line_number":39,"context_line":"        self.keepalived_manager \u003d None"},{"line_number":40,"context_line":"        self.snat_ns_name \u003d self.ns_name"},{"line_number":41,"context_line":"        self.state_change_callback \u003d state_change_callback"},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"    @property"}],"source_content_type":"text/x-python","patch_set":31,"id":"da20952f_5e5f1c75","line":40,"in_reply_to":"da20952f_a81c1a6b","updated":"2015-09-09 18:41:47.000000000","message":"hmm so I think there is a bit of confusion going on here. \n\nns_name is the qrouter namespace. I don\u0027t like the name but it is all over the code. (do a search on .ns_name)\nsnat_ns_name, is the new variable here which is meant to hold the name of the namespace which holds the snat stuff. \n\n\nns_name \u003d qrouter-\u003c#####\u003e\nsnat_ns_name \u003d snat-\u003c######\u003e \n\nnow in ha_router.py there are different functions which need to act on different places (snat namespace or qrouter namespace) \nin the case of a regular ha router or legacy router, the NAME of the namespace for ns_name and snat_ns_name are the same. \n\n\nso I agree that your comments are correct, we might want to use qrotuer_ns_name instead of ns_name, but that is a bigger questions than for this patch :). I really don\u0027t want to go around changing ns_name wherever it appears. \n\n\nBTW the code does not say we create an snat_namespace. it says that the namespaces\u0027 name are the same.  note the \"ns_NAME and snat_ns_NAME) :) \nHey I agree this is a bit confusing but frankly, its the easiest way to modify this class to allow for using different namespace NAMES. \nI am open to suggestions.. just keep in mind we want to touch this class as  little as possible.","commit_id":"2cd813d82796f0b66bd7d3ae235162bf2c6b1a5f"},{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"5fc077e5d16a2875efa58219211aaaaa0873aaf5","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        super(HaRouter, self).__init__(*args, **kwargs)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        self.ha_port \u003d None"},{"line_number":39,"context_line":"        self.keepalived_manager \u003d None"},{"line_number":40,"context_line":"        self.snat_ns_name \u003d self.ns_name"},{"line_number":41,"context_line":"        self.state_change_callback \u003d state_change_callback"},{"line_number":42,"context_line":""}],"source_content_type":"text/x-python","patch_set":34,"id":"da20952f_fe835ea7","line":39,"updated":"2015-09-10 20:43:04.000000000","message":"Should this be in the HaRouter? since you have named it to \u0027snat_ns_name\",  should it not be in DVRHAEdgerouter.","commit_id":"1efbf11944864039040df37f936dbe18d99aa58e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"fda133806242acf939887b067f8826c631634618","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        super(HaRouter, self).__init__(*args, **kwargs)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        self.ha_port \u003d None"},{"line_number":39,"context_line":"        self.keepalived_manager \u003d None"},{"line_number":40,"context_line":"        self.snat_ns_name \u003d self.ns_name"},{"line_number":41,"context_line":"        self.state_change_callback \u003d state_change_callback"},{"line_number":42,"context_line":""}],"source_content_type":"text/x-python","patch_set":34,"id":"da20952f_26dbff7d","line":39,"in_reply_to":"da20952f_fe835ea7","updated":"2015-09-11 05:34:14.000000000","message":"In and HA router the ns_name and snat_ns_name are the same. \nso we have to set it to  be the same at __init__ That way older code is none the wiser.  If we pass it along as a call to all the methods, there is larger amount of code that has to be changed.\nPerhaps I should call the variable something else like ha_ns_name?","commit_id":"1efbf11944864039040df37f936dbe18d99aa58e"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"47b2cb7eb0c1a23d0004eb245b85a7439728fca2","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":161,"context_line":"            try:"},{"line_number":162,"context_line":"                self._add_vip(ip_cidr, interface_name)"},{"line_number":163,"context_line":"            except keepalived.VIPDuplicateAddressException:"},{"line_number":164,"context_line":"                LOG.info(_LI(\"%s has already been added to keepalive\"),"},{"line_number":165,"context_line":"                         ip_cidr)"},{"line_number":166,"context_line":""}],"source_content_type":"text/x-python","patch_set":40,"id":"ba15a1d1_5526b7ce","line":163,"updated":"2015-09-23 22:03:37.000000000","message":"Ignoring this exception is copied over from PS39. I\u0027ll have to examine why that is correct (It\u0027s probably not).","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"ebf89bfa56e1a604685c73f587d4b7f137bf84f8","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":161,"context_line":"            try:"},{"line_number":162,"context_line":"                self._add_vip(ip_cidr, interface_name)"},{"line_number":163,"context_line":"            except keepalived.VIPDuplicateAddressException:"},{"line_number":164,"context_line":"                LOG.info(_LI(\"%s has already been added to keepalive\"),"},{"line_number":165,"context_line":"                         ip_cidr)"},{"line_number":166,"context_line":""}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_d272737c","line":163,"in_reply_to":"ba15a1d1_5526b7ce","updated":"2015-09-24 22:49:21.000000000","message":"a few days ago there was a new patch introduce which throws this exception if you try to add the same ip twice to the keepalive config. \nIt turns out that as the code stands there, is no way  to avoid trying to add the ip address two times during the process() method for the router. \nThe first time it happens in \ndvr_edge_ha_router.py l 149\nwhen processing internal networks because there is an external gateway \n\nthe second tiem it happens in \ndvr_edge_ha_router.py l 151 \n when the external network is processed because there is an internal network :) \n\nThere might be another way to check, in other words don\u0027t call _add_vip if it is already there...  that is what exception basically does, but not sure if catching the exception is the best way to handle.\nAt least perhaps it would be best to override this method in dvr_edge_ha_router.py so it won\u0027t affect anything else","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e223baae9a093c7695fdf109af1a79c08c50fde0","unresolved":false,"context_lines":[{"line_number":156,"context_line":"                           prefix\u003dHA_DEV_PREFIX)"},{"line_number":157,"context_line":"        self.ha_port \u003d None"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _add_vips(self, port, interface_name):"},{"line_number":160,"context_line":"        for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":161,"context_line":"            try:"},{"line_number":162,"context_line":"                self._add_vip(ip_cidr, interface_name)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_cb52f966","line":159,"updated":"2015-09-30 17:55:43.000000000","message":"nit:  This looks like it could be an independent patch set given that it adds a try/catch which seems a little bit tangential to this patch set.","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"21c88928b3e1728abcdadf8e6bb3c14e230d968d","unresolved":false,"context_lines":[{"line_number":156,"context_line":"                           prefix\u003dHA_DEV_PREFIX)"},{"line_number":157,"context_line":"        self.ha_port \u003d None"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _add_vips(self, port, interface_name):"},{"line_number":160,"context_line":"        for ip_cidr in common_utils.fixed_ip_cidrs(port[\u0027fixed_ips\u0027]):"},{"line_number":161,"context_line":"            try:"},{"line_number":162,"context_line":"                self._add_vip(ip_cidr, interface_name)"}],"source_content_type":"text/x-python","patch_set":46,"id":"9a1a9d01_da3f1db2","line":159,"in_reply_to":"9a1a9d01_cb52f966","updated":"2015-09-30 19:24:24.000000000","message":"This is in response to a recent patch, which made _add_vip throw an exception error when an already configured ip is attempted to be added again.  I agree it is tangential but without it this patch will throw an error. shoudl we add it as a dependency?","commit_id":"6362f354538b90f78be2143c64435aac12ae72c8"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"1995d47a5f1a6a5c6b299b7db679e6319ba13626","unresolved":false,"context_lines":[{"line_number":161,"context_line":"            try:"},{"line_number":162,"context_line":"                self._add_vip(ip_cidr, interface_name)"},{"line_number":163,"context_line":"            except keepalived.VIPDuplicateAddressException:"},{"line_number":164,"context_line":"                LOG.info(_LI(\"%s has already been added to keepalive\"),"},{"line_number":165,"context_line":"                         ip_cidr)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    def _add_vip(self, ip_cidr, interface, scope\u003dNone):"}],"source_content_type":"text/x-python","patch_set":49,"id":"7a2fa921_35b6238b","line":164,"updated":"2015-10-06 12:18:56.000000000","message":"nit: should this be a debug-level message?","commit_id":"58df349a2cf2bea2ba84527e014288536d151135"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"ddcc5493a95b55fa17d020f7f41a6663afc837e9","unresolved":false,"context_lines":[{"line_number":161,"context_line":"            try:"},{"line_number":162,"context_line":"                self._add_vip(ip_cidr, interface_name)"},{"line_number":163,"context_line":"            except keepalived.VIPDuplicateAddressException:"},{"line_number":164,"context_line":"                LOG.info(_LI(\"%s has already been added to keepalive\"),"},{"line_number":165,"context_line":"                         ip_cidr)"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    def _add_vip(self, ip_cidr, interface, scope\u003dNone):"}],"source_content_type":"text/x-python","patch_set":49,"id":"7a2fa921_024f20be","line":164,"in_reply_to":"7a2fa921_35b6238b","updated":"2015-10-06 15:27:02.000000000","message":"Done","commit_id":"58df349a2cf2bea2ba84527e014288536d151135"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"1995d47a5f1a6a5c6b299b7db679e6319ba13626","unresolved":false,"context_lines":[{"line_number":387,"context_line":"                self.ha_state \u003d\u003d \u0027master\u0027):"},{"line_number":388,"context_line":"            super(HaRouter, self).enable_radvd(internal_ports)"},{"line_number":389,"context_line":""},{"line_number":390,"context_line":"    @property"},{"line_number":391,"context_line":"    def ha_namespace(self):"},{"line_number":392,"context_line":"        return self.ns_name"}],"source_content_type":"text/x-python","patch_set":49,"id":"7a2fa921_f502eb3d","line":390,"updated":"2015-10-06 12:18:56.000000000","message":"not a nit: can this function be moved further up, to be somewhere among the other class properties?\n\nThis actually really effects readability in this file and deserves a -1.. :(","commit_id":"58df349a2cf2bea2ba84527e014288536d151135"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"ddcc5493a95b55fa17d020f7f41a6663afc837e9","unresolved":false,"context_lines":[{"line_number":387,"context_line":"                self.ha_state \u003d\u003d \u0027master\u0027):"},{"line_number":388,"context_line":"            super(HaRouter, self).enable_radvd(internal_ports)"},{"line_number":389,"context_line":""},{"line_number":390,"context_line":"    @property"},{"line_number":391,"context_line":"    def ha_namespace(self):"},{"line_number":392,"context_line":"        return self.ns_name"}],"source_content_type":"text/x-python","patch_set":49,"id":"7a2fa921_224c9cc6","line":390,"in_reply_to":"7a2fa921_f502eb3d","updated":"2015-10-06 15:27:02.000000000","message":"Done","commit_id":"58df349a2cf2bea2ba84527e014288536d151135"}],"neutron/agent/l3/router_info.py":[{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"e9f9c7bfbc8de2d254b71b5c89b4b672d4aabe1b","unresolved":false,"context_lines":[{"line_number":42,"context_line":"                 agent_conf,"},{"line_number":43,"context_line":"                 interface_driver,"},{"line_number":44,"context_line":"                 use_ipv6\u003dFalse):"},{"line_number":45,"context_line":"        super(RouterInfo, self).__init__()"},{"line_number":46,"context_line":"        self.router_id \u003d router_id"},{"line_number":47,"context_line":"        self.ex_gw_port \u003d None"},{"line_number":48,"context_line":"        self._snat_enabled \u003d None"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_6c79722e","line":45,"updated":"2015-07-09 22:10:45.000000000","message":"Why is this init required here?","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"e234fb1f950bfcec2a24963542c0ab6159b905bc","unresolved":false,"context_lines":[{"line_number":42,"context_line":"                 agent_conf,"},{"line_number":43,"context_line":"                 interface_driver,"},{"line_number":44,"context_line":"                 use_ipv6\u003dFalse):"},{"line_number":45,"context_line":"        super(RouterInfo, self).__init__()"},{"line_number":46,"context_line":"        self.router_id \u003d router_id"},{"line_number":47,"context_line":"        self.ex_gw_port \u003d None"},{"line_number":48,"context_line":"        self._snat_enabled \u003d None"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_2004ca16","line":45,"in_reply_to":"ba3cc151_6c79722e","updated":"2015-07-10 00:05:44.000000000","message":"MOR (method resolution order). \ncurrently we have class DvrEdgeHaRouter(DvrEdgeRouter, HaRouter)\n\nwith out this init something like\nclass newRouterClass(DvrEdgeHaRouter, SomeOtherClass) \nwould not execute the __init__() for SomeOtherClass\n\njust coding for cooperative class inheritance.\nBut will remove to see if affects this patch at all. might have to add it back later if needed.","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"}],"neutron/tests/functional/agent/test_l3_agent.py":[{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"e9f9c7bfbc8de2d254b71b5c89b4b672d4aabe1b","unresolved":false,"context_lines":[{"line_number":1437,"context_line":"                \u0027priority\u0027: 1}"},{"line_number":1438,"context_line":""},{"line_number":1439,"context_line":"    def test_dvr_ha_router_failover(self):"},{"line_number":1440,"context_line":"        # prepair second agent."},{"line_number":1441,"context_line":"        self.failover_agent \u003d self._configure_agent(\u0027agent2\u0027)"},{"line_number":1442,"context_line":""},{"line_number":1443,"context_line":"        br_int_1 \u003d get_ovs_bridge(self.agent.conf.ovs_integration_bridge)"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_4cd34e04","line":1440,"updated":"2015-07-09 22:10:45.000000000","message":"This test case seems to be too long. Is it possible to break this up into smaller sub functions.","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"e234fb1f950bfcec2a24963542c0ab6159b905bc","unresolved":false,"context_lines":[{"line_number":1437,"context_line":"                \u0027priority\u0027: 1}"},{"line_number":1438,"context_line":""},{"line_number":1439,"context_line":"    def test_dvr_ha_router_failover(self):"},{"line_number":1440,"context_line":"        # prepair second agent."},{"line_number":1441,"context_line":"        self.failover_agent \u003d self._configure_agent(\u0027agent2\u0027)"},{"line_number":1442,"context_line":""},{"line_number":1443,"context_line":"        br_int_1 \u003d get_ovs_bridge(self.agent.conf.ovs_integration_bridge)"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_2304d416","line":1440,"in_reply_to":"ba3cc151_4cd34e04","updated":"2015-07-10 00:05:44.000000000","message":"Not really. the reason it is so long is because we have to get quite a lot of information: ip_addreses, interfac_names, etc.. which can only be obtained from an instantiated router, then compared those values to what\u0027s on the real system (unix). \nI could move it to a setup() but since this is the only test which needs that information right now I did not want to do it until it is needed. \nIf more tests require the same information I will refactor it then. \n\nBut right now, no other test needs any of these code, so it really doesn\u0027t do any good to refactor it at this time, but will keep an eye out.","commit_id":"833110dd8bdb0eda0410e6e1579ef092e0728917"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":789,"context_line":"            namespaces.RouterNamespace._get_ns_name(router_info[\u0027id\u0027]),"},{"line_number":790,"context_line":"            self.NESTED_NAMESPACE_SEPARATOR, self.agent.host)"},{"line_number":791,"context_line":"        router1 \u003d self.manage_router(self.agent, router_info)"},{"line_number":792,"context_line":""},{"line_number":793,"context_line":"        router_info_2 \u003d copy.deepcopy(router_info)"},{"line_number":794,"context_line":"        router_info_2[l3_constants.HA_INTERFACE_KEY] \u003d ("},{"line_number":795,"context_line":"            l3_test_common.get_ha_interface(ip\u003d\u0027169.254.192.2\u0027,"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_8e92599e","side":"PARENT","line":792,"updated":"2015-08-04 19:13:41.000000000","message":"Unnecessary change.","commit_id":"37eab0bb4ce3040a2e19d8df1f00071175f17894"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":1048,"context_line":"        # only dvr and dvr_snat are applicable"},{"line_number":1049,"context_line":"        self.agent.conf.agent_mode \u003d \u0027dvr_snat\u0027 if enable_snat else \u0027dvr\u0027"},{"line_number":1050,"context_line":"        self.agent.conf.network_device_mtu \u003d custom_mtu"},{"line_number":1051,"context_line":""},{"line_number":1052,"context_line":"        # We get the router info particular to a dvr router"},{"line_number":1053,"context_line":"        router_info \u003d self.generate_dvr_router_info("},{"line_number":1054,"context_line":"            enable_ha, enable_snat)"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_2ee8e50f","side":"PARENT","line":1051,"updated":"2015-08-04 19:13:41.000000000","message":"Unnecessary change.","commit_id":"37eab0bb4ce3040a2e19d8df1f00071175f17894"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":27,"context_line":"import time"},{"line_number":28,"context_line":"import webob.dec"},{"line_number":29,"context_line":"import webob.exc"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"from neutron.agent.common import config as agent_config"},{"line_number":33,"context_line":"from neutron.agent.common import ovs_lib"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_ee82fd51","line":30,"updated":"2015-08-04 19:13:41.000000000","message":"Extra line.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":27,"context_line":"import time"},{"line_number":28,"context_line":"import webob.dec"},{"line_number":29,"context_line":"import webob.exc"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"from neutron.agent.common import config as agent_config"},{"line_number":33,"context_line":"from neutron.agent.common import ovs_lib"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_6902223f","line":30,"in_reply_to":"1a4dcd0f_ee82fd51","updated":"2015-08-06 19:36:01.000000000","message":"Done","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":79,"context_line":"        logging.register_options(config)"},{"line_number":80,"context_line":"        agent_config.register_process_monitor_opts(config)"},{"line_number":81,"context_line":"        return config"},{"line_number":82,"context_line":""},{"line_number":83,"context_line":"    def _configure_agent(self, host):"},{"line_number":84,"context_line":"        conf \u003d self._get_config_opts()"},{"line_number":85,"context_line":"        l3_agent_main.register_opts(conf)"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_ceb381e8","line":82,"updated":"2015-08-04 19:13:41.000000000","message":"I think that _configure_agent should accept the agent_mode parameter.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":79,"context_line":"        logging.register_options(config)"},{"line_number":80,"context_line":"        agent_config.register_process_monitor_opts(config)"},{"line_number":81,"context_line":"        return config"},{"line_number":82,"context_line":""},{"line_number":83,"context_line":"    def _configure_agent(self, host):"},{"line_number":84,"context_line":"        conf \u003d self._get_config_opts()"},{"line_number":85,"context_line":"        l3_agent_main.register_opts(conf)"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_6989c2c8","line":82,"in_reply_to":"1a4dcd0f_ceb381e8","updated":"2015-08-06 19:36:01.000000000","message":"Done","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":159,"context_line":"            conf,"},{"line_number":160,"context_line":"            router.router_id,"},{"line_number":161,"context_line":"            router.ns_name)"},{"line_number":162,"context_line":"        for _ in range(300):"},{"line_number":163,"context_line":"            time.sleep(0.01)"},{"line_number":164,"context_line":"            if pm.active:"},{"line_number":165,"context_line":"                break"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_cea861de","line":162,"updated":"2015-08-04 19:13:41.000000000","message":"Here you want to use utils.wait_until_true instead.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":159,"context_line":"            conf,"},{"line_number":160,"context_line":"            router.router_id,"},{"line_number":161,"context_line":"            router.ns_name)"},{"line_number":162,"context_line":"        for _ in range(300):"},{"line_number":163,"context_line":"            time.sleep(0.01)"},{"line_number":164,"context_line":"            if pm.active:"},{"line_number":165,"context_line":"                break"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_c9333613","line":162,"in_reply_to":"1a4dcd0f_cea861de","updated":"2015-08-06 19:36:01.000000000","message":"ok will do, but utils.wait_until_tru, raises exception. so will have to add try catch finally block, because we don\u0027t want this function to raise exception, we want it to return true or false. \nDone","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":998,"context_line":""},{"line_number":999,"context_line":""},{"line_number":1000,"context_line":"class TestDvrRouter(L3AgentTestFramework):"},{"line_number":1001,"context_line":""},{"line_number":1002,"context_line":"    def test_dvr_router_lifecycle_without_ha_without_snat_with_fips(self):"},{"line_number":1003,"context_line":"        self._dvr_router_lifecycle(enable_ha\u003dFalse, enable_snat\u003dFalse)"},{"line_number":1004,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_4e8c517f","line":1001,"updated":"2015-08-04 19:13:41.000000000","message":"Unnecessary change.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":998,"context_line":""},{"line_number":999,"context_line":""},{"line_number":1000,"context_line":"class TestDvrRouter(L3AgentTestFramework):"},{"line_number":1001,"context_line":""},{"line_number":1002,"context_line":"    def test_dvr_router_lifecycle_without_ha_without_snat_with_fips(self):"},{"line_number":1003,"context_line":"        self._dvr_router_lifecycle(enable_ha\u003dFalse, enable_snat\u003dFalse)"},{"line_number":1004,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_e91f126a","line":1001,"in_reply_to":"1a4dcd0f_4e8c517f","updated":"2015-08-06 19:36:01.000000000","message":"Done","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":1077,"context_line":"        # correct ip address for the ha interface:"},{"line_number":1078,"context_line":"        if enable_ha and router_is_master:"},{"line_number":1079,"context_line":"            router_info[l3_constants.HA_INTERFACE_KEY] \u003d \\"},{"line_number":1080,"context_line":"                self.get_ha_primary_interface()"},{"line_number":1081,"context_line":""},{"line_number":1082,"context_line":"        # We need to mock the get_agent_gateway_port return value"},{"line_number":1083,"context_line":"        # because the whole L3PluginApi is mocked and we need the port"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_8efed936","line":1080,"updated":"2015-08-04 19:13:41.000000000","message":"I have to say I really don\u0027t understand why this is needed. Can you please explain?","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":1077,"context_line":"        # correct ip address for the ha interface:"},{"line_number":1078,"context_line":"        if enable_ha and router_is_master:"},{"line_number":1079,"context_line":"            router_info[l3_constants.HA_INTERFACE_KEY] \u003d \\"},{"line_number":1080,"context_line":"                self.get_ha_primary_interface()"},{"line_number":1081,"context_line":""},{"line_number":1082,"context_line":"        # We need to mock the get_agent_gateway_port return value"},{"line_number":1083,"context_line":"        # because the whole L3PluginApi is mocked and we need the port"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_690c6241","line":1080,"in_reply_to":"1a4dcd0f_8efed936","updated":"2015-08-06 19:36:01.000000000","message":"Not really :) :). I thik you are correct.  I was atempting to make sure we start off with the correct ip address (not leave it to the keepalived process) but now that I think about it, I don\u0027t think is necessary to do so. will take out and retest to see if it makes a difference. \nDone","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":1233,"context_line":"        # that the correct ports and ip addresses exist in the"},{"line_number":1234,"context_line":"        # snat_ns_name namespace"},{"line_number":1235,"context_line":"        if self.agent.conf.agent_mode \u003d\u003d \u0027dvr_snat\u0027:"},{"line_number":1236,"context_line":"            if isinstance(router, DvrEdgeHaRouter):"},{"line_number":1237,"context_line":"                device_name \u003d router.get_external_device_name"},{"line_number":1238,"context_line":"                if router.ha_state \u003d\u003d \u0027backup\u0027:"},{"line_number":1239,"context_line":"                    self.assertFalse(self.device_exists_with_ips_and_mac("}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_0e62c9ad","line":1236,"updated":"2015-08-04 19:13:41.000000000","message":"isinstance -\u003e check that the \u0027router\u0027 parameter is HA? Or just pass \u0027enable_ha\u0027 in to this method.\n\nIt assumes less from the implementation and more from the input of the test.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":1235,"context_line":"        if self.agent.conf.agent_mode \u003d\u003d \u0027dvr_snat\u0027:"},{"line_number":1236,"context_line":"            if isinstance(router, DvrEdgeHaRouter):"},{"line_number":1237,"context_line":"                device_name \u003d router.get_external_device_name"},{"line_number":1238,"context_line":"                if router.ha_state \u003d\u003d \u0027backup\u0027:"},{"line_number":1239,"context_line":"                    self.assertFalse(self.device_exists_with_ips_and_mac("},{"line_number":1240,"context_line":"                        external_port, device_name,"},{"line_number":1241,"context_line":"                        snat_ns_name))"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_4e72114b","line":1238,"updated":"2015-08-04 19:13:41.000000000","message":"I don\u0027t understand this. The only test that uses this method is the dvr HA lifecycle test, and it waits until the router becomes active before calling this method.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":1235,"context_line":"        if self.agent.conf.agent_mode \u003d\u003d \u0027dvr_snat\u0027:"},{"line_number":1236,"context_line":"            if isinstance(router, DvrEdgeHaRouter):"},{"line_number":1237,"context_line":"                device_name \u003d router.get_external_device_name"},{"line_number":1238,"context_line":"                if router.ha_state \u003d\u003d \u0027backup\u0027:"},{"line_number":1239,"context_line":"                    self.assertFalse(self.device_exists_with_ips_and_mac("},{"line_number":1240,"context_line":"                        external_port, device_name,"},{"line_number":1241,"context_line":"                        snat_ns_name))"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_9fb31ce9","line":1238,"in_reply_to":"1a4dcd0f_4e72114b","updated":"2015-08-06 19:36:01.000000000","message":"yes. correct. I\u0027ll take the check for backup out. Originally I wanted to add a test to check a router in backup state, but I\u0027ll just add this back when time comes to need it.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":1240,"context_line":"                        external_port, device_name,"},{"line_number":1241,"context_line":"                        snat_ns_name))"},{"line_number":1242,"context_line":"                else:"},{"line_number":1243,"context_line":"                    self._wait_for_device_to_be_configured(external_port,"},{"line_number":1244,"context_line":"                                                           device_name,"},{"line_number":1245,"context_line":"                                                           snat_ns_name)"},{"line_number":1246,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_2e6305de","line":1243,"updated":"2015-08-04 19:13:41.000000000","message":"Here you want to use utils.wait_until_true instead.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":1240,"context_line":"                        external_port, device_name,"},{"line_number":1241,"context_line":"                        snat_ns_name))"},{"line_number":1242,"context_line":"                else:"},{"line_number":1243,"context_line":"                    self._wait_for_device_to_be_configured(external_port,"},{"line_number":1244,"context_line":"                                                           device_name,"},{"line_number":1245,"context_line":"                                                           snat_ns_name)"},{"line_number":1246,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_3fcd2866","line":1243,"in_reply_to":"1a4dcd0f_2e6305de","updated":"2015-08-06 19:36:01.000000000","message":"ok will do, but utils.wait_until_tru, raises exception. so will have to add try catch finally block, because we don\u0027t want this function to raise exception, we just want it to wait long enough to allow system to catch up. The decision to raise exception or \"fail/pass\" test should be left to caller. \nDone","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":1435,"context_line":"        self.assertTrue(self._namespace_exists(fip_ns))"},{"line_number":1436,"context_line":"        self._assert_snat_namespace_does_not_exist(router1)"},{"line_number":1437,"context_line":""},{"line_number":1438,"context_line":"    def get_ha_primary_interface(self):"},{"line_number":1439,"context_line":"        ip \u003d \u0027169.254.0.1\u0027"},{"line_number":1440,"context_line":"        mac \u003d \u002700:00:12:34:56:78\u0027"},{"line_number":1441,"context_line":"        subnet_id \u003d _uuid()"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_6e782dc5","line":1438,"updated":"2015-08-04 19:13:41.000000000","message":"I\u0027m not convinced this is needed, but if it is you can just use tests/common/l3_test_common.get_ha_interface and pass in \u0027169.254.0.1\u0027 as the IP parameter.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":1435,"context_line":"        self.assertTrue(self._namespace_exists(fip_ns))"},{"line_number":1436,"context_line":"        self._assert_snat_namespace_does_not_exist(router1)"},{"line_number":1437,"context_line":""},{"line_number":1438,"context_line":"    def get_ha_primary_interface(self):"},{"line_number":1439,"context_line":"        ip \u003d \u0027169.254.0.1\u0027"},{"line_number":1440,"context_line":"        mac \u003d \u002700:00:12:34:56:78\u0027"},{"line_number":1441,"context_line":"        subnet_id \u003d _uuid()"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_e01853d5","line":1438,"in_reply_to":"1a4dcd0f_6e782dc5","updated":"2015-08-06 19:36:01.000000000","message":"removing. Don\u0027t need this any more.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":1465,"context_line":"        self.failover_agent \u003d self._configure_agent(\u0027agent2\u0027)"},{"line_number":1466,"context_line":"        self.failover_agent.conf.agent_mode \u003d \u0027dvr_snat\u0027"},{"line_number":1467,"context_line":""},{"line_number":1468,"context_line":"        br_int_1 \u003d get_ovs_bridge(self.agent.conf.ovs_integration_bridge)"},{"line_number":1469,"context_line":"        br_int_2 \u003d get_ovs_bridge("},{"line_number":1470,"context_line":"            self.failover_agent.conf.ovs_integration_bridge)"},{"line_number":1471,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_2e78e5f1","line":1468,"updated":"2015-08-04 19:13:41.000000000","message":"You can take the code from 787, extract it to a method and reuse it here.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":1465,"context_line":"        self.failover_agent \u003d self._configure_agent(\u0027agent2\u0027)"},{"line_number":1466,"context_line":"        self.failover_agent.conf.agent_mode \u003d \u0027dvr_snat\u0027"},{"line_number":1467,"context_line":""},{"line_number":1468,"context_line":"        br_int_1 \u003d get_ovs_bridge(self.agent.conf.ovs_integration_bridge)"},{"line_number":1469,"context_line":"        br_int_2 \u003d get_ovs_bridge("},{"line_number":1470,"context_line":"            self.failover_agent.conf.ovs_integration_bridge)"},{"line_number":1471,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_9bcc5ebc","line":1468,"in_reply_to":"1a4dcd0f_2e78e5f1","updated":"2015-08-06 19:36:01.000000000","message":"Done","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":1473,"context_line":"        br_int_1.add_port(veth1.name)"},{"line_number":1474,"context_line":"        br_int_2.add_port(veth2.name)"},{"line_number":1475,"context_line":""},{"line_number":1476,"context_line":"        r_1_info \u003d self.generate_dvr_router_info(enable_ha\u003dTrue,"},{"line_number":1477,"context_line":"                                                 enable_snat\u003dTrue,"},{"line_number":1478,"context_line":"                                                 agent\u003dself.agent)"},{"line_number":1479,"context_line":"        r_1_snat_ns_name \u003d namespaces.build_ns_name(dvr_snat_ns.SNAT_NS_PREFIX,"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_2edc6511","line":1476,"updated":"2015-08-04 19:13:41.000000000","message":"Looks like lines 1476 to 1481 repeat themselves for r1 and r2.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":1473,"context_line":"        br_int_1.add_port(veth1.name)"},{"line_number":1474,"context_line":"        br_int_2.add_port(veth2.name)"},{"line_number":1475,"context_line":""},{"line_number":1476,"context_line":"        r_1_info \u003d self.generate_dvr_router_info(enable_ha\u003dTrue,"},{"line_number":1477,"context_line":"                                                 enable_snat\u003dTrue,"},{"line_number":1478,"context_line":"                                                 agent\u003dself.agent)"},{"line_number":1479,"context_line":"        r_1_snat_ns_name \u003d namespaces.build_ns_name(dvr_snat_ns.SNAT_NS_PREFIX,"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_7b82b2c9","line":1476,"in_reply_to":"1a4dcd0f_2edc6511","updated":"2015-08-06 19:36:01.000000000","message":"refactroed\nDone","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":1567,"context_line":"        self._assert_no_ip_addresses_on_interface(mocked_r_1_snat_ns_name,"},{"line_number":1568,"context_line":"                                                  r_1_snat_port_name)"},{"line_number":1569,"context_line":""},{"line_number":1570,"context_line":"    def fail_dvr_ha_router(self, router):"},{"line_number":1571,"context_line":"        device_name \u003d router.get_ha_device_name()"},{"line_number":1572,"context_line":"        ha_device \u003d ip_lib.IPDevice(device_name, router.snat_ns_name)"},{"line_number":1573,"context_line":"        ha_device.link.set_down()"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_6edcade4","line":1570,"updated":"2015-08-04 19:13:41.000000000","message":"You can reuse the existing fail_ha_router method and get the namespace name conditionally according to the router type. That functionality should probably belong to the router classes themselves, and the method should polymorphically return the correct namespace.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"8ec2c9846ab7123195dda89a5134f314683c5ad3","unresolved":false,"context_lines":[{"line_number":1567,"context_line":"        self._assert_no_ip_addresses_on_interface(mocked_r_1_snat_ns_name,"},{"line_number":1568,"context_line":"                                                  r_1_snat_port_name)"},{"line_number":1569,"context_line":""},{"line_number":1570,"context_line":"    def fail_dvr_ha_router(self, router):"},{"line_number":1571,"context_line":"        device_name \u003d router.get_ha_device_name()"},{"line_number":1572,"context_line":"        ha_device \u003d ip_lib.IPDevice(device_name, router.snat_ns_name)"},{"line_number":1573,"context_line":"        ha_device.link.set_down()"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_1bf7ee59","line":1570,"in_reply_to":"1a4dcd0f_6edcade4","updated":"2015-08-06 19:36:01.000000000","message":"yes. you are correct, and as a matter of fact wit the changes done to HaRouter class it already does. so will change fail_ha_router to do so.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"a9853faf916a825b7da2e63036e563a11e155ebe","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from oslo_utils import uuidutils"},{"line_number":25,"context_line":"import six"},{"line_number":26,"context_line":"import testtools"},{"line_number":27,"context_line":"import webob.dec"},{"line_number":28,"context_line":"import webob.exc"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"1a4dcd0f_259ec863","line":27,"updated":"2015-08-15 09:20:47.000000000","message":"unrelated changes?","commit_id":"d920b6183f9c8b7c05fc1b0d90733f6e26871554"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"45847fa74f64a83f723184b77081ab5663ee8dec","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from oslo_utils import uuidutils"},{"line_number":25,"context_line":"import six"},{"line_number":26,"context_line":"import testtools"},{"line_number":27,"context_line":"import webob.dec"},{"line_number":28,"context_line":"import webob.exc"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"fa1b9901_ef3561a8","line":27,"in_reply_to":"1a4dcd0f_259ec863","updated":"2015-08-21 18:15:19.000000000","message":"not sure... i did not do anything intentional.\ncorrected.\nDONE","commit_id":"d920b6183f9c8b7c05fc1b0d90733f6e26871554"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"a9853faf916a825b7da2e63036e563a11e155ebe","unresolved":false,"context_lines":[{"line_number":163,"context_line":"            router.ns_name)"},{"line_number":164,"context_line":"        try:"},{"line_number":165,"context_line":"            utils.wait_until_true(lambda: pm.active is True, timeout, .01)"},{"line_number":166,"context_line":"        except Exception:"},{"line_number":167,"context_line":"            pass"},{"line_number":168,"context_line":"        finally:"},{"line_number":169,"context_line":"            return pm.active"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a4dcd0f_058aac1a","line":166,"updated":"2015-08-15 09:20:47.000000000","message":"it seems a too broad exception catch","commit_id":"d920b6183f9c8b7c05fc1b0d90733f6e26871554"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"45847fa74f64a83f723184b77081ab5663ee8dec","unresolved":false,"context_lines":[{"line_number":163,"context_line":"            router.ns_name)"},{"line_number":164,"context_line":"        try:"},{"line_number":165,"context_line":"            utils.wait_until_true(lambda: pm.active is True, timeout, .01)"},{"line_number":166,"context_line":"        except Exception:"},{"line_number":167,"context_line":"            pass"},{"line_number":168,"context_line":"        finally:"},{"line_number":169,"context_line":"            return pm.active"}],"source_content_type":"text/x-python","patch_set":16,"id":"fa1b9901_36b24d07","line":166,"in_reply_to":"1a4dcd0f_058aac1a","updated":"2015-08-21 18:15:19.000000000","message":"yeah. The wait_until_true is mean to bomb out if timeout happens, but here it is being used just to wait a few seconds (like a sleep) and then continue. I had a sort of sleep while loop before but previous review comments asked that I use wait_until_timeoiut. \nAdded custom exception and will catch that instead. \n\nDone","commit_id":"d920b6183f9c8b7c05fc1b0d90733f6e26871554"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"a9853faf916a825b7da2e63036e563a11e155ebe","unresolved":false,"context_lines":[{"line_number":1447,"context_line":""},{"line_number":1448,"context_line":"    def test_dvr_ha_router_failover(self):"},{"line_number":1449,"context_line":"        # prepair second agent."},{"line_number":1450,"context_line":"        self.agent.conf.agent_mode \u003d \u0027dvr_snat\u0027"},{"line_number":1451,"context_line":""},{"line_number":1452,"context_line":"        self.failover_agent \u003d self._configure_agent(\u0027agent2\u0027)"},{"line_number":1453,"context_line":"        self.failover_agent.conf.agent_mode \u003d \u0027dvr_snat\u0027"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a4dcd0f_25756831","line":1450,"updated":"2015-08-15 09:20:47.000000000","message":"imo, you should split this implementation into many methods to help developers to understand what the test does","commit_id":"d920b6183f9c8b7c05fc1b0d90733f6e26871554"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"45847fa74f64a83f723184b77081ab5663ee8dec","unresolved":false,"context_lines":[{"line_number":1447,"context_line":""},{"line_number":1448,"context_line":"    def test_dvr_ha_router_failover(self):"},{"line_number":1449,"context_line":"        # prepair second agent."},{"line_number":1450,"context_line":"        self.agent.conf.agent_mode \u003d \u0027dvr_snat\u0027"},{"line_number":1451,"context_line":""},{"line_number":1452,"context_line":"        self.failover_agent \u003d self._configure_agent(\u0027agent2\u0027)"},{"line_number":1453,"context_line":"        self.failover_agent.conf.agent_mode \u003d \u0027dvr_snat\u0027"}],"source_content_type":"text/x-python","patch_set":16,"id":"fa1b9901_0fba15c1","line":1450,"in_reply_to":"1a4dcd0f_25756831","updated":"2015-08-21 18:15:19.000000000","message":"Done","commit_id":"d920b6183f9c8b7c05fc1b0d90733f6e26871554"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"477f5136adb4c9b7596532148ff7302304a99c18","unresolved":false,"context_lines":[{"line_number":28,"context_line":"import webob.dec"},{"line_number":29,"context_line":"import webob.exc"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"from neutron.agent.common import config as agent_config"},{"line_number":33,"context_line":"from neutron.agent.common import ovs_lib"},{"line_number":34,"context_line":"from neutron.agent.l3 import agent as neutron_l3_agent"}],"source_content_type":"text/x-python","patch_set":19,"id":"fa1b9901_b9c9adae","line":31,"updated":"2015-08-24 17:43:36.000000000","message":"Whitespace?  Some others in this file too","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"2fe4153b80da6e03d4a8556456d67544cad7b4f5","unresolved":false,"context_lines":[{"line_number":28,"context_line":"import webob.dec"},{"line_number":29,"context_line":"import webob.exc"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"from neutron.agent.common import config as agent_config"},{"line_number":33,"context_line":"from neutron.agent.common import ovs_lib"},{"line_number":34,"context_line":"from neutron.agent.l3 import agent as neutron_l3_agent"}],"source_content_type":"text/x-python","patch_set":19,"id":"fa1b9901_c4ad949c","line":31,"in_reply_to":"fa1b9901_b9c9adae","updated":"2015-08-24 18:17:44.000000000","message":"sorry, they keep sneaking in and my pep8 is not catching them. I \u0027ll clean them up","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"4215ce7bf1291f3d79c8bc45f9060cca3c4aa638","unresolved":false,"context_lines":[{"line_number":1263,"context_line":"                                          device_name,"},{"line_number":1264,"context_line":"                                          ns_name):"},{"line_number":1265,"context_line":"        try:"},{"line_number":1266,"context_line":"            utils.wait_until_true(self.device_exists_with_ips_and_mac("},{"line_number":1267,"context_line":"                                                            external_port,"},{"line_number":1268,"context_line":"                                                            device_name,"},{"line_number":1269,"context_line":"                                                            ns_name) is True,"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_3424c9ad","line":1266,"updated":"2015-08-27 20:30:02.000000000","message":"nit: I think if you put self.devices_exists_with_ips_and_mac on the next line indented over this will be more readable.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":1263,"context_line":"                                          device_name,"},{"line_number":1264,"context_line":"                                          ns_name):"},{"line_number":1265,"context_line":"        try:"},{"line_number":1266,"context_line":"            utils.wait_until_true(self.device_exists_with_ips_and_mac("},{"line_number":1267,"context_line":"                                                            external_port,"},{"line_number":1268,"context_line":"                                                            device_name,"},{"line_number":1269,"context_line":"                                                            ns_name) is True,"}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_ce23f154","line":1266,"in_reply_to":"da20952f_3424c9ad","updated":"2015-08-31 19:15:25.000000000","message":"Done","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"4215ce7bf1291f3d79c8bc45f9060cca3c4aa638","unresolved":false,"context_lines":[{"line_number":1268,"context_line":"                                                            device_name,"},{"line_number":1269,"context_line":"                                                            ns_name) is True,"},{"line_number":1270,"context_line":"                                  timeout\u003d5)"},{"line_number":1271,"context_line":"        except Exception:"},{"line_number":1272,"context_line":"            pass"},{"line_number":1273,"context_line":"        return"},{"line_number":1274,"context_line":""}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_f43df1bb","line":1271,"updated":"2015-08-27 20:30:02.000000000","message":"what exceptions are getting raised here? This seems like this will just hide bugs.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":1268,"context_line":"                                                            device_name,"},{"line_number":1269,"context_line":"                                                            ns_name) is True,"},{"line_number":1270,"context_line":"                                  timeout\u003d5)"},{"line_number":1271,"context_line":"        except Exception:"},{"line_number":1272,"context_line":"            pass"},{"line_number":1273,"context_line":"        return"},{"line_number":1274,"context_line":""}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_cee0d12b","line":1271,"in_reply_to":"da20952f_f43df1bb","updated":"2015-08-31 19:15:25.000000000","message":"I was asked by other reviews to use wait_until_true to do this waiting, however wait_until_true throws an excpetion if timeout is reached. But in this case it is ok for timeout to be reached. It is just meant to wait long enough to avoid any false failures. If the timeout is reached and the device still not exists, it is up to the caller to figure out if it is a failure or not (remove/add device, negative testing, etc..)\nI added a more specific Exception to only catch the timeout exception and no more.","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"4215ce7bf1291f3d79c8bc45f9060cca3c4aa638","unresolved":false,"context_lines":[{"line_number":1270,"context_line":"                                  timeout\u003d5)"},{"line_number":1271,"context_line":"        except Exception:"},{"line_number":1272,"context_line":"            pass"},{"line_number":1273,"context_line":"        return"},{"line_number":1274,"context_line":""},{"line_number":1275,"context_line":"    def _assert_dvr_gateway(self, router):"},{"line_number":1276,"context_line":"        gateway_expected_in_snat_namespace \u003d ("}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_5475bd95","line":1273,"updated":"2015-08-27 20:30:02.000000000","message":"nit: return unneeded","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"7a26f9fd010768d5927aeadff961b4c1286581c8","unresolved":false,"context_lines":[{"line_number":1270,"context_line":"                                  timeout\u003d5)"},{"line_number":1271,"context_line":"        except Exception:"},{"line_number":1272,"context_line":"            pass"},{"line_number":1273,"context_line":"        return"},{"line_number":1274,"context_line":""},{"line_number":1275,"context_line":"    def _assert_dvr_gateway(self, router):"},{"line_number":1276,"context_line":"        gateway_expected_in_snat_namespace \u003d ("}],"source_content_type":"text/x-python","patch_set":29,"id":"da20952f_cec77194","line":1273,"in_reply_to":"da20952f_5475bd95","updated":"2015-08-31 19:15:25.000000000","message":"Done","commit_id":"46e5ebd52d5e52466e8afb46000827657ccd511e"},{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"5fc077e5d16a2875efa58219211aaaaa0873aaf5","unresolved":false,"context_lines":[{"line_number":799,"context_line":""},{"line_number":800,"context_line":"    def setUp(self):"},{"line_number":801,"context_line":"        super(L3HATestFramework, self).setUp()"},{"line_number":802,"context_line":""},{"line_number":803,"context_line":"        self.failover_agent \u003d self._configure_agent(\u0027agent2\u0027)"},{"line_number":804,"context_line":""},{"line_number":805,"context_line":"        br_int_1 \u003d self._get_agent_ovs_integration_bridge(self.agent)"}],"source_content_type":"text/x-python","patch_set":34,"id":"da20952f_fe711edf","line":802,"updated":"2015-09-10 20:43:04.000000000","message":"blank line.","commit_id":"1efbf11944864039040df37f936dbe18d99aa58e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"fda133806242acf939887b067f8826c631634618","unresolved":false,"context_lines":[{"line_number":799,"context_line":""},{"line_number":800,"context_line":"    def setUp(self):"},{"line_number":801,"context_line":"        super(L3HATestFramework, self).setUp()"},{"line_number":802,"context_line":""},{"line_number":803,"context_line":"        self.failover_agent \u003d self._configure_agent(\u0027agent2\u0027)"},{"line_number":804,"context_line":""},{"line_number":805,"context_line":"        br_int_1 \u003d self._get_agent_ovs_integration_bridge(self.agent)"}],"source_content_type":"text/x-python","patch_set":34,"id":"da20952f_66425711","line":802,"in_reply_to":"da20952f_fe711edf","updated":"2015-09-11 05:34:14.000000000","message":"how are this dam blank lines getting in??? \nThanks","commit_id":"1efbf11944864039040df37f936dbe18d99aa58e"},{"author":{"_account_id":7016,"name":"Swaminathan Vasudevan","email":"swvasude@cisco.com","username":"souminathan"},"change_message_id":"5fc077e5d16a2875efa58219211aaaaa0873aaf5","unresolved":false,"context_lines":[{"line_number":806,"context_line":"        br_int_2 \u003d self._get_agent_ovs_integration_bridge(self.failover_agent)"},{"line_number":807,"context_line":""},{"line_number":808,"context_line":"        veth1, veth2 \u003d self.useFixture(net_helpers.VethFixture()).ports"},{"line_number":809,"context_line":""},{"line_number":810,"context_line":"        br_int_1.add_port(veth1.name)"},{"line_number":811,"context_line":"        br_int_2.add_port(veth2.name)"},{"line_number":812,"context_line":""}],"source_content_type":"text/x-python","patch_set":34,"id":"da20952f_9e80228d","line":809,"updated":"2015-09-10 20:43:04.000000000","message":"Blank line.","commit_id":"1efbf11944864039040df37f936dbe18d99aa58e"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"fda133806242acf939887b067f8826c631634618","unresolved":false,"context_lines":[{"line_number":806,"context_line":"        br_int_2 \u003d self._get_agent_ovs_integration_bridge(self.failover_agent)"},{"line_number":807,"context_line":""},{"line_number":808,"context_line":"        veth1, veth2 \u003d self.useFixture(net_helpers.VethFixture()).ports"},{"line_number":809,"context_line":""},{"line_number":810,"context_line":"        br_int_1.add_port(veth1.name)"},{"line_number":811,"context_line":"        br_int_2.add_port(veth2.name)"},{"line_number":812,"context_line":""}],"source_content_type":"text/x-python","patch_set":34,"id":"da20952f_06c59b8c","line":809,"in_reply_to":"da20952f_9e80228d","updated":"2015-09-11 05:34:14.000000000","message":"Done","commit_id":"1efbf11944864039040df37f936dbe18d99aa58e"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":165,"context_line":"        ip \u003d ip_lib.IPWrapper(namespace\u003dnamespace)"},{"line_number":166,"context_line":"        return ip.netns.exists(namespace)"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    def _metadata_proxy_exists(self, conf, router, timeout\u003d0):"},{"line_number":169,"context_line":"        pm \u003d external_process.ProcessManager("},{"line_number":170,"context_line":"            conf,"},{"line_number":171,"context_line":"            router.router_id,"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_4ff32374","line":168,"updated":"2015-09-24 23:40:08.000000000","message":"This change is not needed. See my comment in line 1140.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":1137,"context_line":"            utils.wait_until_true(device_exists)"},{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"        self.assertTrue(self._namespace_exists(router.ns_name))"},{"line_number":1140,"context_line":"        self.assertTrue(self._metadata_proxy_exists(self.agent.conf,"},{"line_number":1141,"context_line":"                                                    router,"},{"line_number":1142,"context_line":"                                                    timeout\u003d3))"},{"line_number":1143,"context_line":"        self._assert_internal_devices(router)"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_cf8653b9","line":1140,"updated":"2015-09-24 23:40:08.000000000","message":"Instead of changing metadata_proxy_exists, we\u0027re better off changing this from assertTrue to wait_until_true like in other calls to metadata_proxy_exists.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":1277,"context_line":"        else:"},{"line_number":1278,"context_line":"            self.assertTrue(False, \" agent not configured for dvr or dvr_snat\")"},{"line_number":1279,"context_line":""},{"line_number":1280,"context_line":"    def _wait_for_device_to_be_configured(self,"},{"line_number":1281,"context_line":"                                          external_port,"},{"line_number":1282,"context_line":"                                          device_name,"},{"line_number":1283,"context_line":"                                          ns_name):"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_8f037b0f","line":1280,"updated":"2015-09-24 23:40:08.000000000","message":"I\u0027m not sure what is the intention here. Wait for something to happen with a maximum of 5 seconds, but don\u0027t error out if 5 seconds have passed? Why does this make sense? I think it would be simpler to remove this method and lines 1258 to 1263, and instead change line 1264 from assertTrue to a wait_until_true.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":1643,"context_line":"        self._assert_no_ip_addresses_on_interface(namespace,"},{"line_number":1644,"context_line":"                                                  ex_gw_port_name)"},{"line_number":1645,"context_line":""},{"line_number":1646,"context_line":"    def fail_dvr_ha_router(self, router):"},{"line_number":1647,"context_line":"        device_name \u003d router.get_ha_device_name()"},{"line_number":1648,"context_line":"        ha_device \u003d ip_lib.IPDevice(device_name, router.snat_ns_name)"},{"line_number":1649,"context_line":"        ha_device.link.set_down()"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_920080ac","line":1646,"updated":"2015-09-24 23:40:08.000000000","message":"We can get rid of this function by changing fail_ha_router to use router.ha_namespace instead of router.ns_name.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":1670,"context_line":"        self._assert_ip_addresses_in_dvr_ha_snat_namespace(router2)"},{"line_number":1671,"context_line":"        self._assert_no_ip_addresses_in_dvr_ha_snat_namespace(router1)"},{"line_number":1672,"context_line":""},{"line_number":1673,"context_line":"    def _assert_dvr_extra_subnet_of_ex_gw(self, router, ip_versions):"},{"line_number":1674,"context_line":"        routes \u003d []"},{"line_number":1675,"context_line":"        ns_name \u003d router.snat_namespace.name"},{"line_number":1676,"context_line":"        for ip_version in ip_versions:"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_1288f0e6","line":1673,"updated":"2015-09-24 23:40:08.000000000","message":"We can get rid of this function with a simple change to _assert_onlink_subnet_routes.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"99c2e5e8fe6d2fd84b8a25c234514dbe82825c83","unresolved":false,"context_lines":[{"line_number":1639,"context_line":"        self._assert_ip_addresses_in_dvr_ha_snat_namespace(router2)"},{"line_number":1640,"context_line":"        self._assert_no_ip_addresses_in_dvr_ha_snat_namespace(router1)"},{"line_number":1641,"context_line":""},{"line_number":1642,"context_line":"    def _assert_dvr_extra_subnet_of_ex_gw(self, router, ip_versions):"},{"line_number":1643,"context_line":"        routes \u003d []"},{"line_number":1644,"context_line":"        ns_name \u003d router.snat_namespace.name"},{"line_number":1645,"context_line":"        for ip_version in ip_versions:"}],"source_content_type":"text/x-python","patch_set":42,"id":"9a1a9d01_68361805","line":1642,"updated":"2015-09-25 00:19:04.000000000","message":"This is not needed.","commit_id":"1ed8239c7f0a750407755a234a2ae54537d7eeb0"}],"neutron/tests/unit/agent/l3/test_dvr_edge_ha_router.py":[{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":32,"context_line":"HOSTNAME \u003d \u0027myhost\u0027"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"class TestDvrEdgeHaRouter(base.BaseTestCase):"},{"line_number":36,"context_line":"    def setUp(self):"},{"line_number":37,"context_line":"        super(TestDvrEdgeHaRouter, self).setUp()"},{"line_number":38,"context_line":"        self.agent \u003d mock.MagicMock()"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_39a90d7c","line":35,"updated":"2015-08-04 19:13:41.000000000","message":"I\u0027d rather get rid of all of these tests (In their current form) than merge them. They test internal method calls instead of a method\u0027s expected output. Please see:\nhttp://assafmuller.com/2015/05/17/testing-lightning-talk/\n\nThe goal is not to test that methods were called. The goal is to check that methods do what they\u0027re supposed to do. There\u0027s no reason to mock internal methods in unit tests apart from mocking system interaction (Such as utils.execute). Call a method, see that it returns what it\u0027s supposed to return given a certain set of input. That can be hard when we\u0027re trying to test an agent, which is the reason I prefer functional to unit tests when testing agents, and the reason I originally added the L3 agent functional tests. These sort of unit tests are not effective, they barely reveal regressions but very often require maintenance whenever you refactor something trivial. They don\u0027t test that the code is doing what it\u0027s supposed to, it\u0027s only verifying that the code is written as it is.\n\nIf you can test a method without mocking the world, fantastic. If you can\u0027t, you need to make the method more testable. Failing that, go a level higher (Functional in this case).","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":61,"context_line":"                              self.mocked_ex_port,"},{"line_number":62,"context_line":"                              \u0027gw_port_host\u0027: self.host"},{"line_number":63,"context_line":"                              }"},{"line_number":64,"context_line":"        self.kwargs \u003d {"},{"line_number":65,"context_line":"            \u0027agent\u0027: self.agent,"},{"line_number":66,"context_line":"            \u0027host\u0027: self.host,"},{"line_number":67,"context_line":"            \u0027state_change_callback\u0027: self.state_change_callback,"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_0e1c0976","line":64,"updated":"2015-08-04 19:13:41.000000000","message":"Can you rename this to something less generic, so that it could be understood what it\u0027s used for?","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":136,"context_line":"        expected_ns_name \u003d str(dvr_snat_ns.SNAT_NS_PREFIX +"},{"line_number":137,"context_line":"                               self.mocked_router[\u0027id\u0027])"},{"line_number":138,"context_line":"        self.assertEqual(expected_ns_name, router.get_snat_ns_name())"},{"line_number":139,"context_line":"        ns_create.assert_called_once_with()"},{"line_number":140,"context_line":""},{"line_number":141,"context_line":"    @mock.patch.object(router_info.RouterInfo, \u0027_internal_network_added\u0027)"},{"line_number":142,"context_line":"    @mock.patch.object(ha_router.HaRouter,"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_9909412f","line":139,"updated":"2015-08-04 19:13:41.000000000","message":"you can instead self.assertTrue(ns_create.called), or self.assertEqual(1, ns_create.call_count)","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"431c7ae89638357eebebf56eedd7c76b3f67d51d","unresolved":false,"context_lines":[{"line_number":292,"context_line":"            mocked_process_monitor)"},{"line_number":293,"context_line":"        self.assertEqual(2, namespace_create.call_count)"},{"line_number":294,"context_line":""},{"line_number":295,"context_line":"    def test__is_this_snat_host(self):"},{"line_number":296,"context_line":"        kwargs \u003d self.kwargs"},{"line_number":297,"context_line":"        router \u003d dvr_edge_ha_router.DvrEdgeHaRouter(**kwargs)"},{"line_number":298,"context_line":"        self.agent_conf.agent_mode \u003d l3_constants.L3_AGENT_MODE_DVR_SNAT"}],"source_content_type":"text/x-python","patch_set":10,"id":"1a4dcd0f_d99b49c8","line":295,"updated":"2015-08-04 19:13:41.000000000","message":"This is the only effective test in this file if you ask me, and even it is questionable as it\u0027s testing a method truly trivial.","commit_id":"94f72231673c4b3fb46f212df8696a73afde58e9"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"477f5136adb4c9b7596532148ff7302304a99c18","unresolved":false,"context_lines":[{"line_number":13,"context_line":"#    under the License."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"import mock"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"from neutron.agent.l3 import dvr_edge_ha_router"},{"line_number":19,"context_line":"from neutron.agent.l3 import dvr_edge_router"}],"source_content_type":"text/x-python","patch_set":19,"id":"fa1b9901_192f793f","line":16,"updated":"2015-08-24 17:43:36.000000000","message":"Think you only need one blank line here.","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":13770,"name":"Adolfo Duarte","email":"adduarte@redhat.com","username":"adolfoduarte"},"change_message_id":"2fe4153b80da6e03d4a8556456d67544cad7b4f5","unresolved":false,"context_lines":[{"line_number":13,"context_line":"#    under the License."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"import mock"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"from neutron.agent.l3 import dvr_edge_ha_router"},{"line_number":19,"context_line":"from neutron.agent.l3 import dvr_edge_router"}],"source_content_type":"text/x-python","patch_set":19,"id":"fa1b9901_e4877810","line":16,"in_reply_to":"fa1b9901_192f793f","updated":"2015-08-24 18:17:44.000000000","message":"will do. \nDone","commit_id":"35c78469b73c3f4e41e30f6d61a7e405806b66e9"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":159,"context_line":""},{"line_number":160,"context_line":"        interface_name \u003d router.get_internal_device_name("},{"line_number":161,"context_line":"            self.port_1_id)"},{"line_number":162,"context_line":"        _disable_ipv6_addressing_on_interface.asser_any_call("},{"line_number":163,"context_line":"            interface_name, router.ns_name)"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"        snat_interface_name \u003d router._get_snat_int_device_name("}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_84ef471c","line":162,"updated":"2015-09-23 21:58:56.000000000","message":"This isn\u0027t failing because it\u0027s calling \u0027asser_*\u0027 and not \u0027assert_*\u0027. If you change it to assert it fails, because the assertion is wrong, there\u0027s no reason to call disable_ipv6 on an interface in the qrouter namespace.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"3422e5e0febec552cbbd3c580460391b9d27778a","unresolved":false,"context_lines":[{"line_number":165,"context_line":"        snat_interface_name \u003d router._get_snat_int_device_name("},{"line_number":166,"context_line":"            self.snat_port_1_id)"},{"line_number":167,"context_line":"        interface_name \u003d router.get_internal_device_name(self.port_1_id)"},{"line_number":168,"context_line":"        _disable_ipv6_addressing_on_interface.asser_any_call("},{"line_number":169,"context_line":"            snat_interface_name, router.snat_ns_name)"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"        ip \u003d self.snat_ports[0][\"fixed_ips\"][0][\u0027ip_address\u0027]"}],"source_content_type":"text/x-python","patch_set":39,"id":"ba15a1d1_a47dab6e","line":168,"updated":"2015-09-23 21:58:56.000000000","message":"asser_any_call just calls a non-existent method on the mock. You probably mean assert_any_call.","commit_id":"20188a399ac725e87862c8a9448229cf81deda85"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"cb0af8a351321a5bc32c545fa65b0ff6d6b49f40","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright (c) 2015 Hewlett-Packard Development Company, L.P."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":40,"id":"9a1a9d01_32aacfd5","line":1,"updated":"2015-09-24 23:40:08.000000000","message":"I gave this some thought. After tinkering a lot with the code, and examining the tests here, I\u0027ve come to the conclusion that there\u0027s not a single test here that brings any value to the table. Only negative value. I\u0027m removing the entire file.","commit_id":"7618667289b6fc994db02518f57abcbc92829f3d"}]}