)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"5190fa1f46fd6efb8347e0add627bb84a3cf7223","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Joshua Harlow \u003charlowja@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2015-08-29 21:59:58 -0700"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Use psutil and TERM to KILL backoff loop"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Instead of directly trying to SIGKILL a active rsync/scp process"},{"line_number":10,"context_line":"it is nicer to first try to give that process a SIGTERM so that it"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"da20952f_6e977748","line":7,"updated":"2015-08-30 05:16:38.000000000","message":"please add libvirt: prefix if you respin","commit_id":"46309b7afada0e3a2ffe040b59fb951e5df7a9db"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"cf61f99dfe0f30c256cc98eebcd1b4179ed509ea","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Joshua Harlow \u003charlowja@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2015-08-29 21:59:58 -0700"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Use psutil and TERM to KILL backoff loop"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Instead of directly trying to SIGKILL a active rsync/scp process"},{"line_number":10,"context_line":"it is nicer to first try to give that process a SIGTERM so that it"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"da20952f_78dad4e9","line":7,"in_reply_to":"da20952f_6e977748","updated":"2015-08-31 17:35:24.000000000","message":"Done","commit_id":"46309b7afada0e3a2ffe040b59fb951e5df7a9db"}],"nova/virt/libvirt/instancejobtracker.py":[{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"3fb6a62c514353fe7d0c8b9cd833ef42884740d5","unresolved":false,"context_lines":[{"line_number":25,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"def _try_term_then_kill(pid, wait_delay\u003d0.1, interrupt_attempts\u003d3):"},{"line_number":29,"context_line":"    \"\"\"Tries to SIGTERM the pid, then if that doesn\u0027t work SIGKILL(s) it.\"\"\""},{"line_number":30,"context_line":"    try:"},{"line_number":31,"context_line":"        p \u003d psutil.Process(pid)"}],"source_content_type":"text/x-python","patch_set":2,"id":"da20952f_08891375","line":28,"updated":"2015-08-30 01:26:31.000000000","message":"would it be possible to have a unit test(s) that cover all of the flows of this method?","commit_id":"74421dfc47b1869356b91976ad65ca1bc2d13433"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"5b8d9b6ea741fb16578a7b0c598729a4e1ee7887","unresolved":false,"context_lines":[{"line_number":25,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"def _try_term_then_kill(pid, wait_delay\u003d0.1, interrupt_attempts\u003d3):"},{"line_number":29,"context_line":"    \"\"\"Tries to SIGTERM the pid, then if that doesn\u0027t work SIGKILL(s) it.\"\"\""},{"line_number":30,"context_line":"    try:"},{"line_number":31,"context_line":"        p \u003d psutil.Process(pid)"}],"source_content_type":"text/x-python","patch_set":2,"id":"da20952f_abe581a4","line":28,"in_reply_to":"da20952f_08891375","updated":"2015-08-30 04:10:29.000000000","message":"yup, working on that.","commit_id":"74421dfc47b1869356b91976ad65ca1bc2d13433"},{"author":{"_account_id":14819,"name":"Timofey Durakov","email":"timofei.nd@gmail.com","username":"tdurakov"},"change_message_id":"b33d3344f602c85c5f530ed1b5a074a970c668da","unresolved":false,"context_lines":[{"line_number":67,"context_line":""},{"line_number":68,"context_line":"class InstanceJobTracker(object):"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"    term_attempts \u003d 3"},{"line_number":71,"context_line":"    \"\"\""},{"line_number":72,"context_line":"    When trying to stop a active process, these many SIGTERM attempts will"},{"line_number":73,"context_line":"    be tried. If the process does not respond to those SIGTERM attempts it"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_a9de0dd8","line":70,"updated":"2015-09-10 21:38:59.000000000","message":"Is it really required to be class instance member? maybe it would be better to move that to constant on module level?","commit_id":"b3d722ba1623caff826d94f2871ea443168e31a9"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"2fc11992fb86d5c38516c845610a28fbc4226cde","unresolved":false,"context_lines":[{"line_number":67,"context_line":""},{"line_number":68,"context_line":"class InstanceJobTracker(object):"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"    term_attempts \u003d 3"},{"line_number":71,"context_line":"    \"\"\""},{"line_number":72,"context_line":"    When trying to stop a active process, these many SIGTERM attempts will"},{"line_number":73,"context_line":"    be tried. If the process does not respond to those SIGTERM attempts it"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_43214a9d","line":70,"in_reply_to":"da20952f_a9de0dd8","updated":"2015-09-11 00:52:57.000000000","message":"Meh :-P","commit_id":"b3d722ba1623caff826d94f2871ea443168e31a9"},{"author":{"_account_id":14819,"name":"Timofey Durakov","email":"timofei.nd@gmail.com","username":"tdurakov"},"change_message_id":"b33d3344f602c85c5f530ed1b5a074a970c668da","unresolved":false,"context_lines":[{"line_number":74,"context_line":"    will be stopped via SIGKILL forcefully."},{"line_number":75,"context_line":"    \"\"\""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"    death_wait_delay \u003d 0.1"},{"line_number":78,"context_line":"    \"\"\""},{"line_number":79,"context_line":"    When trying to stop a active process, and after a SIGTERM/SIGKILL has been"},{"line_number":80,"context_line":"    sent this process will wait this many seconds waiting for that process to"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_09e4d9a5","line":77,"updated":"2015-09-10 21:38:59.000000000","message":"ditto","commit_id":"b3d722ba1623caff826d94f2871ea443168e31a9"},{"author":{"_account_id":17139,"name":"Chuck Carmack","email":"carmack@us.ibm.com","username":"ccarmack"},"change_message_id":"7ba4067a122ddab722eb32d0db22294452afa3f3","unresolved":false,"context_lines":[{"line_number":141,"context_line":"                                 \"deleting it.\"), {\u0027pid\u0027: pid},"},{"line_number":142,"context_line":"                             instance\u003dinstance)"},{"line_number":143,"context_line":"                else:"},{"line_number":144,"context_line":"                    self.remove_job(instance, pid)"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_53078e81","line":144,"updated":"2015-08-31 18:49:48.000000000","message":"Does this change the behavior from the old code?  In the old code it looks like the job was removed even if the process was still around.  Are we assuming that the p.kill( ) call will eventually kill it?","commit_id":"b3d722ba1623caff826d94f2871ea443168e31a9"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"001405dcbb9bbb9a642af4326f58ecb3357f2bd8","unresolved":false,"context_lines":[{"line_number":141,"context_line":"                                 \"deleting it.\"), {\u0027pid\u0027: pid},"},{"line_number":142,"context_line":"                             instance\u003dinstance)"},{"line_number":143,"context_line":"                else:"},{"line_number":144,"context_line":"                    self.remove_job(instance, pid)"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_f47a4823","line":144,"in_reply_to":"da20952f_53078e81","updated":"2015-08-31 19:47:53.000000000","message":"I\u0027m hoping so, but it seems sorta odd to remove it if the process is really still around (for some reason SIGKILL didn\u0027t work out); I can just remove it, but the prior behavior seems like the wrong behavior from what I feel. Thoughts?","commit_id":"b3d722ba1623caff826d94f2871ea443168e31a9"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"2b40ce3cf49e1f2539c0de62dd15d5340f6e122a","unresolved":false,"context_lines":[{"line_number":141,"context_line":"                                 \"deleting it.\"), {\u0027pid\u0027: pid},"},{"line_number":142,"context_line":"                             instance\u003dinstance)"},{"line_number":143,"context_line":"                else:"},{"line_number":144,"context_line":"                    self.remove_job(instance, pid)"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_90df4ed5","line":144,"in_reply_to":"da20952f_5e7eae6a","updated":"2015-09-01 01:12:40.000000000","message":"Hmmm, well I can remove it always, but the removal won\u0027t actually mean the process is dead; so I\u0027ll let a core reviewer express there input since I\u0027m not sure either. Either way can be done, which is *better* who knows, ha.","commit_id":"b3d722ba1623caff826d94f2871ea443168e31a9"},{"author":{"_account_id":14819,"name":"Timofey Durakov","email":"timofei.nd@gmail.com","username":"tdurakov"},"change_message_id":"b33d3344f602c85c5f530ed1b5a074a970c668da","unresolved":false,"context_lines":[{"line_number":141,"context_line":"                                 \"deleting it.\"), {\u0027pid\u0027: pid},"},{"line_number":142,"context_line":"                             instance\u003dinstance)"},{"line_number":143,"context_line":"                else:"},{"line_number":144,"context_line":"                    self.remove_job(instance, pid)"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_097ff9df","line":144,"in_reply_to":"da20952f_90df4ed5","updated":"2015-09-10 21:38:59.000000000","message":"I think, deletion of live process wasn\u0027t good idea","commit_id":"b3d722ba1623caff826d94f2871ea443168e31a9"},{"author":{"_account_id":17139,"name":"Chuck Carmack","email":"carmack@us.ibm.com","username":"ccarmack"},"change_message_id":"ad79714c3fdd3ef1115e2136e5481a352df094ae","unresolved":false,"context_lines":[{"line_number":141,"context_line":"                                 \"deleting it.\"), {\u0027pid\u0027: pid},"},{"line_number":142,"context_line":"                             instance\u003dinstance)"},{"line_number":143,"context_line":"                else:"},{"line_number":144,"context_line":"                    self.remove_job(instance, pid)"}],"source_content_type":"text/x-python","patch_set":4,"id":"da20952f_5e7eae6a","line":144,"in_reply_to":"da20952f_f47a4823","updated":"2015-08-31 20:49:40.000000000","message":"I looked into the code calling terminate_jobs (this is all new to me :-) )\n\nThere is a periodic task that would later remove the job from the cache, but only if the instance is marked as not cleaned\nhttps://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6402\n\nThe function directly calling terminate_jobs is\nhttps://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L7091\n\nIf delete_instance_files() returns true then the instance is marked cleaned.   delete_instance_files() returns true when a directory tree is removed.  It doesn\u0027t look like the result of terminate_jobs matters for the true/false return.\n\nIt looks like it was assumed that terminate_jobs will kill all the processes eventually, and to avoid the job leaking in the cache, they always removed it.\n\nThe prior behavior does seem wrong; maybe delete_instance_files should have returned false if terminate_jobs failed, but the way code is now, seems like the job should always be removed.\n\nAnyway, a core reviewer will look at this I\u0027m sure.","commit_id":"b3d722ba1623caff826d94f2871ea443168e31a9"}]}