)]}'
{"kuryr_kubernetes/cni/daemon/service.py":[{"author":{"_account_id":28082,"name":"Yash Gupta","email":"y.gupta@samsung.com","username":"y.gupta"},"change_message_id":"1a6fdd6125d72fc8d6b861e3927ac7b24999c9ab","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            LOG.error(\"Timed out waiting for requested pod to appear in \""},{"line_number":87,"context_line":"                      \"registry: %s.\", e)"},{"line_number":88,"context_line":"            return \u0027\u0027, httplib.GATEWAY_TIMEOUT, self.headers"},{"line_number":89,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":90,"context_line":"            LOG.error(\"Unable to execute ovs-vsctl command: %s.\", e)"},{"line_number":91,"context_line":"            with self.healthy.get_lock():"},{"line_number":92,"context_line":"                self.healthy.value \u003d False"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_d0dcb446","line":89,"range":{"start_line":89,"start_character":7,"end_line":89,"end_character":55},"updated":"2019-08-30 05:29:49.000000000","message":"Are we sure that all ProcessExecutionError exceptions will come from ovs only? For example, sriov driver exec\u0027s \"ip link set ..\" kind of commands that can fail and generate same exception. Assuming it to be ovs-vsctl is maybe not really correct.\n\nI think service.py should have general exception handling, which is common to all drivers.","commit_id":"9ae4f40e6ae1cd654870ae250e15fe1780042db9"},{"author":{"_account_id":29615,"name":"Nayan Deshmukh","email":"n.deshmukh@samsung.com","username":"n.deshmukh"},"change_message_id":"3e0c0fe4c049628ba0510bb26f10819eaa0d36a0","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            LOG.error(\"Timed out waiting for requested pod to appear in \""},{"line_number":87,"context_line":"                      \"registry: %s.\", e)"},{"line_number":88,"context_line":"            return \u0027\u0027, httplib.GATEWAY_TIMEOUT, self.headers"},{"line_number":89,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":90,"context_line":"            LOG.error(\"Unable to execute ovs-vsctl command: %s.\", e)"},{"line_number":91,"context_line":"            with self.healthy.get_lock():"},{"line_number":92,"context_line":"                self.healthy.value \u003d False"}],"source_content_type":"text/x-python","patch_set":1,"id":"dfbec78f_edda62e7","line":89,"range":{"start_line":89,"start_character":28,"end_line":89,"end_character":49},"updated":"2019-05-09 05:04:29.000000000","message":"It might be better to use a custom exception such OVSExecutionError. But I am not sure if it is really needed.","commit_id":"9ae4f40e6ae1cd654870ae250e15fe1780042db9"},{"author":{"_account_id":28082,"name":"Yash Gupta","email":"y.gupta@samsung.com","username":"y.gupta"},"change_message_id":"cd68dfb51979958525ad7606cd8829ff0013e2ce","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            LOG.error(\"Timed out waiting for requested pod to appear in \""},{"line_number":87,"context_line":"                      \"registry: %s.\", e)"},{"line_number":88,"context_line":"            return \u0027\u0027, httplib.GATEWAY_TIMEOUT, self.headers"},{"line_number":89,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":90,"context_line":"            LOG.error(\"Unable to execute ovs-vsctl command: %s.\", e)"},{"line_number":91,"context_line":"            with self.healthy.get_lock():"},{"line_number":92,"context_line":"                self.healthy.value \u003d False"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_55564f07","line":89,"range":{"start_line":89,"start_character":7,"end_line":89,"end_character":55},"in_reply_to":"7faddb67_0b50dd31","updated":"2019-09-03 05:40:55.000000000","message":"If we simply print a generic message, then this doesn\u0027t add too much in addition to what default Exception handler below provides (_check_failure() will eventually mark cni unhealthy).\n\nIn case of sriov binding driver, error may be caused due to factors outside of kuryr cni (like race condition or timeout in iproute2 commands, driver errors, etc), in which case immediately marking ourselves unhealthy might not be desirable.\n\nI think we should catch specific exceptions where they are generated (ovs binding driver) and wrap them in more general categories for handling here.","commit_id":"9ae4f40e6ae1cd654870ae250e15fe1780042db9"},{"author":{"_account_id":29615,"name":"Nayan Deshmukh","email":"n.deshmukh@samsung.com","username":"n.deshmukh"},"change_message_id":"e6d21a8a919cb570611a8a1d2545313e95634e75","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            LOG.error(\"Timed out waiting for requested pod to appear in \""},{"line_number":87,"context_line":"                      \"registry: %s.\", e)"},{"line_number":88,"context_line":"            return \u0027\u0027, httplib.GATEWAY_TIMEOUT, self.headers"},{"line_number":89,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":90,"context_line":"            LOG.error(\"Unable to execute ovs-vsctl command: %s.\", e)"},{"line_number":91,"context_line":"            with self.healthy.get_lock():"},{"line_number":92,"context_line":"                self.healthy.value \u003d False"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_0b50dd31","line":89,"range":{"start_line":89,"start_character":7,"end_line":89,"end_character":55},"in_reply_to":"7faddb67_aba509b6","updated":"2019-08-30 08:10:56.000000000","message":"I can make the LOG message to be an generic error assuming that the code from where the exception came also prints the error message. This happens in case of OVS but I am not sure about sriov.\n\nIs that acceptable? Or can we deal with it in another way?","commit_id":"9ae4f40e6ae1cd654870ae250e15fe1780042db9"},{"author":{"_account_id":11600,"name":"Michał Dulko","email":"michal.dulko@gmail.com","username":"dulek"},"change_message_id":"91fb8b6e1a5ac18dad471be19b25d650e5a374f2","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            LOG.error(\"Timed out waiting for requested pod to appear in \""},{"line_number":87,"context_line":"                      \"registry: %s.\", e)"},{"line_number":88,"context_line":"            return \u0027\u0027, httplib.GATEWAY_TIMEOUT, self.headers"},{"line_number":89,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":90,"context_line":"            LOG.error(\"Unable to execute ovs-vsctl command: %s.\", e)"},{"line_number":91,"context_line":"            with self.healthy.get_lock():"},{"line_number":92,"context_line":"                self.healthy.value \u003d False"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_aba509b6","line":89,"range":{"start_line":89,"start_character":7,"end_line":89,"end_character":55},"in_reply_to":"7faddb67_d0dcb446","updated":"2019-08-30 07:25:06.000000000","message":"That\u0027s a fair point.","commit_id":"9ae4f40e6ae1cd654870ae250e15fe1780042db9"},{"author":{"_account_id":11600,"name":"Michał Dulko","email":"michal.dulko@gmail.com","username":"dulek"},"change_message_id":"2d33918e7563e9cc389a235eb1ccb6349f439dd6","unresolved":false,"context_lines":[{"line_number":87,"context_line":"                      \"registry: %s.\", e)"},{"line_number":88,"context_line":"            return \u0027\u0027, httplib.GATEWAY_TIMEOUT, self.headers"},{"line_number":89,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":90,"context_line":"            LOG.error(\"Unable to execute ovs-vsctl command: %s.\", e)"},{"line_number":91,"context_line":"            with self.healthy.get_lock():"},{"line_number":92,"context_line":"                self.healthy.value \u003d False"},{"line_number":93,"context_line":"            return \u0027\u0027, httplib.INTERNAL_SERVER_ERROR, self.headers"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_6ca2ebaf","line":90,"range":{"start_line":90,"start_character":0,"end_line":90,"end_character":68},"updated":"2019-08-28 15:22:14.000000000","message":"LOG.exception would be better. Then you don\u0027t need to include exception in the message, it will get added automatically.","commit_id":"9ae4f40e6ae1cd654870ae250e15fe1780042db9"},{"author":{"_account_id":11600,"name":"Michał Dulko","email":"michal.dulko@gmail.com","username":"dulek"},"change_message_id":"2d33918e7563e9cc389a235eb1ccb6349f439dd6","unresolved":false,"context_lines":[{"line_number":118,"context_line":"                        \"registry: %s. Ignoring.\", e)"},{"line_number":119,"context_line":"            return \u0027\u0027, httplib.NO_CONTENT, self.headers"},{"line_number":120,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":121,"context_line":"            LOG.error(\"Unable to execute ovs-vsctl command: %s.\", e)"},{"line_number":122,"context_line":"            with self.healthy.get_lock():"},{"line_number":123,"context_line":"                self.healthy.value \u003d False"},{"line_number":124,"context_line":"            return \u0027\u0027, httplib.INTERNAL_SERVER_ERROR, self.headers"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_0ca9f78e","line":121,"range":{"start_line":121,"start_character":0,"end_line":121,"end_character":68},"updated":"2019-08-28 15:22:14.000000000","message":"Same here.","commit_id":"9ae4f40e6ae1cd654870ae250e15fe1780042db9"},{"author":{"_account_id":11600,"name":"Michał Dulko","email":"michal.dulko@gmail.com","username":"dulek"},"change_message_id":"cac4f34b929eae60831784b8134ebeefe4443178","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            LOG.error(\u0027Error when processing addNetwork request\u0027)"},{"line_number":87,"context_line":"            return \u0027\u0027, httplib.GATEWAY_TIMEOUT, self.headers"},{"line_number":88,"context_line":"        except processutils.ProcessExecutionError:"},{"line_number":89,"context_line":"            LOG.exception(\u0027Unable to execute ovs-vsctl command\u0027)"},{"line_number":90,"context_line":"            with self.healthy.get_lock():"},{"line_number":91,"context_line":"                self.healthy.value \u003d False"},{"line_number":92,"context_line":"            return \u0027\u0027, httplib.INTERNAL_SERVER_ERROR, self.headers"},{"line_number":93,"context_line":"        except Exception:"},{"line_number":94,"context_line":"            self._check_failure()"},{"line_number":95,"context_line":"            LOG.exception(\u0027Error when processing addNetwork request. CNI \u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa7e38b_24a35c45","line":92,"range":{"start_line":89,"start_character":0,"end_line":92,"end_character":66},"updated":"2019-11-20 15:38:22.000000000","message":"Can you make sure code checks the exception\u0027s cmd [1] to make sure it\u0027s ovs-vsctl? Apparently it\u0027s a string, so simple startswith(\u0027ovs-vsctl\u0027) should be enough (it\u0027s hardcoded at [2]):\n\n In [1]: from oslo_concurrency import processutils\n\n In [2]: try:\n    ...:     processutils.execute(\u0027bash -c \"exit 1\"\u0027, shell\u003dTrue)\n    ...: except processutils.ProcessExecutionError as e:\n    ...:     print e.cmd\n    ...:     print type(e.cmd)\n    ...:     \n bash -c \"exit 1\"\n \u003ctype \u0027unicode\u0027\u003e\n\n\n[1] https://docs.openstack.org/oslo.concurrency/ocata/api/processutils.html#oslo_concurrency.processutils.ProcessExecutionError\n[2] https://github.com/openstack/os-vif/blob/0c6a21c06406ca942c92e39a4a2b3fab61fefb60/vif_plug_ovs/ovsdb/impl_vsctl.py#L100","commit_id":"d5698457897ed178c38c8d1bf18315e61b8b58b1"},{"author":{"_account_id":11600,"name":"Michał Dulko","email":"michal.dulko@gmail.com","username":"dulek"},"change_message_id":"cac4f34b929eae60831784b8134ebeefe4443178","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                        \u0027Ignoring this error, pod is most likely gone\u0027)"},{"line_number":118,"context_line":"            return \u0027\u0027, httplib.NO_CONTENT, self.headers"},{"line_number":119,"context_line":"        except processutils.ProcessExecutionError:"},{"line_number":120,"context_line":"            LOG.exception(\u0027Unable to execute ovs-vsctl command\u0027)"},{"line_number":121,"context_line":"            with self.healthy.get_lock():"},{"line_number":122,"context_line":"                self.healthy.value \u003d False"},{"line_number":123,"context_line":"            return \u0027\u0027, httplib.INTERNAL_SERVER_ERROR, self.headers"},{"line_number":124,"context_line":"        except Exception:"},{"line_number":125,"context_line":"            self._check_failure()"},{"line_number":126,"context_line":"            LOG.exception(\u0027Error when processing delNetwork request. CNI \u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa7e38b_c4612813","line":123,"range":{"start_line":120,"start_character":0,"end_line":123,"end_character":66},"updated":"2019-11-20 15:38:22.000000000","message":"Same here","commit_id":"d5698457897ed178c38c8d1bf18315e61b8b58b1"},{"author":{"_account_id":28082,"name":"Yash Gupta","email":"y.gupta@samsung.com","username":"y.gupta"},"change_message_id":"7e2cd2720c8f9523a6da181ebf83fc97b8f2516d","unresolved":false,"context_lines":[{"line_number":85,"context_line":"            self._check_failure()"},{"line_number":86,"context_line":"            LOG.error(\u0027Error when processing addNetwork request\u0027)"},{"line_number":87,"context_line":"            return \u0027\u0027, httplib.GATEWAY_TIMEOUT, self.headers"},{"line_number":88,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":89,"context_line":"            if e.cmd.startswith(\u0027ovs-vsctl\u0027):"},{"line_number":90,"context_line":"                LOG.exception(\u0027Unable to execute ovs-vsctl command\u0027)"},{"line_number":91,"context_line":"            with self.healthy.get_lock():"},{"line_number":92,"context_line":"                self.healthy.value \u003d False"},{"line_number":93,"context_line":"            return \u0027\u0027, httplib.INTERNAL_SERVER_ERROR, self.headers"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_1cbc6dda","line":90,"range":{"start_line":88,"start_character":5,"end_line":90,"end_character":68},"updated":"2019-11-21 03:11:48.000000000","message":"I still think it is not the best practice to handle this exception here, when we can be more granular and handle it in the driver which actually uses ovs, instead of polluting service.py with driver-specific code.","commit_id":"b56eb6c5af9124498befa6a33f4bf93245f12155"},{"author":{"_account_id":11600,"name":"Michał Dulko","email":"michal.dulko@gmail.com","username":"dulek"},"change_message_id":"dbbde08d04e5346e656dc03fd064e84d77c319a8","unresolved":false,"context_lines":[{"line_number":85,"context_line":"            self._check_failure()"},{"line_number":86,"context_line":"            LOG.error(\u0027Error when processing addNetwork request\u0027)"},{"line_number":87,"context_line":"            return \u0027\u0027, httplib.GATEWAY_TIMEOUT, self.headers"},{"line_number":88,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":89,"context_line":"            if e.cmd.startswith(\u0027ovs-vsctl\u0027):"},{"line_number":90,"context_line":"                LOG.exception(\u0027Unable to execute ovs-vsctl command\u0027)"},{"line_number":91,"context_line":"            with self.healthy.get_lock():"},{"line_number":92,"context_line":"                self.healthy.value \u003d False"},{"line_number":93,"context_line":"            return \u0027\u0027, httplib.INTERNAL_SERVER_ERROR, self.headers"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_b6e69015","line":90,"range":{"start_line":88,"start_character":5,"end_line":90,"end_character":68},"in_reply_to":"3fa7e38b_1cbc6dda","updated":"2019-11-21 12:35:39.000000000","message":"True, though it\u0027s both VIFOpenVSwitchDriver and os-vif that do ovs-vsctl. Both are run in base.connect(). Maybe we can put it there?","commit_id":"b56eb6c5af9124498befa6a33f4bf93245f12155"},{"author":{"_account_id":29615,"name":"Nayan Deshmukh","email":"n.deshmukh@samsung.com","username":"n.deshmukh"},"change_message_id":"6b6c3251861c18b02035cb93feccbedc38666ae8","unresolved":false,"context_lines":[{"line_number":85,"context_line":"            self._check_failure()"},{"line_number":86,"context_line":"            LOG.error(\u0027Error when processing addNetwork request\u0027)"},{"line_number":87,"context_line":"            return \u0027\u0027, httplib.GATEWAY_TIMEOUT, self.headers"},{"line_number":88,"context_line":"        except processutils.ProcessExecutionError as e:"},{"line_number":89,"context_line":"            if e.cmd.startswith(\u0027ovs-vsctl\u0027):"},{"line_number":90,"context_line":"                LOG.exception(\u0027Unable to execute ovs-vsctl command\u0027)"},{"line_number":91,"context_line":"            with self.healthy.get_lock():"},{"line_number":92,"context_line":"                self.healthy.value \u003d False"},{"line_number":93,"context_line":"            return \u0027\u0027, httplib.INTERNAL_SERVER_ERROR, self.headers"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_e82df945","line":90,"range":{"start_line":88,"start_character":5,"end_line":90,"end_character":68},"in_reply_to":"3fa7e38b_b6e69015","updated":"2019-11-22 03:01:47.000000000","message":"Yes but then we lose the access to healthy variable. If we want to take a quick action to resolve this error then we need to have it in service.py. \n\nI do agree that the base.connect() might be the best place to catch this exception but since we won\u0027t have access to healthy variable we can\u0027t do much after raising that exception. It doesn\u0027t make a lot of sense to have this.\n\nAnother option is to have a new class of exceptions (the fatal exceptions) which require us to restart the cni at once.","commit_id":"b56eb6c5af9124498befa6a33f4bf93245f12155"}]}