)]}'
{"neutron/db/agents_db.py":[{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"9cca957d2861264c8573916f3cbcf10c476af992","unresolved":false,"context_lines":[{"line_number":379,"context_line":"            greenthread.sleep(0)"},{"line_number":380,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":381,"context_line":"            if bridge_mappings:"},{"line_number":382,"context_line":"                segments_db.update_segment_host_mapping(context,"},{"line_number":383,"context_line":"                                                        bridge_mappings,"},{"line_number":384,"context_line":"                                                        agent_state[\u0027host\u0027])"},{"line_number":385,"context_line":"        return status"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_7315cee3","line":382,"range":{"start_line":382,"start_character":16,"end_line":382,"end_character":55},"updated":"2016-04-22 23:14:04.000000000","message":"we should be cautious when adding logic here. Just filed https://bugs.launchpad.net/neutron/+bug/1573843. I think no action required here for now, I\u0027ll try to come up with the fix soon and let\u0027s see whether it makes sense to base this patch on it or vice versa","commit_id":"436782f28561c17fb49f41828e0624e2ab95b075"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"a32cee7ee421d3394eee2db370a4dc6c45291acc","unresolved":false,"context_lines":[{"line_number":379,"context_line":"            greenthread.sleep(0)"},{"line_number":380,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":381,"context_line":"            if bridge_mappings:"},{"line_number":382,"context_line":"                segments_db.update_segment_host_mapping(context,"},{"line_number":383,"context_line":"                                                        bridge_mappings,"},{"line_number":384,"context_line":"                                                        agent_state[\u0027host\u0027])"},{"line_number":385,"context_line":"        return status"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_cf6aa980","line":382,"range":{"start_line":382,"start_character":16,"end_line":382,"end_character":55},"in_reply_to":"1a122d0e_7315cee3","updated":"2016-04-28 22:51:13.000000000","message":"Per our conversation, let\u0027s discuss during contributors meet-up in Austin","commit_id":"436782f28561c17fb49f41828e0624e2ab95b075"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d10d1e000de425aaf5daee6c8e25f2905fd919f9","unresolved":false,"context_lines":[{"line_number":380,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":381,"context_line":"            if bridge_mappings:"},{"line_number":382,"context_line":"                segments_db.update_segment_host_mapping(context,"},{"line_number":383,"context_line":"                                                        bridge_mappings,"},{"line_number":384,"context_line":"                                                        agent_state[\u0027host\u0027])"},{"line_number":385,"context_line":"        return status"},{"line_number":386,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_858109c6","line":383,"range":{"start_line":383,"start_character":56,"end_line":383,"end_character":71},"updated":"2016-04-19 14:29:13.000000000","message":"We probably shouldn\u0027t pass this directly.  I think we\u0027ll need to unpack the data structure and pass something more sensible down to the DB method.  It isn\u0027t reasonable to expect a DB method to understand the details of this blob of data.","commit_id":"436782f28561c17fb49f41828e0624e2ab95b075"},{"author":{"_account_id":13667,"name":"ZongKai LI","email":"onionpiece@163.com","username":"lzklibj"},"change_message_id":"ef9e5a6d776282fdc2e11a5cded9e97f1a7504c7","unresolved":false,"context_lines":[{"line_number":380,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":381,"context_line":"            if bridge_mappings:"},{"line_number":382,"context_line":"                segments_db.update_segment_host_mapping(context,"},{"line_number":383,"context_line":"                                                        bridge_mappings,"},{"line_number":384,"context_line":"                                                        agent_state[\u0027host\u0027])"},{"line_number":385,"context_line":"        return status"},{"line_number":386,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_f3ad0785","line":383,"range":{"start_line":383,"start_character":56,"end_line":383,"end_character":71},"in_reply_to":"1a122d0e_858109c6","updated":"2016-04-20 03:10:40.000000000","message":"+1, since only phys_net in bridge_mappings will be used.","commit_id":"436782f28561c17fb49f41828e0624e2ab95b075"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"a32cee7ee421d3394eee2db370a4dc6c45291acc","unresolved":false,"context_lines":[{"line_number":380,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":381,"context_line":"            if bridge_mappings:"},{"line_number":382,"context_line":"                segments_db.update_segment_host_mapping(context,"},{"line_number":383,"context_line":"                                                        bridge_mappings,"},{"line_number":384,"context_line":"                                                        agent_state[\u0027host\u0027])"},{"line_number":385,"context_line":"        return status"},{"line_number":386,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_ef0965de","line":383,"range":{"start_line":383,"start_character":56,"end_line":383,"end_character":71},"in_reply_to":"1a122d0e_858109c6","updated":"2016-04-28 22:51:13.000000000","message":"I am passing the phys nets to the method directly now.","commit_id":"436782f28561c17fb49f41828e0624e2ab95b075"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"ca83f41879c5a25bb5925ccfce57de5efbbbd4f9","unresolved":false,"context_lines":[{"line_number":397,"context_line":"                self._log_heartbeat(agent_state, agent_db, configurations_dict)"},{"line_number":398,"context_line":"                status \u003d n_const.AGENT_NEW"},{"line_number":399,"context_line":"            greenthread.sleep(0)"},{"line_number":400,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":401,"context_line":"            if bridge_mappings:"},{"line_number":402,"context_line":"                kwargs \u003d {\u0027agent\u0027: agent_state}"},{"line_number":403,"context_line":"                segments_db.update_segment_host_mapping(context,"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_99a836dd","line":400,"range":{"start_line":400,"start_character":12,"end_line":400,"end_character":72},"updated":"2016-05-10 08:31:19.000000000","message":"From [1] \u0026 [2] \u0026 [3], the bridge_mapping might comes from other configurations. Are they in consideration?\n\n[1] https://github.com/openstack/neutron/blob/065e5d66b650e336965b5ca425427c5d8e763d52/neutron/plugins/ml2/drivers/linuxbridge/mech_driver/mech_linuxbridge.py#L49-L52\n\n[2] https://github.com/openstack/neutron/blob/065e5d66b650e336965b5ca425427c5d8e763d52/neutron/plugins/ml2/drivers/mech_sriov/mech_driver/mech_driver.py#L167\n\n[3] https://github.com/openstack/neutron/blob/065e5d66b650e336965b5ca425427c5d8e763d52/neutron/plugins/ml2/drivers/macvtap/mech_driver/mech_macvtap.py#L51","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"4c73373680cd1dd715df4ca68d8f2c4cc7fd9e54","unresolved":false,"context_lines":[{"line_number":397,"context_line":"                self._log_heartbeat(agent_state, agent_db, configurations_dict)"},{"line_number":398,"context_line":"                status \u003d n_const.AGENT_NEW"},{"line_number":399,"context_line":"            greenthread.sleep(0)"},{"line_number":400,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":401,"context_line":"            if bridge_mappings:"},{"line_number":402,"context_line":"                kwargs \u003d {\u0027agent\u0027: agent_state}"},{"line_number":403,"context_line":"                segments_db.update_segment_host_mapping(context,"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_14d5031b","line":400,"range":{"start_line":400,"start_character":12,"end_line":400,"end_character":72},"in_reply_to":"dab17558_99a836dd","updated":"2016-05-11 00:51:20.000000000","message":"Yes, we are addressing all of these. Please see my response to Carl to hist comment in this same line","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"4f1eac32c9ec8d299f08b09fa431f920d7105bfb","unresolved":false,"context_lines":[{"line_number":397,"context_line":"                self._log_heartbeat(agent_state, agent_db, configurations_dict)"},{"line_number":398,"context_line":"                status \u003d n_const.AGENT_NEW"},{"line_number":399,"context_line":"            greenthread.sleep(0)"},{"line_number":400,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":401,"context_line":"            if bridge_mappings:"},{"line_number":402,"context_line":"                kwargs \u003d {\u0027agent\u0027: agent_state}"},{"line_number":403,"context_line":"                segments_db.update_segment_host_mapping(context,"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_9f50fa33","line":400,"range":{"start_line":400,"start_character":12,"end_line":400,"end_character":72},"in_reply_to":"dab17558_99a836dd","updated":"2016-05-10 18:18:23.000000000","message":"device_mappings, interface_mappings, bridge_mappings, oh my!\n\nActually, it would be good to understand the relationship between all of these various mappings.","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"4c73373680cd1dd715df4ca68d8f2c4cc7fd9e54","unresolved":false,"context_lines":[{"line_number":397,"context_line":"                self._log_heartbeat(agent_state, agent_db, configurations_dict)"},{"line_number":398,"context_line":"                status \u003d n_const.AGENT_NEW"},{"line_number":399,"context_line":"            greenthread.sleep(0)"},{"line_number":400,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":401,"context_line":"            if bridge_mappings:"},{"line_number":402,"context_line":"                kwargs \u003d {\u0027agent\u0027: agent_state}"},{"line_number":403,"context_line":"                segments_db.update_segment_host_mapping(context,"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_54566bce","line":400,"range":{"start_line":400,"start_character":12,"end_line":400,"end_character":72},"in_reply_to":"dab17558_9f50fa33","updated":"2016-05-11 00:51:20.000000000","message":"These differences in nomenclature reflect the technology differences inherent in the range of mechanism drives that we have in ML2. Here\u0027s the terminology used in each one of them:\n\nLinuxbridge: interface_mappings, bridge_mappings\nMacvtap: interface_mappings\nSR-IOV: device_mappings\nOVS: bridge_mappings\n\nFrom the point of view of the segments host mapping, we just need to be aware of the following:\n\n1) Each mapping is a 2-tuple, where the first element is always a physical network. The second element might be an interface, a device or a bridge. This is relevant because in the segment host mapping we use the physical network to find all the segments mapped to it. We don\u0027t use the second element directly.\n\n2) Since we merged https://review.openstack.org/#/c/312289/, we have a uniform interface in all the ML2 agent based mechanism drivers to decide whether a segment can be bound to a host:\n\ndef check_segment_for_agent(self, segment, agent)\n\nThis is because now all the ML2 agent based mechanism drivers inherit from SimpleAgentMechanismDriverBase. This method in turn uses method get_mappings, which is also mandated by the SimpleAgentMechanismDriverBase interface. These two methods together relieve us from having to worry about the nomenclature differences mentioned above.","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":13667,"name":"ZongKai LI","email":"onionpiece@163.com","username":"lzklibj"},"change_message_id":"5730d3a18bacf2c64ba2502eec14127ef5d9e283","unresolved":false,"context_lines":[{"line_number":397,"context_line":"                self._log_heartbeat(agent_state, agent_db, configurations_dict)"},{"line_number":398,"context_line":"                status \u003d n_const.AGENT_NEW"},{"line_number":399,"context_line":"            greenthread.sleep(0)"},{"line_number":400,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":401,"context_line":"            if bridge_mappings:"},{"line_number":402,"context_line":"                kwargs \u003d {\u0027agent\u0027: agent_state}"},{"line_number":403,"context_line":"                segments_db.update_segment_host_mapping(context,"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_c8e62fe7","line":400,"range":{"start_line":400,"start_character":30,"end_line":400,"end_character":72},"updated":"2016-05-11 07:16:40.000000000","message":"You mentioned method get_mappings in PS8 comments, but I\u0027m afraid it has nothing to do here.\n\nagent_state recevied by method report_state is generated in agent side:\n  for OVSNeutronAgent, it\u0027s \u0027bridge_mappings\u0027,\n  for SriovNicSwitchAgent, it\u0027s \u0027device_mappings\u0027,\n  for LinuxBridgeAgent, it has \u0027bridge_mappings\u0027 and \u0027interface_mappings\u0027,\n  for MacvtapManager, it \u0027interface_mappings\u0027.\n\nI think you can check \"self.agent_state\" for ML2 agents for more details.","commit_id":"5c881e322f09671093fb2129fefbb2fc2936ece5"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"2cb7a2b031ff34c44e82a732d7f9d5ffa69a941b","unresolved":false,"context_lines":[{"line_number":397,"context_line":"                self._log_heartbeat(agent_state, agent_db, configurations_dict)"},{"line_number":398,"context_line":"                status \u003d n_const.AGENT_NEW"},{"line_number":399,"context_line":"            greenthread.sleep(0)"},{"line_number":400,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":401,"context_line":"            if bridge_mappings:"},{"line_number":402,"context_line":"                kwargs \u003d {\u0027agent\u0027: agent_state}"},{"line_number":403,"context_line":"                segments_db.update_segment_host_mapping(context,"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_52d12022","line":400,"range":{"start_line":400,"start_character":30,"end_line":400,"end_character":72},"in_reply_to":"dab17558_41fdd96c","updated":"2016-05-11 22:46:25.000000000","message":"Thanks both for catching this. I intended to make a general explanation and then fix the code. Forgot to fixing part","commit_id":"5c881e322f09671093fb2129fefbb2fc2936ece5"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"652ac773893c6025866984d86f0cbbcfb638f623","unresolved":false,"context_lines":[{"line_number":397,"context_line":"                self._log_heartbeat(agent_state, agent_db, configurations_dict)"},{"line_number":398,"context_line":"                status \u003d n_const.AGENT_NEW"},{"line_number":399,"context_line":"            greenthread.sleep(0)"},{"line_number":400,"context_line":"            bridge_mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027)"},{"line_number":401,"context_line":"            if bridge_mappings:"},{"line_number":402,"context_line":"                kwargs \u003d {\u0027agent\u0027: agent_state}"},{"line_number":403,"context_line":"                segments_db.update_segment_host_mapping(context,"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_41fdd96c","line":400,"range":{"start_line":400,"start_character":30,"end_line":400,"end_character":72},"in_reply_to":"dab17558_c8e62fe7","updated":"2016-05-11 17:53:34.000000000","message":"Actually, I had the same feeling as ZongKai.","commit_id":"5c881e322f09671093fb2129fefbb2fc2936ece5"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"b3e415154e9752ccf412446e9e050fd5bb5b0e7e","unresolved":false,"context_lines":[{"line_number":397,"context_line":"                self._log_heartbeat(agent_state, agent_db, configurations_dict)"},{"line_number":398,"context_line":"                status \u003d n_const.AGENT_NEW"},{"line_number":399,"context_line":"            greenthread.sleep(0)"},{"line_number":400,"context_line":"            segments_db.SegmentHostMappingUpdaterAgent(self).update("},{"line_number":401,"context_line":"                context, agent_state[\u0027host\u0027], agent_state)"},{"line_number":402,"context_line":"        return status"},{"line_number":403,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"dab17558_78708308","line":400,"range":{"start_line":400,"start_character":12,"end_line":400,"end_character":60},"updated":"2016-05-17 20:54:32.000000000","message":"Since you instantiate a new updater each time you call this you aren\u0027t benefiting from separating __init__ from update.","commit_id":"9ad79631762e073a811a723bfe9077b527508349"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"e97072c2b5d4feaa4507437deb3f0130fe91ddb5","unresolved":false,"context_lines":[{"line_number":397,"context_line":"                self._log_heartbeat(agent_state, agent_db, configurations_dict)"},{"line_number":398,"context_line":"                status \u003d n_const.AGENT_NEW"},{"line_number":399,"context_line":"            greenthread.sleep(0)"},{"line_number":400,"context_line":"            segments_db.SegmentHostMappingUpdaterAgent(self).update("},{"line_number":401,"context_line":"                context, agent_state[\u0027host\u0027], agent_state)"},{"line_number":402,"context_line":"        return status"},{"line_number":403,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_de483430","line":400,"range":{"start_line":400,"start_character":12,"end_line":400,"end_character":60},"in_reply_to":"dab17558_78708308","updated":"2016-05-18 22:37:58.000000000","message":"I assume this is why you advised to create a factory method in neutron /neutron/services/segments/db.py. The reason I create it every time is that this being a mixin, its __init__ method is not executed when the plugin is instantiated. I didn\u0027t feel comfortable creating an attribute in the plugin __init__ method to save the updater","commit_id":"9ad79631762e073a811a723bfe9077b527508349"}],"neutron/db/migration/alembic_migrations/versions/newton/contract/8fd3918ef6f4_add_segment_host_mapping.py":[{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"52acbb27b1c92d88d877e5652678e8282e383c1a","unresolved":false,"context_lines":[{"line_number":41,"context_line":"                              nullable\u003dFalse),"},{"line_number":42,"context_line":"                    sa.PrimaryKeyConstraint(\u0027segment_id\u0027),"},{"line_number":43,"context_line":"                    sa.ForeignKeyConstraint([\u0027segment_id\u0027],"},{"line_number":44,"context_line":"                                            [\u0027networksegments.id\u0027],"},{"line_number":45,"context_line":"                                            ondelete\u003d\u0027CASCADE\u0027))"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_9c7ddf70","line":44,"range":{"start_line":44,"start_character":46,"end_line":44,"end_character":61},"updated":"2016-05-09 20:55:29.000000000","message":"Because the networksegments table was renamed in a contract migration, this FK constraint can only be created in a contract migration.\n\nHowever, the table could be created without this FK constraint in the expand branch, and the FK constraint added in a contract migration.\n\nQuestions:\n\n1) How will the feature be affected by not having the constraint and the cascading deletes?\n\n2) What do we do in the contract migration upgrade to add the FK constraint if there are segmenthostmappings records referring to non-existent networksegments records?","commit_id":"84cdd446c74d9387ff9a62c074dc5e4f15e70c65"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"4c5e631b662a679a569da5d8a767891399257b7b","unresolved":false,"context_lines":[{"line_number":28,"context_line":"from alembic import op"},{"line_number":29,"context_line":"import sqlalchemy as sa"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"def upgrade():"},{"line_number":33,"context_line":"    op.create_table(\u0027segmenthostmappings\u0027,"},{"line_number":34,"context_line":"                    sa.Column(\u0027segment_id\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"dab17558_6c51e42f","line":31,"updated":"2016-05-17 21:18:20.000000000","message":"Rebase on https://review.openstack.org/317733 and add this function here:\n\n\ndef contract_creation_exceptions():\n    \"\"\"These elements depend on the networksegments table which was renamed\n    in the contract branch.\"\"\"\n    return {\n        sa.Table: [\u0027segmenthostmappings\u0027],\n        sa.Index: [\u0027segmenthostmappings\u0027]\n    }","commit_id":"9ad79631762e073a811a723bfe9077b527508349"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"e97072c2b5d4feaa4507437deb3f0130fe91ddb5","unresolved":false,"context_lines":[{"line_number":28,"context_line":"from alembic import op"},{"line_number":29,"context_line":"import sqlalchemy as sa"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"def upgrade():"},{"line_number":33,"context_line":"    op.create_table(\u0027segmenthostmappings\u0027,"},{"line_number":34,"context_line":"                    sa.Column(\u0027segment_id\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_531ba01b","line":31,"in_reply_to":"dab17558_6c51e42f","updated":"2016-05-18 22:37:58.000000000","message":"Thanks and done","commit_id":"9ad79631762e073a811a723bfe9077b527508349"}],"neutron/db/migration/alembic_migrations/versions/newton/expand/8fd3918ef6f4_add_segment_host_mapping.py":[{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"d87730978659f8a89043898937267aff897cc766","unresolved":false,"context_lines":[{"line_number":41,"context_line":"                              index\u003dTrue,"},{"line_number":42,"context_line":"                              nullable\u003dFalse),"},{"line_number":43,"context_line":"                    sa.ForeignKeyConstraint([\u0027segment_id\u0027],"},{"line_number":44,"context_line":"                                            [\u0027ml2_network_segments.id\u0027],"},{"line_number":45,"context_line":"                                            ondelete\u003d\u0027CASCADE\u0027))"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba0121b8_461c3712","line":44,"range":{"start_line":44,"start_character":46,"end_line":44,"end_character":66},"updated":"2016-03-30 11:14:37.000000000","message":"networksegments","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"667276433ca01af9c1ab3391a88dc4404364b467","unresolved":false,"context_lines":[{"line_number":41,"context_line":"                              index\u003dTrue,"},{"line_number":42,"context_line":"                              nullable\u003dFalse),"},{"line_number":43,"context_line":"                    sa.ForeignKeyConstraint([\u0027segment_id\u0027],"},{"line_number":44,"context_line":"                                            [\u0027ml2_network_segments.id\u0027],"},{"line_number":45,"context_line":"                                            ondelete\u003d\u0027CASCADE\u0027))"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba0121b8_f180f58d","line":44,"range":{"start_line":44,"start_character":46,"end_line":44,"end_character":66},"in_reply_to":"ba0121b8_461c3712","updated":"2016-04-18 16:30:24.000000000","message":"Done","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"}],"neutron/db/providernet_db.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"eedb9d97ad0650c215628822322321aa89d21ea6","unresolved":false,"context_lines":[{"line_number":60,"context_line":"    description \u003d sa.Column(sa.String(attributes.DESCRIPTION_MAX_LEN))"},{"line_number":61,"context_line":""},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"class SegmentHostMapping(model_base.BASEV2):"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    segment_id \u003d sa.Column(sa.String(36),"},{"line_number":66,"context_line":"                           sa.ForeignKey(\u0027ml2_network_segments.id\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"da0c15f0_b6d12fd0","line":63,"range":{"start_line":63,"start_character":6,"end_line":63,"end_character":24},"updated":"2016-03-25 22:27:39.000000000","message":"We should call this mapping \"PhysicalNetworkHostMapping\" mapping instead of \"SegmentHostMapping\" given the assumptions we\u0027re making.  Then, I think we should probably revisit the decision to do this instead of true \"SegmentHostMapping\".","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"667276433ca01af9c1ab3391a88dc4404364b467","unresolved":false,"context_lines":[{"line_number":60,"context_line":"    description \u003d sa.Column(sa.String(attributes.DESCRIPTION_MAX_LEN))"},{"line_number":61,"context_line":""},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"class SegmentHostMapping(model_base.BASEV2):"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    segment_id \u003d sa.Column(sa.String(36),"},{"line_number":66,"context_line":"                           sa.ForeignKey(\u0027ml2_network_segments.id\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a18252c_f56d5933","line":63,"range":{"start_line":63,"start_character":6,"end_line":63,"end_character":24},"in_reply_to":"9a061dce_66037774","updated":"2016-04-18 16:30:24.000000000","message":"All the in-tree mechanism driver will have a check_segment_for_agent, which is what we need for this to work. In the case of the SR-IOV driver, I am working on a patchset to have it inherit from SimpleAgentMechanismDriverBase, after consulting with irenab. Since Mitaka, this driver requires to run with agent all the time: https://github.com/openstack/neutron/commit/83c9578fcc5d06b634d48e0e186bebd60a60322e. In the case of the l2pop driver, I\u0027ll just add a check_segment_for_agent that will be a nop, like you did in https://review.openstack.org/#/c/205631/33","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"5b849d5d33958f5860d507b8f609a2dace704bc5","unresolved":false,"context_lines":[{"line_number":60,"context_line":"    description \u003d sa.Column(sa.String(attributes.DESCRIPTION_MAX_LEN))"},{"line_number":61,"context_line":""},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"class SegmentHostMapping(model_base.BASEV2):"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    segment_id \u003d sa.Column(sa.String(36),"},{"line_number":66,"context_line":"                           sa.ForeignKey(\u0027ml2_network_segments.id\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a061dce_66037774","line":63,"range":{"start_line":63,"start_character":6,"end_line":63,"end_character":24},"in_reply_to":"da0c15f0_b6d12fd0","updated":"2016-04-04 22:30:28.000000000","message":"Are we sure that PhysicalNetworkHostMapping would work for  mechanisms not inheriting from SimpleAgentMechanismDriverBase[1]?\n\n[1] neutron/plugins/ml2/drivers/mech_agent.py","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"667276433ca01af9c1ab3391a88dc4404364b467","unresolved":false,"context_lines":[{"line_number":60,"context_line":"    description \u003d sa.Column(sa.String(attributes.DESCRIPTION_MAX_LEN))"},{"line_number":61,"context_line":""},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"class SegmentHostMapping(model_base.BASEV2):"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    segment_id \u003d sa.Column(sa.String(36),"},{"line_number":66,"context_line":"                           sa.ForeignKey(\u0027ml2_network_segments.id\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a122d0e_a32bb4ea","line":63,"range":{"start_line":63,"start_character":6,"end_line":63,"end_character":24},"in_reply_to":"da0c15f0_b6d12fd0","updated":"2016-04-18 16:30:24.000000000","message":"I am going to evolve this patchset to remove the 1 to 1 correspondence between segments and physical networks. As you know, I need to update the SR-IOV mechanism driver to get there. So, I propose not to change this now and then change it back again in the near future","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"2246d5abc044256958443b9767d13595d1a7a77f","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            context.session.delete(segment_db)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"def update_segment_host_mapping(context, configurations, host):"},{"line_number":270,"context_line":"    \"\"\"Update the segment host mapping from L2 agents state reports."},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    This method makes two assumptions:"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a1b2928_ff6c530a","line":269,"updated":"2016-04-10 22:35:31.000000000","message":"Can we make it general to use parameter list like \n\ndef update_segment_host_mapping(context, bridge_mappings, host):\n\nFor the agent_less plugin, there might be not configurations.","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"667276433ca01af9c1ab3391a88dc4404364b467","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            context.session.delete(segment_db)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":""},{"line_number":269,"context_line":"def update_segment_host_mapping(context, configurations, host):"},{"line_number":270,"context_line":"    \"\"\"Update the segment host mapping from L2 agents state reports."},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    This method makes two assumptions:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a122d0e_4bbf69d0","line":269,"in_reply_to":"7a1b2928_ff6c530a","updated":"2016-04-18 16:30:24.000000000","message":"It is a good suggestion. Done","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"5b849d5d33958f5860d507b8f609a2dace704bc5","unresolved":false,"context_lines":[{"line_number":274,"context_line":"    1) There is a one-to-one relationship between segments and"},{"line_number":275,"context_line":"    physical_networks."},{"line_number":276,"context_line":""},{"line_number":277,"context_line":"    2) It is called within a DB transaction."},{"line_number":278,"context_line":"    \"\"\""},{"line_number":279,"context_line":"    if not configurations:"},{"line_number":280,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a061dce_267e9fe4","line":277,"updated":"2016-04-04 22:30:28.000000000","message":"IMO, we should avoid to assume how a method will be called and use a subtransaction\n\nThis method is not threadsafe","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"667276433ca01af9c1ab3391a88dc4404364b467","unresolved":false,"context_lines":[{"line_number":274,"context_line":"    1) There is a one-to-one relationship between segments and"},{"line_number":275,"context_line":"    physical_networks."},{"line_number":276,"context_line":""},{"line_number":277,"context_line":"    2) It is called within a DB transaction."},{"line_number":278,"context_line":"    \"\"\""},{"line_number":279,"context_line":"    if not configurations:"},{"line_number":280,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a122d0e_2bae6d85","line":277,"in_reply_to":"9a061dce_267e9fe4","updated":"2016-04-18 16:30:24.000000000","message":"Added subtransaction","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"5b849d5d33958f5860d507b8f609a2dace704bc5","unresolved":false,"context_lines":[{"line_number":278,"context_line":"    \"\"\""},{"line_number":279,"context_line":"    if not configurations:"},{"line_number":280,"context_line":"        return"},{"line_number":281,"context_line":"    bridge_mappings \u003d configurations.get(\u0027bridge_mappings\u0027, {})"},{"line_number":282,"context_line":"    if not bridge_mappings:"},{"line_number":283,"context_line":"        return"},{"line_number":284,"context_line":"    segments_host \u003d _get_segments_for_host(context, host)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a061dce_26305f56","line":281,"range":{"start_line":281,"start_character":60,"end_line":281,"end_character":63},"updated":"2016-04-04 22:30:28.000000000","message":"no need to specify a default value","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"667276433ca01af9c1ab3391a88dc4404364b467","unresolved":false,"context_lines":[{"line_number":278,"context_line":"    \"\"\""},{"line_number":279,"context_line":"    if not configurations:"},{"line_number":280,"context_line":"        return"},{"line_number":281,"context_line":"    bridge_mappings \u003d configurations.get(\u0027bridge_mappings\u0027, {})"},{"line_number":282,"context_line":"    if not bridge_mappings:"},{"line_number":283,"context_line":"        return"},{"line_number":284,"context_line":"    segments_host \u003d _get_segments_for_host(context, host)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a122d0e_abb43df2","line":281,"range":{"start_line":281,"start_character":60,"end_line":281,"end_character":63},"in_reply_to":"9a061dce_26305f56","updated":"2016-04-18 16:30:24.000000000","message":"Not needed anymore","commit_id":"6ddfebae3642f1358e0fd722073ae70c158cf03a"}],"neutron/db/segment_db.py":[{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"c53a80d885b84e72250081f1d93cf7f60380ecdb","unresolved":false,"context_lines":[{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    This method makes two assumptions:"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"    1) There is a one-to-one relationship between segments and"},{"line_number":141,"context_line":"    physical_networks."},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    2) It is called within a DB transaction."},{"line_number":144,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"3afc51ec_3947cabc","line":141,"range":{"start_line":140,"start_character":7,"end_line":141,"end_character":22},"updated":"2016-03-13 12:58:35.000000000","message":"There is no guarantee that this relationship will be one-one. Actually, one physical_network can map to multiple segments. For example, if I have this configuration in my plugin.\n\nnetwork_vlan_ranges \u003d default:1000:4000\n\nI can have these records in my DB:\nMariaDB [neutron]\u003e select * from ml2_network_segments;\n+--------------------------------------+--------------------------------------+--------------+------------------+-----------------+------------+---------------+\n| id                                   | network_id                           | network_type | physical_network | segmentation_id | is_dynamic | segment_index |\n+--------------------------------------+--------------------------------------+--------------+------------------+-----------------+------------+---------------+\n| db1e98af-0668-4fd4-ab5b-6e23330faef1 | d58f2cfe-f582-4845-ad73-38cf8ae9faff | vlan         | default          |            1034 |          0 |             0 |\n| e70fb45c-0063-4658-96db-01f12ac21e02 | 1cd1f760-f7b8-4cb2-9339-cb5986708492 | vlan         | default          |            1033 |          0 |             0","commit_id":"34ee58f5c3d374b2413cbb16e67a9616e0fe6f46"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"2f4413d44eacfb65ebaca715929b6584662e681a","unresolved":false,"context_lines":[{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    This method makes two assumptions:"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"    1) There is a one-to-one relationship between segments and"},{"line_number":141,"context_line":"    physical_networks."},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    2) It is called within a DB transaction."},{"line_number":144,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"1af94dfe_c842acad","line":141,"range":{"start_line":140,"start_character":7,"end_line":141,"end_character":22},"in_reply_to":"3afc51ec_3947cabc","updated":"2016-03-18 20:38:05.000000000","message":"I specifically had a conversation with Carl Baldwin about this about 3 weeks ago. He saiud that we should make this assumption. That is why I specifically added this comment to the docstring. At any rate, let\u0027s discuss this with Carl again during next IRC meeting","commit_id":"34ee58f5c3d374b2413cbb16e67a9616e0fe6f46"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"c53a80d885b84e72250081f1d93cf7f60380ecdb","unresolved":false,"context_lines":[{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    2) It is called within a DB transaction."},{"line_number":144,"context_line":"    \"\"\""},{"line_number":145,"context_line":"    l2_types \u003d [constants.AGENT_TYPE_OVS, constants.AGENT_TYPE_LINUXBRIDGE,"},{"line_number":146,"context_line":"                constants.AGENT_TYPE_OFA]"},{"line_number":147,"context_line":"    if agent_type not in l2_types or not configurations:"},{"line_number":148,"context_line":"        return"},{"line_number":149,"context_line":"    bridge_mappings \u003d configurations.get(\u0027bridge_mappings\u0027, {})"}],"source_content_type":"text/x-python","patch_set":1,"id":"5aef4532_a692cc49","line":146,"range":{"start_line":145,"start_character":0,"end_line":146,"end_character":41},"updated":"2016-03-13 12:58:35.000000000","message":"It is going to be limited by these 3 kinds of agent. That will restrict the functionality.\n\nCan we just count on the \u0027bridge_mappings\u0027 in the configuration to filter out the agent?","commit_id":"34ee58f5c3d374b2413cbb16e67a9616e0fe6f46"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"2f4413d44eacfb65ebaca715929b6584662e681a","unresolved":false,"context_lines":[{"line_number":142,"context_line":""},{"line_number":143,"context_line":"    2) It is called within a DB transaction."},{"line_number":144,"context_line":"    \"\"\""},{"line_number":145,"context_line":"    l2_types \u003d [constants.AGENT_TYPE_OVS, constants.AGENT_TYPE_LINUXBRIDGE,"},{"line_number":146,"context_line":"                constants.AGENT_TYPE_OFA]"},{"line_number":147,"context_line":"    if agent_type not in l2_types or not configurations:"},{"line_number":148,"context_line":"        return"},{"line_number":149,"context_line":"    bridge_mappings \u003d configurations.get(\u0027bridge_mappings\u0027, {})"}],"source_content_type":"text/x-python","patch_set":1,"id":"1af94dfe_b8008402","line":146,"range":{"start_line":145,"start_character":0,"end_line":146,"end_character":41},"in_reply_to":"5aef4532_a692cc49","updated":"2016-03-18 20:38:05.000000000","message":"Yes! Your suggestion is a much sounder approach","commit_id":"34ee58f5c3d374b2413cbb16e67a9616e0fe6f46"}],"neutron/db/segments_db.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d10d1e000de425aaf5daee6c8e25f2905fd919f9","unresolved":false,"context_lines":[{"line_number":53,"context_line":"    segment_index \u003d sa.Column(sa.Integer, nullable\u003dFalse, server_default\u003d\u00270\u0027)"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"class SegmentHostMapping(model_base.BASEV2):"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    segment_id \u003d sa.Column(sa.String(36),"},{"line_number":59,"context_line":"                           sa.ForeignKey(\u0027networksegments.id\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_650f8d88","line":56,"updated":"2016-04-19 14:29:13.000000000","message":"There doesn\u0027t seem to be any primary key in this table.  I would expect segment_id to be a key, I think.","commit_id":"436782f28561c17fb49f41828e0624e2ab95b075"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"a32cee7ee421d3394eee2db370a4dc6c45291acc","unresolved":false,"context_lines":[{"line_number":53,"context_line":"    segment_index \u003d sa.Column(sa.Integer, nullable\u003dFalse, server_default\u003d\u00270\u0027)"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"class SegmentHostMapping(model_base.BASEV2):"},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    segment_id \u003d sa.Column(sa.String(36),"},{"line_number":59,"context_line":"                           sa.ForeignKey(\u0027networksegments.id\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_0407c91c","line":56,"in_reply_to":"1a122d0e_650f8d88","updated":"2016-04-28 22:51:13.000000000","message":"You are right, segment_id should be the primary key","commit_id":"436782f28561c17fb49f41828e0624e2ab95b075"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"baba2da7c4a97ccde37808e47cfcd0897474200a","unresolved":false,"context_lines":[{"line_number":169,"context_line":"    \"\"\"Update the segment host mapping.\"\"\""},{"line_number":170,"context_line":"    if not phys_nets:"},{"line_number":171,"context_line":"        return"},{"line_number":172,"context_line":"    ordered_mech_drivers \u003d plugin.mechanism_manager.ordered_mech_drivers"},{"line_number":173,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":174,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":175,"context_line":"        for phys_net in phys_nets:"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_6ecc0562","line":172,"updated":"2016-05-02 00:29:13.000000000","message":"Is this too \u0027hacky\u0027. The Ml2 mechanism manager doesn\u0027t have public method to retrieve the list of configured mechanism drivers. So I go directly to the attribute that point to that list. This way, I can call check_segment_for_agent in line 181 below","commit_id":"012487c42e332e4624e2f945b97098f9279e937d"},{"author":{"_account_id":11762,"name":"Han Zhou","email":"zhouhan@gmail.com","username":"hanzhou"},"change_message_id":"d005d97ac94831aef6d67956c97825743c029440","unresolved":false,"context_lines":[{"line_number":169,"context_line":"    \"\"\"Update the segment host mapping.\"\"\""},{"line_number":170,"context_line":"    if not phys_nets:"},{"line_number":171,"context_line":"        return"},{"line_number":172,"context_line":"    ordered_mech_drivers \u003d plugin.mechanism_manager.ordered_mech_drivers"},{"line_number":173,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":174,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":175,"context_line":"        for phys_net in phys_nets:"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_811c946b","line":172,"in_reply_to":"1a122d0e_17a9f97b","updated":"2016-05-03 23:57:03.000000000","message":"Yes, and even \"agent\" in the parameter is questionable because other plugins such as OVN may not have agent at all.","commit_id":"012487c42e332e4624e2f945b97098f9279e937d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"b7ca67eea7c2b706a07830efc9c642946892bbf6","unresolved":false,"context_lines":[{"line_number":169,"context_line":"    \"\"\"Update the segment host mapping.\"\"\""},{"line_number":170,"context_line":"    if not phys_nets:"},{"line_number":171,"context_line":"        return"},{"line_number":172,"context_line":"    ordered_mech_drivers \u003d plugin.mechanism_manager.ordered_mech_drivers"},{"line_number":173,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":174,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":175,"context_line":"        for phys_net in phys_nets:"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_17a9f97b","line":172,"in_reply_to":"1a122d0e_6ecc0562","updated":"2016-05-03 19:33:51.000000000","message":"This does seem to be ML2 internals leaking in to the core.  We\u0027ll have to add some sort of interface here that other plugins, like OVN, can also implement.  It should be optional so that this code does the right thing when the plugin does not implement it.","commit_id":"012487c42e332e4624e2f945b97098f9279e937d"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"7d465e671e99f813d5c01763e7453cc91e8fefd4","unresolved":false,"context_lines":[{"line_number":169,"context_line":"    \"\"\"Update the segment host mapping.\"\"\""},{"line_number":170,"context_line":"    if not phys_nets:"},{"line_number":171,"context_line":"        return"},{"line_number":172,"context_line":"    ordered_mech_drivers \u003d plugin.mechanism_manager.ordered_mech_drivers"},{"line_number":173,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":174,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":175,"context_line":"        for phys_net in phys_nets:"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_613b4ac9","line":172,"in_reply_to":"1a122d0e_6ecc0562","updated":"2016-05-03 15:23:39.000000000","message":"Yeah this will only work for ML2, but it needs to work for any plugins that want to use segments.  There probably needs to be an optionally implemented method added to the plugin interface called check_segment_for_agent(), just like you call below.  ML2 will implement this method and loop through its mech drivers calling each drivers\u0027 check_segment_for_agent method.\n\nMight be a better way to do this but this is just a quick suggestion, I\u0027ll give it more thought later.  Maybe someone else has a better solution.","commit_id":"012487c42e332e4624e2f945b97098f9279e937d"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"a541c17b67c04efb9b05141160beaf1e36d7759b","unresolved":false,"context_lines":[{"line_number":169,"context_line":"    \"\"\"Update the segment host mapping.\"\"\""},{"line_number":170,"context_line":"    if not phys_nets:"},{"line_number":171,"context_line":"        return"},{"line_number":172,"context_line":"    ordered_mech_drivers \u003d plugin.mechanism_manager.ordered_mech_drivers"},{"line_number":173,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":174,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":175,"context_line":"        for phys_net in phys_nets:"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_5de98b7e","line":172,"in_reply_to":"1a122d0e_811c946b","updated":"2016-05-04 22:32:57.000000000","message":"Next revision removes this and adds abstraction","commit_id":"012487c42e332e4624e2f945b97098f9279e937d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"edfbc1f624fdd7cd1433b3c29943f7c2396b389e","unresolved":false,"context_lines":[{"line_number":180,"context_line":"                for driver in ordered_mech_drivers:"},{"line_number":181,"context_line":"                    if driver.obj.check_segment_for_agent(segment, agent):"},{"line_number":182,"context_line":"                        can_be_bound_to_agent \u003d True"},{"line_number":183,"context_line":"                        break"},{"line_number":184,"context_line":"                if can_be_bound_to_agent:"},{"line_number":185,"context_line":"                    if segment[\u0027id\u0027] in segments_host:"},{"line_number":186,"context_line":"                        segments_host.pop(segment[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_2612c8b4","line":183,"range":{"start_line":183,"start_character":24,"end_line":183,"end_character":29},"updated":"2016-05-04 19:42:37.000000000","message":"why not just move the code on L185 - L189 under this if?  You can get rid of \"can_be_bound_to_agent\" altogether.","commit_id":"012487c42e332e4624e2f945b97098f9279e937d"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"a541c17b67c04efb9b05141160beaf1e36d7759b","unresolved":false,"context_lines":[{"line_number":180,"context_line":"                for driver in ordered_mech_drivers:"},{"line_number":181,"context_line":"                    if driver.obj.check_segment_for_agent(segment, agent):"},{"line_number":182,"context_line":"                        can_be_bound_to_agent \u003d True"},{"line_number":183,"context_line":"                        break"},{"line_number":184,"context_line":"                if can_be_bound_to_agent:"},{"line_number":185,"context_line":"                    if segment[\u0027id\u0027] in segments_host:"},{"line_number":186,"context_line":"                        segments_host.pop(segment[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_ddf59b51","line":183,"range":{"start_line":183,"start_character":24,"end_line":183,"end_character":29},"in_reply_to":"1a122d0e_2612c8b4","updated":"2016-05-04 22:32:57.000000000","message":"Done","commit_id":"012487c42e332e4624e2f945b97098f9279e937d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"edfbc1f624fdd7cd1433b3c29943f7c2396b389e","unresolved":false,"context_lines":[{"line_number":187,"context_line":"                    else:"},{"line_number":188,"context_line":"                        context.session.add(SegmentHostMapping("},{"line_number":189,"context_line":"                            segment_id\u003dsegment[\u0027id\u0027], host\u003dhost))"},{"line_number":190,"context_line":"        greenthread.sleep(0)"},{"line_number":191,"context_line":"        for seg_host in segments_host:"},{"line_number":192,"context_line":"            context.session.delete(seg_host)"},{"line_number":193,"context_line":"        greenthread.sleep(0)"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_c6d4b4ea","line":190,"range":{"start_line":190,"start_character":8,"end_line":190,"end_character":28},"updated":"2016-05-04 19:42:37.000000000","message":"What are these sleeps for?","commit_id":"012487c42e332e4624e2f945b97098f9279e937d"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"a541c17b67c04efb9b05141160beaf1e36d7759b","unresolved":false,"context_lines":[{"line_number":187,"context_line":"                    else:"},{"line_number":188,"context_line":"                        context.session.add(SegmentHostMapping("},{"line_number":189,"context_line":"                            segment_id\u003dsegment[\u0027id\u0027], host\u003dhost))"},{"line_number":190,"context_line":"        greenthread.sleep(0)"},{"line_number":191,"context_line":"        for seg_host in segments_host:"},{"line_number":192,"context_line":"            context.session.delete(seg_host)"},{"line_number":193,"context_line":"        greenthread.sleep(0)"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_fd8fb7ca","line":190,"range":{"start_line":190,"start_character":8,"end_line":190,"end_character":28},"in_reply_to":"1a122d0e_c6d4b4ea","updated":"2016-05-04 22:32:57.000000000","message":"I saw in agents_db that they yield using this method. I can remove it","commit_id":"012487c42e332e4624e2f945b97098f9279e937d"},{"author":{"_account_id":11762,"name":"Han Zhou","email":"zhouhan@gmail.com","username":"hanzhou"},"change_message_id":"003a77a88b9fa21254f8865086e183384d711786","unresolved":false,"context_lines":[{"line_number":194,"context_line":"        return"},{"line_number":195,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":196,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        for phys_net in phys_nets:"},{"line_number":198,"context_line":"            segments \u003d context.session.query(NetworkSegment).filter_by("},{"line_number":199,"context_line":"                physical_network\u003dphys_net).all()"},{"line_number":200,"context_line":"            for segment in segments:"},{"line_number":201,"context_line":"                if plugin.check_segment_for_host(segment, **kwargs):"},{"line_number":202,"context_line":"                    if segment[\u0027id\u0027] in segments_host:"},{"line_number":203,"context_line":"                        segments_host.pop(segment[\u0027id\u0027])"},{"line_number":204,"context_line":"                    else:"},{"line_number":205,"context_line":"                        context.session.add(SegmentHostMapping("},{"line_number":206,"context_line":"                            segment_id\u003dsegment[\u0027id\u0027], host\u003dhost))"},{"line_number":207,"context_line":"        greenthread.sleep(0)"},{"line_number":208,"context_line":"        for seg_host in segments_host.values():"},{"line_number":209,"context_line":"            context.session.delete(seg_host)"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_02786629","line":206,"range":{"start_line":197,"start_character":0,"end_line":206,"end_character":65},"updated":"2016-05-09 22:13:28.000000000","message":"This is trying to bind the host to all the segments that are available for the given phys_nets, if the plugin.check_segment_for_host returns true. \n\nNot sure if I understand this correctly, but how could the plugin know if a segment is should be bound to a host or not? Is there any examples? In the ML2 plugin code in this patch it is mechanism driver that decides, but still, that seems confusing to me. \n\nWould it be better to be able to specify host-segment bindings directly from the API? For example, the cloud admin want a host to be bound only to the VLANs according to the tenants it is allowed to service.","commit_id":"84cdd446c74d9387ff9a62c074dc5e4f15e70c65"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"ca83f41879c5a25bb5925ccfce57de5efbbbd4f9","unresolved":false,"context_lines":[{"line_number":178,"context_line":"     be bound to host"},{"line_number":179,"context_line":"    :type phys_nets: List of Strings"},{"line_number":180,"context_line":"    :param host: the host that will be checked to be bound to the segments"},{"line_number":181,"context_line":"    :type dns_name: String"},{"line_number":182,"context_line":"    :param plugin: the core plug-in"},{"line_number":183,"context_line":"    :type records: Neutron plug-in object"},{"line_number":184,"context_line":"    :param kwargs: additional arguments that method \u0027check_segment_for_host\u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_87429a66","line":181,"range":{"start_line":181,"start_character":10,"end_line":181,"end_character":18},"updated":"2016-05-10 08:31:19.000000000","message":"I think you want to type \"host\" here.","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"4c73373680cd1dd715df4ca68d8f2c4cc7fd9e54","unresolved":false,"context_lines":[{"line_number":178,"context_line":"     be bound to host"},{"line_number":179,"context_line":"    :type phys_nets: List of Strings"},{"line_number":180,"context_line":"    :param host: the host that will be checked to be bound to the segments"},{"line_number":181,"context_line":"    :type dns_name: String"},{"line_number":182,"context_line":"    :param plugin: the core plug-in"},{"line_number":183,"context_line":"    :type records: Neutron plug-in object"},{"line_number":184,"context_line":"    :param kwargs: additional arguments that method \u0027check_segment_for_host\u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_b49a376c","line":181,"range":{"start_line":181,"start_character":10,"end_line":181,"end_character":18},"in_reply_to":"dab17558_87429a66","updated":"2016-05-11 00:51:20.000000000","message":"Done","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"ca83f41879c5a25bb5925ccfce57de5efbbbd4f9","unresolved":false,"context_lines":[{"line_number":180,"context_line":"    :param host: the host that will be checked to be bound to the segments"},{"line_number":181,"context_line":"    :type dns_name: String"},{"line_number":182,"context_line":"    :param plugin: the core plug-in"},{"line_number":183,"context_line":"    :type records: Neutron plug-in object"},{"line_number":184,"context_line":"    :param kwargs: additional arguments that method \u0027check_segment_for_host\u0027"},{"line_number":185,"context_line":"     needs to establish if host is bound to a given segment"},{"line_number":186,"context_line":"    :type kwargs: Arguments Dictionary"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_c7602204","line":183,"range":{"start_line":183,"start_character":10,"end_line":183,"end_character":17},"updated":"2016-05-10 08:31:19.000000000","message":"\"plugin\"?","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"4c73373680cd1dd715df4ca68d8f2c4cc7fd9e54","unresolved":false,"context_lines":[{"line_number":180,"context_line":"    :param host: the host that will be checked to be bound to the segments"},{"line_number":181,"context_line":"    :type dns_name: String"},{"line_number":182,"context_line":"    :param plugin: the core plug-in"},{"line_number":183,"context_line":"    :type records: Neutron plug-in object"},{"line_number":184,"context_line":"    :param kwargs: additional arguments that method \u0027check_segment_for_host\u0027"},{"line_number":185,"context_line":"     needs to establish if host is bound to a given segment"},{"line_number":186,"context_line":"    :type kwargs: Arguments Dictionary"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_f48e7f2c","line":183,"range":{"start_line":183,"start_character":10,"end_line":183,"end_character":17},"in_reply_to":"dab17558_c7602204","updated":"2016-05-11 00:51:20.000000000","message":"Done","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"4f1eac32c9ec8d299f08b09fa431f920d7105bfb","unresolved":false,"context_lines":[{"line_number":193,"context_line":"                     \"a hosts segments mapping\"))"},{"line_number":194,"context_line":"        return"},{"line_number":195,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":196,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        for phys_net in phys_nets:"},{"line_number":198,"context_line":"            segments \u003d context.session.query(NetworkSegment).filter_by("},{"line_number":199,"context_line":"                physical_network\u003dphys_net).all()"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_ba322ca5","line":196,"range":{"start_line":196,"start_character":24,"end_line":196,"end_character":46},"updated":"2016-05-10 18:18:23.000000000","message":"I\u0027m trying to think of how to make this as efficient as possible while making the code easy to follow.\n\nIn the common case, no changes will be made to the DB.  But, we need to do this query at least to know that no changes are needed.  So, I guess my best suggestion would be to shoot for a pattern like this (using sets):\n\n  previous_segment_ids \u003d _get_segments_for_host(...)\n  current_segment_ids \u003d {segment[\u0027id\u0027] for segment in segments\n                         if plugin.check_segment_for_host(segment, **kwargs)}\n  for segment_id in current_segment_ids - previous_segment_ids:\n      context.session.add(SegmentHostMapping(...))\n  stale_segment_ids \u003d previous_segment_ids - current_segment_ids\n  context.session.query(SegmentHostMapping).filter_by(IN stale_segment_ids).delete()","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"4c73373680cd1dd715df4ca68d8f2c4cc7fd9e54","unresolved":false,"context_lines":[{"line_number":193,"context_line":"                     \"a hosts segments mapping\"))"},{"line_number":194,"context_line":"        return"},{"line_number":195,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":196,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        for phys_net in phys_nets:"},{"line_number":198,"context_line":"            segments \u003d context.session.query(NetworkSegment).filter_by("},{"line_number":199,"context_line":"                physical_network\u003dphys_net).all()"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_cd912af3","line":196,"range":{"start_line":196,"start_character":24,"end_line":196,"end_character":46},"in_reply_to":"dab17558_ba322ca5","updated":"2016-05-11 00:51:20.000000000","message":"Done","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"4f1eac32c9ec8d299f08b09fa431f920d7105bfb","unresolved":false,"context_lines":[{"line_number":195,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":196,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        for phys_net in phys_nets:"},{"line_number":198,"context_line":"            segments \u003d context.session.query(NetworkSegment).filter_by("},{"line_number":199,"context_line":"                physical_network\u003dphys_net).all()"},{"line_number":200,"context_line":"            for segment in segments:"},{"line_number":201,"context_line":"                if plugin.check_segment_for_host(segment, **kwargs):"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_7ab9e414","line":198,"range":{"start_line":198,"start_character":39,"end_line":198,"end_character":44},"updated":"2016-05-10 18:18:23.000000000","message":"This can be done all in one query by querying for segments IN phys_nets.  It might be a little more efficient by avoid round trips to the DB.","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"4c73373680cd1dd715df4ca68d8f2c4cc7fd9e54","unresolved":false,"context_lines":[{"line_number":195,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":196,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        for phys_net in phys_nets:"},{"line_number":198,"context_line":"            segments \u003d context.session.query(NetworkSegment).filter_by("},{"line_number":199,"context_line":"                physical_network\u003dphys_net).all()"},{"line_number":200,"context_line":"            for segment in segments:"},{"line_number":201,"context_line":"                if plugin.check_segment_for_host(segment, **kwargs):"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_ad8e66cd","line":198,"range":{"start_line":198,"start_character":39,"end_line":198,"end_character":44},"in_reply_to":"dab17558_7ab9e414","updated":"2016-05-11 00:51:20.000000000","message":"Done","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"4f1eac32c9ec8d299f08b09fa431f920d7105bfb","unresolved":false,"context_lines":[{"line_number":196,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        for phys_net in phys_nets:"},{"line_number":198,"context_line":"            segments \u003d context.session.query(NetworkSegment).filter_by("},{"line_number":199,"context_line":"                physical_network\u003dphys_net).all()"},{"line_number":200,"context_line":"            for segment in segments:"},{"line_number":201,"context_line":"                if plugin.check_segment_for_host(segment, **kwargs):"},{"line_number":202,"context_line":"                    if segment[\u0027id\u0027] in segments_host:"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_709aa56f","line":199,"range":{"start_line":199,"start_character":42,"end_line":199,"end_character":48},"updated":"2016-05-10 18:18:23.000000000","message":"Typically, we leave .all() off.  You can iterate segments without it.","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"4c73373680cd1dd715df4ca68d8f2c4cc7fd9e54","unresolved":false,"context_lines":[{"line_number":196,"context_line":"        segments_host \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        for phys_net in phys_nets:"},{"line_number":198,"context_line":"            segments \u003d context.session.query(NetworkSegment).filter_by("},{"line_number":199,"context_line":"                physical_network\u003dphys_net).all()"},{"line_number":200,"context_line":"            for segment in segments:"},{"line_number":201,"context_line":"                if plugin.check_segment_for_host(segment, **kwargs):"},{"line_number":202,"context_line":"                    if segment[\u0027id\u0027] in segments_host:"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_0dbe9264","line":199,"range":{"start_line":199,"start_character":42,"end_line":199,"end_character":48},"in_reply_to":"dab17558_709aa56f","updated":"2016-05-11 00:51:20.000000000","message":"Done","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":11762,"name":"Han Zhou","email":"zhouhan@gmail.com","username":"hanzhou"},"change_message_id":"313aa270141e9577dcd1bde3d0e3d02fcc4c35bd","unresolved":false,"context_lines":[{"line_number":169,"context_line":"    \"\"\"Update the segments host mapping."},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    This method relies on the core plugin to implement a"},{"line_number":172,"context_line":"    \u0027check_segment_for_host\u0027 method to establish whether the host is bound to a"},{"line_number":173,"context_line":"    given segment"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    :param context: neutron api request context"}],"source_content_type":"text/x-python","patch_set":10,"id":"dab17558_bd877d5f","line":172,"range":{"start_line":172,"start_character":5,"end_line":172,"end_character":27},"updated":"2016-05-11 23:25:03.000000000","message":"It seems this is renamed to check_segment_for_agent, so the comment need to be updated, too.\n\nHowever, is this function \"update_segment_host_mapping\" supposed to be called only by agent based plugins? If it is supposed to be more general purpose, to be reused by plugins not relying on agents (e.g. OVN, which can get physnet-host mapping from centralized DB), then it is better to keep the original name \"check_segment_for_host\".\n\nIf it is only for agent based implementations, would it be better to change this function name itself to something like: update_segment_host_mapping_from_agent(), and agent can be specified as a mandatory parameter.","commit_id":"ea3ef49ca9d0a8eb13be44263e359d97ee987e92"},{"author":{"_account_id":11762,"name":"Han Zhou","email":"zhouhan@gmail.com","username":"hanzhou"},"change_message_id":"093d13e88b0f60a2261e83cd9e8e39927ba1afef","unresolved":false,"context_lines":[{"line_number":169,"context_line":"    \"\"\"Update the segments host mapping."},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    This method relies on the core plugin to implement a"},{"line_number":172,"context_line":"    \u0027check_segment_for_host\u0027 method to establish whether the host is bound to a"},{"line_number":173,"context_line":"    given segment"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    :param context: neutron api request context"}],"source_content_type":"text/x-python","patch_set":10,"id":"dab17558_e143e46a","line":172,"range":{"start_line":172,"start_character":5,"end_line":172,"end_character":27},"in_reply_to":"dab17558_a15f1c74","updated":"2016-05-12 23:40:06.000000000","message":"Miguel, this sounds good to me!","commit_id":"ea3ef49ca9d0a8eb13be44263e359d97ee987e92"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"432fe4ec4a06dc0b7d81028f8263418af125692a","unresolved":false,"context_lines":[{"line_number":169,"context_line":"    \"\"\"Update the segments host mapping."},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    This method relies on the core plugin to implement a"},{"line_number":172,"context_line":"    \u0027check_segment_for_host\u0027 method to establish whether the host is bound to a"},{"line_number":173,"context_line":"    given segment"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    :param context: neutron api request context"}],"source_content_type":"text/x-python","patch_set":10,"id":"dab17558_a15f1c74","line":172,"range":{"start_line":172,"start_character":5,"end_line":172,"end_character":27},"in_reply_to":"dab17558_bd877d5f","updated":"2016-05-12 23:32:47.000000000","message":"Would it make sense to split this function in two:\n\n1) def _update_segment_host_mapping(context, segments, host) that takes as input a host and the segments that have been confirmed to be bound to that host. This function is only responsible of the SegmentHostMapping table with the given inputs and can be leveraged by many plugins.\n\n2) def update_segment_host_mapping_for_agent(context, mappings, host, plugin, agent, **kwargs) that is used by agent based plugins and calls the function in point 1. Other plugins might need to add equivalent functions depending on their implementation requirementes.\n\nWe could even encapsulate this two functions in a class, different plugins can inherit from it","commit_id":"ea3ef49ca9d0a8eb13be44263e359d97ee987e92"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"cee20454c3d2fbc12e3d45397b091051db9abfa1","unresolved":false,"context_lines":[{"line_number":169,"context_line":"    \"\"\"Update the segments host mapping."},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    This method relies on the core plugin to implement a"},{"line_number":172,"context_line":"    \u0027check_segment_for_host\u0027 method to establish whether the host is bound to a"},{"line_number":173,"context_line":"    given segment"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    :param context: neutron api request context"}],"source_content_type":"text/x-python","patch_set":10,"id":"dab17558_c41fc959","line":172,"range":{"start_line":172,"start_character":5,"end_line":172,"end_character":27},"in_reply_to":"dab17558_e143e46a","updated":"2016-05-13 02:59:09.000000000","message":"Sounds good.","commit_id":"ea3ef49ca9d0a8eb13be44263e359d97ee987e92"},{"author":{"_account_id":11762,"name":"Han Zhou","email":"zhouhan@gmail.com","username":"hanzhou"},"change_message_id":"313aa270141e9577dcd1bde3d0e3d02fcc4c35bd","unresolved":false,"context_lines":[{"line_number":188,"context_line":"    if not phys_nets:"},{"line_number":189,"context_line":"        return"},{"line_number":190,"context_line":"    if not hasattr(plugin, \u0027check_segment_for_agent\u0027):"},{"line_number":191,"context_line":"        LOG.info(_LI(\"Core plug-in doesn\u0027t implement \""},{"line_number":192,"context_line":"                     \"\u0027check_segment_for_host\u0027. It is not possible to build \""},{"line_number":193,"context_line":"                     \"a hosts segments mapping\"))"},{"line_number":194,"context_line":"        return"},{"line_number":195,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":196,"context_line":"        previous_segment_ids \u003d _get_segments_for_host(context, host)"}],"source_content_type":"text/x-python","patch_set":10,"id":"dab17558_bdee5d1f","line":193,"range":{"start_line":191,"start_character":22,"end_line":193,"end_character":47},"updated":"2016-05-11 23:25:03.000000000","message":"Again, this is valid for agent-based plugins, but for other plugins, they should be allowed to create/update/delete segment-host mappings directly. For example, a plugin can be implemented in a way that when host-segment mapping creation is requested, it programs the hardware switch to add VLAN binding to the host. So I would suggest to either make this function more general purpose, or rename it to reflect that it is only for agent based solutions, and add new functions to support other use cases.","commit_id":"ea3ef49ca9d0a8eb13be44263e359d97ee987e92"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6f68bc63c2070263b6b147179fb57d4cca381b16","unresolved":false,"context_lines":[{"line_number":48,"context_line":"    segment_index \u003d sa.Column(sa.Integer, nullable\u003dFalse, server_default\u003d\u00270\u0027)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"class SegmentHostMapping(model_base.BASEV2):"},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"    segment_id \u003d sa.Column(sa.String(36),"},{"line_number":54,"context_line":"                           sa.ForeignKey(\u0027networksegments.id\u0027,"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_60aff394","line":51,"range":{"start_line":51,"start_character":6,"end_line":51,"end_character":24},"updated":"2016-05-12 23:35:27.000000000","message":"My comment on L82 got me thinking that maybe this should go in neutron/services/segments/db.py.  What you you think?  I tend to think so since it is that service plugin that exposes the segments as an object we can map to.","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"70f58d877d71d9986b18a0f93cec3ca5c25b562c","unresolved":false,"context_lines":[{"line_number":48,"context_line":"    segment_index \u003d sa.Column(sa.Integer, nullable\u003dFalse, server_default\u003d\u00270\u0027)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"class SegmentHostMapping(model_base.BASEV2):"},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"    segment_id \u003d sa.Column(sa.String(36),"},{"line_number":54,"context_line":"                           sa.ForeignKey(\u0027networksegments.id\u0027,"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_0a1b4f74","line":51,"range":{"start_line":51,"start_character":6,"end_line":51,"end_character":24},"in_reply_to":"dab17558_60aff394","updated":"2016-05-16 00:26:50.000000000","message":"I agree. Actually I moved this class and the entire segment host mapping class to that module","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"771eb35983a7401ae8591f086a42b671bff2511b","unresolved":false,"context_lines":[{"line_number":48,"context_line":"    segment_index \u003d sa.Column(sa.Integer, nullable\u003dFalse, server_default\u003d\u00270\u0027)"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"class SegmentHostMapping(model_base.BASEV2):"},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"    segment_id \u003d sa.Column(sa.String(36),"},{"line_number":54,"context_line":"                           sa.ForeignKey(\u0027networksegments.id\u0027,"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_0720b348","line":51,"range":{"start_line":51,"start_character":6,"end_line":51,"end_character":24},"in_reply_to":"dab17558_60aff394","updated":"2016-05-13 05:34:20.000000000","message":"I\u0027d have to agree with you","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"f21207f2bb6758f30b9de2e935955b0f5ab314d6","unresolved":false,"context_lines":[{"line_number":64,"context_line":"    # Add a relationship to the NetworkSegment model in order to instruct"},{"line_number":65,"context_line":"    # SQLAlchemy to eagerly load this association"},{"line_number":66,"context_line":"    network_segment \u003d orm.relationship("},{"line_number":67,"context_line":"        NetworkSegment, backref\u003dorm.backref(\"segment_host_mapping\","},{"line_number":68,"context_line":"                                            lazy\u003d\u0027joined\u0027,"},{"line_number":69,"context_line":"                                            cascade\u003d\u0027delete\u0027))"},{"line_number":70,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_8f696828","line":67,"range":{"start_line":67,"start_character":45,"end_line":67,"end_character":65},"updated":"2016-05-12 20:29:28.000000000","message":"would it be better to just call it \"host_mapping\" so its not so redundant?","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"f21207f2bb6758f30b9de2e935955b0f5ab314d6","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    return {\u0027id\u0027: record.id,"},{"line_number":80,"context_line":"            NETWORK_TYPE: record.network_type,"},{"line_number":81,"context_line":"            PHYSICAL_NETWORK: record.physical_network,"},{"line_number":82,"context_line":"            SEGMENTATION_ID: record.segmentation_id}"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"def add_network_segment(session, network_id, segment, segment_index\u003d0,"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_6f43646d","line":82,"updated":"2016-05-12 20:29:28.000000000","message":"might want to add the relationship of teh host mappings here as well.  I\u0027m doing it in my review but this seems to be the proper place to do it.","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"70f58d877d71d9986b18a0f93cec3ca5c25b562c","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    return {\u0027id\u0027: record.id,"},{"line_number":80,"context_line":"            NETWORK_TYPE: record.network_type,"},{"line_number":81,"context_line":"            PHYSICAL_NETWORK: record.physical_network,"},{"line_number":82,"context_line":"            SEGMENTATION_ID: record.segmentation_id}"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"def add_network_segment(session, network_id, segment, segment_index\u003d0,"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_4a4ef76f","line":82,"in_reply_to":"dab17558_00c8a773","updated":"2016-05-16 00:26:50.000000000","message":"I\u0027ll do this in my next revision. Tonight I am running out of time","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"771eb35983a7401ae8591f086a42b671bff2511b","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    return {\u0027id\u0027: record.id,"},{"line_number":80,"context_line":"            NETWORK_TYPE: record.network_type,"},{"line_number":81,"context_line":"            PHYSICAL_NETWORK: record.physical_network,"},{"line_number":82,"context_line":"            SEGMENTATION_ID: record.segmentation_id}"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"def add_network_segment(session, network_id, segment, segment_index\u003d0,"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_270057aa","line":82,"in_reply_to":"dab17558_00c8a773","updated":"2016-05-13 05:34:20.000000000","message":"yeah that should be fine, good idea","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6f68bc63c2070263b6b147179fb57d4cca381b16","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    return {\u0027id\u0027: record.id,"},{"line_number":80,"context_line":"            NETWORK_TYPE: record.network_type,"},{"line_number":81,"context_line":"            PHYSICAL_NETWORK: record.physical_network,"},{"line_number":82,"context_line":"            SEGMENTATION_ID: record.segmentation_id}"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"def add_network_segment(session, network_id, segment, segment_index\u003d0,"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_00c8a773","line":82,"in_reply_to":"dab17558_6f43646d","updated":"2016-05-12 23:35:27.000000000","message":"I\u0027m not sure we should touch this one.  What about the _make_segment_dict in neutron/services/segments/db.py","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6f68bc63c2070263b6b147179fb57d4cca381b16","unresolved":false,"context_lines":[{"line_number":193,"context_line":"                     \"a hosts segments mapping\"))"},{"line_number":194,"context_line":"        return"},{"line_number":195,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":196,"context_line":"        previous_segment_ids \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        segments \u003d context.session.query(NetworkSegment).filter("},{"line_number":198,"context_line":"            NetworkSegment.physical_network.in_(phys_nets))"},{"line_number":199,"context_line":"        current_segment_ids \u003d {"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_60489388","line":196,"range":{"start_line":196,"start_character":31,"end_line":196,"end_character":53},"updated":"2016-05-12 23:35:27.000000000","message":"Hardly worth a new method, especially since the query on the next line has a little more complexity than this one and doesn\u0027t get its own method.  Also, \n\nI\u0027d say collapse it in to this method until there is a clear reason to refactor it into another method.\n\nI know that there is a similar method in test_segment.py but that is actually a totally different method.","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"70f58d877d71d9986b18a0f93cec3ca5c25b562c","unresolved":false,"context_lines":[{"line_number":193,"context_line":"                     \"a hosts segments mapping\"))"},{"line_number":194,"context_line":"        return"},{"line_number":195,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":196,"context_line":"        previous_segment_ids \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        segments \u003d context.session.query(NetworkSegment).filter("},{"line_number":198,"context_line":"            NetworkSegment.physical_network.in_(phys_nets))"},{"line_number":199,"context_line":"        current_segment_ids \u003d {"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_2a2c5398","line":196,"range":{"start_line":196,"start_character":31,"end_line":196,"end_character":53},"in_reply_to":"dab17558_60489388","updated":"2016-05-16 00:26:50.000000000","message":"Done","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"4f6be6882a1199322b8f07283e1b7431e2c11720","unresolved":false,"context_lines":[{"line_number":193,"context_line":"                     \"a hosts segments mapping\"))"},{"line_number":194,"context_line":"        return"},{"line_number":195,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":196,"context_line":"        previous_segment_ids \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        segments \u003d context.session.query(NetworkSegment).filter("},{"line_number":198,"context_line":"            NetworkSegment.physical_network.in_(phys_nets))"},{"line_number":199,"context_line":"        current_segment_ids \u003d {"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_d3d11afd","line":196,"range":{"start_line":196,"start_character":31,"end_line":196,"end_character":53},"in_reply_to":"dab17558_60489388","updated":"2016-05-14 05:17:16.000000000","message":"Well, it might be worth a new method if the method is public.  My IPAM patch coming up next will probably need something like this method.  However, I like to wait until the need is clear before creating new abstractions.  So, in this patch, I think I\u0027d just leave it inline.","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"70f58d877d71d9986b18a0f93cec3ca5c25b562c","unresolved":false,"context_lines":[{"line_number":193,"context_line":"                     \"a hosts segments mapping\"))"},{"line_number":194,"context_line":"        return"},{"line_number":195,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":196,"context_line":"        previous_segment_ids \u003d _get_segments_for_host(context, host)"},{"line_number":197,"context_line":"        segments \u003d context.session.query(NetworkSegment).filter("},{"line_number":198,"context_line":"            NetworkSegment.physical_network.in_(phys_nets))"},{"line_number":199,"context_line":"        current_segment_ids \u003d {"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_4a2717b6","line":196,"range":{"start_line":196,"start_character":31,"end_line":196,"end_character":53},"in_reply_to":"dab17558_d3d11afd","updated":"2016-05-16 00:26:50.000000000","message":"Well, I changed the implementation to be class based before I saw this comment. Let\u0027s see how it evolves. We may add a class method to that class to serve your need :-)","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6f68bc63c2070263b6b147179fb57d4cca381b16","unresolved":false,"context_lines":[{"line_number":206,"context_line":"        if stale_segment_ids:"},{"line_number":207,"context_line":"            context.session.query(SegmentHostMapping).filter("},{"line_number":208,"context_line":"                SegmentHostMapping.segment_id.in_(stale_segment_ids)).delete("},{"line_number":209,"context_line":"                synchronize_session\u003dFalse)"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":""},{"line_number":212,"context_line":"def _get_segments_for_host(context, host):"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_3b307e07","line":209,"range":{"start_line":209,"start_character":16,"end_line":209,"end_character":41},"updated":"2016-05-12 23:35:27.000000000","message":"I\u0027m curious what led you to add this.","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"70f58d877d71d9986b18a0f93cec3ca5c25b562c","unresolved":false,"context_lines":[{"line_number":206,"context_line":"        if stale_segment_ids:"},{"line_number":207,"context_line":"            context.session.query(SegmentHostMapping).filter("},{"line_number":208,"context_line":"                SegmentHostMapping.segment_id.in_(stale_segment_ids)).delete("},{"line_number":209,"context_line":"                synchronize_session\u003dFalse)"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":""},{"line_number":212,"context_line":"def _get_segments_for_host(context, host):"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_8a6bbfb8","line":209,"range":{"start_line":209,"start_character":16,"end_line":209,"end_character":41},"in_reply_to":"dab17558_3b307e07","updated":"2016-05-16 00:26:50.000000000","message":"If I don\u0027t specify this, I get the following error message:\n\nsqlalchemy.exc.InvalidRequestError: Could not evaluate current criteria in Python. Specify \u0027fetch\u0027 or False for the synchronize_session parameter.\n\nLooking at http://docs.sqlalchemy.org/en/latest/orm/query.html, I found the following:\n\nParameters:\tsynchronize_session –\n\nchooses the strategy for the removal of matched objects from the session. Valid values are:\n\nFalse - don’t synchronize the session. This option is the most efficient and is reliable once the session is expired, which typically occurs after a commit(), or explicitly using expire_all(). Before the expiration, objects may still remain in the session which were in fact deleted which can lead to confusing results if they are accessed via get() or already loaded collections.\n\n\u0027fetch\u0027 - performs a select query before the delete to find objects that are matched by the delete query and need to be removed from the session. Matched objects are removed from the session.\n\n\u0027evaluate\u0027 - Evaluate the query’s criteria in Python straight on the objects in the session. If evaluation of the criteria isn’t implemented, an error is raised.\n\nThe expression evaluator currently doesn’t account for differing string collations between the database and Python.","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6f68bc63c2070263b6b147179fb57d4cca381b16","unresolved":false,"context_lines":[{"line_number":211,"context_line":""},{"line_number":212,"context_line":"def _get_segments_for_host(context, host):"},{"line_number":213,"context_line":"    segments_host_list \u003d context.session.query(SegmentHostMapping).filter_by("},{"line_number":214,"context_line":"        host\u003dhost).all()"},{"line_number":215,"context_line":"    return {seg_host[\u0027segment_id\u0027] for seg_host in segments_host_list}"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_20f20b70","line":214,"range":{"start_line":214,"start_character":18,"end_line":214,"end_character":24},"updated":"2016-05-12 23:35:27.000000000","message":"Leave off .all()","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"70f58d877d71d9986b18a0f93cec3ca5c25b562c","unresolved":false,"context_lines":[{"line_number":211,"context_line":""},{"line_number":212,"context_line":"def _get_segments_for_host(context, host):"},{"line_number":213,"context_line":"    segments_host_list \u003d context.session.query(SegmentHostMapping).filter_by("},{"line_number":214,"context_line":"        host\u003dhost).all()"},{"line_number":215,"context_line":"    return {seg_host[\u0027segment_id\u0027] for seg_host in segments_host_list}"}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_8a1d7f64","line":214,"range":{"start_line":214,"start_character":18,"end_line":214,"end_character":24},"in_reply_to":"dab17558_20f20b70","updated":"2016-05-16 00:26:50.000000000","message":"Done","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"}],"neutron/plugins/ml2/plugin.py":[{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"ca83f41879c5a25bb5925ccfce57de5efbbbd4f9","unresolved":false,"context_lines":[{"line_number":1633,"context_line":"        return self.mechanism_manager.filter_hosts_with_segment_access("},{"line_number":1634,"context_line":"            context, segments, candidate_hosts, self.get_agents)"},{"line_number":1635,"context_line":""},{"line_number":1636,"context_line":"    def check_segment_for_host(self, segment, agent):"},{"line_number":1637,"context_line":"        for mech_driver in self.mechanism_manager.ordered_mech_drivers:"},{"line_number":1638,"context_line":"            if mech_driver.obj.check_segment_for_agent(segment, agent):"},{"line_number":1639,"context_line":"                return True"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_2af57356","line":1636,"range":{"start_line":1636,"start_character":8,"end_line":1636,"end_character":30},"updated":"2016-05-10 08:31:19.000000000","message":"I think it is more suitable with the name of \"check_segment\". A host is usually with one l2 agent, but it is possible to have multiple l2 agents in one host(if my memory is correct)","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"4f1eac32c9ec8d299f08b09fa431f920d7105bfb","unresolved":false,"context_lines":[{"line_number":1633,"context_line":"        return self.mechanism_manager.filter_hosts_with_segment_access("},{"line_number":1634,"context_line":"            context, segments, candidate_hosts, self.get_agents)"},{"line_number":1635,"context_line":""},{"line_number":1636,"context_line":"    def check_segment_for_host(self, segment, agent):"},{"line_number":1637,"context_line":"        for mech_driver in self.mechanism_manager.ordered_mech_drivers:"},{"line_number":1638,"context_line":"            if mech_driver.obj.check_segment_for_agent(segment, agent):"},{"line_number":1639,"context_line":"                return True"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_90f0e992","line":1636,"range":{"start_line":1636,"start_character":8,"end_line":1636,"end_character":30},"in_reply_to":"dab17558_2af57356","updated":"2016-05-10 18:18:23.000000000","message":"Why not check_segment_for_agent?  It is analogous to the existing check_segment_for_agent on a mechanism driver but checks all of them.","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"4c73373680cd1dd715df4ca68d8f2c4cc7fd9e54","unresolved":false,"context_lines":[{"line_number":1633,"context_line":"        return self.mechanism_manager.filter_hosts_with_segment_access("},{"line_number":1634,"context_line":"            context, segments, candidate_hosts, self.get_agents)"},{"line_number":1635,"context_line":""},{"line_number":1636,"context_line":"    def check_segment_for_host(self, segment, agent):"},{"line_number":1637,"context_line":"        for mech_driver in self.mechanism_manager.ordered_mech_drivers:"},{"line_number":1638,"context_line":"            if mech_driver.obj.check_segment_for_agent(segment, agent):"},{"line_number":1639,"context_line":"                return True"}],"source_content_type":"text/x-python","patch_set":8,"id":"dab17558_884d5040","line":1636,"range":{"start_line":1636,"start_character":8,"end_line":1636,"end_character":30},"in_reply_to":"dab17558_90f0e992","updated":"2016-05-11 00:51:20.000000000","message":"Done","commit_id":"e47943bbf6dc98961e62387e3c94b995d760ffb1"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"62ea3d753a7042a822b9b13d1fa69412ce490836","unresolved":false,"context_lines":[{"line_number":1688,"context_line":""},{"line_number":1689,"context_line":"    def check_segment_for_agent(self, segment, agent):"},{"line_number":1690,"context_line":"        for mech_driver in self.mechanism_manager.ordered_mech_drivers:"},{"line_number":1691,"context_line":"            if mech_driver.obj.check_segment_for_agent(segment, agent):"},{"line_number":1692,"context_line":"                return True"},{"line_number":1693,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_f22efdf8","line":1691,"range":{"start_line":1691,"start_character":31,"end_line":1691,"end_character":54},"updated":"2016-05-19 06:55:14.000000000","message":"\"check_segment_for_agent\" is only work for subclass of SimpleAgentMechanismDriverBase. But mechanism_manager.ordered_mech_drivers might contain other mech_drivers like \"L2populationMechanismDriver\", which don\u0027t have \"check_segment_for_agent\" defined. I guess here will have exception when l2population is enabled in neutron.ml2.mechanism_drivers.\n\n\nAnd I have this doubt, agent is a specified type l2 agent, like ovs-agent, or lb-agent. But \"ordered_mech_drivers\" is all mech drivers that configured for ml2. What if a ovs-agent is passed in, but the check_segment_for_agent success only for linuxbridge mechanism driver?","commit_id":"a914de873d5c1dbab7f43442817145d048377c76"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"d208f7c988499b7ee15825cd5e67abc83870caa5","unresolved":false,"context_lines":[{"line_number":1688,"context_line":""},{"line_number":1689,"context_line":"    def check_segment_for_agent(self, segment, agent):"},{"line_number":1690,"context_line":"        for mech_driver in self.mechanism_manager.ordered_mech_drivers:"},{"line_number":1691,"context_line":"            if mech_driver.obj.check_segment_for_agent(segment, agent):"},{"line_number":1692,"context_line":"                return True"},{"line_number":1693,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_9facb1a1","line":1691,"range":{"start_line":1691,"start_character":31,"end_line":1691,"end_character":54},"in_reply_to":"bab6814e_f22efdf8","updated":"2016-05-19 18:25:07.000000000","message":"I added an additional check to match the mechanism driver called to the agent type. That addresses both of your concerns","commit_id":"a914de873d5c1dbab7f43442817145d048377c76"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"3b49d2859bcd5794a13447fb20a6edfc16eb6d31","unresolved":false,"context_lines":[{"line_number":1688,"context_line":""},{"line_number":1689,"context_line":"    def check_segment_for_agent(self, segment, agent):"},{"line_number":1690,"context_line":"        for mech_driver in self.mechanism_manager.ordered_mech_drivers:"},{"line_number":1691,"context_line":"            if mech_driver.obj.agent_type \u003d\u003d agent[\u0027agent_type\u0027]:"},{"line_number":1692,"context_line":"                if mech_driver.obj.check_segment_for_agent(segment, agent):"},{"line_number":1693,"context_line":"                    return True"},{"line_number":1694,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":15,"id":"bab6814e_35bae80a","line":1691,"range":{"start_line":1691,"start_character":12,"end_line":1691,"end_character":65},"updated":"2016-05-20 08:42:46.000000000","message":"This could mitigate the possible error, but with l2pop enabled, I can still get the following error with the patch running.\n\n*** AttributeError: \u0027L2populationMechanismDriver\u0027 object has no attribute \u0027agent_type\u0027.\n\nTo avoid it, you could also check if \"check_segment_for_agent\" is implemented in mechanismDriver, because, as [1] said, it is a must implement method for agent based mechanismDriver.\nAlso, I would suggest to put all the code in to managers.py, because the plugin don\u0027t have to know the details of mechanismDriver.\n\n[1] https://github.com/openstack/neutron/blob/f3eb620ebf300f44ba388aaa784e20037d74fd8b/neutron/plugins/ml2/drivers/mech_agent.py#L122","commit_id":"7b95f5bc9636a9e069f2a9eb794ea8ccb964d23a"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"a52a3132558a1bc80fa3d020644f6b9c865b44b0","unresolved":false,"context_lines":[{"line_number":1688,"context_line":""},{"line_number":1689,"context_line":"    def check_segment_for_agent(self, segment, agent):"},{"line_number":1690,"context_line":"        for mech_driver in self.mechanism_manager.ordered_mech_drivers:"},{"line_number":1691,"context_line":"            if mech_driver.obj.agent_type \u003d\u003d agent[\u0027agent_type\u0027]:"},{"line_number":1692,"context_line":"                if mech_driver.obj.check_segment_for_agent(segment, agent):"},{"line_number":1693,"context_line":"                    return True"},{"line_number":1694,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":15,"id":"bab6814e_f7a16b47","line":1691,"range":{"start_line":1691,"start_character":12,"end_line":1691,"end_character":65},"in_reply_to":"bab6814e_35bae80a","updated":"2016-05-20 17:19:03.000000000","message":"Yes, last night I thought of the possibility of \n\n*** AttributeError: \u0027L2populationMechanismDriver\u0027 object has no attribute \u0027agent_type\u0027\n\nYou beat me to it :-) Thanks. I\u0027d rather check directly whether driver has \u0027agent_type\u0027 attribute","commit_id":"7b95f5bc9636a9e069f2a9eb794ea8ccb964d23a"}],"neutron/services/segments/db.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"b3e415154e9752ccf412446e9e050fd5bb5b0e7e","unresolved":false,"context_lines":[{"line_number":158,"context_line":""},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"@six.add_metaclass(abc.ABCMeta)"},{"line_number":161,"context_line":"class SegmentHostMappingUpdaterBase(object):"},{"line_number":162,"context_line":""},{"line_number":163,"context_line":"    def _update(self, context, host, current_segment_ids):"},{"line_number":164,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"}],"source_content_type":"text/x-python","patch_set":13,"id":"dab17558_2c107c4d","line":161,"range":{"start_line":161,"start_character":6,"end_line":161,"end_character":35},"updated":"2016-05-17 20:54:32.000000000","message":"While I like the separation of concerns here, I\u0027m not sure that this class structure is needed.  It feels like a little bit of overkill.  I mean basically, it is just two methods, _update in this class which handles the DB update and update in the next class which handles the agents_db specific stuff.","commit_id":"9ad79631762e073a811a723bfe9077b527508349"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"87b59676af1b87a163176082d1cae897a09e776a","unresolved":false,"context_lines":[{"line_number":158,"context_line":""},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"@six.add_metaclass(abc.ABCMeta)"},{"line_number":161,"context_line":"class SegmentHostMappingUpdaterBase(object):"},{"line_number":162,"context_line":""},{"line_number":163,"context_line":"    def _update(self, context, host, current_segment_ids):"},{"line_number":164,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_d14585b3","line":161,"range":{"start_line":161,"start_character":6,"end_line":161,"end_character":35},"in_reply_to":"bab6814e_7eb668dd","updated":"2016-05-19 21:13:25.000000000","message":"I think you\u0027re arguing for the separation of concerns between update and _update.  I agree with that.  I just wonder if a class hierarchy is necessary to accomplish it or if it can just be two methods.","commit_id":"9ad79631762e073a811a723bfe9077b527508349"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"e97072c2b5d4feaa4507437deb3f0130fe91ddb5","unresolved":false,"context_lines":[{"line_number":158,"context_line":""},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"@six.add_metaclass(abc.ABCMeta)"},{"line_number":161,"context_line":"class SegmentHostMappingUpdaterBase(object):"},{"line_number":162,"context_line":""},{"line_number":163,"context_line":"    def _update(self, context, host, current_segment_ids):"},{"line_number":164,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_7eb668dd","line":161,"range":{"start_line":161,"start_character":6,"end_line":161,"end_character":35},"in_reply_to":"dab17558_2c107c4d","updated":"2016-05-18 22:37:58.000000000","message":"In PS10, Han Zhou commented: \"Again, this is valid for agent-based plugins, but for other plugins, they should be allowed to create/update/delete segment-host mappings directly. For example, a plugin can be implemented in a way that when host-segment mapping creation is requested, it programs the hardware switch to add VLAN binding to the host.\"\n\nSo trying to support cases like the one described by Han Zhou, I created this base class constraining my assumptions to the following minimum:\n\n1) The _update method will be solely responsible of updating the DB, receiving as inputs the host and the ids of segments confirmed to be bound to that host\n\n2) The update method can compute the set of bound segments ids\nin anyway that suits best the plugin. No assumptions are made about the plugin implementing a certain method, no agents are assumed, etc. The only assumption is that when _update is called, the host and the ids of segments confirmed to be bound to the host are passed as arguments.\n\nI am trying to be diligent in responding to reviewers feedback and in showing that we are trying to think hard in situations beyond ML2 and our agents based implementation.\n\nOf course, other plugin developers might reuse SegmentHostMappingUpdaterForAgent below and achieve the same purpose I describe in this response. I will be fine getting rid of this abstract class if we agree that providing only SegmentHostMappingUpdaterForAgent is sufficient. I am just trying to provide structure :-)","commit_id":"9ad79631762e073a811a723bfe9077b527508349"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"b3e415154e9752ccf412446e9e050fd5bb5b0e7e","unresolved":false,"context_lines":[{"line_number":189,"context_line":"        \"\"\""},{"line_number":190,"context_line":""},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"class SegmentHostMappingUpdaterAgent(SegmentHostMappingUpdaterBase):"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    def __init__(self, plugin):"},{"line_number":195,"context_line":"        self.check_segment_for_agent \u003d getattr(plugin,"}],"source_content_type":"text/x-python","patch_set":13,"id":"dab17558_b8cb0b1d","line":192,"range":{"start_line":192,"start_character":6,"end_line":192,"end_character":36},"updated":"2016-05-17 20:54:32.000000000","message":"AgentSegmentHostmappingUpdater?  The current name makes it sound like this class is an agent.  But, iiuc, it is *for* use by the agents_db.\n\nThis makes me wonder if it should live here or in the agents_db.","commit_id":"9ad79631762e073a811a723bfe9077b527508349"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"87b59676af1b87a163176082d1cae897a09e776a","unresolved":false,"context_lines":[{"line_number":189,"context_line":"        \"\"\""},{"line_number":190,"context_line":""},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"class SegmentHostMappingUpdaterAgent(SegmentHostMappingUpdaterBase):"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    def __init__(self, plugin):"},{"line_number":195,"context_line":"        self.check_segment_for_agent \u003d getattr(plugin,"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_718499d3","line":192,"range":{"start_line":192,"start_character":6,"end_line":192,"end_character":36},"in_reply_to":"bab6814e_282dcaa7","updated":"2016-05-19 21:13:25.000000000","message":"You mean SegmentHostmappingUpdaterForAgent (without the redundant Agent)?","commit_id":"9ad79631762e073a811a723bfe9077b527508349"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"e97072c2b5d4feaa4507437deb3f0130fe91ddb5","unresolved":false,"context_lines":[{"line_number":189,"context_line":"        \"\"\""},{"line_number":190,"context_line":""},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"class SegmentHostMappingUpdaterAgent(SegmentHostMappingUpdaterBase):"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    def __init__(self, plugin):"},{"line_number":195,"context_line":"        self.check_segment_for_agent \u003d getattr(plugin,"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_282dcaa7","line":192,"range":{"start_line":192,"start_character":6,"end_line":192,"end_character":36},"in_reply_to":"dab17558_b8cb0b1d","updated":"2016-05-18 22:37:58.000000000","message":"How about AgentSegmentHostmappingUpdaterForAgent. As indicated in other responses to this review, I am trying to provide structure for non agent based plugins","commit_id":"9ad79631762e073a811a723bfe9077b527508349"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"b3e415154e9752ccf412446e9e050fd5bb5b0e7e","unresolved":false,"context_lines":[{"line_number":201,"context_line":"                         \"build a hosts segments mapping\"))"},{"line_number":202,"context_line":""},{"line_number":203,"context_line":"    def update(self, context, host, agent):"},{"line_number":204,"context_line":"        if not self.check_segment_for_agent:"},{"line_number":205,"context_line":"            return"},{"line_number":206,"context_line":"        phys_nets \u003d self._get_phys_nets(agent)"},{"line_number":207,"context_line":"        if not phys_nets:"},{"line_number":208,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":13,"id":"dab17558_38f6fbd6","line":205,"range":{"start_line":204,"start_character":7,"end_line":205,"end_character":18},"updated":"2016-05-17 20:54:32.000000000","message":"nit:  Instead of checking this, I\u0027d use a factory instead of the __init__ method above.  If the plugin doesn\u0027t support it, then the factory would return an instance of this:\n\nclass SegmentHostMappingNonUpdater(SegmentHostMappingUpdaterBase):\n    def update(self, context, host, **kwargs):\n        pass\n\nThat\u0027s just what I would probably do instead of checking this.  But, others might not like it.","commit_id":"9ad79631762e073a811a723bfe9077b527508349"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"e97072c2b5d4feaa4507437deb3f0130fe91ddb5","unresolved":false,"context_lines":[{"line_number":201,"context_line":"                         \"build a hosts segments mapping\"))"},{"line_number":202,"context_line":""},{"line_number":203,"context_line":"    def update(self, context, host, agent):"},{"line_number":204,"context_line":"        if not self.check_segment_for_agent:"},{"line_number":205,"context_line":"            return"},{"line_number":206,"context_line":"        phys_nets \u003d self._get_phys_nets(agent)"},{"line_number":207,"context_line":"        if not phys_nets:"},{"line_number":208,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_1ebc7c04","line":205,"range":{"start_line":204,"start_character":7,"end_line":205,"end_character":18},"in_reply_to":"dab17558_38f6fbd6","updated":"2016-05-18 22:37:58.000000000","message":"Done","commit_id":"9ad79631762e073a811a723bfe9077b527508349"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"62ea3d753a7042a822b9b13d1fa69412ce490836","unresolved":false,"context_lines":[{"line_number":204,"context_line":"                         \"build a hosts segments mapping\"))"},{"line_number":205,"context_line":"            return SegmentHostMappingNonUpdater()"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def update(self, context, host, agent):"},{"line_number":208,"context_line":"        if not self.check_segment_for_agent:"},{"line_number":209,"context_line":"            return"},{"line_number":210,"context_line":"        phys_nets \u003d self._get_phys_nets(agent)"}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_1d3f57d7","line":207,"range":{"start_line":207,"start_character":0,"end_line":207,"end_character":43},"updated":"2016-05-19 06:55:14.000000000","message":"The definition here is different from the method defined in interface at line 179.","commit_id":"a914de873d5c1dbab7f43442817145d048377c76"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"d208f7c988499b7ee15825cd5e67abc83870caa5","unresolved":false,"context_lines":[{"line_number":204,"context_line":"                         \"build a hosts segments mapping\"))"},{"line_number":205,"context_line":"            return SegmentHostMappingNonUpdater()"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def update(self, context, host, agent):"},{"line_number":208,"context_line":"        if not self.check_segment_for_agent:"},{"line_number":209,"context_line":"            return"},{"line_number":210,"context_line":"        phys_nets \u003d self._get_phys_nets(agent)"}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_b63ba237","line":207,"range":{"start_line":207,"start_character":0,"end_line":207,"end_character":43},"in_reply_to":"bab6814e_1d3f57d7","updated":"2016-05-19 18:25:07.000000000","message":"Fair enough","commit_id":"a914de873d5c1dbab7f43442817145d048377c76"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"62ea3d753a7042a822b9b13d1fa69412ce490836","unresolved":false,"context_lines":[{"line_number":205,"context_line":"            return SegmentHostMappingNonUpdater()"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def update(self, context, host, agent):"},{"line_number":208,"context_line":"        if not self.check_segment_for_agent:"},{"line_number":209,"context_line":"            return"},{"line_number":210,"context_line":"        phys_nets \u003d self._get_phys_nets(agent)"},{"line_number":211,"context_line":"        if not phys_nets:"},{"line_number":212,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_2f24a2ad","line":209,"range":{"start_line":208,"start_character":0,"end_line":209,"end_character":18},"updated":"2016-05-19 06:55:14.000000000","message":"Is it necessary to check check_segment_for_agent? If the instance is instantiated by \"get_updater\", the \"check_segment_for_agent\" must be not-None. If instance is instantiated by class, there will be AttributeError here.","commit_id":"a914de873d5c1dbab7f43442817145d048377c76"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"d208f7c988499b7ee15825cd5e67abc83870caa5","unresolved":false,"context_lines":[{"line_number":205,"context_line":"            return SegmentHostMappingNonUpdater()"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def update(self, context, host, agent):"},{"line_number":208,"context_line":"        if not self.check_segment_for_agent:"},{"line_number":209,"context_line":"            return"},{"line_number":210,"context_line":"        phys_nets \u003d self._get_phys_nets(agent)"},{"line_number":211,"context_line":"        if not phys_nets:"},{"line_number":212,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_76481a88","line":209,"range":{"start_line":208,"start_character":0,"end_line":209,"end_character":18},"in_reply_to":"bab6814e_2f24a2ad","updated":"2016-05-19 18:25:07.000000000","message":"Yes, good catch. Not necessary","commit_id":"a914de873d5c1dbab7f43442817145d048377c76"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"62ea3d753a7042a822b9b13d1fa69412ce490836","unresolved":false,"context_lines":[{"line_number":220,"context_line":""},{"line_number":221,"context_line":"    def _get_phys_nets(self, agent):"},{"line_number":222,"context_line":"        configurations_dict \u003d agent.get(\u0027configurations\u0027, {})"},{"line_number":223,"context_line":"        mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027, {})"},{"line_number":224,"context_line":"        mappings.update(configurations_dict.get(\u0027interface_mappings\u0027, {}))"},{"line_number":225,"context_line":"        mappings.update(configurations_dict.get(\u0027device_mappings\u0027, {}))"},{"line_number":226,"context_line":"        return mappings.keys()"},{"line_number":227,"context_line":""},{"line_number":228,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_f2bf1d5e","line":225,"range":{"start_line":223,"start_character":0,"end_line":225,"end_character":71},"updated":"2016-05-19 06:55:14.000000000","message":"I don\u0027t think it is a good way to consider all situations.\n1) agent is a specified type agent, only need to consider one condition. \n2) It will be hard to add another agent type with different configuration.\n\nI think the sub-implementation of [1] can be used.\n\n[1] https://github.com/openstack/neutron/blob/b934db80c6f27a2298964742995d7b139802afa0/neutron/plugins/ml2/drivers/mech_agent.py#L187","commit_id":"a914de873d5c1dbab7f43442817145d048377c76"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"d208f7c988499b7ee15825cd5e67abc83870caa5","unresolved":false,"context_lines":[{"line_number":220,"context_line":""},{"line_number":221,"context_line":"    def _get_phys_nets(self, agent):"},{"line_number":222,"context_line":"        configurations_dict \u003d agent.get(\u0027configurations\u0027, {})"},{"line_number":223,"context_line":"        mappings \u003d configurations_dict.get(\u0027bridge_mappings\u0027, {})"},{"line_number":224,"context_line":"        mappings.update(configurations_dict.get(\u0027interface_mappings\u0027, {}))"},{"line_number":225,"context_line":"        mappings.update(configurations_dict.get(\u0027device_mappings\u0027, {}))"},{"line_number":226,"context_line":"        return mappings.keys()"},{"line_number":227,"context_line":""},{"line_number":228,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_1f4ba1ca","line":225,"range":{"start_line":223,"start_character":0,"end_line":225,"end_character":71},"in_reply_to":"bab6814e_f2bf1d5e","updated":"2016-05-19 18:25:07.000000000","message":"To do what you suggest I would need to add another method to the plugin, to call the appropriate get_mappings method. I am going to hold off doing that to let other reviewers weigh in","commit_id":"a914de873d5c1dbab7f43442817145d048377c76"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"0da26a6439d510acf5ab87ad7b42917c2cf9fa5f","unresolved":false,"context_lines":[{"line_number":204,"context_line":"                         \"build a hosts segments mapping\"))"},{"line_number":205,"context_line":"            return SegmentHostMappingNonUpdater()"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def update(self, context, host, **kwargs):"},{"line_number":208,"context_line":"        agent \u003d kwargs[\u0027agent\u0027]"},{"line_number":209,"context_line":"        phys_nets \u003d self._get_phys_nets(agent)"},{"line_number":210,"context_line":"        if not phys_nets:"}],"source_content_type":"text/x-python","patch_set":15,"id":"bab6814e_f1824993","line":207,"range":{"start_line":207,"start_character":8,"end_line":207,"end_character":14},"updated":"2016-05-19 21:29:45.000000000","message":"To reiterate, I think the separation of concerns between this method and _update above is good.  I still wonder if is a little too heavy-weight with the whole class hierarchy.  But, it isn\u0027t really that much more.","commit_id":"7b95f5bc9636a9e069f2a9eb794ea8ccb964d23a"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"a52a3132558a1bc80fa3d020644f6b9c865b44b0","unresolved":false,"context_lines":[{"line_number":204,"context_line":"                         \"build a hosts segments mapping\"))"},{"line_number":205,"context_line":"            return SegmentHostMappingNonUpdater()"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def update(self, context, host, **kwargs):"},{"line_number":208,"context_line":"        agent \u003d kwargs[\u0027agent\u0027]"},{"line_number":209,"context_line":"        phys_nets \u003d self._get_phys_nets(agent)"},{"line_number":210,"context_line":"        if not phys_nets:"}],"source_content_type":"text/x-python","patch_set":15,"id":"bab6814e_37949341","line":207,"range":{"start_line":207,"start_character":8,"end_line":207,"end_character":14},"in_reply_to":"bab6814e_f1824993","updated":"2016-05-20 17:19:03.000000000","message":"In agreement with you. Removing class hierarchy and going back to functions based solution","commit_id":"7b95f5bc9636a9e069f2a9eb794ea8ccb964d23a"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"0da26a6439d510acf5ab87ad7b42917c2cf9fa5f","unresolved":false,"context_lines":[{"line_number":205,"context_line":"            return SegmentHostMappingNonUpdater()"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def update(self, context, host, **kwargs):"},{"line_number":208,"context_line":"        agent \u003d kwargs[\u0027agent\u0027]"},{"line_number":209,"context_line":"        phys_nets \u003d self._get_phys_nets(agent)"},{"line_number":210,"context_line":"        if not phys_nets:"},{"line_number":211,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":15,"id":"bab6814e_4ce1fefd","line":208,"range":{"start_line":208,"start_character":8,"end_line":208,"end_character":31},"updated":"2016-05-19 21:29:45.000000000","message":"Passing extra arguments to an abstract method depend on the context where it is called is not a good pattern.  If the method is abstract, you should be able to call it the same everywhere. This kind of defeats the purpose of making it an abstract method in the first place.\n\nThe way to solve this might be to pass the agent to the factory and then to __init__() and set self.agent in init.  That way all of the agent-specific stuff is handled by the class and not passed in as extra arguments.\n\nAnother way to solve this is to just eliminate the class structure and use two simple methods to implement this separation of concerns.  One method to update the DB and another to be called in the agent context which handles the agent stuff.","commit_id":"7b95f5bc9636a9e069f2a9eb794ea8ccb964d23a"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"a52a3132558a1bc80fa3d020644f6b9c865b44b0","unresolved":false,"context_lines":[{"line_number":205,"context_line":"            return SegmentHostMappingNonUpdater()"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def update(self, context, host, **kwargs):"},{"line_number":208,"context_line":"        agent \u003d kwargs[\u0027agent\u0027]"},{"line_number":209,"context_line":"        phys_nets \u003d self._get_phys_nets(agent)"},{"line_number":210,"context_line":"        if not phys_nets:"},{"line_number":211,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":15,"id":"bab6814e_377bf329","line":208,"range":{"start_line":208,"start_character":8,"end_line":208,"end_character":31},"in_reply_to":"bab6814e_4ce1fefd","updated":"2016-05-20 17:19:03.000000000","message":"Chose your second recommendation","commit_id":"7b95f5bc9636a9e069f2a9eb794ea8ccb964d23a"},{"author":{"_account_id":11762,"name":"Han Zhou","email":"zhouhan@gmail.com","username":"hanzhou"},"change_message_id":"546c3f557fdc5cbe81bdc422d8d9cf670063c570","unresolved":false,"context_lines":[{"line_number":155,"context_line":"                raise exceptions.SegmentNotFound(segment_id\u003duuid)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"def _update_segment_host_mapping(context, host, current_segment_ids):"},{"line_number":159,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":160,"context_line":"        segments_host_query \u003d context.session.query("},{"line_number":161,"context_line":"            SegmentHostMapping).filter_by(host\u003dhost)"}],"source_content_type":"text/x-python","patch_set":18,"id":"bab6814e_7205700f","line":158,"updated":"2016-05-21 23:50:14.000000000","message":"nit: shall we make this function public (i.e. remove the leading _)? I expect this function to be called at least by networking-ovn.","commit_id":"446c28d93a202c5369d790ade625ef9bd9fd9b70"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"99317b3b6948af2b73959fe403c293edf3c1ba5e","unresolved":false,"context_lines":[{"line_number":155,"context_line":"                raise exceptions.SegmentNotFound(segment_id\u003duuid)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"def _update_segment_host_mapping(context, host, current_segment_ids):"},{"line_number":159,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":160,"context_line":"        segments_host_query \u003d context.session.query("},{"line_number":161,"context_line":"            SegmentHostMapping).filter_by(host\u003dhost)"}],"source_content_type":"text/x-python","patch_set":18,"id":"bab6814e_9277349f","line":158,"in_reply_to":"bab6814e_7205700f","updated":"2016-05-22 00:08:25.000000000","message":"That\u0027s a good idea. Let\u0027s give it a try :-)","commit_id":"446c28d93a202c5369d790ade625ef9bd9fd9b70"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"7c0e6a4b1181b0b7f2fc29691ba4202adddc5101","unresolved":false,"context_lines":[{"line_number":190,"context_line":"                     \"build a hosts segments mapping\"))"},{"line_number":191,"context_line":"        return"},{"line_number":192,"context_line":"    start_flag \u003d agent.get(\u0027start_flag\u0027, None)"},{"line_number":193,"context_line":"    if host in reported_hosts and not start_flag:"},{"line_number":194,"context_line":"        return"},{"line_number":195,"context_line":"    reported_hosts.add(host)"},{"line_number":196,"context_line":"    phys_nets \u003d _get_phys_nets(agent)"},{"line_number":197,"context_line":"    if not phys_nets:"}],"source_content_type":"text/x-python","patch_set":19,"id":"bab6814e_f50d9c8f","line":194,"range":{"start_line":193,"start_character":4,"end_line":194,"end_character":14},"updated":"2016-05-23 15:59:51.000000000","message":"no need with https://review.openstack.org/#/c/319957/","commit_id":"a499407c20ad776d816082f6bfe574dfb934aca2"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"ac6f162f281e552544f6bcb58c2f522a839abafa","unresolved":false,"context_lines":[{"line_number":192,"context_line":"    phys_nets \u003d _get_phys_nets(agent)"},{"line_number":193,"context_line":"    if not phys_nets:"},{"line_number":194,"context_line":"        return"},{"line_number":195,"context_line":"    start_flag \u003d agent.get(\u0027start_flag\u0027, None)"},{"line_number":196,"context_line":"    if host in reported_hosts and not start_flag:"},{"line_number":197,"context_line":"        return"},{"line_number":198,"context_line":"    reported_hosts.add(host)"},{"line_number":199,"context_line":"    with context.session.begin(subtransactions\u003dTrue):"},{"line_number":200,"context_line":"        segments \u003d context.session.query(db.NetworkSegment).filter("},{"line_number":201,"context_line":"            db.NetworkSegment.physical_network.in_(phys_nets))"}],"source_content_type":"text/x-python","patch_set":20,"id":"bab6814e_d6eb3477","line":198,"range":{"start_line":195,"start_character":4,"end_line":198,"end_character":28},"updated":"2016-05-23 20:05:07.000000000","message":"We should execute this only after we know that this agent update carries physical nets to interface/ bridge / device mappings. Otherwise, we might mark this host as known in our configuration when in fact we are processing an L3 or DHCP agent.","commit_id":"f1ebd7ab1a0f007e49588e882950e4a3a35db334"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"054aec890649995d2a45b93e63fd4a2d3a4333c5","unresolved":false,"context_lines":[{"line_number":179,"context_line":"    return mappings.keys()"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"reported_hosts \u003d set()"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":""},{"line_number":185,"context_line":"def update_segment_host_mapping_for_agent(context, host, plugin, agent):"}],"source_content_type":"text/x-python","patch_set":21,"id":"bab6814e_7cddc96f","line":182,"updated":"2016-05-24 21:29:22.000000000","message":"This seems a little odd/error prone? Why isn\u0027t this inside of a class?","commit_id":"3d3f0595ebb3a20949d52b7226a2d4551f0eaf6d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"dbbf446220855adc1ec1dbbc5d12664530a0cffb","unresolved":false,"context_lines":[{"line_number":179,"context_line":"    return mappings.keys()"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"reported_hosts \u003d set()"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":""},{"line_number":185,"context_line":"def update_segment_host_mapping_for_agent(context, host, plugin, agent):"}],"source_content_type":"text/x-python","patch_set":21,"id":"bab6814e_65e05efb","line":182,"in_reply_to":"bab6814e_7cddc96f","updated":"2016-05-25 15:44:40.000000000","message":"It is meant to remember which agents the server has already heard from since it started.  I think that module level is appropriate for that.  Which class would have put it under?\n\nIt did occur to me that this optimization is limited because a scaled environment will typically run many server workers.  But, still, it can\u0027t hurt.","commit_id":"3d3f0595ebb3a20949d52b7226a2d4551f0eaf6d"}],"neutron/tests/unit/extensions/test_segment.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"6f68bc63c2070263b6b147179fb57d4cca381b16","unresolved":false,"context_lines":[{"line_number":291,"context_line":"    def _get_segments_for_host(self, host):"},{"line_number":292,"context_line":"        ctx \u003d context.get_admin_context()"},{"line_number":293,"context_line":"        segments_host_list \u003d ctx.session.query("},{"line_number":294,"context_line":"            segments_db.SegmentHostMapping).filter_by(host\u003dhost).all()"},{"line_number":295,"context_line":"        return {seg_host[\u0027segment_id\u0027]: seg_host"},{"line_number":296,"context_line":"                for seg_host in segments_host_list}"},{"line_number":297,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_3b4cfe75","line":294,"range":{"start_line":294,"start_character":64,"end_line":294,"end_character":70},"updated":"2016-05-12 23:35:27.000000000","message":"Get in the habit of not using .all().  :)","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"70f58d877d71d9986b18a0f93cec3ca5c25b562c","unresolved":false,"context_lines":[{"line_number":291,"context_line":"    def _get_segments_for_host(self, host):"},{"line_number":292,"context_line":"        ctx \u003d context.get_admin_context()"},{"line_number":293,"context_line":"        segments_host_list \u003d ctx.session.query("},{"line_number":294,"context_line":"            segments_db.SegmentHostMapping).filter_by(host\u003dhost).all()"},{"line_number":295,"context_line":"        return {seg_host[\u0027segment_id\u0027]: seg_host"},{"line_number":296,"context_line":"                for seg_host in segments_host_list}"},{"line_number":297,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"dab17558_6784d60a","line":294,"range":{"start_line":294,"start_character":64,"end_line":294,"end_character":70},"in_reply_to":"dab17558_3b4cfe75","updated":"2016-05-16 00:26:50.000000000","message":"Will do. Thanks!","commit_id":"f232661eeb4599a3a9039872e5a6ffddf22c09c4"}]}