)]}'
{"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py":[{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"inactive contributor"},"change_message_id":"99404d0b18055dc16e22757db2b79276bf43f1be","unresolved":false,"context_lines":[{"line_number":660,"context_line":"                                  if ovn_const.OVN_QOS_POLICY_EXT_ID_KEY"},{"line_number":661,"context_line":"                                  in ovn_net.external_ids else None)"},{"line_number":662,"context_line":""},{"line_number":663,"context_line":"            if qos_options_new:"},{"line_number":664,"context_line":"                if qos_options_old:"},{"line_number":665,"context_line":"                    qos_rule_column_old \u003d self._create_qos_rules(qos_options_old,"},{"line_number":666,"context_line":"                                                                 port,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa7e38b_02096681","line":663,"range":{"start_line":663,"start_character":15,"end_line":663,"end_character":30},"updated":"2020-02-12 11:11:42.000000000","message":"My current devstack does not have qos enabled in it, so I didn\u0027t try, but can we update a port from having a qos_policy to not having one? This code reads like we\u0027d miss deleting the qos rules in a port update like that.\n\nI know you did not change this line, just wondering.","commit_id":"f9001456d8570de8aa248bc8a6406c9f93451a9d"},{"author":{"_account_id":29157,"name":"zhanghao","email":"hao.zhang.am.i@gmail.com","username":"zhanghao2"},"change_message_id":"695a1ba603ba4456684932f18595a5cb01051c82","unresolved":false,"context_lines":[{"line_number":660,"context_line":"                                  if ovn_const.OVN_QOS_POLICY_EXT_ID_KEY"},{"line_number":661,"context_line":"                                  in ovn_net.external_ids else None)"},{"line_number":662,"context_line":""},{"line_number":663,"context_line":"            if qos_options_new:"},{"line_number":664,"context_line":"                if qos_options_old:"},{"line_number":665,"context_line":"                    qos_rule_column_old \u003d self._create_qos_rules(qos_options_old,"},{"line_number":666,"context_line":"                                                                 port,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa7e38b_d03234f3","line":663,"range":{"start_line":663,"start_character":15,"end_line":663,"end_character":30},"in_reply_to":"3fa7e38b_02096681","updated":"2020-02-12 12:46:01.000000000","message":"It is ok to update the qos policy for a port without qos policy.Since there is no test_ovn_client module, I have not added UTs, but I have tested it in my local environment.","commit_id":"f9001456d8570de8aa248bc8a6406c9f93451a9d"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"inactive contributor"},"change_message_id":"0bc2ea89292ada1131f3266df24d22bcb7a8a275","unresolved":false,"context_lines":[{"line_number":660,"context_line":"                                  if ovn_const.OVN_QOS_POLICY_EXT_ID_KEY"},{"line_number":661,"context_line":"                                  in ovn_net.external_ids else None)"},{"line_number":662,"context_line":""},{"line_number":663,"context_line":"            if qos_options_new:"},{"line_number":664,"context_line":"                if qos_options_old:"},{"line_number":665,"context_line":"                    qos_rule_column_old \u003d self._create_qos_rules(qos_options_old,"},{"line_number":666,"context_line":"                                                                 port,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa7e38b_23b7b224","line":663,"range":{"start_line":663,"start_character":15,"end_line":663,"end_character":30},"in_reply_to":"3fa7e38b_d03234f3","updated":"2020-02-13 10:59:27.000000000","message":"I still think we are missing some tests, but I\u0027m looking where it would be the simplest to add them. Maybe we already have some just we don\u0027t run it against the ovn driver.\n\nBut generally speaking when we find a bug and a whole unit test file is missing that\u0027s a very good indication that we should add some tests.","commit_id":"f9001456d8570de8aa248bc8a6406c9f93451a9d"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"baf54daaf871f0e92842f34d1983c85853fd98ae","unresolved":false,"context_lines":[{"line_number":661,"context_line":"                                  in ovn_net.external_ids else None)"},{"line_number":662,"context_line":""},{"line_number":663,"context_line":"            if qos_options_new:"},{"line_number":664,"context_line":"                if qos_options_old:"},{"line_number":665,"context_line":"                    qos_rule_column_old \u003d self._create_qos_rules(qos_options_old,"},{"line_number":666,"context_line":"                                                                 port,"},{"line_number":667,"context_line":"                                                                 lswitch_name,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa7e38b_02e2c6d3","line":664,"range":{"start_line":664,"start_character":19,"end_line":664,"end_character":34},"updated":"2020-02-12 11:04:01.000000000","message":"\"qos_options_old\" can be \u003d\u003d \"qos_options_new\" in case no \"qos_policy_id\" is in the port. In this case, do we need to delete the rules??\n\nI think you should also add a check for this:\n\n  if qos_options_old or qos_options_old !\u003d qos_options_new:","commit_id":"f9001456d8570de8aa248bc8a6406c9f93451a9d"},{"author":{"_account_id":29157,"name":"zhanghao","email":"hao.zhang.am.i@gmail.com","username":"zhanghao2"},"change_message_id":"695a1ba603ba4456684932f18595a5cb01051c82","unresolved":false,"context_lines":[{"line_number":661,"context_line":"                                  in ovn_net.external_ids else None)"},{"line_number":662,"context_line":""},{"line_number":663,"context_line":"            if qos_options_new:"},{"line_number":664,"context_line":"                if qos_options_old:"},{"line_number":665,"context_line":"                    qos_rule_column_old \u003d self._create_qos_rules(qos_options_old,"},{"line_number":666,"context_line":"                                                                 port,"},{"line_number":667,"context_line":"                                                                 lswitch_name,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa7e38b_8521e0b4","line":664,"range":{"start_line":664,"start_character":19,"end_line":664,"end_character":34},"in_reply_to":"3fa7e38b_02e2c6d3","updated":"2020-02-12 12:46:01.000000000","message":"If the port does not have \"qos_policy_id\", \"qos_options_new\" will not be equal to \"qos_options_old\", because \"qos_options_new\" will be the new qos rules, and \"qos_options_old\" is empty.","commit_id":"f9001456d8570de8aa248bc8a6406c9f93451a9d"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"inactive contributor"},"change_message_id":"0bc2ea89292ada1131f3266df24d22bcb7a8a275","unresolved":false,"context_lines":[{"line_number":661,"context_line":"                                  in ovn_net.external_ids else None)"},{"line_number":662,"context_line":""},{"line_number":663,"context_line":"            if qos_options_new:"},{"line_number":664,"context_line":"                if qos_options_old:"},{"line_number":665,"context_line":"                    qos_rule_column_old \u003d self._create_qos_rules("},{"line_number":666,"context_line":"                        qos_options_old,"},{"line_number":667,"context_line":"                        port,"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa7e38b_43abced2","line":664,"updated":"2020-02-13 10:59:27.000000000","message":"As explained in my comment to the bug report here:\n\nhttps://bugs.launchpad.net/neutron/+bug/1862893/comments/2\n\nI\u0027m wondering if we should also fix the wrong assumptions of _create_qos_rules too (as marked below).","commit_id":"881a9240597eb833b79ad3244e473f08348e4b02"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"inactive contributor"},"change_message_id":"0bc2ea89292ada1131f3266df24d22bcb7a8a275","unresolved":false,"context_lines":[{"line_number":703,"context_line":"    def _create_qos_rules(self, qos_options, port, lswitch_name,"},{"line_number":704,"context_line":"                          if_delete\u003dFalse):"},{"line_number":705,"context_line":"        qos_rule \u003d {}"},{"line_number":706,"context_line":"        direction \u003d \u0027from-lport\u0027 if qos_options[\u0027direction\u0027] \u003d\u003d\\"},{"line_number":707,"context_line":"                    \u0027egress\u0027 else \u0027to-lport\u0027"},{"line_number":708,"context_line":"        qos_rule.update(switch\u003dlswitch_name, direction\u003ddirection,"},{"line_number":709,"context_line":"                        priority\u003d2002)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa7e38b_8360e6d1","line":706,"range":{"start_line":706,"start_character":36,"end_line":706,"end_character":60},"updated":"2020-02-13 10:59:27.000000000","message":"There may be no \u0027direction\u0027 key in qos_options.","commit_id":"881a9240597eb833b79ad3244e473f08348e4b02"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"inactive contributor"},"change_message_id":"0bc2ea89292ada1131f3266df24d22bcb7a8a275","unresolved":false,"context_lines":[{"line_number":717,"context_line":"        # QoS of bandwidth_limit"},{"line_number":718,"context_line":"        if \u0027qos_max_rate\u0027 in qos_options:"},{"line_number":719,"context_line":"            qos_rule.update(rate\u003dint(qos_options[\u0027qos_max_rate\u0027]),"},{"line_number":720,"context_line":"                            burst\u003dint(qos_options[\u0027qos_burst\u0027]),"},{"line_number":721,"context_line":"                            dscp\u003dNone)"},{"line_number":722,"context_line":"        # QoS of dscp"},{"line_number":723,"context_line":"        elif \u0027dscp_mark\u0027 in qos_options:"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa7e38b_e3967aa0","line":720,"range":{"start_line":720,"start_character":38,"end_line":720,"end_character":62},"updated":"2020-02-13 10:59:27.000000000","message":"There may be no \u0027qos_burst\u0027 key in qos_options.","commit_id":"881a9240597eb833b79ad3244e473f08348e4b02"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"5662043ace0a1d4b362ca0567d1832b9aa58db84","unresolved":false,"context_lines":[{"line_number":651,"context_line":"                               else self._qos_driver.get_qos_options(port))"},{"line_number":652,"context_line":"            # If port_object is None, we also need to get necessary params"},{"line_number":653,"context_line":"            # to delete the qos rule"},{"line_number":654,"context_line":"            qos_options_old \u003d (self._qos_driver.get_qos_options(port_object)"},{"line_number":655,"context_line":"                               if port_object else qos_options_new)"},{"line_number":656,"context_line":""},{"line_number":657,"context_line":"            ovn_net \u003d self._nb_idl.get_lswitch(lswitch_name)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa7e38b_1ea2e4ac","line":654,"range":{"start_line":654,"start_character":12,"end_line":654,"end_character":27},"updated":"2020-02-17 08:48:46.000000000","message":"We don\u0027t validate here if qos_options_new and qos_options_old those are equal or not.\nWhy to delete and update things even those not changed?","commit_id":"d0e7b8865617d748dca485422a53fac3eb09b63c"},{"author":{"_account_id":29157,"name":"zhanghao","email":"hao.zhang.am.i@gmail.com","username":"zhanghao2"},"change_message_id":"09719a25306c88086d3105e74b006b745ffa949f","unresolved":false,"context_lines":[{"line_number":651,"context_line":"                               else self._qos_driver.get_qos_options(port))"},{"line_number":652,"context_line":"            # If port_object is None, we also need to get necessary params"},{"line_number":653,"context_line":"            # to delete the qos rule"},{"line_number":654,"context_line":"            qos_options_old \u003d (self._qos_driver.get_qos_options(port_object)"},{"line_number":655,"context_line":"                               if port_object else qos_options_new)"},{"line_number":656,"context_line":""},{"line_number":657,"context_line":"            ovn_net \u003d self._nb_idl.get_lswitch(lswitch_name)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa7e38b_642a6a2f","line":654,"range":{"start_line":654,"start_character":12,"end_line":654,"end_character":27},"in_reply_to":"3fa7e38b_1ea2e4ac","updated":"2020-02-17 10:16:20.000000000","message":"Done","commit_id":"d0e7b8865617d748dca485422a53fac3eb09b63c"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"2db5c747a01284664ca1e843f0de1aa487acc13a","unresolved":false,"context_lines":[{"line_number":661,"context_line":"                                  in ovn_net.external_ids else None)"},{"line_number":662,"context_line":""},{"line_number":663,"context_line":"            if qos_options_new:"},{"line_number":664,"context_line":"                if qos_options_new !\u003d qos_options_old:"},{"line_number":665,"context_line":"                    if qos_options_old:"},{"line_number":666,"context_line":"                        qos_rule_column_old \u003d self._create_qos_rules("},{"line_number":667,"context_line":"                            qos_options_old,"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_ea6aa3e8","line":664,"range":{"start_line":664,"start_character":38,"end_line":664,"end_character":53},"updated":"2020-02-17 13:33:43.000000000","message":"qos_options_old comes from \u0027port_object\u0027 (L654) which according to comment in L457 we should move away from.\nIs there a way we can achieve the same by removing this dependency?\n\nThis can be definitely a follow-up as the dependency was already there. Just wondering as at some point we\u0027ll like to remove it.","commit_id":"dd96935a4c75854b692b190f23f02c6729b1dc44"},{"author":{"_account_id":29874,"name":"Taoyunxiang","email":"taoyunxiang_yewu@cmss.chinamobile.com","username":"Yunxiang"},"change_message_id":"ca3caef66fb12245f0de47321864a98f47bbbc9b","unresolved":false,"context_lines":[{"line_number":661,"context_line":"                                  in ovn_net.external_ids else None)"},{"line_number":662,"context_line":""},{"line_number":663,"context_line":"            if qos_options_new:"},{"line_number":664,"context_line":"                if qos_options_new !\u003d qos_options_old:"},{"line_number":665,"context_line":"                    if qos_options_old:"},{"line_number":666,"context_line":"                        qos_rule_column_old \u003d self._create_qos_rules("},{"line_number":667,"context_line":"                            qos_options_old,"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_80197977","line":664,"range":{"start_line":664,"start_character":38,"end_line":664,"end_character":53},"in_reply_to":"3fa7e38b_ea6aa3e8","updated":"2020-02-18 13:20:04.000000000","message":"Hi Daniel,we also talked about this in [0].\n\nI am agree with you and zhanghao,it isn\u0027t good way to get QoS infomation from port_object.\n\n\nI think the orginal problem of this is that QoS policy has not a direct releated resource in core OVN. \n\nI think the best way is to make a connection from UUID of qos policy of neutron and QoS row of core OVN. The probelm is how to record the UUID in core OVN, I think we can add a \"name\" column in QoS table of core OVN to do it, so I summit [1]. Han Zhou suggest me to make use of [2] to make the UUIDs be the same between neutron and Core OVN,but this patch colud not support ovn-*ctl commands, so I summit [3].\n\nI think we have three ways to solve this.\n1. As zhanghao says,we could record new and old qos policy in port and network. This just need to change neutron code.\n\n2.Make the UUIDs be the same between neutron and Core OVN, this need to wait my patch[3] to merge, I think it needs a long time, and we need do much work on ovsdbapp.\n\n3.If we record UUID of qos policy of neutron in external_ids of QoS in core OVN is not a bad choice,though I think it will be costly when search the target,but we have done many similar features. It need to change ovsdbapp and neutron. I think it is not complicated, and is a suitable way to do.\n\nWhat do you think?\n\n\n[0]https://review.opendev.org/#/c/692084/ \n[1]http://patchwork.ozlabs.org/cover/1214897/\n[2]https://patchwork.ozlabs.org/patch/1220644/\n[3]http://patchwork.ozlabs.org/patch/1239790/","commit_id":"dd96935a4c75854b692b190f23f02c6729b1dc44"},{"author":{"_account_id":29157,"name":"zhanghao","email":"hao.zhang.am.i@gmail.com","username":"zhanghao2"},"change_message_id":"aef4f2d627e17dce089ce2da11d4474e736eada7","unresolved":false,"context_lines":[{"line_number":661,"context_line":"                                  in ovn_net.external_ids else None)"},{"line_number":662,"context_line":""},{"line_number":663,"context_line":"            if qos_options_new:"},{"line_number":664,"context_line":"                if qos_options_new !\u003d qos_options_old:"},{"line_number":665,"context_line":"                    if qos_options_old:"},{"line_number":666,"context_line":"                        qos_rule_column_old \u003d self._create_qos_rules("},{"line_number":667,"context_line":"                            qos_options_old,"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_f9b35134","line":664,"range":{"start_line":664,"start_character":38,"end_line":664,"end_character":53},"in_reply_to":"3fa7e38b_ea6aa3e8","updated":"2020-02-18 04:14:58.000000000","message":"I noticed this. If we want to remove \u0027port_object\u0027, I suggest using a follow-up patch.Maybe we can insert \u0027new_qos_policy_id\u0027 and \u0027old_qos_policy_id\u0027 in the port to distinguish the changes.","commit_id":"dd96935a4c75854b692b190f23f02c6729b1dc44"}]}