)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":10338,"name":"Daniel Speichert","email":"daniel@speichert.pl","username":"daniel"},"change_message_id":"b686f3e49e33d8c81685d3e52792cda1e2beac0f","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"9195e4e6_2e52a557","updated":"2021-10-13 18:49:43.000000000","message":"This may not be an ideal fix, I\u0027d imagine integrating the \"label\" field may be useful in Neutron, although if it isn\u0027t present in earlier versions of the OVN model, that wouldn\u0027t be backwards compatible.\n\nI also am not sure when and how \"label\" was added (was not a problem with OVN 20.03).\n\nConsider this a very early draft for a real problem in stable Xena. Suggestions very welcome.","commit_id":"37948d0c4f513fa9645e761330b93e8642817abd"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"adb029d81b01f84503f6c38ec1d13f1a95290a87","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"560c7d17_9a83f436","updated":"2021-10-15 09:24:54.000000000","message":"Hi Daniel,\n\nThanks very much for ur fix. Even tho just removing that label key works, I think we can make the code better by comparing only the keys we care about. That will also make the code more bullet proof for future changes in the OVSDB schema.","commit_id":"d987e43a03c10038f105b1be607a54e890bdc88b"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"886f2365ae15f6f64245d3023462b3fe07cc321c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"59d2144e_377995c3","updated":"2021-10-18 07:44:51.000000000","message":"Sorry for this comment but I just though that maybe it would be good to add/modify some UT in the neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py to be sure that this code will compare only those keys which we want to. Wdyt?","commit_id":"ec8a796f6b431ac139d09e7464a9d77a63bcbea6"},{"author":{"_account_id":10338,"name":"Daniel Speichert","email":"daniel@speichert.pl","username":"daniel"},"change_message_id":"797f64b57a0ca13a226ece83821bfa875cdbea9a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"adfe5983_abe970d5","in_reply_to":"59d2144e_377995c3","updated":"2021-11-02 13:22:31.000000000","message":"I believe the current unit tests did not actually test ACL sync at all. I made changes according to my best interpretation of what was supposed to happen.","commit_id":"ec8a796f6b431ac139d09e7464a9d77a63bcbea6"},{"author":{"_account_id":10338,"name":"Daniel Speichert","email":"daniel@speichert.pl","username":"daniel"},"change_message_id":"797f64b57a0ca13a226ece83821bfa875cdbea9a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"b4171c19_cb6b4165","updated":"2021-11-02 13:22:31.000000000","message":"Found that there is a list of fields defined in constants, so reusing that.\nAdded minimal changes to make ACL syncing actually be tested in unit tests.","commit_id":"a2389f7ec9ad14b601933af9603dbddfff465804"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py":[{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"509e0beea20b135ce7610b95b9a01c56a6f32a4b","unresolved":true,"context_lines":[{"line_number":284,"context_line":"            for ovn_a in ovn_acls:"},{"line_number":285,"context_line":"                # OVN 21.09+ introduced a \"label\" field that we must skip"},{"line_number":286,"context_line":"                # (if present) when doing the comparison."},{"line_number":287,"context_line":"                ovn_a.pop(\u0027label\u0027, None)"},{"line_number":288,"context_line":"                if all(item in na.items() for item in ovn_a.items()):"},{"line_number":289,"context_line":"                    neutron_acls.remove(na)"},{"line_number":290,"context_line":"                    ovn_acls.remove(ovn_a)"}],"source_content_type":"text/x-python","patch_set":1,"id":"cf477bc3_770bf122","line":287,"updated":"2021-10-14 10:36:48.000000000","message":"that will remove \"label\" from the ovn_a object so it will not be available in further processing. Shouldn\u0027t You make copy of the ovn_a first and remove that label from the copy only?","commit_id":"37948d0c4f513fa9645e761330b93e8642817abd"},{"author":{"_account_id":10338,"name":"Daniel Speichert","email":"daniel@speichert.pl","username":"daniel"},"change_message_id":"797f64b57a0ca13a226ece83821bfa875cdbea9a","unresolved":false,"context_lines":[{"line_number":284,"context_line":"            for ovn_a in ovn_acls:"},{"line_number":285,"context_line":"                # OVN 21.09+ introduced a \"label\" field that we must skip"},{"line_number":286,"context_line":"                # (if present) when doing the comparison."},{"line_number":287,"context_line":"                ovn_a.pop(\u0027label\u0027, None)"},{"line_number":288,"context_line":"                if all(item in na.items() for item in ovn_a.items()):"},{"line_number":289,"context_line":"                    neutron_acls.remove(na)"},{"line_number":290,"context_line":"                    ovn_acls.remove(ovn_a)"}],"source_content_type":"text/x-python","patch_set":1,"id":"b4a379b5_62eef5ac","line":287,"in_reply_to":"2df90346_e7af3b8a","updated":"2021-11-02 13:22:31.000000000","message":"Done","commit_id":"37948d0c4f513fa9645e761330b93e8642817abd"},{"author":{"_account_id":10338,"name":"Daniel Speichert","email":"daniel@speichert.pl","username":"daniel"},"change_message_id":"b2ed2133856a69933ae3ff157b8145e4d2b50bbf","unresolved":true,"context_lines":[{"line_number":284,"context_line":"            for ovn_a in ovn_acls:"},{"line_number":285,"context_line":"                # OVN 21.09+ introduced a \"label\" field that we must skip"},{"line_number":286,"context_line":"                # (if present) when doing the comparison."},{"line_number":287,"context_line":"                ovn_a.pop(\u0027label\u0027, None)"},{"line_number":288,"context_line":"                if all(item in na.items() for item in ovn_a.items()):"},{"line_number":289,"context_line":"                    neutron_acls.remove(na)"},{"line_number":290,"context_line":"                    ovn_acls.remove(ovn_a)"}],"source_content_type":"text/x-python","patch_set":1,"id":"2df90346_e7af3b8a","line":287,"in_reply_to":"cf477bc3_770bf122","updated":"2021-10-14 20:48:49.000000000","message":"That is a good observation. I did not think \"label\" is used in this codepath, but I\u0027ll update the patch to make a copy for comparison.","commit_id":"37948d0c4f513fa9645e761330b93e8642817abd"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"adb029d81b01f84503f6c38ec1d13f1a95290a87","unresolved":true,"context_lines":[{"line_number":286,"context_line":"                # (if present) when doing the comparison."},{"line_number":287,"context_line":"                ovn_a_copy \u003d ovn_a.copy()"},{"line_number":288,"context_line":"                ovn_a_copy.pop(\u0027label\u0027, None)"},{"line_number":289,"context_line":"                if all(item in na.items() for item in ovn_a_copy.items()):"},{"line_number":290,"context_line":"                    neutron_acls.remove(na)"},{"line_number":291,"context_line":"                    ovn_acls.remove(ovn_a)"},{"line_number":292,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":2,"id":"aaf18b76_ff75daa8","line":289,"updated":"2021-10-15 09:24:54.000000000","message":"Thanks for this fix! Great catch.\n\nInstead of removing this new key, maybe we could make a list of keys that we expect and compare those ?\n\nI am afraid this may break again in the future once another column is added to the OVSDB schema. I think we could make it better by not blindly checking every key but instead just the ones that we care about.","commit_id":"d987e43a03c10038f105b1be607a54e890bdc88b"},{"author":{"_account_id":10338,"name":"Daniel Speichert","email":"daniel@speichert.pl","username":"daniel"},"change_message_id":"6868f691ee1bcce08c81a2be3eb185801a9a5e34","unresolved":false,"context_lines":[{"line_number":286,"context_line":"                # (if present) when doing the comparison."},{"line_number":287,"context_line":"                ovn_a_copy \u003d ovn_a.copy()"},{"line_number":288,"context_line":"                ovn_a_copy.pop(\u0027label\u0027, None)"},{"line_number":289,"context_line":"                if all(item in na.items() for item in ovn_a_copy.items()):"},{"line_number":290,"context_line":"                    neutron_acls.remove(na)"},{"line_number":291,"context_line":"                    ovn_acls.remove(ovn_a)"},{"line_number":292,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":2,"id":"1cd5605b_7db83802","line":289,"in_reply_to":"aaf18b76_ff75daa8","updated":"2021-10-15 18:27:05.000000000","message":"Thanks for the suggestions. I\u0027ve listed explicit fields to compare. I started with all the existing ones that worked so far - not sure if all do need to be compared (but it doesn\u0027t hurt).\n\nThere may be other places in the code where the list of fields to compare may need to be reused, in which case it would make sense to move it somewhere to be imported, etc - but I personally don\u0027t know that.","commit_id":"d987e43a03c10038f105b1be607a54e890bdc88b"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"e7fb5d110b119f59c40e185f71bf888a0752c10c","unresolved":true,"context_lines":[{"line_number":283,"context_line":"        # Since OVN can have extra fields, we explicitly list the fields"},{"line_number":284,"context_line":"        # to compare."},{"line_number":285,"context_line":"        compared_fields \u003d ["},{"line_number":286,"context_line":"            \u0027action\u0027,"},{"line_number":287,"context_line":"            \u0027direction\u0027,"},{"line_number":288,"context_line":"            \u0027log\u0027,"},{"line_number":289,"context_line":"            \u0027match\u0027,"},{"line_number":290,"context_line":"            \u0027name\u0027,"},{"line_number":291,"context_line":"            \u0027port_group\u0027,"},{"line_number":292,"context_line":"            \u0027priority\u0027,"},{"line_number":293,"context_line":"            \u0027severity\u0027,"},{"line_number":294,"context_line":"        ]"},{"line_number":295,"context_line":"        for na in list(neutron_acls):"},{"line_number":296,"context_line":"            for ovn_a in ovn_acls:"}],"source_content_type":"text/x-python","patch_set":6,"id":"32c10049_50f6b660","line":293,"range":{"start_line":286,"start_character":12,"end_line":293,"end_character":23},"updated":"2021-10-20 15:18:19.000000000","message":"Where are those parameters coming from? Because the ACL register has other fields [1]\n\n[1]https://github.com/ovn-org/ovn/blob/ddd69d81c8ad5c1568a5656ef5f0abe442fb3e94/ovn-nb.ovsschema#L238-L268","commit_id":"ec8a796f6b431ac139d09e7464a9d77a63bcbea6"},{"author":{"_account_id":10338,"name":"Daniel Speichert","email":"daniel@speichert.pl","username":"daniel"},"change_message_id":"7453ca28c5a8cdf01b12277f5c402e148de2b7dd","unresolved":true,"context_lines":[{"line_number":283,"context_line":"        # Since OVN can have extra fields, we explicitly list the fields"},{"line_number":284,"context_line":"        # to compare."},{"line_number":285,"context_line":"        compared_fields \u003d ["},{"line_number":286,"context_line":"            \u0027action\u0027,"},{"line_number":287,"context_line":"            \u0027direction\u0027,"},{"line_number":288,"context_line":"            \u0027log\u0027,"},{"line_number":289,"context_line":"            \u0027match\u0027,"},{"line_number":290,"context_line":"            \u0027name\u0027,"},{"line_number":291,"context_line":"            \u0027port_group\u0027,"},{"line_number":292,"context_line":"            \u0027priority\u0027,"},{"line_number":293,"context_line":"            \u0027severity\u0027,"},{"line_number":294,"context_line":"        ]"},{"line_number":295,"context_line":"        for na in list(neutron_acls):"},{"line_number":296,"context_line":"            for ovn_a in ovn_acls:"}],"source_content_type":"text/x-python","patch_set":6,"id":"be3adc85_35822a58","line":293,"range":{"start_line":286,"start_character":12,"end_line":293,"end_character":23},"in_reply_to":"32c10049_50f6b660","updated":"2021-10-20 17:32:53.000000000","message":"Correct. The only difference comes from how ovn_acls goes through _get_acls_from_port_groups function [1]. That drops external_ids in favor of port_group field. Also drops \"meter\" field for whatever reason.\n\nSo the list of fields to compare is not based on pure-form OVN field but after existing transformations in the code.\n\n[1] https://github.com/openstack/neutron/blob/6d8e830859cd4ac9708701b8e344fdc68cbcaebb/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py#L226-L239\n\nBtw, I am working on a unit test.","commit_id":"ec8a796f6b431ac139d09e7464a9d77a63bcbea6"},{"author":{"_account_id":10338,"name":"Daniel Speichert","email":"daniel@speichert.pl","username":"daniel"},"change_message_id":"797f64b57a0ca13a226ece83821bfa875cdbea9a","unresolved":false,"context_lines":[{"line_number":283,"context_line":"        # Since OVN can have extra fields, we explicitly list the fields"},{"line_number":284,"context_line":"        # to compare."},{"line_number":285,"context_line":"        compared_fields \u003d ["},{"line_number":286,"context_line":"            \u0027action\u0027,"},{"line_number":287,"context_line":"            \u0027direction\u0027,"},{"line_number":288,"context_line":"            \u0027log\u0027,"},{"line_number":289,"context_line":"            \u0027match\u0027,"},{"line_number":290,"context_line":"            \u0027name\u0027,"},{"line_number":291,"context_line":"            \u0027port_group\u0027,"},{"line_number":292,"context_line":"            \u0027priority\u0027,"},{"line_number":293,"context_line":"            \u0027severity\u0027,"},{"line_number":294,"context_line":"        ]"},{"line_number":295,"context_line":"        for na in list(neutron_acls):"},{"line_number":296,"context_line":"            for ovn_a in ovn_acls:"}],"source_content_type":"text/x-python","patch_set":6,"id":"c743797a_bceb8582","line":293,"range":{"start_line":286,"start_character":12,"end_line":293,"end_character":23},"in_reply_to":"be3adc85_35822a58","updated":"2021-11-02 13:22:31.000000000","message":"Done","commit_id":"ec8a796f6b431ac139d09e7464a9d77a63bcbea6"}]}