)]}'
{"ironic/conductor/utils.py":[{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"adda19a938cd56e403ab5ad15da5bf589e14f8f7","unresolved":false,"context_lines":[{"line_number":36,"context_line":"    new_state \u003d states.POWER_ON if state \u003d\u003d states.REBOOT else state"},{"line_number":37,"context_line":"    try:"},{"line_number":38,"context_line":"        if not task.driver.power.validate(node):"},{"line_number":39,"context_line":"            raise exception.IncorrectDriverInfo()"},{"line_number":40,"context_line":"        if state !\u003d states.REBOOT:"},{"line_number":41,"context_line":"            curr_state \u003d task.driver.power.get_power_state(task, node)"},{"line_number":42,"context_line":"    except Exception as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9jHM%3D","line":39,"updated":"2013-12-10 11:41:17.000000000","message":"So power.validate now able to do both - return False or raise InvalidParameterValue if invalid. Can we somehow unify it - e.g. to move this raise inside validate so it will do nothing if valid, or raise appropriate exception otherwise?","commit_id":"ae47e0c88eb7d0049e8ae4dfb5fd74516b16c5b8"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"2573ea9f9a531cd8d07dc3578fe926d643410559","unresolved":false,"context_lines":[{"line_number":36,"context_line":"    new_state \u003d states.POWER_ON if state \u003d\u003d states.REBOOT else state"},{"line_number":37,"context_line":"    try:"},{"line_number":38,"context_line":"        if not task.driver.power.validate(node):"},{"line_number":39,"context_line":"            raise exception.IncorrectDriverInfo()"},{"line_number":40,"context_line":"        if state !\u003d states.REBOOT:"},{"line_number":41,"context_line":"            curr_state \u003d task.driver.power.get_power_state(task, node)"},{"line_number":42,"context_line":"    except Exception as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9ieQ%3D","line":39,"in_reply_to":"AAAATn%2F9ipM%3D","updated":"2013-12-10 13:29:22.000000000","message":"Hmm true. Then we should discuss this with bug originator - Deva","commit_id":"ae47e0c88eb7d0049e8ae4dfb5fd74516b16c5b8"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"e9d449a49f15cce41936b8221f57fdebb8f6154a","unresolved":false,"context_lines":[{"line_number":36,"context_line":"    new_state \u003d states.POWER_ON if state \u003d\u003d states.REBOOT else state"},{"line_number":37,"context_line":"    try:"},{"line_number":38,"context_line":"        if not task.driver.power.validate(node):"},{"line_number":39,"context_line":"            raise exception.IncorrectDriverInfo()"},{"line_number":40,"context_line":"        if state !\u003d states.REBOOT:"},{"line_number":41,"context_line":"            curr_state \u003d task.driver.power.get_power_state(task, node)"},{"line_number":42,"context_line":"    except Exception as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9ipM%3D","line":39,"in_reply_to":"AAAATn%2F9jHM%3D","updated":"2013-12-10 12:49:12.000000000","message":"Maybe makes sense. I looked to bug description https://bugs.launchpad.net/ironic/+bug/1255648.","commit_id":"ae47e0c88eb7d0049e8ae4dfb5fd74516b16c5b8"}],"ironic/drivers/base.py":[{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"adda19a938cd56e403ab5ad15da5bf589e14f8f7","unresolved":false,"context_lines":[{"line_number":126,"context_line":""},{"line_number":127,"context_line":"        :param node: a single Node to validate."},{"line_number":128,"context_line":"        :returns: True/False, indicating if the requisite info is correct."},{"line_number":129,"context_line":"        :raises: InvalidParameterValue if any parameter is missing."},{"line_number":130,"context_line":"        \"\"\""},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    @abc.abstractmethod"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9jGY%3D","line":129,"updated":"2013-12-10 11:41:17.000000000","message":"same comment - we\u0027ll need to check return value and catch exception, would like to unify it.","commit_id":"ae47e0c88eb7d0049e8ae4dfb5fd74516b16c5b8"}],"ironic/drivers/modules/ssh.py":[{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"adda19a938cd56e403ab5ad15da5bf589e14f8f7","unresolved":false,"context_lines":[{"line_number":263,"context_line":"        \"\"\"Create ssh connection.\"\"\""},{"line_number":264,"context_line":"        if not self._ssh_connection:"},{"line_number":265,"context_line":"            self._ssh_connection \u003d utils.ssh_connect(_parse_driver_info(node))"},{"line_number":266,"context_line":"        return self._ssh_connection"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def _validate_ssh_connection(self, node):"},{"line_number":269,"context_line":"        \"\"\"Validate ssh connection info.\"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9jCM%3D","line":266,"updated":"2013-12-10 11:41:17.000000000","message":"How do you think is it safe to cache _ssh_connection? It seems we also need to check it\u0027s state somehow (or do they override __nonzero__() in ssh object so that it shows whether the connection is alive?)","commit_id":"ae47e0c88eb7d0049e8ae4dfb5fd74516b16c5b8"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"2573ea9f9a531cd8d07dc3578fe926d643410559","unresolved":false,"context_lines":[{"line_number":263,"context_line":"        \"\"\"Create ssh connection.\"\"\""},{"line_number":264,"context_line":"        if not self._ssh_connection:"},{"line_number":265,"context_line":"            self._ssh_connection \u003d utils.ssh_connect(_parse_driver_info(node))"},{"line_number":266,"context_line":"        return self._ssh_connection"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def _validate_ssh_connection(self, node):"},{"line_number":269,"context_line":"        \"\"\"Validate ssh connection info.\"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9idc%3D","line":266,"in_reply_to":"AAAATn%2F9is4%3D","updated":"2013-12-10 13:29:22.000000000","message":"Ok, makes sense. Thanks for the rationale :)","commit_id":"ae47e0c88eb7d0049e8ae4dfb5fd74516b16c5b8"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"e9d449a49f15cce41936b8221f57fdebb8f6154a","unresolved":false,"context_lines":[{"line_number":263,"context_line":"        \"\"\"Create ssh connection.\"\"\""},{"line_number":264,"context_line":"        if not self._ssh_connection:"},{"line_number":265,"context_line":"            self._ssh_connection \u003d utils.ssh_connect(_parse_driver_info(node))"},{"line_number":266,"context_line":"        return self._ssh_connection"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def _validate_ssh_connection(self, node):"},{"line_number":269,"context_line":"        \"\"\"Validate ssh connection info.\"\"\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9is4%3D","line":266,"in_reply_to":"AAAATn%2F9jCM%3D","updated":"2013-12-10 12:49:12.000000000","message":"Max, time lag between create ssh connection and use _ssh_connection object very short. \n\n1) Task manager creates instance of SSHPower.\n\n2) validate() called.\n\n3) *power* method called.\n\nThat is all. New power operation - new instance of SSHPower.\nIMO check that connection is alive not needed.","commit_id":"ae47e0c88eb7d0049e8ae4dfb5fd74516b16c5b8"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"adda19a938cd56e403ab5ad15da5bf589e14f8f7","unresolved":false,"context_lines":[{"line_number":268,"context_line":"    def _validate_ssh_connection(self, node):"},{"line_number":269,"context_line":"        \"\"\"Validate ssh connection info.\"\"\""},{"line_number":270,"context_line":"        try:"},{"line_number":271,"context_line":"            self._get_ssh_connection(node)"},{"line_number":272,"context_line":"        except exception.SSHConnectFailed:"},{"line_number":273,"context_line":"            return False"},{"line_number":274,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9jBk%3D","line":271,"updated":"2013-12-10 11:41:17.000000000","message":"again, does \"if not self._ssh_connection\" in method above show that connection is alive, or just that it\u0027s not None?","commit_id":"ae47e0c88eb7d0049e8ae4dfb5fd74516b16c5b8"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"e9d449a49f15cce41936b8221f57fdebb8f6154a","unresolved":false,"context_lines":[{"line_number":268,"context_line":"    def _validate_ssh_connection(self, node):"},{"line_number":269,"context_line":"        \"\"\"Validate ssh connection info.\"\"\""},{"line_number":270,"context_line":"        try:"},{"line_number":271,"context_line":"            self._get_ssh_connection(node)"},{"line_number":272,"context_line":"        except exception.SSHConnectFailed:"},{"line_number":273,"context_line":"            return False"},{"line_number":274,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9izw%3D","line":271,"in_reply_to":"AAAATn%2F9jBk%3D","updated":"2013-12-10 12:49:12.000000000","message":"Only not None, see my comment on l266.","commit_id":"ae47e0c88eb7d0049e8ae4dfb5fd74516b16c5b8"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"236cfc2105f906697a35e1978ec6c9bb94f6c827","unresolved":false,"context_lines":[{"line_number":270,"context_line":"        try:"},{"line_number":271,"context_line":"            self._get_ssh_connection(node)"},{"line_number":272,"context_line":"        except exception.SSHConnectFailed:"},{"line_number":273,"context_line":"            raise exception.IncorrectDriverInfo"},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"    def validate(self, node):"},{"line_number":276,"context_line":"        \"\"\"Check that node[\u0027driver_info\u0027] contains the requisite fields."}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F9bfQ%3D","line":273,"updated":"2013-12-11 16:07:32.000000000","message":"Now I\u0027m wondering whether we need a separate exception for this. Would InvalidParameterValue be sufficient for all cases? They have similar codes. Just seems that InvalidParameterValue and IncorrectDriverInfo is the same thing... \n\nOr maybe for ssh, to send SSHConnectFailed in this case to say what exactly failed validation?","commit_id":"bfcd9f5acf2d48c1188d9a9315e871f4b1f89143"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"236cfc2105f906697a35e1978ec6c9bb94f6c827","unresolved":false,"context_lines":[{"line_number":279,"context_line":"        :raises: InvalidParameterValue"},{"line_number":280,"context_line":"        :raises: IncorrectDriverInfo"},{"line_number":281,"context_line":"        \"\"\""},{"line_number":282,"context_line":"        _parse_driver_info(node)"},{"line_number":283,"context_line":"        self._validate_ssh_connection(node)"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    def get_power_state(self, task, node):"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F9bl4%3D","line":282,"updated":"2013-12-11 16:07:32.000000000","message":"If we cache _ssh_connection maybe we can cache parsed info here too?","commit_id":"bfcd9f5acf2d48c1188d9a9315e871f4b1f89143"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"519a39c6c29cb7091ca77ea71ccf0ba90e30521e","unresolved":false,"context_lines":[{"line_number":279,"context_line":"        :raises: InvalidParameterValue"},{"line_number":280,"context_line":"        :raises: IncorrectDriverInfo"},{"line_number":281,"context_line":"        \"\"\""},{"line_number":282,"context_line":"        _parse_driver_info(node)"},{"line_number":283,"context_line":"        self._validate_ssh_connection(node)"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    def get_power_state(self, task, node):"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATn%2F9bIs%3D","line":282,"in_reply_to":"AAAATn%2F9bl4%3D","updated":"2013-12-11 17:07:16.000000000","message":"Will you please address this one?","commit_id":"bfcd9f5acf2d48c1188d9a9315e871f4b1f89143"},{"author":{"_account_id":8106,"name":"Haomeng,Wang","email":"wanghaomeng@gmail.com","username":"whaom"},"change_message_id":"a0af7669260a0c1d697201dd7d25767b33616bab","unresolved":false,"context_lines":[{"line_number":266,"context_line":"        return self._ssh_connection"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def _validate_ssh_connection(self, node):"},{"line_number":269,"context_line":"        \"\"\"Validate ssh connection info.\"\"\""},{"line_number":270,"context_line":"        try:"},{"line_number":271,"context_line":"            self._get_ssh_connection(node)"},{"line_number":272,"context_line":"        except exception.SSHConnectFailed:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F9Was%3D","line":269,"updated":"2013-12-12 11:48:05.000000000","message":"I think we are trying to check the SSH connectivity, so could we have such comments \"Validate  SSH connectivity\"? \nYuriy, correct me if I am wrong understanding here.","commit_id":"afe9875ce63ab5c9ce60d9feac3774b1b802c621"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"8293e5b5390a5cd693b6b44a465005cb408cf26d","unresolved":false,"context_lines":[{"line_number":266,"context_line":"        return self._ssh_connection"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def _validate_ssh_connection(self, node):"},{"line_number":269,"context_line":"        \"\"\"Validate ssh connection info.\"\"\""},{"line_number":270,"context_line":"        try:"},{"line_number":271,"context_line":"            self._get_ssh_connection(node)"},{"line_number":272,"context_line":"        except exception.SSHConnectFailed:"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F9WMo%3D","line":269,"in_reply_to":"AAAATn%2F9Was%3D","updated":"2013-12-12 12:42:16.000000000","message":"Done","commit_id":"afe9875ce63ab5c9ce60d9feac3774b1b802c621"},{"author":{"_account_id":8106,"name":"Haomeng,Wang","email":"wanghaomeng@gmail.com","username":"whaom"},"change_message_id":"a0af7669260a0c1d697201dd7d25767b33616bab","unresolved":false,"context_lines":[{"line_number":268,"context_line":"    def _validate_ssh_connection(self, node):"},{"line_number":269,"context_line":"        \"\"\"Validate ssh connection info.\"\"\""},{"line_number":270,"context_line":"        try:"},{"line_number":271,"context_line":"            self._get_ssh_connection(node)"},{"line_number":272,"context_line":"        except exception.SSHConnectFailed:"},{"line_number":273,"context_line":"            raise exception.InvalidParameterValue(_(\"SSH connection cannot\""},{"line_number":274,"context_line":"                                                    \" be established.\"))"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F9WaM%3D","line":271,"updated":"2013-12-12 11:48:05.000000000","message":"It looks like self._ssh_connection will never be None once it is assigned with a SSH client handler instance, so we can not validate the SSH connectivity if the connection is closed already.\n\nAnd I try to find some \u0027ssh ping\u0027 method for this SSH Client lib, no luck, no such method to valid the SSH connection I think, maybe we have good idea here, need more comments.","commit_id":"afe9875ce63ab5c9ce60d9feac3774b1b802c621"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"8293e5b5390a5cd693b6b44a465005cb408cf26d","unresolved":false,"context_lines":[{"line_number":268,"context_line":"    def _validate_ssh_connection(self, node):"},{"line_number":269,"context_line":"        \"\"\"Validate ssh connection info.\"\"\""},{"line_number":270,"context_line":"        try:"},{"line_number":271,"context_line":"            self._get_ssh_connection(node)"},{"line_number":272,"context_line":"        except exception.SSHConnectFailed:"},{"line_number":273,"context_line":"            raise exception.InvalidParameterValue(_(\"SSH connection cannot\""},{"line_number":274,"context_line":"                                                    \" be established.\"))"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATn%2F9WMM%3D","line":271,"in_reply_to":"AAAATn%2F9WaM%3D","updated":"2013-12-12 12:42:16.000000000","message":"Haomeng, please read my comments for Max: https://review.openstack.org/#/c/60876/1/ironic/drivers/modules/ssh.py","commit_id":"afe9875ce63ab5c9ce60d9feac3774b1b802c621"},{"author":{"_account_id":2889,"name":"Aeva Black","email":"aeva.online@gmail.com","username":"tenbrae"},"change_message_id":"362a145936f22762bbbcfe202890e0887e8d7580","unresolved":false,"context_lines":[{"line_number":257,"context_line":"    NOTE: This driver does not currently support multi-node operations."},{"line_number":258,"context_line":"    \"\"\""},{"line_number":259,"context_line":""},{"line_number":260,"context_line":"    _ssh_connection \u003d None"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"    def _get_ssh_connection(self, node):"},{"line_number":263,"context_line":"        \"\"\"Create SSH connection.\"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAATn%2F9Ukc%3D","line":260,"updated":"2013-12-12 17:16:42.000000000","message":"The SSHPower class is instantiated only once by ConductorManager -- not once per node or once per connection, but once per service.\n\nI like the idea of reusing ssh connection objects, but this is not the right way to do it.","commit_id":"52725f4a76f7169d39590d95ed47619888dd8fe4"},{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"a921f94f8455ae6c649b1e63547877ef79947d7a","unresolved":false,"context_lines":[{"line_number":279,"context_line":"        :param node: Single node object."},{"line_number":280,"context_line":"        :raises: InvalidParameterValue"},{"line_number":281,"context_line":"        \"\"\""},{"line_number":282,"context_line":"        _parse_driver_info(node)"},{"line_number":283,"context_line":"        self._validate_ssh_connection(node)"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    def get_power_state(self, task, node):"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAATn%2F9Uks%3D","line":282,"updated":"2013-12-12 17:16:34.000000000","message":"I asked somewhere in previous reviews, but again, how do you think will it be reasonable to cache parsed driver info here as well as ssh connection? Just to avoid parsing it twice?","commit_id":"52725f4a76f7169d39590d95ed47619888dd8fe4"}],"ironic/tests/conductor/test_conductor_utils.py":[{"author":{"_account_id":8968,"name":"Max Lobur","email":"max_lobur@outlook.com","username":"max_lobur"},"change_message_id":"adda19a938cd56e403ab5ad15da5bf589e14f8f7","unresolved":false,"context_lines":[{"line_number":208,"context_line":"            self.assertEqual(node[\u0027target_power_state\u0027], None)"},{"line_number":209,"context_line":"            self.assertNotEqual(node[\u0027last_error\u0027], None)"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"    def test_change_node_power_state_driver_info_not_validated(self):"},{"line_number":212,"context_line":"        ndict \u003d utils.get_test_node(driver\u003d\u0027fake\u0027,"},{"line_number":213,"context_line":"                                    power_state\u003dstates.POWER_ON)"},{"line_number":214,"context_line":"        node \u003d self.dbapi.create_node(ndict)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATn%2F9jAI%3D","line":211,"updated":"2013-12-10 11:41:17.000000000","message":"Maybe rename to ..._not_valid(self): ? And rename the method above too","commit_id":"ae47e0c88eb7d0049e8ae4dfb5fd74516b16c5b8"}],"ironic/tests/drivers/test_ssh.py":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"1bdfe32b801827158445bf02a185cc6992282e57","unresolved":false,"context_lines":[{"line_number":422,"context_line":"                \u0027_get_nodes_mac_addresses\u0027)"},{"line_number":423,"context_line":"        self.get_mac_addr_mock \u003d self.get_mac_addr_patcher.start()"},{"line_number":424,"context_line":"        self.get_conn_patcher \u003d mock.patch.object(ssh, \u0027_get_connection\u0027)"},{"line_number":425,"context_line":""},{"line_number":426,"context_line":"        self.get_conn_mock \u003d self.get_conn_patcher.start()"},{"line_number":427,"context_line":""},{"line_number":428,"context_line":"        def stop_patchers():"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAATn%2F8%2Fnk%3D","line":425,"updated":"2013-12-17 22:53:42.000000000","message":"/me wonders why you added a blank line here.","commit_id":"0b3d725ed70a6584b85bfeda5e8020d7d3498ae2"},{"author":{"_account_id":7711,"name":"Yuriy Zveryanskyy","email":"yzveryanskyy@mirantis.com","username":"yuriyz"},"change_message_id":"5ccd06e904a394167cca609ac387db31f6660a06","unresolved":false,"context_lines":[{"line_number":422,"context_line":"                \u0027_get_nodes_mac_addresses\u0027)"},{"line_number":423,"context_line":"        self.get_mac_addr_mock \u003d self.get_mac_addr_patcher.start()"},{"line_number":424,"context_line":"        self.get_conn_patcher \u003d mock.patch.object(ssh, \u0027_get_connection\u0027)"},{"line_number":425,"context_line":""},{"line_number":426,"context_line":"        self.get_conn_mock \u003d self.get_conn_patcher.start()"},{"line_number":427,"context_line":""},{"line_number":428,"context_line":"        def stop_patchers():"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAATn%2F8%2BVI%3D","line":425,"in_reply_to":"AAAATn%2F8%2Fnk%3D","updated":"2013-12-18 08:34:02.000000000","message":"Done","commit_id":"0b3d725ed70a6584b85bfeda5e8020d7d3498ae2"}]}