)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"b0edd4dbe8b6957e9080b3d964690773605d2670","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"ae62125f_7f0ef662","updated":"2022-09-02 06:38:39.000000000","message":"recheck","commit_id":"4342f80722d21fc3695ad917419bc5edcc78ec05"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"169ba39127dada7d704eabcf5b710cfeeffbe295","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"e02001cf_5b5b6459","updated":"2022-09-13 17:05:08.000000000","message":"recheck","commit_id":"4951790eb6035aa72a70f3bc1f26cc530cf2920f"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"354aab5d05f2ab02c739cabd2ac2905a203fb0de","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"02cca7da_6b87c288","updated":"2022-10-11 17:58:52.000000000","message":"PS15 is the same as PS13, skipping all attempted modifications from PS14","commit_id":"95e9b407a187e23cfcd3525dd83f70649e23cb7d"}],"nodepool/cmd/nodepoolcmd.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"c3829c8be41ef2194d144988658da1ef89815c8d","unresolved":false,"context_lines":[{"line_number":356,"context_line":""},{"line_number":357,"context_line":"    def disable(self):"},{"line_number":358,"context_line":"        timeout \u003d self.args.timeout"},{"line_number":359,"context_line":"        self._change_node_state(zk.HOLD, timeout\u003dtimeout)"},{"line_number":360,"context_line":""},{"line_number":361,"context_line":"    def enable(self):"},{"line_number":362,"context_line":"        timeout \u003d self.args.timeout"}],"source_content_type":"text/x-python","patch_set":4,"id":"4fce2e07_feb0c51d","line":359,"updated":"2022-08-23 21:30:21.000000000","message":"Instead of calling the command \"disable\" why not call it \"hold\"?","commit_id":"fed7d59c07c603bbf7c0aa0861a92b6f0cfc7e9a"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"9f656d511acdd1bc682d972b6a69d5ae2367f4ff","unresolved":true,"context_lines":[{"line_number":360,"context_line":""},{"line_number":361,"context_line":"    def enable(self):"},{"line_number":362,"context_line":"        timeout \u003d self.args.timeout"},{"line_number":363,"context_line":"        self._change_node_state(zk.READY, timeout\u003dtimeout)"},{"line_number":364,"context_line":""},{"line_number":365,"context_line":"    def dib_image_delete(self):"},{"line_number":366,"context_line":"        (image, build_num) \u003d self.args.id.rsplit(\u0027-\u0027, 1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"7c6f6671_a89a2a74","line":363,"updated":"2022-08-23 21:21:09.000000000","message":"It might be a good idea to only allow a transition from the HOLD state to the READY state to avoid accidentally allowing a node to be reused while another job is still using the node.\n\nThis may not be sufficient though as we can disable a node and set it to HOLD before a job compeletes as well. It is more difficult to enforce the state change to HOLD though as you would race the next job starting if you want to only allow READY -\u003e HOLD.\n\nIt might also be a good idea to to restrict this to static nodes or require a --force type flag to apply these state changes to ephemeral nodes. This way it is more difficult to accidentally perform a state change on the wrong sort of node.","commit_id":"fed7d59c07c603bbf7c0aa0861a92b6f0cfc7e9a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"c3829c8be41ef2194d144988658da1ef89815c8d","unresolved":false,"context_lines":[{"line_number":360,"context_line":""},{"line_number":361,"context_line":"    def enable(self):"},{"line_number":362,"context_line":"        timeout \u003d self.args.timeout"},{"line_number":363,"context_line":"        self._change_node_state(zk.READY, timeout\u003dtimeout)"},{"line_number":364,"context_line":""},{"line_number":365,"context_line":"    def dib_image_delete(self):"},{"line_number":366,"context_line":"        (image, build_num) \u003d self.args.id.rsplit(\u0027-\u0027, 1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"ddb266c3_cdbf2bc2","line":363,"updated":"2022-08-23 21:30:21.000000000","message":"We should actually just delete the node and let the driver handle processing it.  In the case of the static driver, the node will be re-registered.\n\nThis means you can drop the \"enable\" command entirely and use \"delete\" in your workflow.","commit_id":"fed7d59c07c603bbf7c0aa0861a92b6f0cfc7e9a"},{"author":{"_account_id":13252,"name":"Dr. Jens Harbott","display_name":"Jens Harbott (frickler)","email":"frickler@offenerstapel.de","username":"jrosenboom"},"change_message_id":"ecb3298e243601b83e5473062952ad27b169cb2d","unresolved":true,"context_lines":[{"line_number":360,"context_line":""},{"line_number":361,"context_line":"    def enable(self):"},{"line_number":362,"context_line":"        timeout \u003d self.args.timeout"},{"line_number":363,"context_line":"        self._change_node_state(zk.READY, timeout\u003dtimeout)"},{"line_number":364,"context_line":""},{"line_number":365,"context_line":"    def dib_image_delete(self):"},{"line_number":366,"context_line":"        (image, build_num) \u003d self.args.id.rsplit(\u0027-\u0027, 1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"06e4f0c6_ac94fb49","line":363,"in_reply_to":"2ab0ab6b_471ada5b","updated":"2022-10-11 11:53:57.000000000","message":"Having to delete a node in order to move from \"hold\" to \"ready\" sounds very counter-intuitive. At the very least that would need to be very explicitly documented as the expected workflow. For user-friendlyness I would like to see a \"unhold\" command matching the \"image-pause/-unpause\" pair, even if it is only an alias for \"delete\".","commit_id":"fed7d59c07c603bbf7c0aa0861a92b6f0cfc7e9a"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"c7418e199045d678318b3c8681223d8264f23836","unresolved":true,"context_lines":[{"line_number":360,"context_line":""},{"line_number":361,"context_line":"    def enable(self):"},{"line_number":362,"context_line":"        timeout \u003d self.args.timeout"},{"line_number":363,"context_line":"        self._change_node_state(zk.READY, timeout\u003dtimeout)"},{"line_number":364,"context_line":""},{"line_number":365,"context_line":"    def dib_image_delete(self):"},{"line_number":366,"context_line":"        (image, build_num) \u003d self.args.id.rsplit(\u0027-\u0027, 1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"2ab0ab6b_471ada5b","line":363,"in_reply_to":"60c4bb33_6b4a9c11","updated":"2022-08-25 19:19:25.000000000","message":"That sounds good to me; at least in the case of static nodes that is in line with what I had in mind for this feature.\nBut I\u0027ll let James have the final word here of course","commit_id":"fed7d59c07c603bbf7c0aa0861a92b6f0cfc7e9a"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"aac3a360391a85f174f5a4db395d3bc59ee94ef6","unresolved":true,"context_lines":[{"line_number":360,"context_line":""},{"line_number":361,"context_line":"    def enable(self):"},{"line_number":362,"context_line":"        timeout \u003d self.args.timeout"},{"line_number":363,"context_line":"        self._change_node_state(zk.READY, timeout\u003dtimeout)"},{"line_number":364,"context_line":""},{"line_number":365,"context_line":"    def dib_image_delete(self):"},{"line_number":366,"context_line":"        (image, build_num) \u003d self.args.id.rsplit(\u0027-\u0027, 1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"1e606d8b_2e089d7d","line":363,"in_reply_to":"7c6f6671_a89a2a74","updated":"2022-08-24 08:45:16.000000000","message":"\u003e This may not be sufficient though as we can disable a node and set it to HOLD before a job completes as well\n\nWhen I tested it, this was not possible. I don\u0027t know the exact mechanism behind it in nodepool but it looked like a node is locked during a job execution and hence can\u0027t be disabled\n\n\n\u003e It might be a good idea to only allow a transition from the HOLD state to the READY state to avoid accidentally allowing a node to be reused while another job is still using the node.\n\nI agree that could be a good safeguard but when testing it, it was not possible to interrupt an ongoing build. I could only test it with static nodes.\n\nLooking at James\u0027 comments, I think it makes sense to drop the \u0027enable\u0027 command altogether\n\n\n\u003e It might also be a good idea to to restrict this to static nodes \n\nI like this idea but how do I get information about the node type or the corresponding driver?","commit_id":"fed7d59c07c603bbf7c0aa0861a92b6f0cfc7e9a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"dc33fc9e6494fc2ff3fe391b94dbf77107f8a351","unresolved":true,"context_lines":[{"line_number":360,"context_line":""},{"line_number":361,"context_line":"    def enable(self):"},{"line_number":362,"context_line":"        timeout \u003d self.args.timeout"},{"line_number":363,"context_line":"        self._change_node_state(zk.READY, timeout\u003dtimeout)"},{"line_number":364,"context_line":""},{"line_number":365,"context_line":"    def dib_image_delete(self):"},{"line_number":366,"context_line":"        (image, build_num) \u003d self.args.id.rsplit(\u0027-\u0027, 1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff1e0f0b_1e07882d","line":363,"in_reply_to":"7c6f6671_a89a2a74","updated":"2022-08-25 17:00:23.000000000","message":"I think it\u0027s okay to allow the \u0027hold\u0027 command for any node type.  It would let someone grab a min-ready node and use it for something.  Then they can delete it.\n\nIt\u0027s true that you can\u0027t go from in-use to hold because the client is expected to hold the lock for the entire time the node is in-use.","commit_id":"fed7d59c07c603bbf7c0aa0861a92b6f0cfc7e9a"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"aac3a360391a85f174f5a4db395d3bc59ee94ef6","unresolved":false,"context_lines":[{"line_number":360,"context_line":""},{"line_number":361,"context_line":"    def enable(self):"},{"line_number":362,"context_line":"        timeout \u003d self.args.timeout"},{"line_number":363,"context_line":"        self._change_node_state(zk.READY, timeout\u003dtimeout)"},{"line_number":364,"context_line":""},{"line_number":365,"context_line":"    def dib_image_delete(self):"},{"line_number":366,"context_line":"        (image, build_num) \u003d self.args.id.rsplit(\u0027-\u0027, 1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"96ad37a3_da92ff47","line":363,"in_reply_to":"ddb266c3_cdbf2bc2","updated":"2022-08-24 08:45:16.000000000","message":"Fair point. That should be enough for our use-case still","commit_id":"fed7d59c07c603bbf7c0aa0861a92b6f0cfc7e9a"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"6f189fea85a5df1aed45e08416cb8c3514383cd0","unresolved":true,"context_lines":[{"line_number":360,"context_line":""},{"line_number":361,"context_line":"    def enable(self):"},{"line_number":362,"context_line":"        timeout \u003d self.args.timeout"},{"line_number":363,"context_line":"        self._change_node_state(zk.READY, timeout\u003dtimeout)"},{"line_number":364,"context_line":""},{"line_number":365,"context_line":"    def dib_image_delete(self):"},{"line_number":366,"context_line":"        (image, build_num) \u003d self.args.id.rsplit(\u0027-\u0027, 1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"60c4bb33_6b4a9c11","line":363,"in_reply_to":"ff1e0f0b_1e07882d","updated":"2022-08-25 18:59:55.000000000","message":"Making sure I understand the proposed workflow here. Users would hold a node, then when any required work is done delete the node. In the case of ephemeral cloud nodes that frees the resources to be used by a new node. In the case of a static node the static driver will recreate the static node as ready?\n\nIf I\u0027ve understood correctly then I think this will work well.","commit_id":"fed7d59c07c603bbf7c0aa0861a92b6f0cfc7e9a"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"e4cb396f4347c3bee8b8ba6c186cfcab9c23e029","unresolved":true,"context_lines":[{"line_number":283,"context_line":"            print(\"Node id %s not found\" % self.args.id)"},{"line_number":284,"context_line":"            return"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"        self.zk.lockNode(node, blocking\u003dTrue, timeout\u003d5)"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"        if self.args.now:"},{"line_number":289,"context_line":"            if node.provider not in self.pool.config.providers:"}],"source_content_type":"text/x-python","patch_set":12,"id":"2853224d_3716448b","side":"PARENT","line":286,"range":{"start_line":286,"start_character":31,"end_line":286,"end_character":44},"updated":"2022-09-13 09:48:35.000000000","message":"Is there a reason this needs to be blocking for the delete command?\nIs it okay to not have it blocking for the hold command?","commit_id":"95b3d4c30270c50a6974be7145a5c0c00b7f3503"},{"author":{"_account_id":13252,"name":"Dr. Jens Harbott","display_name":"Jens Harbott (frickler)","email":"frickler@offenerstapel.de","username":"jrosenboom"},"change_message_id":"ecb3298e243601b83e5473062952ad27b169cb2d","unresolved":true,"context_lines":[{"line_number":283,"context_line":"            print(\"Node id %s not found\" % self.args.id)"},{"line_number":284,"context_line":"            return"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"        self.zk.lockNode(node, blocking\u003dTrue, timeout\u003d5)"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"        if self.args.now:"},{"line_number":289,"context_line":"            if node.provider not in self.pool.config.providers:"}],"source_content_type":"text/x-python","patch_set":12,"id":"f3bdce09_2905d792","side":"PARENT","line":286,"range":{"start_line":286,"start_character":31,"end_line":286,"end_character":44},"in_reply_to":"2853224d_3716448b","updated":"2022-10-11 11:53:57.000000000","message":"Overwriting the state from \"in-use\" to \"hold\" doesn\u0027t seem like a good idea. On the other hand, as an operator of a busy setup I would really like to be able to issue the hold command while the node is in use and then have nodepool wait until the node is unlocked in the background and apply the hold after that. No idea how to actually implement that, but that\u0027s the way I\u0027d like to see this work.","commit_id":"95b3d4c30270c50a6974be7145a5c0c00b7f3503"},{"author":{"_account_id":13252,"name":"Dr. Jens Harbott","display_name":"Jens Harbott (frickler)","email":"frickler@offenerstapel.de","username":"jrosenboom"},"change_message_id":"6f140a6cf25f27a428828fef42add57f505ae85a","unresolved":true,"context_lines":[{"line_number":283,"context_line":"            print(\"Node id %s not found\" % self.args.id)"},{"line_number":284,"context_line":"            return"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"        self.zk.lockNode(node, blocking\u003dTrue, timeout\u003d5)"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"        if self.args.now:"},{"line_number":289,"context_line":"            if node.provider not in self.pool.config.providers:"}],"source_content_type":"text/x-python","patch_set":12,"id":"caac28fd_846f218f","side":"PARENT","line":286,"range":{"start_line":286,"start_character":31,"end_line":286,"end_character":44},"in_reply_to":"2eedf460_60d32e48","updated":"2022-10-11 12:45:34.000000000","message":"I would very much prefer not having to modify the config in order to perform maintenance. Also, would removing an in-use node from the config actually be safer?","commit_id":"95b3d4c30270c50a6974be7145a5c0c00b7f3503"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"e9c6bcab71a5f5c3b79d2b779286c0af22f7e334","unresolved":true,"context_lines":[{"line_number":283,"context_line":"            print(\"Node id %s not found\" % self.args.id)"},{"line_number":284,"context_line":"            return"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"        self.zk.lockNode(node, blocking\u003dTrue, timeout\u003d5)"},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"        if self.args.now:"},{"line_number":289,"context_line":"            if node.provider not in self.pool.config.providers:"}],"source_content_type":"text/x-python","patch_set":12,"id":"2eedf460_60d32e48","side":"PARENT","line":286,"range":{"start_line":286,"start_character":31,"end_line":286,"end_character":44},"in_reply_to":"f3bdce09_2905d792","updated":"2022-10-11 12:25:10.000000000","message":"\u003e Overwriting the state from \"in-use\" to \"hold\" doesn\u0027t seem like a good idea. \n\nAs far as I can see, this is not possible as the node will be locked when it\u0027s in use.\n\n\u003e I would really like to be able to issue the hold command while the node is in use and then have nodepool wait until the node is unlocked in the background and apply the hold after that.\n\nThis was part of my first idea/draft as well but it was deemed unsafe as it would possibly introduce a race condition with other node requests.\nQuoting James: \n\u003e i would not recommend that workflow. you will be racing nodepool itself. in that case i would recommend removing the node from the config file.","commit_id":"95b3d4c30270c50a6974be7145a5c0c00b7f3503"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9c07472f5633b019853ee1244f15a2d0d6617eb","unresolved":false,"context_lines":[{"line_number":300,"context_line":"    def delete(self):"},{"line_number":301,"context_line":"        node \u003d self._get_and_lock_node(blocking\u003dTrue)"},{"line_number":302,"context_line":"        if node is None:"},{"line_number":303,"context_line":"            return"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"        if self.args.now:"},{"line_number":306,"context_line":"            if node.provider not in self.pool.config.providers:"}],"source_content_type":"text/x-python","patch_set":14,"id":"06fb2e1d_1a00076d","line":303,"updated":"2022-10-11 15:41:55.000000000","message":"If I\u0027m following correctly, trying to delete a locked node is now non-fatal?  Or am I missing something?  If not, then I don\u0027t think that\u0027s a change we should make.","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9c07472f5633b019853ee1244f15a2d0d6617eb","unresolved":false,"context_lines":[{"line_number":323,"context_line":"    def _change_node_state(self, new_state):"},{"line_number":324,"context_line":"        node \u003d self._get_and_lock_node()"},{"line_number":325,"context_line":"        if node is None:"},{"line_number":326,"context_line":"            return"},{"line_number":327,"context_line":""},{"line_number":328,"context_line":"        node.state \u003d new_state"},{"line_number":329,"context_line":"        self.zk.storeNode(node)"}],"source_content_type":"text/x-python","patch_set":14,"id":"b1457123_7c42ad8e","line":326,"updated":"2022-10-11 15:41:55.000000000","message":"Similarly to deleting, it seems like if we are unable to lock a node, the command should exit non-zero?","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"e72b1b8bc62d90e13fb79a956692574f3792ee96","unresolved":false,"context_lines":[{"line_number":323,"context_line":"    def _change_node_state(self, new_state):"},{"line_number":324,"context_line":"        node \u003d self._get_and_lock_node()"},{"line_number":325,"context_line":"        if node is None:"},{"line_number":326,"context_line":"            return"},{"line_number":327,"context_line":""},{"line_number":328,"context_line":"        node.state \u003d new_state"},{"line_number":329,"context_line":"        self.zk.storeNode(node)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5ab2f8be_856020cd","line":326,"in_reply_to":"0e84dcac_1da57ce9","updated":"2022-10-14 14:20:26.000000000","message":"I think I was confused about that earlier.  Now that I understand that this is a ZK lock timeout and not a test timeout, I think this is fine.","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"e273d98ab2ba528363dd6bb13e7c4304f249d1af","unresolved":false,"context_lines":[{"line_number":323,"context_line":"    def _change_node_state(self, new_state):"},{"line_number":324,"context_line":"        node \u003d self._get_and_lock_node()"},{"line_number":325,"context_line":"        if node is None:"},{"line_number":326,"context_line":"            return"},{"line_number":327,"context_line":""},{"line_number":328,"context_line":"        node.state \u003d new_state"},{"line_number":329,"context_line":"        self.zk.storeNode(node)"}],"source_content_type":"text/x-python","patch_set":14,"id":"fdb75664_41bb4764","line":326,"in_reply_to":"2ba062ca_6a87d189","updated":"2022-10-11 18:32:21.000000000","message":"However, delete() and hold() behave slightly differently when locking a node: delete() will be blocking, leading to a TimeoutException while hold() is non-blocking and will cause a ZKLockException. Not sure what is the best behavior here but maybe it is okay like this?","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"ff8746d98798eddf0b3ce815e8fe2cb1bd685206","unresolved":false,"context_lines":[{"line_number":323,"context_line":"    def _change_node_state(self, new_state):"},{"line_number":324,"context_line":"        node \u003d self._get_and_lock_node()"},{"line_number":325,"context_line":"        if node is None:"},{"line_number":326,"context_line":"            return"},{"line_number":327,"context_line":""},{"line_number":328,"context_line":"        node.state \u003d new_state"},{"line_number":329,"context_line":"        self.zk.storeNode(node)"}],"source_content_type":"text/x-python","patch_set":14,"id":"efd7aafd_e1d98d21","line":326,"in_reply_to":"2e4edfa1_f6e9a642","updated":"2022-10-11 18:18:31.000000000","message":"My understanding (and I haven\u0027t run this locally to double check, so correct me if I\u0027m wrong) is that the current code will raise an exception if we run \"nodepool delete\" on a locked node.  Raising the exception will cause the CLI to exit rc\u003d1.  And some versions of this patch do the same for the \"hold\" command, which I think is the best behavior, but my concern with this version is that it would only print a statement that the node was locked and then exit rc\u003d0 because the exception was suppressed (and the same applied to delete).\n\nAnyway, to be clear, I think the best behavior for both delete and hold is to try to lock the node and exit with an error code if that fails.","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"2c8ba08259688ca25349d8c765f528169848d2dd","unresolved":false,"context_lines":[{"line_number":323,"context_line":"    def _change_node_state(self, new_state):"},{"line_number":324,"context_line":"        node \u003d self._get_and_lock_node()"},{"line_number":325,"context_line":"        if node is None:"},{"line_number":326,"context_line":"            return"},{"line_number":327,"context_line":""},{"line_number":328,"context_line":"        node.state \u003d new_state"},{"line_number":329,"context_line":"        self.zk.storeNode(node)"}],"source_content_type":"text/x-python","patch_set":14,"id":"2e4edfa1_f6e9a642","line":326,"in_reply_to":"b1457123_7c42ad8e","updated":"2022-10-11 17:28:29.000000000","message":"Not sure if I understand. I don\u0027t see the delete function or any other command return non-zero anywhere.","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"18dc4349e1ff91a6b7bf23bc2dd45f88e0a6afa2","unresolved":false,"context_lines":[{"line_number":323,"context_line":"    def _change_node_state(self, new_state):"},{"line_number":324,"context_line":"        node \u003d self._get_and_lock_node()"},{"line_number":325,"context_line":"        if node is None:"},{"line_number":326,"context_line":"            return"},{"line_number":327,"context_line":""},{"line_number":328,"context_line":"        node.state \u003d new_state"},{"line_number":329,"context_line":"        self.zk.storeNode(node)"}],"source_content_type":"text/x-python","patch_set":14,"id":"2ba062ca_6a87d189","line":326,"in_reply_to":"efd7aafd_e1d98d21","updated":"2022-10-11 18:30:13.000000000","message":"\u003e the current code will raise an exception if we run \"nodepool delete\" on a locked node.  Raising the exception will cause the CLI to exit rc\u003d1\n\nI see now, I thought you were referring to some explicit return 1 or something like that in the code.\nPS15 should behave as you describe now, exiting with an error if locking a node fails","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"f0a6a5f0be5a859216e46108c73a633178f48e54","unresolved":true,"context_lines":[{"line_number":323,"context_line":"    def _change_node_state(self, new_state):"},{"line_number":324,"context_line":"        node \u003d self._get_and_lock_node()"},{"line_number":325,"context_line":"        if node is None:"},{"line_number":326,"context_line":"            return"},{"line_number":327,"context_line":""},{"line_number":328,"context_line":"        node.state \u003d new_state"},{"line_number":329,"context_line":"        self.zk.storeNode(node)"}],"source_content_type":"text/x-python","patch_set":14,"id":"0e84dcac_1da57ce9","line":326,"in_reply_to":"f4c61753_571ce5ce","updated":"2022-10-13 10:47:55.000000000","message":"Got it, I changed it now. But with blocking\u003dTrue, we are back to a TimeoutException in one of the test cases which you commented on earlier. So I don\u0027t really know how to improve this if you\u0027re not happy with the test","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"c862f0424b822545f65748ea24d0a95e5322cfe8","unresolved":true,"context_lines":[{"line_number":323,"context_line":"    def _change_node_state(self, new_state):"},{"line_number":324,"context_line":"        node \u003d self._get_and_lock_node()"},{"line_number":325,"context_line":"        if node is None:"},{"line_number":326,"context_line":"            return"},{"line_number":327,"context_line":""},{"line_number":328,"context_line":"        node.state \u003d new_state"},{"line_number":329,"context_line":"        self.zk.storeNode(node)"}],"source_content_type":"text/x-python","patch_set":14,"id":"f4c61753_571ce5ce","line":326,"in_reply_to":"fdb75664_41bb4764","updated":"2022-10-12 22:27:53.000000000","message":"Is there any reason they should be different?  I would think that the current behavior for \"delete\" should be fine for both: blocking for 5 seconds.  That means if we can\u0027t obtain the lock within 5 seconds we give up.  The reason for that is to increase the chance that this succeeds even if one of the system components briefly locks the node for some purpose (they are permitted to do that).\n\nIn short, absent a compelling reason otherwise, I think we should just use the existing delete behavior for both commands.\n\nAlso, this is a bit of a nit, but this method isn\u0027t really necessary and it keeps tripping me up when I look for what else is using it, so if you make a new patchset to resolve the locking situation, maybe you could just move this to the hold() method.","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"8c489de1bc6b87760b4fa0e3dc9f3541b5ea0000","unresolved":false,"context_lines":[{"line_number":310,"context_line":""},{"line_number":311,"context_line":"        self.list(node_id\u003dnode.id)"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"    def _change_node_state(self, new_state):"},{"line_number":314,"context_line":"        node \u003d self.zk.getNode(self.args.id)"},{"line_number":315,"context_line":"        if not node:"},{"line_number":316,"context_line":"            print(\"Node id %s not found\" % self.args.id)"}],"source_content_type":"text/x-python","patch_set":16,"id":"2a884586_ae067b3b","line":313,"updated":"2022-10-14 14:22:22.000000000","message":"Nit: this method is not necessary, and this should just be the \"hold\" method.","commit_id":"1658aa9851ad67a1f966be0a5e52287d4b2a12d6"}],"nodepool/tests/unit/test_commands.py":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"bfebbec5b41fbf1f16a79f8239540024938950a9","unresolved":true,"context_lines":[{"line_number":428,"context_line":"        # Attempt to disable node, this should fail"},{"line_number":429,"context_line":"        # since another process holds the lock"},{"line_number":430,"context_line":"        with testtools.ExpectedException(TimeoutException):"},{"line_number":431,"context_line":"            self.patch_argv(\u0027-c\u0027, configfile, \u0027hold\u0027, nodes[0].id)"},{"line_number":432,"context_line":"            nodepoolcmd.main()"},{"line_number":433,"context_line":""},{"line_number":434,"context_line":"    def test_attempt_request_disabled_static_node(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"92630355_ad5db18e","line":431,"updated":"2022-08-25 20:19:55.000000000","message":"Consider setting the timeout flag to the command here so that we can reduce the length of the timeout. That will make this test run more quickly.","commit_id":"51e57897593a2223bdc9a38353b63a06a24fb6c5"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"8a6cb4a96b6dfaf9ddb031283f10c54ed803d03d","unresolved":false,"context_lines":[{"line_number":428,"context_line":"        # Attempt to disable node, this should fail"},{"line_number":429,"context_line":"        # since another process holds the lock"},{"line_number":430,"context_line":"        with testtools.ExpectedException(TimeoutException):"},{"line_number":431,"context_line":"            self.patch_argv(\u0027-c\u0027, configfile, \u0027hold\u0027, nodes[0].id)"},{"line_number":432,"context_line":"            nodepoolcmd.main()"},{"line_number":433,"context_line":""},{"line_number":434,"context_line":"    def test_attempt_request_disabled_static_node(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"5a5f8906_dc041cde","line":431,"in_reply_to":"92630355_ad5db18e","updated":"2022-08-25 20:55:22.000000000","message":"Done","commit_id":"51e57897593a2223bdc9a38353b63a06a24fb6c5"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"bfebbec5b41fbf1f16a79f8239540024938950a9","unresolved":true,"context_lines":[{"line_number":499,"context_line":"        req \u003d self.waitForNodeRequest(req, (zk.FULFILLED,))"},{"line_number":500,"context_line":""},{"line_number":501,"context_line":"        # NOTE: Is it expected behavior that in a node list of length 1,"},{"line_number":502,"context_line":"        # a new node gets assigned to this request?"},{"line_number":503,"context_line":"        self.assertNotEqual(nodes[0].id, req.nodes[0])"},{"line_number":504,"context_line":""},{"line_number":505,"context_line":"    def test_image_build(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"1335ecaa_61a6ab6a","line":502,"updated":"2022-08-25 20:19:55.000000000","message":"Yes, because the max-servers value set in node.yaml is 96. The nodes list value is initially satisfied via min ready configuration of 1 for fake-label. Then when a new request comes in nodepool cannot fulfill that request using the existing node because its state is HOLD. Nodepool proceeds by provisioning a new node instead since it has max-servers headroom to do so. If it didn\u0027t have max-servers headroom it would pause and wait for space to become available.","commit_id":"51e57897593a2223bdc9a38353b63a06a24fb6c5"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"b52ec93d033a67c46e01d794f7a63d9cfeb48624","unresolved":false,"context_lines":[{"line_number":499,"context_line":"        req \u003d self.waitForNodeRequest(req, (zk.FULFILLED,))"},{"line_number":500,"context_line":""},{"line_number":501,"context_line":"        # NOTE: Is it expected behavior that in a node list of length 1,"},{"line_number":502,"context_line":"        # a new node gets assigned to this request?"},{"line_number":503,"context_line":"        self.assertNotEqual(nodes[0].id, req.nodes[0])"},{"line_number":504,"context_line":""},{"line_number":505,"context_line":"    def test_image_build(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"ebd5f3d3_e55ecd22","line":502,"in_reply_to":"1335ecaa_61a6ab6a","updated":"2022-08-25 20:56:31.000000000","message":"Thanks for clarifying!","commit_id":"51e57897593a2223bdc9a38353b63a06a24fb6c5"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"e3577ffa5906890c525f176376ba8f6c8de7f077","unresolved":false,"context_lines":[{"line_number":385,"context_line":"        self.assert_nodes_listed(configfile, 1, zk.READY)"},{"line_number":386,"context_line":"        self.assertNotEqual(old_node_id, new_nodes[0].id)"},{"line_number":387,"context_line":""},{"line_number":388,"context_line":"        # Set node as IN-USE"},{"line_number":389,"context_line":"        req \u003d zk.NodeRequest()"},{"line_number":390,"context_line":"        req.state \u003d zk.REQUESTED"},{"line_number":391,"context_line":"        req.node_types.append(\u0027fake-label\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"c763ba39_f302c96c","line":388,"updated":"2022-08-31 13:31:15.000000000","message":"This comment isn\u0027t correct?","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"e3577ffa5906890c525f176376ba8f6c8de7f077","unresolved":false,"context_lines":[{"line_number":397,"context_line":"        req \u003d self.waitForNodeRequest(req, (zk.FULFILLED,))"},{"line_number":398,"context_line":"        self.assertEqual(len(req.nodes), 1)"},{"line_number":399,"context_line":""},{"line_number":400,"context_line":"    def test_attempt_disable_busy_node(self):"},{"line_number":401,"context_line":"        configfile \u003d self.setup_config(\u0027node.yaml\u0027)"},{"line_number":402,"context_line":"        pool \u003d self.useNodepool(configfile, watermark_sleep\u003d1)"},{"line_number":403,"context_line":"        self.useBuilder(configfile)"}],"source_content_type":"text/x-python","patch_set":8,"id":"2ea0d83a_8f8dde83","line":400,"updated":"2022-08-31 13:31:15.000000000","message":"I suggest s/disable/hold/ so that it\u0027s more clear what this does.","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"e3577ffa5906890c525f176376ba8f6c8de7f077","unresolved":false,"context_lines":[{"line_number":410,"context_line":"        self.assert_listed(configfile, [\u0027list\u0027], 0, nodes[0].id, 1)"},{"line_number":411,"context_line":"        self.assert_nodes_listed(configfile, 1, zk.READY)"},{"line_number":412,"context_line":""},{"line_number":413,"context_line":"        # Set node as IN-USE"},{"line_number":414,"context_line":"        req1 \u003d zk.NodeRequest()"},{"line_number":415,"context_line":"        req1.state \u003d zk.REQUESTED"},{"line_number":416,"context_line":"        req1.node_types.append(\u0027fake-label\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa2719d6_ac570ba2","line":413,"updated":"2022-08-31 13:31:15.000000000","message":"Ditto?","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"e3577ffa5906890c525f176376ba8f6c8de7f077","unresolved":false,"context_lines":[{"line_number":427,"context_line":""},{"line_number":428,"context_line":"        # Attempt to disable node, this should fail"},{"line_number":429,"context_line":"        # since another process holds the lock"},{"line_number":430,"context_line":"        with testtools.ExpectedException(TimeoutException):"},{"line_number":431,"context_line":"            self.patch_argv(\u0027-c\u0027, configfile, \u0027hold\u0027, nodes[0].id, \u0027-t\u0027, \u00271\u0027)"},{"line_number":432,"context_line":"            nodepoolcmd.main()"},{"line_number":433,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"81cb7f64_f7957747","line":430,"updated":"2022-08-31 13:31:15.000000000","message":"Is there something we can do for this assertion other than waiting for a test timeout?\n\nMaybe the command should return immediately (exit code 1) if it can\u0027t lock it.  That\u0027s probably better for user experience.","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"e4cb396f4347c3bee8b8ba6c186cfcab9c23e029","unresolved":true,"context_lines":[{"line_number":427,"context_line":""},{"line_number":428,"context_line":"        # Attempt to disable node, this should fail"},{"line_number":429,"context_line":"        # since another process holds the lock"},{"line_number":430,"context_line":"        with testtools.ExpectedException(TimeoutException):"},{"line_number":431,"context_line":"            self.patch_argv(\u0027-c\u0027, configfile, \u0027hold\u0027, nodes[0].id, \u0027-t\u0027, \u00271\u0027)"},{"line_number":432,"context_line":"            nodepoolcmd.main()"},{"line_number":433,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"4b320f5c_5851e259","line":430,"in_reply_to":"045300cf_11d9f7c0","updated":"2022-09-13 09:48:35.000000000","message":"When I change the hold command to not have the lockNode invocation blocking, I think I get the expected exception that you were referring to, namely npe.ZKLockException","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"acf36852dc923766f586912b654af77b08275f12","unresolved":true,"context_lines":[{"line_number":427,"context_line":""},{"line_number":428,"context_line":"        # Attempt to disable node, this should fail"},{"line_number":429,"context_line":"        # since another process holds the lock"},{"line_number":430,"context_line":"        with testtools.ExpectedException(TimeoutException):"},{"line_number":431,"context_line":"            self.patch_argv(\u0027-c\u0027, configfile, \u0027hold\u0027, nodes[0].id, \u0027-t\u0027, \u00271\u0027)"},{"line_number":432,"context_line":"            nodepoolcmd.main()"},{"line_number":433,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"974e53fc_ceaad1d2","line":430,"in_reply_to":"81cb7f64_f7957747","updated":"2022-08-31 17:39:20.000000000","message":"I think it would actually be good if the command waited for a locked node a reasonable amount of time (for example to finish a job) and afterwards set it to hold, hence the addition of the timeout flag to this command.","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"e081a4aefb97e35c4b158b38c355b4563eb16168","unresolved":true,"context_lines":[{"line_number":427,"context_line":""},{"line_number":428,"context_line":"        # Attempt to disable node, this should fail"},{"line_number":429,"context_line":"        # since another process holds the lock"},{"line_number":430,"context_line":"        with testtools.ExpectedException(TimeoutException):"},{"line_number":431,"context_line":"            self.patch_argv(\u0027-c\u0027, configfile, \u0027hold\u0027, nodes[0].id, \u0027-t\u0027, \u00271\u0027)"},{"line_number":432,"context_line":"            nodepoolcmd.main()"},{"line_number":433,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"a63be71f_6d89eacd","line":430,"in_reply_to":"81cb7f64_f7957747","updated":"2022-09-08 17:11:30.000000000","message":"It looks like the command should fail after 5 seconds now.  The actual code lgtm, but I\u0027m not sure why the test is passing with a timeoutexception instead of whatever exception is raised by the command failing.","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"92edd0cacc24ee06c62c69997a4ce7e22bfd2938","unresolved":true,"context_lines":[{"line_number":427,"context_line":""},{"line_number":428,"context_line":"        # Attempt to disable node, this should fail"},{"line_number":429,"context_line":"        # since another process holds the lock"},{"line_number":430,"context_line":"        with testtools.ExpectedException(TimeoutException):"},{"line_number":431,"context_line":"            self.patch_argv(\u0027-c\u0027, configfile, \u0027hold\u0027, nodes[0].id, \u0027-t\u0027, \u00271\u0027)"},{"line_number":432,"context_line":"            nodepoolcmd.main()"},{"line_number":433,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"9c56c8c1_efc60e8e","line":430,"in_reply_to":"974e53fc_ceaad1d2","updated":"2022-09-01 17:05:24.000000000","message":"Maybe it is indeed better to have the command return w/ non-zero instead of misusing any timeout flag.\n\nBut I can\u0027t really think of a better way to test these cases than by expecting a timeout.\nThat begs the question, is it even necessary to test these cases? Are they already covered in other places? I guess the behavior of zk.HOLD and nodes that are busy are well-known","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":35200,"name":"Marvin Becker","email":"marvinbecker@mail.de","username":"mbecker"},"change_message_id":"88dbb5689c368c7e3335b9b4ebd73a353d861d0f","unresolved":true,"context_lines":[{"line_number":427,"context_line":""},{"line_number":428,"context_line":"        # Attempt to disable node, this should fail"},{"line_number":429,"context_line":"        # since another process holds the lock"},{"line_number":430,"context_line":"        with testtools.ExpectedException(TimeoutException):"},{"line_number":431,"context_line":"            self.patch_argv(\u0027-c\u0027, configfile, \u0027hold\u0027, nodes[0].id, \u0027-t\u0027, \u00271\u0027)"},{"line_number":432,"context_line":"            nodepoolcmd.main()"},{"line_number":433,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"045300cf_11d9f7c0","line":430,"in_reply_to":"a63be71f_6d89eacd","updated":"2022-09-08 19:07:38.000000000","message":"It\u0027s timing out waiting for the lock:\n\n[...]\nFile \"nodepool/.tox/py3/lib/python3.8/site-packages/kazoo/recipe/lock.py\", line 280, in _inner_acquire\n    raise LockTimeout(\nkazoo.exceptions.LockTimeout: Failed to acquire lock on /nodepool/nodes/0000000000/lock after 5 seconds\n\nDuring handling of the above exception, another exception occurred:\n[...]\nFile \"nodepool/nodepool/zk/zookeeper.py\", line 1815, in lockNode\n    raise npe.TimeoutException(\nnodepool.exceptions.TimeoutException: Timeout trying to acquire lock /nodepool/nodes/0000000000/lock\n\nSo I assumed this is to be expected somehow","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"e3577ffa5906890c525f176376ba8f6c8de7f077","unresolved":false,"context_lines":[{"line_number":431,"context_line":"            self.patch_argv(\u0027-c\u0027, configfile, \u0027hold\u0027, nodes[0].id, \u0027-t\u0027, \u00271\u0027)"},{"line_number":432,"context_line":"            nodepoolcmd.main()"},{"line_number":433,"context_line":""},{"line_number":434,"context_line":"    def test_attempt_request_disabled_static_node(self):"},{"line_number":435,"context_line":"        configfile \u003d self.setup_config(\u0027static-basic.yaml\u0027)"},{"line_number":436,"context_line":"        pool \u003d self.useNodepool(configfile, watermark_sleep\u003d1)"},{"line_number":437,"context_line":"        pool.start()"}],"source_content_type":"text/x-python","patch_set":8,"id":"8b918028_480deff0","line":434,"updated":"2022-08-31 13:31:15.000000000","message":"Ditto.","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"e3577ffa5906890c525f176376ba8f6c8de7f077","unresolved":false,"context_lines":[{"line_number":459,"context_line":""},{"line_number":460,"context_line":"        # Make a node request"},{"line_number":461,"context_line":"        # Expect to timeout since the node is not ready"},{"line_number":462,"context_line":"        with testtools.ExpectedException(Exception):"},{"line_number":463,"context_line":"            self.log.debug(\"Waiting for request %s\", req.id)"},{"line_number":464,"context_line":"            req \u003d self.waitForNodeRequest(req, (zk.FULFILLED,))"},{"line_number":465,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"b617b336_70272b6b","line":462,"updated":"2022-08-31 13:31:15.000000000","message":"Ditto.","commit_id":"05ab4ebfef396bad1787955af402a8101e244f84"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"c1442c5c4dabdee34ff093809dd04997b1890269","unresolved":false,"context_lines":[{"line_number":467,"context_line":"        req \u003d self.zk.getNodeRequest(req.id)"},{"line_number":468,"context_line":""},{"line_number":469,"context_line":"        with testtools.ExpectedException(Exception):"},{"line_number":470,"context_line":"            req \u003d self.waitForNodeRequest(req, (zk.FULFILLED,), max_time\u003d5)"},{"line_number":471,"context_line":""},{"line_number":472,"context_line":"        self.assertEqual(len(req.nodes), 0)"},{"line_number":473,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"c8c02d0f_825ad171","line":470,"updated":"2022-10-11 17:21:00.000000000","message":"This is pretty fast for a unit test when they\u0027re all running in parallel.  Since we expect this to timeout it can be less than a minute, but it should probably be at least 30 seconds.","commit_id":"4951790eb6035aa72a70f3bc1f26cc530cf2920f"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9c07472f5633b019853ee1244f15a2d0d6617eb","unresolved":false,"context_lines":[{"line_number":433,"context_line":"        # and the node should still be in use"},{"line_number":434,"context_line":"        self.patch_argv(\u0027-c\u0027, configfile, \u0027hold\u0027, nodes[0].id)"},{"line_number":435,"context_line":"        nodepoolcmd.main()"},{"line_number":436,"context_line":"        time.sleep(0.1)"},{"line_number":437,"context_line":"        self.assert_nodes_listed(configfile, 1, zk.IN_USE)"},{"line_number":438,"context_line":""},{"line_number":439,"context_line":"    def test_attempt_request_held_static_node(self):"}],"source_content_type":"text/x-python","patch_set":14,"id":"01b52bec_2e696b64","line":436,"updated":"2022-10-11 15:41:55.000000000","message":"What is this tiny sleep for?","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b9c07472f5633b019853ee1244f15a2d0d6617eb","unresolved":false,"context_lines":[{"line_number":467,"context_line":"        self.log.debug(\"Waiting for request %s\", req.id)"},{"line_number":468,"context_line":"        req \u003d self.zk.getNodeRequest(req.id)"},{"line_number":469,"context_line":""},{"line_number":470,"context_line":"        time.sleep(1)"},{"line_number":471,"context_line":"        req \u003d self.zk.getNodeRequest(req.id)"},{"line_number":472,"context_line":"        self.assertEqual(req.state, zk.REQUESTED)"},{"line_number":473,"context_line":"        self.assertEqual(len(req.nodes), 0)"}],"source_content_type":"text/x-python","patch_set":14,"id":"889ab349_6f769e83","line":470,"updated":"2022-10-11 15:41:55.000000000","message":"As I mentioned in the previous patchset, because of the paralellism in unit tests, we need longer delays in order for the test to be valid.","commit_id":"b16a1d8bfe32fbe0dc21752562b5d14d889a29cc"}]}