)]}'
{"neutron/agent/dhcp/agent.py":[{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"b4bd7257028205741fb6a153b6b11bbf39f0ce94","unresolved":false,"context_lines":[{"line_number":727,"context_line":"            self.heartbeat.start(interval\u003dreport_interval)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"    def init_host(self):"},{"line_number":730,"context_line":"        if not self.report_interval:"},{"line_number":731,"context_line":"            self.sync_state()"},{"line_number":732,"context_line":""},{"line_number":733,"context_line":"    def _report_state(self):"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba5201f7_8495a8e5","line":730,"updated":"2017-01-02 16:13:33.000000000","message":"I think this approach is better and you did a good catch realizing that initial sync was being done twice. However, I\u0027d add a comment here explaining why skipping sync if report_interval is set.\nAlso, why not doing the sync here only and remove it from the report_state? Makes more sense to me doing the initial sync from the init_host function. What do you think?","commit_id":"31405481799d300dda6ae016ba16cf043eb12115"},{"author":{"_account_id":8157,"name":"Bertrand LALLAU","email":"bertrand.lallau@gmail.com","username":"blallau"},"change_message_id":"ee7b56efbfd3240378033b5ae27ce0aa2683fedd","unresolved":false,"context_lines":[{"line_number":727,"context_line":"            self.heartbeat.start(interval\u003dreport_interval)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"    def init_host(self):"},{"line_number":730,"context_line":"        if not self.report_interval:"},{"line_number":731,"context_line":"            self.sync_state()"},{"line_number":732,"context_line":""},{"line_number":733,"context_line":"    def _report_state(self):"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba5201f7_676a8682","line":730,"in_reply_to":"ba5201f7_8495a8e5","updated":"2017-01-02 16:44:43.000000000","message":"@Daniel: I agree, I\u0027ll add a comment.\nIt make sense to do the network config reconciliation in a periodic task. That\u0027s why I prefer to let the sync_state call in the report_state method.","commit_id":"31405481799d300dda6ae016ba16cf043eb12115"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"b012b9d771403c2c375d9f67cb8bb5aaa87cbd21","unresolved":false,"context_lines":[{"line_number":729,"context_line":"    def init_host(self):"},{"line_number":730,"context_line":"        # NOTE(blallau): sync_state is performed during _report_state"},{"line_number":731,"context_line":"        # to do network config reconciliation."},{"line_number":732,"context_line":"        # To avoid configuring all networks twice it MUST be skip here"},{"line_number":733,"context_line":"        # when report_interval is \u003e 0."},{"line_number":734,"context_line":"        if not self.conf.AGENT.report_interval:"},{"line_number":735,"context_line":"            self.sync_state()"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba5201f7_dc4db573","line":732,"range":{"start_line":732,"start_character":61,"end_line":732,"end_character":65},"updated":"2017-01-03 12:32:46.000000000","message":"skipped","commit_id":"c8a63491d0716bf2cd767d998f4ed6b0486e5902"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"1035dc750e5829606295780db17585331696dd73","unresolved":false,"context_lines":[{"line_number":113,"context_line":"                self.cache.put(net)"},{"line_number":114,"context_line":"        except NotImplementedError:"},{"line_number":115,"context_line":"            # just go ahead with an empty networks cache"},{"line_number":116,"context_line":"            LOG.debug(\"The \u0027%s\u0027 DHCP-driver does not support retrieving of a \""},{"line_number":117,"context_line":"                      \"list of existing networks\","},{"line_number":118,"context_line":"                      self.conf.dhcp_driver)"},{"line_number":119,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"ba5201f7_44a479a5","line":116,"updated":"2017-01-03 22:43:26.000000000","message":"Removing this call here means that the sync won\u0027t take place in L750 at _report_state() function.\nThis has several considerations to take into account:\n- Currently, _report_state will block until the sync is done. This means that no state reports are sent to server and, if it takes long, \"neutron agent-list\" will show dhcp agent as dead.\n- If you remove this, _report_state() won\u0027t block and will keep reporting it as alive from the very beginning so it\u0027s changing the behavior completely.\n\nWith your changes, dhcp-agent will show as alive so what happens if the initial sync takes a lot (sometimes we see ~2 hours)? I\u0027m not sure if neutron-server will forward messages to this agent and they won\u0027t be processed after the sync\u0027s done. What if there\u0027re more DHCP agents running in other nodes? Maybe showing its state as dead would make neutron server forward messages to other agents and this patch will prevent this from happening...\n\nI\u0027m -1\u0027ing since it\u0027s not clear to me the side effects of removing this line. If you have thought of them please, clarify on the commit message. Also, I\u0027ll try to figure out myself.\nThanks for working on this :)","commit_id":"3d340c69c19b44f2bde1dc8913d943e0fb8dba96"},{"author":{"_account_id":8157,"name":"Bertrand LALLAU","email":"bertrand.lallau@gmail.com","username":"blallau"},"change_message_id":"ee2eb7b2516aff492d65712487a76f88793fc022","unresolved":false,"context_lines":[{"line_number":113,"context_line":"                self.cache.put(net)"},{"line_number":114,"context_line":"        except NotImplementedError:"},{"line_number":115,"context_line":"            # just go ahead with an empty networks cache"},{"line_number":116,"context_line":"            LOG.debug(\"The \u0027%s\u0027 DHCP-driver does not support retrieving of a \""},{"line_number":117,"context_line":"                      \"list of existing networks\","},{"line_number":118,"context_line":"                      self.conf.dhcp_driver)"},{"line_number":119,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"ba5201f7_7abae2c1","line":116,"in_reply_to":"ba5201f7_44a479a5","updated":"2017-01-04 10:31:18.000000000","message":"@Daniel: your following assumption concerning _report_state seems wrong to me. \n\"Currently, _report_state will block until the sync is done. This means that no state reports are sent to server and, if it takes long, neutron agent-list will show dhcp agent as dead.\"\n\nAgent state reports (\u0027report_state\u0027 AMQP call) are sent at the beginning of the _report_state method L733 independently of this patch. So agents are always shown as alive from the very beginning of this method.\nHence this change proposing doesn\u0027t change this behavior.\n\nI think the misunderstanding comes from the _report_state naming.\nActually this method do more than reporting the agent state, this is due to the call to run() method.\n_report_state method actually do \"reporting\" AND \"DHCP state reconciliation\", for my part this method should be renamed.\n\nI don\u0027t think it\u0027s necessary to modify the commit message cause explaining this is really not related to this fix. I hope my answer help, regards.","commit_id":"3d340c69c19b44f2bde1dc8913d943e0fb8dba96"}]}