)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":12860,"name":"Dongcan Ye","email":"yedongcan@yeah.net","username":"yedongcan"},"change_message_id":"a3b48dcd2a75fcd7ea74f23cafcc8cd8f202760f","unresolved":false,"context_lines":[{"line_number":7,"context_line":"[Qos] Fix residues of ovs in ingress bw limit"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When we delete vm port with attached QoS policy,"},{"line_number":10,"context_line":"it is just doing nothing if vif_port not exists anymore driver."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For ingress bw limit however it\u0027s uses additional records in Openvswitch"},{"line_number":13,"context_line":"database: qos and queue. Those records wasn\u0027t cleaned in such case."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"3f4b6375_afbd7d69","line":10,"range":{"start_line":10,"start_character":0,"end_line":10,"end_character":63},"updated":"2017-10-31 07:15:41.000000000","message":"it is just doing nothing if vif_port not exists.","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"0bcdcdef59fe934d024382bee09af4e7d077b380","unresolved":false,"context_lines":[{"line_number":7,"context_line":"[Qos] Fix residues of ovs in ingress bw limit"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When we delete vm port with attached QoS policy,"},{"line_number":10,"context_line":"it is just doing nothing if vif_port not exists anymore driver."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For ingress bw limit however it\u0027s uses additional records in Openvswitch"},{"line_number":13,"context_line":"database: qos and queue. Those records wasn\u0027t cleaned in such case."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"3f4b6375_d28cdcd8","line":10,"range":{"start_line":10,"start_character":0,"end_line":10,"end_character":63},"in_reply_to":"3f4b6375_afbd7d69","updated":"2017-10-31 07:33:48.000000000","message":"Done","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":12860,"name":"Dongcan Ye","email":"yedongcan@yeah.net","username":"yedongcan"},"change_message_id":"a3b48dcd2a75fcd7ea74f23cafcc8cd8f202760f","unresolved":false,"context_lines":[{"line_number":9,"context_line":"When we delete vm port with attached QoS policy,"},{"line_number":10,"context_line":"it is just doing nothing if vif_port not exists anymore driver."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For ingress bw limit however it\u0027s uses additional records in Openvswitch"},{"line_number":13,"context_line":"database: qos and queue. Those records wasn\u0027t cleaned in such case."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"This patch record port_name in self.ports refer to dscp approach."},{"line_number":16,"context_line":"Never execute port clear if vif_port not exists. Finally, ovs"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"3f4b6375_d22f3c86","line":13,"range":{"start_line":12,"start_character":0,"end_line":13,"end_character":67},"updated":"2017-10-31 07:15:41.000000000","message":"For ingress bw limit, it\u0027s uses additional records in Openvswitch\ndatabase: qos and queue. Those records wasn\u0027t cleaned in such case.","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"0bcdcdef59fe934d024382bee09af4e7d077b380","unresolved":false,"context_lines":[{"line_number":9,"context_line":"When we delete vm port with attached QoS policy,"},{"line_number":10,"context_line":"it is just doing nothing if vif_port not exists anymore driver."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"For ingress bw limit however it\u0027s uses additional records in Openvswitch"},{"line_number":13,"context_line":"database: qos and queue. Those records wasn\u0027t cleaned in such case."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"This patch record port_name in self.ports refer to dscp approach."},{"line_number":16,"context_line":"Never execute port clear if vif_port not exists. Finally, ovs"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"3f4b6375_b2bde069","line":13,"range":{"start_line":12,"start_character":0,"end_line":13,"end_character":67},"in_reply_to":"3f4b6375_d22f3c86","updated":"2017-10-31 07:33:48.000000000","message":"Done","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":12860,"name":"Dongcan Ye","email":"yedongcan@yeah.net","username":"yedongcan"},"change_message_id":"a3b48dcd2a75fcd7ea74f23cafcc8cd8f202760f","unresolved":false,"context_lines":[{"line_number":12,"context_line":"For ingress bw limit however it\u0027s uses additional records in Openvswitch"},{"line_number":13,"context_line":"database: qos and queue. Those records wasn\u0027t cleaned in such case."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"This patch record port_name in self.ports refer to dscp approach."},{"line_number":16,"context_line":"Never execute port clear if vif_port not exists. Finally, ovs"},{"line_number":17,"context_line":"driver can clean such qos and queue records"},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"3f4b6375_924f8469","line":15,"range":{"start_line":15,"start_character":18,"end_line":15,"end_character":27},"updated":"2017-10-31 07:15:41.000000000","message":"port?","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"0bcdcdef59fe934d024382bee09af4e7d077b380","unresolved":false,"context_lines":[{"line_number":12,"context_line":"For ingress bw limit however it\u0027s uses additional records in Openvswitch"},{"line_number":13,"context_line":"database: qos and queue. Those records wasn\u0027t cleaned in such case."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"This patch record port_name in self.ports refer to dscp approach."},{"line_number":16,"context_line":"Never execute port clear if vif_port not exists. Finally, ovs"},{"line_number":17,"context_line":"driver can clean such qos and queue records"},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"3f4b6375_92c0a4e2","line":15,"range":{"start_line":15,"start_character":18,"end_line":15,"end_character":27},"in_reply_to":"3f4b6375_924f8469","updated":"2017-10-31 07:33:48.000000000","message":"Done","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"1e87e525f81fdad8e0ee87a72574f5832be2759f","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Zachary \u003cmazengxie@126.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-11-03 15:24:17 +0800"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"[Qos] Fix residues of ovs in ingress bw limit"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When we delete vm port with attached QoS policy,"},{"line_number":10,"context_line":"it is just doing nothing if vif_port not exists."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":22,"id":"1f485f77_a80faadf","line":7,"updated":"2017-11-13 21:59:09.000000000","message":"This corner case could benefit from a proper test involving backend, like functional or even fullstack. We would add port / delete port, then check that queues are gone. Seems like right now we have unit tests for this piece of qos codebase, but it\u0027s not enough. Thoughts?","commit_id":"549a7a54f591a972394f2f3480ecaf8abec89ca0"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"8c01c0a95161ea2a4cf60b210f822472c8ee1810","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Zachary \u003cmazengxie@126.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-11-03 15:24:17 +0800"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"[Qos] Fix residues of ovs in ingress bw limit"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When we delete vm port with attached QoS policy,"},{"line_number":10,"context_line":"it is just doing nothing if vif_port not exists."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":22,"id":"1f485f77_8373b1b1","line":7,"in_reply_to":"1f485f77_a80faadf","updated":"2017-11-13 22:33:46.000000000","message":"I agree with You that fullstack test would be good to cover this case","commit_id":"549a7a54f591a972394f2f3480ecaf8abec89ca0"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"8221c7317779f13624e8cf5397506f1f34b0f148","unresolved":false,"context_lines":[{"line_number":7,"context_line":"[Qos] Fix residues of ovs in ingress bw limit"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When we delete vm port with attached QoS policy,"},{"line_number":10,"context_line":"it is just doing nothing if vif_port not exists."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"This is fine for egress bandwidth limit as it is configured"},{"line_number":13,"context_line":"directly on vif_port in OVS."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"ff82abbf_272e6343","line":10,"range":{"start_line":10,"start_character":37,"end_line":10,"end_character":47},"updated":"2017-11-21 22:39:45.000000000","message":"does not exist","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"eb0eaaabccd138bd38ce53b494db435a5392bc03","unresolved":false,"context_lines":[{"line_number":7,"context_line":"[Qos] Fix residues of ovs in ingress bw limit"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When we delete vm port with attached QoS policy,"},{"line_number":10,"context_line":"it is just doing nothing if vif_port not exists."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"This is fine for egress bandwidth limit as it is configured"},{"line_number":13,"context_line":"directly on vif_port in OVS."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"ff82abbf_8c54d222","line":10,"range":{"start_line":10,"start_character":37,"end_line":10,"end_character":47},"in_reply_to":"ff82abbf_272e6343","updated":"2017-11-27 07:28:00.000000000","message":"Done","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"8221c7317779f13624e8cf5397506f1f34b0f148","unresolved":false,"context_lines":[{"line_number":12,"context_line":"This is fine for egress bandwidth limit as it is configured"},{"line_number":13,"context_line":"directly on vif_port in OVS."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"For ingress bw limit however it\u0027s uses additional records in"},{"line_number":16,"context_line":"Openvswitch database: qos and queue. Those records wasn\u0027t"},{"line_number":17,"context_line":"cleaned in such case."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"ff82abbf_c71e970b","line":15,"range":{"start_line":15,"start_character":29,"end_line":15,"end_character":38},"updated":"2017-11-21 22:39:45.000000000","message":"it uses","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"eb0eaaabccd138bd38ce53b494db435a5392bc03","unresolved":false,"context_lines":[{"line_number":12,"context_line":"This is fine for egress bandwidth limit as it is configured"},{"line_number":13,"context_line":"directly on vif_port in OVS."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"For ingress bw limit however it\u0027s uses additional records in"},{"line_number":16,"context_line":"Openvswitch database: qos and queue. Those records wasn\u0027t"},{"line_number":17,"context_line":"cleaned in such case."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"ff82abbf_6c570e28","line":15,"range":{"start_line":15,"start_character":29,"end_line":15,"end_character":38},"in_reply_to":"ff82abbf_c71e970b","updated":"2017-11-27 07:28:00.000000000","message":"Done","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"8221c7317779f13624e8cf5397506f1f34b0f148","unresolved":false,"context_lines":[{"line_number":13,"context_line":"directly on vif_port in OVS."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"For ingress bw limit however it\u0027s uses additional records in"},{"line_number":16,"context_line":"Openvswitch database: qos and queue. Those records wasn\u0027t"},{"line_number":17,"context_line":"cleaned in such case."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"This patch record port in self.ports refer to dscp approach."},{"line_number":20,"context_line":"Never execute port clear if vif_port not exists. Finally, ovs"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"ff82abbf_47d84752","line":17,"range":{"start_line":16,"start_character":51,"end_line":17,"end_character":20},"updated":"2017-11-21 22:39:45.000000000","message":"are not cleaned up","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"eb0eaaabccd138bd38ce53b494db435a5392bc03","unresolved":false,"context_lines":[{"line_number":13,"context_line":"directly on vif_port in OVS."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"For ingress bw limit however it\u0027s uses additional records in"},{"line_number":16,"context_line":"Openvswitch database: qos and queue. Those records wasn\u0027t"},{"line_number":17,"context_line":"cleaned in such case."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"This patch record port in self.ports refer to dscp approach."},{"line_number":20,"context_line":"Never execute port clear if vif_port not exists. Finally, ovs"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"ff82abbf_cc5e5a43","line":17,"range":{"start_line":16,"start_character":51,"end_line":17,"end_character":20},"in_reply_to":"ff82abbf_47d84752","updated":"2017-11-27 07:28:00.000000000","message":"Done","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"8221c7317779f13624e8cf5397506f1f34b0f148","unresolved":false,"context_lines":[{"line_number":16,"context_line":"Openvswitch database: qos and queue. Those records wasn\u0027t"},{"line_number":17,"context_line":"cleaned in such case."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"This patch record port in self.ports refer to dscp approach."},{"line_number":20,"context_line":"Never execute port clear if vif_port not exists. Finally, ovs"},{"line_number":21,"context_line":"driver can clean such qos and queue records"},{"line_number":22,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"ff82abbf_47c12754","line":19,"range":{"start_line":19,"start_character":11,"end_line":19,"end_character":17},"updated":"2017-11-21 22:39:45.000000000","message":"also records","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"8221c7317779f13624e8cf5397506f1f34b0f148","unresolved":false,"context_lines":[{"line_number":16,"context_line":"Openvswitch database: qos and queue. Those records wasn\u0027t"},{"line_number":17,"context_line":"cleaned in such case."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"This patch record port in self.ports refer to dscp approach."},{"line_number":20,"context_line":"Never execute port clear if vif_port not exists. Finally, ovs"},{"line_number":21,"context_line":"driver can clean such qos and queue records"},{"line_number":22,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"ff82abbf_67c9cb76","line":19,"range":{"start_line":19,"start_character":37,"end_line":19,"end_character":59},"updated":"2017-11-21 22:39:45.000000000","message":"in the case of bandwidth limit rules, just as in the case of dscp rules.","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"eb0eaaabccd138bd38ce53b494db435a5392bc03","unresolved":false,"context_lines":[{"line_number":16,"context_line":"Openvswitch database: qos and queue. Those records wasn\u0027t"},{"line_number":17,"context_line":"cleaned in such case."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"This patch record port in self.ports refer to dscp approach."},{"line_number":20,"context_line":"Never execute port clear if vif_port not exists. Finally, ovs"},{"line_number":21,"context_line":"driver can clean such qos and queue records"},{"line_number":22,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"ff82abbf_2c334616","line":19,"range":{"start_line":19,"start_character":11,"end_line":19,"end_character":17},"in_reply_to":"ff82abbf_47c12754","updated":"2017-11-27 07:28:00.000000000","message":"Done","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"eb0eaaabccd138bd38ce53b494db435a5392bc03","unresolved":false,"context_lines":[{"line_number":16,"context_line":"Openvswitch database: qos and queue. Those records wasn\u0027t"},{"line_number":17,"context_line":"cleaned in such case."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"This patch record port in self.ports refer to dscp approach."},{"line_number":20,"context_line":"Never execute port clear if vif_port not exists. Finally, ovs"},{"line_number":21,"context_line":"driver can clean such qos and queue records"},{"line_number":22,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"ff82abbf_4c380aee","line":19,"range":{"start_line":19,"start_character":37,"end_line":19,"end_character":59},"in_reply_to":"ff82abbf_67c9cb76","updated":"2017-11-27 07:28:00.000000000","message":"Done","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"}],"neutron/agent/common/ovs_lib.py":[{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"6f3993a5bbd0103f0245cee2eeddc04488024227","unresolved":false,"context_lines":[{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"},{"line_number":792,"context_line":"            if qos:"},{"line_number":793,"context_line":"                txn.add(self.ovsdb.db_destroy(\u0027QoS\u0027, qos[\u0027_uuid\u0027]))"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f4b6375_266fc42b","line":790,"updated":"2017-10-27 07:26:17.000000000","message":"this additional variable is not necessary IMHO, You can do self.port_exists() directly in this \"if\" condition","commit_id":"26e42edd627d7a79cfbb6200bce1bd5bcd521b63"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"64620290315c8e25a7524a7c865713dab13fb49e","unresolved":false,"context_lines":[{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"},{"line_number":792,"context_line":"            if qos:"},{"line_number":793,"context_line":"                txn.add(self.ovsdb.db_destroy(\u0027QoS\u0027, qos[\u0027_uuid\u0027]))"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f4b6375_6107e619","line":790,"in_reply_to":"3f4b6375_266fc42b","updated":"2017-10-27 08:10:55.000000000","message":"in self.ovsdb.transaction(check_error\u003dTrue) \nuse self.port_exists always return False","commit_id":"26e42edd627d7a79cfbb6200bce1bd5bcd521b63"},{"author":{"_account_id":12860,"name":"Dongcan Ye","email":"yedongcan@yeah.net","username":"yedongcan"},"change_message_id":"a3b48dcd2a75fcd7ea74f23cafcc8cd8f202760f","unresolved":false,"context_lines":[{"line_number":691,"context_line":""},{"line_number":692,"context_line":"        return max_kbps, max_burst_kbps"},{"line_number":693,"context_line":""},{"line_number":694,"context_line":"    def delete_egress_bw_limit_for_port(self, port_name):"},{"line_number":695,"context_line":"        if not self.port_exists(port_name):"},{"line_number":696,"context_line":"            return"},{"line_number":697,"context_line":"        self._set_egress_bw_limit_for_port("},{"line_number":698,"context_line":"            port_name, 0, 0)"},{"line_number":699,"context_line":""},{"line_number":700,"context_line":"    def _find_qos(self, port_name):"},{"line_number":701,"context_line":"        qos \u003d self.ovsdb.db_find("}],"source_content_type":"text/x-python","patch_set":7,"id":"3f4b6375_121cb411","line":698,"range":{"start_line":694,"start_character":4,"end_line":698,"end_character":28},"updated":"2017-10-31 07:15:41.000000000","message":"Does egress bw limit have the same problem?","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"907446293aaefb3ab711952a9cecdef2c4240a21","unresolved":false,"context_lines":[{"line_number":785,"context_line":"    def delete_ingress_bw_limit_for_port(self, port_name):"},{"line_number":786,"context_line":"        qos \u003d self._find_qos(port_name)"},{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f4b6375_898959cb","line":788,"updated":"2017-11-02 08:48:50.000000000","message":"nit: maybe \"port_exists\"?","commit_id":"a12853d9109086d2d417e0f3ffbe774b59f6d22b"},{"author":{"_account_id":18051,"name":"David Shaughnessy","email":"david.shaughnessy@intel.com","username":"davidsha"},"change_message_id":"6cc489d92ffbd3ed5473384f90d8556519f373af","unresolved":false,"context_lines":[{"line_number":785,"context_line":"    def delete_ingress_bw_limit_for_port(self, port_name):"},{"line_number":786,"context_line":"        qos \u003d self._find_qos(port_name)"},{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"}],"source_content_type":"text/x-python","patch_set":22,"id":"3f4b6375_58573f02","line":788,"range":{"start_line":788,"start_character":8,"end_line":788,"end_character":10},"updated":"2017-11-07 16:34:11.000000000","message":"nit: does_port_exist","commit_id":"549a7a54f591a972394f2f3480ecaf8abec89ca0"},{"author":{"_account_id":18051,"name":"David Shaughnessy","email":"david.shaughnessy@intel.com","username":"davidsha"},"change_message_id":"6cc489d92ffbd3ed5473384f90d8556519f373af","unresolved":false,"context_lines":[{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"},{"line_number":792,"context_line":"            if qos:"},{"line_number":793,"context_line":"                txn.add(self.ovsdb.db_destroy(\u0027QoS\u0027, qos[\u0027_uuid\u0027]))"}],"source_content_type":"text/x-python","patch_set":22,"id":"3f4b6375_3842433f","line":790,"range":{"start_line":790,"start_character":15,"end_line":790,"end_character":17},"updated":"2017-11-07 16:34:11.000000000","message":"ditto","commit_id":"549a7a54f591a972394f2f3480ecaf8abec89ca0"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"8221c7317779f13624e8cf5397506f1f34b0f148","unresolved":false,"context_lines":[{"line_number":785,"context_line":"    def delete_ingress_bw_limit_for_port(self, port_name):"},{"line_number":786,"context_line":"        qos \u003d self._find_qos(port_name)"},{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_2cbe4a65","line":788,"range":{"start_line":788,"start_character":8,"end_line":788,"end_character":21},"updated":"2017-11-21 22:39:45.000000000","message":"does_port_exist","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"8221c7317779f13624e8cf5397506f1f34b0f148","unresolved":false,"context_lines":[{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"},{"line_number":792,"context_line":"            if qos:"},{"line_number":793,"context_line":"                txn.add(self.ovsdb.db_destroy(\u0027QoS\u0027, qos[\u0027_uuid\u0027]))"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_4cc38eee","line":790,"range":{"start_line":790,"start_character":15,"end_line":790,"end_character":28},"updated":"2017-11-21 22:39:45.000000000","message":"does_port_exist","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"85992449a389102998a4dcf04dddcee2cc34bac5","unresolved":false,"context_lines":[{"line_number":785,"context_line":"    def delete_ingress_bw_limit_for_port(self, port_name):"},{"line_number":786,"context_line":"        qos \u003d self._find_qos(port_name)"},{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"}],"source_content_type":"text/x-python","patch_set":25,"id":"ff82abbf_08f7f98c","line":788,"updated":"2017-11-29 21:58:33.000000000","message":"Back in PS23, I left you this comment: https://review.openstack.org/#/c/515566/23/neutron/agent/common/ovs_lib.py@788? Don\u0027t you plan to address it? \"is_port_exist\u0027 doesn\u0027t make sense in English. \"does_port_exist\" does","commit_id":"dae655e99ec49d835274fe3aa505b60a7bc6ce28"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"10342fca8c79a1e7757281260468ef05723a1534","unresolved":false,"context_lines":[{"line_number":785,"context_line":"    def delete_ingress_bw_limit_for_port(self, port_name):"},{"line_number":786,"context_line":"        qos \u003d self._find_qos(port_name)"},{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"}],"source_content_type":"text/x-python","patch_set":25,"id":"ff82abbf_4062adaf","line":788,"in_reply_to":"ff82abbf_08f7f98c","updated":"2017-11-30 08:21:00.000000000","message":"Done","commit_id":"dae655e99ec49d835274fe3aa505b60a7bc6ce28"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"85992449a389102998a4dcf04dddcee2cc34bac5","unresolved":false,"context_lines":[{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"},{"line_number":792,"context_line":"            if qos:"},{"line_number":793,"context_line":"                txn.add(self.ovsdb.db_destroy(\u0027QoS\u0027, qos[\u0027_uuid\u0027]))"}],"source_content_type":"text/x-python","patch_set":25,"id":"ff82abbf_c8ecb1bc","line":790,"updated":"2017-11-29 21:58:33.000000000","message":"Ditto","commit_id":"dae655e99ec49d835274fe3aa505b60a7bc6ce28"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"10342fca8c79a1e7757281260468ef05723a1534","unresolved":false,"context_lines":[{"line_number":787,"context_line":"        queue \u003d self._find_queue(port_name, QOS_DEFAULT_QUEUE)"},{"line_number":788,"context_line":"        is_port_exist \u003d self.port_exists(port_name)"},{"line_number":789,"context_line":"        with self.ovsdb.transaction(check_error\u003dTrue) as txn:"},{"line_number":790,"context_line":"            if is_port_exist:"},{"line_number":791,"context_line":"                txn.add(self.ovsdb.db_clear(\"Port\", port_name, \u0027qos\u0027))"},{"line_number":792,"context_line":"            if qos:"},{"line_number":793,"context_line":"                txn.add(self.ovsdb.db_destroy(\u0027QoS\u0027, qos[\u0027_uuid\u0027]))"}],"source_content_type":"text/x-python","patch_set":25,"id":"ff82abbf_606571b4","line":790,"in_reply_to":"ff82abbf_c8ecb1bc","updated":"2017-11-30 08:21:00.000000000","message":"Done","commit_id":"dae655e99ec49d835274fe3aa505b60a7bc6ce28"}],"neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py":[{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"6f3993a5bbd0103f0245cee2eeddc04488024227","unresolved":false,"context_lines":[{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def delete_bandwidth_limit_ingress(self, port):"},{"line_number":76,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":77,"context_line":"        port \u003d self.ports[port_id].pop(constants.INGRESS_DIRECTION, 0)"},{"line_number":78,"context_line":"        if not port:"},{"line_number":79,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":80,"context_line":"                      \"for port %s but vif_port was not found. \""}],"source_content_type":"text/x-python","patch_set":2,"id":"3f4b6375_465d2077","line":77,"updated":"2017-10-27 07:26:17.000000000","message":"what if agent will be restarted? there is possibility that QoS and queue for such port will not be cleaned as it will not exists in self.ports dict anymore, right?","commit_id":"26e42edd627d7a79cfbb6200bce1bd5bcd521b63"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"98dfcb7fe063bfc5d38a7d137ebfff4e0bfb70df","unresolved":false,"context_lines":[{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def delete_bandwidth_limit_ingress(self, port):"},{"line_number":76,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":77,"context_line":"        port \u003d self.ports[port_id].pop(constants.INGRESS_DIRECTION, 0)"},{"line_number":78,"context_line":"        if not port:"},{"line_number":79,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":80,"context_line":"                      \"for port %s but vif_port was not found. \""}],"source_content_type":"text/x-python","patch_set":2,"id":"3f4b6375_8c67b94d","line":77,"in_reply_to":"3f4b6375_465d2077","updated":"2017-10-27 08:28:28.000000000","message":"when the ovs agent started, it will call update_bandwidth_limit function again.\nI test it in this case.","commit_id":"26e42edd627d7a79cfbb6200bce1bd5bcd521b63"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f1c3a458fd4c1aa5a7583aa62383d284cff3fa9a","unresolved":false,"context_lines":[{"line_number":55,"context_line":"                      \"vif_port was not found. It seems that port is already \""},{"line_number":56,"context_line":"                      \"deleted\", port_id)"},{"line_number":57,"context_line":"            return"},{"line_number":58,"context_line":"        self.ports[port[\u0027port_id\u0027]][rule.direction] \u003d port"},{"line_number":59,"context_line":"        if rule.direction \u003d\u003d constants.INGRESS_DIRECTION:"},{"line_number":60,"context_line":"            self._update_ingress_bandwidth_limit(vif_port, rule)"},{"line_number":61,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_cb97ad3a","line":58,"updated":"2017-10-30 08:22:40.000000000","message":"why You are using \"rule.direction\" as key? For DSCP it is rule type name and here You should use also rule type. Maybe add one more dict inside it with direction as a key","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"1659ea176375369de88ef311a2455f434ada856b","unresolved":false,"context_lines":[{"line_number":55,"context_line":"                      \"vif_port was not found. It seems that port is already \""},{"line_number":56,"context_line":"                      \"deleted\", port_id)"},{"line_number":57,"context_line":"            return"},{"line_number":58,"context_line":"        self.ports[port[\u0027port_id\u0027]][rule.direction] \u003d port"},{"line_number":59,"context_line":"        if rule.direction \u003d\u003d constants.INGRESS_DIRECTION:"},{"line_number":60,"context_line":"            self._update_ingress_bandwidth_limit(vif_port, rule)"},{"line_number":61,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_159dbe56","line":58,"in_reply_to":"3f4b6375_10a5d3b5","updated":"2017-10-31 08:01:37.000000000","message":"but \"ingress\" and \"egress\" can be used also for minimum_bandwidth for example so it\u0027s not so easy.\nOf course currently minimum_bandwidth is available only for SR-IOV but in (I hope) near feature it should be available also for other drivers.","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"bffb2119ef1d99e36d22fa0eafaf4cbcf4a09f3e","unresolved":false,"context_lines":[{"line_number":55,"context_line":"                      \"vif_port was not found. It seems that port is already \""},{"line_number":56,"context_line":"                      \"deleted\", port_id)"},{"line_number":57,"context_line":"            return"},{"line_number":58,"context_line":"        self.ports[port[\u0027port_id\u0027]][rule.direction] \u003d port"},{"line_number":59,"context_line":"        if rule.direction \u003d\u003d constants.INGRESS_DIRECTION:"},{"line_number":60,"context_line":"            self._update_ingress_bandwidth_limit(vif_port, rule)"},{"line_number":61,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_10a5d3b5","line":58,"in_reply_to":"3f4b6375_cb97ad3a","updated":"2017-10-31 02:22:48.000000000","message":"“ingress” and “egress” belong to bandwidth_limit， but \"ingress\",\"egress\" and \"dscp\" are tied. New dict with direction as a key to make it complicated.","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f1c3a458fd4c1aa5a7583aa62383d284cff3fa9a","unresolved":false,"context_lines":[{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    def delete_bandwidth_limit(self, port):"},{"line_number":65,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":66,"context_line":"        port \u003d self.ports[port_id].pop(constants.EGRESS_DIRECTION, 0)"},{"line_number":67,"context_line":"        if not port:"},{"line_number":68,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received for port %s but \""},{"line_number":69,"context_line":"                      \"vif_port was not found. It seems that port is already \""}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_ab9c715d","line":66,"updated":"2017-10-30 08:22:40.000000000","message":"Why \"0\" as default value? Can\u0027t be just None?","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"bffb2119ef1d99e36d22fa0eafaf4cbcf4a09f3e","unresolved":false,"context_lines":[{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    def delete_bandwidth_limit(self, port):"},{"line_number":65,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":66,"context_line":"        port \u003d self.ports[port_id].pop(constants.EGRESS_DIRECTION, 0)"},{"line_number":67,"context_line":"        if not port:"},{"line_number":68,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received for port %s but \""},{"line_number":69,"context_line":"                      \"vif_port was not found. It seems that port is already \""}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_9090a350","line":66,"in_reply_to":"3f4b6375_ab9c715d","updated":"2017-10-31 02:22:48.000000000","message":"dscp use 0. The code is consistent.","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f1c3a458fd4c1aa5a7583aa62383d284cff3fa9a","unresolved":false,"context_lines":[{"line_number":67,"context_line":"        if not port:"},{"line_number":68,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received for port %s but \""},{"line_number":69,"context_line":"                      \"vif_port was not found. It seems that port is already \""},{"line_number":70,"context_line":"                      \"deleted\", port_id)"},{"line_number":71,"context_line":"            return"},{"line_number":72,"context_line":"        vif_port \u003d port.get(\u0027vif_port\u0027)"},{"line_number":73,"context_line":"        self.br_int.delete_egress_bw_limit_for_port(vif_port.port_name)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_8ba1b598","line":70,"updated":"2017-10-30 08:22:40.000000000","message":"IMO this message should be changed now as it not means that port was deleted or not now.","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"bffb2119ef1d99e36d22fa0eafaf4cbcf4a09f3e","unresolved":false,"context_lines":[{"line_number":67,"context_line":"        if not port:"},{"line_number":68,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received for port %s but \""},{"line_number":69,"context_line":"                      \"vif_port was not found. It seems that port is already \""},{"line_number":70,"context_line":"                      \"deleted\", port_id)"},{"line_number":71,"context_line":"            return"},{"line_number":72,"context_line":"        vif_port \u003d port.get(\u0027vif_port\u0027)"},{"line_number":73,"context_line":"        self.br_int.delete_egress_bw_limit_for_port(vif_port.port_name)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_d0873b0f","line":70,"in_reply_to":"3f4b6375_8ba1b598","updated":"2017-10-31 02:22:48.000000000","message":"Done","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f1c3a458fd4c1aa5a7583aa62383d284cff3fa9a","unresolved":false,"context_lines":[{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def delete_bandwidth_limit_ingress(self, port):"},{"line_number":76,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":77,"context_line":"        port \u003d self.ports[port_id].pop(constants.INGRESS_DIRECTION, 0)"},{"line_number":78,"context_line":"        if not port:"},{"line_number":79,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":80,"context_line":"                      \"for port %s but vif_port was not found. \""}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_6b9e7955","line":77,"updated":"2017-10-30 08:22:40.000000000","message":"Why \"0\" as default value? Can\u0027t be just None?","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"bffb2119ef1d99e36d22fa0eafaf4cbcf4a09f3e","unresolved":false,"context_lines":[{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def delete_bandwidth_limit_ingress(self, port):"},{"line_number":76,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":77,"context_line":"        port \u003d self.ports[port_id].pop(constants.INGRESS_DIRECTION, 0)"},{"line_number":78,"context_line":"        if not port:"},{"line_number":79,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":80,"context_line":"                      \"for port %s but vif_port was not found. \""}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_b0cf1f56","line":77,"in_reply_to":"3f4b6375_6b9e7955","updated":"2017-10-31 02:22:48.000000000","message":"keep consistent keep pace with dscp","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f1c3a458fd4c1aa5a7583aa62383d284cff3fa9a","unresolved":false,"context_lines":[{"line_number":78,"context_line":"        if not port:"},{"line_number":79,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":80,"context_line":"                      \"for port %s but vif_port was not found. \""},{"line_number":81,"context_line":"                      \"It seems that port is already deleted\", port_id)"},{"line_number":82,"context_line":"            return"},{"line_number":83,"context_line":"        vif_port \u003d port.get(\u0027vif_port\u0027)"},{"line_number":84,"context_line":"        self.br_int.delete_ingress_bw_limit_for_port(vif_port.port_name)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_4babbd74","line":81,"updated":"2017-10-30 08:22:40.000000000","message":"This message should be changed also in same way as one above","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"bffb2119ef1d99e36d22fa0eafaf4cbcf4a09f3e","unresolved":false,"context_lines":[{"line_number":78,"context_line":"        if not port:"},{"line_number":79,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":80,"context_line":"                      \"for port %s but vif_port was not found. \""},{"line_number":81,"context_line":"                      \"It seems that port is already deleted\", port_id)"},{"line_number":82,"context_line":"            return"},{"line_number":83,"context_line":"        vif_port \u003d port.get(\u0027vif_port\u0027)"},{"line_number":84,"context_line":"        self.br_int.delete_ingress_bw_limit_for_port(vif_port.port_name)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_709d2765","line":81,"in_reply_to":"3f4b6375_4babbd74","updated":"2017-10-31 02:22:48.000000000","message":"Done","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":19853,"name":"Nguyen Hung Phuong","email":"phuongnh@vn.fujitsu.com","username":"phuongnh"},"change_message_id":"eabe4b6538d1f2a6c8d0a24089fc75b03f092fe7","unresolved":false,"context_lines":[{"line_number":66,"context_line":"        port \u003d self.ports[port_id].pop(constants.EGRESS_DIRECTION, 0)"},{"line_number":67,"context_line":"        if not port:"},{"line_number":68,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received for port %s but \""},{"line_number":69,"context_line":"                      \"vif_port was not found. It seems that bandwidth_limit is\""},{"line_number":70,"context_line":"                      \" already deleted\", port_id)"},{"line_number":71,"context_line":"            return"},{"line_number":72,"context_line":"        vif_port \u003d port.get(\u0027vif_port\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f4b6375_6fdc2546","line":69,"range":{"start_line":69,"start_character":71,"end_line":69,"end_character":80},"updated":"2017-10-31 06:17:49.000000000","message":"Line is \u003e 79 chars.","commit_id":"adb8f6516ad82fd401f03b4e190cf0385b4b2df2"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"69b76a1f2b7e78d49ae2dcd505af34b30052cc5a","unresolved":false,"context_lines":[{"line_number":66,"context_line":"        port \u003d self.ports[port_id].pop(constants.EGRESS_DIRECTION, 0)"},{"line_number":67,"context_line":"        if not port:"},{"line_number":68,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received for port %s but \""},{"line_number":69,"context_line":"                      \"vif_port was not found. It seems that bandwidth_limit is\""},{"line_number":70,"context_line":"                      \" already deleted\", port_id)"},{"line_number":71,"context_line":"            return"},{"line_number":72,"context_line":"        vif_port \u003d port.get(\u0027vif_port\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f4b6375_afdc5d18","line":69,"range":{"start_line":69,"start_character":71,"end_line":69,"end_character":80},"in_reply_to":"3f4b6375_6fdc2546","updated":"2017-10-31 06:32:25.000000000","message":"Done","commit_id":"adb8f6516ad82fd401f03b4e190cf0385b4b2df2"},{"author":{"_account_id":19853,"name":"Nguyen Hung Phuong","email":"phuongnh@vn.fujitsu.com","username":"phuongnh"},"change_message_id":"eabe4b6538d1f2a6c8d0a24089fc75b03f092fe7","unresolved":false,"context_lines":[{"line_number":78,"context_line":"        if not port:"},{"line_number":79,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":80,"context_line":"                      \"for port %s but vif_port was not found. \""},{"line_number":81,"context_line":"                      \"It seems that bandwidth_limit is already deleted\", port_id)"},{"line_number":82,"context_line":"            return"},{"line_number":83,"context_line":"        vif_port \u003d port.get(\u0027vif_port\u0027)"},{"line_number":84,"context_line":"        self.br_int.delete_ingress_bw_limit_for_port(vif_port.port_name)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f4b6375_afd21d33","line":81,"range":{"start_line":81,"start_character":68,"end_line":81,"end_character":82},"updated":"2017-10-31 06:17:49.000000000","message":"Line is \u003e 79 chars.","commit_id":"adb8f6516ad82fd401f03b4e190cf0385b4b2df2"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"69b76a1f2b7e78d49ae2dcd505af34b30052cc5a","unresolved":false,"context_lines":[{"line_number":78,"context_line":"        if not port:"},{"line_number":79,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":80,"context_line":"                      \"for port %s but vif_port was not found. \""},{"line_number":81,"context_line":"                      \"It seems that bandwidth_limit is already deleted\", port_id)"},{"line_number":82,"context_line":"            return"},{"line_number":83,"context_line":"        vif_port \u003d port.get(\u0027vif_port\u0027)"},{"line_number":84,"context_line":"        self.br_int.delete_ingress_bw_limit_for_port(vif_port.port_name)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f4b6375_8fdf610b","line":81,"range":{"start_line":81,"start_character":68,"end_line":81,"end_character":82},"in_reply_to":"3f4b6375_afd21d33","updated":"2017-10-31 06:32:25.000000000","message":"Done","commit_id":"adb8f6516ad82fd401f03b4e190cf0385b4b2df2"},{"author":{"_account_id":12860,"name":"Dongcan Ye","email":"yedongcan@yeah.net","username":"yedongcan"},"change_message_id":"a3b48dcd2a75fcd7ea74f23cafcc8cd8f202760f","unresolved":false,"context_lines":[{"line_number":65,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":66,"context_line":"        port \u003d self.ports[port_id].pop(constants.EGRESS_DIRECTION, 0)"},{"line_number":67,"context_line":"        if not port:"},{"line_number":68,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received \""},{"line_number":69,"context_line":"                      \"for port %s but vif_port was not found. \""},{"line_number":70,"context_line":"                      \"It seems that bandwidth_limit is already deleted\","},{"line_number":71,"context_line":"                      port_id)"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f4b6375_f26238d8","line":68,"range":{"start_line":68,"start_character":23,"end_line":68,"end_character":45},"updated":"2017-10-31 07:15:41.000000000","message":"How about modify this by: delete_bandwidth_limit_egress?","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"78d3bacd1fda6ec54e20ad055a27d7e220c97543","unresolved":false,"context_lines":[{"line_number":65,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":66,"context_line":"        port \u003d self.ports[port_id].pop(constants.EGRESS_DIRECTION, 0)"},{"line_number":67,"context_line":"        if not port:"},{"line_number":68,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received \""},{"line_number":69,"context_line":"                      \"for port %s but vif_port was not found. \""},{"line_number":70,"context_line":"                      \"It seems that bandwidth_limit is already deleted\","},{"line_number":71,"context_line":"                      port_id)"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f4b6375_72ff6867","line":68,"range":{"start_line":68,"start_character":23,"end_line":68,"end_character":45},"in_reply_to":"3f4b6375_f26238d8","updated":"2017-10-31 07:21:38.000000000","message":"\u003e How about modify this by: delete_bandwidth_limit_egress?\nwhy ??","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":12860,"name":"Dongcan Ye","email":"yedongcan@yeah.net","username":"yedongcan"},"change_message_id":"a3b48dcd2a75fcd7ea74f23cafcc8cd8f202760f","unresolved":false,"context_lines":[{"line_number":66,"context_line":"        port \u003d self.ports[port_id].pop(constants.EGRESS_DIRECTION, 0)"},{"line_number":67,"context_line":"        if not port:"},{"line_number":68,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received \""},{"line_number":69,"context_line":"                      \"for port %s but vif_port was not found. \""},{"line_number":70,"context_line":"                      \"It seems that bandwidth_limit is already deleted\","},{"line_number":71,"context_line":"                      port_id)"},{"line_number":72,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f4b6375_92eb0411","line":69,"range":{"start_line":69,"start_character":39,"end_line":69,"end_character":47},"updated":"2017-10-31 07:15:41.000000000","message":"port?","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"0bcdcdef59fe934d024382bee09af4e7d077b380","unresolved":false,"context_lines":[{"line_number":66,"context_line":"        port \u003d self.ports[port_id].pop(constants.EGRESS_DIRECTION, 0)"},{"line_number":67,"context_line":"        if not port:"},{"line_number":68,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received \""},{"line_number":69,"context_line":"                      \"for port %s but vif_port was not found. \""},{"line_number":70,"context_line":"                      \"It seems that bandwidth_limit is already deleted\","},{"line_number":71,"context_line":"                      port_id)"},{"line_number":72,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f4b6375_92330414","line":69,"range":{"start_line":69,"start_character":39,"end_line":69,"end_character":47},"in_reply_to":"3f4b6375_92eb0411","updated":"2017-10-31 07:33:48.000000000","message":"Done","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":12860,"name":"Dongcan Ye","email":"yedongcan@yeah.net","username":"yedongcan"},"change_message_id":"a3b48dcd2a75fcd7ea74f23cafcc8cd8f202760f","unresolved":false,"context_lines":[{"line_number":78,"context_line":"        port \u003d self.ports[port_id].pop(constants.INGRESS_DIRECTION, 0)"},{"line_number":79,"context_line":"        if not port:"},{"line_number":80,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":81,"context_line":"                      \"for port %s but vif_port was not found. \""},{"line_number":82,"context_line":"                      \"It seems that bandwidth_limit is already deleted\","},{"line_number":83,"context_line":"                      port_id)"},{"line_number":84,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f4b6375_32dc5073","line":81,"range":{"start_line":81,"start_character":39,"end_line":81,"end_character":47},"updated":"2017-10-31 07:15:41.000000000","message":"ditto","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"0bcdcdef59fe934d024382bee09af4e7d077b380","unresolved":false,"context_lines":[{"line_number":78,"context_line":"        port \u003d self.ports[port_id].pop(constants.INGRESS_DIRECTION, 0)"},{"line_number":79,"context_line":"        if not port:"},{"line_number":80,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":81,"context_line":"                      \"for port %s but vif_port was not found. \""},{"line_number":82,"context_line":"                      \"It seems that bandwidth_limit is already deleted\","},{"line_number":83,"context_line":"                      port_id)"},{"line_number":84,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":7,"id":"3f4b6375_f22d98a8","line":81,"range":{"start_line":81,"start_character":39,"end_line":81,"end_character":47},"in_reply_to":"3f4b6375_32dc5073","updated":"2017-10-31 07:33:48.000000000","message":"Done","commit_id":"6d4daecbd1d8db804b099239bf71290d4fbf4c55"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"907446293aaefb3ab711952a9cecdef2c4240a21","unresolved":false,"context_lines":[{"line_number":56,"context_line":"                      \"deleted\", port_id)"},{"line_number":57,"context_line":"            return"},{"line_number":58,"context_line":"        self.ports[port[\u0027port_id\u0027]][qos_consts.RULE_TYPE_BANDWIDTH_LIMIT] \u003d\\"},{"line_number":59,"context_line":"            {rule.direction: port}"},{"line_number":60,"context_line":"        if rule.direction \u003d\u003d constants.INGRESS_DIRECTION:"},{"line_number":61,"context_line":"            self._update_ingress_bandwidth_limit(vif_port, rule)"},{"line_number":62,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f4b6375_69861dbc","line":59,"updated":"2017-11-02 08:48:50.000000000","message":"You can move opening bracket to line above and then avoid this \"\\\"","commit_id":"a12853d9109086d2d417e0f3ffbe774b59f6d22b"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"907446293aaefb3ab711952a9cecdef2c4240a21","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    def delete_bandwidth_limit(self, port):"},{"line_number":66,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":67,"context_line":"        port \u003d None"},{"line_number":68,"context_line":"        bw_limit_type \u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT"},{"line_number":69,"context_line":"        if bw_limit_type in self.ports[port_id]:"},{"line_number":70,"context_line":"            port \u003d self.ports[port_id][bw_limit_type].\\"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f4b6375_498fe1cc","line":67,"updated":"2017-11-02 08:48:50.000000000","message":"it is confusing for me. Maybe You could use different variable name here or in method argument?","commit_id":"a12853d9109086d2d417e0f3ffbe774b59f6d22b"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"907446293aaefb3ab711952a9cecdef2c4240a21","unresolved":false,"context_lines":[{"line_number":67,"context_line":"        port \u003d None"},{"line_number":68,"context_line":"        bw_limit_type \u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT"},{"line_number":69,"context_line":"        if bw_limit_type in self.ports[port_id]:"},{"line_number":70,"context_line":"            port \u003d self.ports[port_id][bw_limit_type].\\"},{"line_number":71,"context_line":"                pop(constants.EGRESS_DIRECTION, 0)"},{"line_number":72,"context_line":"        if not port:"},{"line_number":73,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received \""}],"source_content_type":"text/x-python","patch_set":15,"id":"3f4b6375_298ca5d5","line":70,"updated":"2017-11-02 08:48:50.000000000","message":"You can use get() to get element from dict. Than You can avoid this \"if\" probably","commit_id":"a12853d9109086d2d417e0f3ffbe774b59f6d22b"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"907446293aaefb3ab711952a9cecdef2c4240a21","unresolved":false,"context_lines":[{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def delete_bandwidth_limit_ingress(self, port):"},{"line_number":82,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":83,"context_line":"        port \u003d None"},{"line_number":84,"context_line":"        bw_limit_type \u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT"},{"line_number":85,"context_line":"        if bw_limit_type in self.ports[port_id]:"},{"line_number":86,"context_line":"            port \u003d self.ports[port_id][bw_limit_type]\\"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f4b6375_09bfa961","line":83,"updated":"2017-11-02 08:48:50.000000000","message":"please check my comments in method above. Here are the same :)","commit_id":"a12853d9109086d2d417e0f3ffbe774b59f6d22b"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"907446293aaefb3ab711952a9cecdef2c4240a21","unresolved":false,"context_lines":[{"line_number":83,"context_line":"        port \u003d None"},{"line_number":84,"context_line":"        bw_limit_type \u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT"},{"line_number":85,"context_line":"        if bw_limit_type in self.ports[port_id]:"},{"line_number":86,"context_line":"            port \u003d self.ports[port_id][bw_limit_type]\\"},{"line_number":87,"context_line":"                .pop(constants.INGRESS_DIRECTION, 0)"},{"line_number":88,"context_line":"        if not port:"},{"line_number":89,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""}],"source_content_type":"text/x-python","patch_set":15,"id":"3f4b6375_e9c32dec","line":86,"updated":"2017-11-02 08:48:50.000000000","message":"nit: move \"pop(\" to line 86 and then You can get rid of \"\\\" (Brian for sure will don\u0027t like it like it is now :P)","commit_id":"a12853d9109086d2d417e0f3ffbe774b59f6d22b"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"166eb8dd7f7a6ac4de05cbdca45d78dafafc47a9","unresolved":false,"context_lines":[{"line_number":55,"context_line":"                      \"vif_port was not found. It seems that port is already \""},{"line_number":56,"context_line":"                      \"deleted\", port_id)"},{"line_number":57,"context_line":"            return"},{"line_number":58,"context_line":"        bw_limit_type \u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT"},{"line_number":59,"context_line":"        self.ports[port[\u0027port_id\u0027]][bw_limit_type] \u003d {rule.direction: port}"},{"line_number":60,"context_line":"        if rule.direction \u003d\u003d constants.INGRESS_DIRECTION:"},{"line_number":61,"context_line":"            self._update_ingress_bandwidth_limit(vif_port, rule)"}],"source_content_type":"text/x-python","patch_set":18,"id":"3f4b6375_25296189","line":58,"updated":"2017-11-02 12:34:51.000000000","message":"it looks better but IMHO this additional variable is not necessary","commit_id":"858130aedcc8eb93e1a3022d7f4e5f8a3ddfc750"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"166eb8dd7f7a6ac4de05cbdca45d78dafafc47a9","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    def delete_bandwidth_limit(self, port):"},{"line_number":66,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":67,"context_line":"        bw_port \u003d None"},{"line_number":68,"context_line":"        bw_limit_type \u003d qos_consts.RULE_TYPE_BANDWIDTH_LIMIT"},{"line_number":69,"context_line":"        bw_limit \u003d self.ports[port_id].get(bw_limit_type, 0)"},{"line_number":70,"context_line":"        if bw_limit:"}],"source_content_type":"text/x-python","patch_set":18,"id":"3f4b6375_052e6591","line":67,"updated":"2017-11-02 12:34:51.000000000","message":"I would propose something like \"port_details\" or \"port_info\" instead of bw_port","commit_id":"858130aedcc8eb93e1a3022d7f4e5f8a3ddfc750"},{"author":{"_account_id":18051,"name":"David Shaughnessy","email":"david.shaughnessy@intel.com","username":"davidsha"},"change_message_id":"6cc489d92ffbd3ed5473384f90d8556519f373af","unresolved":false,"context_lines":[{"line_number":65,"context_line":"    def delete_bandwidth_limit(self, port):"},{"line_number":66,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":67,"context_line":"        port \u003d self.ports[port_id].pop((qos_consts.RULE_TYPE_BANDWIDTH_LIMIT,"},{"line_number":68,"context_line":"                                        constants.EGRESS_DIRECTION),"},{"line_number":69,"context_line":"                                       0)"},{"line_number":70,"context_line":"        if not port:"},{"line_number":71,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received \""},{"line_number":72,"context_line":"                      \"for port %s but port was not found. \""}],"source_content_type":"text/x-python","patch_set":22,"id":"3f4b6375_98edb726","line":69,"range":{"start_line":68,"start_character":68,"end_line":69,"end_character":41},"updated":"2017-11-07 16:34:11.000000000","message":"nit: this could be included on the previous line, also None may be better than 0, 0 implies we may be expecting a number to return, not a dictionary.","commit_id":"549a7a54f591a972394f2f3480ecaf8abec89ca0"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"00933bc18109522dedd9791dede9708e6d1ab349","unresolved":false,"context_lines":[{"line_number":65,"context_line":"    def delete_bandwidth_limit(self, port):"},{"line_number":66,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":67,"context_line":"        port \u003d self.ports[port_id].pop((qos_consts.RULE_TYPE_BANDWIDTH_LIMIT,"},{"line_number":68,"context_line":"                                        constants.EGRESS_DIRECTION),"},{"line_number":69,"context_line":"                                       0)"},{"line_number":70,"context_line":"        if not port:"},{"line_number":71,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received \""},{"line_number":72,"context_line":"                      \"for port %s but port was not found. \""}],"source_content_type":"text/x-python","patch_set":22,"id":"1f485f77_12d4a0bc","line":69,"range":{"start_line":68,"start_character":68,"end_line":69,"end_character":41},"in_reply_to":"1f485f77_28929a9f","updated":"2017-11-14 08:27:12.000000000","message":"Done","commit_id":"549a7a54f591a972394f2f3480ecaf8abec89ca0"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"1e87e525f81fdad8e0ee87a72574f5832be2759f","unresolved":false,"context_lines":[{"line_number":65,"context_line":"    def delete_bandwidth_limit(self, port):"},{"line_number":66,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":67,"context_line":"        port \u003d self.ports[port_id].pop((qos_consts.RULE_TYPE_BANDWIDTH_LIMIT,"},{"line_number":68,"context_line":"                                        constants.EGRESS_DIRECTION),"},{"line_number":69,"context_line":"                                       0)"},{"line_number":70,"context_line":"        if not port:"},{"line_number":71,"context_line":"            LOG.debug(\"delete_bandwidth_limit was received \""},{"line_number":72,"context_line":"                      \"for port %s but port was not found. \""}],"source_content_type":"text/x-python","patch_set":22,"id":"1f485f77_28929a9f","line":69,"range":{"start_line":68,"start_character":68,"end_line":69,"end_character":41},"in_reply_to":"3f4b6375_98edb726","updated":"2017-11-13 21:59:09.000000000","message":"Agreed.","commit_id":"549a7a54f591a972394f2f3480ecaf8abec89ca0"},{"author":{"_account_id":18051,"name":"David Shaughnessy","email":"david.shaughnessy@intel.com","username":"davidsha"},"change_message_id":"6cc489d92ffbd3ed5473384f90d8556519f373af","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    def delete_bandwidth_limit_ingress(self, port):"},{"line_number":80,"context_line":"        port_id \u003d port.get(\u0027port_id\u0027)"},{"line_number":81,"context_line":"        port \u003d self.ports[port_id].pop((qos_consts.RULE_TYPE_BANDWIDTH_LIMIT,"},{"line_number":82,"context_line":"                                        constants.INGRESS_DIRECTION),"},{"line_number":83,"context_line":"                                       0)"},{"line_number":84,"context_line":"        if not port:"},{"line_number":85,"context_line":"            LOG.debug(\"delete_bandwidth_limit_ingress was received \""},{"line_number":86,"context_line":"                      \"for port %s but port was not found. \""}],"source_content_type":"text/x-python","patch_set":22,"id":"3f4b6375_18c2e785","line":83,"range":{"start_line":82,"start_character":69,"end_line":83,"end_character":41},"updated":"2017-11-07 16:34:11.000000000","message":"ditto","commit_id":"549a7a54f591a972394f2f3480ecaf8abec89ca0"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f77a2aa7ba715d3a12ca940931728a01e3698a4a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"ff82abbf_c3115654","updated":"2017-11-24 21:08:31.000000000","message":"@Miguel: Why? IMO all those changes are necessary to fix this bug and are described in commit message also","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"8221c7317779f13624e8cf5397506f1f34b0f148","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"ff82abbf_e7aabb83","updated":"2017-11-21 22:39:45.000000000","message":"Shouldn\u0027t the changes in this file be part of a different patchset?","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"}],"neutron/tests/fullstack/test_qos.py":[{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"60dd8ed57f82fdb054dafdbdcfcc0d5d19dd7628","unresolved":false,"context_lines":[{"line_number":171,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"        # Delete port with qos policy attached"},{"line_number":174,"context_line":"        self.client.update_bandwidth_limit_rule("},{"line_number":175,"context_line":"            new_rule[\u0027id\u0027], qos_policy_id,"},{"line_number":176,"context_line":"            body\u003d{\u0027bandwidth_limit_rule\u0027: {\u0027max_kbps\u0027: BANDWIDTH_LIMIT,"},{"line_number":177,"context_line":"                                           \u0027max_burst_kbps\u0027: BANDWIDTH_BURST}})"}],"source_content_type":"text/x-python","patch_set":27,"id":"ff82abbf_0cf8587a","line":174,"updated":"2017-11-30 15:46:18.000000000","message":"it will not work this way, You should here call update_port method again (like in L168) but with proper qos policy id.\nNow You are just updating qos bw limit rule again but it\u0027s still not applied on port and that\u0027s why tests are failing.","commit_id":"35fc25713a1216c66bd184705c7ca6e61644809a"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"7790317e6273e6a625051fb02b3b5c81611d7f29","unresolved":false,"context_lines":[{"line_number":180,"context_line":"            vm, BANDWIDTH_LIMIT, BANDWIDTH_BURST, self.direction)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"        # Delete port with qos policy attached"},{"line_number":183,"context_line":"        self.client.delete_port(vm.neutron_port[\u0027id\u0027])"},{"line_number":184,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def test_bw_limit_direction_change(self):"}],"source_content_type":"text/x-python","patch_set":28,"id":"ff82abbf_ff710d0e","line":183,"updated":"2017-11-30 22:15:29.000000000","message":"I made little bit more digging into it and it looks that calling delete_port is not enough because linuxbridge agent don\u0027t remove then tap interface from host and is not cleaning at all.\nI\u0027m not sure if this is proper behavior of LB agent or it should be treated as a bug and that should be tested.\nBut as this patch is only about OVS agent maybe we could create this as separate test case and for now define it only for ovs agent?\nI will check LB agent behavior and will fix fullstack test/or LB agent also.\n@Ihar: what You think about doing this as \"ovs only\" for now (with todo note maybe)?","commit_id":"16388c85acefe084f9d2581a81a6151590c77dba"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"06f9e47324396a1310d2792caa266b0b974990a0","unresolved":false,"context_lines":[{"line_number":180,"context_line":"            vm, BANDWIDTH_LIMIT, BANDWIDTH_BURST, self.direction)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"        # Delete port with qos policy attached"},{"line_number":183,"context_line":"        self.client.delete_port(vm.neutron_port[\u0027id\u0027])"},{"line_number":184,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def test_bw_limit_direction_change(self):"}],"source_content_type":"text/x-python","patch_set":28,"id":"df87a7cf_72d0d8ca","line":183,"in_reply_to":"ff82abbf_7e6bb51e","updated":"2017-12-02 05:52:10.000000000","message":"thanks for your help","commit_id":"16388c85acefe084f9d2581a81a6151590c77dba"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"af10adc44abfd71c7dbca03f716162a096c70d6c","unresolved":false,"context_lines":[{"line_number":180,"context_line":"            vm, BANDWIDTH_LIMIT, BANDWIDTH_BURST, self.direction)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"        # Delete port with qos policy attached"},{"line_number":183,"context_line":"        self.client.delete_port(vm.neutron_port[\u0027id\u0027])"},{"line_number":184,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def test_bw_limit_direction_change(self):"}],"source_content_type":"text/x-python","patch_set":28,"id":"ff82abbf_7e6bb51e","line":183,"in_reply_to":"ff82abbf_ff710d0e","updated":"2017-12-01 08:16:03.000000000","message":"So we talked with Ihar on IRC yesterday and it looks that this test should be done as separate test only for Openvswitch agent.\nFor Linuxbridge it\u0027s not necessary because in such case rate limit is configured directly on tap device so if Nova will remove tap device, then rate limit is for sure deleted also.","commit_id":"16388c85acefe084f9d2581a81a6151590c77dba"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"146eec016f8d7a2b9dbacdcfdce91adfc606c206","unresolved":false,"context_lines":[{"line_number":221,"context_line":""},{"line_number":222,"context_line":"    def test_bw_limit_qos_port_removed(self):"},{"line_number":223,"context_line":"        \"\"\"Test if  rate limit config is properly removed when whole port is"},{"line_number":224,"context_line":"        removed\"\"\""},{"line_number":225,"context_line":""},{"line_number":226,"context_line":"        # Create port with qos policy attached"},{"line_number":227,"context_line":"        vm, qos_policy \u003d self._prepare_vm_with_qos_policy("}],"source_content_type":"text/x-python","patch_set":29,"id":"df87a7cf_f8f4c25d","line":224,"range":{"start_line":224,"start_character":15,"end_line":224,"end_character":18},"updated":"2017-12-02 22:00:25.000000000","message":"this should be moved to new line to make pep8 happy :)","commit_id":"8da4b65ada53b97978740514e18ca1fa6f62df6c"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"ff85e0f7f1ea83a54798c86ec888162e56196cd4","unresolved":false,"context_lines":[{"line_number":221,"context_line":""},{"line_number":222,"context_line":"    def test_bw_limit_qos_port_removed(self):"},{"line_number":223,"context_line":"        \"\"\"Test if  rate limit config is properly removed when whole port is"},{"line_number":224,"context_line":"        removed\"\"\""},{"line_number":225,"context_line":""},{"line_number":226,"context_line":"        # Create port with qos policy attached"},{"line_number":227,"context_line":"        vm, qos_policy \u003d self._prepare_vm_with_qos_policy("}],"source_content_type":"text/x-python","patch_set":29,"id":"df87a7cf_1de2afd6","line":224,"range":{"start_line":224,"start_character":15,"end_line":224,"end_character":18},"in_reply_to":"df87a7cf_f8f4c25d","updated":"2017-12-04 01:49:18.000000000","message":"Done","commit_id":"8da4b65ada53b97978740514e18ca1fa6f62df6c"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"a15d6d6516aaddb29bb637b0f78fd6c8ebbf6526","unresolved":false,"context_lines":[{"line_number":232,"context_line":"            vm, BANDWIDTH_LIMIT, BANDWIDTH_BURST, self.direction)"},{"line_number":233,"context_line":""},{"line_number":234,"context_line":"        # Delete port with qos policy attached"},{"line_number":235,"context_line":"        self.client.delete_port(vm.neutron_port[\u0027id\u0027])"},{"line_number":236,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":237,"context_line":""},{"line_number":238,"context_line":""}],"source_content_type":"text/x-python","patch_set":29,"id":"df87a7cf_e30178b1","line":235,"updated":"2017-12-02 12:00:28.000000000","message":"I\u0027m not sure if You shouldn\u0027t call vm.destroy() here but maybe that will be enough","commit_id":"8da4b65ada53b97978740514e18ca1fa6f62df6c"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"2da9656843917b0bb3d6bb71f961517712cdbc5c","unresolved":false,"context_lines":[{"line_number":232,"context_line":"            vm, BANDWIDTH_LIMIT, BANDWIDTH_BURST, self.direction)"},{"line_number":233,"context_line":""},{"line_number":234,"context_line":"        # Delete port with qos policy attached"},{"line_number":235,"context_line":"        self.client.delete_port(vm.neutron_port[\u0027id\u0027])"},{"line_number":236,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":237,"context_line":""},{"line_number":238,"context_line":""}],"source_content_type":"text/x-python","patch_set":29,"id":"df87a7cf_f87da2e7","line":235,"in_reply_to":"df87a7cf_e30178b1","updated":"2017-12-02 21:43:29.000000000","message":"Ok, I made small test and I run this test against master branch without Your patch. Test passes which is wrong.\nSo I changed this line to call \"vm.destroy()\" instead of delete_port() and then test failed on master branch.\nSo it looks that You have to change it to \"vm.destroy()\" to simulate test case which You are trying to fix with this patch.","commit_id":"8da4b65ada53b97978740514e18ca1fa6f62df6c"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"ff85e0f7f1ea83a54798c86ec888162e56196cd4","unresolved":false,"context_lines":[{"line_number":232,"context_line":"            vm, BANDWIDTH_LIMIT, BANDWIDTH_BURST, self.direction)"},{"line_number":233,"context_line":""},{"line_number":234,"context_line":"        # Delete port with qos policy attached"},{"line_number":235,"context_line":"        self.client.delete_port(vm.neutron_port[\u0027id\u0027])"},{"line_number":236,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":237,"context_line":""},{"line_number":238,"context_line":""}],"source_content_type":"text/x-python","patch_set":29,"id":"df87a7cf_bde623e8","line":235,"in_reply_to":"df87a7cf_f87da2e7","updated":"2017-12-04 01:49:18.000000000","message":"Done","commit_id":"8da4b65ada53b97978740514e18ca1fa6f62df6c"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"37ac06f28f26355f13276819699b62b2dddbbf97","unresolved":false,"context_lines":[{"line_number":220,"context_line":"                    vm.port.name) \u003d\u003d (limit, burst))"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"    def test_bw_limit_qos_port_removed(self):"},{"line_number":223,"context_line":"        \"\"\"Test if  rate limit config is properly removed when whole port is"},{"line_number":224,"context_line":"        removed"},{"line_number":225,"context_line":"        \"\"\""},{"line_number":226,"context_line":""}],"source_content_type":"text/x-python","patch_set":30,"id":"df87a7cf_a88804c6","line":223,"range":{"start_line":223,"start_character":19,"end_line":223,"end_character":21},"updated":"2017-12-04 23:01:24.000000000","message":"two spaces!!! :)","commit_id":"4adac14db4e2ef53db287c57511295b2bfceae3d"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"3d1b2abe3a036fd93c76496111df0e87feb3de9b","unresolved":false,"context_lines":[{"line_number":220,"context_line":"                    vm.port.name) \u003d\u003d (limit, burst))"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"    def test_bw_limit_qos_port_removed(self):"},{"line_number":223,"context_line":"        \"\"\"Test if  rate limit config is properly removed when whole port is"},{"line_number":224,"context_line":"        removed"},{"line_number":225,"context_line":"        \"\"\""},{"line_number":226,"context_line":""}],"source_content_type":"text/x-python","patch_set":30,"id":"df87a7cf_1963841c","line":223,"range":{"start_line":223,"start_character":19,"end_line":223,"end_character":21},"in_reply_to":"df87a7cf_a88804c6","updated":"2017-12-05 01:45:21.000000000","message":"I will modify it","commit_id":"4adac14db4e2ef53db287c57511295b2bfceae3d"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"37ac06f28f26355f13276819699b62b2dddbbf97","unresolved":false,"context_lines":[{"line_number":221,"context_line":""},{"line_number":222,"context_line":"    def test_bw_limit_qos_port_removed(self):"},{"line_number":223,"context_line":"        \"\"\"Test if  rate limit config is properly removed when whole port is"},{"line_number":224,"context_line":"        removed"},{"line_number":225,"context_line":"        \"\"\""},{"line_number":226,"context_line":""},{"line_number":227,"context_line":"        # Create port with qos policy attached"}],"source_content_type":"text/x-python","patch_set":30,"id":"df87a7cf_888f40cb","line":224,"range":{"start_line":224,"start_character":14,"end_line":224,"end_character":15},"updated":"2017-12-04 23:01:24.000000000","message":"no trailing dot!!!","commit_id":"4adac14db4e2ef53db287c57511295b2bfceae3d"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"3d1b2abe3a036fd93c76496111df0e87feb3de9b","unresolved":false,"context_lines":[{"line_number":221,"context_line":""},{"line_number":222,"context_line":"    def test_bw_limit_qos_port_removed(self):"},{"line_number":223,"context_line":"        \"\"\"Test if  rate limit config is properly removed when whole port is"},{"line_number":224,"context_line":"        removed"},{"line_number":225,"context_line":"        \"\"\""},{"line_number":226,"context_line":""},{"line_number":227,"context_line":"        # Create port with qos policy attached"}],"source_content_type":"text/x-python","patch_set":30,"id":"df87a7cf_9976b4d8","line":224,"range":{"start_line":224,"start_character":14,"end_line":224,"end_character":15},"in_reply_to":"df87a7cf_888f40cb","updated":"2017-12-05 01:45:21.000000000","message":"I will modify it","commit_id":"4adac14db4e2ef53db287c57511295b2bfceae3d"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"37ac06f28f26355f13276819699b62b2dddbbf97","unresolved":false,"context_lines":[{"line_number":234,"context_line":""},{"line_number":235,"context_line":"        # Delete port with qos policy attached"},{"line_number":236,"context_line":"        vm.destroy()"},{"line_number":237,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":238,"context_line":""},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"class TestBwLimitQoSLinuxbridge(_TestBwLimitQoS, base.BaseFullStackTestCase):"}],"source_content_type":"text/x-python","patch_set":30,"id":"df87a7cf_e8136c65","line":237,"updated":"2017-12-04 23:01:24.000000000","message":"for what I see in the code, this will only check that get_ingress_bw_limit_for_port returns None, None, but it won\u0027t check that the queue is gone. (The queue may be present but max-rate and burst are None.) So it doesn\u0027t seem like the test case covers what\u0027s described in the bug. Or am I missing something here?","commit_id":"4adac14db4e2ef53db287c57511295b2bfceae3d"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"eefb67f506a74ddd87e98a16e8d9bcf015aa4025","unresolved":false,"context_lines":[{"line_number":234,"context_line":""},{"line_number":235,"context_line":"        # Delete port with qos policy attached"},{"line_number":236,"context_line":"        vm.destroy()"},{"line_number":237,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":238,"context_line":""},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"class TestBwLimitQoSLinuxbridge(_TestBwLimitQoS, base.BaseFullStackTestCase):"}],"source_content_type":"text/x-python","patch_set":30,"id":"df87a7cf_13035c11","line":237,"in_reply_to":"df87a7cf_59ab4c1c","updated":"2017-12-05 08:30:14.000000000","message":"I see Ihar\u0027s point here. Method \"get_ingress_bw_limit_for_port\" will return true also in case if e.g. queue will still exists but will not have max-burst and burst values in it (it can be e.g. empty).\nI think that You should add one small check here and assert that _find_queue method didn\u0027t found queue.","commit_id":"4adac14db4e2ef53db287c57511295b2bfceae3d"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"3d1b2abe3a036fd93c76496111df0e87feb3de9b","unresolved":false,"context_lines":[{"line_number":234,"context_line":""},{"line_number":235,"context_line":"        # Delete port with qos policy attached"},{"line_number":236,"context_line":"        vm.destroy()"},{"line_number":237,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":238,"context_line":""},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"class TestBwLimitQoSLinuxbridge(_TestBwLimitQoS, base.BaseFullStackTestCase):"}],"source_content_type":"text/x-python","patch_set":30,"id":"df87a7cf_59ab4c1c","line":237,"in_reply_to":"df87a7cf_e8136c65","updated":"2017-12-05 01:45:21.000000000","message":"the function get_ingress_bw_limit_for_port will call self._find_queue.\nself._find_queue will return queue\u0027s other_config(max-rate and burst).\nIt passed the indirect test.","commit_id":"4adac14db4e2ef53db287c57511295b2bfceae3d"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"3dd09cc182396e08dab75200d692245ce0c8babb","unresolved":false,"context_lines":[{"line_number":236,"context_line":"        vm.destroy()"},{"line_number":237,"context_line":"        self._wait_for_bw_rule_removed(vm, self.direction)"},{"line_number":238,"context_line":"        self.assertIsNone(vm.bridge.find_qos(vm.port.name))"},{"line_number":239,"context_line":"        self.assertIsNone(vm.bridge.find_queue(vm.port.name))"},{"line_number":240,"context_line":""},{"line_number":241,"context_line":""},{"line_number":242,"context_line":"class TestBwLimitQoSLinuxbridge(_TestBwLimitQoS, base.BaseFullStackTestCase):"}],"source_content_type":"text/x-python","patch_set":32,"id":"df87a7cf_3813d85e","line":239,"updated":"2017-12-05 10:02:47.000000000","message":"find_queue should have 2 parameters given so You should use ovs_lib.QOS_DEFAULT_QUEUE as second one","commit_id":"808f2cf9d3a4b34282de87bcf366d867918be77f"}],"neutron/tests/unit/agent/common/test_ovs_lib.py":[{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"1e87e525f81fdad8e0ee87a72574f5832be2759f","unresolved":false,"context_lines":[{"line_number":813,"context_line":"            port_exists_mock.assert_called_once_with(\"test_port\")"},{"line_number":814,"context_line":"            set_egress_mock.assert_not_called()"},{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    def test_delete_ingress_bw_limit_for_port(self):"},{"line_number":817,"context_line":"        with mock.patch.object("},{"line_number":818,"context_line":"            self.br, \"_delete_ingress_bw_limit_for_port\""},{"line_number":819,"context_line":"        ) as delete_ingress_mock, mock.patch.object("}],"source_content_type":"text/x-python","patch_set":22,"id":"1f485f77_68dd326f","side":"PARENT","line":816,"updated":"2017-11-13 21:59:09.000000000","message":"why do we remove those tests instead of changing them to reflect new behavior?","commit_id":"5fc8e47786c91f76d253010b194bd5637de895b8"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"eb0eaaabccd138bd38ce53b494db435a5392bc03","unresolved":false,"context_lines":[{"line_number":813,"context_line":"            port_exists_mock.assert_called_once_with(\"test_port\")"},{"line_number":814,"context_line":"            set_egress_mock.assert_not_called()"},{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    def test_delete_ingress_bw_limit_for_port(self):"},{"line_number":817,"context_line":"        with mock.patch.object("},{"line_number":818,"context_line":"            self.br, \"_delete_ingress_bw_limit_for_port\""},{"line_number":819,"context_line":"        ) as delete_ingress_mock, mock.patch.object("}],"source_content_type":"text/x-python","patch_set":22,"id":"ff82abbf_eed0f321","side":"PARENT","line":816,"in_reply_to":"1f485f77_68dd326f","updated":"2017-11-27 07:28:00.000000000","message":"how to modify in fullstack ? Change the neutron/test/fullstack/test_qos.py ?","commit_id":"5fc8e47786c91f76d253010b194bd5637de895b8"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"8c01c0a95161ea2a4cf60b210f822472c8ee1810","unresolved":false,"context_lines":[{"line_number":813,"context_line":"            port_exists_mock.assert_called_once_with(\"test_port\")"},{"line_number":814,"context_line":"            set_egress_mock.assert_not_called()"},{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    def test_delete_ingress_bw_limit_for_port(self):"},{"line_number":817,"context_line":"        with mock.patch.object("},{"line_number":818,"context_line":"            self.br, \"_delete_ingress_bw_limit_for_port\""},{"line_number":819,"context_line":"        ) as delete_ingress_mock, mock.patch.object("}],"source_content_type":"text/x-python","patch_set":22,"id":"1f485f77_c379a993","side":"PARENT","line":816,"in_reply_to":"1f485f77_68dd326f","updated":"2017-11-13 22:33:46.000000000","message":"some time ago I was fixing another bug related to router ports and I added then this helper method _delete_ingress_bw_limit_for_port() which was called only if port really exists.\nNow it is done differently and there is no this helper method again and IMO those tests are not necessary.\nBut I agree with You that we should cover this case with fullstack test.","commit_id":"5fc8e47786c91f76d253010b194bd5637de895b8"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"8100571944924fe4cf27991d9aa15118dea267d5","unresolved":false,"context_lines":[{"line_number":813,"context_line":"            port_exists_mock.assert_called_once_with(\"test_port\")"},{"line_number":814,"context_line":"            set_egress_mock.assert_not_called()"},{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    def test_delete_ingress_bw_limit_for_port(self):"},{"line_number":817,"context_line":"        with mock.patch.object("},{"line_number":818,"context_line":"            self.br, \"_delete_ingress_bw_limit_for_port\""},{"line_number":819,"context_line":"        ) as delete_ingress_mock, mock.patch.object("}],"source_content_type":"text/x-python","patch_set":22,"id":"1f485f77_5c28ea32","side":"PARENT","line":816,"in_reply_to":"1f485f77_92f89049","updated":"2017-11-14 10:01:58.000000000","message":"about what test_ingress_bw_limit You are talking? In functional tests? or in fullstack?","commit_id":"5fc8e47786c91f76d253010b194bd5637de895b8"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"00933bc18109522dedd9791dede9708e6d1ab349","unresolved":false,"context_lines":[{"line_number":813,"context_line":"            port_exists_mock.assert_called_once_with(\"test_port\")"},{"line_number":814,"context_line":"            set_egress_mock.assert_not_called()"},{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    def test_delete_ingress_bw_limit_for_port(self):"},{"line_number":817,"context_line":"        with mock.patch.object("},{"line_number":818,"context_line":"            self.br, \"_delete_ingress_bw_limit_for_port\""},{"line_number":819,"context_line":"        ) as delete_ingress_mock, mock.patch.object("}],"source_content_type":"text/x-python","patch_set":22,"id":"1f485f77_92f89049","side":"PARENT","line":816,"in_reply_to":"1f485f77_c379a993","updated":"2017-11-14 08:27:12.000000000","message":"I think the function test_ingress_bw_limit\ncan test create/update/delete","commit_id":"5fc8e47786c91f76d253010b194bd5637de895b8"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"2ca19de4103f10fd54ced1181ffe7956987c5693","unresolved":false,"context_lines":[{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    def test_delete_ingress_bw_limit_for_port(self):"},{"line_number":817,"context_line":"        self.br.delete_ingress_bw_limit_for_port(\"test_port\")"},{"line_number":818,"context_line":"        max_rate, burst \u003d self.br.get_ingress_bw_limit_for_port(\"test_port\")"},{"line_number":819,"context_line":"        self.assertIsNone(max_rate)"},{"line_number":820,"context_line":"        self.assertIsNone(burst)"},{"line_number":821,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"ff82abbf_48b98176","line":818,"updated":"2017-11-29 22:31:20.000000000","message":"what\u0027s the goal of calling the getter here? if you assume that line 817 changes anything in ovsdb, then you should understand it\u0027s a unit test, so ovsdb is not talked to (execute is mocked out).\n\nI think if you just test the delete call, then getter call is not needed.","commit_id":"dae655e99ec49d835274fe3aa505b60a7bc6ce28"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"10342fca8c79a1e7757281260468ef05723a1534","unresolved":false,"context_lines":[{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    def test_delete_ingress_bw_limit_for_port(self):"},{"line_number":817,"context_line":"        self.br.delete_ingress_bw_limit_for_port(\"test_port\")"},{"line_number":818,"context_line":"        max_rate, burst \u003d self.br.get_ingress_bw_limit_for_port(\"test_port\")"},{"line_number":819,"context_line":"        self.assertIsNone(max_rate)"},{"line_number":820,"context_line":"        self.assertIsNone(burst)"},{"line_number":821,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"ff82abbf_c0755d65","line":818,"in_reply_to":"ff82abbf_48b98176","updated":"2017-11-30 08:21:00.000000000","message":"agree\nI don\u0027t know where fullstack/funtional test need to be changed. Do we need to create a real tap, and set\nqos and queue ?\n\nin neutron\\tests\\fullstack\\test_qos.py:test_bw_limit_qos_policy_rule_lifecycle\nthe function self.client.delete_bandwidth_limit_rule(bw_rule[\u0027id\u0027], qos_policy_id) will delete ingress rule,\nthen, self._wait_for_bw_rule_removed(vm, self.direction) will check wheater delete qos and queue or not.\n\nin neutron\\tests\\functional\\agent\\l2\\extensions\\test_ovs-agent_qos_extensions.py:test_policy_rule_delete\nthe function wait_until_bandwidth_limit_rule_applied also check the result of qos and queue.\n\n\nin patch https://review.openstack.org/#/c/519573/\nI think that the function get_ingress_bw_limit_for_port should check qos and queue at the same time.","commit_id":"dae655e99ec49d835274fe3aa505b60a7bc6ce28"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"2ca19de4103f10fd54ced1181ffe7956987c5693","unresolved":false,"context_lines":[{"line_number":821,"context_line":""},{"line_number":822,"context_line":"    def test_delete_ingress_bw_limit_for_port_port_not_exists(self):"},{"line_number":823,"context_line":"        self.br.delete_ingress_bw_limit_for_port(\"test_port\")"},{"line_number":824,"context_line":"        max_rate, burst \u003d self.br.get_ingress_bw_limit_for_port(\"test_port\")"},{"line_number":825,"context_line":"        self.assertIsNone(max_rate)"},{"line_number":826,"context_line":"        self.assertIsNone(burst)"},{"line_number":827,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"ff82abbf_286bddf0","line":824,"updated":"2017-11-29 22:31:20.000000000","message":"same comments. also, the test case is now exactly the same as the one above. I think you should make them different. in the first case, you would mock port_exists so that it returns True; in the second case, you would return False.","commit_id":"dae655e99ec49d835274fe3aa505b60a7bc6ce28"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"10342fca8c79a1e7757281260468ef05723a1534","unresolved":false,"context_lines":[{"line_number":821,"context_line":""},{"line_number":822,"context_line":"    def test_delete_ingress_bw_limit_for_port_port_not_exists(self):"},{"line_number":823,"context_line":"        self.br.delete_ingress_bw_limit_for_port(\"test_port\")"},{"line_number":824,"context_line":"        max_rate, burst \u003d self.br.get_ingress_bw_limit_for_port(\"test_port\")"},{"line_number":825,"context_line":"        self.assertIsNone(max_rate)"},{"line_number":826,"context_line":"        self.assertIsNone(burst)"},{"line_number":827,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"ff82abbf_e070a174","line":824,"in_reply_to":"ff82abbf_286bddf0","updated":"2017-11-30 08:21:00.000000000","message":"agree","commit_id":"dae655e99ec49d835274fe3aa505b60a7bc6ce28"}],"neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/extension_drivers/test_qos_driver.py":[{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f1c3a458fd4c1aa5a7583aa62383d284cff3fa9a","unresolved":false,"context_lines":[{"line_number":147,"context_line":"            return_value\u003d(self.rules[1].max_kbps,"},{"line_number":148,"context_line":"                          self.rules[1].max_burst_kbps))"},{"line_number":149,"context_line":"        self.qos_driver.delete(self.port)"},{"line_number":150,"context_line":"        if self.port_name in self.qos_driver.ports:"},{"line_number":151,"context_line":"            self.delete_egress.assert_called_once_with(self.port_name)"},{"line_number":152,"context_line":"            self.delete_ingress.assert_called_once_with(self.port_name)"},{"line_number":153,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_eb92694b","line":150,"updated":"2017-10-30 08:22:40.000000000","message":"This will never be True currently as self.port_name is \"fakeport\" and in self.qos_driver.ports You have port_id as key\nYou should change those tests to cover the case when Your port_id is not in qos_driver.ports dict","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"bffb2119ef1d99e36d22fa0eafaf4cbcf4a09f3e","unresolved":false,"context_lines":[{"line_number":147,"context_line":"            return_value\u003d(self.rules[1].max_kbps,"},{"line_number":148,"context_line":"                          self.rules[1].max_burst_kbps))"},{"line_number":149,"context_line":"        self.qos_driver.delete(self.port)"},{"line_number":150,"context_line":"        if self.port_name in self.qos_driver.ports:"},{"line_number":151,"context_line":"            self.delete_egress.assert_called_once_with(self.port_name)"},{"line_number":152,"context_line":"            self.delete_ingress.assert_called_once_with(self.port_name)"},{"line_number":153,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f4b6375_10cb931e","line":150,"in_reply_to":"3f4b6375_eb92694b","updated":"2017-10-31 02:22:48.000000000","message":"Done","commit_id":"0fc1896a73f68442278ac53c5ddbabd0dfd5fd56"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"1659ea176375369de88ef311a2455f434ada856b","unresolved":false,"context_lines":[{"line_number":148,"context_line":"                          self.rules[1].max_burst_kbps))"},{"line_number":149,"context_line":"        self.qos_driver.delete(self.port)"},{"line_number":150,"context_line":"        self.delete_egress.assert_not_called()"},{"line_number":151,"context_line":"        self.delete_ingress.assert_not_called()"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"    def test_delete_rules(self):"},{"line_number":154,"context_line":"        self._test_delete_rules(self.qos_policy)"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f4b6375_15cb7e4a","line":151,"updated":"2017-10-31 08:01:37.000000000","message":"You should add some test to cover this \"new\" case IMO","commit_id":"d63d987fca5fd3618417604e4451029ee6dd08d2"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"b3e837bd8c02468b1b190e93d68360adf30f695d","unresolved":false,"context_lines":[{"line_number":148,"context_line":"                          self.rules[1].max_burst_kbps))"},{"line_number":149,"context_line":"        self.qos_driver.delete(self.port)"},{"line_number":150,"context_line":"        self.delete_egress.assert_not_called()"},{"line_number":151,"context_line":"        self.delete_ingress.assert_not_called()"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"    def test_delete_rules(self):"},{"line_number":154,"context_line":"        self._test_delete_rules(self.qos_policy)"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f4b6375_d49dcf66","line":151,"in_reply_to":"3f4b6375_15cb7e4a","updated":"2017-11-01 02:31:23.000000000","message":"Done","commit_id":"d63d987fca5fd3618417604e4451029ee6dd08d2"},{"author":{"_account_id":19853,"name":"Nguyen Hung Phuong","email":"phuongnh@vn.fujitsu.com","username":"phuongnh"},"change_message_id":"3cc4accf6d3711a43471395490f80bfa6b5ec36d","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"        class FakeVifPort(object):"},{"line_number":104,"context_line":"            port_name \u003d self.port_name"},{"line_number":105,"context_line":"            ofport \u003d INVALID_OFPORT"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        return {\u0027vif_port\u0027: FakeVifPort(),"},{"line_number":108,"context_line":"                \u0027qos_policy_id\u0027: policy_id,"}],"source_content_type":"text/x-python","patch_set":12,"id":"3f4b6375_cad7634f","line":105,"range":{"start_line":105,"start_character":21,"end_line":105,"end_character":35},"updated":"2017-11-01 09:22:01.000000000","message":"ovs_lib.","commit_id":"51451ba08e5de123dabef5221ece221859ed6ba2"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"907446293aaefb3ab711952a9cecdef2c4240a21","unresolved":false,"context_lines":[{"line_number":55,"context_line":"            self.qos_driver.br_int.delete_egress_bw_limit_for_port)"},{"line_number":56,"context_line":"        self.delete_ingress \u003d ("},{"line_number":57,"context_line":"            self.qos_driver.br_int.delete_ingress_bw_limit_for_port)"},{"line_number":58,"context_line":"        self.qos_driver.br_int.create_egress_bw_limit_for_port \u003d mock.Mock()"},{"line_number":59,"context_line":"        self.create_egress \u003d ("},{"line_number":60,"context_line":"            self.qos_driver.br_int.create_egress_bw_limit_for_port)"},{"line_number":61,"context_line":"        self.update_ingress \u003d ("}],"source_content_type":"text/x-python","patch_set":15,"id":"3f4b6375_099689f1","side":"PARENT","line":58,"updated":"2017-11-02 08:48:50.000000000","message":"why this mock is removed?","commit_id":"5fc8e47786c91f76d253010b194bd5637de895b8"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"907446293aaefb3ab711952a9cecdef2c4240a21","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"        class FakeVifPort(object):"},{"line_number":104,"context_line":"            port_name \u003d self.port_name"},{"line_number":105,"context_line":"            ofport \u003d ovs_lib.INVALID_OFPORT"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        return {\u0027vif_port\u0027: FakeVifPort(),"},{"line_number":108,"context_line":"                \u0027qos_policy_id\u0027: policy_id,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f4b6375_e99a0d12","line":105,"updated":"2017-11-02 08:48:50.000000000","message":"is it really fine to use INVALID_OFPORT as ofport in this mock? I would rather set some positive value here","commit_id":"a12853d9109086d2d417e0f3ffbe774b59f6d22b"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"907446293aaefb3ab711952a9cecdef2c4240a21","unresolved":false,"context_lines":[{"line_number":143,"context_line":"        self.create_egress.assert_not_called()"},{"line_number":144,"context_line":"        self.update_ingress.assert_not_called()"},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"    def _test_delete_rules(self, qos_policy):"},{"line_number":147,"context_line":"        self.qos_driver.br_int.get_ingress_bw_limit_for_port \u003d mock.Mock("},{"line_number":148,"context_line":"            return_value\u003d(self.rules[1].max_kbps,"},{"line_number":149,"context_line":"                          self.rules[1].max_burst_kbps))"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f4b6375_c99bd114","line":146,"updated":"2017-11-02 08:48:50.000000000","message":"is this change related to patch?","commit_id":"a12853d9109086d2d417e0f3ffbe774b59f6d22b"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"907446293aaefb3ab711952a9cecdef2c4240a21","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        self.delete_egress.assert_called_once_with(self.port_name)"},{"line_number":153,"context_line":"        self.delete_ingress.assert_called_once_with(self.port_name)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def _test_delete_rules_no_policy(self):"},{"line_number":156,"context_line":"        self.qos_driver.br_int.get_ingress_bw_limit_for_port \u003d mock.Mock("},{"line_number":157,"context_line":"            return_value\u003d(self.rules[1].max_kbps,"},{"line_number":158,"context_line":"                          self.rules[1].max_burst_kbps))"}],"source_content_type":"text/x-python","patch_set":15,"id":"3f4b6375_a9a09543","line":155,"updated":"2017-11-02 08:48:50.000000000","message":"why You need this additional method if it is used only once? Can\u0027t You just move code from it directly to \"test_delete_rules_no_policy()\"?","commit_id":"a12853d9109086d2d417e0f3ffbe774b59f6d22b"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f77a2aa7ba715d3a12ca940931728a01e3698a4a","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"        class FakeVifPort(object):"},{"line_number":104,"context_line":"            port_name \u003d self.port_name"},{"line_number":105,"context_line":"            ofport \u003d ovs_lib.INVALID_OFPORT"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        return {\u0027vif_port\u0027: FakeVifPort(),"},{"line_number":108,"context_line":"                \u0027qos_policy_id\u0027: policy_id,"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_e306da1c","line":105,"updated":"2017-11-24 21:08:31.000000000","message":"why You are creating fake vif port with INVALID_OFPORT as ofport value? Shouldn\u0027t You use something \"more proper\" (like some positive int value) for tests?","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"eaf34bfae578884f2f40fedc542a90b06edcd2ee","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"        class FakeVifPort(object):"},{"line_number":104,"context_line":"            port_name \u003d self.port_name"},{"line_number":105,"context_line":"            ofport \u003d ovs_lib.INVALID_OFPORT"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        return {\u0027vif_port\u0027: FakeVifPort(),"},{"line_number":108,"context_line":"                \u0027qos_policy_id\u0027: policy_id,"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_71c84621","line":105,"in_reply_to":"ff82abbf_1111ba52","updated":"2017-11-27 08:55:25.000000000","message":"of course I know that but You are using it to mock real port, right? And code which You are testing can work differently when ofport is INVALID_OFPORT instead of good ofport number.","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"8ad2ce956332b0ca2f15735a63e67fe3a992017c","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"        class FakeVifPort(object):"},{"line_number":104,"context_line":"            port_name \u003d self.port_name"},{"line_number":105,"context_line":"            ofport \u003d ovs_lib.INVALID_OFPORT"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        return {\u0027vif_port\u0027: FakeVifPort(),"},{"line_number":108,"context_line":"                \u0027qos_policy_id\u0027: policy_id,"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_83800748","line":105,"in_reply_to":"ff82abbf_235eec43","updated":"2017-11-28 01:39:48.000000000","message":"then,ovs_lib.INVALID_OFPORT and 111, respectively, what is the difference in the test result ? Why fake port must have postive value ?","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"4dc8d31fa40833021a201376f46176c603b82c8b","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"        class FakeVifPort(object):"},{"line_number":104,"context_line":"            port_name \u003d self.port_name"},{"line_number":105,"context_line":"            ofport \u003d ovs_lib.INVALID_OFPORT"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        return {\u0027vif_port\u0027: FakeVifPort(),"},{"line_number":108,"context_line":"                \u0027qos_policy_id\u0027: policy_id,"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_7d001702","line":105,"in_reply_to":"ff82abbf_71c84621","updated":"2017-11-27 11:47:46.000000000","message":"give me a number that you think good!","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"a4482a738c10d590103cdfa402d75602a760632d","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"        class FakeVifPort(object):"},{"line_number":104,"context_line":"            port_name \u003d self.port_name"},{"line_number":105,"context_line":"            ofport \u003d ovs_lib.INVALID_OFPORT"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        return {\u0027vif_port\u0027: FakeVifPort(),"},{"line_number":108,"context_line":"                \u0027qos_policy_id\u0027: policy_id,"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_235eec43","line":105,"in_reply_to":"ff82abbf_7d001702","updated":"2017-11-27 14:23:08.000000000","message":"I think that any positive value should be fine so maybe 111? :)","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"7fd50359e35c0327f77c6bf26ecba6b98c387cc5","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"        class FakeVifPort(object):"},{"line_number":104,"context_line":"            port_name \u003d self.port_name"},{"line_number":105,"context_line":"            ofport \u003d ovs_lib.INVALID_OFPORT"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        return {\u0027vif_port\u0027: FakeVifPort(),"},{"line_number":108,"context_line":"                \u0027qos_policy_id\u0027: policy_id,"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_ad90944b","line":105,"in_reply_to":"ff82abbf_83800748","updated":"2017-11-28 07:58:11.000000000","message":"I just said that INVALID_OFPORT is kind of \"special\" number which means that port is could not be created due to some error (https://github.com/osrg/openvswitch/blob/master/FAQ#L807). In such case our code can do some different actions then it is expected.\nThats why I don\u0027t want to use this value here.\nBesides that it can be also confusing to other people if they will read this test\u0027s code in future because they can think that this is used for some test cases where port wasn\u0027t created properly.","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"71133df9b14c7f17d723d6c363f25ece4af0b2a1","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"        class FakeVifPort(object):"},{"line_number":104,"context_line":"            port_name \u003d self.port_name"},{"line_number":105,"context_line":"            ofport \u003d ovs_lib.INVALID_OFPORT"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        return {\u0027vif_port\u0027: FakeVifPort(),"},{"line_number":108,"context_line":"                \u0027qos_policy_id\u0027: policy_id,"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_0a796392","line":105,"in_reply_to":"ff82abbf_ad90944b","updated":"2017-11-28 09:12:32.000000000","message":"Done","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":25943,"name":"Zachary Ma","email":"mazengxie@126.com","username":"Zachary"},"change_message_id":"a62cbb34e86bb8ba05d614d85e0be06809f8d21a","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"        class FakeVifPort(object):"},{"line_number":104,"context_line":"            port_name \u003d self.port_name"},{"line_number":105,"context_line":"            ofport \u003d ovs_lib.INVALID_OFPORT"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        return {\u0027vif_port\u0027: FakeVifPort(),"},{"line_number":108,"context_line":"                \u0027qos_policy_id\u0027: policy_id,"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_1111ba52","line":105,"in_reply_to":"ff82abbf_e306da1c","updated":"2017-11-27 08:17:40.000000000","message":"fake port is not real port","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f77a2aa7ba715d3a12ca940931728a01e3698a4a","unresolved":false,"context_lines":[{"line_number":143,"context_line":"        self.create_egress.assert_not_called()"},{"line_number":144,"context_line":"        self.update_ingress.assert_not_called()"},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"    def _test_delete_rules(self, qos_policy):"},{"line_number":147,"context_line":"        self.qos_driver.br_int.get_ingress_bw_limit_for_port \u003d mock.Mock("},{"line_number":148,"context_line":"            return_value\u003d(self.rules[1].max_kbps,"},{"line_number":149,"context_line":"                          self.rules[1].max_burst_kbps))"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_030c1e39","line":146,"updated":"2017-11-24 21:08:31.000000000","message":"Now this helper function is called only in one place (or I missed something) and is not necessary IMHO. You can move code from it directly into test_delete_rules method probably","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"f77a2aa7ba715d3a12ca940931728a01e3698a4a","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        self.delete_egress.assert_called_once_with(self.port_name)"},{"line_number":153,"context_line":"        self.delete_ingress.assert_called_once_with(self.port_name)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def _test_delete_rules_no_policy(self):"},{"line_number":156,"context_line":"        self.qos_driver.br_int.get_ingress_bw_limit_for_port \u003d mock.Mock("},{"line_number":157,"context_line":"            return_value\u003d(self.rules[1].max_kbps,"},{"line_number":158,"context_line":"                          self.rules[1].max_burst_kbps))"}],"source_content_type":"text/x-python","patch_set":23,"id":"ff82abbf_a3145245","line":155,"updated":"2017-11-24 21:08:31.000000000","message":"IMO this helper method is not necessary at all. You can put this code directly in test_delete_rules_no_policy() method","commit_id":"cfcc1c34710fc4527544f55e3abf4d9c0981ff27"}]}