)]}'
{"neutron/services/qos/drivers/manager.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"1d0b86428f65e18b06040af8e2df7e5c67c2307d","unresolved":false,"context_lines":[{"line_number":149,"context_line":"                    continue"},{"line_number":150,"context_line":""},{"line_number":151,"context_line":"            if driver.is_rule_supported(rule):"},{"line_number":152,"context_line":"                driver.validate_rule_for_port(context, rule, port)"},{"line_number":153,"context_line":"                return True"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"        return False"},{"line_number":156,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_073fa684","line":153,"range":{"start_line":152,"start_character":16,"end_line":153,"end_character":27},"updated":"2020-02-05 09:24:25.000000000","message":"I think this should be:\n\n  if driver.validate_rule_for_port(context, rule, port):\n    return True","commit_id":"220d54832556c5bbeb8a56848a1d784f829c935e"},{"author":{"_account_id":26106,"name":"Tom Stappaerts","email":"tom.stappaerts@nokia.com","username":"TomStappaerts"},"change_message_id":"f29026f2696114cda78aad487766eb368264b53c","unresolved":false,"context_lines":[{"line_number":149,"context_line":"                    continue"},{"line_number":150,"context_line":""},{"line_number":151,"context_line":"            if driver.is_rule_supported(rule):"},{"line_number":152,"context_line":"                driver.validate_rule_for_port(context, rule, port)"},{"line_number":153,"context_line":"                return True"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"        return False"},{"line_number":156,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_0797a639","line":153,"range":{"start_line":152,"start_character":16,"end_line":153,"end_character":27},"in_reply_to":"3fa7e38b_073fa684","updated":"2020-02-05 09:44:23.000000000","message":"Agreed, that is also the structure of the base method here: return True or False; same structure in drivers seems logical\n\nBut I propose the following do a continue instead; so if at least one driver will be able to act they can.","commit_id":"220d54832556c5bbeb8a56848a1d784f829c935e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"b46c882294a96e0589a020acb2ec8b73e79c3fff","unresolved":false,"context_lines":[{"line_number":149,"context_line":"                    continue"},{"line_number":150,"context_line":""},{"line_number":151,"context_line":"            if (driver.is_rule_supported(rule) and"},{"line_number":152,"context_line":"                    driver.validate_rule_for_port(context, rule, port)):"},{"line_number":153,"context_line":"                return True"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa7e38b_d3df0c37","line":152,"updated":"2020-02-05 13:38:54.000000000","message":"+1","commit_id":"13d9bbc4afba107c61566f0024ac11ffa493c26c"}],"neutron/services/qos/drivers/openvswitch/driver.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"1d0b86428f65e18b06040af8e2df7e5c67c2307d","unresolved":false,"context_lines":[{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def validate_rule_for_port(self, context, rule, port):"},{"line_number":64,"context_line":"        \"\"\"Minimum-bandwidth rule is only supported on networks whose"},{"line_number":65,"context_line":"        first segment is backed by a physnet."},{"line_number":66,"context_line":"        \"\"\""},{"line_number":67,"context_line":"        if rule.rule_type \u003d\u003d qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH:"},{"line_number":68,"context_line":"            net \u003d network_object.Network.get_object("}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_6759bafe","line":65,"updated":"2020-02-05 09:24:25.000000000","message":"nit: this is the method description. Any other comment like this one, should be this, a comment inside the function.","commit_id":"220d54832556c5bbeb8a56848a1d784f829c935e"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"working for Ericsson, UTC+1 (+DST)"},"change_message_id":"a382c938dba3d634455afc2c218e471224dc3144","unresolved":false,"context_lines":[{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def validate_rule_for_port(self, context, rule, port):"},{"line_number":64,"context_line":"        \"\"\"Minimum-bandwidth rule is only supported on networks whose"},{"line_number":65,"context_line":"        first segment is backed by a physnet."},{"line_number":66,"context_line":"        \"\"\""},{"line_number":67,"context_line":"        if rule.rule_type \u003d\u003d qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH:"},{"line_number":68,"context_line":"            net \u003d network_object.Network.get_object("}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_b8b90fc6","line":65,"in_reply_to":"3fa7e38b_6759bafe","updated":"2020-02-05 13:23:45.000000000","message":"Done","commit_id":"220d54832556c5bbeb8a56848a1d784f829c935e"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"1d0b86428f65e18b06040af8e2df7e5c67c2307d","unresolved":false,"context_lines":[{"line_number":70,"context_line":"            physnet \u003d net.segments[0].physical_network"},{"line_number":71,"context_line":"            if physnet is None:"},{"line_number":72,"context_line":"                raise qos_exc.QosRuleNotSupported(rule_type\u003drule.rule_type,"},{"line_number":73,"context_line":"                                                  port_id\u003dport[\u0027id\u0027])"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"def register():"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_a7fb32bd","line":73,"updated":"2020-02-05 09:24:25.000000000","message":"Now my question is: if we have two drivers, for example LB and OVS, and in [1] we check LB first, this will pass, but with OVS this will raise an exception.\n\nThat means in a system with two drivers, we won\u0027t be able to schedule a port for the accepted one because of OVS.\n\nAm I missing something here?\n\n[1] https://review.opendev.org/#/c/705695/1/neutron/services/qos/drivers/manager.py@152","commit_id":"220d54832556c5bbeb8a56848a1d784f829c935e"},{"author":{"_account_id":26106,"name":"Tom Stappaerts","email":"tom.stappaerts@nokia.com","username":"TomStappaerts"},"change_message_id":"f29026f2696114cda78aad487766eb368264b53c","unresolved":false,"context_lines":[{"line_number":70,"context_line":"            physnet \u003d net.segments[0].physical_network"},{"line_number":71,"context_line":"            if physnet is None:"},{"line_number":72,"context_line":"                raise qos_exc.QosRuleNotSupported(rule_type\u003drule.rule_type,"},{"line_number":73,"context_line":"                                                  port_id\u003dport[\u0027id\u0027])"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"def register():"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_e7f28a9c","line":73,"in_reply_to":"3fa7e38b_a7fb32bd","updated":"2020-02-05 09:44:23.000000000","message":"In my view on the manager side it should be continue instead of return True|False, just like for ex. vif type.\n\nBut how does it generally work with multiple drivers? If it is based on vif/vnic type then one of them acts upon it then maybe a bigger refactoring is needed to take all of the variables (vif/vnic\u0026this method) into account.","commit_id":"220d54832556c5bbeb8a56848a1d784f829c935e"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"working for Ericsson, UTC+1 (+DST)"},"change_message_id":"a382c938dba3d634455afc2c218e471224dc3144","unresolved":false,"context_lines":[{"line_number":70,"context_line":"            physnet \u003d net.segments[0].physical_network"},{"line_number":71,"context_line":"            if physnet is None:"},{"line_number":72,"context_line":"                raise qos_exc.QosRuleNotSupported(rule_type\u003drule.rule_type,"},{"line_number":73,"context_line":"                                                  port_id\u003dport[\u0027id\u0027])"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"def register():"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_189e23b7","line":73,"in_reply_to":"3fa7e38b_e7f28a9c","updated":"2020-02-05 13:23:45.000000000","message":"Ahem, I didn\u0027t think about the need to support more than one drivers.\n\nThe original validation logic is:\nfor all ports (qos/qos_plugin.py line 219)\nfor all rules (qos/qos_plugin.py line 223)\nfor any drivers (qos/drivers/manager.py line 141)\n\nNow I changed the raise/pass behavior to returning a boolean.\n\nTheoretically we still run the chance that qosDriverA validates True for portB, while qosDriverB validates False but mechDriverB binds portB. This is possible, right? Is it realistic? I don\u0027t know.\n\nUnless each driver cares only about its own ports (that is the ports it bound). For example the ovs driver would validate ports with vif_type\u003d\u003dovs. Other drivers would not even look at ports with vif_type\u003d\u003dovs. But that means we\u0027d need to re-validate qos policies/rules at port binding time - possibly affecting the outcome of port binding. We don\u0027t have anything like that today, do we?","commit_id":"220d54832556c5bbeb8a56848a1d784f829c935e"}],"neutron/tests/unit/services/qos/drivers/openvswitch/test_driver.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"8dac27d70942c855a77a941a5d833bd1570c38cb","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        super(TestOVSDriver, self).setUp()"},{"line_number":28,"context_line":"        self.driver \u003d driver.OVSDriver.create()"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    def test_validate_min_bw_rule_on_physnet_port(self):"},{"line_number":31,"context_line":"        segment \u003d network_object.NetworkSegment("},{"line_number":32,"context_line":"            physical_network\u003d\u0027fake physnet\u0027)"},{"line_number":33,"context_line":"        net \u003d network_object.Network(mock.Mock(), segments\u003d[segment])"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa7e38b_c0742606","line":30,"updated":"2020-02-20 13:58:50.000000000","message":"Both tests look very similar.\n\nCan you join both in a single one? Then you can iterate though:\n  test_inputs \u003d [(\u0027fake physnet\u0027, self.assertTrue), (None, self.assertFalse)]\n  for physnet, test_method:\n    ...","commit_id":"ff1f7ff313d6eaede986364d19734d0f2ecc0ccf"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"working for Ericsson, UTC+1 (+DST)"},"change_message_id":"7aa0102a60c23e52177f373abda81253750416be","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        super(TestOVSDriver, self).setUp()"},{"line_number":28,"context_line":"        self.driver \u003d driver.OVSDriver.create()"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    def test_validate_min_bw_rule_on_physnet_port(self):"},{"line_number":31,"context_line":"        segment \u003d network_object.NetworkSegment("},{"line_number":32,"context_line":"            physical_network\u003d\u0027fake physnet\u0027)"},{"line_number":33,"context_line":"        net \u003d network_object.Network(mock.Mock(), segments\u003d[segment])"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa7e38b_d3614cd7","line":30,"in_reply_to":"3fa7e38b_c0742606","updated":"2020-02-21 14:13:46.000000000","message":"Done","commit_id":"ff1f7ff313d6eaede986364d19734d0f2ecc0ccf"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"d538d2d687ec56704aee77ebac44a79719a36071","unresolved":false,"context_lines":[{"line_number":32,"context_line":"            ({\u0027physical_network\u0027: \u0027fake physnet\u0027}, self.assertTrue),"},{"line_number":33,"context_line":"            ({}, self.assertFalse),"},{"line_number":34,"context_line":"        ]"},{"line_number":35,"context_line":"        for segment_kwargs, test_method in scenarios:"},{"line_number":36,"context_line":"            segment \u003d network_object.NetworkSegment(**segment_kwargs)"},{"line_number":37,"context_line":"            net \u003d network_object.Network(mock.Mock(), segments\u003d[segment])"},{"line_number":38,"context_line":"            rule \u003d mock.Mock()"}],"source_content_type":"text/x-python","patch_set":4,"id":"1fa4df85_e26883bb","line":35,"range":{"start_line":35,"start_character":28,"end_line":35,"end_character":39},"updated":"2020-02-24 11:25:35.000000000","message":"nit: personally I would use \"True\" and \"False\" here, name it as expected_result and in L44 use self.assertEqual() instead of this \"test_method\". IMO this would be easier to read but that solution is also fine.","commit_id":"321afc8f897c8132ddc6edd2b59285111d0d73e0"}]}