)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":16366,"name":"Randall Nortman","email":"openstack@nortman.net","username":"rnortman"},"change_message_id":"1f0e54db19783afdcb8505ec68064e6950b75de3","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When downloading a streamed chunk from swift through glance api"},{"line_number":10,"context_line":"while an EOF will cause tar to exit cleanly and glance plugin"},{"line_number":11,"context_line":"to exit and re spawn, bad data at the end of the EOF will cause"},{"line_number":12,"context_line":"a second tar process to kick off that is owned by parent pid id 1"},{"line_number":13,"context_line":"this commit addresses the bug below by reclaiming the failed tar"},{"line_number":14,"context_line":"process which will close the pipe and kill the new tar aswell."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"7af24918_5b449548","line":11,"updated":"2016-03-04 13:24:59.000000000","message":"ITYM \"bad data at the end of the file\"","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":16366,"name":"Randall Nortman","email":"openstack@nortman.net","username":"rnortman"},"change_message_id":"1f0e54db19783afdcb8505ec68064e6950b75de3","unresolved":false,"context_lines":[{"line_number":10,"context_line":"while an EOF will cause tar to exit cleanly and glance plugin"},{"line_number":11,"context_line":"to exit and re spawn, bad data at the end of the EOF will cause"},{"line_number":12,"context_line":"a second tar process to kick off that is owned by parent pid id 1"},{"line_number":13,"context_line":"this commit addresses the bug below by reclaiming the failed tar"},{"line_number":14,"context_line":"process which will close the pipe and kill the new tar aswell."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: Ic2bc89fa6d08db505b044a9498c1bfa5b884a056"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"7af24918_3b3f09b5","line":13,"updated":"2016-03-04 13:24:59.000000000","message":"ITYM \"... owned by parent pid 1. This commit ...\"","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"4c3a0e809195a7d16d8f6c2f04f6c4f27ffae7b4","unresolved":false,"context_lines":[{"line_number":10,"context_line":"while an EOF will cause tar to exit cleanly and glance plugin"},{"line_number":11,"context_line":"to exit and re spawn, bad data at the end of the EOF will cause"},{"line_number":12,"context_line":"a second tar process to kick off that is owned by parent pid id 1"},{"line_number":13,"context_line":"this commit addresses the bug below by reclaiming the failed tar"},{"line_number":14,"context_line":"process which will close the pipe and kill the new tar aswell."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: Ic2bc89fa6d08db505b044a9498c1bfa5b884a056"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":13,"id":"5aef4532_be8d9a7d","line":13,"updated":"2016-03-07 16:26:19.000000000","message":"Where does this reclaim occur?","commit_id":"de6894e7901e17bc69c65d93877760df2947e0df"},{"author":{"_account_id":10896,"name":"Tim Pownall","email":"pownalltim@gmail.com","username":"pownalltim"},"change_message_id":"2051e5d365eb15ef753ff8352e26011cf6b46ff3","unresolved":false,"context_lines":[{"line_number":10,"context_line":"while an EOF will cause tar to exit cleanly and glance plugin"},{"line_number":11,"context_line":"to exit and re spawn, bad data at the end of the EOF will cause"},{"line_number":12,"context_line":"a second tar process to kick off that is owned by parent pid id 1"},{"line_number":13,"context_line":"this commit addresses the bug below by reclaiming the failed tar"},{"line_number":14,"context_line":"process which will close the pipe and kill the new tar aswell."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: Ic2bc89fa6d08db505b044a9498c1bfa5b884a056"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":13,"id":"5aef4532_1ed3ee39","line":13,"in_reply_to":"5aef4532_be8d9a7d","updated":"2016-03-07 16:29:26.000000000","message":"When we poll for the process we check the return code and the zombie process goes away. The process waits on hanging it\u0027s return code read, but we never check it in the original code.","commit_id":"de6894e7901e17bc69c65d93877760df2947e0df"}],"plugins/xenserver/xenapi/etc/xapi.d/plugins/utils.py":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"81831f17163e788020de1d283e3f867d262713c5","unresolved":false,"context_lines":[{"line_number":451,"context_line":"    tar_proc \u003d make_subprocess(tar_cmd, stderr\u003dTrue, stdin\u003dTrue)"},{"line_number":452,"context_line":""},{"line_number":453,"context_line":"    try:"},{"line_number":454,"context_line":"        while True and tar_proc.poll() is None:"},{"line_number":455,"context_line":"            chunk \u003d fileobj.read(CHUNK_SIZE)"},{"line_number":456,"context_line":"            if chunk \u003d\u003d \u0027\u0027:"},{"line_number":457,"context_line":"                break"}],"source_content_type":"text/x-python","patch_set":1,"id":"7af24918_0cecc3f1","line":454,"updated":"2016-03-03 18:03:01.000000000","message":"Nit: It is tempting to add an has_proc_terminated() method that just calls poll and does the None check, so the intent is slightly clearer.","commit_id":"b536ad204c6d036b653dfd9272fd3f7ecb65465b"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"3302c1af7f976929455fd281778b01c80c646797","unresolved":false,"context_lines":[{"line_number":107,"context_line":"    out, err \u003d proc.communicate(cmd_input)"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"    ret \u003d proc.returncode"},{"line_number":110,"context_line":"    if ret not in ok_exit_codes:"},{"line_number":111,"context_line":"        LOG.error(\"Command \u0027%(cmdline)s\u0027 with process id \u0027%(pid)s\u0027 expected \""},{"line_number":112,"context_line":"                  \"return code in \u0027%(ok)s\u0027 but got \u0027%(rc)s\u0027: %(err)s\" %"},{"line_number":113,"context_line":"                  {\u0027cmdline\u0027: cmdline, \u0027pid\u0027: proc.pid, \u0027ok\u0027: ok_exit_codes,"}],"source_content_type":"text/x-python","patch_set":4,"id":"7af24918_a571390e","line":110,"updated":"2016-03-04 11:59:28.000000000","message":"we check the return code here.","commit_id":"00b3b6171112be661f28beed00e69db7f841ef0f"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"9b3609617b27ac03fae02dc52aafaed824e48766","unresolved":false,"context_lines":[{"line_number":439,"context_line":""},{"line_number":440,"context_line":"    finish_subprocess(tar_proc, tar_cmd)"},{"line_number":441,"context_line":""},{"line_number":442,"context_line":"def has_proc_terminated(proc):"},{"line_number":443,"context_line":"    proc_running \u003d False"},{"line_number":444,"context_line":"    if proc.poll() is None:"},{"line_number":445,"context_line":"        proc_running \u003d True"}],"source_content_type":"text/x-python","patch_set":4,"id":"7af24918_c3aaa184","line":442,"updated":"2016-03-03 20:22:17.000000000","message":"From what I can tell you want one of three things to happen. if proc.poll() is None tar is still running so you should return True, if proc.poll() \u003d\u003d 0 tar has exited cleanly and you should return False, if proc.poll() is anything else it has exited with an error and you want to raise an exception to trigger the try_kill_process(tar_proc) on line 469.\n\nYou might rename this to has_process_terminated_cleanly or something to indicate that termination with a nonzero exit code is considered an exception.","commit_id":"00b3b6171112be661f28beed00e69db7f841ef0f"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"518a83c212afe2a3745a818e38706e52ac49efb5","unresolved":false,"context_lines":[{"line_number":439,"context_line":""},{"line_number":440,"context_line":"    finish_subprocess(tar_proc, tar_cmd)"},{"line_number":441,"context_line":""},{"line_number":442,"context_line":"def has_proc_terminated(proc):"},{"line_number":443,"context_line":"    proc_running \u003d False"},{"line_number":444,"context_line":"    if proc.poll() is None:"},{"line_number":445,"context_line":"        proc_running \u003d True"}],"source_content_type":"text/x-python","patch_set":4,"id":"7af24918_271ec60b","line":442,"in_reply_to":"7af24918_0c081127","updated":"2016-03-04 14:31:33.000000000","message":"OK, I am running on old info here, sorry.","commit_id":"00b3b6171112be661f28beed00e69db7f841ef0f"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"3302c1af7f976929455fd281778b01c80c646797","unresolved":false,"context_lines":[{"line_number":439,"context_line":""},{"line_number":440,"context_line":"    finish_subprocess(tar_proc, tar_cmd)"},{"line_number":441,"context_line":""},{"line_number":442,"context_line":"def has_proc_terminated(proc):"},{"line_number":443,"context_line":"    proc_running \u003d False"},{"line_number":444,"context_line":"    if proc.poll() is None:"},{"line_number":445,"context_line":"        proc_running \u003d True"}],"source_content_type":"text/x-python","patch_set":4,"id":"7af24918_e5650145","line":442,"in_reply_to":"7af24918_c3aaa184","updated":"2016-03-04 11:59:28.000000000","message":"I am confused.\n\nThis is the tar process we are checking. If it has exited, why do we need to kill it? its already dead?\n\nWe already check the exit code in finish_subprocess.","commit_id":"00b3b6171112be661f28beed00e69db7f841ef0f"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"49cadf671a17fb2533cff915b3aefd2ae05ef0de","unresolved":false,"context_lines":[{"line_number":439,"context_line":""},{"line_number":440,"context_line":"    finish_subprocess(tar_proc, tar_cmd)"},{"line_number":441,"context_line":""},{"line_number":442,"context_line":"def has_proc_terminated(proc):"},{"line_number":443,"context_line":"    proc_running \u003d False"},{"line_number":444,"context_line":"    if proc.poll() is None:"},{"line_number":445,"context_line":"        proc_running \u003d True"}],"source_content_type":"text/x-python","patch_set":4,"id":"7af24918_0c081127","line":442,"in_reply_to":"7af24918_e5650145","updated":"2016-03-04 14:13:26.000000000","message":"My reading of the bug is that the tar process is not actually being killed. The mechanism isn\u0027t clear to me but the claim was that when it exited with return code 2 the process was actually still around and needed to be explicitly cleaned up which try_kill_process would do.","commit_id":"00b3b6171112be661f28beed00e69db7f841ef0f"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2f9e326559c5509c33bc74c1d48cbea3c1300643","unresolved":false,"context_lines":[{"line_number":133,"context_line":"    \"\"\"Sends the given process the SIGKILL signal.\"\"\""},{"line_number":134,"context_line":"    try:"},{"line_number":135,"context_line":"        pid \u003d proc.pid"},{"line_number":136,"context_line":"        os.lstat(\"/proc/%s\" % pid)"},{"line_number":137,"context_line":"    except Exception:"},{"line_number":138,"context_line":"        msg \u003d \"Tar process exited unexpectedly.\""},{"line_number":139,"context_line":"        LOG.info(\"%s\" % msg)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_8a66ccb8","line":136,"updated":"2016-03-04 12:02:04.000000000","message":"Why are we adding this here?","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":16366,"name":"Randall Nortman","email":"openstack@nortman.net","username":"rnortman"},"change_message_id":"1f0e54db19783afdcb8505ec68064e6950b75de3","unresolved":false,"context_lines":[{"line_number":134,"context_line":"    try:"},{"line_number":135,"context_line":"        pid \u003d proc.pid"},{"line_number":136,"context_line":"        os.lstat(\"/proc/%s\" % pid)"},{"line_number":137,"context_line":"    except Exception:"},{"line_number":138,"context_line":"        msg \u003d \"Tar process exited unexpectedly.\""},{"line_number":139,"context_line":"        LOG.info(\"%s\" % msg)"},{"line_number":140,"context_line":"        raise Exception(msg)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_db28a588","line":137,"updated":"2016-03-04 13:24:59.000000000","message":"I dislike catching all exceptions indiscriminately, and I really dislike doing that and then swallowing the original exception silently.  If we know what exception(s) we expect here, let\u0027s catch those specifically.  If we really want to catch everything, then we need to preserve the original exception, either in the logs (LOG.exception()) or preferably by re-raising the original exception (after logging your \"Tar process exited unexpectedly\" message) instead of creating a new exception.","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":13692,"name":"Roman Dobosz","email":"gryf73@gmail.com","username":"gryf"},"change_message_id":"906ea591fb440eb6b7c4280f07944afd9bf326ba","unresolved":false,"context_lines":[{"line_number":134,"context_line":"    try:"},{"line_number":135,"context_line":"        pid \u003d proc.pid"},{"line_number":136,"context_line":"        os.lstat(\"/proc/%s\" % pid)"},{"line_number":137,"context_line":"    except Exception:"},{"line_number":138,"context_line":"        msg \u003d \"Tar process exited unexpectedly.\""},{"line_number":139,"context_line":"        LOG.info(\"%s\" % msg)"},{"line_number":140,"context_line":"        raise Exception(msg)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_dbb8a5ca","line":137,"range":{"start_line":137,"start_character":4,"end_line":137,"end_character":21},"updated":"2016-03-04 13:36:28.000000000","message":"The Exception class is too broad, IMO. Just catch the OSError and check the errno.\n\nOther option for checking the pid on proc filesystem is to use it like:\n\n  if not os.path.exists(\"/proc/%s\" % proc.pid):\n    …\n\nWhich gives you what you\u0027re looking for.","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":13692,"name":"Roman Dobosz","email":"gryf73@gmail.com","username":"gryf"},"change_message_id":"906ea591fb440eb6b7c4280f07944afd9bf326ba","unresolved":false,"context_lines":[{"line_number":135,"context_line":"        pid \u003d proc.pid"},{"line_number":136,"context_line":"        os.lstat(\"/proc/%s\" % pid)"},{"line_number":137,"context_line":"    except Exception:"},{"line_number":138,"context_line":"        msg \u003d \"Tar process exited unexpectedly.\""},{"line_number":139,"context_line":"        LOG.info(\"%s\" % msg)"},{"line_number":140,"context_line":"        raise Exception(msg)"},{"line_number":141,"context_line":"    LOG.info(\"Killing process %s\" % pid)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_dbeb8511","line":138,"range":{"start_line":138,"start_character":0,"end_line":138,"end_character":48},"updated":"2016-03-04 13:36:28.000000000","message":"This function was generic before. Now you explicitly tell that something happen to tar process. Maybe a better idea is to either change the function name (and it\u0027s docstring) or pass the message as an argument.","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2f9e326559c5509c33bc74c1d48cbea3c1300643","unresolved":false,"context_lines":[{"line_number":136,"context_line":"        os.lstat(\"/proc/%s\" % pid)"},{"line_number":137,"context_line":"    except Exception:"},{"line_number":138,"context_line":"        msg \u003d \"Tar process exited unexpectedly.\""},{"line_number":139,"context_line":"        LOG.info(\"%s\" % msg)"},{"line_number":140,"context_line":"        raise Exception(msg)"},{"line_number":141,"context_line":"    LOG.info(\"Killing process %s\" % pid)"},{"line_number":142,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_0a111c04","line":139,"updated":"2016-03-04 12:02:04.000000000","message":"This should be a warning or an error log, I think.","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":16366,"name":"Randall Nortman","email":"openstack@nortman.net","username":"rnortman"},"change_message_id":"1f0e54db19783afdcb8505ec68064e6950b75de3","unresolved":false,"context_lines":[{"line_number":446,"context_line":"    finish_subprocess(tar_proc, tar_cmd)"},{"line_number":447,"context_line":""},{"line_number":448,"context_line":""},{"line_number":449,"context_line":"def has_proc_terminated(proc):"},{"line_number":450,"context_line":"    if proc.poll() \u003d\u003d 0:"},{"line_number":451,"context_line":"        proc_running \u003d False"},{"line_number":452,"context_line":"    elif proc.poll() is None:"}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_bb2b1981","line":449,"updated":"2016-03-04 13:24:59.000000000","message":"Instead of calling proc.poll() a bunch of times, instead call it once and save the result:\n\nexitcode \u003d proc.poll()\nif exitcode \u003d\u003d 0:\n   ...","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":16366,"name":"Randall Nortman","email":"openstack@nortman.net","username":"rnortman"},"change_message_id":"1f0e54db19783afdcb8505ec68064e6950b75de3","unresolved":false,"context_lines":[{"line_number":448,"context_line":""},{"line_number":449,"context_line":"def has_proc_terminated(proc):"},{"line_number":450,"context_line":"    if proc.poll() \u003d\u003d 0:"},{"line_number":451,"context_line":"        proc_running \u003d False"},{"line_number":452,"context_line":"    elif proc.poll() is None:"},{"line_number":453,"context_line":"        proc_running \u003d True"},{"line_number":454,"context_line":"    elif proc.poll() \u003d\u003d 2:"}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_1b1fad5b","line":451,"updated":"2016-03-04 13:24:59.000000000","message":"This is fine but you actually don\u0027t need to store this in a local var.  Just \"return False\" here, and \"return True\" below, and get rid of proc_running.","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2f9e326559c5509c33bc74c1d48cbea3c1300643","unresolved":false,"context_lines":[{"line_number":451,"context_line":"        proc_running \u003d False"},{"line_number":452,"context_line":"    elif proc.poll() is None:"},{"line_number":453,"context_line":"        proc_running \u003d True"},{"line_number":454,"context_line":"    elif proc.poll() \u003d\u003d 2:"},{"line_number":455,"context_line":"        raise SubprocessException(Exception)"},{"line_number":456,"context_line":"    return proc_running"},{"line_number":457,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_e59a2113","line":454,"updated":"2016-03-04 12:02:04.000000000","message":"I think this should be an else clause really.","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":16366,"name":"Randall Nortman","email":"openstack@nortman.net","username":"rnortman"},"change_message_id":"1f0e54db19783afdcb8505ec68064e6950b75de3","unresolved":false,"context_lines":[{"line_number":452,"context_line":"    elif proc.poll() is None:"},{"line_number":453,"context_line":"        proc_running \u003d True"},{"line_number":454,"context_line":"    elif proc.poll() \u003d\u003d 2:"},{"line_number":455,"context_line":"        raise SubprocessException(Exception)"},{"line_number":456,"context_line":"    return proc_running"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_5b3db5c2","line":455,"updated":"2016-03-04 13:24:59.000000000","message":"I\u0027m not sure what you\u0027re intending to raise here, but probably not this.  Maybe you want something like:\n\n    raise SubprocessException(\"Subprocess exited abnormally with exit code %s\" % exitcode)","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"d87572dba3a42a29e009a65dea8407d80678e319","unresolved":false,"context_lines":[{"line_number":452,"context_line":"    elif proc.poll() is None:"},{"line_number":453,"context_line":"        proc_running \u003d True"},{"line_number":454,"context_line":"    elif proc.poll() \u003d\u003d 2:"},{"line_number":455,"context_line":"        raise SubprocessException(Exception)"},{"line_number":456,"context_line":"    return proc_running"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_ac3c7d93","line":455,"in_reply_to":"7af24918_5b3db5c2","updated":"2016-03-04 14:16:06.000000000","message":"SubprocessException is defined on line 91 above and takes 4 parameters. Those are not available here but would need to be passed in to use that exception class.","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2f9e326559c5509c33bc74c1d48cbea3c1300643","unresolved":false,"context_lines":[{"line_number":453,"context_line":"        proc_running \u003d True"},{"line_number":454,"context_line":"    elif proc.poll() \u003d\u003d 2:"},{"line_number":455,"context_line":"        raise SubprocessException(Exception)"},{"line_number":456,"context_line":"    return proc_running"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":""},{"line_number":459,"context_line":"def extract_tarball(fileobj, path, callback\u003dNone):"}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_ca63b4af","line":456,"updated":"2016-03-04 12:02:04.000000000","message":"I don\u0027t think this is correct.\n\nIf the process is dead, why should we try kill it again?\n\nWe already check the error code, and raise appropriately later on.","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"d87572dba3a42a29e009a65dea8407d80678e319","unresolved":false,"context_lines":[{"line_number":453,"context_line":"        proc_running \u003d True"},{"line_number":454,"context_line":"    elif proc.poll() \u003d\u003d 2:"},{"line_number":455,"context_line":"        raise SubprocessException(Exception)"},{"line_number":456,"context_line":"    return proc_running"},{"line_number":457,"context_line":""},{"line_number":458,"context_line":""},{"line_number":459,"context_line":"def extract_tarball(fileobj, path, callback\u003dNone):"}],"source_content_type":"text/x-python","patch_set":7,"id":"7af24918_2c94ed88","line":456,"in_reply_to":"7af24918_ca63b4af","updated":"2016-03-04 14:16:06.000000000","message":"The reported bug is saying that even though tar has exited with return code 2 it\u0027s still around. I\u0027m not clear on how that\u0027s happening, but that\u0027s what this is attempting to fix.","commit_id":"7d606e1f117349ace04bdcc50a4aece6171c5885"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"f18240d3ee81a2a9d4428eaadbecd763c566ad09","unresolved":false,"context_lines":[{"line_number":462,"context_line":"            tar_proc.stdin.write(chunk)"},{"line_number":463,"context_line":""},{"line_number":464,"context_line":"            returncode \u003d tar_proc.poll()"},{"line_number":465,"context_line":"            if returncode is not None:"},{"line_number":466,"context_line":"                LOG.error(\"tar extract with process id \u0027%(pid)s\u0027 \""},{"line_number":467,"context_line":"                          \"exited early with \u0027%(rc)s\u0027\" %"},{"line_number":468,"context_line":"                          {\u0027pid\u0027: tar_proc.pid, \u0027rc\u0027: returncode})"}],"source_content_type":"text/x-python","patch_set":11,"id":"7af24918_52f7448c","line":465,"updated":"2016-03-04 18:35:54.000000000","message":"If returncode is 0 then tar exited cleanly.","commit_id":"18a1e4c670f298ae12944f20be1946555aafa51d"},{"author":{"_account_id":16366,"name":"Randall Nortman","email":"openstack@nortman.net","username":"rnortman"},"change_message_id":"8a191a755f9ca83c6166fd0007dab6a61b0f6a31","unresolved":false,"context_lines":[{"line_number":462,"context_line":"            tar_proc.stdin.write(chunk)"},{"line_number":463,"context_line":""},{"line_number":464,"context_line":"            returncode \u003d tar_proc.poll()"},{"line_number":465,"context_line":"            if returncode is not None:"},{"line_number":466,"context_line":"                LOG.error(\"tar extract with process id \u0027%(pid)s\u0027 \""},{"line_number":467,"context_line":"                          \"exited early with \u0027%(rc)s\u0027\" %"},{"line_number":468,"context_line":"                          {\u0027pid\u0027: tar_proc.pid, \u0027rc\u0027: returncode})"}],"source_content_type":"text/x-python","patch_set":11,"id":"7af24918_0ddc7d45","line":465,"in_reply_to":"7af24918_52f7448c","updated":"2016-03-04 19:01:09.000000000","message":"I think the idea is that if it exits before we\u0027ve finished writing data to it, then it exited early, whatever the exit code.  But most importantly for this fix we need to notice if the original process died.  In that case, we need to quit trying to write anything to it and raise an exception so this task errors out instead of hanging.  Checking for tar_proc.poll() not None does the trick I think.","commit_id":"18a1e4c670f298ae12944f20be1946555aafa51d"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"f18240d3ee81a2a9d4428eaadbecd763c566ad09","unresolved":false,"context_lines":[{"line_number":471,"context_line":""},{"line_number":472,"context_line":"    except SubprocessException:"},{"line_number":473,"context_line":"        # no need to kill already dead process"},{"line_number":474,"context_line":"        raise"},{"line_number":475,"context_line":"    except Exception:"},{"line_number":476,"context_line":"        LOG.exception(\"Failed while sending data to tar pid: %s\" % pid)"},{"line_number":477,"context_line":"        try_kill_process(tar_proc)"}],"source_content_type":"text/x-python","patch_set":11,"id":"7af24918_d21074ae","line":474,"updated":"2016-03-04 18:35:54.000000000","message":"I\u0027m confused about what this is for. My understanding was that Tim wanted try_kill_process to run when returncode was not None and not 0.","commit_id":"18a1e4c670f298ae12944f20be1946555aafa51d"},{"author":{"_account_id":10896,"name":"Tim Pownall","email":"pownalltim@gmail.com","username":"pownalltim"},"change_message_id":"080d69528bee208eb49a1830de317663e0280244","unresolved":false,"context_lines":[{"line_number":471,"context_line":""},{"line_number":472,"context_line":"    except SubprocessException:"},{"line_number":473,"context_line":"        # no need to kill already dead process"},{"line_number":474,"context_line":"        raise"},{"line_number":475,"context_line":"    except Exception:"},{"line_number":476,"context_line":"        LOG.exception(\"Failed while sending data to tar pid: %s\" % pid)"},{"line_number":477,"context_line":"        try_kill_process(tar_proc)"}],"source_content_type":"text/x-python","patch_set":11,"id":"7af24918_18431d6e","line":474,"in_reply_to":"7af24918_d21074ae","updated":"2016-03-04 19:44:14.000000000","message":"so polling for the process will close the pipe and the tar will exit on it\u0027s own, that\u0027s why i had to put the check to make sure the process still existed.  I just left the os.kill in there for edge cases.","commit_id":"18a1e4c670f298ae12944f20be1946555aafa51d"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"f7c959b40356487b4eb59f16e80f1316db400aa0","unresolved":false,"context_lines":[{"line_number":463,"context_line":""},{"line_number":464,"context_line":"            returncode \u003d tar_proc.poll()"},{"line_number":465,"context_line":"            tar_pid \u003d tar_proc.pid"},{"line_number":466,"context_line":"            if returncode is not None:"},{"line_number":467,"context_line":"                LOG.error(\"tar extract with process id \u0027%(pid)s\u0027 \""},{"line_number":468,"context_line":"                          \"exited early with \u0027%(rc)s\u0027\" %"},{"line_number":469,"context_line":"                          {\u0027pid\u0027: tar_pid, \u0027rc\u0027: returncode})"}],"source_content_type":"text/x-python","patch_set":13,"id":"5aef4532_435d9fe8","line":466,"updated":"2016-03-07 16:15:10.000000000","message":"I am a little worried about how success if handled, and if that will be spotted here too, as a race condition, but the CI seems to be +1ing OK right now, which makes me happier about that...","commit_id":"de6894e7901e17bc69c65d93877760df2947e0df"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"e7a9eb648f5726ea296cec216e4894f274bc8ec8","unresolved":false,"context_lines":[{"line_number":463,"context_line":""},{"line_number":464,"context_line":"            returncode \u003d tar_proc.poll()"},{"line_number":465,"context_line":"            tar_pid \u003d tar_proc.pid"},{"line_number":466,"context_line":"            if returncode is not None:"},{"line_number":467,"context_line":"                LOG.error(\"tar extract with process id \u0027%(pid)s\u0027 \""},{"line_number":468,"context_line":"                          \"exited early with \u0027%(rc)s\u0027\" %"},{"line_number":469,"context_line":"                          {\u0027pid\u0027: tar_pid, \u0027rc\u0027: returncode})"}],"source_content_type":"text/x-python","patch_set":13,"id":"5aef4532_e3582b59","line":466,"in_reply_to":"5aef4532_435d9fe8","updated":"2016-03-07 16:23:34.000000000","message":"Actually thinking about it, I don\u0027t think tar should exit until the pipe is closed, which happens during finish_subprocess, when we call proc.communicate, so this should be OK, I think. But I really wish I understood that a bit better to be sure.","commit_id":"de6894e7901e17bc69c65d93877760df2947e0df"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"7bbafcb03eb803ddc2778519619b179e0906e2de","unresolved":false,"context_lines":[{"line_number":471,"context_line":"                    \u0027 \u0027.join(tar_cmd), returncode, \"\", \"\")"},{"line_number":472,"context_line":""},{"line_number":473,"context_line":"    except SubprocessException:"},{"line_number":474,"context_line":"        # no need to kill already dead process"},{"line_number":475,"context_line":"        raise"},{"line_number":476,"context_line":"    except Exception:"},{"line_number":477,"context_line":"        LOG.exception(\"Failed while sending data to tar pid: %s\" % tar_pid)"}],"source_content_type":"text/x-python","patch_set":13,"id":"5aef4532_7e3792e3","line":474,"updated":"2016-03-07 16:25:22.000000000","message":"I\u0027m still suffering a disconnect somewhere. The original claim was that there was a process that needed cleaning up here, if that\u0027s not the case then why raise this? It\u0027s not clear to me what this exception raise is actually accomplishing.","commit_id":"de6894e7901e17bc69c65d93877760df2947e0df"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"b871cc6eb15236ff36adcc0d313deeeecf33ebcd","unresolved":false,"context_lines":[{"line_number":471,"context_line":"                    \u0027 \u0027.join(tar_cmd), returncode, \"\", \"\")"},{"line_number":472,"context_line":""},{"line_number":473,"context_line":"    except SubprocessException:"},{"line_number":474,"context_line":"        # no need to kill already dead process"},{"line_number":475,"context_line":"        raise"},{"line_number":476,"context_line":"    except Exception:"},{"line_number":477,"context_line":"        LOG.exception(\"Failed while sending data to tar pid: %s\" % tar_pid)"}],"source_content_type":"text/x-python","patch_set":13,"id":"5aef4532_deaa2619","line":474,"in_reply_to":"5aef4532_7e3792e3","updated":"2016-03-07 16:37:25.000000000","message":"I don\u0027t know where the process that needs killing came from myself. I think this is a process that dies early, that becomes a zombie while the parent is still running and has a pipe open to it.\n\nThis exception raise ensures that when tar exits early (for example, when it sees bad input), we notice, stop trying to send more data to tar, and raise the exception back up to Nova over the xenapi RPC thingy bob.\n\nIf we didn\u0027t check this, we would fill up the stdin buffer, and evenutally block on that, I believe. Leaving to a stuck process, that has no timeout (actually I think xenapi might eventually timeout).","commit_id":"de6894e7901e17bc69c65d93877760df2947e0df"},{"author":{"_account_id":10896,"name":"Tim Pownall","email":"pownalltim@gmail.com","username":"pownalltim"},"change_message_id":"ac1a773730ecfee23427be1eec2558197036d712","unresolved":false,"context_lines":[{"line_number":471,"context_line":"                    \u0027 \u0027.join(tar_cmd), returncode, \"\", \"\")"},{"line_number":472,"context_line":""},{"line_number":473,"context_line":"    except SubprocessException:"},{"line_number":474,"context_line":"        # no need to kill already dead process"},{"line_number":475,"context_line":"        raise"},{"line_number":476,"context_line":"    except Exception:"},{"line_number":477,"context_line":"        LOG.exception(\"Failed while sending data to tar pid: %s\" % tar_pid)"}],"source_content_type":"text/x-python","patch_set":13,"id":"5aef4532_beef5aaf","line":474,"in_reply_to":"5aef4532_7e3792e3","updated":"2016-03-07 16:34:46.000000000","message":"The process does not need cleaning up. The kill logic has been part of the script for a long time, however when we pull the tar pid to kill via tar_proc.pid, it\u0027s the pid that  already exited, but hangs indefinitely until the return code is read. When we poll for the return code the process goes away and the pipe closes and the script exits and respawns under glance api retry logic.  The kill logic still exists for edge cases where tar may be stuck for other reasons.","commit_id":"de6894e7901e17bc69c65d93877760df2947e0df"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"108cca03dbbb46134364d468dc7d9e3c29e369d5","unresolved":false,"context_lines":[{"line_number":471,"context_line":"                    \u0027 \u0027.join(tar_cmd), returncode, \"\", \"\")"},{"line_number":472,"context_line":""},{"line_number":473,"context_line":"    except SubprocessException:"},{"line_number":474,"context_line":"        # no need to kill already dead process"},{"line_number":475,"context_line":"        raise"},{"line_number":476,"context_line":"    except Exception:"},{"line_number":477,"context_line":"        LOG.exception(\"Failed while sending data to tar pid: %s\" % tar_pid)"}],"source_content_type":"text/x-python","patch_set":13,"id":"5aef4532_de9346f9","line":474,"in_reply_to":"5aef4532_beef5aaf","updated":"2016-03-07 16:36:52.000000000","message":"Okay, that makes sense but is not at all obvious here. A comment explaining that should be added so that later changes don\u0027t re-introduce the problem by not realizing the importance of the poll().","commit_id":"de6894e7901e17bc69c65d93877760df2947e0df"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"da9275f111aece4f3713f8d357f37698e651d99f","unresolved":false,"context_lines":[{"line_number":461,"context_line":""},{"line_number":462,"context_line":"            tar_proc.stdin.write(chunk)"},{"line_number":463,"context_line":""},{"line_number":464,"context_line":"            returncode \u003d tar_proc.poll()"},{"line_number":465,"context_line":"            tar_pid \u003d tar_proc.pid"},{"line_number":466,"context_line":"            if returncode is not None:"},{"line_number":467,"context_line":"                LOG.error(\"tar extract with process id \u0027%(pid)s\u0027 \""}],"source_content_type":"text/x-python","patch_set":14,"id":"5aef4532_2a593f5b","line":464,"updated":"2016-03-07 17:59:29.000000000","message":"In addition to the commit message there should be a comment here about the importance of this line.","commit_id":"7c57c431f0ca3efc02219a4b0087b130f1d97b0b"}]}