)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"f8cd2968ef06ed807d4a04811169f36cffae7ace","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"9b2e9809_cb5986c6","updated":"2021-10-15 07:41:22.000000000","message":"Added more comments, and marking this patch as WIP","commit_id":"7b8354e400beeaab438e023be1b5d85f48999363"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"57ac91d48bee02a5f7dba914e48958cb4915e424","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"47529a1d_6ff9b545","updated":"2021-10-14 09:20:56.000000000","message":"I think we may have race conditions, though I could be wrong","commit_id":"7b8354e400beeaab438e023be1b5d85f48999363"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"4fb0e6c00b874298fc2e44b876b75a7c64dc499c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"7a0c8b5c_793a0b67","updated":"2021-10-14 10:00:14.000000000","message":"Thank you for the insight!","commit_id":"7b8354e400beeaab438e023be1b5d85f48999363"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"969996afa6a05fb13caf3cc7280ff5d4b5629741","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"b18f6548_3f09ba2c","updated":"2021-12-14 09:24:56.000000000","message":"We have reviewed this change with KumoScale team, based on insights also mentioned in my previous comments, we believe this change is good as it is - especially because it solves a more harmful race condition that exists in the current code.","commit_id":"7b8354e400beeaab438e023be1b5d85f48999363"},{"author":{"_account_id":34173,"name":"Lior Friedman","display_name":"Lior Friedman","email":"lior.friedman@kioxia.com","username":"liorf95"},"change_message_id":"229f44000dcb59970c285cc71de4ab1d6928d6a8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"8ebebd34_cc63f363","updated":"2022-02-10 10:38:11.000000000","message":"Brian,\n\nYour review here will be appreciated very much.","commit_id":"8588dc6af35b2f81ac5631227458ccd4f2a0606a"},{"author":{"_account_id":34550,"name":"Gili Buzaglo","email":"gili.buzaglo@kioxia.com","username":"gilib"},"change_message_id":"396cd270d9a269d70355e23ceedf0c0f2dd9a349","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"5bca3b6a_e007ddec","updated":"2022-02-10 10:29:49.000000000","message":"Looks ok to me","commit_id":"8588dc6af35b2f81ac5631227458ccd4f2a0606a"},{"author":{"_account_id":34173,"name":"Lior Friedman","display_name":"Lior Friedman","email":"lior.friedman@kioxia.com","username":"liorf95"},"change_message_id":"a896d89ba0ac72c7d93160aa3fb95c27de43a473","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"3d262b09_35cf008d","updated":"2022-02-09 13:14:57.000000000","message":"run-KIOXIA CI","commit_id":"8588dc6af35b2f81ac5631227458ccd4f2a0606a"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"2955d87359d12b8b8dde9f2d199d6f2160595546","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"067ad405_2ad485b9","updated":"2022-02-16 14:39:59.000000000","message":"Race condition exists for backends that don\u0027t support namespace AER, but since those are completely broken at the moment, we can let it slide.","commit_id":"5902166149ba46da5f36eb352e62f3361269fb2c"},{"author":{"_account_id":7198,"name":"Jay Bryant","email":"jungleboyj@electronicjungle.net","username":"jsbryant"},"change_message_id":"d99aba3e5a173b6455b80eb347177a528b04c5a2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"b586a12b_b7b8d962","updated":"2022-02-16 14:48:57.000000000","message":"This looks ok to me but I am not an NVMe expert so would prefer other eyes on it.","commit_id":"5902166149ba46da5f36eb352e62f3361269fb2c"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"c3802ce4c9e6bc62df9f20ee38d30ce42630f5a1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"9663d5b5_424cc787","updated":"2022-02-16 22:32:33.000000000","message":"We discussed this patch at today\u0027s cinder meeting.  It turns out that we\u0027ve been assuming namespace AER support on the backend.  Under that assumption, this patch is fine.  We have some issues, for example, Bug #1961102, that arise when that assumption is relaxed.  We agreed to accept this patch and add a release note about NVMe Namespace AER support for the Yoga release [0].\n\n[0] https://review.opendev.org/c/openstack/os-brick/+/829588","commit_id":"5902166149ba46da5f36eb352e62f3361269fb2c"},{"author":{"_account_id":29122,"name":"Raghavendra Tilay","email":"raghavendra-uddhav.tilay@hpe.com","username":"raghavendrat"},"change_message_id":"11c42d34a5a4e18e18c12ca9fb648b2c7436eef2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"8188d490_e197a7f0","updated":"2022-02-16 10:32:57.000000000","message":"minor comment.","commit_id":"5902166149ba46da5f36eb352e62f3361269fb2c"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"cb6ba9a120dd9af165e5ca88b28dacecbdf9b42d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"94c2c159_e608037f","updated":"2022-02-15 17:33:26.000000000","message":"recheck os-brick-src-tempest-lvm-lio-barbican : failure is server failed to build in tempest.api.compute.servers.test_multiple_create.MultipleCreateTestJSON.test_multiple_create_with_reservation_return","commit_id":"5902166149ba46da5f36eb352e62f3361269fb2c"}],"os_brick/initiator/connectors/nvmeof.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"57ac91d48bee02a5f7dba914e48958cb4915e424","unresolved":true,"context_lines":[{"line_number":524,"context_line":"    def _connect_target_volume(self, target_nqn, vol_uuid, portals):"},{"line_number":525,"context_line":"        try:"},{"line_number":526,"context_line":"            NVMeOFConnector._get_nvme_controller(self, target_nqn)"},{"line_number":527,"context_line":"            NVMeOFConnector.rescan(self, target_nqn, vol_uuid)"},{"line_number":528,"context_line":"        except exception.VolumeDeviceNotFound:"},{"line_number":529,"context_line":"            if not NVMeOFConnector.connect_to_portals("},{"line_number":530,"context_line":"                    self, target_nqn, portals):"}],"source_content_type":"text/x-python","patch_set":1,"id":"ce800536_4dd8e94e","line":527,"updated":"2021-10-14 09:20:56.000000000","message":"-1: Wouldn\u0027t this rescan have potential race conditions?\n\nLet\u0027s say we have 3 volumes, vol1 and vol2 are connected to nova.\n\n- Request disconnect vol1\n- Request connect vol3\n\n- Nova calls cinder to attach vol3 \n- Nova calls os-brick to disconnect vol1 and completes that operation\n\n- Nova calls os-brick to connect vol3 and the rescan is triggered\n  The rescan brings back vol1 to the nova host\n\n- Nova calls cinder to detach vol1\n  We end up with a leftover device on the nova host","commit_id":"7b8354e400beeaab438e023be1b5d85f48999363"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"f8cd2968ef06ed807d4a04811169f36cffae7ace","unresolved":true,"context_lines":[{"line_number":524,"context_line":"    def _connect_target_volume(self, target_nqn, vol_uuid, portals):"},{"line_number":525,"context_line":"        try:"},{"line_number":526,"context_line":"            NVMeOFConnector._get_nvme_controller(self, target_nqn)"},{"line_number":527,"context_line":"            NVMeOFConnector.rescan(self, target_nqn, vol_uuid)"},{"line_number":528,"context_line":"        except exception.VolumeDeviceNotFound:"},{"line_number":529,"context_line":"            if not NVMeOFConnector.connect_to_portals("},{"line_number":530,"context_line":"                    self, target_nqn, portals):"}],"source_content_type":"text/x-python","patch_set":1,"id":"c98b4450_7165fd74","line":527,"in_reply_to":"b90b45c4_fd4940ac","updated":"2021-10-15 07:41:22.000000000","message":"Scratch what I said about three volumes belonging to same controller :) - What I should have said is that vol3 should be on a controller that is already connected to the host too.\n\nI think that another relevant point here is that nvme rescan also happens asynchronously even without explicitly calling \u0027nvme rescan\u0027\nIf that is true, this should mean that the same possible race condition will happen with the old code too (though less commonly)\n\nAlso now that I\u0027m looking at the old code, it does a rescan that seems unnecessary. (that whole \"else:\" statement seems unnecessary, but need to test that old code again to make sure) - I believe that rescan is what was causing other race conditions which we were seeing where errors were thrown due to \u0027nvme connect\u0027 calls with already connected controllers\n\nAnother big point here is that currently this code path does not do any \u0027nvme disconnect\u0027 at all - due to race conditions :) - it expects termination to happen on the backend - so this race condition will not happen here anyway\n\nOverall thank you  I am going to refactor this patch - I think one thing that could help is adding \u0027-D\u0027 to the \u0027nvme connect\u0027 command (for resolving the original issue this patch was addressing)","commit_id":"7b8354e400beeaab438e023be1b5d85f48999363"},{"author":{"_account_id":34173,"name":"Lior Friedman","display_name":"Lior Friedman","email":"lior.friedman@kioxia.com","username":"liorf95"},"change_message_id":"71edfa01bb2d7dd1ef2795a2d070bcf6e3907089","unresolved":false,"context_lines":[{"line_number":524,"context_line":"    def _connect_target_volume(self, target_nqn, vol_uuid, portals):"},{"line_number":525,"context_line":"        try:"},{"line_number":526,"context_line":"            NVMeOFConnector._get_nvme_controller(self, target_nqn)"},{"line_number":527,"context_line":"            NVMeOFConnector.rescan(self, target_nqn, vol_uuid)"},{"line_number":528,"context_line":"        except exception.VolumeDeviceNotFound:"},{"line_number":529,"context_line":"            if not NVMeOFConnector.connect_to_portals("},{"line_number":530,"context_line":"                    self, target_nqn, portals):"}],"source_content_type":"text/x-python","patch_set":1,"id":"e1eb943b_f7003d80","line":527,"in_reply_to":"c98b4450_7165fd74","updated":"2022-02-02 14:23:51.000000000","message":"We believe this change is good as it is - especially because it solves a more harmful race condition that exists in the current code.","commit_id":"7b8354e400beeaab438e023be1b5d85f48999363"},{"author":{"_account_id":16721,"name":"Zohar Mamedov","email":"zohar.cloud@gmail.com","username":"zohar"},"change_message_id":"4fb0e6c00b874298fc2e44b876b75a7c64dc499c","unresolved":true,"context_lines":[{"line_number":524,"context_line":"    def _connect_target_volume(self, target_nqn, vol_uuid, portals):"},{"line_number":525,"context_line":"        try:"},{"line_number":526,"context_line":"            NVMeOFConnector._get_nvme_controller(self, target_nqn)"},{"line_number":527,"context_line":"            NVMeOFConnector.rescan(self, target_nqn, vol_uuid)"},{"line_number":528,"context_line":"        except exception.VolumeDeviceNotFound:"},{"line_number":529,"context_line":"            if not NVMeOFConnector.connect_to_portals("},{"line_number":530,"context_line":"                    self, target_nqn, portals):"}],"source_content_type":"text/x-python","patch_set":1,"id":"b90b45c4_fd4940ac","line":527,"in_reply_to":"ce800536_4dd8e94e","updated":"2021-10-14 10:00:14.000000000","message":"This is a very interesting scenario and I need to think it through more.\n\nI\u0027m pretty sure that overall you are right.\n\nI think it is also worth mentioning that all three volumes should belong to the same controller (and hence target namespace).\n\nThe simplest thing that I can gather now is:\n\nThis race condition will be an issue only if the detach call to cinder doing terminate_connection will not cause a connection termination on the backend end.","commit_id":"7b8354e400beeaab438e023be1b5d85f48999363"},{"author":{"_account_id":29122,"name":"Raghavendra Tilay","email":"raghavendra-uddhav.tilay@hpe.com","username":"raghavendrat"},"change_message_id":"11c42d34a5a4e18e18c12ca9fb648b2c7436eef2","unresolved":true,"context_lines":[{"line_number":522,"context_line":"        dev_path \u003d NVMeOFConnector.get_nvme_device_path("},{"line_number":523,"context_line":"            self, target_nqn, vol_uuid)"},{"line_number":524,"context_line":"        if not dev_path:"},{"line_number":525,"context_line":"            LOG.error(\"Target %s volume %s not found\", target_nqn, vol_uuid)"},{"line_number":526,"context_line":"            raise exception.VolumeDeviceNotFound(device\u003dvol_uuid)"},{"line_number":527,"context_line":"        return dev_path"},{"line_number":528,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"66beba0f_5252a8c2","line":525,"updated":"2022-02-16 10:32:57.000000000","message":"nit: Since there are two variables, would it be better to write this as below:\nLOG.error(\"Target %(target)s of volume %(volume)s not found\",\n  {\u0027target\u0027: target_nqn, \u0027volume\u0027: vol_uuid} )","commit_id":"5902166149ba46da5f36eb352e62f3361269fb2c"},{"author":{"_account_id":34173,"name":"Lior Friedman","display_name":"Lior Friedman","email":"lior.friedman@kioxia.com","username":"liorf95"},"change_message_id":"bf6aed7a55bab46bbad63a2caadc39331e6463f4","unresolved":false,"context_lines":[{"line_number":522,"context_line":"        dev_path \u003d NVMeOFConnector.get_nvme_device_path("},{"line_number":523,"context_line":"            self, target_nqn, vol_uuid)"},{"line_number":524,"context_line":"        if not dev_path:"},{"line_number":525,"context_line":"            LOG.error(\"Target %s volume %s not found\", target_nqn, vol_uuid)"},{"line_number":526,"context_line":"            raise exception.VolumeDeviceNotFound(device\u003dvol_uuid)"},{"line_number":527,"context_line":"        return dev_path"},{"line_number":528,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3b1d726e_c9eb49fd","line":525,"in_reply_to":"66beba0f_5252a8c2","updated":"2022-02-16 16:35:28.000000000","message":"Ack","commit_id":"5902166149ba46da5f36eb352e62f3361269fb2c"}]}