)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"90d54d07050c27bdd13e3b94d0fbb7f9a40a180d","unresolved":false,"context_lines":[{"line_number":9,"context_line":"ovs-vsctl, unless --no-wait is passed, will wait until ovs-vswitchd"},{"line_number":10,"context_line":"has reacted to a successful transaction. This patch implements"},{"line_number":11,"context_line":"the same logic, waiting for next_cfg to be incremented and checking"},{"line_number":12,"context_line":"that any added interfaces have actually been assigned ofports."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: I638b82c13394f150c0bd23301285bd3375e66139"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"dada55a8_1df2289a","line":12,"updated":"2016-07-21 15:25:04.000000000","message":"Bug number?\n\nI suspect it\u0027s also related to:\n\nhttps://bugs.launchpad.net/neutron/+bug/1599329 (see comment 8)","commit_id":"c96ec78c7ca48338cce553cfe210bfb80acac0d9"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"94cd6d72bcffa1418d04175e0602a88352d6d947","unresolved":false,"context_lines":[{"line_number":12,"context_line":"that any added interfaces have actually been assigned ofports."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Closes-Bug: #1604816"},{"line_number":15,"context_line":"Closes-Bug: #1599329"},{"line_number":16,"context_line":"Related-Bug: #1604115"},{"line_number":17,"context_line":"Change-Id: I638b82c13394f150c0bd23301285bd3375e66139"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"dada55a8_72736916","line":15,"updated":"2016-07-22 09:35:22.000000000","message":"Are you sure about it closing this second bug? OVS is assigning the same ofport to multiple interfaces in that bug. If this timing change fixes it because it prevents the port from being moved into the namespace until OVS is done, can you mention that in the commit message?","commit_id":"9e79b60bc7380280a682667091935e77d9e51650"}],"neutron/agent/common/ovs_lib.py":[{"author":{"_account_id":13717,"name":"Inessa Vasilevskaya","email":"ivasilevskaya@mirantis.com","username":"ivasilevskaya"},"change_message_id":"057aaa615726bd2f2b3dff73eb6407e545328fd9","unresolved":false,"context_lines":[{"line_number":122,"context_line":"                          datapath_type).execute()"},{"line_number":123,"context_line":"        br \u003d OVSBridge(bridge_name)"},{"line_number":124,"context_line":"        # Don\u0027t return until vswitchd sets up the internal port"},{"line_number":125,"context_line":"        br.get_port_ofport(bridge_name)"},{"line_number":126,"context_line":"        return br"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"    def delete_bridge(self, bridge_name):"}],"source_content_type":"text/x-python","patch_set":5,"id":"dada55a8_3afead6b","side":"PARENT","line":125,"range":{"start_line":125,"start_character":8,"end_line":125,"end_character":39},"updated":"2016-07-24 13:10:23.000000000","message":"As far as I understand, get_port_ofport was used here to ensure port readiness with ovsdb_interface\u003dvsctl.\nIf we remove this logic vsctl mode will be prone to the same port access timing problems you are solving with this patch.\n\nVsctl mode isn\u0027t deprecated yet. Shouldn\u0027t we keep get_port_ofport checks before official deprecation?","commit_id":"60db12f9d2f8a44d411e2ee8742c33be08078864"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"489897765c8d72d356d4c3cbc8f61c870a3271f8","unresolved":false,"context_lines":[{"line_number":122,"context_line":"                          datapath_type).execute()"},{"line_number":123,"context_line":"        br \u003d OVSBridge(bridge_name)"},{"line_number":124,"context_line":"        # Don\u0027t return until vswitchd sets up the internal port"},{"line_number":125,"context_line":"        br.get_port_ofport(bridge_name)"},{"line_number":126,"context_line":"        return br"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"    def delete_bridge(self, bridge_name):"}],"source_content_type":"text/x-python","patch_set":5,"id":"dada55a8_0db436f0","side":"PARENT","line":125,"range":{"start_line":125,"start_character":8,"end_line":125,"end_character":39},"in_reply_to":"dada55a8_3afead6b","updated":"2016-07-25 14:00:26.000000000","message":"I\u0027m pretty sure this was added as a workaround for the native interface issue that this patch fixes. There is no reason to get the ofport here for vsctl because it already waits for the ofport to be set before returning.","commit_id":"60db12f9d2f8a44d411e2ee8742c33be08078864"}],"neutron/agent/ovsdb/impl_idl.py":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"0d99f0cb342294275b8795b737143d764a7a1311","unresolved":false,"context_lines":[{"line_number":76,"context_line":"            attempts +\u003d 1"},{"line_number":77,"context_line":"            # TODO(twilson) Make sure we don\u0027t loop longer than vsctl_timeout"},{"line_number":78,"context_line":"            txn \u003d idl.Transaction(self.api.idl)"},{"line_number":79,"context_line":"            self.api._ovs.increment(\u0027next_cfg\u0027)"},{"line_number":80,"context_line":"            for i, command in enumerate(self.commands):"},{"line_number":81,"context_line":"                LOG.debug(\"Running txn command(idx\u003d%(idx)s): %(cmd)s\","},{"line_number":82,"context_line":"                          {\u0027idx\u0027: i, \u0027cmd\u0027: command})"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_155520f9","line":79,"range":{"start_line":79,"start_character":21,"end_line":79,"end_character":22},"updated":"2016-07-20 15:12:27.000000000","message":"Would it make sense to create a new method for increment? Seems like _ovs is encapsulated inside of api object.","commit_id":"2b0683f6ac7107193690a7b14e6ef42ef0568699"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"eae5e8fca7e9b40be17952fa36227873acc08f87","unresolved":false,"context_lines":[{"line_number":76,"context_line":"            attempts +\u003d 1"},{"line_number":77,"context_line":"            # TODO(twilson) Make sure we don\u0027t loop longer than vsctl_timeout"},{"line_number":78,"context_line":"            txn \u003d idl.Transaction(self.api.idl)"},{"line_number":79,"context_line":"            self.api._ovs.increment(\u0027next_cfg\u0027)"},{"line_number":80,"context_line":"            for i, command in enumerate(self.commands):"},{"line_number":81,"context_line":"                LOG.debug(\"Running txn command(idx\u003d%(idx)s): %(cmd)s\","},{"line_number":82,"context_line":"                          {\u0027idx\u0027: i, \u0027cmd\u0027: command})"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_7eb9d914","line":79,"range":{"start_line":79,"start_character":21,"end_line":79,"end_character":22},"in_reply_to":"dada55a8_155520f9","updated":"2016-07-20 16:08:02.000000000","message":"Sounds reasonable.","commit_id":"2b0683f6ac7107193690a7b14e6ef42ef0568699"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"0d99f0cb342294275b8795b737143d764a7a1311","unresolved":false,"context_lines":[{"line_number":116,"context_line":"                try:"},{"line_number":117,"context_line":"                    while True:"},{"line_number":118,"context_line":"                        self.api.idl.run()"},{"line_number":119,"context_line":"                        if self.api._ovs.cur_cfg \u003e\u003d next_cfg:"},{"line_number":120,"context_line":"                            self.reload_checks(txn)"},{"line_number":121,"context_line":"                            break"},{"line_number":122,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_356784ea","line":119,"range":{"start_line":119,"start_character":36,"end_line":119,"end_character":60},"updated":"2016-07-20 15:12:27.000000000","message":"Same as L79 - maybe is_current_cfg(next_cfg) method for api would make more sense? Or is api object methods strictly a \u0027methods talking to ovsdb\u0027?","commit_id":"2b0683f6ac7107193690a7b14e6ef42ef0568699"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"eae5e8fca7e9b40be17952fa36227873acc08f87","unresolved":false,"context_lines":[{"line_number":116,"context_line":"                try:"},{"line_number":117,"context_line":"                    while True:"},{"line_number":118,"context_line":"                        self.api.idl.run()"},{"line_number":119,"context_line":"                        if self.api._ovs.cur_cfg \u003e\u003d next_cfg:"},{"line_number":120,"context_line":"                            self.reload_checks(txn)"},{"line_number":121,"context_line":"                            break"},{"line_number":122,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_1ecffd29","line":119,"range":{"start_line":119,"start_character":36,"end_line":119,"end_character":60},"in_reply_to":"dada55a8_356784ea","updated":"2016-07-20 16:08:02.000000000","message":"Normally API methods were just mostly implementing the ABC. But adding a helper function or two should be fine. Looks like I for some reason defined _tables property and _ovs property, then used them mostly outside of the class anyway. I could probably just get rid of the underscores.","commit_id":"2b0683f6ac7107193690a7b14e6ef42ef0568699"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"0d99f0cb342294275b8795b737143d764a7a1311","unresolved":false,"context_lines":[{"line_number":122,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"},{"line_number":123,"context_line":"                        self.ovsdb_connection.poller.block()"},{"line_number":124,"context_line":"                except Exception as e:"},{"line_number":125,"context_line":"                    with excutils.save_and_reraise_exception():"},{"line_number":126,"context_line":"                        LOG.exception(e)"},{"line_number":127,"context_line":"            return [cmd.result for cmd in self.commands]"},{"line_number":128,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_75c3bded","line":125,"range":{"start_line":125,"start_character":34,"end_line":125,"end_character":60},"updated":"2016-07-20 15:12:27.000000000","message":"Won\u0027t this cause that traceback will be logged twice? Once by LOG.exception() and once when exception gets into the agent?","commit_id":"2b0683f6ac7107193690a7b14e6ef42ef0568699"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"eae5e8fca7e9b40be17952fa36227873acc08f87","unresolved":false,"context_lines":[{"line_number":122,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"},{"line_number":123,"context_line":"                        self.ovsdb_connection.poller.block()"},{"line_number":124,"context_line":"                except Exception as e:"},{"line_number":125,"context_line":"                    with excutils.save_and_reraise_exception():"},{"line_number":126,"context_line":"                        LOG.exception(e)"},{"line_number":127,"context_line":"            return [cmd.result for cmd in self.commands]"},{"line_number":128,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_7e539939","line":125,"range":{"start_line":125,"start_character":34,"end_line":125,"end_character":60},"in_reply_to":"dada55a8_75c3bded","updated":"2016-07-20 16:08:02.000000000","message":"Probably. I think I added this when I was using some scripts to test it out and was getting an error I wasn\u0027t seeing or something. I\u0027ll take a look.","commit_id":"2b0683f6ac7107193690a7b14e6ef42ef0568699"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"066069da7be54de573582bffd65f4d215e87b322","unresolved":false,"context_lines":[{"line_number":77,"context_line":"            # TODO(twilson) Make sure we don\u0027t loop longer than vsctl_timeout"},{"line_number":78,"context_line":"            txn \u003d idl.Transaction(self.api.idl)"},{"line_number":79,"context_line":"            # Cache ref to the set on txn since that\u0027s all we pass to run_idl"},{"line_number":80,"context_line":"            txn.expected_ifaces \u003d self.expected_ifaces"},{"line_number":81,"context_line":"            self.api._ovs.increment(\u0027next_cfg\u0027)"},{"line_number":82,"context_line":"            for i, command in enumerate(self.commands):"},{"line_number":83,"context_line":"                LOG.debug(\"Running txn command(idx\u003d%(idx)s): %(cmd)s\","}],"source_content_type":"text/x-python","patch_set":3,"id":"dada55a8_a2e2fd2c","line":80,"updated":"2016-07-21 01:38:04.000000000","message":"I don\u0027t particularly like this, but it does allow me to confine the change to the AddPortCommand and not change what is passed to run_idl.","commit_id":"c96ec78c7ca48338cce553cfe210bfb80acac0d9"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"9134b343bed0c52295c346245cca30681b540318","unresolved":false,"context_lines":[{"line_number":115,"context_line":"                try:"},{"line_number":116,"context_line":"                    while True:"},{"line_number":117,"context_line":"                        self.api.idl.run()"},{"line_number":118,"context_line":"                        if self.api._ovs.cur_cfg \u003e\u003d next_cfg:"},{"line_number":119,"context_line":"                            self.reload_checks(txn)"},{"line_number":120,"context_line":"                            break"},{"line_number":121,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"}],"source_content_type":"text/x-python","patch_set":3,"id":"dada55a8_0284a95b","line":118,"updated":"2016-07-21 01:35:19.000000000","message":"jakub: I may leave the ._ovs since I use ._table, and then change all of those in a separate patch.","commit_id":"c96ec78c7ca48338cce553cfe210bfb80acac0d9"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"9134b343bed0c52295c346245cca30681b540318","unresolved":false,"context_lines":[{"line_number":120,"context_line":"                            break"},{"line_number":121,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"},{"line_number":122,"context_line":"                        self.ovsdb_connection.poller.block()"},{"line_number":123,"context_line":"                except Exception as e:"},{"line_number":124,"context_line":"                    with excutils.save_and_reraise_exception():"},{"line_number":125,"context_line":"                        LOG.exception(e)"},{"line_number":126,"context_line":"            return [cmd.result for cmd in self.commands]"}],"source_content_type":"text/x-python","patch_set":3,"id":"dada55a8_22a8cdda","line":123,"updated":"2016-07-21 01:35:19.000000000","message":"Note to self to remember to look at removing the exception logging.","commit_id":"c96ec78c7ca48338cce553cfe210bfb80acac0d9"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"42896ea038dda1bf5ed489cad4690663e579aaa1","unresolved":false,"context_lines":[{"line_number":115,"context_line":"                try:"},{"line_number":116,"context_line":"                    while True:"},{"line_number":117,"context_line":"                        self.api.idl.run()"},{"line_number":118,"context_line":"                        if self.api._ovs.cur_cfg \u003e\u003d next_cfg:"},{"line_number":119,"context_line":"                            self.reload_checks(txn)"},{"line_number":120,"context_line":"                            break"},{"line_number":121,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"}],"source_content_type":"text/x-python","patch_set":5,"id":"dada55a8_f29c795f","line":118,"updated":"2016-07-22 09:31:08.000000000","message":"I apologize since I\u0027m not familiar with this code, but if the cur_cfg is greater than next_cfg, are we guarunteed that this is because of our transaction? \n\nConsider a network node where we have L3 agent, L2 agent, and DHCP agent. All 3 are using these transactions to add/configure interfaces on the same OVS instance. So if we request to increment and have a slow transaction and two other agents quickly increment, this number will increase and we will think everything is done.","commit_id":"9e79b60bc7380280a682667091935e77d9e51650"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"2bbfbfe2bd69525b52c63caaab48956e08ea4985","unresolved":false,"context_lines":[{"line_number":115,"context_line":"                try:"},{"line_number":116,"context_line":"                    while True:"},{"line_number":117,"context_line":"                        self.api.idl.run()"},{"line_number":118,"context_line":"                        if self.api._ovs.cur_cfg \u003e\u003d next_cfg:"},{"line_number":119,"context_line":"                            self.reload_checks(txn)"},{"line_number":120,"context_line":"                            break"},{"line_number":121,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"}],"source_content_type":"text/x-python","patch_set":5,"id":"dada55a8_8eb5fb7f","line":118,"in_reply_to":"dada55a8_235fd376","updated":"2016-07-22 17:25:09.000000000","message":"After talking to Terry, and looking at the vsctl code, the next_cfg++ \u0026 the change happen both inside a transactions, and ovs-vswitchd process those one after another in order,\n\nSo even in the case of several competing clients, it should be safe.\n\nAnyway I guess mimicking what ovs-vsctl does will be safe enough.","commit_id":"9e79b60bc7380280a682667091935e77d9e51650"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"2bef85e62200a446aced74a98598ffeafc9cab9d","unresolved":false,"context_lines":[{"line_number":115,"context_line":"                try:"},{"line_number":116,"context_line":"                    while True:"},{"line_number":117,"context_line":"                        self.api.idl.run()"},{"line_number":118,"context_line":"                        if self.api._ovs.cur_cfg \u003e\u003d next_cfg:"},{"line_number":119,"context_line":"                            self.reload_checks(txn)"},{"line_number":120,"context_line":"                            break"},{"line_number":121,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"}],"source_content_type":"text/x-python","patch_set":5,"id":"dada55a8_ba08f92e","line":118,"in_reply_to":"dada55a8_235fd376","updated":"2016-07-22 18:57:44.000000000","message":"To be more clear, whether or not ovsdb-server interleaves reads for multiple transactions at the same time, since each transaction we care about is modifying next_cfg, they cannot complete at the same time. Also, next_cfg is not incremented client-side from a read value, a \"mutate next_cfg +1\" operation is sent to increment whatever is there. So each agent\u0027s next_cfg value would be both different and ordered.\n\novs-vswitchd gets a notification that the db has changed. It then applies the whole database config, looping through deleting and adding bridges/ports. It may get a copy of the committed db state at any point, but the operations it takes will be consistent with that ordered state. After applying the changes, it will create a new transaction to set cur_cfg to the next_cfg value that was in that state copy.\n\nWhat happens if between vswitchd applying the config and updating the db\u0027s cur_cfg, another client sends a next_cfg increment? I think:\n\nclient1 sends ovsdb inc(next_cfg) -\u003e 11\nclient1 waits until cur_cfg \u003e\u003d 11\nvswitch_idl gets change notification\nvswitch_idl cur_cfg \u003d 10, next_cfg \u003d 11\nclient1 continues\nclient2 sends ovsdb inc(next_cfg) -\u003e 12\nclient2 waits until cur_cfg \u003e\u003d 12\nvswitch_idl applies config and sets cur_cfg \u003d next_cfg \u003d 11\nvswitch_idl gets change notification\nvswitch_idl cur_cfg \u003d 11, next_cfg \u003d 12\nvswitch_idl applies config and sets cur_cfg \u003d next_cfg \u003d 12\nclient2 continues\n\nOf course, if transaction 1 and 2 are processed in the same read for ovs_vswitchd, and 1 and 2 are add_port(foo) and del_port(foo), the state of the system does not reflect that port_foo exists or has an ofport. But, welcome to eventual consistency.\n\nWhat happens if there is a transaction failure with the incrementing cur_cfg? I honestly have no idea. It looks like it sets the status to try_again in case it fails, but I haven\u0027t yet traced that far to see what it does with that value.\n\ntl;dr Atomic db increment of both next_cfg and cur_cfg + ovs_vswitchd applying the whole snapshot should  \"guarantee\" that if cur_cfg \u003e\u003d next_cfg, our transaction\u0027s changes have been considered in normal operation. The database is the arbiter of what the state is.","commit_id":"9e79b60bc7380280a682667091935e77d9e51650"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"32dc7958ac7c295a8695acab3cad1227bab4b287","unresolved":false,"context_lines":[{"line_number":115,"context_line":"                try:"},{"line_number":116,"context_line":"                    while True:"},{"line_number":117,"context_line":"                        self.api.idl.run()"},{"line_number":118,"context_line":"                        if self.api._ovs.cur_cfg \u003e\u003d next_cfg:"},{"line_number":119,"context_line":"                            self.reload_checks(txn)"},{"line_number":120,"context_line":"                            break"},{"line_number":121,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"}],"source_content_type":"text/x-python","patch_set":5,"id":"dada55a8_ee9e3262","line":118,"in_reply_to":"dada55a8_ba08f92e","updated":"2016-07-22 19:08:28.000000000","message":"Oops move client1 continues down until after vswitch_idl applies config and sets cur_cfg \u003d next_cfg \u003d 11.","commit_id":"9e79b60bc7380280a682667091935e77d9e51650"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"68db74e3d7520c21961d9efb4f72c32295a4acaa","unresolved":false,"context_lines":[{"line_number":115,"context_line":"                try:"},{"line_number":116,"context_line":"                    while True:"},{"line_number":117,"context_line":"                        self.api.idl.run()"},{"line_number":118,"context_line":"                        if self.api._ovs.cur_cfg \u003e\u003d next_cfg:"},{"line_number":119,"context_line":"                            self.reload_checks(txn)"},{"line_number":120,"context_line":"                            break"},{"line_number":121,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"}],"source_content_type":"text/x-python","patch_set":5,"id":"dada55a8_932b5a33","line":118,"in_reply_to":"dada55a8_ee9e3262","updated":"2016-07-22 22:36:45.000000000","message":"Ack. Didn\u0027t realize it serialized all writes.","commit_id":"9e79b60bc7380280a682667091935e77d9e51650"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"fce40e3f970f76f36c37fb727b58bc6ebfc0ad5c","unresolved":false,"context_lines":[{"line_number":115,"context_line":"                try:"},{"line_number":116,"context_line":"                    while True:"},{"line_number":117,"context_line":"                        self.api.idl.run()"},{"line_number":118,"context_line":"                        if self.api._ovs.cur_cfg \u003e\u003d next_cfg:"},{"line_number":119,"context_line":"                            self.reload_checks(txn)"},{"line_number":120,"context_line":"                            break"},{"line_number":121,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"}],"source_content_type":"text/x-python","patch_set":5,"id":"dada55a8_235fd376","line":118,"in_reply_to":"dada55a8_f29c795f","updated":"2016-07-22 14:48:24.000000000","message":"ovsdb-server is single-threaded. It cannot simultaneously process multiple transactions from different connections. If we increment next_cfg, we should be guaranteed that if cur_cfg is \u003e\u003d next_cfg, that our change has been applied via vswitchd.","commit_id":"9e79b60bc7380280a682667091935e77d9e51650"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"42896ea038dda1bf5ed489cad4690663e579aaa1","unresolved":false,"context_lines":[{"line_number":135,"context_line":"                if iface and (not iface.ofport or iface.ofport \u003d\u003d -1):"},{"line_number":136,"context_line":"                    failed.append(iface_uuid)"},{"line_number":137,"context_line":"        if failed:"},{"line_number":138,"context_line":"            LOG.error(_LE(\"Failed to add interfaces: %(ifs)s\"),"},{"line_number":139,"context_line":"                      {\u0027ifs\u0027: \", \".join(failed)})"},{"line_number":140,"context_line":""},{"line_number":141,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"dada55a8_52a2a59b","line":138,"updated":"2016-07-22 09:31:08.000000000","message":"shouldn\u0027t we do something here other than logging (e.g. wait and retry several times)?","commit_id":"9e79b60bc7380280a682667091935e77d9e51650"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"fce40e3f970f76f36c37fb727b58bc6ebfc0ad5c","unresolved":false,"context_lines":[{"line_number":135,"context_line":"                if iface and (not iface.ofport or iface.ofport \u003d\u003d -1):"},{"line_number":136,"context_line":"                    failed.append(iface_uuid)"},{"line_number":137,"context_line":"        if failed:"},{"line_number":138,"context_line":"            LOG.error(_LE(\"Failed to add interfaces: %(ifs)s\"),"},{"line_number":139,"context_line":"                      {\u0027ifs\u0027: \", \".join(failed)})"},{"line_number":140,"context_line":""},{"line_number":141,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"dada55a8_56a44707","line":138,"in_reply_to":"dada55a8_52a2a59b","updated":"2016-07-22 14:48:24.000000000","message":"My goal with the native ovsdb implementation has always been to mirror the behavior of the ovs-vsctl implementation as closely as possible. This is the behavior of ovs-vsctl: no error returned (transaction was successful, after all) but log the error.\n\nBut, we also do retry in practice as long as the ovs_lib functions add_port* functions are used since we actually pull back the ofport with retries before returning. Interfaces created as a result of a creating a bridge could theoretically show up here without hitting the retry logic (as a result of this patch). But something very seriously wrong would have to happen for that retry to ever take place though (and I\u0027m not even sure what that would be).","commit_id":"9e79b60bc7380280a682667091935e77d9e51650"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"9d4ed8799a8e8fd65a715a45fd9db0aeacf181d4","unresolved":false,"context_lines":[{"line_number":119,"context_line":"                            self.reload_checks(txn)"},{"line_number":120,"context_line":"                            break"},{"line_number":121,"context_line":"                        self.api.idl.wait(self.ovsdb_connection.poller)"},{"line_number":122,"context_line":"                        self.ovsdb_connection.poller.block()"},{"line_number":123,"context_line":"                except Exception as e:"},{"line_number":124,"context_line":"                    with excutils.save_and_reraise_exception():"},{"line_number":125,"context_line":"                        LOG.exception(e)"}],"source_content_type":"text/x-python","patch_set":6,"id":"dada55a8_ed772139","line":122,"range":{"start_line":122,"start_character":53,"end_line":122,"end_character":58},"updated":"2016-07-25 14:24:21.000000000","message":"I wonder whether this will bring back my favourite https://bugs.launchpad.net/bugs/1567668 :)","commit_id":"9eb61ac6c712fa99fbb938e475d2e1253790ea5f"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"67af90e82e44fa9fa58f810491753c11dfc94ae9","unresolved":false,"context_lines":[{"line_number":137,"context_line":"            uuid \u003d txn.get_insert_uuid(iface_uuid)"},{"line_number":138,"context_line":"            if uuid:"},{"line_number":139,"context_line":"                ifaces \u003d self.api.idl.tables[\u0027Interface\u0027]"},{"line_number":140,"context_line":"                iface \u003d ifaces.rows.get(uuid, None)"},{"line_number":141,"context_line":"                if iface and (not iface.ofport or iface.ofport \u003d\u003d -1):"},{"line_number":142,"context_line":"                    failed.append(iface_uuid)"},{"line_number":143,"context_line":"        if failed:"}],"source_content_type":"text/x-python","patch_set":8,"id":"dada55a8_6164a8b8","line":140,"updated":"2016-07-26 11:26:07.000000000","message":"by default get returns None","commit_id":"f86777431a6e5902defee46b249271c1ffb432d4"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"61db759dd31e41ccc71f55487c6a384ede1f4488","unresolved":false,"context_lines":[{"line_number":39,"context_line":""},{"line_number":40,"context_line":"class Transaction(api.Transaction):"},{"line_number":41,"context_line":"    def __init__(self, api, ovsdb_connection, timeout,"},{"line_number":42,"context_line":"                 check_error\u003dFalse, log_errors\u003dTrue):"},{"line_number":43,"context_line":"        self.api \u003d api"},{"line_number":44,"context_line":"        self.check_error \u003d check_error"},{"line_number":45,"context_line":"        self.log_errors \u003d log_errors"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_ef9483e9","line":42,"updated":"2016-07-27 21:57:13.000000000","message":"I don\u0027t know if we want this or not, but this doesn\u0027t seem related to the patch?","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"6ec34bff3303d22b654da706f645f82e54568f6d","unresolved":false,"context_lines":[{"line_number":39,"context_line":""},{"line_number":40,"context_line":"class Transaction(api.Transaction):"},{"line_number":41,"context_line":"    def __init__(self, api, ovsdb_connection, timeout,"},{"line_number":42,"context_line":"                 check_error\u003dFalse, log_errors\u003dTrue):"},{"line_number":43,"context_line":"        self.api \u003d api"},{"line_number":44,"context_line":"        self.check_error \u003d check_error"},{"line_number":45,"context_line":"        self.log_errors \u003d log_errors"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_e12f779f","line":42,"in_reply_to":"bacf61ea_418803e8","updated":"2016-07-27 22:07:08.000000000","message":"I don\u0027t object to lumping that in to this patch, hopefully other people won\u0027t notice :)","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"4105d9804d248197f5c8d59176d3536c58dc3b04","unresolved":false,"context_lines":[{"line_number":39,"context_line":""},{"line_number":40,"context_line":"class Transaction(api.Transaction):"},{"line_number":41,"context_line":"    def __init__(self, api, ovsdb_connection, timeout,"},{"line_number":42,"context_line":"                 check_error\u003dFalse, log_errors\u003dTrue):"},{"line_number":43,"context_line":"        self.api \u003d api"},{"line_number":44,"context_line":"        self.check_error \u003d check_error"},{"line_number":45,"context_line":"        self.log_errors \u003d log_errors"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_418803e8","line":42,"in_reply_to":"bacf61ea_ef9483e9","updated":"2016-07-27 22:04:39.000000000","message":"It is closely related to the check_error fix that is necessary to the patch. The vsctl version defaults log_errors\u003dTrue. I suppose ideally the check_error/log_errors fixes would be in a separate patch that this patch depends on but I was hoping to avoid that. :p","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"61db759dd31e41ccc71f55487c6a384ede1f4488","unresolved":false,"context_lines":[{"line_number":150,"context_line":"        except Exception as e:"},{"line_number":151,"context_line":"            with excutils.save_and_reraise_exception() as ctx:"},{"line_number":152,"context_line":"                ctx.reraise \u003d False"},{"line_number":153,"context_line":"                LOG.exception(e)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def do_post_commit(self, txn):"},{"line_number":156,"context_line":"        next_cfg \u003d txn.get_increment_new_value()"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_ef492390","line":153,"updated":"2016-07-27 21:57:13.000000000","message":"Am I crazy or can you just delete lines 151 and 152?","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"ad7930f21beb48b3ae104819617a6785f8ee4942","unresolved":false,"context_lines":[{"line_number":150,"context_line":"        except Exception as e:"},{"line_number":151,"context_line":"            with excutils.save_and_reraise_exception() as ctx:"},{"line_number":152,"context_line":"                ctx.reraise \u003d False"},{"line_number":153,"context_line":"                LOG.exception(e)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def do_post_commit(self, txn):"},{"line_number":156,"context_line":"        next_cfg \u003d txn.get_increment_new_value()"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_442bf130","line":153,"in_reply_to":"bacf61ea_21186f84","updated":"2016-07-27 22:23:13.000000000","message":"Eventlet coroutine switching is the reason we using excutils. A coroutine switch to another one that throws and exception can mess up the traceback info. But since we aren\u0027t referencing the traceback info on the ctx object, it\u0027s not buying us anything. Just dump it.\n\n\nAlso, LOG.exception() already dumps the traceback. The arg passed into it should be some other type of message, e.g. \"Failure in do_post_commit\".","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"6ec34bff3303d22b654da706f645f82e54568f6d","unresolved":false,"context_lines":[{"line_number":150,"context_line":"        except Exception as e:"},{"line_number":151,"context_line":"            with excutils.save_and_reraise_exception() as ctx:"},{"line_number":152,"context_line":"                ctx.reraise \u003d False"},{"line_number":153,"context_line":"                LOG.exception(e)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def do_post_commit(self, txn):"},{"line_number":156,"context_line":"        next_cfg \u003d txn.get_increment_new_value()"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_21186f84","line":153,"in_reply_to":"bacf61ea_81515b65","updated":"2016-07-27 22:07:08.000000000","message":"*Shrugs*","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"4105d9804d248197f5c8d59176d3536c58dc3b04","unresolved":false,"context_lines":[{"line_number":150,"context_line":"        except Exception as e:"},{"line_number":151,"context_line":"            with excutils.save_and_reraise_exception() as ctx:"},{"line_number":152,"context_line":"                ctx.reraise \u003d False"},{"line_number":153,"context_line":"                LOG.exception(e)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    def do_post_commit(self, txn):"},{"line_number":156,"context_line":"        next_cfg \u003d txn.get_increment_new_value()"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_81515b65","line":153,"in_reply_to":"bacf61ea_ef492390","updated":"2016-07-27 22:04:39.000000000","message":"For some reason I was thinking that we weren\u0027t supposed to just catch an exception and LOG due to some technical reason that I cannot remember.","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"61db759dd31e41ccc71f55487c6a384ede1f4488","unresolved":false,"context_lines":[{"line_number":167,"context_line":"            self.api.idl.wait(self.ovsdb_connection.poller)"},{"line_number":168,"context_line":"            self.ovsdb_connection.poller.block()"},{"line_number":169,"context_line":"        else:"},{"line_number":170,"context_line":"            raise api.TimeoutException(_(\"Timed out during post-commit wait\"))"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def post_commit_failed_interfaces(self, txn):"},{"line_number":173,"context_line":"        failed \u003d []"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_4f094f31","line":170,"updated":"2016-07-27 21:57:13.000000000","message":"Should we spit out tnx.commands and self.timeout here?","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"4105d9804d248197f5c8d59176d3536c58dc3b04","unresolved":false,"context_lines":[{"line_number":167,"context_line":"            self.api.idl.wait(self.ovsdb_connection.poller)"},{"line_number":168,"context_line":"            self.ovsdb_connection.poller.block()"},{"line_number":169,"context_line":"        else:"},{"line_number":170,"context_line":"            raise api.TimeoutException(_(\"Timed out during post-commit wait\"))"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def post_commit_failed_interfaces(self, txn):"},{"line_number":173,"context_line":"        failed \u003d []"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_21c7cf13","line":170,"in_reply_to":"bacf61ea_4f094f31","updated":"2016-07-27 22:04:39.000000000","message":"Wouldn\u0027t hurt.","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"}],"neutron/agent/ovsdb/native/commands.py":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"0d99f0cb342294275b8795b737143d764a7a1311","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        br.ports \u003d ports"},{"line_number":305,"context_line":""},{"line_number":306,"context_line":"        iface \u003d txn.insert(self.api._tables[\u0027Interface\u0027])"},{"line_number":307,"context_line":"        self.iface_uuid \u003d iface.__dict__[\u0027uuid\u0027]"},{"line_number":308,"context_line":"        iface.name \u003d self.port"},{"line_number":309,"context_line":"        port.verify(\u0027interfaces\u0027)"},{"line_number":310,"context_line":"        ifaces \u003d getattr(port, \u0027interfaces\u0027, [])"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_3502e50b","line":307,"range":{"start_line":307,"start_character":26,"end_line":307,"end_character":48},"updated":"2016-07-20 15:12:27.000000000","message":"Just out of curiosity, how is it different from iface.uuid ?","commit_id":"2b0683f6ac7107193690a7b14e6ef42ef0568699"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"eae5e8fca7e9b40be17952fa36227873acc08f87","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        br.ports \u003d ports"},{"line_number":305,"context_line":""},{"line_number":306,"context_line":"        iface \u003d txn.insert(self.api._tables[\u0027Interface\u0027])"},{"line_number":307,"context_line":"        self.iface_uuid \u003d iface.__dict__[\u0027uuid\u0027]"},{"line_number":308,"context_line":"        iface.name \u003d self.port"},{"line_number":309,"context_line":"        port.verify(\u0027interfaces\u0027)"},{"line_number":310,"context_line":"        ifaces \u003d getattr(port, \u0027interfaces\u0027, [])"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_5b0ea73b","line":307,"range":{"start_line":307,"start_character":26,"end_line":307,"end_character":48},"in_reply_to":"dada55a8_3502e50b","updated":"2016-07-20 16:08:02.000000000","message":"iface.uuid would go through Row.__getattr__ which, since insert() calls  Row(self.idl, table, new_uuid, None), _changes \u003d\u003d {} and _data \u003d\u003d None, which would produce an error. (I think!) It is one of the uglinesses in the OVS python library that I\u0027m not fond of.\n\nOn a side note, I think I\u0027ll do txn.expected_ifaces.add(...) instead of storing it on the command itself. /me investigates.","commit_id":"2b0683f6ac7107193690a7b14e6ef42ef0568699"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"eabdfbcd1220de62f3979479b0fbcf5a2fbcb48e","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        br.ports \u003d ports"},{"line_number":305,"context_line":""},{"line_number":306,"context_line":"        iface \u003d txn.insert(self.api._tables[\u0027Interface\u0027])"},{"line_number":307,"context_line":"        self.iface_uuid \u003d iface.__dict__[\u0027uuid\u0027]"},{"line_number":308,"context_line":"        iface.name \u003d self.port"},{"line_number":309,"context_line":"        port.verify(\u0027interfaces\u0027)"},{"line_number":310,"context_line":"        ifaces \u003d getattr(port, \u0027interfaces\u0027, [])"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_d011b25e","line":307,"range":{"start_line":307,"start_character":26,"end_line":307,"end_character":48},"in_reply_to":"dada55a8_5b0ea73b","updated":"2016-07-21 08:32:09.000000000","message":"Thanks for explanation :)","commit_id":"2b0683f6ac7107193690a7b14e6ef42ef0568699"}],"neutron/tests/functional/agent/ovsdb/test_impl_idl.py":[{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"7520dac53263a832bda36e6305f197c726ce0056","unresolved":false,"context_lines":[{"line_number":23,"context_line":"from neutron.tests.functional import base"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"# For the purposes of these tests, we need the exceptions to pass through"},{"line_number":26,"context_line":"impl_idl.NeutronOVSDBTransaction.post_commit \u003d \\"},{"line_number":27,"context_line":"    impl_idl.NeutronOVSDBTransaction.do_post_commit"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"bacf61ea_5409e230","line":26,"updated":"2016-07-27 21:21:23.000000000","message":"I fear this may override for all tests in the current worker process. We\u0027re probably better off using mock that will unmock automatically at the end of the test.","commit_id":"6da1806a8b2ae96b8589e6aa1d21b050fbf6370f"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"61db759dd31e41ccc71f55487c6a384ede1f4488","unresolved":false,"context_lines":[{"line_number":39,"context_line":"        self.brname \u003d test_base.get_rand_device_name(net_helpers.BR_PREFIX)"},{"line_number":40,"context_line":"        # Make sure exceptions pass through by calling do_post_commit directly"},{"line_number":41,"context_line":"        mock.patch.object("},{"line_number":42,"context_line":"            impl_idl.NeutronOVSDBTransaction, \"post_commit\","},{"line_number":43,"context_line":"            side_effect\u003dimpl_idl.NeutronOVSDBTransaction.do_post_commit,"},{"line_number":44,"context_line":"            autospec\u003dTrue).start()"},{"line_number":45,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_c1eeb3eb","line":42,"updated":"2016-07-27 21:57:13.000000000","message":"One code path we don\u0027t have coverage for is the exceptional case in post_commit. For that we can just create a dumb unit test that makes do_post_commit raise an exception. The test wouldn\u0027t make assertions it\u0027d just run the code.","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"a727cafe9b005d4ff28e49fc984b476615eca5ad","unresolved":false,"context_lines":[{"line_number":56,"context_line":"        self._add_br()"},{"line_number":57,"context_line":"        ofport \u003d self.ovs.db_get_val(\"Interface\", self.brname, \"ofport\")"},{"line_number":58,"context_line":"        self.assertTrue(int(ofport))"},{"line_number":59,"context_line":"        self.assertTrue(ofport \u003e -1)"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    def test_post_commit_vswitchd_completed_no_failures(self):"},{"line_number":62,"context_line":"        self._add_br_and_test()"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_2f44db72","line":59,"updated":"2016-07-27 21:53:30.000000000","message":"this assertion implies ofport is already an integer, why the conversion on L58? And if L58 is asserting that it\u0027s non-zero, why not just get rid of it completely and make this \u0027\u003e 0\u0027 ?","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"c04d4e8c1e1bd249b75191496ecfe61f358ae6fa","unresolved":false,"context_lines":[{"line_number":56,"context_line":"        self._add_br()"},{"line_number":57,"context_line":"        ofport \u003d self.ovs.db_get_val(\"Interface\", self.brname, \"ofport\")"},{"line_number":58,"context_line":"        self.assertTrue(int(ofport))"},{"line_number":59,"context_line":"        self.assertTrue(ofport \u003e -1)"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"    def test_post_commit_vswitchd_completed_no_failures(self):"},{"line_number":62,"context_line":"        self._add_br_and_test()"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_e176573d","line":59,"in_reply_to":"bacf61ea_2f44db72","updated":"2016-07-27 21:59:04.000000000","message":"Unfortunately, it doesn\u0027t. [] \u003e 0.","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"a727cafe9b005d4ff28e49fc984b476615eca5ad","unresolved":false,"context_lines":[{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    @trpatch(\"vswitchd_has_completed\", return_value\u003dTrue)"},{"line_number":65,"context_line":"    @trpatch(\"post_commit_failed_interfaces\", return_value\u003d[\"failed_if1\"])"},{"line_number":66,"context_line":"    @trpatch(\"timeout_exceeded\", return_value\u003dFalse)"},{"line_number":67,"context_line":"    def test_post_commit_vswitchd_completed_failures(self, *args):"},{"line_number":68,"context_line":"        self.assertRaises(impl_idl.VswitchdInterfaceAddException, self._add_br)"},{"line_number":69,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_a1e73f11","line":66,"updated":"2016-07-27 21:53:30.000000000","message":"By patching the module itself and checking that specific exceptions are raised, I\u0027m not sure I would call these functional tests. We do add an actual bridge, but all of the failures are purely synthetic inside of the impl_idl module as a result of monkey patches rather than the behavior of OVSDB itself.\n\nWhat does doing this as a functional test actually buying us?","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"ad7930f21beb48b3ae104819617a6785f8ee4942","unresolved":false,"context_lines":[{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    @trpatch(\"vswitchd_has_completed\", return_value\u003dTrue)"},{"line_number":65,"context_line":"    @trpatch(\"post_commit_failed_interfaces\", return_value\u003d[\"failed_if1\"])"},{"line_number":66,"context_line":"    @trpatch(\"timeout_exceeded\", return_value\u003dFalse)"},{"line_number":67,"context_line":"    def test_post_commit_vswitchd_completed_failures(self, *args):"},{"line_number":68,"context_line":"        self.assertRaises(impl_idl.VswitchdInterfaceAddException, self._add_br)"},{"line_number":69,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_c48d217d","line":66,"in_reply_to":"bacf61ea_a1d1fffd","updated":"2016-07-27 22:23:13.000000000","message":"Well these are effectively unit tests, the only mocks missing are the ones setting the return values from add_br and db_get_val. :)","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"c04d4e8c1e1bd249b75191496ecfe61f358ae6fa","unresolved":false,"context_lines":[{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    @trpatch(\"vswitchd_has_completed\", return_value\u003dTrue)"},{"line_number":65,"context_line":"    @trpatch(\"post_commit_failed_interfaces\", return_value\u003d[\"failed_if1\"])"},{"line_number":66,"context_line":"    @trpatch(\"timeout_exceeded\", return_value\u003dFalse)"},{"line_number":67,"context_line":"    def test_post_commit_vswitchd_completed_failures(self, *args):"},{"line_number":68,"context_line":"        self.assertRaises(impl_idl.VswitchdInterfaceAddException, self._add_br)"},{"line_number":69,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_a1d1fffd","line":66,"in_reply_to":"bacf61ea_a1e73f11","updated":"2016-07-27 21:59:04.000000000","message":"It is the only way to ensure that the exceptional cases in the code are actually called and therefor don\u0027t contain syntax errors. It is also the only way to get people to accept the patch. :p\n\nUnit tests just can\u0027t really work here unfortunately. There\u0027s just too much stuff that happens behind the scenes.","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"61db759dd31e41ccc71f55487c6a384ede1f4488","unresolved":false,"context_lines":[{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    @trpatch(\"vswitchd_has_completed\", return_value\u003dTrue)"},{"line_number":65,"context_line":"    @trpatch(\"post_commit_failed_interfaces\", return_value\u003d[\"failed_if1\"])"},{"line_number":66,"context_line":"    @trpatch(\"timeout_exceeded\", return_value\u003dFalse)"},{"line_number":67,"context_line":"    def test_post_commit_vswitchd_completed_failures(self, *args):"},{"line_number":68,"context_line":"        self.assertRaises(impl_idl.VswitchdInterfaceAddException, self._add_br)"},{"line_number":69,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_c1aa3370","line":66,"in_reply_to":"bacf61ea_a1e73f11","updated":"2016-07-27 21:57:13.000000000","message":"Terry and I spend a lot of time on IRC and on the phone to try to get this right. The goal was pretty humble: Get coverage for the successful and exceptional cases, which we figured would be better than nothing. Going for pure unit tests would have resulted in a mock-heavy disaster and this is the best we could come up with. No these aren\u0027t similar to other functional tests we have, but since there is system interaction here it needs to be in the functional test tree.","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"8c86ca6cfdcc33bb06eed0a31bee18c30205ef8d","unresolved":false,"context_lines":[{"line_number":64,"context_line":"    @trpatch(\"vswitchd_has_completed\", return_value\u003dTrue)"},{"line_number":65,"context_line":"    @trpatch(\"post_commit_failed_interfaces\", return_value\u003d[\"failed_if1\"])"},{"line_number":66,"context_line":"    @trpatch(\"timeout_exceeded\", return_value\u003dFalse)"},{"line_number":67,"context_line":"    def test_post_commit_vswitchd_completed_failures(self, *args):"},{"line_number":68,"context_line":"        self.assertRaises(impl_idl.VswitchdInterfaceAddException, self._add_br)"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"    @trpatch(\"vswitchd_has_completed\", return_value\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":12,"id":"bacf61ea_61fee76c","line":67,"updated":"2016-07-27 21:58:27.000000000","message":"I went through the code and the tests and it looks like we have full coverage, apart from the comment I left in line 42.","commit_id":"a9802feccf32e8b0c561e6434a46b31b53af8068"}]}