)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"8d6410dec125defc9ed775ada4bacedb818250b7","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This came from an empty build node, for reference it was"},{"line_number":10,"context_line":""},{"line_number":11,"context_line":" /nodepool/images/fedora-32/builds/0000057968"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This had an empty node"},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"f96fed1b_ba3f9597","line":11,"updated":"2021-04-23 01:17:18.000000000","message":"I believe you noted that node didn\u0027t have any data as well.  That\u0027s an important clue.\n\nThat suggests that deleteBuild may be implicated too.  It behaves the same way as deleteUpload.","commit_id":"8bb18f38d257b57fa78ddfd8a5e0a3c949a8151b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"8d6410dec125defc9ed775ada4bacedb818250b7","unresolved":false,"context_lines":[{"line_number":36,"context_line":"                      upload)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"We did not see an exception, so both builders must have thought they go"},{"line_number":39,"context_line":"imageUploadNumberLock()"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"I think the problem here is the way that the lock node is placed under"},{"line_number":42,"context_line":"the image node and the subsequent recursive delete."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"73c1e286_9ab1d582","line":39,"updated":"2021-04-23 01:17:18.000000000","message":"Just to be clear -- they did both get the lock; I don\u0027t think we\u0027re suggesting that one acted without the lock.  I think that\u0027s what you meant.","commit_id":"8bb18f38d257b57fa78ddfd8a5e0a3c949a8151b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"8d6410dec125defc9ed775ada4bacedb818250b7","unresolved":false,"context_lines":[{"line_number":44,"context_line":"When we go to delete \"images\" with deleteUpload(recursive\u003dTrue) we"},{"line_number":45,"context_line":"first delete \"lock\", which then indicates another process can come in"},{"line_number":46,"context_line":"and recreate the lock, try and delete as well and we end up with nodes"},{"line_number":47,"context_line":"left behind (I admit this last bit is a bit hand-wavy...)"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"The solution proposed here is to put the upload lock as a *sibling* of"},{"line_number":50,"context_line":"the upload id.  So we have"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"4e0abe59_eee739b5","line":47,"updated":"2021-04-23 01:17:18.000000000","message":"I think the mechanism is this:\n\nThe both started cleaning the same image build, and then also the same image upload.  nb02 deleted the image from the provider.  Then nb01 deleted the same image from the provider (this is safe; it\u0027s idempotent) but did so more slowly.  While nb01 was busy doing that, nb02 locked the upload, deleted it, releasing the lock; then it proceded to delete the build from the local disk, then finally deleted the build record from ZK.  At this moment, for almost an entire second, there was no ZK node at /nodepool/images/fedora-32/builds/0000057968 or below.  But then nb01 started moving again and locked the upload.  The lock recipe will happily recreate the entire path, so all of 0000057968/providers/ovh-bhs1/images/lock/... was recreated.  This time the data znodes are empty, which explains why you saw not only the upload node empty, but also the build.","commit_id":"8bb18f38d257b57fa78ddfd8a5e0a3c949a8151b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"8d6410dec125defc9ed775ada4bacedb818250b7","unresolved":false,"context_lines":[{"line_number":57,"context_line":"the existing Kazoo Lock object with a function that deletes its path."},{"line_number":58,"context_line":"So in the context manager, when we drop the lock, we also remove the"},{"line_number":59,"context_line":"lock node.  By this time, the image-id is gone, so no other builder"},{"line_number":60,"context_line":"will be trying to remove it."},{"line_number":61,"context_line":""},{"line_number":62,"context_line":"Thus we do not need a recursive delete any more, this is removed."},{"line_number":63,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"ded2728a_636b0072","line":60,"updated":"2021-04-23 01:17:18.000000000","message":"This specific error likely happened because we locked something after it was deleted, and so we recreated the thing that was deleted.  If we only look at image uploads, this sibling approach should solve the problem, except that uploads are children of builds, and the build can also be deleted (as in this case).  So not only can this happen to uploads when uploads are deleted, it can also happen to builds when builds are deleted, and the most likely case (due to timing) and what happened in this instance: to uploads when builds are deleted.\n\nSo this patch would not have avoided this issue (the json error would have been on the build data being null instead of the upload data).  But in general, putting locks outside of things that are deleted can be a solution to this problem.  If we want to go that way, we would need to bput both the builds and the image locks outside of the build hierarchy.  We do something similar with node requests right now.  The cleanup for this is indeed a little complicated, though it\u0027s done by a separate thread since we can\u0027t actually rely on the lock owner cleaning itself up in the case of a crash.  See _cleanupNodeRequestLocks().\n\nI can think of two other alternatives:\n\n#1: We could use shared locks to mediate deletes.  Essentially we could acquire a read lock on a build before we do anything to it or any of its images.  Before deleting the build, we acquire a write lock on the build.  You can\u0027t get a write lock if someone has a read lock, and you can\u0027t get a read lock if someone has a write lock.  So we can make sure we\u0027re not deleting a build unless we are certain that no one is in an upload delete loop.  That has some nice characteristics in that it helps avoid duplicate work and collisions, but it doesn\u0027t completely solve the problem since we could have the same issue we\u0027re talking about at the build level (someone grabs a build lock immediately after someone else deletes the entire build).  So we\u0027d still need to either make build locks siblings or do the next idea:\n\n#2: It seems like the especially dangerous thing here is the kazoo.recipe.Lock\u0027s willingness to create entire paths in order to get a lock.  If we make a Lock recipe that omits the _ensure_path method[1], we can know that we will never have a lock on a deleted entry.  We would explicity create the \"lock\" path when we first create our upload number nodes or build nodes, etc.  Thereafter, if anyone tried to obtain a lock and \".../lock\" didn\u0027t exist, it would be an error.\n\n#2 seems like a fairly simple and straightforward method which avoids the complexity of dealing with sibling lock nodes.\n\n[1] https://kazoo.readthedocs.io/en/latest/_modules/kazoo/recipe/lock.html","commit_id":"8bb18f38d257b57fa78ddfd8a5e0a3c949a8151b"}],"nodepool/zk.py":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"310d1f4a331b99fbae58ba23ab15cff667209039","unresolved":true,"context_lines":[{"line_number":1649,"context_line":"        path \u003d path + \"/%s\" % upload_number"},{"line_number":1650,"context_line":"        try:"},{"line_number":1651,"context_line":"            # NOTE: Need to do recursively to remove lock znodes"},{"line_number":1652,"context_line":"            self.client.delete(path, recursive\u003dTrue)"},{"line_number":1653,"context_line":"        except kze.NoNodeError:"},{"line_number":1654,"context_line":"            pass"},{"line_number":1655,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3dafb709_0c731064","side":"PARENT","line":1652,"updated":"2021-04-22 22:50:09.000000000","message":"Do we need to keep performing recursive deletes for some time in order to migrate from the old structure to the new structure?\n\nRelated: do we need to check both lock paths for a time? Without that you\u0027ll need to do a full restart I think?","commit_id":"252d10510a44542e9c2660887651c0f06712df96"}]}