)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"dd93c75e590c370ffc3ebd77888575e752837a44","unresolved":false,"context_lines":[{"line_number":7,"context_line":"WIP - DHCP Agent scheduling with segments"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Since a network can now be broken up by segments, each segment will"},{"line_number":10,"context_line":"need to have its own DHCP Agent.  This maintains backwards"},{"line_number":11,"context_line":"compatibility when a network does not have a segment. However,"},{"line_number":12,"context_line":"once a segment is created on a network, a dhcp agent should be"},{"line_number":13,"context_line":"scheduled per segment."},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"dab17558_d1d8fb81","line":11,"range":{"start_line":10,"start_character":39,"end_line":11,"end_character":13},"updated":"2016-05-13 21:43:45.000000000","message":"This is good.  We need to stick to this regardless of how it comes out.","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"dd93c75e590c370ffc3ebd77888575e752837a44","unresolved":false,"context_lines":[{"line_number":9,"context_line":"Since a network can now be broken up by segments, each segment will"},{"line_number":10,"context_line":"need to have its own DHCP Agent.  This maintains backwards"},{"line_number":11,"context_line":"compatibility when a network does not have a segment. However,"},{"line_number":12,"context_line":"once a segment is created on a network, a dhcp agent should be"},{"line_number":13,"context_line":"scheduled per segment."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"This just modifies the scheduling side.  Follow up patches will address"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"dab17558_b141f70d","line":12,"range":{"start_line":12,"start_character":7,"end_line":12,"end_character":38},"updated":"2016-05-13 21:43:45.000000000","message":"We should be a little more clear about what this means.  To me, this means that a subnet with DHCP enabled is associated to a segment on a network.  Not necessarily that any segment is created on a network.","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":10,"context_line":"need to have its own DHCP Agent.  This maintains backwards"},{"line_number":11,"context_line":"compatibility when a network does not have a segment. However,"},{"line_number":12,"context_line":"once a segment is created on a network, a dhcp agent should be"},{"line_number":13,"context_line":"scheduled per segment."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"This just modifies the scheduling side.  Follow up patches will address"},{"line_number":16,"context_line":"the actual agent side."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"9abb7d3a_65b6eedd","line":13,"range":{"start_line":13,"start_character":14,"end_line":13,"end_character":21},"updated":"2016-05-27 20:01:01.000000000","message":"To get really nit-picky, it should be per segment with DHCP enabled subnets.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"876ff6f0be4384efc81ca9a048cf79dbd51a10b4","unresolved":false,"context_lines":[{"line_number":10,"context_line":"need to have its own DHCP Agent.  This maintains backwards"},{"line_number":11,"context_line":"compatibility when a network does not have a segment. However,"},{"line_number":12,"context_line":"once a segment is created on a network, a dhcp agent should be"},{"line_number":13,"context_line":"scheduled per segment."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"This just modifies the scheduling side.  Follow up patches will address"},{"line_number":16,"context_line":"the actual agent side."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"9abb7d3a_7ee8eab4","line":13,"range":{"start_line":13,"start_character":14,"end_line":13,"end_character":21},"in_reply_to":"9abb7d3a_65b6eedd","updated":"2016-05-31 06:43:01.000000000","message":"Done","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"}],"neutron/api/rpc/agentnotifiers/dhcp_rpc_agent_api.py":[{"author":{"_account_id":12906,"name":"tonytan4ever","email":"tony.tan@rackspace.com","username":"tonytan4ever"},"change_message_id":"e15c9f9de8ce5d36b629b0bb01beeb90595c65b5","unresolved":false,"context_lines":[{"line_number":76,"context_line":"        new_agents \u003d self.plugin.schedule_network(context, network) or []"},{"line_number":77,"context_line":"        if new_agents:"},{"line_number":78,"context_line":"            for agent in new_agents:"},{"line_number":79,"context_line":"                # TODO(blogan): Check for segment and send segment cast"},{"line_number":80,"context_line":"                self._cast_message("},{"line_number":81,"context_line":"                    context, \u0027network_create_end\u0027,"},{"line_number":82,"context_line":"                    {\u0027network\u0027: {\u0027id\u0027: network[\u0027id\u0027]}}, agent[\u0027host\u0027])"}],"source_content_type":"text/x-python","patch_set":1,"id":"1a122d0e_8468cb89","line":79,"updated":"2016-05-05 19:38:11.000000000","message":"How can we check the segment ? by checking the context","commit_id":"608633b282c4fd63244eff1145c9622040023d63"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e3df1e35c2f185282b7df057d16a3f6f163ac352","unresolved":false,"context_lines":[{"line_number":86,"context_line":"                        network[\u0027id\u0027])"},{"line_number":87,"context_line":"        return new_agents + existing_agents"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def _schedule_segment(self, context, segment, existing_agents):"},{"line_number":90,"context_line":"        \"\"\"Schedule the segment to new agents"},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"        :return: all agents associated with the segment"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_3f72f349","line":89,"range":{"start_line":89,"start_character":8,"end_line":89,"end_character":25},"updated":"2016-05-13 21:40:39.000000000","message":"This looks an awful lot like the method above.  It makes me wonder if we should just use _schedule_network and give it an optional segment argument.  I\u0027m not suggesting that yet, just thinking out loud.","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":12906,"name":"tonytan4ever","email":"tony.tan@rackspace.com","username":"tonytan4ever"},"change_message_id":"d305d9c4cae2b8623a8ba67e7dadf384ba02f754","unresolved":false,"context_lines":[{"line_number":86,"context_line":"                        network[\u0027id\u0027])"},{"line_number":87,"context_line":"        return new_agents + existing_agents"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def _schedule_segment(self, context, segment, existing_agents):"},{"line_number":90,"context_line":"        \"\"\"Schedule the segment to new agents"},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"        :return: all agents associated with the segment"}],"source_content_type":"text/x-python","patch_set":6,"id":"bab6814e_7583f356","line":89,"range":{"start_line":89,"start_character":8,"end_line":89,"end_character":25},"in_reply_to":"dab17558_3f72f349","updated":"2016-05-19 17:58:09.000000000","message":"If the two function\u0027s logic is largely the same (which it is)m using the same function with an optional name would probably be good.","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e3df1e35c2f185282b7df057d16a3f6f163ac352","unresolved":false,"context_lines":[{"line_number":169,"context_line":"                    context, [segment_id])"},{"line_number":170,"context_line":"                # retain the network information and add the segment to it"},{"line_number":171,"context_line":"                # because the network information will be needed"},{"line_number":172,"context_line":"                segment[\u0027network\u0027] \u003d network"},{"line_number":173,"context_line":"            else:"},{"line_number":174,"context_line":"                agents \u003d self.plugin.get_dhcp_agents_hosting_networks("},{"line_number":175,"context_line":"                    admin_ctx, [network_id])"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_bf7f0365","line":172,"range":{"start_line":172,"start_character":16,"end_line":172,"end_character":44},"updated":"2016-05-13 21:40:39.000000000","message":"Why not just pass network to _schedule_segment below (and, by extension, to plugin.schedule_segment) instead of having it piggy-back on the segment?","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":12906,"name":"tonytan4ever","email":"tony.tan@rackspace.com","username":"tonytan4ever"},"change_message_id":"d305d9c4cae2b8623a8ba67e7dadf384ba02f754","unresolved":false,"context_lines":[{"line_number":169,"context_line":"                    context, [segment_id])"},{"line_number":170,"context_line":"                # retain the network information and add the segment to it"},{"line_number":171,"context_line":"                # because the network information will be needed"},{"line_number":172,"context_line":"                segment[\u0027network\u0027] \u003d network"},{"line_number":173,"context_line":"            else:"},{"line_number":174,"context_line":"                agents \u003d self.plugin.get_dhcp_agents_hosting_networks("},{"line_number":175,"context_line":"                    admin_ctx, [network_id])"}],"source_content_type":"text/x-python","patch_set":6,"id":"bab6814e_95865711","line":172,"range":{"start_line":172,"start_character":16,"end_line":172,"end_character":44},"in_reply_to":"dab17558_bf7f0365","updated":"2016-05-19 17:58:09.000000000","message":"I didn\u0027t see segment[\u0027network\u0027] is used at any place, can you give me a hint on that ?","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":12906,"name":"tonytan4ever","email":"tony.tan@rackspace.com","username":"tonytan4ever"},"change_message_id":"d305d9c4cae2b8623a8ba67e7dadf384ba02f754","unresolved":false,"context_lines":[{"line_number":180,"context_line":"                method \u003d\u003d \u0027port_create_end\u0027 and"},{"line_number":181,"context_line":"                not self._is_reserved_dhcp_port(payload[\u0027port\u0027]))"},{"line_number":182,"context_line":"            if schedule_required:"},{"line_number":183,"context_line":"                if segment_id:"},{"line_number":184,"context_line":"                    agents \u003d self._schedule_segment(admin_ctx, segment, agents)"},{"line_number":185,"context_line":"                else:"},{"line_number":186,"context_line":"                    agents \u003d self._schedule_network(admin_ctx, network, agents)"}],"source_content_type":"text/x-python","patch_set":6,"id":"bab6814e_35c68b48","line":183,"updated":"2016-05-19 17:58:09.000000000","message":"If just using _schedule_network function with optional segment argument this code can be tidied up a little bit.","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e3df1e35c2f185282b7df057d16a3f6f163ac352","unresolved":false,"context_lines":[{"line_number":230,"context_line":"                            {\u0027port_id\u0027: kwargs[\u0027port\u0027][\u0027id\u0027]},"},{"line_number":231,"context_line":"                            kwargs[\u0027port\u0027][\u0027network_id\u0027])"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":"    def notify(self, context, data, method_name):"},{"line_number":234,"context_line":"        # data is {\u0027key\u0027 : \u0027value\u0027} with only one key"},{"line_number":235,"context_line":"        if method_name not in self.VALID_METHOD_NAMES:"},{"line_number":236,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_bf2383a6","line":233,"range":{"start_line":233,"start_character":8,"end_line":233,"end_character":14},"updated":"2016-05-13 21:40:39.000000000","message":"This seems like such a strange method.  It looks like it can be called with a port, subnet, or network.  The only way that would work with segments is calling it with a subnet, right?\n\nI tried briefly to trace where this is being called but the code is so overly generic that it was difficult to make sense of it.  Do you a better sense for it than I do?","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":157,"context_line":"                    context, [segment_id])"},{"line_number":158,"context_line":"            else:"},{"line_number":159,"context_line":"                agents \u003d self.plugin.get_dhcp_agents_hosting_networks("},{"line_number":160,"context_line":"                    admin_ctx, [network_id])"},{"line_number":161,"context_line":""},{"line_number":162,"context_line":"            # schedule the network first, if needed"},{"line_number":163,"context_line":"            schedule_required \u003d ("}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_a5e736e3","line":160,"range":{"start_line":160,"start_character":20,"end_line":160,"end_character":29},"updated":"2016-05-27 20:01:01.000000000","message":"I\u0027m wondering why this changed to admin_ctx from context in the original.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"876ff6f0be4384efc81ca9a048cf79dbd51a10b4","unresolved":false,"context_lines":[{"line_number":157,"context_line":"                    context, [segment_id])"},{"line_number":158,"context_line":"            else:"},{"line_number":159,"context_line":"                agents \u003d self.plugin.get_dhcp_agents_hosting_networks("},{"line_number":160,"context_line":"                    admin_ctx, [network_id])"},{"line_number":161,"context_line":""},{"line_number":162,"context_line":"            # schedule the network first, if needed"},{"line_number":163,"context_line":"            schedule_required \u003d ("}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_a351e964","line":160,"range":{"start_line":160,"start_character":20,"end_line":160,"end_character":29},"in_reply_to":"9abb7d3a_a5e736e3","updated":"2016-05-31 06:43:01.000000000","message":"must an unintended consequence of moving a bunch of stuff around.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"e954092186e37b2171702ed68251ea31dd68ece5","unresolved":false,"context_lines":[{"line_number":150,"context_line":"                # TODO(blogan): import the service plugin\u0027s actual plugin type"},{"line_number":151,"context_line":"                # and use it here whenever that constant is available"},{"line_number":152,"context_line":"                plugin_type \u003d \u0027segments\u0027"},{"line_number":153,"context_line":"                segment_plugin \u003d manager.NeutronManager.get_service_plugins()["},{"line_number":154,"context_line":"                    plugin_type"},{"line_number":155,"context_line":"                ]"},{"line_number":156,"context_line":"                segment \u003d segment_plugin.get_segment(admin_ctx, segment_id)"},{"line_number":157,"context_line":"                segment_ids \u003d [segment_id]"},{"line_number":158,"context_line":"            agents \u003d self.plugin.get_dhcp_agents_hosting_networks("}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_1295b3e3","line":155,"range":{"start_line":153,"start_character":0,"end_line":155,"end_character":17},"updated":"2016-06-06 07:07:22.000000000","message":"Should we consider the possibility that the segment extension is not enabled, and the plugin here might be None.","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"a4f958f9217d02db97263c0051636a05288e5994","unresolved":false,"context_lines":[{"line_number":150,"context_line":"                # TODO(blogan): import the service plugin\u0027s actual plugin type"},{"line_number":151,"context_line":"                # and use it here whenever that constant is available"},{"line_number":152,"context_line":"                plugin_type \u003d \u0027segments\u0027"},{"line_number":153,"context_line":"                segment_plugin \u003d manager.NeutronManager.get_service_plugins()["},{"line_number":154,"context_line":"                    plugin_type"},{"line_number":155,"context_line":"                ]"},{"line_number":156,"context_line":"                segment \u003d segment_plugin.get_segment(admin_ctx, segment_id)"},{"line_number":157,"context_line":"                segment_ids \u003d [segment_id]"},{"line_number":158,"context_line":"            agents \u003d self.plugin.get_dhcp_agents_hosting_networks("}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_17ef3388","line":155,"range":{"start_line":153,"start_character":0,"end_line":155,"end_character":17},"in_reply_to":"7aa08908_1295b3e3","updated":"2016-06-06 21:07:18.000000000","message":"if the segment extension is not enabled then segment_id will always be None.  I think this is safe, unless you can think of a reason this method would be called by anything other than the API code that calls the notify methods.","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"7c46bbc1276a0828feba943feeea1195938f2c7d","unresolved":false,"context_lines":[{"line_number":150,"context_line":"                # TODO(blogan): import the service plugin\u0027s actual plugin type"},{"line_number":151,"context_line":"                # and use it here whenever that constant is available"},{"line_number":152,"context_line":"                plugin_type \u003d \u0027segments\u0027"},{"line_number":153,"context_line":"                segment_plugin \u003d manager.NeutronManager.get_service_plugins()["},{"line_number":154,"context_line":"                    plugin_type"},{"line_number":155,"context_line":"                ]"},{"line_number":156,"context_line":"                segment \u003d segment_plugin.get_segment(admin_ctx, segment_id)"},{"line_number":157,"context_line":"                segment_ids \u003d [segment_id]"},{"line_number":158,"context_line":"            agents \u003d self.plugin.get_dhcp_agents_hosting_networks("}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_7b193a79","line":155,"range":{"start_line":153,"start_character":0,"end_line":155,"end_character":17},"in_reply_to":"7aa08908_17ef3388","updated":"2016-06-07 01:51:50.000000000","message":"thanks","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"e62b1590b7035e5bb32257f78d9ee93980a8c661","unresolved":false,"context_lines":[{"line_number":122,"context_line":"        return port.get(\u0027device_id\u0027) \u003d\u003d n_const.DEVICE_ID_RESERVED_DHCP_PORT"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    def _notify_agents(self, context, method, payload, network_id,"},{"line_number":125,"context_line":"                       subnet_id\u003dNone):"},{"line_number":126,"context_line":"        \"\"\"Notify all the agents that are hosting the network or segment.\"\"\""},{"line_number":127,"context_line":"        # fanout is required as we do not know who is \"listening\""},{"line_number":128,"context_line":"        no_agents \u003d not utils.is_extension_supported("}],"source_content_type":"text/x-python","patch_set":20,"id":"7aa08908_bcbced06","line":125,"range":{"start_line":125,"start_character":23,"end_line":125,"end_character":32},"updated":"2016-06-08 06:32:39.000000000","message":"When the subnet_id is not None, the payload(which is {\u0027subnet\u0027: subnet_obj} if my memory correct) may have the information you want. So I think this parameter could be removed. And even the db query at line 143 could be removed.","commit_id":"4c8316c821a53085d70d4f560e7ce48482e30c5a"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"b1c10e0de9f6806e9e7656a7e5b13f30e4f54116","unresolved":false,"context_lines":[{"line_number":122,"context_line":"        return port.get(\u0027device_id\u0027) \u003d\u003d n_const.DEVICE_ID_RESERVED_DHCP_PORT"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    def _notify_agents(self, context, method, payload, network_id,"},{"line_number":125,"context_line":"                       subnet_id\u003dNone):"},{"line_number":126,"context_line":"        \"\"\"Notify all the agents that are hosting the network or segment.\"\"\""},{"line_number":127,"context_line":"        # fanout is required as we do not know who is \"listening\""},{"line_number":128,"context_line":"        no_agents \u003d not utils.is_extension_supported("}],"source_content_type":"text/x-python","patch_set":20,"id":"7aa08908_f79ccec1","line":125,"range":{"start_line":125,"start_character":23,"end_line":125,"end_character":32},"in_reply_to":"7aa08908_bcbced06","updated":"2016-06-08 06:47:11.000000000","message":"oh good call, i was hoping i could get rid of this parameter and all that db query.  good catch!","commit_id":"4c8316c821a53085d70d4f560e7ce48482e30c5a"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"99aee7a4cb25f98e96c3ee2171623eeb218ba2ad","unresolved":false,"context_lines":[{"line_number":147,"context_line":"                    nm \u003d manager.NeutronManager"},{"line_number":148,"context_line":"                    segment_plugin \u003d nm.get_service_plugins()[\u0027segments\u0027]"},{"line_number":149,"context_line":"                    segment \u003d segment_plugin.get_segment(context, segment_id)"},{"line_number":150,"context_line":"                    network[\u0027segment\u0027] \u003d segment"},{"line_number":151,"context_line":"            agents \u003d self.plugin.get_dhcp_agents_hosting_networks("},{"line_number":152,"context_line":"                context, [network_id], segment_ids\u003dsegment_ids)"},{"line_number":153,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"7aa08908_e424b3b3","line":150,"range":{"start_line":150,"start_character":29,"end_line":150,"end_character":36},"updated":"2016-06-09 13:31:47.000000000","message":"There will be an attribute named \u0027segments\u0027 in network, if there is multiple segments in network. So I would recommend another name here, maybe \u0027schedule_segment\u0027 or something similar.","commit_id":"03174ae8342ecb5e3e438681e46e4a167b035c0d"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"9acaab76453ff6cba1425f17df0e26d97b0f523c","unresolved":false,"context_lines":[{"line_number":144,"context_line":"                segment_plugin \u003d nm.get_service_plugins()[\u0027segments\u0027]"},{"line_number":145,"context_line":"                segment \u003d segment_plugin.get_segment("},{"line_number":146,"context_line":"                    context, payload[\u0027subnet\u0027][\u0027segment_id\u0027])"},{"line_number":147,"context_line":"                network[\u0027candidate_hosts\u0027] \u003d segment[\u0027hosts\u0027]"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"            agents \u003d self.plugin.get_dhcp_agents_hosting_networks("},{"line_number":150,"context_line":"                context, [network_id], hosts\u003dnetwork.get(\u0027candidate_hosts\u0027))"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_11c1112f","line":147,"updated":"2016-06-10 08:23:25.000000000","message":"i still have to send additional information down to the schedule. I don\u0027t see a way around that.","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"03042d31e26e9465a6579245f3d32fca5995e1e1","unresolved":false,"context_lines":[{"line_number":144,"context_line":"                segment_plugin \u003d nm.get_service_plugins()[\u0027segments\u0027]"},{"line_number":145,"context_line":"                segment \u003d segment_plugin.get_segment("},{"line_number":146,"context_line":"                    context, payload[\u0027subnet\u0027][\u0027segment_id\u0027])"},{"line_number":147,"context_line":"                network[\u0027candidate_hosts\u0027] \u003d segment[\u0027hosts\u0027]"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"            agents \u003d self.plugin.get_dhcp_agents_hosting_networks("},{"line_number":150,"context_line":"                context, [network_id], hosts\u003dnetwork.get(\u0027candidate_hosts\u0027))"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_7eb74ff5","line":147,"in_reply_to":"7aa08908_11c1112f","updated":"2016-06-11 08:48:56.000000000","message":"I have the same feeling when working on [1]. When adding external gateway to a router, the router will need to be rescheduled to a subset of l3 agents which has the external network connectivity of the newly added external gateway. \n\nI think it is OK to use candidate_hosts this way first. I might take care of it in future work in [1].\n\n[1] https://review.openstack.org/#/c/320347/1","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"64851d435e6380f0f7037fe64b982c4c53bf3c03","unresolved":false,"context_lines":[{"line_number":144,"context_line":"                segment_plugin \u003d nm.get_service_plugins()[\u0027segments\u0027]"},{"line_number":145,"context_line":"                segment \u003d segment_plugin.get_segment("},{"line_number":146,"context_line":"                    context, payload[\u0027subnet\u0027][\u0027segment_id\u0027])"},{"line_number":147,"context_line":"                network[\u0027candidate_hosts\u0027] \u003d segment[\u0027hosts\u0027]"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"            agents \u003d self.plugin.get_dhcp_agents_hosting_networks("},{"line_number":150,"context_line":"                context, [network_id], hosts\u003dnetwork.get(\u0027candidate_hosts\u0027))"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_362e40a5","line":147,"in_reply_to":"7aa08908_11c1112f","updated":"2016-06-14 07:20:57.000000000","message":"what if you don\u0027t tell the scheduler anything and it just processes all segments for the network?\n\nI\u0027m okay with the scheduler becoming segment aware. I just didn\u0027t want to expose it via the API.","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"30c80756d53e0b00d046f86cbbabc1037f542aa3","unresolved":false,"context_lines":[{"line_number":144,"context_line":"                segment_plugin \u003d nm.get_service_plugins()[\u0027segments\u0027]"},{"line_number":145,"context_line":"                segment \u003d segment_plugin.get_segment("},{"line_number":146,"context_line":"                    context, payload[\u0027subnet\u0027][\u0027segment_id\u0027])"},{"line_number":147,"context_line":"                network[\u0027candidate_hosts\u0027] \u003d segment[\u0027hosts\u0027]"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"            agents \u003d self.plugin.get_dhcp_agents_hosting_networks("},{"line_number":150,"context_line":"                context, [network_id], hosts\u003dnetwork.get(\u0027candidate_hosts\u0027))"}],"source_content_type":"text/x-python","patch_set":22,"id":"3aaa91ec_1eaaebd4","line":147,"in_reply_to":"7aa08908_362e40a5","updated":"2016-06-21 21:29:46.000000000","message":"going to keep it as is for now because I think doing multiple segments on an API request for a specific segment would be odd.  I think more discussion needs to be done for this and can easily be changed in the future.","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"}],"neutron/db/agentschedulers_db.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e3df1e35c2f185282b7df057d16a3f6f163ac352","unresolved":false,"context_lines":[{"line_number":64,"context_line":"                       \u0027selected for automatic scheduling regardless of this \u0027"},{"line_number":65,"context_line":"                       \u0027option. But manual scheduling to such agents is \u0027"},{"line_number":66,"context_line":"                       \u0027available if this option is True.\u0027)),"},{"line_number":67,"context_line":"    cfg.StrOpt(\u0027segment_scheduler_driver\u0027,"},{"line_number":68,"context_line":"               default\u003d\u0027neutron.scheduler.\u0027"},{"line_number":69,"context_line":"                       \u0027dhcp_agent_scheduler.SegmentWeightScheduler\u0027,"},{"line_number":70,"context_line":"               help\u003d_(\u0027Driver to use for scheduling segment to DHCP agent\u0027)),"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_bf0f0387","line":67,"range":{"start_line":67,"start_character":16,"end_line":67,"end_character":40},"updated":"2016-05-13 21:40:39.000000000","message":"Should it be a different driver altogether?  I was thinking more along the lines of enhancing the existing driver to be segment aware with some way to discover if the driver is capable of it.  But, we can weigh the options.  I\u0027m not necessarily saying to change.  Just consider it.\n\nI wonder if any operators can give feedback on how they\u0027d want to configure it.","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e3df1e35c2f185282b7df057d16a3f6f163ac352","unresolved":false,"context_lines":[{"line_number":68,"context_line":"               default\u003d\u0027neutron.scheduler.\u0027"},{"line_number":69,"context_line":"                       \u0027dhcp_agent_scheduler.SegmentWeightScheduler\u0027,"},{"line_number":70,"context_line":"               help\u003d_(\u0027Driver to use for scheduling segment to DHCP agent\u0027)),"},{"line_number":71,"context_line":"    cfg.IntOpt(\u0027dhcp_agents_per_segment\u0027, default\u003d1,"},{"line_number":72,"context_line":"               help\u003d_(\u0027Number of DHCP agents scheduled to host a segment.\u0027"},{"line_number":73,"context_line":"                      \u0027If this number is greater than 1, the \u0027"},{"line_number":74,"context_line":"                      \u0027scheduler automatically assigns multiple DHCP agents \u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_3fe1539b","line":71,"range":{"start_line":71,"start_character":16,"end_line":71,"end_character":39},"updated":"2016-05-13 21:40:39.000000000","message":"I\u0027m wondering if an operator would want this to be different than the dhcp_agents_per_network.  Maybe they would.","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e3df1e35c2f185282b7df057d16a3f6f163ac352","unresolved":false,"context_lines":[{"line_number":79,"context_line":"cfg.CONF.register_opts(AGENTS_SCHEDULER_OPTS)"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"class SegmentDhcpAgentBinding(model_base.BASEV2):"},{"line_number":83,"context_line":"    \"\"\"Represents binding between neutron network segments and DHCP agents.\"\"\""},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    segment_id \u003d sa.Column("}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_7ab929cb","line":82,"range":{"start_line":82,"start_character":6,"end_line":82,"end_character":29},"updated":"2016-05-13 21:40:39.000000000","message":"I was thinking of how to  expand the NetworkDhcpAgentBinding table with a segment_id column which could be null.  But, to be honest, I haven\u0027t worked it out yet in my head.  I\u0027m not sure that we could use one table for both unless we use some non-null dummy value for a network without a segment.  I see why you added another table here.\n\nNow I\u0027m curious if there are any API changes to go along with this.  Do you plan any changes to the neutron/extensions/dhcpagentscheduler.py file?\n\nI imagine that the API for this would be a parallel of the dhcp/network API but for segments.  If that\u0027s the case, I wonder what the following would return for a segmented network:\n\n  GET http://{api}/v2.0/agents/{agent-id}/dhcp-networks.json\n  GET http://{api}/v2.0/networks/{net-id}/dhcp-agents.json\n\nWill they both return empty?  With totally distinct parallel APIs, DB tables, code, and everything, I imagine they would.  My question now is that what we want it to look like?\n\nWe should also consider what happens with these API calls for removing and adding networks to DHCP agents.\n\n  DELETE http://{api}/v2.0/agents/{agent-id}/dhcp-networks/{net-id}.json\n  POST http://{api}/v2.0/agents/{agent-id}/dhcp-networks.json\n        {\"network_id\": \"{net-id}\"}","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":69,"context_line":"cfg.CONF.register_opts(AGENTS_SCHEDULER_OPTS)"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"class SegmentDhcpAgentBinding(model_base.BASEV2):"},{"line_number":73,"context_line":"    \"\"\"Represents binding between neutron network segments and DHCP agents.\"\"\""},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    segment_id \u003d sa.Column("}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_257866f2","line":72,"range":{"start_line":72,"start_character":6,"end_line":72,"end_character":29},"updated":"2016-05-27 20:01:01.000000000","message":"I\u0027m still wondering if adding segment_id to the existing binding table would be better.  What are the pros for doing a separate table like this?\n\nI guess it wouldn\u0027t allow segment_id to be part of the primary key since segment_id would be nullable in such a table.  I think that would be okay though because I\u0027m not sure that segment_id would need to be part of the primary key.  It may need an index though.\n\nThe one thing that would prevent is scheduling multiple segments within the same network to one agent that is connected to more than one.\n\nSo, maybe this model is better afterall.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"876ff6f0be4384efc81ca9a048cf79dbd51a10b4","unresolved":false,"context_lines":[{"line_number":69,"context_line":"cfg.CONF.register_opts(AGENTS_SCHEDULER_OPTS)"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"class SegmentDhcpAgentBinding(model_base.BASEV2):"},{"line_number":73,"context_line":"    \"\"\"Represents binding between neutron network segments and DHCP agents.\"\"\""},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    segment_id \u003d sa.Column("}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_23369903","line":72,"range":{"start_line":72,"start_character":6,"end_line":72,"end_character":29},"in_reply_to":"9abb7d3a_257866f2","updated":"2016-05-31 06:43:01.000000000","message":"So you might be right on this now that I\u0027m more informed on this.  Since we can\u0027t guarantee a dhcp agent on one segment can communicate to other segments, we have to have a different agent per segment.  So adding a segment_id column to the table makes sense to me now.\n\nI\u0027ll add the column instead and see how it works out.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":82,"context_line":"                              primary_key\u003dTrue)"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"class NetworkDhcpAgentBinding(model_base.BASEV2):"},{"line_number":86,"context_line":"    \"\"\"Represents binding between neutron networks and DHCP agents.\"\"\""},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    network_id \u003d sa.Column(sa.String(36),"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_e5b01e8e","line":85,"range":{"start_line":85,"start_character":6,"end_line":85,"end_character":29},"updated":"2016-05-27 20:01:01.000000000","message":"If there is an entry is the previous table, will there also be an entry in the this table?  I tend to think there should be but would like to hear your comments.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"876ff6f0be4384efc81ca9a048cf79dbd51a10b4","unresolved":false,"context_lines":[{"line_number":82,"context_line":"                              primary_key\u003dTrue)"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"class NetworkDhcpAgentBinding(model_base.BASEV2):"},{"line_number":86,"context_line":"    \"\"\"Represents binding between neutron networks and DHCP agents.\"\"\""},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    network_id \u003d sa.Column(sa.String(36),"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_659ce9c0","line":85,"range":{"start_line":85,"start_character":6,"end_line":85,"end_character":29},"in_reply_to":"9abb7d3a_e5b01e8e","updated":"2016-05-31 06:43:01.000000000","message":"i was going to either not insert into this table or isnert network_id and agent_id, hadn\u0027t decided yet.  However, not giong to be an issue now I don\u0027t believe.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":349,"context_line":"                              SegmentDhcpAgentBinding.dhcp_agent))"},{"line_number":350,"context_line":"        query \u003d query.join(SegmentDhcpAgentBinding.dhcp_agent)"},{"line_number":351,"context_line":"        if len(segment_ids) \u003d\u003d 1:"},{"line_number":352,"context_line":"            query \u003d query.filter("},{"line_number":353,"context_line":"                SegmentDhcpAgentBinding.segment_id \u003d\u003d segment_ids[0])"},{"line_number":354,"context_line":"        elif segment_ids:"},{"line_number":355,"context_line":"            query \u003d query.filter("},{"line_number":356,"context_line":"                SegmentDhcpAgentBinding.segment_id in segment_ids)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_45364a04","line":353,"range":{"start_line":352,"start_character":20,"end_line":353,"end_character":69},"updated":"2016-05-27 20:01:01.000000000","message":"I don\u0027t think you need this query.  I think that the query on L355 - L356 would perform just as well as this with one segment.  It looks like you just copied this from L374.  But, I really doubt that was necessary.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"876ff6f0be4384efc81ca9a048cf79dbd51a10b4","unresolved":false,"context_lines":[{"line_number":349,"context_line":"                              SegmentDhcpAgentBinding.dhcp_agent))"},{"line_number":350,"context_line":"        query \u003d query.join(SegmentDhcpAgentBinding.dhcp_agent)"},{"line_number":351,"context_line":"        if len(segment_ids) \u003d\u003d 1:"},{"line_number":352,"context_line":"            query \u003d query.filter("},{"line_number":353,"context_line":"                SegmentDhcpAgentBinding.segment_id \u003d\u003d segment_ids[0])"},{"line_number":354,"context_line":"        elif segment_ids:"},{"line_number":355,"context_line":"            query \u003d query.filter("},{"line_number":356,"context_line":"                SegmentDhcpAgentBinding.segment_id in segment_ids)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_af8503ae","line":353,"range":{"start_line":352,"start_character":20,"end_line":353,"end_character":69},"in_reply_to":"9abb7d3a_45364a04","updated":"2016-05-31 06:43:01.000000000","message":"Done","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"526709b4acd966ebcc41323654ee4d9de7f1ee29","unresolved":false,"context_lines":[{"line_number":339,"context_line":"        query \u003d query.options(orm.contains_eager("},{"line_number":340,"context_line":"                              NetworkDhcpAgentBinding.dhcp_agent))"},{"line_number":341,"context_line":"        query \u003d query.join(NetworkDhcpAgentBinding.dhcp_agent)"},{"line_number":342,"context_line":"        if network_ids:"},{"line_number":343,"context_line":"            query \u003d query.filter("},{"line_number":344,"context_line":"                NetworkDhcpAgentBinding.network_id in network_ids)"},{"line_number":345,"context_line":"        if segment_ids:"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_187a6077","line":342,"range":{"start_line":342,"start_character":8,"end_line":342,"end_character":23},"updated":"2016-05-31 21:20:33.000000000","message":"nit:  I know I pointed out in a previous review that this this code probably wasn\u0027t optimizing anything.  However, that was for the newly added code below to filter on segments.  Technically, we should probably propose a different patch to simplify the existing code for the existing network filter code.","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"276b5e21839f0b085389240138de7a4c92e427a1","unresolved":false,"context_lines":[{"line_number":339,"context_line":"        query \u003d query.options(orm.contains_eager("},{"line_number":340,"context_line":"                              NetworkDhcpAgentBinding.dhcp_agent))"},{"line_number":341,"context_line":"        query \u003d query.join(NetworkDhcpAgentBinding.dhcp_agent)"},{"line_number":342,"context_line":"        if network_ids:"},{"line_number":343,"context_line":"            query \u003d query.filter("},{"line_number":344,"context_line":"                NetworkDhcpAgentBinding.network_id in network_ids)"},{"line_number":345,"context_line":"        if segment_ids:"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_69b92015","line":342,"range":{"start_line":342,"start_character":8,"end_line":342,"end_character":23},"in_reply_to":"9abb7d3a_187a6077","updated":"2016-05-31 22:27:14.000000000","message":"this actually expose a possible bug in thise code, using the in keyword does not actually do what the previous code thought it shoudl be doing. it needs to do\n\nNetworkDhcpAgentBinding.network_id.in_(network_ids)\n\nwhen using the in keyword the sql query ends up doing a WHERE 0 \u003d 1 instead of the WHERE network_id in (\u0027NETWORK_ID\u0027, ...)\n\ni can remove the network pieces out of this change.","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"2c2e00ca298f241ef8a04bb6f353eb84936e4e44","unresolved":false,"context_lines":[{"line_number":80,"context_line":"                              sa.ForeignKey(\"agents.id\","},{"line_number":81,"context_line":"                                            ondelete\u003d\u0027CASCADE\u0027),"},{"line_number":82,"context_line":"                              primary_key\u003dTrue)"},{"line_number":83,"context_line":"    segment_id \u003d sa.Column("},{"line_number":84,"context_line":"        sa.String(36), sa.ForeignKey(\"networksegments.id\", ondelete\u003d\u0027CASCADE\u0027),"},{"line_number":85,"context_line":"        nullable\u003dTrue)"},{"line_number":86,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_d70fd534","line":83,"updated":"2016-06-01 17:25:57.000000000","message":"tl;dr:  I think it is fine to restrict a DHCP agent to one segment on a network.\n\nNote that this model means that a DHCP agent can only serve a single segment because the primary key is still just (network_id, dhcp_agent_id).\n\nI reviewed the ML thread again and thought about it.  I think this is reasonable.  I think that since segments are smaller, DHCP agents could be run on a couple of the compute nodes.  And, hopefully, we can go to a DHCP model that puts DHCP essentially on the port with a light-weight DHCP responder or something.  But, don\u0027t forget metadata.\n\n [1] http://lists.openstack.org/pipermail/openstack-dev/2016-May/095278.html","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"2c2e00ca298f241ef8a04bb6f353eb84936e4e44","unresolved":false,"context_lines":[{"line_number":465,"context_line":"                kwargs[\u0027segment\u0027] \u003d segment"},{"line_number":466,"context_line":"                kwargs[\u0027segment_id\u0027] \u003d segment[\u0027id\u0027]"},{"line_number":467,"context_line":"            return self.network_scheduler.schedule("},{"line_number":468,"context_line":"                self, context, created_network, **kwargs)"},{"line_number":469,"context_line":""},{"line_number":470,"context_line":"    def auto_schedule_networks(self, context, host):"},{"line_number":471,"context_line":"        if self.network_scheduler:"}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_90f8e4c4","line":468,"range":{"start_line":468,"start_character":48,"end_line":468,"end_character":57},"updated":"2016-06-01 17:25:57.000000000","message":"You can just pass them here like this:\n\n  ..., segment\u003dsegment, segment_id\u003dsegment[\u0027id\u0027])","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"650bdf32bf0281b3249754dcf10b7a805c497bcc","unresolved":false,"context_lines":[{"line_number":465,"context_line":"                kwargs[\u0027segment\u0027] \u003d segment"},{"line_number":466,"context_line":"                kwargs[\u0027segment_id\u0027] \u003d segment[\u0027id\u0027]"},{"line_number":467,"context_line":"            return self.network_scheduler.schedule("},{"line_number":468,"context_line":"                self, context, created_network, **kwargs)"},{"line_number":469,"context_line":""},{"line_number":470,"context_line":"    def auto_schedule_networks(self, context, host):"},{"line_number":471,"context_line":"        if self.network_scheduler:"}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_9cb8c812","line":468,"range":{"start_line":468,"start_character":48,"end_line":468,"end_character":57},"in_reply_to":"9abb7d3a_90f8e4c4","updated":"2016-06-01 19:33:39.000000000","message":"ah i couldn\u0027t though bc segment could be None and the [\u0027id\u0027] would fail, so i had to do an explicit check if segment was not None.  I did what you suggested first.  Doesn\u0027t matter though, this is going away :)","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d4a1632346018857edb6b9ab0bd02ada478d87de","unresolved":false,"context_lines":[{"line_number":335,"context_line":"            segment_ids\u003dNone):"},{"line_number":336,"context_line":"        if not network_ids:"},{"line_number":337,"context_line":"            return []"},{"line_number":338,"context_line":"        segment_ids \u003d segment_ids or []"},{"line_number":339,"context_line":"        query \u003d context.session.query(NetworkDhcpAgentBinding)"},{"line_number":340,"context_line":"        query \u003d query.options(orm.contains_eager("},{"line_number":341,"context_line":"                              NetworkDhcpAgentBinding.dhcp_agent))"}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_e8a62e52","line":338,"range":{"start_line":338,"start_character":8,"end_line":338,"end_character":39},"updated":"2016-06-02 20:14:00.000000000","message":"This is unnecessary given L349.","commit_id":"a1165021cd0b64e0b78f739582aad9d203ccc9b9"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"fc4383bf2698a98a82f71145335f46b5466723b9","unresolved":false,"context_lines":[{"line_number":335,"context_line":"            segment_ids\u003dNone):"},{"line_number":336,"context_line":"        if not network_ids:"},{"line_number":337,"context_line":"            return []"},{"line_number":338,"context_line":"        segment_ids \u003d segment_ids or []"},{"line_number":339,"context_line":"        query \u003d context.session.query(NetworkDhcpAgentBinding)"},{"line_number":340,"context_line":"        query \u003d query.options(orm.contains_eager("},{"line_number":341,"context_line":"                              NetworkDhcpAgentBinding.dhcp_agent))"}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_200cca41","line":338,"range":{"start_line":338,"start_character":8,"end_line":338,"end_character":39},"in_reply_to":"9abb7d3a_e8a62e52","updated":"2016-06-02 21:42:04.000000000","message":"oh good catch","commit_id":"a1165021cd0b64e0b78f739582aad9d203ccc9b9"}],"neutron/db/migration/alembic_migrations/versions/newton/expand/98c198bd5848_add_segment_id_to_network_dhcp_agent.py":[{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"0b233b5318d620d9b378275822e811937ab9d297","unresolved":false,"context_lines":[{"line_number":29,"context_line":""},{"line_number":30,"context_line":"def upgrade():"},{"line_number":31,"context_line":"    seg_col \u003d sa.Column(\u0027segment_id\u0027, sa.String(length\u003d36),"},{"line_number":32,"context_line":"                        sa.ForeignKey(\u0027networksegments.id\u0027,"},{"line_number":33,"context_line":"                                      ondelete\u003d\u0027CASCADE\u0027),"},{"line_number":34,"context_line":"                        nullable\u003dTrue)"},{"line_number":35,"context_line":"    op.add_column(\u0027networkdhcpagentbindings\u0027, seg_col)"}],"source_content_type":"text/x-python","patch_set":17,"id":"7aa08908_6551da62","line":32,"updated":"2016-06-05 16:22:40.000000000","message":"The new column depends on the networksegments table which was renamed in a contract migration. Therefore this migration must also go in the contract branch. You will need to implement a contract_creation_exceptions() method similar to https://review.openstack.org/285548","commit_id":"1bb9fd401e1ae7336d1f9e3e628a5787b12ddd43"},{"author":{"_account_id":6524,"name":"Henry Gessau","email":"HenryG@gessau.net","username":"gessau"},"change_message_id":"4b042cfe17dc1e82c278e7122bc46be64edb6f7b","unresolved":false,"context_lines":[{"line_number":29,"context_line":""},{"line_number":30,"context_line":"def upgrade():"},{"line_number":31,"context_line":"    seg_col \u003d sa.Column(\u0027segment_id\u0027, sa.String(length\u003d36),"},{"line_number":32,"context_line":"                        sa.ForeignKey(\u0027networksegments.id\u0027,"},{"line_number":33,"context_line":"                                      ondelete\u003d\u0027CASCADE\u0027),"},{"line_number":34,"context_line":"                        nullable\u003dTrue)"},{"line_number":35,"context_line":"    op.add_column(\u0027networkdhcpagentbindings\u0027, seg_col)"}],"source_content_type":"text/x-python","patch_set":17,"id":"7aa08908_91e2489e","line":32,"in_reply_to":"7aa08908_6551da62","updated":"2016-06-06 02:18:41.000000000","message":"You can rebase your patch on https://review.openstack.org/325699 to get proper branch checking.","commit_id":"1bb9fd401e1ae7336d1f9e3e628a5787b12ddd43"}],"neutron/db/segments_db.py":[{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"0fd55fd629a593d41262cf44df2c12416ef8c786","unresolved":false,"context_lines":[{"line_number":83,"context_line":"            PHYSICAL_NETWORK: record.physical_network,"},{"line_number":84,"context_line":"            SEGMENTATION_ID: record.segmentation_id,"},{"line_number":85,"context_line":"            \u0027hosts\u0027: [mapping.host for mapping in record.segment_host_mapping],"},{"line_number":86,"context_line":"            }"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"def add_network_segment(session, network_id, segment, segment_index\u003d0,"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_ee6d5f9a","line":86,"updated":"2016-05-13 19:02:26.000000000","message":"i need to move this to neutron/services/segments/db.py instead.","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e3df1e35c2f185282b7df057d16a3f6f163ac352","unresolved":false,"context_lines":[{"line_number":83,"context_line":"            PHYSICAL_NETWORK: record.physical_network,"},{"line_number":84,"context_line":"            SEGMENTATION_ID: record.segmentation_id,"},{"line_number":85,"context_line":"            \u0027hosts\u0027: [mapping.host for mapping in record.segment_host_mapping],"},{"line_number":86,"context_line":"            }"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"def add_network_segment(session, network_id, segment, segment_index\u003d0,"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_1a557d4f","line":86,"in_reply_to":"dab17558_ee6d5f9a","updated":"2016-05-13 21:40:39.000000000","message":"+1  That feels like where it should go.  We probably shouldn\u0027t change this older interface.  :)","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"}],"neutron/plugins/ml2/plugin.py":[{"author":{"_account_id":12906,"name":"tonytan4ever","email":"tony.tan@rackspace.com","username":"tonytan4ever"},"change_message_id":"e356d0401d11974954a294cabcccfbadbb85d1eb","unresolved":false,"context_lines":[{"line_number":1635,"context_line":"        # might get ugly.  This implementation isn\u0027t without its own"},{"line_number":1636,"context_line":"        # unsightliness either."},{"line_number":1637,"context_line":"        if segment_id:"},{"line_number":1638,"context_line":"            segments \u003d [db.get_segment_by_id(context.session, segment_id)]"},{"line_number":1639,"context_line":"        else:"},{"line_number":1640,"context_line":"            segments \u003d db.get_network_segments(context.session, network_id)"},{"line_number":1641,"context_line":"        return self.mechanism_manager.filter_hosts_with_segment_access("}],"source_content_type":"text/x-python","patch_set":1,"id":"1a122d0e_9a4a4426","line":1638,"updated":"2016-05-03 18:37:37.000000000","message":"So if both network_id and segment_id are present,  function will be treated as filter with segement_access right ?\nA little unclear to me though.","commit_id":"608633b282c4fd63244eff1145c9622040023d63"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"0fd55fd629a593d41262cf44df2c12416ef8c786","unresolved":false,"context_lines":[{"line_number":184,"context_line":"        )"},{"line_number":185,"context_line":"        self.segment_scheduler \u003d importutils.import_object("},{"line_number":186,"context_line":"            cfg.CONF.segment_scheduler_driver"},{"line_number":187,"context_line":"        )"},{"line_number":188,"context_line":"        self.start_periodic_dhcp_agent_status_check()"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"    def _verify_service_plugins_requirements(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_2eff67ab","line":187,"updated":"2016-05-13 19:02:26.000000000","message":"i decided to have a separate scheduler for segment because it isolates the old behavior from the new nicely.  The only problem with it is that there has to be config options to set it and also set the dhcp agents per segment.  However, I think this is much cleaner and much better than trying to integrate segment scheduling in with the network scheduling.","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e3df1e35c2f185282b7df057d16a3f6f163ac352","unresolved":false,"context_lines":[{"line_number":184,"context_line":"        )"},{"line_number":185,"context_line":"        self.segment_scheduler \u003d importutils.import_object("},{"line_number":186,"context_line":"            cfg.CONF.segment_scheduler_driver"},{"line_number":187,"context_line":"        )"},{"line_number":188,"context_line":"        self.start_periodic_dhcp_agent_status_check()"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"    def _verify_service_plugins_requirements(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"dab17558_ae9fd201","line":187,"in_reply_to":"dab17558_2eff67ab","updated":"2016-05-13 21:40:39.000000000","message":"I wish I would\u0027ve read this first.  I think I see the pain you\u0027re going through here and I do appreciate it from a developers perspective.  This is a little bit more involved than we foresaw.","commit_id":"7e98a05a7c5b7cea89edebb59414aa5ce55a306d"}],"neutron/scheduler/base_resource_filter.py":[{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"f6eba62ecfc2d8e0694f2e89f05fc83bd37e5c62","unresolved":false,"context_lines":[{"line_number":22,"context_line":"    def filter_agents(self, plugin, context, resource):"},{"line_number":23,"context_line":"        \"\"\"Return the agents that can host the resource.\"\"\""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"    def bind(self, context, agents, resource):"},{"line_number":26,"context_line":"        \"\"\"Bind the resource to the agents.\"\"\""},{"line_number":27,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"},{"line_number":28,"context_line":"            res \u003d {}"}],"source_content_type":"text/x-python","patch_set":1,"id":"1a122d0e_ac2449b2","line":25,"range":{"start_line":25,"start_character":36,"end_line":25,"end_character":44},"updated":"2016-05-03 05:17:04.000000000","message":"This has me extremely nervous about doing so I may end up doing this entirely differently.  While I can change this here and all resource filters that inherit/override this method, out-of-tree ones I cannot.  BGP is one example, so really this needs to be rethought.","commit_id":"608633b282c4fd63244eff1145c9622040023d63"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"2c2e00ca298f241ef8a04bb6f353eb84936e4e44","unresolved":false,"context_lines":[{"line_number":22,"context_line":"class BaseResourceFilter(object):"},{"line_number":23,"context_line":"    \"\"\"Encapsulate logic that is specific to the resource type.\"\"\""},{"line_number":24,"context_line":"    @abc.abstractmethod"},{"line_number":25,"context_line":"    def filter_agents(self, plugin, context, resource, **kwargs):"},{"line_number":26,"context_line":"        \"\"\"Return the agents that can host the resource.\"\"\""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    def bind(self, context, agents, resource_id, **kwargs):"}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_73939c5b","line":25,"range":{"start_line":25,"start_character":55,"end_line":25,"end_character":63},"updated":"2016-06-01 17:25:57.000000000","message":"ditto (next file)","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"}],"neutron/scheduler/base_scheduler.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"2c2e00ca298f241ef8a04bb6f353eb84936e4e44","unresolved":false,"context_lines":[{"line_number":34,"context_line":"               resource_hosted_agents, num_agents_needed):"},{"line_number":35,"context_line":"        \"\"\"Return a subset of agents based on the specific scheduling logic.\"\"\""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    def schedule(self, plugin, context, resource, **kwargs):"},{"line_number":38,"context_line":"        \"\"\"Select and bind agents to a given resource.\"\"\""},{"line_number":39,"context_line":"        if not self.resource_filter:"},{"line_number":40,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_f38dcc31","line":37,"range":{"start_line":37,"start_character":50,"end_line":37,"end_character":58},"updated":"2016-06-01 17:25:57.000000000","message":"I have mixed feelings about this.  Essentially, you\u0027re saying that a caller who knows its working with a sub-class of this, but still calling using this method signature, can pass whatever opaque data it knows that the sub-class will understand.  This seems to be an abuse of the inheritance paradigm.\n\nI still tend to think that if the caller knows it is a special sub-class of the base resource filter, then the method name should change to formalize the fact that the caller *needs* an instance of the sub-class.","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e529d646ade0a1e9e5295b5be41498e6db4c8903","unresolved":false,"context_lines":[{"line_number":34,"context_line":"               resource_hosted_agents, num_agents_needed):"},{"line_number":35,"context_line":"        \"\"\"Return a subset of agents based on the specific scheduling logic.\"\"\""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    def schedule(self, plugin, context, resource, **kwargs):"},{"line_number":38,"context_line":"        \"\"\"Select and bind agents to a given resource.\"\"\""},{"line_number":39,"context_line":"        if not self.resource_filter:"},{"line_number":40,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_e8ee2eb3","line":37,"range":{"start_line":37,"start_character":50,"end_line":37,"end_character":58},"in_reply_to":"9abb7d3a_3ca43c06","updated":"2016-06-02 20:00:52.000000000","message":"With a new method, the base class doesn\u0027t have to be changed.  I think this is goodness.\n\nAlso, with a new method, you *are* adhering to an interface.  It is just an interface that is only implemented by certain sub-classes.  As far as I know, this is acceptable object oriented.  There are lots of cases where the caller knows that it is working with an instance of a sub-class and can use interfaces specific to that sub-class.  I see this kind of thing everywhere and I think the following sums it up:\n\n  A List is Iterable but not all Iterables are Lists.\n  A DhcpScheduler is a Scheduler but not all Schedulers are DhcpSchedulers.","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"650bdf32bf0281b3249754dcf10b7a805c497bcc","unresolved":false,"context_lines":[{"line_number":34,"context_line":"               resource_hosted_agents, num_agents_needed):"},{"line_number":35,"context_line":"        \"\"\"Return a subset of agents based on the specific scheduling logic.\"\"\""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    def schedule(self, plugin, context, resource, **kwargs):"},{"line_number":38,"context_line":"        \"\"\"Select and bind agents to a given resource.\"\"\""},{"line_number":39,"context_line":"        if not self.resource_filter:"},{"line_number":40,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_3ca43c06","line":37,"range":{"start_line":37,"start_character":50,"end_line":37,"end_character":58},"in_reply_to":"9abb7d3a_f38dcc31","updated":"2016-06-01 19:33:39.000000000","message":"there\u0027s not too much difference between a caller knowing what parameters a scheduler can handle that the base class can\u0027t, vs what method a scheduler can handle that the base class can\u0027t.  Both are not adhering to a strict interface.  Unless you\u0027re suggesting the base class has a \"schedule_dhcp\" method.\n\nI will say a benefit of doing another method vs kwargs, is that the caller can check if that method is supported a few different ways, so thats probably a big difference and you\u0027ve convinced me :)","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"2c2e00ca298f241ef8a04bb6f353eb84936e4e44","unresolved":false,"context_lines":[{"line_number":40,"context_line":"            return"},{"line_number":41,"context_line":"        # filter the agents that can host the resource"},{"line_number":42,"context_line":"        filtered_agents_dict \u003d self.resource_filter.filter_agents("},{"line_number":43,"context_line":"            plugin, context, resource, **kwargs)"},{"line_number":44,"context_line":"        num_agents \u003d filtered_agents_dict[\u0027n_agents\u0027]"},{"line_number":45,"context_line":"        hostable_agents \u003d filtered_agents_dict[\u0027hostable_agents\u0027]"},{"line_number":46,"context_line":"        hosted_agents \u003d filtered_agents_dict[\u0027hosted_agents\u0027]"}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_b3f6a45a","line":43,"range":{"start_line":43,"start_character":39,"end_line":43,"end_character":47},"updated":"2016-06-01 17:25:57.000000000","message":"This kind of makes me feel even worse.  The caller of \"schedule\" has to send in opaque data to that call knowing that it will pass said opaque data to the resource_filter methods which will be able to interpret it because it is an instance of a particular special sub-class of the resource filter.\n\nIt just smells and I\u0027m having trouble talking myself into accepting it.","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"650bdf32bf0281b3249754dcf10b7a805c497bcc","unresolved":false,"context_lines":[{"line_number":40,"context_line":"            return"},{"line_number":41,"context_line":"        # filter the agents that can host the resource"},{"line_number":42,"context_line":"        filtered_agents_dict \u003d self.resource_filter.filter_agents("},{"line_number":43,"context_line":"            plugin, context, resource, **kwargs)"},{"line_number":44,"context_line":"        num_agents \u003d filtered_agents_dict[\u0027n_agents\u0027]"},{"line_number":45,"context_line":"        hostable_agents \u003d filtered_agents_dict[\u0027hostable_agents\u0027]"},{"line_number":46,"context_line":"        hosted_agents \u003d filtered_agents_dict[\u0027hosted_agents\u0027]"}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_9cb3a84c","line":43,"range":{"start_line":43,"start_character":39,"end_line":43,"end_character":47},"in_reply_to":"9abb7d3a_b3f6a45a","updated":"2016-06-01 19:33:39.000000000","message":"yeah i hated these too, and another reason i\u0027m so willing to drop this strategy.","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"8f4ffa4f1468ecba82216134bc020950703a34f3","unresolved":false,"context_lines":[{"line_number":48,"context_line":"                                    hosted_agents, num_agents)"},{"line_number":49,"context_line":"        # bind the resource to the agents"},{"line_number":50,"context_line":"        self.resource_filter.bind(context, chosen_agents, resource[\u0027id\u0027],"},{"line_number":51,"context_line":"                                  **kwargs)"},{"line_number":52,"context_line":"        return chosen_agents"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_cd4937b9","line":51,"range":{"start_line":51,"start_character":34,"end_line":51,"end_character":42},"updated":"2016-05-31 23:25:16.000000000","message":"having to pass kwargs into this method and the filter_agents method on line 42 are not ideal, but i think this so far is the best bad option.  Maybe someone else can have a better idea?","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"650bdf32bf0281b3249754dcf10b7a805c497bcc","unresolved":false,"context_lines":[{"line_number":48,"context_line":"                                    hosted_agents, num_agents)"},{"line_number":49,"context_line":"        # bind the resource to the agents"},{"line_number":50,"context_line":"        self.resource_filter.bind(context, chosen_agents, resource[\u0027id\u0027],"},{"line_number":51,"context_line":"                                  **kwargs)"},{"line_number":52,"context_line":"        return chosen_agents"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_3c7b9c7c","line":51,"range":{"start_line":51,"start_character":34,"end_line":51,"end_character":42},"in_reply_to":"9abb7d3a_505b1cf6","updated":"2016-06-01 19:33:39.000000000","message":"agreed.","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"2c2e00ca298f241ef8a04bb6f353eb84936e4e44","unresolved":false,"context_lines":[{"line_number":48,"context_line":"                                    hosted_agents, num_agents)"},{"line_number":49,"context_line":"        # bind the resource to the agents"},{"line_number":50,"context_line":"        self.resource_filter.bind(context, chosen_agents, resource[\u0027id\u0027],"},{"line_number":51,"context_line":"                                  **kwargs)"},{"line_number":52,"context_line":"        return chosen_agents"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_505b1cf6","line":51,"range":{"start_line":51,"start_character":34,"end_line":51,"end_character":42},"in_reply_to":"9abb7d3a_cd4937b9","updated":"2016-06-01 17:25:57.000000000","message":"My first thought was that maybe we shouldn\u0027t make it look like generic kwargs, maybe we should call it resource_args or something and just pass it through as an opaque handle to the resource filter methods.\n\nBut, either way, it is still an opaque handle to some data that the base class can\u0027t understand.","commit_id":"86f38dc22354b4283588be3248ed0c40f057d38c"}],"neutron/scheduler/dhcp_agent_scheduler.py":[{"author":{"_account_id":12906,"name":"tonytan4ever","email":"tony.tan@rackspace.com","username":"tonytan4ever"},"change_message_id":"e15c9f9de8ce5d36b629b0bb01beeb90595c65b5","unresolved":false,"context_lines":[{"line_number":174,"context_line":"            res_id \u003d network[\u0027segment\u0027][\u0027id\u0027]"},{"line_number":175,"context_line":"        else:"},{"line_number":176,"context_line":"            res_id \u003d network[\u0027id\u0027]"},{"line_number":177,"context_line":"        super(DhcpFilter, self).bind(context, bound_agents, res_id)"},{"line_number":178,"context_line":""},{"line_number":179,"context_line":"    def filter_agents(self, plugin, context, network):"},{"line_number":180,"context_line":"        \"\"\"Return the agents that can host the network or segment."}],"source_content_type":"text/x-python","patch_set":1,"id":"faac7902_12ff8c35","line":177,"updated":"2016-05-05 19:38:11.000000000","message":"This bind with the id, which is contradicting the:\nhttps://review.openstack.org/#/c/311931/1/neutron/scheduler/base_resource_filter.py\nbinding with the resource.","commit_id":"608633b282c4fd63244eff1145c9622040023d63"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":197,"context_line":"                return"},{"line_number":198,"context_line":"        return network_hosted_agents"},{"line_number":199,"context_line":""},{"line_number":200,"context_line":"    def _get_active_agents(self, plugin, context, az_hints):"},{"line_number":201,"context_line":"        \"\"\"Return a list of active dhcp agents.\"\"\""},{"line_number":202,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"},{"line_number":203,"context_line":"            filters \u003d {\u0027agent_type\u0027: [constants.AGENT_TYPE_DHCP],"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_3a6881f5","side":"PARENT","line":200,"range":{"start_line":200,"start_character":8,"end_line":200,"end_character":26},"updated":"2016-05-27 20:01:01.000000000","message":"Why are you moving this method?  I\u0027d prefer it stay where it is so that this diff shows what changes (or doesn\u0027t change as is the case in this one).","commit_id":"f1ebd7ab1a0f007e49588e882950e4a3a35db334"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"876ff6f0be4384efc81ca9a048cf79dbd51a10b4","unresolved":false,"context_lines":[{"line_number":197,"context_line":"                return"},{"line_number":198,"context_line":"        return network_hosted_agents"},{"line_number":199,"context_line":""},{"line_number":200,"context_line":"    def _get_active_agents(self, plugin, context, az_hints):"},{"line_number":201,"context_line":"        \"\"\"Return a list of active dhcp agents.\"\"\""},{"line_number":202,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"},{"line_number":203,"context_line":"            filters \u003d {\u0027agent_type\u0027: [constants.AGENT_TYPE_DHCP],"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_aa50515f","side":"PARENT","line":200,"range":{"start_line":200,"start_character":8,"end_line":200,"end_character":26},"in_reply_to":"9abb7d3a_3a6881f5","updated":"2016-05-31 06:43:01.000000000","message":"whoops, good catch","commit_id":"f1ebd7ab1a0f007e49588e882950e4a3a35db334"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"e7959d03cf704126baff0315cc2c1edf2ae4bf02","unresolved":false,"context_lines":[{"line_number":89,"context_line":"    def __init__(self):"},{"line_number":90,"context_line":"        super(ChanceScheduler, self).__init__(DhcpFilter())"},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"    def schedule(self, plugin, context, network, segment\u003dNone):"},{"line_number":93,"context_line":"        \"\"\"Select and bind agents to a network."},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"        This will optionally take in a segment resource if there is a need for"}],"source_content_type":"text/x-python","patch_set":9,"id":"bab6814e_b2a3ea4e","line":92,"updated":"2016-05-24 15:44:39.000000000","message":"this method and the one below are not going to be the end solution, it\u0027s just something I\u0027ve kept so it might be easier to see the issue with how the base scheduler only accepts one resource, and the base filter\u0027s bind only accepts one resource_id.  With the possibility of the scheduler needing to bind to a network OR a segment if one exists, the base classes\u0027 interface does not work.  Anyone have any ideas on how to get around this?\n\nMy alternative method to get around this was to instantiate a segment_scheduler alongside the network_scheduler in the plugin, but that meant having 2 different schedulers to configure, so it wasn\u0027t ideal.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":116,"context_line":"    def __init__(self):"},{"line_number":117,"context_line":"        super(WeightScheduler, self).__init__(DhcpFilter())"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def schedule(self, plugin, context, network, segment\u003dNone):"},{"line_number":120,"context_line":"        \"\"\"Select and bind agents to a network."},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"        This will optionally take in a segment resource if there is a need for"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_5aa105a9","line":119,"updated":"2016-05-27 20:01:01.000000000","message":"As far as I can tell, this is exactly the same as the one above.  I think this method could be added in a third mixin style class despite the fact that some think that mixin is a bad word.  I think it is the right solution for this.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":185,"context_line":"        \"\"\"Bind the network to the agents.\"\"\""},{"line_number":186,"context_line":"        # customize the bind logic"},{"line_number":187,"context_line":"        bound_agents \u003d agents[:]"},{"line_number":188,"context_line":"        resource_id \u003d segment_id if segment_id else network_id"},{"line_number":189,"context_line":"        for agent in agents:"},{"line_number":190,"context_line":"            # saving agent_id to use it after rollback to avoid"},{"line_number":191,"context_line":"            # DetachedInstanceError"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_faaa796f","line":188,"range":{"start_line":188,"start_character":8,"end_line":188,"end_character":19},"updated":"2016-05-27 20:01:01.000000000","message":"I\u0027m usually not a fan of conflating two concepts like this.  I think I\u0027d rather keep network_id and segment_id separate instead of trying to send one or the other through the same code path.  In my experience, it rarely ends up with better code to go this route.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":191,"context_line":"            # DetachedInstanceError"},{"line_number":192,"context_line":"            agent_id \u003d agent.id"},{"line_number":193,"context_line":"            binding \u003d self._get_binding(agent_id, resource_id,"},{"line_number":194,"context_line":"                                        is_segment\u003dsegment_id is not None)"},{"line_number":195,"context_line":"            try:"},{"line_number":196,"context_line":"                with db_api.autonested_transaction(context.session):"},{"line_number":197,"context_line":"                    context.session.add(binding)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_daf8557f","line":194,"range":{"start_line":194,"start_character":40,"end_line":194,"end_character":73},"updated":"2016-05-27 20:01:01.000000000","message":"I think I\u0027d rather send both network_id and segment_id down instead of this is_segment boolean.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":209,"context_line":"                       \u0027agent_id\u0027: agent_id})"},{"line_number":210,"context_line":"        super(DhcpFilter, self).bind(context, bound_agents, resource_id)"},{"line_number":211,"context_line":""},{"line_number":212,"context_line":"    def _get_binding(self, agent_id, resource_id, is_segment\u003dFalse):"},{"line_number":213,"context_line":"        if is_segment:"},{"line_number":214,"context_line":"            binding \u003d agentschedulers_db.SegmentDhcpAgentBinding()"},{"line_number":215,"context_line":"            binding.segment_id \u003d resource_id"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_da6df533","line":212,"range":{"start_line":212,"start_character":37,"end_line":212,"end_character":66},"updated":"2016-05-27 20:01:01.000000000","message":"I think this method signature would be better like the next one (network, segment\u003dNone).  I like that pattern better.  The if / else on L213 - L218 would end up looking more like the one on L232 - L237.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":264,"context_line":"        return hostable_dhcp_agents"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _construct_agents_dict(self, hostable_dhcp_agents, hosted_agents):"},{"line_number":267,"context_line":"        n_agents \u003d cfg.CONF.dhcp_agents_per_network - len(hosted_agents)"},{"line_number":268,"context_line":"        if not hostable_dhcp_agents:"},{"line_number":269,"context_line":"            return {\u0027n_agents\u0027: 0, \u0027hostable_agents\u0027: [],"},{"line_number":270,"context_line":"                    \u0027hosted_agents\u0027: hosted_agents}"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_3af36175","line":267,"range":{"start_line":267,"start_character":19,"end_line":267,"end_character":72},"updated":"2016-05-27 20:01:01.000000000","message":"Should this be wrapped in max(0, ...) to ensure it doesn\u0027t go negative for whatever reason?  (For example, the config could be decreased.  When the server comes back up, it could be less than hosted_agents and this will end up negative.)","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _construct_agents_dict(self, hostable_dhcp_agents, hosted_agents):"},{"line_number":267,"context_line":"        n_agents \u003d cfg.CONF.dhcp_agents_per_network - len(hosted_agents)"},{"line_number":268,"context_line":"        if not hostable_dhcp_agents:"},{"line_number":269,"context_line":"            return {\u0027n_agents\u0027: 0, \u0027hostable_agents\u0027: [],"},{"line_number":270,"context_line":"                    \u0027hosted_agents\u0027: hosted_agents}"},{"line_number":271,"context_line":"        n_agents \u003d min(len(hostable_dhcp_agents), n_agents)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_7aede98a","line":268,"range":{"start_line":268,"start_character":8,"end_line":268,"end_character":36},"updated":"2016-05-27 20:01:01.000000000","message":"nit:  Move this before the above line.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":266,"context_line":"    def _construct_agents_dict(self, hostable_dhcp_agents, hosted_agents):"},{"line_number":267,"context_line":"        n_agents \u003d cfg.CONF.dhcp_agents_per_network - len(hosted_agents)"},{"line_number":268,"context_line":"        if not hostable_dhcp_agents:"},{"line_number":269,"context_line":"            return {\u0027n_agents\u0027: 0, \u0027hostable_agents\u0027: [],"},{"line_number":270,"context_line":"                    \u0027hosted_agents\u0027: hosted_agents}"},{"line_number":271,"context_line":"        n_agents \u003d min(len(hostable_dhcp_agents), n_agents)"},{"line_number":272,"context_line":"        return {\u0027n_agents\u0027: n_agents, \u0027hostable_agents\u0027: hostable_dhcp_agents,"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_7aa4c956","line":269,"range":{"start_line":269,"start_character":33,"end_line":269,"end_character":35},"updated":"2016-05-27 20:01:01.000000000","message":"femtonit:  I prefer one line per item unless they all fit on one line.  But, that might be just my personal preference.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":269,"context_line":"            return {\u0027n_agents\u0027: 0, \u0027hostable_agents\u0027: [],"},{"line_number":270,"context_line":"                    \u0027hosted_agents\u0027: hosted_agents}"},{"line_number":271,"context_line":"        n_agents \u003d min(len(hostable_dhcp_agents), n_agents)"},{"line_number":272,"context_line":"        return {\u0027n_agents\u0027: n_agents, \u0027hostable_agents\u0027: hostable_dhcp_agents,"},{"line_number":273,"context_line":"                \u0027hosted_agents\u0027: hosted_agents}"},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"    def _filter_agents_with_network_access(self, hostable_dhcp_agents, plugin,"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_faf9d94e","line":272,"range":{"start_line":272,"start_character":36,"end_line":272,"end_character":38},"updated":"2016-05-27 20:01:01.000000000","message":"ditto (L269)","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":273,"context_line":"                \u0027hosted_agents\u0027: hosted_agents}"},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"    def _filter_agents_with_network_access(self, hostable_dhcp_agents, plugin,"},{"line_number":276,"context_line":"                                        context, network):"},{"line_number":277,"context_line":"        hostable_dhcp_hosts \u003d plugin.filter_hosts_with_network_access("},{"line_number":278,"context_line":"            context, network[\u0027id\u0027],"},{"line_number":279,"context_line":"            [agent[\u0027host\u0027] for agent in hostable_dhcp_agents])"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_ba8631b7","line":276,"range":{"start_line":276,"start_character":40,"end_line":276,"end_character":43},"updated":"2016-05-27 20:01:01.000000000","message":"nit:  Funny indentation.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":277,"context_line":"        hostable_dhcp_hosts \u003d plugin.filter_hosts_with_network_access("},{"line_number":278,"context_line":"            context, network[\u0027id\u0027],"},{"line_number":279,"context_line":"            [agent[\u0027host\u0027] for agent in hostable_dhcp_agents])"},{"line_number":280,"context_line":"        hostable_dhcp_agents \u003d [agent for agent in hostable_dhcp_agents"},{"line_number":281,"context_line":"                                if agent[\u0027host\u0027] in hostable_dhcp_hosts]"},{"line_number":282,"context_line":"        return hostable_dhcp_agents"},{"line_number":283,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_fa03990d","line":280,"range":{"start_line":280,"start_character":8,"end_line":280,"end_character":28},"updated":"2016-05-27 20:01:01.000000000","message":"nit:  avoid shadowing the original value passed in.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":305,"context_line":"        network, and a list of DHCP agents currently hosting the network."},{"line_number":306,"context_line":"        \"\"\""},{"line_number":307,"context_line":"        hosted_agents \u003d self._get_dhcp_agents_hosting_network("},{"line_number":308,"context_line":"            plugin, context, network)"},{"line_number":309,"context_line":"        if hosted_agents is None:"},{"line_number":310,"context_line":"            return {\u0027n_agents\u0027: 0, \u0027hostable_agents\u0027: [], \u0027hosted_agents\u0027: []}"},{"line_number":311,"context_line":"        az_hints \u003d (network.get(az_ext.AZ_HINTS) or"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_da5e9544","line":308,"range":{"start_line":308,"start_character":12,"end_line":308,"end_character":37},"updated":"2016-05-27 20:01:01.000000000","message":"nit:  avoid gratuitous whitespace changes","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":314,"context_line":"        if not active_dhcp_agents:"},{"line_number":315,"context_line":"            return {\u0027n_agents\u0027: 0, \u0027hostable_agents\u0027: [],"},{"line_number":316,"context_line":"                    \u0027hosted_agents\u0027: hosted_agents}"},{"line_number":317,"context_line":"        hostable_dhcp_agents \u003d self._get_hostable_agents("},{"line_number":318,"context_line":"            plugin, context, hosted_agents, active_dhcp_agents)"},{"line_number":319,"context_line":"        hostable_dhcp_agents \u003d self._filter_agents_with_network_access("},{"line_number":320,"context_line":"            hostable_dhcp_agents, plugin, context, network)"},{"line_number":321,"context_line":"        return self._construct_agents_dict(hostable_dhcp_agents, hosted_agents)"},{"line_number":322,"context_line":""},{"line_number":323,"context_line":"    def _get_dhcp_agents_hosting_segment(self, plugin, context, segment):"},{"line_number":324,"context_line":"        \"\"\"Return dhcp agents hosting the given segment or None if a given"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_a5573601","line":321,"range":{"start_line":317,"start_character":8,"end_line":321,"end_character":79},"updated":"2016-05-27 20:01:01.000000000","message":"Some of this looks like some refactoring which could be done first and probably merged quickly simplifying the final patch for reviewers.  As a reviewer, I find it very beneficial to do pure refactoring that is needed first, and then follow up with the changes.  To me, the sum of the two patches ends up being easier to review that the big patch.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":320,"context_line":"            hostable_dhcp_agents, plugin, context, network)"},{"line_number":321,"context_line":"        return self._construct_agents_dict(hostable_dhcp_agents, hosted_agents)"},{"line_number":322,"context_line":""},{"line_number":323,"context_line":"    def _get_dhcp_agents_hosting_segment(self, plugin, context, segment):"},{"line_number":324,"context_line":"        \"\"\"Return dhcp agents hosting the given segment or None if a given"},{"line_number":325,"context_line":"           segment is already hosted by enough number of agents."},{"line_number":326,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_e572de7b","line":323,"range":{"start_line":323,"start_character":64,"end_line":323,"end_character":71},"updated":"2016-05-27 20:01:01.000000000","message":"consider just passing segment_id here.  (Law of Demeter)","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":326,"context_line":"        \"\"\""},{"line_number":327,"context_line":"        # NOTE(blogan): Should we reuse this config option to also determine"},{"line_number":328,"context_line":"        # how many agents per segment? That seems like it might get confusing."},{"line_number":329,"context_line":"        # We could make a new agents_per_segment option and have it default"},{"line_number":330,"context_line":"        # to the agents_per_network value."},{"line_number":331,"context_line":"        agents_per_network \u003d cfg.CONF.dhcp_agents_per_network"},{"line_number":332,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"},{"line_number":333,"context_line":"            hosted_agents \u003d plugin.get_dhcp_agents_hosting_segments("}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_659b8eca","line":330,"range":{"start_line":329,"start_character":26,"end_line":330,"end_character":41},"updated":"2016-05-27 20:01:01.000000000","message":"Not a bad idea.  But, I vote we keep it simple at first and then follow up with more complexity as its needed.  I think using the same option for both is not a bad starting point.  Also, by putting it in a follow-on patch, it allows the conversation to happen in that review all by itself which tends to produce a more focused discussion.  We don\u0027t really have to nail it down until Newton is released.  Then, we\u0027ve got to support whatever we end up with.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":332,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"},{"line_number":333,"context_line":"            hosted_agents \u003d plugin.get_dhcp_agents_hosting_segments("},{"line_number":334,"context_line":"                context, [segment[\u0027id\u0027]])"},{"line_number":335,"context_line":"            if len(hosted_agents) \u003e\u003d agents_per_network:"},{"line_number":336,"context_line":"                LOG.debug(\u0027Segment %s is already hosted by enough agents.\u0027,"},{"line_number":337,"context_line":"                          segment[\u0027id\u0027])"},{"line_number":338,"context_line":"                return"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_450acad3","line":335,"range":{"start_line":335,"start_character":34,"end_line":335,"end_character":36},"updated":"2016-05-27 20:01:01.000000000","message":"This supports my comment on L267.  This code plans for the possibility of more in hosted_agents.","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":335,"context_line":"            if len(hosted_agents) \u003e\u003d agents_per_network:"},{"line_number":336,"context_line":"                LOG.debug(\u0027Segment %s is already hosted by enough agents.\u0027,"},{"line_number":337,"context_line":"                          segment[\u0027id\u0027])"},{"line_number":338,"context_line":"                return"},{"line_number":339,"context_line":"        return hosted_agents"},{"line_number":340,"context_line":""},{"line_number":341,"context_line":"    def _filter_agents_with_segment_access(self, hostable_agents, segment):"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_a5261666","line":338,"range":{"start_line":338,"start_character":16,"end_line":338,"end_character":22},"updated":"2016-05-27 20:01:01.000000000","message":"It seems odd to return None for \"agents_hosting_segments\" even if there are enough.  Wouldn\u0027t you still want to return them given the name of this method?","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a7b51755e4902052323b111fab32d4870752f64b","unresolved":false,"context_lines":[{"line_number":338,"context_line":"                return"},{"line_number":339,"context_line":"        return hosted_agents"},{"line_number":340,"context_line":""},{"line_number":341,"context_line":"    def _filter_agents_with_segment_access(self, hostable_agents, segment):"},{"line_number":342,"context_line":"        hostable_dhcp_agents \u003d [agent for agent in hostable_agents"},{"line_number":343,"context_line":"                                if agent[\u0027host\u0027] in segment[\u0027hosts\u0027]]"},{"line_number":344,"context_line":"        return hostable_dhcp_agents"}],"source_content_type":"text/x-python","patch_set":9,"id":"9abb7d3a_658f0e24","line":341,"updated":"2016-05-27 20:01:01.000000000","message":"Consider passing only host (law of demeter again)","commit_id":"fed7e00a1d9e16d47d60ed9a6e0f46104491449c"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"526709b4acd966ebcc41323654ee4d9de7f1ee29","unresolved":false,"context_lines":[{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class SegmentScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def schedule(self, plugin, context, network, segment\u003dNone):"},{"line_number":90,"context_line":"        \"\"\"Select and bind agents to a network."},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"        This will optionally take in a segment resource if there is a need for"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_3dc6369a","line":89,"range":{"start_line":89,"start_character":49,"end_line":89,"end_character":61},"updated":"2016-05-31 21:20:33.000000000","message":"I\u0027m not a fan of changing the call signature of an inherited method like this.  To call this correctly, the caller needs to know it is working with an instance of SegmentScheduler.  I think this is a little bit of an abuse of inheritance.  Especially given that the base scheduler was designed to work with a single resource and this one is designed to take in to account a network and segment.\n\nPart of me thinks maybe inheritance might be the wrong thing here.  Maybe the DHCP scheduler could instantiate a base scheduler and make use of it without inheriting (composition).  Or, the base scheduler could be refactored a little bit in order to allow the filtering and binding steps to more easily take extra information in to account.\n\nAnother option is to use a different method name, like schedule_dhcp(...).  That would solve the MRO issue below.  Callers already have to know they\u0027re calling it a little bit differently.  Changing the name kind of formalizes that.  If you do nothing else, I\u0027d recommend at least doing this.  It will solve the MRO issue below and avoid changing the call signature of \"schedule\".","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"276b5e21839f0b085389240138de7a4c92e427a1","unresolved":false,"context_lines":[{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class SegmentScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def schedule(self, plugin, context, network, segment\u003dNone):"},{"line_number":90,"context_line":"        \"\"\"Select and bind agents to a network."},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"        This will optionally take in a segment resource if there is a need for"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_e92230a2","line":89,"range":{"start_line":89,"start_character":49,"end_line":89,"end_character":61},"in_reply_to":"9abb7d3a_3dc6369a","updated":"2016-05-31 22:27:14.000000000","message":"I know, we discussed this in IRC the other day.  I hate it too because of it changing the call signature.\n\nI did try your suggestion with the approach of having to refactor the base scheduler a bit to allow filtering and binding with extra information, but then that\u0027s breaking the interface and will break out of tree schedulers and filters like bgp has.\n\nI\u0027ll try composition of the base scheduler out too, but that doesn\u0027t feel right.  Then again none of these do :(\n\nAnother option is I was thinking that I could just throw the segment into the network dictionary, and just check for its existence in the schedule body to keep the call signature the same.  However, this will not solve the issue with the DhcpFilter.bind method because it just takes network_id, so that needs to change the call signature, so probably not worth it.\n\nMy best alternative was making a segment_aware_schedule method and segment_aware_bind method like you suggested, but then I was thinking, whats the point of even having a base scheduler or filter? the base scheduler\u0027s schedule method won\u0027t be used which is almost the entirety of its function.  The select methods are defined in the Weight, Chance, and AZAware classes.  Perhaps thats overthinking that one too?","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"403e475d842bb0d3898cfd735551a62f492a5c4d","unresolved":false,"context_lines":[{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class SegmentScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def schedule(self, plugin, context, network, segment\u003dNone):"},{"line_number":90,"context_line":"        \"\"\"Select and bind agents to a network."},{"line_number":91,"context_line":""},{"line_number":92,"context_line":"        This will optionally take in a segment resource if there is a need for"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_69c3e057","line":89,"range":{"start_line":89,"start_character":49,"end_line":89,"end_character":61},"in_reply_to":"9abb7d3a_e92230a2","updated":"2016-05-31 22:33:38.000000000","message":"actually if i did do **kwargs in the base scheduler\u0027s schedule method and base resource filter\u0027s filter and bind methods, that might work too. what do you think?","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"526709b4acd966ebcc41323654ee4d9de7f1ee29","unresolved":false,"context_lines":[{"line_number":96,"context_line":"            return"},{"line_number":97,"context_line":"        # filter the agents that can host the resource"},{"line_number":98,"context_line":"        filtered_agents_dict \u003d self.resource_filter.filter_agents("},{"line_number":99,"context_line":"            plugin, context, network, segment\u003dsegment)"},{"line_number":100,"context_line":"        num_agents \u003d filtered_agents_dict[\u0027n_agents\u0027]"},{"line_number":101,"context_line":"        hostable_agents \u003d filtered_agents_dict[\u0027hostable_agents\u0027]"},{"line_number":102,"context_line":"        hosted_agents \u003d filtered_agents_dict[\u0027hosted_agents\u0027]"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_b8c234e7","line":99,"range":{"start_line":99,"start_character":38,"end_line":99,"end_character":53},"updated":"2016-05-31 21:20:33.000000000","message":"The only two differences in this method are this and L107 where the segment is passed to the bind step.  Other stuff is common.  Maybe we can think of a better pattern.  But, it doesn\u0027t have to be in this patch.","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"276b5e21839f0b085389240138de7a4c92e427a1","unresolved":false,"context_lines":[{"line_number":96,"context_line":"            return"},{"line_number":97,"context_line":"        # filter the agents that can host the resource"},{"line_number":98,"context_line":"        filtered_agents_dict \u003d self.resource_filter.filter_agents("},{"line_number":99,"context_line":"            plugin, context, network, segment\u003dsegment)"},{"line_number":100,"context_line":"        num_agents \u003d filtered_agents_dict[\u0027n_agents\u0027]"},{"line_number":101,"context_line":"        hostable_agents \u003d filtered_agents_dict[\u0027hostable_agents\u0027]"},{"line_number":102,"context_line":"        hosted_agents \u003d filtered_agents_dict[\u0027hosted_agents\u0027]"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_a97ae885","line":99,"range":{"start_line":99,"start_character":38,"end_line":99,"end_character":53},"in_reply_to":"9abb7d3a_b8c234e7","updated":"2016-05-31 22:27:14.000000000","message":"yeah i was juggling the idea of decomposing this into more DRY\u0027able code, but was afraid that\u0027d make it even more unreadable.","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"59e2532350c5bb68604c155b4a2b30d97c0a70db","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        return chosen_agents"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"class ChanceScheduler(SegmentScheduler, base_scheduler.BaseChanceScheduler,"},{"line_number":112,"context_line":"                      AutoScheduler):"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def __init__(self):"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_3e61423a","line":111,"range":{"start_line":111,"start_character":22,"end_line":111,"end_character":38},"updated":"2016-05-31 06:46:24.000000000","message":"Not a big fan of this.  Had to put this one first because of MRO.","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"526709b4acd966ebcc41323654ee4d9de7f1ee29","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        return chosen_agents"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"class ChanceScheduler(SegmentScheduler, base_scheduler.BaseChanceScheduler,"},{"line_number":112,"context_line":"                      AutoScheduler):"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def __init__(self):"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_44dfeb68","line":111,"range":{"start_line":111,"start_character":22,"end_line":111,"end_character":38},"in_reply_to":"9abb7d3a_3e61423a","updated":"2016-05-31 21:20:33.000000000","message":"I like this over complete code duplication.  I know MRO is a pain but it is the way python works.  As a general rule of thumb, you would generally put more specific mixins like this before more general base classes.  So, it does kind of make sense to me.\n\nYou could do it without having to worry about MRO by renaming schedule above to something like _segment_aware_dhcp_schedule and then defining a minimal schedule method in each of these classes like this:\n\n  def schedule(self, plugin, context, network, segment\u003dNone):\n      return self._segment_aware_dhcp_schedule(plugin, context, network, segment)\n\nBut, I prefer my last suggestion or L89 over this.","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"276b5e21839f0b085389240138de7a4c92e427a1","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        return chosen_agents"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"class ChanceScheduler(SegmentScheduler, base_scheduler.BaseChanceScheduler,"},{"line_number":112,"context_line":"                      AutoScheduler):"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def __init__(self):"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_e97eb068","line":111,"range":{"start_line":111,"start_character":22,"end_line":111,"end_character":38},"in_reply_to":"9abb7d3a_44dfeb68","updated":"2016-05-31 22:27:14.000000000","message":"if thats the case then autoscheduler should be before the Base class as well.  Obviously, i won\u0027t fix it in this review.","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"59e2532350c5bb68604c155b4a2b30d97c0a70db","unresolved":false,"context_lines":[{"line_number":115,"context_line":"        super(ChanceScheduler, self).__init__(DhcpFilter())"},{"line_number":116,"context_line":""},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"class WeightScheduler(SegmentScheduler, base_scheduler.BaseWeightScheduler,"},{"line_number":119,"context_line":"                      AutoScheduler):"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    def __init__(self):"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_1e643e47","line":118,"range":{"start_line":118,"start_character":22,"end_line":118,"end_character":38},"updated":"2016-05-31 06:46:24.000000000","message":"same","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"526709b4acd966ebcc41323654ee4d9de7f1ee29","unresolved":false,"context_lines":[{"line_number":164,"context_line":""},{"line_number":165,"context_line":"class DhcpFilter(base_resource_filter.BaseResourceFilter):"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    def bind(self, context, agents, network_id, segment_id\u003dNone):"},{"line_number":168,"context_line":"        \"\"\"Bind the network to the agents.\"\"\""},{"line_number":169,"context_line":"        # customize the bind logic"},{"line_number":170,"context_line":"        bound_agents \u003d agents[:]"}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_7d01deae","line":167,"range":{"start_line":167,"start_character":48,"end_line":167,"end_character":63},"updated":"2016-05-31 21:20:33.000000000","message":"I think the basic idea of my comment on L89 applies here too.","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"526709b4acd966ebcc41323654ee4d9de7f1ee29","unresolved":false,"context_lines":[{"line_number":195,"context_line":""},{"line_number":196,"context_line":"        super(DhcpFilter, self).bind(context, bound_agents, network_id)"},{"line_number":197,"context_line":""},{"line_number":198,"context_line":"    def filter_agents(self, plugin, context, network, segment\u003dNone):"},{"line_number":199,"context_line":"        \"\"\"Return the agents that can host the network or segment."},{"line_number":200,"context_line":""},{"line_number":201,"context_line":"        This function returns a dictionary which has 3 keys."}],"source_content_type":"text/x-python","patch_set":11,"id":"9abb7d3a_fd230e56","line":198,"range":{"start_line":198,"start_character":54,"end_line":198,"end_character":66},"updated":"2016-05-31 21:20:33.000000000","message":"ditto (L167)","commit_id":"70fdbd44dfef538a6ffa86a8788369a67ef26375"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d4a1632346018857edb6b9ab0bd02ada478d87de","unresolved":false,"context_lines":[{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"},{"line_number":91,"context_line":"    # changes."},{"line_number":92,"context_line":"    def schedule(self, plugin, context, network):"},{"line_number":93,"context_line":"        return self.schedule_dhcp(plugin, context, network)"},{"line_number":94,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_6816be56","line":91,"range":{"start_line":89,"start_character":6,"end_line":91,"end_character":14},"updated":"2016-06-02 20:14:00.000000000","message":"I tend to think it should stay in order to override the default implementation of schedule.  Since this structure still maintains \"DhcpScheduler is a Scheduler\" it should have a working implementation of this method.  Unless you thought that the base class implementation would work correctly.","commit_id":"a1165021cd0b64e0b78f739582aad9d203ccc9b9"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"fc4383bf2698a98a82f71145335f46b5466723b9","unresolved":false,"context_lines":[{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"},{"line_number":91,"context_line":"    # changes."},{"line_number":92,"context_line":"    def schedule(self, plugin, context, network):"},{"line_number":93,"context_line":"        return self.schedule_dhcp(plugin, context, network)"},{"line_number":94,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_63341cde","line":91,"range":{"start_line":89,"start_character":6,"end_line":91,"end_character":14},"in_reply_to":"9abb7d3a_6816be56","updated":"2016-06-02 21:42:04.000000000","message":"this is the main reason i kept it so it adheres to the base class, but it probably won\u0027t be used so left the comment just in case someone thinks keeping unused code around is a bad idea.","commit_id":"a1165021cd0b64e0b78f739582aad9d203ccc9b9"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d4a1632346018857edb6b9ab0bd02ada478d87de","unresolved":false,"context_lines":[{"line_number":93,"context_line":"        return self.schedule_dhcp(plugin, context, network)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    # TODO(blogan): This method is very similar to BaseScheduler.schedule"},{"line_number":96,"context_line":"    # Decompose the common parts out to be reused by both."},{"line_number":97,"context_line":"    def schedule_dhcp(self, plugin, context, network, segment\u003dNone):"},{"line_number":98,"context_line":"        if not self.resource_filter:"},{"line_number":99,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_637261af","line":96,"range":{"start_line":96,"start_character":6,"end_line":96,"end_character":57},"updated":"2016-06-02 20:14:00.000000000","message":"A good point but I don\u0027t know the best way off the top of my head.  The only thing that comes to mind is to somehow bind the segment argument of the two method calls (like a functools.partial or something) and somehow pass those bound functions to the base class implementation.  It might take some refactoring that we could follow up with.","commit_id":"a1165021cd0b64e0b78f739582aad9d203ccc9b9"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"fc4383bf2698a98a82f71145335f46b5466723b9","unresolved":false,"context_lines":[{"line_number":93,"context_line":"        return self.schedule_dhcp(plugin, context, network)"},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    # TODO(blogan): This method is very similar to BaseScheduler.schedule"},{"line_number":96,"context_line":"    # Decompose the common parts out to be reused by both."},{"line_number":97,"context_line":"    def schedule_dhcp(self, plugin, context, network, segment\u003dNone):"},{"line_number":98,"context_line":"        if not self.resource_filter:"},{"line_number":99,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_033ef0b7","line":96,"range":{"start_line":96,"start_character":6,"end_line":96,"end_character":57},"in_reply_to":"9abb7d3a_637261af","updated":"2016-06-02 21:42:04.000000000","message":"yeah thats what i was had in mind too.","commit_id":"a1165021cd0b64e0b78f739582aad9d203ccc9b9"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d4a1632346018857edb6b9ab0bd02ada478d87de","unresolved":false,"context_lines":[{"line_number":168,"context_line":""},{"line_number":169,"context_line":"class DhcpFilter(base_resource_filter.BaseResourceFilter):"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":172,"context_line":"    # if so, should be done independently because it can be a more focused"},{"line_number":173,"context_line":"    # changes."},{"line_number":174,"context_line":"    def bind(self, context, agents, network_id):"}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_c3aeade6","line":171,"range":{"start_line":171,"start_character":6,"end_line":171,"end_character":38},"updated":"2016-06-02 20:14:00.000000000","message":"ditto (L91)","commit_id":"a1165021cd0b64e0b78f739582aad9d203ccc9b9"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d4a1632346018857edb6b9ab0bd02ada478d87de","unresolved":false,"context_lines":[{"line_number":205,"context_line":""},{"line_number":206,"context_line":"        super(DhcpFilter, self).bind(context, bound_agents, network_id)"},{"line_number":207,"context_line":""},{"line_number":208,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":209,"context_line":"    # if so, should be done independently because it can be a more focused"},{"line_number":210,"context_line":"    # changes."},{"line_number":211,"context_line":"    def filter_agents(self, plugin, context, network):"}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_23869965","line":208,"range":{"start_line":208,"start_character":6,"end_line":208,"end_character":19},"updated":"2016-06-02 20:14:00.000000000","message":"ditto (L91)","commit_id":"a1165021cd0b64e0b78f739582aad9d203ccc9b9"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"e954092186e37b2171702ed68251ea31dd68ece5","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        return True"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_cd9f0c5e","line":87,"range":{"start_line":87,"start_character":6,"end_line":87,"end_character":19},"updated":"2016-06-06 07:07:22.000000000","message":"I might be missing some context here, but I have these questions for the dhcp scheduler.\n1) Will the network has dhcp agent in every segment, if the network has multiple segments?  I ask this because deleting segment will automatically delete the dhcp port [1]. If dhcp agents are not in every segment, the dhcp service of network might not be usable.\n\n2) Can the scheduler just check the content of network and do the segment scheduling inside the ResourceFilter? It looks like you are bypassing the BaseScheduler.schedule here. Although BaseScheduler is only used by dhcp_scheduler, I am intending to use it for l3_scheduler at [2]. \n\n[1] https://review.openstack.org/#/c/317358/4/neutron/plugins/ml2/db.py\n[2] https://review.openstack.org/#/c/320347/1","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"e5a3dcc57defcff3f5908f3435535427ae9f9764","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        return True"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_368ca7d9","line":87,"range":{"start_line":87,"start_character":6,"end_line":87,"end_character":19},"in_reply_to":"7aa08908_10e9e8e6","updated":"2016-06-07 23:20:33.000000000","message":"So the reason I don\u0027t feel comfortable changing the parameter of bind to resource from resource_id is because there other ResourceFilter implementations out of tree that do expect just the resource_id to be passed in and not an entire resource.  One example is neutron-dynamic-routing:\n\nhttps://github.com/openstack/neutron-dynamic-routing/blob/master/neutron_dynamic_routing/services/bgp/scheduler/bgp_dragent_scheduler.py#L36\n\nAs you can see they inherit from that BaseResourceFilter, and since the BaseScheduler.schedule method calls the contract for bind with resource_id instead of reasource, this would break them.  This is one example I know of, I don\u0027t know if there are any other, but I don\u0027t feel comfortable knowing that it could happen when I have an example of it happening.\n\nAs for your suggestion about passing subnets down, I think I\u0027ve followed it, and I was just about ready to say it may not buy much, but I may have just had an epiphany that you probably already had.  It may not require changing the bind parameters at all.  I\u0027ll test it out and get back to you.  Thanks!\n\nCheck out PS 20","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"1310c5effeb76cd0262529ba167307557dc97766","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        return True"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_af1b587f","line":87,"range":{"start_line":87,"start_character":6,"end_line":87,"end_character":19},"in_reply_to":"7aa08908_1b0ab621","updated":"2016-06-07 05:47:51.000000000","message":"1) That thread did pretty much result in the expectation that there will be a dhcp agent per segment and when a segment is deleted that segment loses dhcp service.  Are you concerned with this or just verifying?\n\n2)changing the BaseResourceFilter.bind method signature was what my first patch set did.  the problem with that is there are importers of the BaseResourceFilter class out of tree, so that would break them.  I don\u0027t think that is a good idea either.\n\nAs for the second idea, the bind call isn\u0027t doing the actual scheduling, its basically just inserting the chosen network, agent, segment_id tuple into the db.  the schedule and select methods are what really do the scheduling. thankfully i didn\u0027t have to do any changes to the select method.  plugin.filter_hosts_with_network_access is being called just for network scheduling.  If you look at L214, that\u0027s where the filtering by network access happens.  If it\u0027s just a network without segments being scheduled, then it uses the plugin.filter_hosts_with_network_access method.  If it\u0027s scheduling a segment, it\u0027s using the segment passed in that already has the hosts in it that are built by the segmenthostmapping table.  I did toy with calling into the plugin for segment connectivity, but its already in the db, so I just have it loaded when I get the segment information.\n\nLet me know if I\u0027m wildly off though, I could be misunderstanding what you\u0027re asking, or I could be totally wrong here.  I\u0027m happy to learn though :)","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"897cca048f1a62d272cc1ab85c66a74f54ad7278","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        return True"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_bc1eadb8","line":87,"range":{"start_line":87,"start_character":6,"end_line":87,"end_character":19},"in_reply_to":"7aa08908_368ca7d9","updated":"2016-06-08 06:24:56.000000000","message":"Thanks, will follow PS 20","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"ca896d4a16757d8fb12d61a0925eafafb8bf43e0","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        return True"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_10e9e8e6","line":87,"range":{"start_line":87,"start_character":6,"end_line":87,"end_character":19},"in_reply_to":"7aa08908_af1b587f","updated":"2016-06-07 08:15:21.000000000","message":"Thanks for the detailed explanation. I went through your previous patches and I think I kind of understand why you change your code like this, while I still have some comments :P\n\n\n1) So this leaves a problem not that the segment loses dhcp service, but the whole network loses dhcp service, because the dhcp port is deleted along with segment. But I am OK to let it be considered in other patches, or just consider it in the segment deletion.\n\n2) I am more neutral to the your approach now. My concern is that the original code path of schedule-\u003efilter_agents-\u003eselect-\u003ebind is almost not used. We are using a dhcp_scheduler which is nearly independent in this patch. This is not something bad, but some previous work looks reverted.\n\nContinuing my last comment, I would proposal implementation like the following, but it is up to you to decide which is better. My proposal will keep the original code path of schedule-\u003efilter_agents-\u003eselect-\u003ebind.\n\nMy understanding is that this patch only considers the case that subnet associated with segment, right? So, we only need to consider the case that a segment-subnet is created in the network. So, maybe we can put only the newly created subnet in the \u0027subnets\u0027 field of network at [1].  Then the code goes schedule-\u003efilter_agents.\n\nIn the filter_agents of DhcpFilter, check if network\u0027s subnet has segment. If so, use that segment to filter agents. With the filtered agents, the code goes filter_agents-\u003eselect-\u003ebind.\n\nIn the bind of DhcpFilter, I will still recommend using resource as method parameter. I am sorry I don\u0027t understand why it don\u0027t work in your first patch. From my understanding, BaseResourceFilter.bind will not use this parameter at all. So, the change in BaseResourceFilter.bind is just a change of name. With the resource, you are able to find out the segment_id, and then the dhcp_agent_binding can be completed.\n\nThis approach is kind of tricky to only put the newly created segment_subnet to the network resource. While I can accept it, because this patch is something like segment_subnet dhcp schedule to me.\n\nAgain, this is my 2 cents opinion, it is up to your decision. What do you think?\n\n[1] dhcp_rpc_agent_api._notify_agents","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"92c522787b47da833a8d253e5ac67248d44bec7b","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        return True"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_fcca4e19","line":87,"range":{"start_line":87,"start_character":6,"end_line":87,"end_character":19},"in_reply_to":"7aa08908_cd9f0c5e","updated":"2016-06-06 20:44:15.000000000","message":"1) Yes it is expected, for now at least, there is a dhcp agent in every segment for a network IF that segment is associated with a dhcp enabled subnet.  \n\n2) That was what I initially wanted to do, but the the base scheduler.schedule method calls the resource_filter.bind method which only takes in resource_id and not just resource, which is the main issue.  So there\u0027s no simple way to get the 2 ids to the bind method.  I\u0027ve gone through many different methods to overcome this in the previous patchsets though and if you have an alternative idea please let me know.","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"9917780f8b57296ef76edf42c5d160257b559f9f","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        return True"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_d854e955","line":87,"range":{"start_line":87,"start_character":6,"end_line":87,"end_character":19},"in_reply_to":"7aa08908_cd9f0c5e","updated":"2016-06-06 08:45:44.000000000","message":"maybe we should consider this refactor to be added prior to the segment addition. that may help with the review process","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"92c522787b47da833a8d253e5ac67248d44bec7b","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        return True"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_9c550a52","line":87,"range":{"start_line":87,"start_character":6,"end_line":87,"end_character":19},"in_reply_to":"7aa08908_d854e955","updated":"2016-06-06 20:44:15.000000000","message":"hmm might be something I can agree with, let me see if I can do that to make it more focused on this piece that will definitely be the part that needs the most discussion.","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"7c46bbc1276a0828feba943feeea1195938f2c7d","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        return True"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_1b0ab621","line":87,"range":{"start_line":87,"start_character":6,"end_line":87,"end_character":19},"in_reply_to":"7aa08908_fcca4e19","updated":"2016-06-07 01:51:50.000000000","message":"1) Assuming a network has multiple segments, and none of the segments is associated with a subnet, creating a dhcp enabled subnet will bind the dhcp port to one of the segment. If that segment is deleted, the dhcp port will be deleted automatically. As a result, the dhcp service will not be usable. There was a discussion about it at [1], I thought dhcp service would be in every segment in such case. So I bring this question out.\n\n2) If resource_filter.bind is going to use resource, I think it is OK to refactor it to accept resource as parameter. But I have another idea, I think segment scheduling should not be the job of bind, it should be the job of resource_filter.filter_agents. Actually, the plugin.filter_hosts_with_network_access is now doing some similar thing in resource_filter.filter_agents\n\n[1] http://lists.openstack.org/pipermail/openstack-dev/2016-May/094833.html","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"190887ea58a14cb1e0cf333bd306749c656e6768","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        return True"},{"line_number":85,"context_line":""},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"}],"source_content_type":"text/x-python","patch_set":19,"id":"7aa08908_b365491f","line":87,"updated":"2016-06-07 22:14:17.000000000","message":"@","commit_id":"cbaae567a6f9136674eb70402ea6e394f006c59a"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a6480b3d23788e86a66888c4242081d287189f69","unresolved":false,"context_lines":[{"line_number":86,"context_line":""},{"line_number":87,"context_line":"class DhcpScheduler(object):"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":90,"context_line":"    # if so, should be done independently because it can be a more focused"},{"line_number":91,"context_line":"    # changes."},{"line_number":92,"context_line":"    def schedule(self, plugin, context, network):"},{"line_number":93,"context_line":"        return self.schedule_dhcp(plugin, context, network)"},{"line_number":94,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"7aa08908_3bb8152a","line":91,"range":{"start_line":89,"start_character":6,"end_line":91,"end_character":14},"updated":"2016-06-07 20:37:09.000000000","message":"I would only remove it if the base classes implementation is suitable for the _dhcp versions.  Otherwise, this maintains the ability to call \"schedule\" from the base class interface specification which is essential to preserving the \"isa\" relationship to the base class.\n\nGiven this, I\u0027d change the NOTE.  I\u0027d avoid hinting that it could be removed.","commit_id":"cbaae567a6f9136674eb70402ea6e394f006c59a"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a6480b3d23788e86a66888c4242081d287189f69","unresolved":false,"context_lines":[{"line_number":168,"context_line":""},{"line_number":169,"context_line":"class DhcpFilter(base_resource_filter.BaseResourceFilter):"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":172,"context_line":"    # if so, should be done independently because it can be a more focused"},{"line_number":173,"context_line":"    # changes."},{"line_number":174,"context_line":"    def bind(self, context, agents, network_id):"}],"source_content_type":"text/x-python","patch_set":19,"id":"7aa08908_bbc885a9","line":171,"range":{"start_line":171,"start_character":6,"end_line":171,"end_character":10},"updated":"2016-06-07 20:37:09.000000000","message":"ditto (L91)","commit_id":"cbaae567a6f9136674eb70402ea6e394f006c59a"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a6480b3d23788e86a66888c4242081d287189f69","unresolved":false,"context_lines":[{"line_number":205,"context_line":""},{"line_number":206,"context_line":"        super(DhcpFilter, self).bind(context, bound_agents, network_id)"},{"line_number":207,"context_line":""},{"line_number":208,"context_line":"    # NOTE(blogan): This method may be able to just be removed completely but"},{"line_number":209,"context_line":"    # if so, should be done independently because it can be a more focused"},{"line_number":210,"context_line":"    # changes."},{"line_number":211,"context_line":"    def filter_agents(self, plugin, context, network):"}],"source_content_type":"text/x-python","patch_set":19,"id":"7aa08908_fbc68d98","line":208,"range":{"start_line":208,"start_character":6,"end_line":208,"end_character":10},"updated":"2016-06-07 20:37:09.000000000","message":"ditto (L91)","commit_id":"cbaae567a6f9136674eb70402ea6e394f006c59a"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"e62b1590b7035e5bb32257f78d9ee93980a8c661","unresolved":false,"context_lines":[{"line_number":166,"context_line":"        \"\"\"Bind the network to the agents.\"\"\""},{"line_number":167,"context_line":"        subnet \u003d manager.NeutronManager.get_plugin().get_subnet(context,"},{"line_number":168,"context_line":"                                                                subnet_id)"},{"line_number":169,"context_line":"        network_id \u003d subnet.get(\u0027network_id\u0027)"},{"line_number":170,"context_line":"        segment_id \u003d subnet.get(\u0027segment_id\u0027)"},{"line_number":171,"context_line":"        # customize the bind logic"},{"line_number":172,"context_line":"        bound_agents \u003d agents[:]"},{"line_number":173,"context_line":"        for agent in agents:"}],"source_content_type":"text/x-python","patch_set":20,"id":"7aa08908_7c41e5a8","line":170,"range":{"start_line":169,"start_character":0,"end_line":170,"end_character":45},"updated":"2016-06-08 06:32:39.000000000","message":"nit: maybe these attributes can be got in the DB transaction. After all, there is already a DB transaction in this method.","commit_id":"4c8316c821a53085d70d4f560e7ce48482e30c5a"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"522b872f8b2196badd479a6db0cb3815ab0261fd","unresolved":false,"context_lines":[{"line_number":166,"context_line":"        \"\"\"Bind the network to the agents.\"\"\""},{"line_number":167,"context_line":"        subnet \u003d manager.NeutronManager.get_plugin().get_subnet(context,"},{"line_number":168,"context_line":"                                                                subnet_id)"},{"line_number":169,"context_line":"        network_id \u003d subnet.get(\u0027network_id\u0027)"},{"line_number":170,"context_line":"        segment_id \u003d subnet.get(\u0027segment_id\u0027)"},{"line_number":171,"context_line":"        # customize the bind logic"},{"line_number":172,"context_line":"        bound_agents \u003d agents[:]"},{"line_number":173,"context_line":"        for agent in agents:"}],"source_content_type":"text/x-python","patch_set":20,"id":"7aa08908_039e1c07","line":170,"range":{"start_line":169,"start_character":0,"end_line":170,"end_character":45},"in_reply_to":"7aa08908_4224b00b","updated":"2016-06-09 12:54:11.000000000","message":"I think you are right change an existing interface will cause uncertainty.","commit_id":"4c8316c821a53085d70d4f560e7ce48482e30c5a"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"fde1ac9ead7e06de9b245adee2edd2ac44196648","unresolved":false,"context_lines":[{"line_number":166,"context_line":"        \"\"\"Bind the network to the agents.\"\"\""},{"line_number":167,"context_line":"        subnet \u003d manager.NeutronManager.get_plugin().get_subnet(context,"},{"line_number":168,"context_line":"                                                                subnet_id)"},{"line_number":169,"context_line":"        network_id \u003d subnet.get(\u0027network_id\u0027)"},{"line_number":170,"context_line":"        segment_id \u003d subnet.get(\u0027segment_id\u0027)"},{"line_number":171,"context_line":"        # customize the bind logic"},{"line_number":172,"context_line":"        bound_agents \u003d agents[:]"},{"line_number":173,"context_line":"        for agent in agents:"}],"source_content_type":"text/x-python","patch_set":20,"id":"7aa08908_4224b00b","line":170,"range":{"start_line":169,"start_character":0,"end_line":170,"end_character":45},"in_reply_to":"7aa08908_574a0227","updated":"2016-06-09 06:18:21.000000000","message":"so i went away from doing this by subnet_id in PS 21.  The reason being that changing the bind from network_id to subnet_id was going to end up touching many different parts of the code because many things had been setup to just pass in the network_id.  So something like autoscheduling networks would have to pass in a subnet_id, but subnets are already created for that network, so the autoschedule networks method would have to know what subnet to pick, which I couldn\u0027t guarantee which one to choose.  I think it causes way too much uncertainty.\n\nI think the strategy in the new PS is the best possible strategy at this point.  I\u0027d like to get your opinion on it.","commit_id":"4c8316c821a53085d70d4f560e7ce48482e30c5a"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"b1c10e0de9f6806e9e7656a7e5b13f30e4f54116","unresolved":false,"context_lines":[{"line_number":166,"context_line":"        \"\"\"Bind the network to the agents.\"\"\""},{"line_number":167,"context_line":"        subnet \u003d manager.NeutronManager.get_plugin().get_subnet(context,"},{"line_number":168,"context_line":"                                                                subnet_id)"},{"line_number":169,"context_line":"        network_id \u003d subnet.get(\u0027network_id\u0027)"},{"line_number":170,"context_line":"        segment_id \u003d subnet.get(\u0027segment_id\u0027)"},{"line_number":171,"context_line":"        # customize the bind logic"},{"line_number":172,"context_line":"        bound_agents \u003d agents[:]"},{"line_number":173,"context_line":"        for agent in agents:"}],"source_content_type":"text/x-python","patch_set":20,"id":"7aa08908_574a0227","line":170,"range":{"start_line":169,"start_character":0,"end_line":170,"end_character":45},"in_reply_to":"7aa08908_7c41e5a8","updated":"2016-06-08 06:47:11.000000000","message":"yeah that might be better, i\u0027m pretty sure i can just call self.plugin to get these guys too, but ill see if i can improve this.","commit_id":"4c8316c821a53085d70d4f560e7ce48482e30c5a"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"e62b1590b7035e5bb32257f78d9ee93980a8c661","unresolved":false,"context_lines":[{"line_number":209,"context_line":"        \"\"\""},{"line_number":210,"context_line":"        segment \u003d None"},{"line_number":211,"context_line":"        subnet \u003d network.get(\u0027subnet\u0027, {})"},{"line_number":212,"context_line":"        if subnet.get(\u0027id\u0027):"},{"line_number":213,"context_line":"            if subnet.get(\u0027segment_id\u0027):"},{"line_number":214,"context_line":"                segment_plugin \u003d manager.NeutronManager.get_service_plugins()["},{"line_number":215,"context_line":"                    \u0027segments\u0027"},{"line_number":216,"context_line":"                ]"}],"source_content_type":"text/x-python","patch_set":20,"id":"7aa08908_5c7b4928","line":213,"range":{"start_line":212,"start_character":0,"end_line":213,"end_character":40},"updated":"2016-06-08 06:32:39.000000000","message":"nit: I think it could be in one if clause, if A and B:\n\nAnd why checking the id of subnet?","commit_id":"4c8316c821a53085d70d4f560e7ce48482e30c5a"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"b1c10e0de9f6806e9e7656a7e5b13f30e4f54116","unresolved":false,"context_lines":[{"line_number":209,"context_line":"        \"\"\""},{"line_number":210,"context_line":"        segment \u003d None"},{"line_number":211,"context_line":"        subnet \u003d network.get(\u0027subnet\u0027, {})"},{"line_number":212,"context_line":"        if subnet.get(\u0027id\u0027):"},{"line_number":213,"context_line":"            if subnet.get(\u0027segment_id\u0027):"},{"line_number":214,"context_line":"                segment_plugin \u003d manager.NeutronManager.get_service_plugins()["},{"line_number":215,"context_line":"                    \u0027segments\u0027"},{"line_number":216,"context_line":"                ]"}],"source_content_type":"text/x-python","patch_set":20,"id":"7aa08908_d7d9d258","line":213,"range":{"start_line":212,"start_character":0,"end_line":213,"end_character":40},"in_reply_to":"7aa08908_5c7b4928","updated":"2016-06-08 06:47:11.000000000","message":"its too late for my brain to work right now i guess, but i think there was a case where network may not have subnet in it.  might be better to just check for subnet existence and subnet, segment_id.\n\nI wrote this fast, and knew there\u0027d be some dumb stuff in it :)","commit_id":"4c8316c821a53085d70d4f560e7ce48482e30c5a"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"99805875e65d56340ad0af88654a3fc5c6dd2811","unresolved":false,"context_lines":[{"line_number":89,"context_line":""},{"line_number":90,"context_line":"    # TODO(blogan): This method is very similar to BaseScheduler.schedule"},{"line_number":91,"context_line":"    # Decompose the common parts out to be reused by both."},{"line_number":92,"context_line":"    def schedule(self, plugin, context, network):"},{"line_number":93,"context_line":"        if not self.resource_filter:"},{"line_number":94,"context_line":"            return"},{"line_number":95,"context_line":"        # filter the agents that can host the resource"}],"source_content_type":"text/x-python","patch_set":21,"id":"7aa08908_5bdc7318","line":92,"range":{"start_line":92,"start_character":40,"end_line":92,"end_character":47},"updated":"2016-06-09 19:19:44.000000000","message":"This is breaking the base class interface by calling this \"network\" instead of \"resource\" [1].  It should be called \"resource.\"  Otherwise, if someone decides to pass this value as a keyword argument, it is dependent on having an instance of DhcpScheduler.\n\n [1] https://github.com/openstack/neutron/blob/16a00e2b067af38e22e2cf2204015e1ae9c07cb4/neutron/scheduler/base_scheduler.py#L37","commit_id":"03174ae8342ecb5e3e438681e46e4a167b035c0d"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"127617830fd575c35494332479cacac78203c34e","unresolved":false,"context_lines":[{"line_number":102,"context_line":"                                    hosted_agents, num_agents)"},{"line_number":103,"context_line":"        # bind the resource to the agents"},{"line_number":104,"context_line":"        segment_id \u003d network.get(\u0027segment\u0027, {}).get(\u0027id\u0027)"},{"line_number":105,"context_line":"        if segment_id:"},{"line_number":106,"context_line":"            self.resource_filter.bind_segment(context, chosen_agents,"},{"line_number":107,"context_line":"                                              network[\u0027id\u0027], segment_id)"},{"line_number":108,"context_line":"        else:"},{"line_number":109,"context_line":"            self.resource_filter.bind(context, chosen_agents, network[\u0027id\u0027])"},{"line_number":110,"context_line":"        return chosen_agents"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"7aa08908_a292b4f3","line":109,"range":{"start_line":105,"start_character":0,"end_line":109,"end_character":76},"updated":"2016-06-09 06:23:52.000000000","message":"I think this is the best possible strategy.  The base schedule method is preserved, and the base bind method is preserved.  The only difference is if there is a segment then we call a different method.","commit_id":"03174ae8342ecb5e3e438681e46e4a167b035c0d"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"99aee7a4cb25f98e96c3ee2171623eeb218ba2ad","unresolved":false,"context_lines":[{"line_number":102,"context_line":"                                    hosted_agents, num_agents)"},{"line_number":103,"context_line":"        # bind the resource to the agents"},{"line_number":104,"context_line":"        segment_id \u003d network.get(\u0027segment\u0027, {}).get(\u0027id\u0027)"},{"line_number":105,"context_line":"        if segment_id:"},{"line_number":106,"context_line":"            self.resource_filter.bind_segment(context, chosen_agents,"},{"line_number":107,"context_line":"                                              network[\u0027id\u0027], segment_id)"},{"line_number":108,"context_line":"        else:"},{"line_number":109,"context_line":"            self.resource_filter.bind(context, chosen_agents, network[\u0027id\u0027])"},{"line_number":110,"context_line":"        return chosen_agents"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"7aa08908_f570e7de","line":109,"range":{"start_line":105,"start_character":0,"end_line":109,"end_character":76},"in_reply_to":"7aa08908_a292b4f3","updated":"2016-06-09 13:31:47.000000000","message":"I agree with you. The gap between existing code and segment binding is that existing code only consider 2 dimensions binding(agent\u003c-\u003enetwork), while the segment binding is a 3 dimensions binding (agent\u003c-\u003esegment\u003c-\u003enetwork). The existing code don\u0027t consider the 3rd variable(segment).","commit_id":"03174ae8342ecb5e3e438681e46e4a167b035c0d"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"99805875e65d56340ad0af88654a3fc5c6dd2811","unresolved":false,"context_lines":[{"line_number":102,"context_line":"                                    hosted_agents, num_agents)"},{"line_number":103,"context_line":"        # bind the resource to the agents"},{"line_number":104,"context_line":"        segment_id \u003d network.get(\u0027segment\u0027, {}).get(\u0027id\u0027)"},{"line_number":105,"context_line":"        if segment_id:"},{"line_number":106,"context_line":"            self.resource_filter.bind_segment(context, chosen_agents,"},{"line_number":107,"context_line":"                                              network[\u0027id\u0027], segment_id)"},{"line_number":108,"context_line":"        else:"},{"line_number":109,"context_line":"            self.resource_filter.bind(context, chosen_agents, network[\u0027id\u0027])"},{"line_number":110,"context_line":"        return chosen_agents"},{"line_number":111,"context_line":""},{"line_number":112,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"7aa08908_3b2a477f","line":109,"range":{"start_line":105,"start_character":0,"end_line":109,"end_character":76},"in_reply_to":"7aa08908_f570e7de","updated":"2016-06-09 19:19:44.000000000","message":"Personally, I don\u0027t see it as preserving the base \"schedule\" method.  I see it more as being restricted or bound to it when it wasn\u0027t designed well in the first place.  And, I\u0027m not sure why that is important enough for us to worry about.\n\nThe more I think about this, the more I don\u0027t like this inheritance model for schedulers.  Why do we need a base interface for all types of schedulers?  Typically, we know with what kind of scheduler we\u0027re dealing with.  But, I don\u0027t think we can fix what I don\u0027t like about it without a refactor which I don\u0027t want to ask for.  So, I guess any of these solutions is about as good as any.\n\nI\u0027m not particularly fond of hiding the segment under the network just to fit it through the base class method.  But, I won\u0027t object if this is the way you want to go.","commit_id":"03174ae8342ecb5e3e438681e46e4a167b035c0d"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"03042d31e26e9465a6579245f3d32fca5995e1e1","unresolved":false,"context_lines":[{"line_number":192,"context_line":"            hostable_dhcp_hosts \u003d plugin.filter_hosts_with_network_access("},{"line_number":193,"context_line":"                context, network[\u0027id\u0027],"},{"line_number":194,"context_line":"                [agent[\u0027host\u0027] for agent in hostable_agents])"},{"line_number":195,"context_line":"            reachable_agents \u003d [agent for agent in hostable_agents"},{"line_number":196,"context_line":"                                if agent[\u0027host\u0027] in hostable_dhcp_hosts]"},{"line_number":197,"context_line":"        return reachable_agents"},{"line_number":198,"context_line":""},{"line_number":199,"context_line":"    def _get_dhcp_agents_hosting_network(self, plugin, context, network):"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_7e858f05","line":196,"range":{"start_line":195,"start_character":0,"end_line":196,"end_character":72},"updated":"2016-06-11 08:48:56.000000000","message":"This is a bit duplicated with line 187 to 190, I think it could be eliminated.","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"4a0530b545897da359134c8f60b79d0733559605","unresolved":false,"context_lines":[{"line_number":192,"context_line":"            hostable_dhcp_hosts \u003d plugin.filter_hosts_with_network_access("},{"line_number":193,"context_line":"                context, network[\u0027id\u0027],"},{"line_number":194,"context_line":"                [agent[\u0027host\u0027] for agent in hostable_agents])"},{"line_number":195,"context_line":"            reachable_agents \u003d [agent for agent in hostable_agents"},{"line_number":196,"context_line":"                                if agent[\u0027host\u0027] in hostable_dhcp_hosts]"},{"line_number":197,"context_line":"        return reachable_agents"},{"line_number":198,"context_line":""},{"line_number":199,"context_line":"    def _get_dhcp_agents_hosting_network(self, plugin, context, network):"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_80bb94e4","line":196,"range":{"start_line":195,"start_character":0,"end_line":196,"end_character":72},"in_reply_to":"7aa08908_7e858f05","updated":"2016-06-13 11:27:18.000000000","message":"if L187-190 is changed to \"hostable_dhcp_hosts \u003d network[\u0027candidate_hosts\u0027]\" then this can be moved outside of the if/else block","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"64851d435e6380f0f7037fe64b982c4c53bf3c03","unresolved":false,"context_lines":[{"line_number":192,"context_line":"            hostable_dhcp_hosts \u003d plugin.filter_hosts_with_network_access("},{"line_number":193,"context_line":"                context, network[\u0027id\u0027],"},{"line_number":194,"context_line":"                [agent[\u0027host\u0027] for agent in hostable_agents])"},{"line_number":195,"context_line":"            reachable_agents \u003d [agent for agent in hostable_agents"},{"line_number":196,"context_line":"                                if agent[\u0027host\u0027] in hostable_dhcp_hosts]"},{"line_number":197,"context_line":"        return reachable_agents"},{"line_number":198,"context_line":""},{"line_number":199,"context_line":"    def _get_dhcp_agents_hosting_network(self, plugin, context, network):"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_fcd01c55","line":196,"range":{"start_line":195,"start_character":0,"end_line":196,"end_character":72},"in_reply_to":"7aa08908_80bb94e4","updated":"2016-06-14 07:20:57.000000000","message":"+1","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"30c80756d53e0b00d046f86cbbabc1037f542aa3","unresolved":false,"context_lines":[{"line_number":192,"context_line":"            hostable_dhcp_hosts \u003d plugin.filter_hosts_with_network_access("},{"line_number":193,"context_line":"                context, network[\u0027id\u0027],"},{"line_number":194,"context_line":"                [agent[\u0027host\u0027] for agent in hostable_agents])"},{"line_number":195,"context_line":"            reachable_agents \u003d [agent for agent in hostable_agents"},{"line_number":196,"context_line":"                                if agent[\u0027host\u0027] in hostable_dhcp_hosts]"},{"line_number":197,"context_line":"        return reachable_agents"},{"line_number":198,"context_line":""},{"line_number":199,"context_line":"    def _get_dhcp_agents_hosting_network(self, plugin, context, network):"}],"source_content_type":"text/x-python","patch_set":22,"id":"3aaa91ec_fe739fb2","line":196,"range":{"start_line":195,"start_character":0,"end_line":196,"end_character":72},"in_reply_to":"7aa08908_fcd01c55","updated":"2016-06-21 21:29:46.000000000","message":"Done","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"564d00f235aaafb5f99d3c034f5c39268c9978ab","unresolved":false,"context_lines":[{"line_number":207,"context_line":"            hostable_dhcp_hosts \u003d plugin.filter_hosts_with_network_access("},{"line_number":208,"context_line":"                context, network[\u0027id\u0027],"},{"line_number":209,"context_line":"                [agent[\u0027host\u0027] for agent in hostable_agents])"},{"line_number":210,"context_line":"        reachable_agents \u003d [agent for agent in hostable_agents"},{"line_number":211,"context_line":"                            if agent[\u0027host\u0027] in hostable_dhcp_hosts]"},{"line_number":212,"context_line":"        return reachable_agents"},{"line_number":213,"context_line":""},{"line_number":214,"context_line":"    def _get_dhcp_agents_hosting_network(self, plugin, context, network):"},{"line_number":215,"context_line":"        \"\"\"Return dhcp agents hosting the given network or None if a given"}],"source_content_type":"text/x-python","patch_set":23,"id":"3aaa91ec_e5fca740","line":212,"range":{"start_line":210,"start_character":0,"end_line":212,"end_character":31},"updated":"2016-06-22 00:18:08.000000000","message":"nit: this could be returned directly","commit_id":"68529b09ae83992fe90f98278a39d087bf9a8407"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"8fdacc03e907858d9c2dc13dc427700213fa4899","unresolved":false,"context_lines":[{"line_number":207,"context_line":"            hostable_dhcp_hosts \u003d plugin.filter_hosts_with_network_access("},{"line_number":208,"context_line":"                context, network[\u0027id\u0027],"},{"line_number":209,"context_line":"                [agent[\u0027host\u0027] for agent in hostable_agents])"},{"line_number":210,"context_line":"        reachable_agents \u003d [agent for agent in hostable_agents"},{"line_number":211,"context_line":"                            if agent[\u0027host\u0027] in hostable_dhcp_hosts]"},{"line_number":212,"context_line":"        return reachable_agents"},{"line_number":213,"context_line":""},{"line_number":214,"context_line":"    def _get_dhcp_agents_hosting_network(self, plugin, context, network):"},{"line_number":215,"context_line":"        \"\"\"Return dhcp agents hosting the given network or None if a given"}],"source_content_type":"text/x-python","patch_set":23,"id":"3aaa91ec_caf52b84","line":212,"range":{"start_line":210,"start_character":0,"end_line":212,"end_character":31},"in_reply_to":"3aaa91ec_e5fca740","updated":"2016-06-22 07:39:30.000000000","message":"i think this is more preference than anything, but i don\u0027t like to return list comprehensions","commit_id":"68529b09ae83992fe90f98278a39d087bf9a8407"}],"neutron/tests/unit/extensions/test_segment.py":[{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"9917780f8b57296ef76edf42c5d160257b559f9f","unresolved":false,"context_lines":[{"line_number":82,"context_line":"            self.assertEqual(segment_res.status_int, expected_res_status)"},{"line_number":83,"context_line":"        return segment_res"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":""},{"line_number":86,"context_line":"    def _make_segment(self, fmt, **kwargs):"},{"line_number":87,"context_line":"        res \u003d self._create_segment(fmt, **kwargs)"},{"line_number":88,"context_line":"        if res.status_int \u003e\u003d webob.exc.HTTPClientError.code:"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_58ee398c","line":85,"updated":"2016-06-06 08:45:44.000000000","message":"unrelated","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"92c522787b47da833a8d253e5ac67248d44bec7b","unresolved":false,"context_lines":[{"line_number":82,"context_line":"            self.assertEqual(segment_res.status_int, expected_res_status)"},{"line_number":83,"context_line":"        return segment_res"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":""},{"line_number":86,"context_line":"    def _make_segment(self, fmt, **kwargs):"},{"line_number":87,"context_line":"        res \u003d self._create_segment(fmt, **kwargs)"},{"line_number":88,"context_line":"        if res.status_int \u003e\u003d webob.exc.HTTPClientError.code:"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_3ce176ff","line":85,"in_reply_to":"7aa08908_58ee398c","updated":"2016-06-06 20:44:15.000000000","message":"bad merge conflict fix...i confess id idn\u0027t run pep8 before i pushed, i fell into that trap","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"9917780f8b57296ef76edf42c5d160257b559f9f","unresolved":false,"context_lines":[{"line_number":90,"context_line":"                code\u003dres.status_int, explanation\u003dstr(res))"},{"line_number":91,"context_line":"        return self.deserialize(fmt, res)"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    def segment(self, **kwargs):"},{"line_number":95,"context_line":"        kwargs.setdefault(\u0027network_type\u0027, \u0027net_type\u0027)"},{"line_number":96,"context_line":"        return self._make_segment("}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_78edf58a","line":93,"updated":"2016-06-06 08:45:44.000000000","message":"unrealted","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"9917780f8b57296ef76edf42c5d160257b559f9f","unresolved":false,"context_lines":[{"line_number":95,"context_line":"        kwargs.setdefault(\u0027network_type\u0027, \u0027net_type\u0027)"},{"line_number":96,"context_line":"        return self._make_segment("},{"line_number":97,"context_line":"            self.fmt, tenant_id\u003dself._tenant_id, **kwargs)"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"    def _test_create_segment(self, expected\u003dNone, **kwargs):"},{"line_number":101,"context_line":"        keys \u003d kwargs.copy()"}],"source_content_type":"text/x-python","patch_set":18,"id":"7aa08908_b8ff5d33","line":98,"updated":"2016-06-06 08:45:44.000000000","message":"unrelated","commit_id":"9431e8f09f1c0ab9f1dac65f26d60d0039ef9490"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"03042d31e26e9465a6579245f3d32fca5995e1e1","unresolved":false,"context_lines":[{"line_number":888,"context_line":"        for host in hosts:"},{"line_number":889,"context_line":"            helpers.register_dhcp_agent(host)"},{"line_number":890,"context_line":""},{"line_number":891,"context_line":"    def test_network_scheduling_on_segment_creation(self):"},{"line_number":892,"context_line":"        self._register_dhcp_agents()"},{"line_number":893,"context_line":"        self._test_create_network_and_segment(\u0027phys_net1\u0027)"},{"line_number":894,"context_line":""},{"line_number":895,"context_line":"    def test_segment_scheduling_no_host_mapping(self):"},{"line_number":896,"context_line":"        self._register_dhcp_agents()"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_dea3e39c","line":893,"range":{"start_line":891,"start_character":0,"end_line":893,"end_character":58},"updated":"2016-06-11 08:48:56.000000000","message":"I think it don\u0027t test anything, and it is covered by line 896 to line 898. So, maybe it could be removed.","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"30c80756d53e0b00d046f86cbbabc1037f542aa3","unresolved":false,"context_lines":[{"line_number":888,"context_line":"        for host in hosts:"},{"line_number":889,"context_line":"            helpers.register_dhcp_agent(host)"},{"line_number":890,"context_line":""},{"line_number":891,"context_line":"    def test_network_scheduling_on_segment_creation(self):"},{"line_number":892,"context_line":"        self._register_dhcp_agents()"},{"line_number":893,"context_line":"        self._test_create_network_and_segment(\u0027phys_net1\u0027)"},{"line_number":894,"context_line":""},{"line_number":895,"context_line":"    def test_segment_scheduling_no_host_mapping(self):"},{"line_number":896,"context_line":"        self._register_dhcp_agents()"}],"source_content_type":"text/x-python","patch_set":22,"id":"3aaa91ec_be4bd758","line":893,"range":{"start_line":891,"start_character":0,"end_line":893,"end_character":58},"in_reply_to":"7aa08908_dea3e39c","updated":"2016-06-21 21:29:46.000000000","message":"it\u0027s verifying that agents aren\u0027t scheduled when a segment is created.  i think its useful because we don\u0027t want the scheduling to happen until a dhcp enabled subnet is added to the segment.","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"03042d31e26e9465a6579245f3d32fca5995e1e1","unresolved":false,"context_lines":[{"line_number":910,"context_line":"        self._test_create_subnet(network, segment)"},{"line_number":911,"context_line":"        dhcp_agents \u003d self.dhcp_agent_db.get_dhcp_agents_hosting_networks("},{"line_number":912,"context_line":"            self.ctx, [network[\u0027id\u0027]])"},{"line_number":913,"context_line":"        self.assertEqual(1, len(dhcp_agents))"},{"line_number":914,"context_line":""},{"line_number":915,"context_line":"    def test_segment_scheduling_with_multiple_host_mappings(self):"},{"line_number":916,"context_line":"        phys_net1 \u003d \u0027phys_net1\u0027"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_1ec56b3b","line":913,"updated":"2016-06-11 08:48:56.000000000","message":"will be better if the only one dhcp agent has hostname DHCP_HOSTA. Because by default the network will be scheduled to one dhcp agent, it will be clear to check which dhcp agent is scheduled.","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"30c80756d53e0b00d046f86cbbabc1037f542aa3","unresolved":false,"context_lines":[{"line_number":910,"context_line":"        self._test_create_subnet(network, segment)"},{"line_number":911,"context_line":"        dhcp_agents \u003d self.dhcp_agent_db.get_dhcp_agents_hosting_networks("},{"line_number":912,"context_line":"            self.ctx, [network[\u0027id\u0027]])"},{"line_number":913,"context_line":"        self.assertEqual(1, len(dhcp_agents))"},{"line_number":914,"context_line":""},{"line_number":915,"context_line":"    def test_segment_scheduling_with_multiple_host_mappings(self):"},{"line_number":916,"context_line":"        phys_net1 \u003d \u0027phys_net1\u0027"}],"source_content_type":"text/x-python","patch_set":22,"id":"3aaa91ec_7e25efc0","line":913,"in_reply_to":"7aa08908_1ec56b3b","updated":"2016-06-21 21:29:46.000000000","message":"Done","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"03042d31e26e9465a6579245f3d32fca5995e1e1","unresolved":false,"context_lines":[{"line_number":915,"context_line":"    def test_segment_scheduling_with_multiple_host_mappings(self):"},{"line_number":916,"context_line":"        phys_net1 \u003d \u0027phys_net1\u0027"},{"line_number":917,"context_line":"        phys_net2 \u003d \u0027phys_net2\u0027"},{"line_number":918,"context_line":"        self._register_dhcp_agents()"},{"line_number":919,"context_line":"        network, segment1 \u003d self._test_create_network_and_segment(phys_net1)"},{"line_number":920,"context_line":"        segment2 \u003d self._test_create_segment(network_id\u003dnetwork[\u0027id\u0027],"},{"line_number":921,"context_line":"                                             physical_network\u003dphys_net2,"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_7e010f6a","line":918,"range":{"start_line":918,"start_character":13,"end_line":918,"end_character":34},"updated":"2016-06-11 08:48:56.000000000","message":"Will be more convincing to register more dhcp agent, and show that the dump dhcp agents are not selected.","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"30c80756d53e0b00d046f86cbbabc1037f542aa3","unresolved":false,"context_lines":[{"line_number":915,"context_line":"    def test_segment_scheduling_with_multiple_host_mappings(self):"},{"line_number":916,"context_line":"        phys_net1 \u003d \u0027phys_net1\u0027"},{"line_number":917,"context_line":"        phys_net2 \u003d \u0027phys_net2\u0027"},{"line_number":918,"context_line":"        self._register_dhcp_agents()"},{"line_number":919,"context_line":"        network, segment1 \u003d self._test_create_network_and_segment(phys_net1)"},{"line_number":920,"context_line":"        segment2 \u003d self._test_create_segment(network_id\u003dnetwork[\u0027id\u0027],"},{"line_number":921,"context_line":"                                             physical_network\u003dphys_net2,"}],"source_content_type":"text/x-python","patch_set":22,"id":"3aaa91ec_3e3b67ed","line":918,"range":{"start_line":918,"start_character":13,"end_line":918,"end_character":34},"in_reply_to":"7aa08908_7e010f6a","updated":"2016-06-21 21:29:46.000000000","message":"Done","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"03042d31e26e9465a6579245f3d32fca5995e1e1","unresolved":false,"context_lines":[{"line_number":932,"context_line":"        dhcp_agents \u003d self.dhcp_agent_db.get_dhcp_agents_hosting_networks("},{"line_number":933,"context_line":"            self.ctx, [network[\u0027id\u0027]])"},{"line_number":934,"context_line":"        self.assertEqual(2, len(dhcp_agents))"},{"line_number":935,"context_line":"        a_dhcp_agents \u003d self.dhcp_agent_db.get_dhcp_agents_hosting_networks("},{"line_number":936,"context_line":"            self.ctx, [network[\u0027id\u0027]], hosts\u003d[DHCP_HOSTA])"},{"line_number":937,"context_line":"        self.assertEqual(1, len(a_dhcp_agents))"},{"line_number":938,"context_line":"        b_dhcp_agents \u003d self.dhcp_agent_db.get_dhcp_agents_hosting_networks("},{"line_number":939,"context_line":"            self.ctx, [network[\u0027id\u0027]], hosts\u003d[DHCP_HOSTB])"},{"line_number":940,"context_line":"        self.assertEqual(1, len(b_dhcp_agents))"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_5e04137d","line":940,"range":{"start_line":935,"start_character":0,"end_line":940,"end_character":47},"updated":"2016-06-11 08:48:56.000000000","message":"nit: I think you could just check the hostname of dhcp_agents.","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"30c80756d53e0b00d046f86cbbabc1037f542aa3","unresolved":false,"context_lines":[{"line_number":932,"context_line":"        dhcp_agents \u003d self.dhcp_agent_db.get_dhcp_agents_hosting_networks("},{"line_number":933,"context_line":"            self.ctx, [network[\u0027id\u0027]])"},{"line_number":934,"context_line":"        self.assertEqual(2, len(dhcp_agents))"},{"line_number":935,"context_line":"        a_dhcp_agents \u003d self.dhcp_agent_db.get_dhcp_agents_hosting_networks("},{"line_number":936,"context_line":"            self.ctx, [network[\u0027id\u0027]], hosts\u003d[DHCP_HOSTA])"},{"line_number":937,"context_line":"        self.assertEqual(1, len(a_dhcp_agents))"},{"line_number":938,"context_line":"        b_dhcp_agents \u003d self.dhcp_agent_db.get_dhcp_agents_hosting_networks("},{"line_number":939,"context_line":"            self.ctx, [network[\u0027id\u0027]], hosts\u003d[DHCP_HOSTB])"},{"line_number":940,"context_line":"        self.assertEqual(1, len(b_dhcp_agents))"}],"source_content_type":"text/x-python","patch_set":22,"id":"3aaa91ec_fe30bf09","line":940,"range":{"start_line":935,"start_character":0,"end_line":940,"end_character":47},"in_reply_to":"7aa08908_5e04137d","updated":"2016-06-21 21:29:46.000000000","message":"Done","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"564d00f235aaafb5f99d3c034f5c39268c9978ab","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from neutron.db import db_base_plugin_v2"},{"line_number":25,"context_line":"from neutron.db import portbindings_db"},{"line_number":26,"context_line":"from neutron.db import segments_db"},{"line_number":27,"context_line":"from neutron.db import agentschedulers_db"},{"line_number":28,"context_line":"from neutron.extensions import portbindings"},{"line_number":29,"context_line":"from neutron.extensions import segment as ext_segment"},{"line_number":30,"context_line":"from neutron.plugins.common import constants as p_constants"}],"source_content_type":"text/x-python","patch_set":23,"id":"3aaa91ec_c5908bef","line":27,"range":{"start_line":27,"start_character":0,"end_line":27,"end_character":41},"updated":"2016-06-22 00:18:08.000000000","message":"I think the pep8 is not satisfied about this. The fact that PS22 won\u0027t report error might be because the recent fix of pep check.\n\nhttps://review.openstack.org/#/c/332059/","commit_id":"68529b09ae83992fe90f98278a39d087bf9a8407"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"8fdacc03e907858d9c2dc13dc427700213fa4899","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from neutron.db import db_base_plugin_v2"},{"line_number":25,"context_line":"from neutron.db import portbindings_db"},{"line_number":26,"context_line":"from neutron.db import segments_db"},{"line_number":27,"context_line":"from neutron.db import agentschedulers_db"},{"line_number":28,"context_line":"from neutron.extensions import portbindings"},{"line_number":29,"context_line":"from neutron.extensions import segment as ext_segment"},{"line_number":30,"context_line":"from neutron.plugins.common import constants as p_constants"}],"source_content_type":"text/x-python","patch_set":23,"id":"3aaa91ec_aa0c1f95","line":27,"range":{"start_line":27,"start_character":0,"end_line":27,"end_character":41},"in_reply_to":"3aaa91ec_c5908bef","updated":"2016-06-22 07:39:30.000000000","message":"well its obviously out of order, not sure how i missed this but this explains how when i ran it locally it didn\u0027t catch it either.  thanks for the info!","commit_id":"68529b09ae83992fe90f98278a39d087bf9a8407"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"564d00f235aaafb5f99d3c034f5c39268c9978ab","unresolved":false,"context_lines":[{"line_number":29,"context_line":"from neutron.extensions import segment as ext_segment"},{"line_number":30,"context_line":"from neutron.plugins.common import constants as p_constants"},{"line_number":31,"context_line":"from neutron.plugins.ml2 import config"},{"line_number":32,"context_line":"from neutron.scheduler import dhcp_agent_scheduler"},{"line_number":33,"context_line":"from neutron.services.segments import db"},{"line_number":34,"context_line":"from neutron.services.segments import exceptions as segment_exc"},{"line_number":35,"context_line":"from neutron.tests.common import helpers"}],"source_content_type":"text/x-python","patch_set":23,"id":"3aaa91ec_a5b0df8c","line":32,"updated":"2016-06-22 00:18:08.000000000","message":"ditto","commit_id":"68529b09ae83992fe90f98278a39d087bf9a8407"}],"neutron/tests/unit/scheduler/test_dhcp_agent_scheduler.py":[{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"03042d31e26e9465a6579245f3d32fca5995e1e1","unresolved":false,"context_lines":[{"line_number":477,"context_line":"        self.assertEqual(\u0027host-c\u0027, agent2[0][\u0027host\u0027])"},{"line_number":478,"context_line":"        self.assertEqual(\u0027host-d\u0027, agent3[0][\u0027host\u0027])"},{"line_number":479,"context_line":""},{"line_number":480,"context_line":"    def test_schedule_segment_one_hostable_agent(self):"},{"line_number":481,"context_line":"        net_id \u003d self._create_network()"},{"line_number":482,"context_line":"        seg_id \u003d self._create_segment(net_id)"},{"line_number":483,"context_line":"        helpers.register_dhcp_agent(HOST_C)"}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_5e79b3fb","line":480,"updated":"2016-06-11 08:48:56.000000000","message":"I feel the tests here are similar to the ones at [1]. For example, if neutron.tests.unit.extensions.test_segment.test_segment_scheduling_with_host_mapping success, this test case will success. The same applies to some of the test cases below.\n\n[1] neutron/tests/unit/extensions/test_segment.py","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"30c80756d53e0b00d046f86cbbabc1037f542aa3","unresolved":false,"context_lines":[{"line_number":477,"context_line":"        self.assertEqual(\u0027host-c\u0027, agent2[0][\u0027host\u0027])"},{"line_number":478,"context_line":"        self.assertEqual(\u0027host-d\u0027, agent3[0][\u0027host\u0027])"},{"line_number":479,"context_line":""},{"line_number":480,"context_line":"    def test_schedule_segment_one_hostable_agent(self):"},{"line_number":481,"context_line":"        net_id \u003d self._create_network()"},{"line_number":482,"context_line":"        seg_id \u003d self._create_segment(net_id)"},{"line_number":483,"context_line":"        helpers.register_dhcp_agent(HOST_C)"}],"source_content_type":"text/x-python","patch_set":22,"id":"3aaa91ec_814b1e2b","line":480,"in_reply_to":"7aa08908_5e79b3fb","updated":"2016-06-21 21:29:46.000000000","message":"there certainly is overlap but the test_segment tests actually test the notification layer AND the scheduling layer, while this just tests the scheduling layer.\n\nThese tests at least verify that what is passed to the dhcp scheduler is handled correctly, whereas these could pass while the test_segments tests fail if the notification layer code changes on how it interfaces with the scheduler code.\n\nHowever, if something does change that causes these tests to fail then the tests in test_segment will fail.  I don\u0027t think thats too big of a deal, but if anyone feels strongly about this I\u0027ll gladly remove them.","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":11159,"name":"Hong Hui Xiao","email":"honghui_xiao@yeah.net","username":"HongHuiXiao"},"change_message_id":"03042d31e26e9465a6579245f3d32fca5995e1e1","unresolved":false,"context_lines":[{"line_number":554,"context_line":"        agents \u003d self.plugin.network_scheduler.schedule("},{"line_number":555,"context_line":"            self.plugin, self.ctx, net)"},{"line_number":556,"context_line":"        self.assertEqual(1, len(agents))"},{"line_number":557,"context_line":"        self.assertEqual(1, len(agents))"},{"line_number":558,"context_line":"        self.assertEqual(HOST_C, agents[0].host)"},{"line_number":559,"context_line":""},{"line_number":560,"context_line":""}],"source_content_type":"text/x-python","patch_set":22,"id":"7aa08908_7e38ef3d","line":557,"range":{"start_line":557,"start_character":0,"end_line":557,"end_character":40},"updated":"2016-06-11 08:48:56.000000000","message":"duplicated check?","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"},{"author":{"_account_id":6951,"name":"Brandon Logan","email":"brandon.logan@rackspace.com","username":"brandon-logan"},"change_message_id":"30c80756d53e0b00d046f86cbbabc1037f542aa3","unresolved":false,"context_lines":[{"line_number":554,"context_line":"        agents \u003d self.plugin.network_scheduler.schedule("},{"line_number":555,"context_line":"            self.plugin, self.ctx, net)"},{"line_number":556,"context_line":"        self.assertEqual(1, len(agents))"},{"line_number":557,"context_line":"        self.assertEqual(1, len(agents))"},{"line_number":558,"context_line":"        self.assertEqual(HOST_C, agents[0].host)"},{"line_number":559,"context_line":""},{"line_number":560,"context_line":""}],"source_content_type":"text/x-python","patch_set":22,"id":"3aaa91ec_21734ab3","line":557,"range":{"start_line":557,"start_character":0,"end_line":557,"end_character":40},"in_reply_to":"7aa08908_7e38ef3d","updated":"2016-06-21 21:29:46.000000000","message":"Done","commit_id":"3ca8d32816d92090e36cc4d3bb5db143780720a6"}]}