)]}'
{"nova/privsep/path.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"5fe3a701c8ac2880b036a257062c59503cdd4849","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    # context wrapper ensures the file exists before trying to modify time"},{"line_number":74,"context_line":"    # which fixes a race condition with NFS image caching (see LP#1809123)"},{"line_number":75,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":76,"context_line":"        os.utime(path, None)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_262bdd33","line":75,"updated":"2019-03-05 15:03:30.000000000","message":"We can\u0027t do this. This will potentially create an empty file in the image cache. The image cache doesn\u0027t verify the contents of files, only that they exist, so this would potentially lead to a situation where the only way to recover is for the operator to manually delete things from the image cache.\n\nThis is most likely only required due to some behaviour that nfs lacks by default. Perhaps something relating to flushing? We need to find and address the root cause rather than adding a workaround which is likely to bite us at some point.","commit_id":"cd7df1864634f09a728bfa9adcb78c431e05ae51"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"6def3a96b735b3c1e9455def35ac7ef80fd6b234","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    # context wrapper ensures the file exists before trying to modify time"},{"line_number":74,"context_line":"    # which fixes a race condition with NFS image caching (see LP#1809123)"},{"line_number":75,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":76,"context_line":"        os.utime(path, None)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_d068c2bb","line":75,"in_reply_to":"5fc1f717_702c6e99","updated":"2019-03-05 17:57:30.000000000","message":"...or possibly not for reasons we don\u0027t understand yet. However I would very much err on the side of caution here and ignore the failing utime, in context, at the point of use, rather than potentially leaving a corrupt file in the image cache.","commit_id":"cd7df1864634f09a728bfa9adcb78c431e05ae51"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"5e3fa1fffd5d885ac5b87483f2421350748295ae","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    # context wrapper ensures the file exists before trying to modify time"},{"line_number":74,"context_line":"    # which fixes a race condition with NFS image caching (see LP#1809123)"},{"line_number":75,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":76,"context_line":"        os.utime(path, None)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_24a2169c","line":75,"in_reply_to":"5fc1f717_a44cc676","updated":"2019-03-06 10:47:02.000000000","message":"Neil, another reason that I prefer your patch to this one is that it inserts the workaround at exactly the point where we have this context. You can\u0027t read the surrounding code here, because it\u0027s not surrounding. You also have to consider that there are 2 callers of this method with different contexts, and 2 other callers of os.utime() which future maintainer might be tempted to replace with a call this this method without considering the difference in behaviour.\n\nThe workaround is much safer when it\u0027s confined in scope.","commit_id":"cd7df1864634f09a728bfa9adcb78c431e05ae51"},{"author":{"_account_id":13734,"name":"Nell Jerram","email":"nell@tigera.io","username":"neiljerram"},"change_message_id":"db2856fa9521afd431e8da4cb5b636dd30088288","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    # context wrapper ensures the file exists before trying to modify time"},{"line_number":74,"context_line":"    # which fixes a race condition with NFS image caching (see LP#1809123)"},{"line_number":75,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":76,"context_line":"        os.utime(path, None)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_e4c88e75","line":75,"in_reply_to":"5fc1f717_a44cc676","updated":"2019-03-06 10:41:17.000000000","message":"Re testing and NFS versions, here\u0027s what I believe I have tried.  But please always bear in mind that I\u0027m not an NFS expert and might have made mistakes.  Also note that I\u0027m talking here about the things I tried _before_ my or Tim\u0027s Gerrit fixes.\n\n- Basic test platform throughout has been Ubuntu 18.04 (Bionic), with OpenStack Queens packages.\n\n- NFS server is nfs-kernel-server, configured with\n\n    echo \"$NFS_SERVER:/ /var/lib/nova/instances nfs4 defaults 0 0\" \u003e\u003e /etc/fstab\n\nSo I\u0027m pretty sure that\u0027s NFSv4.\n\n- I tried changing that \"nfs4\" to \"nfs\", but it made no difference.\n\n- I tried disabling the AppArmor system in Ubuntu, in case it was relevant.  No difference.  (Or else I got the disabling wrong.)\n\n- Our test rig has 3 compute nodes, and makes compute node 1 the NFS server.  I wondered if that was a problem, so tried setting up a separate node as the NFS server.  No difference.\n\n- I then tried making the separate NFS server node run on CentOS 7 instead of Ubuntu 18.04, in case it was a problem with the Ubuntu NFS server code.  No difference.\n\nDoes that help at all?","commit_id":"cd7df1864634f09a728bfa9adcb78c431e05ae51"},{"author":{"_account_id":13734,"name":"Nell Jerram","email":"nell@tigera.io","username":"neiljerram"},"change_message_id":"2eb37920db22fa6b1dbc17fb05227238babab7f5","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    # context wrapper ensures the file exists before trying to modify time"},{"line_number":74,"context_line":"    # which fixes a race condition with NFS image caching (see LP#1809123)"},{"line_number":75,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":76,"context_line":"        os.utime(path, None)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_a44cc676","line":75,"in_reply_to":"5fc1f717_b3d660f1","updated":"2019-03-06 10:22:51.000000000","message":"In all the cases that I\u0027ve seen / tested / debugged, the base image file *does* already exist, with correct content, and the os.utime call *should* work to update the timestamp on that file.  It appears there is some weird NFS bug that causes that to fail sometimes, and the open(...) call - i.e., I think, just acquiring another file descriptor for the existing file - somehow works around that NFS bug or causes it not to fire.\n\nSo - according to my understanding - the comment here is unnecessarily scary where it says \"ensures the file exists\".  It makes it sounds like there are cases where the file doesn\u0027t yet exist, and this call creates it; and in practice I suspect there aren\u0027t.\n\n(It might be possible from reading the surrounding code to prove that the base image file always exists here, but I haven\u0027t done that.)","commit_id":"cd7df1864634f09a728bfa9adcb78c431e05ae51"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"c00423d58c8770f6dc25b150bffe793d03657015","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    # context wrapper ensures the file exists before trying to modify time"},{"line_number":74,"context_line":"    # which fixes a race condition with NFS image caching (see LP#1809123)"},{"line_number":75,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":76,"context_line":"        os.utime(path, None)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_e14e40c0","line":75,"in_reply_to":"5fc1f717_b3d660f1","updated":"2019-03-06 10:13:11.000000000","message":"The issue isn\u0027t that there is definitely a problem with this approach, although it certainly looks incorrect at first glance. The issue is that we don\u0027t understand what the problem is, and we\u0027re patching it in a manner which both looks wrong and leaves us open to an inconsistent image cache. The other proposed patch is much safer, because it ignores a specific failure in context. That\u0027s more \u0027fail safe\u0027, because the worst that will happen in that case is that an instance will fail to spawn, which we can retry.\n\nIncidentally, I\u0027d be curious to know what NFS version was in use in this deployment. Specifically, NFSv3 is known to be problematic for concurrent access in various cases, and we explicitly don\u0027t support it in OSP. We require NFSv4, and recommend NFSv4.2, as it also efficiently handles sparse files.\n\nI can only apologise for not having seen this earlier, btw. I know your pain.","commit_id":"cd7df1864634f09a728bfa9adcb78c431e05ae51"},{"author":{"_account_id":17280,"name":"Tim Rozet","email":"trozet@redhat.com","username":"trozet"},"change_message_id":"f7d50cccd1d9a5d41fbb9aa832803286f4938e7e","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    # context wrapper ensures the file exists before trying to modify time"},{"line_number":74,"context_line":"    # which fixes a race condition with NFS image caching (see LP#1809123)"},{"line_number":75,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":76,"context_line":"        os.utime(path, None)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_b3d660f1","line":75,"in_reply_to":"5fc1f717_d068c2bb","updated":"2019-03-05 18:59:41.000000000","message":"I\u0027m just not sure I buy that there is any leftover file or corruption that prevents subsequent instances from starting. In this case multiple instances are launched at the same time (the original trigger of this bug) so if this did somehow corrupt a base image, then I would presume we would see some type of failure launching instances which neither I nor Neil saw in our testing. If you can produce either of these problems then I will agree we should revert.\n\nHowever, I left this patch open on master for a while (3 months) to get this kind of feedback, which I could have tested for then to see if there was any leftover files that were not cleaned up. I\u0027ve moved on to working on other stuff now, and I don\u0027t have the time to go back and work on this again.","commit_id":"cd7df1864634f09a728bfa9adcb78c431e05ae51"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"5e3fa1fffd5d885ac5b87483f2421350748295ae","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    # context wrapper ensures the file exists before trying to modify time"},{"line_number":74,"context_line":"    # which fixes a race condition with NFS image caching (see LP#1809123)"},{"line_number":75,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":76,"context_line":"        os.utime(path, None)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_84b66ad9","line":75,"in_reply_to":"5fc1f717_e4c88e75","updated":"2019-03-06 10:47:02.000000000","message":"Yep, that\u0027s nfs4. That rules out known nfsv3 issues.","commit_id":"cd7df1864634f09a728bfa9adcb78c431e05ae51"},{"author":{"_account_id":17280,"name":"Tim Rozet","email":"trozet@redhat.com","username":"trozet"},"change_message_id":"07ee1bec0b0647340ed100673c7496316ef3d4a5","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    # context wrapper ensures the file exists before trying to modify time"},{"line_number":74,"context_line":"    # which fixes a race condition with NFS image caching (see LP#1809123)"},{"line_number":75,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":76,"context_line":"        os.utime(path, None)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_dc3dfe8e","line":75,"in_reply_to":"9fdfeff1_262bdd33","updated":"2019-03-05 16:08:44.000000000","message":"Won\u0027t the image cache cleanup eventually delete the file?","commit_id":"cd7df1864634f09a728bfa9adcb78c431e05ae51"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e0ca724ff54f303ce8eb28b27d82c30ad5fef78b","unresolved":false,"context_lines":[{"line_number":72,"context_line":"        raise exception.FileNotFound(file_path\u003dpath)"},{"line_number":73,"context_line":"    # context wrapper ensures the file exists before trying to modify time"},{"line_number":74,"context_line":"    # which fixes a race condition with NFS image caching (see LP#1809123)"},{"line_number":75,"context_line":"    with open(path, \u0027a\u0027):"},{"line_number":76,"context_line":"        os.utime(path, None)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5fc1f717_702c6e99","line":75,"in_reply_to":"9fdfeff1_dc3dfe8e","updated":"2019-03-05 17:54:22.000000000","message":"Yes, but in the intervening period it will assume that because the file exists it is valid. IOW, if you boot anything else using the same image before it is reaped it will be initialized with an empty image.","commit_id":"cd7df1864634f09a728bfa9adcb78c431e05ae51"}]}