)]}'
{"ironic/common/utils.py":[{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"371524b8f4aee789bef0b754a6210f71d5e0e89a","unresolved":false,"context_lines":[{"line_number":690,"context_line":"    )"},{"line_number":691,"context_line":""},{"line_number":692,"context_line":""},{"line_number":693,"context_line":"def check_process_existence(pid):"},{"line_number":694,"context_line":"    \"\"\"Check for the existence of a process given its pid.\"\"\""},{"line_number":695,"context_line":"    return os.path.exists(\u0027/proc/%d\u0027 % pid)"}],"source_content_type":"text/x-python","patch_set":5,"id":"da85f559_194e9896","line":693,"updated":"2015-11-10 06:10:04.000000000","message":"I don\u0027t see a unit test for this function.","commit_id":"9bf946380b0a308367683fa1fa7c6e57959f2ae9"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"2f812088e0f17f5595527dce50e04e1f2599a118","unresolved":false,"context_lines":[{"line_number":690,"context_line":"    )"},{"line_number":691,"context_line":""},{"line_number":692,"context_line":""},{"line_number":693,"context_line":"def check_process_existence(pid):"},{"line_number":694,"context_line":"    \"\"\"Check for the existence of a process given its pid.\"\"\""},{"line_number":695,"context_line":"    return os.path.exists(\u0027/proc/%d\u0027 % pid)"}],"source_content_type":"text/x-python","patch_set":5,"id":"da85f559_f4b63bdb","line":693,"in_reply_to":"da85f559_194e9896","updated":"2015-11-10 06:43:04.000000000","message":"sorry for miss that when moving this from console_utils.py.","commit_id":"9bf946380b0a308367683fa1fa7c6e57959f2ae9"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"371524b8f4aee789bef0b754a6210f71d5e0e89a","unresolved":false,"context_lines":[{"line_number":692,"context_line":""},{"line_number":693,"context_line":"def check_process_existence(pid):"},{"line_number":694,"context_line":"    \"\"\"Check for the existence of a process given its pid.\"\"\""},{"line_number":695,"context_line":"    return os.path.exists(\u0027/proc/%d\u0027 % pid)"}],"source_content_type":"text/x-python","patch_set":5,"id":"da85f559_d9a2504b","line":695,"updated":"2015-11-10 06:10:04.000000000","message":"I think this might be better:\n\n try:\n   # Sending signal 0 to a process does nothing, but will raise OSError if the process does not exist\n   os.kill(pid, 0)\n except OSError:\n   return False\n return True\n\nAs it should work on both Linux and OSX\n\nOr maybe use the \u0027psutil\u0027 library and use psutil.pid_exists()\n\nhttp://stackoverflow.com/questions/568271/how-to-check-if-there-exists-a-process-with-a-given-pid\n\nI know see this was discussed before. I\u0027m not sure that using /proc/ is a good idea but willing to have my mind changed :)","commit_id":"9bf946380b0a308367683fa1fa7c6e57959f2ae9"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"cdadd49b7d095ea5f17ff2f55b53f41fa311b404","unresolved":false,"context_lines":[{"line_number":692,"context_line":""},{"line_number":693,"context_line":"def check_process_existence(pid):"},{"line_number":694,"context_line":"    \"\"\"Check for the existence of a process given its pid.\"\"\""},{"line_number":695,"context_line":"    return os.path.exists(\u0027/proc/%d\u0027 % pid)"}],"source_content_type":"text/x-python","patch_set":5,"id":"da85f559_943b2f38","line":695,"in_reply_to":"da85f559_d9a2504b","updated":"2015-11-10 06:30:48.000000000","message":"From:\nhttp://pubs.opengroup.org/onlinepubs/9699919799/functions/kill.html\n\nThe kill() function shall send a signal to a process or a group of processes specified by pid. The signal to be sent is specified by sig and is either one from the list given in \u003csignal.h\u003e or 0. If sig is 0 (the null signal), error checking is performed but no signal is actually sent. The null signal can be used to check the validity of pid.\n\nSo it seems to be a POSIX standard for using kill(pid, 0) to check the validity of a pid.","commit_id":"9bf946380b0a308367683fa1fa7c6e57959f2ae9"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"2f812088e0f17f5595527dce50e04e1f2599a118","unresolved":false,"context_lines":[{"line_number":692,"context_line":""},{"line_number":693,"context_line":"def check_process_existence(pid):"},{"line_number":694,"context_line":"    \"\"\"Check for the existence of a process given its pid.\"\"\""},{"line_number":695,"context_line":"    return os.path.exists(\u0027/proc/%d\u0027 % pid)"}],"source_content_type":"text/x-python","patch_set":5,"id":"da85f559_14997f1f","line":695,"in_reply_to":"da85f559_d9a2504b","updated":"2015-11-10 06:43:04.000000000","message":"thanks John for the investigation, I also find we use os.kill() in oslo.service to manage processes, will change it back with some more descriptive comments.","commit_id":"9bf946380b0a308367683fa1fa7c6e57959f2ae9"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"6d1bdbb7c73ea3ce7ec40c22d22d148eca4c3b09","unresolved":false,"context_lines":[{"line_number":690,"context_line":"    )"},{"line_number":691,"context_line":""},{"line_number":692,"context_line":""},{"line_number":693,"context_line":"def check_process_existence(pid):"},{"line_number":694,"context_line":"    \"\"\"Check for the existence of a process given its pid.\"\"\""},{"line_number":695,"context_line":"    try:"},{"line_number":696,"context_line":"        # Sending signal 0 to a process does nothing, but"}],"source_content_type":"text/x-python","patch_set":6,"id":"da85f559_5fa07790","line":693,"updated":"2015-11-10 10:35:47.000000000","message":"http://pythonhosted.org/psutil/#psutil.pid_exists","commit_id":"27a66f28fd2dc54e696cf46f887c8fa9d1879392"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"b4bb18505cd37f3b6cbc9903be911571ca605f89","unresolved":false,"context_lines":[{"line_number":690,"context_line":"    )"},{"line_number":691,"context_line":""},{"line_number":692,"context_line":""},{"line_number":693,"context_line":"def check_process_existence(pid):"},{"line_number":694,"context_line":"    \"\"\"Check for the existence of a process given its pid.\"\"\""},{"line_number":695,"context_line":"    try:"},{"line_number":696,"context_line":"        # Sending signal 0 to a process does nothing, but"}],"source_content_type":"text/x-python","patch_set":6,"id":"da85f559_bc4c0371","line":693,"in_reply_to":"da85f559_5fa07790","updated":"2015-11-10 14:28:38.000000000","message":"thanks for the link Dmitry, seems both are fine here.","commit_id":"27a66f28fd2dc54e696cf46f887c8fa9d1879392"}],"ironic/drivers/modules/console_utils.py":[{"author":{"_account_id":8106,"name":"Haomeng,Wang","email":"wanghaomeng@gmail.com","username":"whaom"},"change_message_id":"591448f5931a53e34742579b3ba6b70a4e486a6c","unresolved":false,"context_lines":[{"line_number":113,"context_line":"    \"\"\"Check for the existence of a pid.\"\"\""},{"line_number":114,"context_line":"    try:"},{"line_number":115,"context_line":"        os.kill(pid, 0)"},{"line_number":116,"context_line":"    except OSError:"},{"line_number":117,"context_line":"        return False"},{"line_number":118,"context_line":"    else:"},{"line_number":119,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"fa80f949_7d44401a","line":116,"updated":"2015-11-06 07:30:23.000000000","message":"nit: can we log the error message here? That should be useful for checking the response message from kill call to see what happens, else we know nothing why kill failed, thanks.","commit_id":"624e64a20e503cb21698ebeb2877ee938a23c48a"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"29e257fcc98e9cc2b638324fc56f893d80eb6f9f","unresolved":false,"context_lines":[{"line_number":113,"context_line":"    \"\"\"Check for the existence of a pid.\"\"\""},{"line_number":114,"context_line":"    try:"},{"line_number":115,"context_line":"        os.kill(pid, 0)"},{"line_number":116,"context_line":"    except OSError:"},{"line_number":117,"context_line":"        return False"},{"line_number":118,"context_line":"    else:"},{"line_number":119,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"fa80f949_fdf7d056","line":116,"in_reply_to":"fa80f949_7d44401a","updated":"2015-11-06 07:48:10.000000000","message":"it sends signal 0 here, just checking the process status, failed indicates that the process is not running, no need to log here as is just what expected.","commit_id":"624e64a20e503cb21698ebeb2877ee938a23c48a"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"eb6f31ced23ba924f3dbde692ccde08f6eac0ba8","unresolved":false,"context_lines":[{"line_number":113,"context_line":"    \"\"\"Check for the existence of a pid.\"\"\""},{"line_number":114,"context_line":"    try:"},{"line_number":115,"context_line":"        os.kill(pid, 0)"},{"line_number":116,"context_line":"    except OSError:"},{"line_number":117,"context_line":"        return False"},{"line_number":118,"context_line":"    else:"},{"line_number":119,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"da85f559_30996a6f","line":116,"in_reply_to":"fa80f949_b6a3a2b1","updated":"2015-11-09 06:55:32.000000000","message":"Done","commit_id":"624e64a20e503cb21698ebeb2877ee938a23c48a"},{"author":{"_account_id":8106,"name":"Haomeng,Wang","email":"wanghaomeng@gmail.com","username":"whaom"},"change_message_id":"4e8160bf80d3ad7ead4190c83709a6be0a9cff78","unresolved":false,"context_lines":[{"line_number":113,"context_line":"    \"\"\"Check for the existence of a pid.\"\"\""},{"line_number":114,"context_line":"    try:"},{"line_number":115,"context_line":"        os.kill(pid, 0)"},{"line_number":116,"context_line":"    except OSError:"},{"line_number":117,"context_line":"        return False"},{"line_number":118,"context_line":"    else:"},{"line_number":119,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"fa80f949_b6a3a2b1","line":116,"in_reply_to":"fa80f949_fdf7d056","updated":"2015-11-06 10:01:07.000000000","message":"Ok,got, thanks for your information, so it is better that we have clear comments here to avoid more confusing.","commit_id":"624e64a20e503cb21698ebeb2877ee938a23c48a"},{"author":{"_account_id":8106,"name":"Haomeng,Wang","email":"wanghaomeng@gmail.com","username":"whaom"},"change_message_id":"591448f5931a53e34742579b3ba6b70a4e486a6c","unresolved":false,"context_lines":[{"line_number":136,"context_line":"            # Allow exitcode 99 (RC_UNAUTHORIZED)"},{"line_number":137,"context_line":"            utils.execute(\u0027kill\u0027,"},{"line_number":138,"context_line":"                          str(console_pid),"},{"line_number":139,"context_line":"                          check_exit_code\u003d[0, 99])"},{"line_number":140,"context_line":"    finally:"},{"line_number":141,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"},{"line_number":142,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"fa80f949_7d12000b","line":139,"updated":"2015-11-06 07:30:23.000000000","message":"here, I am confusing, we did os.kill call first, then invoke utils.execute to issue \u0027kill\u0027 command again, can you help to understand here? Thank you.","commit_id":"624e64a20e503cb21698ebeb2877ee938a23c48a"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"fe6f4ca73668babc1ba593df2c2337b865823725","unresolved":false,"context_lines":[{"line_number":136,"context_line":"            # Allow exitcode 99 (RC_UNAUTHORIZED)"},{"line_number":137,"context_line":"            utils.execute(\u0027kill\u0027,"},{"line_number":138,"context_line":"                          str(console_pid),"},{"line_number":139,"context_line":"                          check_exit_code\u003d[0, 99])"},{"line_number":140,"context_line":"    finally:"},{"line_number":141,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"},{"line_number":142,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"fa80f949_58f14a00","line":139,"in_reply_to":"fa80f949_780c4e3b","updated":"2015-11-06 08:01:22.000000000","message":"thanks Kan for the explaination :)","commit_id":"624e64a20e503cb21698ebeb2877ee938a23c48a"},{"author":{"_account_id":14810,"name":"Kan","email":"sofaopenstack@gmail.com","username":"Kan"},"change_message_id":"485edc5392e6a4b4035318581aab7edea3700f83","unresolved":false,"context_lines":[{"line_number":136,"context_line":"            # Allow exitcode 99 (RC_UNAUTHORIZED)"},{"line_number":137,"context_line":"            utils.execute(\u0027kill\u0027,"},{"line_number":138,"context_line":"                          str(console_pid),"},{"line_number":139,"context_line":"                          check_exit_code\u003d[0, 99])"},{"line_number":140,"context_line":"    finally:"},{"line_number":141,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"},{"line_number":142,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"fa80f949_780c4e3b","line":139,"in_reply_to":"fa80f949_7d12000b","updated":"2015-11-06 07:56:04.000000000","message":"These two kill are really easy to cause misunderstanding. :)\n\nThe \u0027os.kill(pid, 0)\u0027 in \u0027_check_pid\u0027 is used to check whether the process with pid is still running. It will do nothing if the job is still running and raise OSError exception if the process is not existed.\n\nSo, the \u0027utils.execute(\u0027kill\u0027...)\u0027 is only executed when \u0027_check_pid\u0027 returns True, which means the process is still running.","commit_id":"624e64a20e503cb21698ebeb2877ee938a23c48a"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"29e257fcc98e9cc2b638324fc56f893d80eb6f9f","unresolved":false,"context_lines":[{"line_number":136,"context_line":"            # Allow exitcode 99 (RC_UNAUTHORIZED)"},{"line_number":137,"context_line":"            utils.execute(\u0027kill\u0027,"},{"line_number":138,"context_line":"                          str(console_pid),"},{"line_number":139,"context_line":"                          check_exit_code\u003d[0, 99])"},{"line_number":140,"context_line":"    finally:"},{"line_number":141,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"},{"line_number":142,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"fa80f949_dd752cfa","line":139,"in_reply_to":"fa80f949_7d12000b","updated":"2015-11-06 07:48:10.000000000","message":"hi Haomeng, os.kill(pid, 0) sends signal 0, which is basically \"Are you running?\"","commit_id":"624e64a20e503cb21698ebeb2877ee938a23c48a"},{"author":{"_account_id":6637,"name":"Sam Betts","email":"sam@code-smash.net","username":"sambetts"},"change_message_id":"ea890fad72ad2d242c2591c05345e5ae65e337db","unresolved":false,"context_lines":[{"line_number":138,"context_line":"                          str(console_pid),"},{"line_number":139,"context_line":"                          check_exit_code\u003d[0, 99])"},{"line_number":140,"context_line":"    finally:"},{"line_number":141,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"def make_persistent_password_file(path, password):"}],"source_content_type":"text/x-python","patch_set":1,"id":"fa80f949_45d156f9","line":141,"updated":"2015-11-06 12:01:16.000000000","message":"I think we should add at least a debug/warning log if the process doesn\u0027t exist because I think that might be useful for operators attempting to debug issues with their deployment.","commit_id":"624e64a20e503cb21698ebeb2877ee938a23c48a"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"eb6f31ced23ba924f3dbde692ccde08f6eac0ba8","unresolved":false,"context_lines":[{"line_number":138,"context_line":"                          str(console_pid),"},{"line_number":139,"context_line":"                          check_exit_code\u003d[0, 99])"},{"line_number":140,"context_line":"    finally:"},{"line_number":141,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"def make_persistent_password_file(path, password):"}],"source_content_type":"text/x-python","patch_set":1,"id":"da85f559_9094d637","line":141,"in_reply_to":"fa80f949_45d156f9","updated":"2015-11-09 06:55:32.000000000","message":"Done","commit_id":"624e64a20e503cb21698ebeb2877ee938a23c48a"},{"author":{"_account_id":6637,"name":"Sam Betts","email":"sam@code-smash.net","username":"sambetts"},"change_message_id":"d5eecd6c9cb9375f50ce5cf6ae8f9748bf0586b3","unresolved":false,"context_lines":[{"line_number":110,"context_line":""},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"def _check_pid(pid):"},{"line_number":113,"context_line":"    \"\"\"Check for the existence of a pid.\"\"\""},{"line_number":114,"context_line":"    try:"},{"line_number":115,"context_line":"        os.kill(pid, 0)"},{"line_number":116,"context_line":"    except OSError:"}],"source_content_type":"text/x-python","patch_set":2,"id":"da85f559_4a030e74","line":113,"updated":"2015-11-09 11:24:33.000000000","message":"I think that this comment should read something more like: \n\"\"\"Check for the existence of a process given its pid.\"\"\"\n\nEven the function name could be a little more descriptive, e.g. _check_process_existence() or something like that.","commit_id":"64c0fad8f40265c01be24cad486f9f0213d8a143"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"fa7b29460cd8aac62d2d789df9d36f629a35ec88","unresolved":false,"context_lines":[{"line_number":112,"context_line":"def _check_pid(pid):"},{"line_number":113,"context_line":"    \"\"\"Check for the existence of a pid.\"\"\""},{"line_number":114,"context_line":"    try:"},{"line_number":115,"context_line":"        os.kill(pid, 0)"},{"line_number":116,"context_line":"    except OSError:"},{"line_number":117,"context_line":"        # rasing OSError if the pid is not running."},{"line_number":118,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":2,"id":"da85f559_67f892e6","line":115,"updated":"2015-11-09 09:13:00.000000000","message":"I know \"kill\" is a very bad system call name in posix. But can we add some information as a comment or at least in the docstring saying that this kill call with the argument 0 won\u0027t try to kill the process?\n\nJust saying because it\u0027s confusing to call os.kill() to check the process and after that call \"kill\" using utils.execute to actually kill the process.\n\nOR\n\nAnother way (simpler) to check it for the existence of the process would be to check the /proc fs, e.g:\n\n def _check_pid(pid):\n    \"\"\"Check for the existence of a pid.\"\"\"\n     return os.path.exists(\u0027/proc/%d\u0027 % pid)","commit_id":"64c0fad8f40265c01be24cad486f9f0213d8a143"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"aaded50cc6dae5d1094249e99a804f035b7568e6","unresolved":false,"context_lines":[{"line_number":112,"context_line":"def _check_pid(pid):"},{"line_number":113,"context_line":"    \"\"\"Check for the existence of a pid.\"\"\""},{"line_number":114,"context_line":"    try:"},{"line_number":115,"context_line":"        os.kill(pid, 0)"},{"line_number":116,"context_line":"    except OSError:"},{"line_number":117,"context_line":"        # rasing OSError if the pid is not running."},{"line_number":118,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":2,"id":"da85f559_fb6c159e","line":115,"in_reply_to":"da85f559_2ae7aae5","updated":"2015-11-09 13:02:47.000000000","message":"thanks for the suggestions Lucas and Sam, will update in the coming patch set.","commit_id":"64c0fad8f40265c01be24cad486f9f0213d8a143"},{"author":{"_account_id":6637,"name":"Sam Betts","email":"sam@code-smash.net","username":"sambetts"},"change_message_id":"d5eecd6c9cb9375f50ce5cf6ae8f9748bf0586b3","unresolved":false,"context_lines":[{"line_number":112,"context_line":"def _check_pid(pid):"},{"line_number":113,"context_line":"    \"\"\"Check for the existence of a pid.\"\"\""},{"line_number":114,"context_line":"    try:"},{"line_number":115,"context_line":"        os.kill(pid, 0)"},{"line_number":116,"context_line":"    except OSError:"},{"line_number":117,"context_line":"        # rasing OSError if the pid is not running."},{"line_number":118,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":2,"id":"da85f559_2ae7aae5","line":115,"in_reply_to":"da85f559_67f892e6","updated":"2015-11-09 11:24:33.000000000","message":"I agree a comment explaining the posix signal 0 would be a great addition, I was looking at the signal package to try and work out if they had a constant for this, e.g. for sighup you can do os.kill(pid,  signal.SIGHUP), for me this helps understand what the call is doing, but I\u0027m not sure if there is one available for the null signal, but maybe we should add our own constant console_utils.SIGNULL or something like that?\n\nI like Lucas\u0027 idea about using the procfs too.","commit_id":"64c0fad8f40265c01be24cad486f9f0213d8a143"},{"author":{"_account_id":6637,"name":"Sam Betts","email":"sam@code-smash.net","username":"sambetts"},"change_message_id":"b3c39bac5af1dee8fd74396f09824c9b8228240f","unresolved":false,"context_lines":[{"line_number":110,"context_line":""},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"def _check_pid(pid):"},{"line_number":113,"context_line":"    \"\"\"Check for the existence of a pid.\"\"\""},{"line_number":114,"context_line":"    return os.path.exists(\u0027/proc/%d\u0027 % pid)"},{"line_number":115,"context_line":""},{"line_number":116,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da85f559_a4174a50","line":113,"updated":"2015-11-09 13:55:18.000000000","message":"I still think that this comment should read something more like: \"\"\"Check for the existence of a process given its pid.\"\"\"\n\nand that the function name could be a little more descriptive, e.g. _check_process_existence(pid) or something like that.","commit_id":"2cae6b8dab02078a7026afc01080a7a34166f39e"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"0f23c3807ffc23495f8cdebef4921c50babe0279","unresolved":false,"context_lines":[{"line_number":110,"context_line":""},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"def _check_pid(pid):"},{"line_number":113,"context_line":"    \"\"\"Check for the existence of a pid.\"\"\""},{"line_number":114,"context_line":"    return os.path.exists(\u0027/proc/%d\u0027 % pid)"},{"line_number":115,"context_line":""},{"line_number":116,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da85f559_312b7a96","line":113,"in_reply_to":"da85f559_a4174a50","updated":"2015-11-10 02:16:15.000000000","message":"ok Sam Betts, will refine it.","commit_id":"2cae6b8dab02078a7026afc01080a7a34166f39e"},{"author":{"_account_id":8106,"name":"Haomeng,Wang","email":"wanghaomeng@gmail.com","username":"whaom"},"change_message_id":"c6fbdfcc56b81b9b0b6c30f62a38558fda74f65d","unresolved":false,"context_lines":[{"line_number":109,"context_line":"        raise exception.NoConsolePid(pid_path\u003dpid_path)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":""},{"line_number":112,"context_line":"def _check_process_existence(pid):"},{"line_number":113,"context_line":"    \"\"\"Check for the existence of a process given its pid.\"\"\""},{"line_number":114,"context_line":"    return os.path.exists(\u0027/proc/%d\u0027 % pid)"},{"line_number":115,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"da85f559_20f24170","line":112,"updated":"2015-11-10 03:32:36.000000000","message":"I understand this is common uitl method to check if the process is existing via the input pid, so can we move this to ironic.common.utils?","commit_id":"1eef29281fb831c51d52be2ee0b90a30f3407458"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"371524b8f4aee789bef0b754a6210f71d5e0e89a","unresolved":false,"context_lines":[{"line_number":124,"context_line":""},{"line_number":125,"context_line":"        if utils.check_process_existence(console_pid):"},{"line_number":126,"context_line":"            # Allow exitcode 99 (RC_UNAUTHORIZED)"},{"line_number":127,"context_line":"            utils.execute(\u0027kill\u0027,"},{"line_number":128,"context_line":"                          str(console_pid),"},{"line_number":129,"context_line":"                          check_exit_code\u003d[0, 99])"},{"line_number":130,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":5,"id":"da85f559_d947b075","line":127,"updated":"2015-11-10 06:10:04.000000000","message":"Why not use \u0027os.kill\u0027?","commit_id":"9bf946380b0a308367683fa1fa7c6e57959f2ae9"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"2f812088e0f17f5595527dce50e04e1f2599a118","unresolved":false,"context_lines":[{"line_number":124,"context_line":""},{"line_number":125,"context_line":"        if utils.check_process_existence(console_pid):"},{"line_number":126,"context_line":"            # Allow exitcode 99 (RC_UNAUTHORIZED)"},{"line_number":127,"context_line":"            utils.execute(\u0027kill\u0027,"},{"line_number":128,"context_line":"                          str(console_pid),"},{"line_number":129,"context_line":"                          check_exit_code\u003d[0, 99])"},{"line_number":130,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":5,"id":"da85f559_34ae0386","line":127,"in_reply_to":"da85f559_d947b075","updated":"2015-11-10 06:43:04.000000000","message":"not sure why not use \u0027os.kill\u0027 originally, but I\u0027d like to change it.","commit_id":"9bf946380b0a308367683fa1fa7c6e57959f2ae9"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"6d1bdbb7c73ea3ce7ec40c22d22d148eca4c3b09","unresolved":false,"context_lines":[{"line_number":124,"context_line":"        console_pid \u003d _get_console_pid(node_uuid)"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"        if utils.check_process_existence(console_pid):"},{"line_number":127,"context_line":"            os.kill(console_pid, signal.SIGTERM)"},{"line_number":128,"context_line":"        else:"},{"line_number":129,"context_line":"            LOG.warning(_LW(\"Console process for node %s is not running \""},{"line_number":130,"context_line":"                            \"but pid file exists while trying to stop \""}],"source_content_type":"text/x-python","patch_set":6,"id":"da85f559_3fa573a1","line":127,"updated":"2015-11-10 10:35:47.000000000","message":"this code is racy, process might die between the check and the kill call. Maybe we should issue os.kill, wait some time, then make sure that process with this PID does not exist. This is, however, also racy, so maybe the best option is to just catch exceptions..","commit_id":"27a66f28fd2dc54e696cf46f887c8fa9d1879392"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"b4bb18505cd37f3b6cbc9903be911571ca605f89","unresolved":false,"context_lines":[{"line_number":124,"context_line":"        console_pid \u003d _get_console_pid(node_uuid)"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"        if utils.check_process_existence(console_pid):"},{"line_number":127,"context_line":"            os.kill(console_pid, signal.SIGTERM)"},{"line_number":128,"context_line":"        else:"},{"line_number":129,"context_line":"            LOG.warning(_LW(\"Console process for node %s is not running \""},{"line_number":130,"context_line":"                            \"but pid file exists while trying to stop \""}],"source_content_type":"text/x-python","patch_set":6,"id":"da85f559_9c51ff10","line":127,"in_reply_to":"da85f559_3fa573a1","updated":"2015-11-10 14:28:38.000000000","message":"thanks Dmitry, yes, we should catch the exception here.","commit_id":"27a66f28fd2dc54e696cf46f887c8fa9d1879392"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"e0b1812b29c0247706f1778ea1306f4ed9f61a06","unresolved":false,"context_lines":[{"line_number":127,"context_line":"            os.kill(console_pid, signal.SIGTERM)"},{"line_number":128,"context_line":"        else:"},{"line_number":129,"context_line":"            LOG.warning(_LW(\"Console process for node %s is not running \""},{"line_number":130,"context_line":"                            \"but pid file exists while trying to stop \""},{"line_number":131,"context_line":"                            \"shellinabox console.\"), node_uuid)"},{"line_number":132,"context_line":"    finally:"},{"line_number":133,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"}],"source_content_type":"text/x-python","patch_set":6,"id":"da85f559_18744066","line":130,"updated":"2015-11-10 13:10:54.000000000","message":"Is this error message still valid?","commit_id":"27a66f28fd2dc54e696cf46f887c8fa9d1879392"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"b4bb18505cd37f3b6cbc9903be911571ca605f89","unresolved":false,"context_lines":[{"line_number":127,"context_line":"            os.kill(console_pid, signal.SIGTERM)"},{"line_number":128,"context_line":"        else:"},{"line_number":129,"context_line":"            LOG.warning(_LW(\"Console process for node %s is not running \""},{"line_number":130,"context_line":"                            \"but pid file exists while trying to stop \""},{"line_number":131,"context_line":"                            \"shellinabox console.\"), node_uuid)"},{"line_number":132,"context_line":"    finally:"},{"line_number":133,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"}],"source_content_type":"text/x-python","patch_set":6,"id":"da85f559_bc1e4361","line":130,"in_reply_to":"da85f559_18744066","updated":"2015-11-10 14:28:38.000000000","message":"yeah, I think it\u0027s still valid.","commit_id":"27a66f28fd2dc54e696cf46f887c8fa9d1879392"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"158ef859d58d0a2f1a77d88bcdd3081da33b739e","unresolved":false,"context_lines":[{"line_number":123,"context_line":"        console_pid \u003d _get_console_pid(node_uuid)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"        # Allow exitcode 99 (RC_UNAUTHORIZED)"},{"line_number":126,"context_line":"        utils.execute(\u0027kill\u0027, str(console_pid), check_exit_code\u003d[0, 99])"},{"line_number":127,"context_line":"    finally:"},{"line_number":128,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"},{"line_number":129,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"da85f559_29b2a1e5","side":"PARENT","line":126,"updated":"2015-11-12 12:19:29.000000000","message":"I wonder if this check on error code 99 was something we need..","commit_id":"1ef1b397bf8de95bd81131a403c317224bfcac90"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"1bba2325b0e430ec965ac473e459babdab7657ee","unresolved":false,"context_lines":[{"line_number":123,"context_line":"        console_pid \u003d _get_console_pid(node_uuid)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"        # Allow exitcode 99 (RC_UNAUTHORIZED)"},{"line_number":126,"context_line":"        utils.execute(\u0027kill\u0027, str(console_pid), check_exit_code\u003d[0, 99])"},{"line_number":127,"context_line":"    finally:"},{"line_number":128,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"},{"line_number":129,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"da85f559_2d8b8420","side":"PARENT","line":126,"in_reply_to":"da85f559_29b2a1e5","updated":"2015-11-12 14:22:23.000000000","message":"seems error code 99 is RC_UNAUTHORIZED.","commit_id":"1ef1b397bf8de95bd81131a403c317224bfcac90"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"a00bef0ca802200f897e7be8e4012a31b4414001","unresolved":false,"context_lines":[{"line_number":123,"context_line":"        console_pid \u003d _get_console_pid(node_uuid)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"        # Allow exitcode 99 (RC_UNAUTHORIZED)"},{"line_number":126,"context_line":"        utils.execute(\u0027kill\u0027, str(console_pid), check_exit_code\u003d[0, 99])"},{"line_number":127,"context_line":"    finally:"},{"line_number":128,"context_line":"        utils.unlink_without_raise(_get_console_pid_file(node_uuid))"},{"line_number":129,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"da85f559_cdae782d","side":"PARENT","line":126,"in_reply_to":"da85f559_2d8b8420","updated":"2015-11-12 14:27:04.000000000","message":"yeah, and I wonder why it was initially added..","commit_id":"1ef1b397bf8de95bd81131a403c317224bfcac90"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"158ef859d58d0a2f1a77d88bcdd3081da33b739e","unresolved":false,"context_lines":[{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    :param node_uuid: the UUID of the node"},{"line_number":120,"context_line":"    :raises: NoConsolePid if no console PID was found"},{"line_number":121,"context_line":"    :raises: processutils.ProcessExecutionError if unable to stop the process"},{"line_number":122,"context_line":"    \"\"\""},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":8,"id":"da85f559_09af9d7d","line":121,"updated":"2015-11-12 12:19:29.000000000","message":"please fix the docstring","commit_id":"24188760c8d261e2f9425900f7deb73ed4a122b3"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"1bba2325b0e430ec965ac473e459babdab7657ee","unresolved":false,"context_lines":[{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    :param node_uuid: the UUID of the node"},{"line_number":120,"context_line":"    :raises: NoConsolePid if no console PID was found"},{"line_number":121,"context_line":"    :raises: processutils.ProcessExecutionError if unable to stop the process"},{"line_number":122,"context_line":"    \"\"\""},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":8,"id":"da85f559_f2aebd7f","line":121,"in_reply_to":"da85f559_09af9d7d","updated":"2015-11-12 14:22:23.000000000","message":"yeah, will add a following patch soon.","commit_id":"24188760c8d261e2f9425900f7deb73ed4a122b3"}],"ironic/tests/unit/drivers/modules/test_console_utils.py":[{"author":{"_account_id":14810,"name":"Kan","email":"sofaopenstack@gmail.com","username":"Kan"},"change_message_id":"8caf930d0b46e72aef0c91035eeeb3b421635b27","unresolved":false,"context_lines":[{"line_number":170,"context_line":"        mock_pid.assert_called_once_with(self.info[\u0027uuid\u0027])"},{"line_number":171,"context_line":"        self.assertFalse(mock_kill.called)"},{"line_number":172,"context_line":"        mock_unlink.assert_called_once_with(pid_file)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"    @mock.patch.object(utils, \u0027unlink_without_raise\u0027, autospec\u003dTrue)"},{"line_number":175,"context_line":"    @mock.patch.object(utils, \u0027check_process_existence\u0027,"},{"line_number":176,"context_line":"                       autospec\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":7,"id":"da85f559_d84c4ea0","line":173,"updated":"2015-11-11 07:38:24.000000000","message":"Just a question here: Should not your previous test \u0027test__stop_console_kill_exception\u0027 case be kept here? I think it might be corresponding to the exception in \u0027_stop_console\u0027 function.","commit_id":"d0e1b6dd6ad43ac229bd8e1d03df1602990a1db5"},{"author":{"_account_id":6610,"name":"Zhenguo Niu","email":"Niu.ZGlinux@gmail.com","username":"niu-zglinux"},"change_message_id":"da120b6cc5797287d847967f8ea10a8b388d4c8d","unresolved":false,"context_lines":[{"line_number":170,"context_line":"        mock_pid.assert_called_once_with(self.info[\u0027uuid\u0027])"},{"line_number":171,"context_line":"        self.assertFalse(mock_kill.called)"},{"line_number":172,"context_line":"        mock_unlink.assert_called_once_with(pid_file)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"    @mock.patch.object(utils, \u0027unlink_without_raise\u0027, autospec\u003dTrue)"},{"line_number":175,"context_line":"    @mock.patch.object(utils, \u0027check_process_existence\u0027,"},{"line_number":176,"context_line":"                       autospec\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":7,"id":"da85f559_eef06800","line":173,"in_reply_to":"da85f559_d84c4ea0","updated":"2015-11-11 08:04:19.000000000","message":"hi Kan, now we change to use os.kill and only log a warning message when got OSError exception, so no need to add assertRaise here.","commit_id":"d0e1b6dd6ad43ac229bd8e1d03df1602990a1db5"},{"author":{"_account_id":14810,"name":"Kan","email":"sofaopenstack@gmail.com","username":"Kan"},"change_message_id":"e71c18fe3079836ee32e4188cecb0a579a20a15d","unresolved":false,"context_lines":[{"line_number":170,"context_line":"        mock_pid.assert_called_once_with(self.info[\u0027uuid\u0027])"},{"line_number":171,"context_line":"        self.assertFalse(mock_kill.called)"},{"line_number":172,"context_line":"        mock_unlink.assert_called_once_with(pid_file)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"    @mock.patch.object(utils, \u0027unlink_without_raise\u0027, autospec\u003dTrue)"},{"line_number":175,"context_line":"    @mock.patch.object(utils, \u0027check_process_existence\u0027,"},{"line_number":176,"context_line":"                       autospec\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":7,"id":"da85f559_8efc3c73","line":173,"in_reply_to":"da85f559_eef06800","updated":"2015-11-11 08:19:09.000000000","message":"Oh sorry, I am too overreacted to the exception. Thanks for correcting me.:)","commit_id":"d0e1b6dd6ad43ac229bd8e1d03df1602990a1db5"}]}