)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"fe6e6204e05a08115c89e4afd987d2f3386d3222","unresolved":false,"context_lines":[{"line_number":15,"context_line":""},{"line_number":16,"context_line":"This patch fixes this by taking into account that directories and"},{"line_number":17,"context_line":"symlinks may already exist."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Closes-Bug: #1819705"},{"line_number":20,"context_line":"Change-Id: Ib61acdd4c469ac4316e2f5e518301c44dd3a6321"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"5fc1f717_1d42243f","line":18,"updated":"2019-03-14 16:37:37.000000000","message":"If you can indicate that now the public method connect can raise exception that would be great, in my point of view it\u0027s important to mention it.","commit_id":"b7ef3731a4057a5875a3fa8a71d27ce4978a9b60"}],"cinderlib/nos_brick.py":[{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"3eea0e9f5ddeaa8cd5e282ffea04a9921cd876b1","unresolved":false,"context_lines":[{"line_number":89,"context_line":"                    self._ensure_link(real_path, link_name)"},{"line_number":90,"context_line":"        except Exception:"},{"line_number":91,"context_line":"            fileutils.delete_if_exists(conf)"},{"line_number":92,"context_line":"            raise"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"        return {\u0027path\u0027: real_path,"},{"line_number":95,"context_line":"                \u0027conf\u0027: conf,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_611082a9","line":92,"updated":"2019-03-13 14:10:15.000000000","message":"This does not look to be related to your patch, can you give a bit more details?","commit_id":"b7ef3731a4057a5875a3fa8a71d27ce4978a9b60"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"fe6e6204e05a08115c89e4afd987d2f3386d3222","unresolved":false,"context_lines":[{"line_number":89,"context_line":"                    self._ensure_link(real_path, link_name)"},{"line_number":90,"context_line":"        except Exception:"},{"line_number":91,"context_line":"            fileutils.delete_if_exists(conf)"},{"line_number":92,"context_line":"            raise"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"        return {\u0027path\u0027: real_path,"},{"line_number":95,"context_line":"                \u0027conf\u0027: conf,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_3dada0e8","line":92,"in_reply_to":"5fc1f717_07cd0e65","updated":"2019-03-14 16:37:37.000000000","message":"What about to catch the message and re-raise it via an internal exception like exception.BrickException() or CinderlibException() I think that would be helpful for users of the library.","commit_id":"b7ef3731a4057a5875a3fa8a71d27ce4978a9b60"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"ea4a0b8c22154186e8018bcca08f3a61933ad91c","unresolved":false,"context_lines":[{"line_number":89,"context_line":"                    self._ensure_link(real_path, link_name)"},{"line_number":90,"context_line":"        except Exception:"},{"line_number":91,"context_line":"            fileutils.delete_if_exists(conf)"},{"line_number":92,"context_line":"            raise"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"        return {\u0027path\u0027: real_path,"},{"line_number":95,"context_line":"                \u0027conf\u0027: conf,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_07cd0e65","line":92,"in_reply_to":"5fc1f717_4459d48b","updated":"2019-03-13 15:17:11.000000000","message":"I see so basically you are fixing the issue when the symlinks already exist and in same time ensure any (other) exceptions will be reported. I would have mentioned that in the commit message. but fair enough :)","commit_id":"b7ef3731a4057a5875a3fa8a71d27ce4978a9b60"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"e11cb55f5a1051bb65bcfcab50ba78b7a7370862","unresolved":false,"context_lines":[{"line_number":89,"context_line":"                    self._ensure_link(real_path, link_name)"},{"line_number":90,"context_line":"        except Exception:"},{"line_number":91,"context_line":"            fileutils.delete_if_exists(conf)"},{"line_number":92,"context_line":"            raise"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"        return {\u0027path\u0027: real_path,"},{"line_number":95,"context_line":"                \u0027conf\u0027: conf,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_4459d48b","line":92,"in_reply_to":"5fc1f717_611082a9","updated":"2019-03-13 14:50:37.000000000","message":"It is related, missing this is the reason why it is silently saying that it was successfully detached when it wasn\u0027t.\n\nIf we don\u0027t create the symlink we won\u0027t be able to detach under the circumstances mentioned in the bug.","commit_id":"b7ef3731a4057a5875a3fa8a71d27ce4978a9b60"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"da67bce62bae0b1884c050889508043fc73cc2a8","unresolved":false,"context_lines":[{"line_number":96,"context_line":"            try:"},{"line_number":97,"context_line":"                try:"},{"line_number":98,"context_line":"                    self._unmap(real_path, conf, connection_properties)"},{"line_number":99,"context_line":"                finally:"},{"line_number":100,"context_line":"                    fileutils.delete_if_exists(conf)"},{"line_number":101,"context_line":"            except Exception:"},{"line_number":102,"context_line":"                exc \u003d traceback.format_exc()"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_67ba6ee2","line":99,"range":{"start_line":99,"start_character":16,"end_line":99,"end_character":24},"updated":"2019-03-19 09:22:07.000000000","message":"Using finally here does not look really profitable.","commit_id":"ec63fa3a260ac156ad50ce05cd5adb3b89ec0689"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"60405125bc5638af8db5bc9d718247a5d14889cb","unresolved":false,"context_lines":[{"line_number":96,"context_line":"            try:"},{"line_number":97,"context_line":"                try:"},{"line_number":98,"context_line":"                    self._unmap(real_path, conf, connection_properties)"},{"line_number":99,"context_line":"                finally:"},{"line_number":100,"context_line":"                    fileutils.delete_if_exists(conf)"},{"line_number":101,"context_line":"            except Exception:"},{"line_number":102,"context_line":"                exc \u003d traceback.format_exc()"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_d91c66c3","line":99,"range":{"start_line":99,"start_character":16,"end_line":99,"end_character":24},"in_reply_to":"5fc1f717_67ba6ee2","updated":"2019-03-19 12:03:42.000000000","message":"How so?\nI used finally because I never want to leave the credentials on the system, so it doesn\u0027t matter if the unmap fails or is successful, the configuration will be deleted.","commit_id":"ec63fa3a260ac156ad50ce05cd5adb3b89ec0689"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"da67bce62bae0b1884c050889508043fc73cc2a8","unresolved":false,"context_lines":[{"line_number":100,"context_line":"                    fileutils.delete_if_exists(conf)"},{"line_number":101,"context_line":"            except Exception:"},{"line_number":102,"context_line":"                exc \u003d traceback.format_exc()"},{"line_number":103,"context_line":"                LOG.error(\u0027Exception occurred while cleaning up after \u0027"},{"line_number":104,"context_line":"                          \u0027connection error\\n%s\u0027, exc)"},{"line_number":105,"context_line":"            finally:"},{"line_number":106,"context_line":"                raise exception.BrickException(\u0027Error connecting volume: %s\u0027 %"},{"line_number":107,"context_line":"                                               six.text_type(exec_exception))"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_a7ee16eb","line":104,"range":{"start_line":103,"start_character":16,"end_line":104,"end_character":54},"updated":"2019-03-19 09:22:07.000000000","message":"I don\u0027t think it\u0027s really a matter here, we just try to cleanup, we should probably not use ERROR severity which would hide the important and initial error during debuging. I would suggest to use info or debug.","commit_id":"ec63fa3a260ac156ad50ce05cd5adb3b89ec0689"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"60405125bc5638af8db5bc9d718247a5d14889cb","unresolved":false,"context_lines":[{"line_number":100,"context_line":"                    fileutils.delete_if_exists(conf)"},{"line_number":101,"context_line":"            except Exception:"},{"line_number":102,"context_line":"                exc \u003d traceback.format_exc()"},{"line_number":103,"context_line":"                LOG.error(\u0027Exception occurred while cleaning up after \u0027"},{"line_number":104,"context_line":"                          \u0027connection error\\n%s\u0027, exc)"},{"line_number":105,"context_line":"            finally:"},{"line_number":106,"context_line":"                raise exception.BrickException(\u0027Error connecting volume: %s\u0027 %"},{"line_number":107,"context_line":"                                               six.text_type(exec_exception))"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_f98602bb","line":104,"range":{"start_line":103,"start_character":16,"end_line":104,"end_character":54},"in_reply_to":"5fc1f717_a7ee16eb","updated":"2019-03-19 12:03:42.000000000","message":"No, this must be an error, it is an error when cleaning up an error and must be logged as such.  The same way we do when using the `excutils.save_and_reraise_exception` method.\nIt will not mask the real error, we will see one after the other, that\u0027s I raised the initial exception on the finally clause.","commit_id":"ec63fa3a260ac156ad50ce05cd5adb3b89ec0689"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"da67bce62bae0b1884c050889508043fc73cc2a8","unresolved":false,"context_lines":[{"line_number":103,"context_line":"                LOG.error(\u0027Exception occurred while cleaning up after \u0027"},{"line_number":104,"context_line":"                          \u0027connection error\\n%s\u0027, exc)"},{"line_number":105,"context_line":"            finally:"},{"line_number":106,"context_line":"                raise exception.BrickException(\u0027Error connecting volume: %s\u0027 %"},{"line_number":107,"context_line":"                                               six.text_type(exec_exception))"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"        return {\u0027path\u0027: real_path,"},{"line_number":110,"context_line":"                \u0027conf\u0027: conf,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_07904a4e","line":107,"range":{"start_line":106,"start_character":0,"end_line":107,"end_character":77},"updated":"2019-03-19 09:22:07.000000000","message":"If I can suggest you a refactor to make all of this simple:\n\n  except Exception as exec_exception:\n    fileutils.delete_if_exists(conf)\n    try:\n       self._unmap(real_path, conf, connection_properties)\n    except:\n      LOG.debug(\"Attempt to cleanup failed with %s\", traceback.format_exc())\n    raise exception.BrickException(\u0027Error connecting volume: %s\u0027 %\n                                               six.text_type(exec_exception))","commit_id":"ec63fa3a260ac156ad50ce05cd5adb3b89ec0689"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"60405125bc5638af8db5bc9d718247a5d14889cb","unresolved":false,"context_lines":[{"line_number":103,"context_line":"                LOG.error(\u0027Exception occurred while cleaning up after \u0027"},{"line_number":104,"context_line":"                          \u0027connection error\\n%s\u0027, exc)"},{"line_number":105,"context_line":"            finally:"},{"line_number":106,"context_line":"                raise exception.BrickException(\u0027Error connecting volume: %s\u0027 %"},{"line_number":107,"context_line":"                                               six.text_type(exec_exception))"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"        return {\u0027path\u0027: real_path,"},{"line_number":110,"context_line":"                \u0027conf\u0027: conf,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_1972eee4","line":107,"range":{"start_line":106,"start_character":0,"end_line":107,"end_character":77},"in_reply_to":"5fc1f717_07904a4e","updated":"2019-03-19 12:03:42.000000000","message":"That will not work.\nIf we remove the configuration file, then we don\u0027t have it for the unmap command, and we\u0027ll never be able to unmap.\n\nAn inferior alternative to what I have written, but that would be more readable, is:\n\n            try:\n                self._unmap(real_path, conf, connection_properties)\n            except Exception:\n                exc \u003d traceback.format_exc()\n                LOG.error(\u0027Exception occurred while cleaning up after \u0027\n                          \u0027connection error\\n%s\u0027, exc)\n            finally:\n                fileutils.delete_if_exists(conf)\n            raise exception.BrickException(\u0027Error connecting volume: %s\u0027 %\n                                           six.text_type(exec_exception))\n\nBut if we have an exception on the logging or the \"delete_if_exists\" we will mask the original error.","commit_id":"ec63fa3a260ac156ad50ce05cd5adb3b89ec0689"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"da67bce62bae0b1884c050889508043fc73cc2a8","unresolved":false,"context_lines":[{"line_number":113,"context_line":"    def _ensure_link(self, source, link_name):"},{"line_number":114,"context_line":"        self._ensure_dir(os.path.dirname(link_name))"},{"line_number":115,"context_line":"        if self.im_root:"},{"line_number":116,"context_line":"            try:"},{"line_number":117,"context_line":"                os.symlink(source, link_name)"},{"line_number":118,"context_line":"            except OSError as exc:"},{"line_number":119,"context_line":"                if exc.errno !\u003d errno.EEXIST:"},{"line_number":120,"context_line":"                    raise"},{"line_number":121,"context_line":"                # If we have a leftover link, clean it up"},{"line_number":122,"context_line":"                if source !\u003d os.path.realpath(link_name):"},{"line_number":123,"context_line":"                    os.remove(link_name)"},{"line_number":124,"context_line":"                    os.symlink(source, link_name)"},{"line_number":125,"context_line":"        else:"},{"line_number":126,"context_line":"            self._execute(\u0027ln\u0027, \u0027-s\u0027, \u0027-f\u0027, source, link_name,"},{"line_number":127,"context_line":"                          run_as_root\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_478652de","line":124,"range":{"start_line":116,"start_character":0,"end_line":124,"end_character":49},"updated":"2019-03-19 09:22:07.000000000","message":"Why not to make things simple insteaod of using try/except and calling several time os.symlink, something like?\n\n    if os.path.exists(link_name):\n      LOG.debug(\"clean up leftover link %s\", link_name)\n      os.remove(link_name)\n    os.symlink(source, link_name)\n\n\nNo need of the try/except anymore, the code you have added in connect_volume will handle everything.","commit_id":"ec63fa3a260ac156ad50ce05cd5adb3b89ec0689"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"60405125bc5638af8db5bc9d718247a5d14889cb","unresolved":false,"context_lines":[{"line_number":113,"context_line":"    def _ensure_link(self, source, link_name):"},{"line_number":114,"context_line":"        self._ensure_dir(os.path.dirname(link_name))"},{"line_number":115,"context_line":"        if self.im_root:"},{"line_number":116,"context_line":"            try:"},{"line_number":117,"context_line":"                os.symlink(source, link_name)"},{"line_number":118,"context_line":"            except OSError as exc:"},{"line_number":119,"context_line":"                if exc.errno !\u003d errno.EEXIST:"},{"line_number":120,"context_line":"                    raise"},{"line_number":121,"context_line":"                # If we have a leftover link, clean it up"},{"line_number":122,"context_line":"                if source !\u003d os.path.realpath(link_name):"},{"line_number":123,"context_line":"                    os.remove(link_name)"},{"line_number":124,"context_line":"                    os.symlink(source, link_name)"},{"line_number":125,"context_line":"        else:"},{"line_number":126,"context_line":"            self._execute(\u0027ln\u0027, \u0027-s\u0027, \u0027-f\u0027, source, link_name,"},{"line_number":127,"context_line":"                          run_as_root\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_0c3aaa33","line":124,"range":{"start_line":116,"start_character":0,"end_line":124,"end_character":49},"in_reply_to":"5fc1f717_478652de","updated":"2019-03-19 12:03:42.000000000","message":"That code looks really nice, but leaves us open to a race condition that will make us raise OSError with errno.EEXIST if ceph-common creates the link between us checking if it exists and us creating the symlink.\n\nI could change it for:\n\n            if os.path.exists(link_name):\n                os.remove(link_name)\n            try:\n                os.symlink(source, link_name)\n            except OSError as exc:\n                if exc.errno !\u003d errno.EEXIST:\n                    raise\n\nIt\u0027s a little bit more readable, but it also means that we\u0027ll remove and create the symlink even when it is already correct...\n\nThe code I added handles the errors, the symlink already existing is not really an error that should prevent the connection from going through.","commit_id":"ec63fa3a260ac156ad50ce05cd5adb3b89ec0689"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"da67bce62bae0b1884c050889508043fc73cc2a8","unresolved":false,"context_lines":[{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    def _ensure_dir(self, path):"},{"line_number":168,"context_line":"        if self.im_root:"},{"line_number":169,"context_line":"            try:"},{"line_number":170,"context_line":"                os.makedirs(path, 0o755)"},{"line_number":171,"context_line":"            except OSError as exc:"},{"line_number":172,"context_line":"                if exc.errno !\u003d errno.EEXIST:"},{"line_number":173,"context_line":"                    raise"},{"line_number":174,"context_line":"        else:"},{"line_number":175,"context_line":"            self._execute(\u0027mkdir\u0027, \u0027-p\u0027, \u0027-m0755\u0027, path, run_as_root\u003dTrue)"},{"line_number":176,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_87f3da73","line":173,"range":{"start_line":169,"start_character":0,"end_line":173,"end_character":25},"updated":"2019-03-19 09:22:07.000000000","message":"Same her,e we don\u0027t need try/except. You should just check the existence. Any other error will be handled by the code you have added in connect_volume","commit_id":"ec63fa3a260ac156ad50ce05cd5adb3b89ec0689"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"60405125bc5638af8db5bc9d718247a5d14889cb","unresolved":false,"context_lines":[{"line_number":166,"context_line":""},{"line_number":167,"context_line":"    def _ensure_dir(self, path):"},{"line_number":168,"context_line":"        if self.im_root:"},{"line_number":169,"context_line":"            try:"},{"line_number":170,"context_line":"                os.makedirs(path, 0o755)"},{"line_number":171,"context_line":"            except OSError as exc:"},{"line_number":172,"context_line":"                if exc.errno !\u003d errno.EEXIST:"},{"line_number":173,"context_line":"                    raise"},{"line_number":174,"context_line":"        else:"},{"line_number":175,"context_line":"            self._execute(\u0027mkdir\u0027, \u0027-p\u0027, \u0027-m0755\u0027, path, run_as_root\u003dTrue)"},{"line_number":176,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_4c175253","line":173,"range":{"start_line":169,"start_character":0,"end_line":173,"end_character":25},"in_reply_to":"5fc1f717_87f3da73","updated":"2019-03-19 12:03:42.000000000","message":"No, the exception handling code is to make sure that we clean up when things fail.  This code is added for when we should ignore the exception (which is what\u0027s reported in the bug), because if the directory exists, we are OK, but if it\u0027s a different error, then is when we need to re-raise the exception, and the exception handling code will take care of it.","commit_id":"ec63fa3a260ac156ad50ce05cd5adb3b89ec0689"}]}