)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":13734,"name":"Nell Jerram","email":"nell@tigera.io","username":"neiljerram"},"change_message_id":"c125e06dac41a93969daceb2f2e67f1c81eaf480","unresolved":false,"context_lines":[{"line_number":9,"context_line":"There is a race condition that occurs over NFS when multiple instances"},{"line_number":10,"context_line":"are being created where utime fails, due to some other process"},{"line_number":11,"context_line":"modifying the file path. This patch locks the file to ensure it can"},{"line_number":12,"context_line":"modify the times."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Closes-Bug: 1809123"},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"9fdfeff1_cde738ac","line":12,"updated":"2019-01-28 12:49:13.000000000","message":"In fact it just opens the file.  Is that the same as locking it?  That wasn\u0027t obvious to me, if so.\n\nIt feels like we\u0027re actually working around some NFS or kernel bug here.","commit_id":"07ed1c3292b46ecddd32ba22386795df732f77a0"}],"nova/privsep/path.py":[{"author":{"_account_id":6167,"name":"Ken\u0027ichi Ohmichi","email":"ken1ohmichi@gmail.com","username":"oomichi"},"change_message_id":"6be0d11c6fdf77a3397f8d1475ca43bc43ae284e","unresolved":false,"context_lines":[{"line_number":74,"context_line":"    # NOTE(mikal): the old version of this used execute(touch, ...), which"},{"line_number":75,"context_line":"    # would apparently fail on shared storage when multiple instances were"},{"line_number":76,"context_line":"    # being launched at the same time. If we see failures here, we might need"},{"line_number":77,"context_line":"    # to wrap this in a try / except."},{"line_number":78,"context_line":"    os.utime(path, None)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_125be488","side":"PARENT","line":77,"updated":"2018-12-21 02:09:00.000000000","message":"This seems to come from https://review.openstack.org/#/c/489190/21/nova/virt/libvirt/utils.py\n\nIt is nice to get feedback from mikal.","commit_id":"48ad73e1faf966badab1f0344baad9f4f4055abf"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"03fab510e734654295e1480b48fa9498df215d26","unresolved":false,"context_lines":[{"line_number":74,"context_line":"    # NOTE(mikal): the old version of this used execute(touch, ...), which"},{"line_number":75,"context_line":"    # would apparently fail on shared storage when multiple instances were"},{"line_number":76,"context_line":"    # being launched at the same time. If we see failures here, we might need"},{"line_number":77,"context_line":"    # to wrap this in a try / except."},{"line_number":78,"context_line":"    os.utime(path, None)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_79eacd0e","side":"PARENT","line":77,"in_reply_to":"3f79a3b5_125be488","updated":"2019-02-21 17:13:47.000000000","message":"\u003e This seems to come from https://review.openstack.org/#/c/489190/21/nova/virt/libvirt/utils.py\n \u003e \n \u003e It is nice to get feedback from mikal.\n\nAnd that originally came from this change:\n\nhttps://review.openstack.org/#/c/386956/\n\nI also found this change:\n\nhttps://review.openstack.org/#/c/366857/4/nova/virt/libvirt/driver.py\n\nAnd apparently that code is still using os.utime directly rather than privsep (maybe mikal has a patch up to convert that though).","commit_id":"48ad73e1faf966badab1f0344baad9f4f4055abf"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"ea59f8d6ad174a88258024614195c2a673a67723","unresolved":false,"context_lines":[{"line_number":74,"context_line":"    # NOTE(mikal): the old version of this used execute(touch, ...), which"},{"line_number":75,"context_line":"    # would apparently fail on shared storage when multiple instances were"},{"line_number":76,"context_line":"    # being launched at the same time. If we see failures here, we might need"},{"line_number":77,"context_line":"    # to wrap this in a try / except."},{"line_number":78,"context_line":"    os.utime(path, None)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_3d5b4889","side":"PARENT","line":77,"in_reply_to":"3f79a3b5_125be488","updated":"2018-12-21 16:15:05.000000000","message":"Yeah I\u0027m not sure why we are dropping this as it\u0027s just legacy context. If anything I\u0027d like see an additional comment added here detailing why the call to open is required.","commit_id":"48ad73e1faf966badab1f0344baad9f4f4055abf"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"587b02b394b552a900a4f057c48da4a8c2f97a68","unresolved":false,"context_lines":[{"line_number":74,"context_line":"    # NOTE(mikal): the old version of this used execute(touch, ...), which"},{"line_number":75,"context_line":"    # would apparently fail on shared storage when multiple instances were"},{"line_number":76,"context_line":"    # being launched at the same time. If we see failures here, we might need"},{"line_number":77,"context_line":"    # to wrap this in a try / except."},{"line_number":78,"context_line":"    os.utime(path, None)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_79d40d20","side":"PARENT","line":77,"in_reply_to":"3f79a3b5_38e91681","updated":"2019-02-21 17:18:15.000000000","message":"\u003e I removed this one because I believe the issue with multiple instances being launched simultaneously is fixed by this patch.\n\nWe still want a comment to explain the context of *why* the open() is being used - if it\u0027s to guard against concurrent access then just comment to that effect, otherwise - as you\u0027ve seen in this discussion thread - people have to dig through git history to figure out how we got here.","commit_id":"48ad73e1faf966badab1f0344baad9f4f4055abf"},{"author":{"_account_id":13734,"name":"Nell Jerram","email":"nell@tigera.io","username":"neiljerram"},"change_message_id":"c125e06dac41a93969daceb2f2e67f1c81eaf480","unresolved":false,"context_lines":[{"line_number":74,"context_line":"    # NOTE(mikal): the old version of this used execute(touch, ...), which"},{"line_number":75,"context_line":"    # would apparently fail on shared storage when multiple instances were"},{"line_number":76,"context_line":"    # being launched at the same time. If we see failures here, we might need"},{"line_number":77,"context_line":"    # to wrap this in a try / except."},{"line_number":78,"context_line":"    os.utime(path, None)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_cd8098c2","side":"PARENT","line":77,"in_reply_to":"3f79a3b5_38e91681","updated":"2019-01-28 12:49:13.000000000","message":"FWIW I\u0027ve also been using no_root_squash in my testing, and saw the problem that is being addressed here.","commit_id":"48ad73e1faf966badab1f0344baad9f4f4055abf"},{"author":{"_account_id":17280,"name":"Tim Rozet","email":"trozet@redhat.com","username":"trozet"},"change_message_id":"65403abc10c6a3c0833a20bf54c448a08bedf728","unresolved":false,"context_lines":[{"line_number":74,"context_line":"    # NOTE(mikal): the old version of this used execute(touch, ...), which"},{"line_number":75,"context_line":"    # would apparently fail on shared storage when multiple instances were"},{"line_number":76,"context_line":"    # being launched at the same time. If we see failures here, we might need"},{"line_number":77,"context_line":"    # to wrap this in a try / except."},{"line_number":78,"context_line":"    os.utime(path, None)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_38e91681","side":"PARENT","line":77,"in_reply_to":"3f79a3b5_3d5b4889","updated":"2018-12-21 17:24:38.000000000","message":"Sure I can add a comment. I removed this one because I believe the issue with multiple instances being launched simultaneously is fixed by this patch. As to why open fixes the problem, I\u0027m still not 100% sure (although I am sure it fixes it from my testing).\n\nMy theory was that some time between when path exists and os.utime was being executed, something else was either deleting/recreating the file or changing permissions on the file due to multiple instances being launched. The open is checking if the file exists first and if it isn\u0027t then it creates it (and creating a FD during the context of updating the time), which is avoiding the race condition. There is a possibility that even with the file open, during the utime the file could be deleted, but that doesn\u0027t seem to happen.\n\nI also thought about this possibility:\nI\u0027m not using no_root_squash on my NFS. I\u0027ve read that that could mean that the root user on the client machine is mapped to an unprivileged user, which could explain the permission denied. However, it wouldn\u0027t explain being able to open the file and this fix working. Additionally, when Martin reproduced this I believe he was using no_root_squash so I doubt this is the reason.","commit_id":"48ad73e1faf966badab1f0344baad9f4f4055abf"},{"author":{"_account_id":17280,"name":"Tim Rozet","email":"trozet@redhat.com","username":"trozet"},"change_message_id":"7b726d30e57dfad8662ead695ba550cc2905deab","unresolved":false,"context_lines":[{"line_number":74,"context_line":"    # NOTE(mikal): the old version of this used execute(touch, ...), which"},{"line_number":75,"context_line":"    # would apparently fail on shared storage when multiple instances were"},{"line_number":76,"context_line":"    # being launched at the same time. If we see failures here, we might need"},{"line_number":77,"context_line":"    # to wrap this in a try / except."},{"line_number":78,"context_line":"    os.utime(path, None)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_b9a4b54c","side":"PARENT","line":77,"in_reply_to":"9fdfeff1_79d40d20","updated":"2019-02-21 17:37:40.000000000","message":"I agree with you. I was just hoping someone else would shed some light as to why this fixes it, as I\u0027m not 100% sure (my theory in my previous comment). I could just add a general comment like the one you suggested if you think that is good enough and we are satisfied as is.","commit_id":"48ad73e1faf966badab1f0344baad9f4f4055abf"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3baf550eddf896ddd2e493967381433c5c54817f","unresolved":false,"context_lines":[{"line_number":74,"context_line":"    # NOTE(mikal): the old version of this used execute(touch, ...), which"},{"line_number":75,"context_line":"    # would apparently fail on shared storage when multiple instances were"},{"line_number":76,"context_line":"    # being launched at the same time. If we see failures here, we might need"},{"line_number":77,"context_line":"    # to wrap this in a try / except."},{"line_number":78,"context_line":"    os.utime(path, None)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_b54888b3","side":"PARENT","line":77,"in_reply_to":"9fdfeff1_b9a4b54c","updated":"2019-03-01 21:43:38.000000000","message":"\u003e I agree with you. I was just hoping someone else would shed some\n \u003e light as to why this fixes it, as I\u0027m not 100% sure (my theory in\n \u003e my previous comment). I could just add a general comment like the\n \u003e one you suggested if you think that is good enough and we are\n \u003e satisfied as is.\n\nSure that works for me. If you can also work references into the commit message about the previous related changes (my comment from Feb 21) that would also be good for git blamers in the future.","commit_id":"48ad73e1faf966badab1f0344baad9f4f4055abf"},{"author":{"_account_id":16771,"name":"mpolenchuk","email":"mpolenchuk@mirantis.com","username":"mpolenchuk"},"change_message_id":"c2b14d9836f8b9ca14c44333f823eb726d639ff4","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    if not os.path.exists(path):"},{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":74,"context_line":"        os.utime(path, None)"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"@nova.privsep.sys_admin_pctxt.entrypoint"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_c4413ba8","line":74,"range":{"start_line":74,"start_character":8,"end_line":74,"end_character":28},"updated":"2019-02-13 13:51:20.000000000","message":"Maybe it\u0027d be better to wrap it up with smth like this?\n  try:\n    os.utime(path, None)\n  except OSError as e:\n    if e.errno !\u003d errno.EACCES:\n      raise e","commit_id":"07ed1c3292b46ecddd32ba22386795df732f77a0"},{"author":{"_account_id":17280,"name":"Tim Rozet","email":"trozet@redhat.com","username":"trozet"},"change_message_id":"7b726d30e57dfad8662ead695ba550cc2905deab","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    if not os.path.exists(path):"},{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":74,"context_line":"        os.utime(path, None)"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"@nova.privsep.sys_admin_pctxt.entrypoint"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_b97915c6","line":74,"range":{"start_line":74,"start_character":8,"end_line":74,"end_character":28},"in_reply_to":"9fdfeff1_c4413ba8","updated":"2019-02-21 17:37:40.000000000","message":"Well if we can avoid the error in the first place by using with open context, wouldn\u0027t that be preferred?","commit_id":"07ed1c3292b46ecddd32ba22386795df732f77a0"}]}