)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"573ee49a64d44d9af873fcb1f6367f9dc0b98991","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"244d7b11_405304c8","updated":"2023-02-07 12:38:32.000000000","message":"recheck","commit_id":"f9e3082b31c7030ade5e9778a008b6796010b00b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"1cdaef1f19edb8cd45cb493bd651a47bd211745f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"4c904618_83e48275","updated":"2023-02-10 19:39:05.000000000","message":"recheck looks like maybe the failures are due to zookeeper problems? Its hard to make out in the large logfile.","commit_id":"00e84cd125859b5fec5fbe1ae8d108612997a7b2"}],"tests/unit/test_zk.py":[{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"39c2f4d2b539cce193cc879e737966b09225b590","unresolved":true,"context_lines":[{"line_number":2052,"context_line":"        pipeline.state \u003d model.PipelineState.create("},{"line_number":2053,"context_line":"            pipeline, layout.uuid, pipeline.state)"},{"line_number":2054,"context_line":"        context \u003d ZKContext(self.zk_client, None, None, self.log)"},{"line_number":2055,"context_line":"        pipeline.state.refresh(context)"},{"line_number":2056,"context_line":""},{"line_number":2057,"context_line":"    def test_pipeline_state_existing_object(self):"},{"line_number":2058,"context_line":"        # Test the initialize-on-refresh code path with a pre-existing object"}],"source_content_type":"text/x-python","patch_set":1,"id":"59dee5cf_91607738","line":2055,"updated":"2023-02-02 13:21:14.000000000","message":"I think we should assert here that the path in ZK exists. Something like:\n\n  self.assertIsNotNone(\n      self.zk_client.client.exists(self.pipeline.state.getPath()))","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"3bd969ee05c02f0abe1ff971bdcdafad01bab524","unresolved":false,"context_lines":[{"line_number":2052,"context_line":"        pipeline.state \u003d model.PipelineState.create("},{"line_number":2053,"context_line":"            pipeline, layout.uuid, pipeline.state)"},{"line_number":2054,"context_line":"        context \u003d ZKContext(self.zk_client, None, None, self.log)"},{"line_number":2055,"context_line":"        pipeline.state.refresh(context)"},{"line_number":2056,"context_line":""},{"line_number":2057,"context_line":"    def test_pipeline_state_existing_object(self):"},{"line_number":2058,"context_line":"        # Test the initialize-on-refresh code path with a pre-existing object"}],"source_content_type":"text/x-python","patch_set":1,"id":"10ee254b_28f21795","line":2055,"in_reply_to":"59dee5cf_91607738","updated":"2023-02-02 15:40:34.000000000","message":"Agreed.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"39c2f4d2b539cce193cc879e737966b09225b590","unresolved":true,"context_lines":[{"line_number":2054,"context_line":"        context \u003d ZKContext(self.zk_client, None, None, self.log)"},{"line_number":2055,"context_line":"        pipeline.state.refresh(context)"},{"line_number":2056,"context_line":""},{"line_number":2057,"context_line":"    def test_pipeline_state_existing_object(self):"},{"line_number":2058,"context_line":"        # Test the initialize-on-refresh code path with a pre-existing object"},{"line_number":2059,"context_line":"        tenant \u003d model.Tenant(\u0027tenant\u0027)"},{"line_number":2060,"context_line":"        pipeline \u003d model.Pipeline(\u0027gate\u0027, tenant)"}],"source_content_type":"text/x-python","patch_set":1,"id":"83f668a0_fd160403","line":2057,"updated":"2023-02-02 13:21:14.000000000","message":"nit: this also tests the change list","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"3bd969ee05c02f0abe1ff971bdcdafad01bab524","unresolved":false,"context_lines":[{"line_number":2054,"context_line":"        context \u003d ZKContext(self.zk_client, None, None, self.log)"},{"line_number":2055,"context_line":"        pipeline.state.refresh(context)"},{"line_number":2056,"context_line":""},{"line_number":2057,"context_line":"    def test_pipeline_state_existing_object(self):"},{"line_number":2058,"context_line":"        # Test the initialize-on-refresh code path with a pre-existing object"},{"line_number":2059,"context_line":"        tenant \u003d model.Tenant(\u0027tenant\u0027)"},{"line_number":2060,"context_line":"        pipeline \u003d model.Pipeline(\u0027gate\u0027, tenant)"}],"source_content_type":"text/x-python","patch_set":1,"id":"c34848cc_d67c77fd","line":2057,"in_reply_to":"83f668a0_fd160403","updated":"2023-02-02 15:40:34.000000000","message":"Yes; it\u0027s needed in order to cause the other code path to run.  These tests are super fast, so I wanted to have explicit testing of each thing, even though there\u0027s some duplication.  I can leave a comment explaining.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"39c2f4d2b539cce193cc879e737966b09225b590","unresolved":true,"context_lines":[{"line_number":2055,"context_line":"        pipeline.state.refresh(context)"},{"line_number":2056,"context_line":""},{"line_number":2057,"context_line":"    def test_pipeline_state_existing_object(self):"},{"line_number":2058,"context_line":"        # Test the initialize-on-refresh code path with a pre-existing object"},{"line_number":2059,"context_line":"        tenant \u003d model.Tenant(\u0027tenant\u0027)"},{"line_number":2060,"context_line":"        pipeline \u003d model.Pipeline(\u0027gate\u0027, tenant)"},{"line_number":2061,"context_line":"        layout \u003d model.Layout(tenant)"}],"source_content_type":"text/x-python","patch_set":1,"id":"62f99489_c6b88a6b","line":2058,"updated":"2023-02-02 13:21:14.000000000","message":"Is this testing with a pre-existing object?\n\nI was expecting something like a create + initial refresh of the state (which would create the ZK path) and then another create + refresh. Otherwise this just seems to be a combination of test 1 and 3.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"c727ecf3d9ab4e926cbb3093774e1cb2c10d0f5a","unresolved":true,"context_lines":[{"line_number":2055,"context_line":"        pipeline.state.refresh(context)"},{"line_number":2056,"context_line":""},{"line_number":2057,"context_line":"    def test_pipeline_state_existing_object(self):"},{"line_number":2058,"context_line":"        # Test the initialize-on-refresh code path with a pre-existing object"},{"line_number":2059,"context_line":"        tenant \u003d model.Tenant(\u0027tenant\u0027)"},{"line_number":2060,"context_line":"        pipeline \u003d model.Pipeline(\u0027gate\u0027, tenant)"},{"line_number":2061,"context_line":"        layout \u003d model.Layout(tenant)"}],"source_content_type":"text/x-python","patch_set":1,"id":"f2b539bc_4441e950","line":2058,"in_reply_to":"62f99489_c6b88a6b","updated":"2023-02-02 13:32:46.000000000","message":"Ok, I think I get it now. The refresh of the change list in line #2069 will create the empty pipeline state as a side-effect (`change list path \u003d\u003d $pipeline_state_path/change_list`). And then the refresh of the state will find an existing but empty node in ZK.\n\nMaybe it\u0027s worth to have a comment for that.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"3bd969ee05c02f0abe1ff971bdcdafad01bab524","unresolved":false,"context_lines":[{"line_number":2055,"context_line":"        pipeline.state.refresh(context)"},{"line_number":2056,"context_line":""},{"line_number":2057,"context_line":"    def test_pipeline_state_existing_object(self):"},{"line_number":2058,"context_line":"        # Test the initialize-on-refresh code path with a pre-existing object"},{"line_number":2059,"context_line":"        tenant \u003d model.Tenant(\u0027tenant\u0027)"},{"line_number":2060,"context_line":"        pipeline \u003d model.Pipeline(\u0027gate\u0027, tenant)"},{"line_number":2061,"context_line":"        layout \u003d model.Layout(tenant)"}],"source_content_type":"text/x-python","patch_set":1,"id":"96702d90_1458685d","line":2058,"in_reply_to":"62f99489_c6b88a6b","updated":"2023-02-02 15:40:34.000000000","message":"Yep.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"39c2f4d2b539cce193cc879e737966b09225b590","unresolved":true,"context_lines":[{"line_number":2080,"context_line":"        pipeline.change_list \u003d model.PipelineChangeList.create("},{"line_number":2081,"context_line":"            pipeline)"},{"line_number":2082,"context_line":"        context \u003d ZKContext(self.zk_client, None, None, self.log)"},{"line_number":2083,"context_line":"        pipeline.change_list.refresh(context)"}],"source_content_type":"text/x-python","patch_set":1,"id":"02527e33_2c9eabc7","line":2083,"updated":"2023-02-02 13:21:14.000000000","message":"I think we should assert that the ZK path exists.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"3bd969ee05c02f0abe1ff971bdcdafad01bab524","unresolved":false,"context_lines":[{"line_number":2080,"context_line":"        pipeline.change_list \u003d model.PipelineChangeList.create("},{"line_number":2081,"context_line":"            pipeline)"},{"line_number":2082,"context_line":"        context \u003d ZKContext(self.zk_client, None, None, self.log)"},{"line_number":2083,"context_line":"        pipeline.change_list.refresh(context)"}],"source_content_type":"text/x-python","patch_set":1,"id":"ab22a16b_144dd692","line":2083,"in_reply_to":"02527e33_2c9eabc7","updated":"2023-02-02 15:40:34.000000000","message":"Agreed.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11653c96a230026f009e00c577e2e7ff0549a3bd","unresolved":true,"context_lines":[{"line_number":2055,"context_line":"            pipeline, layout.uuid, pipeline.state)"},{"line_number":2056,"context_line":"        context \u003d ZKContext(self.zk_client, None, None, self.log)"},{"line_number":2057,"context_line":"        pipeline.state.refresh(context)"},{"line_number":2058,"context_line":"        self.assertTrue(self.zk_client.client.exists(pipeline.state.getPath()))"},{"line_number":2059,"context_line":""},{"line_number":2060,"context_line":"    def test_pipeline_state_existing_object(self):"},{"line_number":2061,"context_line":"        # Test the initialize-on-refresh code path with a pre-existing object"}],"source_content_type":"text/x-python","patch_set":4,"id":"e79dc062_0d0c1db9","line":2058,"updated":"2023-02-10 16:53:35.000000000","message":"Related to some of my other questions should we check the data stored here too? That would give a better sense of what information is written back at this point.","commit_id":"00e84cd125859b5fec5fbe1ae8d108612997a7b2"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"cc385bc18b14d9c19eb9514f2c856e9be92420b4","unresolved":false,"context_lines":[{"line_number":2055,"context_line":"            pipeline, layout.uuid, pipeline.state)"},{"line_number":2056,"context_line":"        context \u003d ZKContext(self.zk_client, None, None, self.log)"},{"line_number":2057,"context_line":"        pipeline.state.refresh(context)"},{"line_number":2058,"context_line":"        self.assertTrue(self.zk_client.client.exists(pipeline.state.getPath()))"},{"line_number":2059,"context_line":""},{"line_number":2060,"context_line":"    def test_pipeline_state_existing_object(self):"},{"line_number":2061,"context_line":"        # Test the initialize-on-refresh code path with a pre-existing object"}],"source_content_type":"text/x-python","patch_set":4,"id":"c1674ae8_2c090779","line":2058,"in_reply_to":"e79dc062_0d0c1db9","updated":"2023-02-10 17:14:33.000000000","message":"I don\u0027t think the additional test complexity of reading data via a second route is worth it.  We have coverage of reading and writing zk objects and this uses the usual read/write methods.  We just need to exercise all the code paths to make sure we get the corner cases for initialization.  I think for this test, we either get a working object back (success) or it raises an exception.","commit_id":"00e84cd125859b5fec5fbe1ae8d108612997a7b2"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11653c96a230026f009e00c577e2e7ff0549a3bd","unresolved":true,"context_lines":[{"line_number":2071,"context_line":"        context \u003d ZKContext(self.zk_client, None, None, self.log)"},{"line_number":2072,"context_line":"        # We refresh the change list here purely for the side effect"},{"line_number":2073,"context_line":"        # of creating the pipeline state object with no data (the list"},{"line_number":2074,"context_line":"        # is a subpath of the state object)."},{"line_number":2075,"context_line":"        pipeline.change_list.refresh(context)"},{"line_number":2076,"context_line":"        pipeline.state.refresh(context)"},{"line_number":2077,"context_line":"        self.assertTrue("}],"source_content_type":"text/x-python","patch_set":4,"id":"cea88ecc_0926ec2b","line":2074,"updated":"2023-02-10 16:53:35.000000000","message":"Similar to above maybe assert the null? state here. Then refresh the change list and check for empty list?","commit_id":"00e84cd125859b5fec5fbe1ae8d108612997a7b2"}],"zuul/model.py":[{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"39c2f4d2b539cce193cc879e737966b09225b590","unresolved":true,"context_lines":[{"line_number":740,"context_line":"            # NoNodeError.  This happens because the postConfig call"},{"line_number":741,"context_line":"            # creates this object without holding the pipeline lock,"},{"line_number":742,"context_line":"            # so it can\u0027t determine whether or not it exists in ZK."},{"line_number":743,"context_line":"            # We do hold the pipeline lock here, so if we get this"},{"line_number":744,"context_line":"            # error, we know we\u0027re initializing the object, and we"},{"line_number":745,"context_line":"            # should write it to ZK."},{"line_number":746,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"859afec9_cfddced2","line":743,"updated":"2023-02-02 13:21:14.000000000","message":"This is only true when `read_only\u003dFalse`.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"3bd969ee05c02f0abe1ff971bdcdafad01bab524","unresolved":false,"context_lines":[{"line_number":740,"context_line":"            # NoNodeError.  This happens because the postConfig call"},{"line_number":741,"context_line":"            # creates this object without holding the pipeline lock,"},{"line_number":742,"context_line":"            # so it can\u0027t determine whether or not it exists in ZK."},{"line_number":743,"context_line":"            # We do hold the pipeline lock here, so if we get this"},{"line_number":744,"context_line":"            # error, we know we\u0027re initializing the object, and we"},{"line_number":745,"context_line":"            # should write it to ZK."},{"line_number":746,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"c10cd11b_2529d09e","line":743,"in_reply_to":"859afec9_cfddced2","updated":"2023-02-02 15:40:34.000000000","message":"There are three places we call pipeline.state.refresh; all in scheduler.py.  And in all three places we do hold the pipeline lock.  Did I miss something?","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"b679b527024611aa066dd14072fd175eca178ce0","unresolved":false,"context_lines":[{"line_number":740,"context_line":"            # NoNodeError.  This happens because the postConfig call"},{"line_number":741,"context_line":"            # creates this object without holding the pipeline lock,"},{"line_number":742,"context_line":"            # so it can\u0027t determine whether or not it exists in ZK."},{"line_number":743,"context_line":"            # We do hold the pipeline lock here, so if we get this"},{"line_number":744,"context_line":"            # error, we know we\u0027re initializing the object, and we"},{"line_number":745,"context_line":"            # should write it to ZK."},{"line_number":746,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"c746ec68_109ab080","line":743,"in_reply_to":"c10cd11b_2529d09e","updated":"2023-02-02 16:06:58.000000000","message":"Yep, you are right. Sorry missed that.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"39c2f4d2b539cce193cc879e737966b09225b590","unresolved":true,"context_lines":[{"line_number":753,"context_line":"                                \"this is expected only for new pipelines\","},{"line_number":754,"context_line":"                                self.pipeline.name)"},{"line_number":755,"context_line":"            self._set(**self._lateInitData())"},{"line_number":756,"context_line":"            self.internalCreate(context)"},{"line_number":757,"context_line":""},{"line_number":758,"context_line":"    def deserialize(self, raw, context):"},{"line_number":759,"context_line":"        # We may have old change objects in the pipeline cache, so"}],"source_content_type":"text/x-python","patch_set":1,"id":"3bc8f3e8_12ea817a","line":756,"updated":"2023-02-02 13:21:14.000000000","message":"I think we must not create the pipeline state when refreshing the pipeline in read-only mode. Otherwise we are re-creating a similar race condition that we are trying to fix (I could imagine that this is also the reason for the test failures).","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"3bd969ee05c02f0abe1ff971bdcdafad01bab524","unresolved":false,"context_lines":[{"line_number":753,"context_line":"                                \"this is expected only for new pipelines\","},{"line_number":754,"context_line":"                                self.pipeline.name)"},{"line_number":755,"context_line":"            self._set(**self._lateInitData())"},{"line_number":756,"context_line":"            self.internalCreate(context)"},{"line_number":757,"context_line":""},{"line_number":758,"context_line":"    def deserialize(self, raw, context):"},{"line_number":759,"context_line":"        # We may have old change objects in the pipeline cache, so"}],"source_content_type":"text/x-python","patch_set":1,"id":"95780611_7bccb169","line":756,"in_reply_to":"3bc8f3e8_12ea817a","updated":"2023-02-02 15:40:34.000000000","message":"Assuming the above question about whether we hold the pipeline lock is resolved such that we do, then this is safe.  The important thing is the lock, not \"read-only\".  It\u0027s confusing, so I put a comment in for that.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"b679b527024611aa066dd14072fd175eca178ce0","unresolved":false,"context_lines":[{"line_number":753,"context_line":"                                \"this is expected only for new pipelines\","},{"line_number":754,"context_line":"                                self.pipeline.name)"},{"line_number":755,"context_line":"            self._set(**self._lateInitData())"},{"line_number":756,"context_line":"            self.internalCreate(context)"},{"line_number":757,"context_line":""},{"line_number":758,"context_line":"    def deserialize(self, raw, context):"},{"line_number":759,"context_line":"        # We may have old change objects in the pipeline cache, so"}],"source_content_type":"text/x-python","patch_set":1,"id":"22076e77_a953ae44","line":756,"in_reply_to":"95780611_7bccb169","updated":"2023-02-02 16:06:58.000000000","message":"Ack","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"39c2f4d2b539cce193cc879e737966b09225b590","unresolved":true,"context_lines":[{"line_number":961,"context_line":"            # NoNodeError.  This happens because the postConfig call"},{"line_number":962,"context_line":"            # creates this object without holding the pipeline lock,"},{"line_number":963,"context_line":"            # so it can\u0027t determine whether or not it exists in ZK."},{"line_number":964,"context_line":"            # We do hold the pipeline lock here, so if we get this"},{"line_number":965,"context_line":"            # error, we know we\u0027re initializing the object, and"},{"line_number":966,"context_line":"            # we should write it to ZK."},{"line_number":967,"context_line":"            context.log.warning("}],"source_content_type":"text/x-python","patch_set":1,"id":"41b42733_6e9c3749","line":964,"updated":"2023-02-02 13:21:14.000000000","message":"This is not always the case. We also refresh the pipeline change list during tenant trigger event processing and when gathering connection cache keys during cleanup w/o holding the pipeline lock.\n\nIn those cases we don\u0027t want to create the initial change list. Maybe we also need a `read_only` flag here.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"3bd969ee05c02f0abe1ff971bdcdafad01bab524","unresolved":false,"context_lines":[{"line_number":961,"context_line":"            # NoNodeError.  This happens because the postConfig call"},{"line_number":962,"context_line":"            # creates this object without holding the pipeline lock,"},{"line_number":963,"context_line":"            # so it can\u0027t determine whether or not it exists in ZK."},{"line_number":964,"context_line":"            # We do hold the pipeline lock here, so if we get this"},{"line_number":965,"context_line":"            # error, we know we\u0027re initializing the object, and"},{"line_number":966,"context_line":"            # we should write it to ZK."},{"line_number":967,"context_line":"            context.log.warning("}],"source_content_type":"text/x-python","patch_set":1,"id":"a23a93ef_89dca027","line":964,"in_reply_to":"41b42733_6e9c3749","updated":"2023-02-02 15:40:34.000000000","message":"That\u0027s true, I missed those two cases.  We do need to handle them, possibly with a flag (but probably not \"read_only\" since that\u0027s confusing) and in that case, we should raise an exception that we catch in the retry method.","commit_id":"c3889f5caaaee2c54bd9adb810c90dfa09f60170"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"ef9a2ef17b8f6d66a0705e8ddffd358d335ec4f9","unresolved":true,"context_lines":[{"line_number":977,"context_line":"                # If we\u0027re called from a context where we can\u0027t"},{"line_number":978,"context_line":"                # initialize the change list, re-raise the exception"},{"line_number":979,"context_line":"                # so that it can be caught and retried."},{"line_number":980,"context_line":"                raise zkobject.ZKObjectNotInitializedError()"},{"line_number":981,"context_line":""},{"line_number":982,"context_line":"    def getPath(self):"},{"line_number":983,"context_line":"        return self.getChangeListPath(self.pipeline)"}],"source_content_type":"text/x-python","patch_set":3,"id":"f92ebcef_414241fa","line":980,"updated":"2023-02-07 12:53:30.000000000","message":"I think the name of the exception is a bit misleading. When we get a `NoNodeError` we don\u0027t know if the pipeline was not initialized or if the change list is currently in the process of being updated (sharded zkobject).","commit_id":"f9e3082b31c7030ade5e9778a008b6796010b00b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"3fa7bbd21a4b0100e86dd5aeb5522015474a8bf8","unresolved":false,"context_lines":[{"line_number":977,"context_line":"                # If we\u0027re called from a context where we can\u0027t"},{"line_number":978,"context_line":"                # initialize the change list, re-raise the exception"},{"line_number":979,"context_line":"                # so that it can be caught and retried."},{"line_number":980,"context_line":"                raise zkobject.ZKObjectNotInitializedError()"},{"line_number":981,"context_line":""},{"line_number":982,"context_line":"    def getPath(self):"},{"line_number":983,"context_line":"        return self.getChangeListPath(self.pipeline)"}],"source_content_type":"text/x-python","patch_set":3,"id":"7338eb96_f4f7f5f9","line":980,"in_reply_to":"f92ebcef_414241fa","updated":"2023-02-07 19:12:33.000000000","message":"Yep.","commit_id":"f9e3082b31c7030ade5e9778a008b6796010b00b"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11653c96a230026f009e00c577e2e7ff0549a3bd","unresolved":true,"context_lines":[{"line_number":645,"context_line":"                old_state._resetObjectRefs()"},{"line_number":646,"context_line":"                return old_state"},{"line_number":647,"context_line":"            return state"},{"line_number":648,"context_line":"        return cls.new(ctx, pipeline\u003dpipeline, layout_uuid\u003dlayout_uuid)"},{"line_number":649,"context_line":""},{"line_number":650,"context_line":"    def _resetObjectRefs(self):"},{"line_number":651,"context_line":"        # Update the pipeline references on the queue objects."}],"source_content_type":"text/x-python","patch_set":4,"id":"2716ce94_0578ff6a","side":"PARENT","line":648,"range":{"start_line":648,"start_character":47,"end_line":648,"end_character":70},"updated":"2023-02-10 16:53:35.000000000","message":"Looks like we stop setting the layout_uuid on the PipelineState here as we get it from the pipeline object in _lateInitData instead.\n\nAlso looks like create() is not a common pattern to all the zkobjects that we\u0027re changing semantics for. This method is only on the two classes modified in this change.","commit_id":"e57548e3714454f54148eac69ef3daaafadab05c"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"063568a842ec5145c11674d06082552ab8e77cdb","unresolved":false,"context_lines":[{"line_number":645,"context_line":"                old_state._resetObjectRefs()"},{"line_number":646,"context_line":"                return old_state"},{"line_number":647,"context_line":"            return state"},{"line_number":648,"context_line":"        return cls.new(ctx, pipeline\u003dpipeline, layout_uuid\u003dlayout_uuid)"},{"line_number":649,"context_line":""},{"line_number":650,"context_line":"    def _resetObjectRefs(self):"},{"line_number":651,"context_line":"        # Update the pipeline references on the queue objects."}],"source_content_type":"text/x-python","patch_set":4,"id":"f59c14b0_31346d0c","side":"PARENT","line":648,"range":{"start_line":648,"start_character":47,"end_line":648,"end_character":70},"in_reply_to":"2716ce94_0578ff6a","updated":"2023-02-10 17:32:52.000000000","message":"Note this was mostly written for my own understanding and for the hopes of helping anyone else reviewing the change. Doesn\u0027t call out something that needs updating.","commit_id":"e57548e3714454f54148eac69ef3daaafadab05c"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11653c96a230026f009e00c577e2e7ff0549a3bd","unresolved":true,"context_lines":[{"line_number":773,"context_line":"            context.log.warning(\"Initializing pipeline state for %s; \""},{"line_number":774,"context_line":"                                \"this is expected only for new pipelines\","},{"line_number":775,"context_line":"                                self.pipeline.name)"},{"line_number":776,"context_line":"            return self._lateInitData()"},{"line_number":777,"context_line":""},{"line_number":778,"context_line":"        data \u003d super().deserialize(raw, context)"},{"line_number":779,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"c1e1b3ea_c12dd03f","line":776,"updated":"2023-02-10 16:53:35.000000000","message":"In refresh we note that this deserialize() path is used instaed. But unlike refresh() we don\u0027t call _set() to set the layout_uuid on this object. And we don\u0027t write out the contents to zookeeper. Should these two be equivalent considering one is taking precedence over the other?\n\nI\u0027m guessing we may not always have a write lock when deserializing which makes that unsafe. This leads me to wonder when we do actually write back to zookeeper for this object? Also, any concern with multiple schedulers passing through this block without a lock?","commit_id":"00e84cd125859b5fec5fbe1ae8d108612997a7b2"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"cc385bc18b14d9c19eb9514f2c856e9be92420b4","unresolved":false,"context_lines":[{"line_number":773,"context_line":"            context.log.warning(\"Initializing pipeline state for %s; \""},{"line_number":774,"context_line":"                                \"this is expected only for new pipelines\","},{"line_number":775,"context_line":"                                self.pipeline.name)"},{"line_number":776,"context_line":"            return self._lateInitData()"},{"line_number":777,"context_line":""},{"line_number":778,"context_line":"        data \u003d super().deserialize(raw, context)"},{"line_number":779,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"c39c914c_1bb5f5ea","line":776,"in_reply_to":"c1e1b3ea_c12dd03f","updated":"2023-02-10 17:14:33.000000000","message":"I\u0027m not seeing a reference to layout_uuid in refresh, so I\u0027m having trouble parsing this.\n\nWe write to ZK at the end of every pipeline processing (or if we call internalCreate).","commit_id":"00e84cd125859b5fec5fbe1ae8d108612997a7b2"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"063568a842ec5145c11674d06082552ab8e77cdb","unresolved":false,"context_lines":[{"line_number":773,"context_line":"            context.log.warning(\"Initializing pipeline state for %s; \""},{"line_number":774,"context_line":"                                \"this is expected only for new pipelines\","},{"line_number":775,"context_line":"                                self.pipeline.name)"},{"line_number":776,"context_line":"            return self._lateInitData()"},{"line_number":777,"context_line":""},{"line_number":778,"context_line":"        data \u003d super().deserialize(raw, context)"},{"line_number":779,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"cc97ca2e_08aa48c4","line":776,"in_reply_to":"c39c914c_1bb5f5ea","updated":"2023-02-10 17:32:52.000000000","message":"I was missing the refresh -\u003e load -\u003e deserialize call path in my head. We end up _set()ing the return value of this call up the stack. THat will set layout_uuid from the _lateInitData() call.","commit_id":"00e84cd125859b5fec5fbe1ae8d108612997a7b2"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"11653c96a230026f009e00c577e2e7ff0549a3bd","unresolved":true,"context_lines":[{"line_number":952,"context_line":"            _change_keys\u003d[],"},{"line_number":953,"context_line":"        )"},{"line_number":954,"context_line":""},{"line_number":955,"context_line":"    def refresh(self, context, allow_init\u003dTrue):"},{"line_number":956,"context_line":"        # Set allow_init to false to indicate that we don\u0027t hold the"},{"line_number":957,"context_line":"        # lock and we should not try to initialize the object in ZK if"},{"line_number":958,"context_line":"        # it does not exist."}],"source_content_type":"text/x-python","patch_set":4,"id":"20678488_6d12463e","line":955,"range":{"start_line":955,"start_character":31,"end_line":955,"end_character":46},"updated":"2023-02-10 16:53:35.000000000","message":"Nit, might be more defensive to default to false here and explicitly indicate the lock is held when it is rather than the opposite.","commit_id":"00e84cd125859b5fec5fbe1ae8d108612997a7b2"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"cc385bc18b14d9c19eb9514f2c856e9be92420b4","unresolved":false,"context_lines":[{"line_number":952,"context_line":"            _change_keys\u003d[],"},{"line_number":953,"context_line":"        )"},{"line_number":954,"context_line":""},{"line_number":955,"context_line":"    def refresh(self, context, allow_init\u003dTrue):"},{"line_number":956,"context_line":"        # Set allow_init to false to indicate that we don\u0027t hold the"},{"line_number":957,"context_line":"        # lock and we should not try to initialize the object in ZK if"},{"line_number":958,"context_line":"        # it does not exist."}],"source_content_type":"text/x-python","patch_set":4,"id":"c54d0f19_2e8b99e7","line":955,"in_reply_to":"20678488_6d12463e","updated":"2023-02-10 17:14:33.000000000","message":"The normal pattern is to hold the lock, so it\u0027s designed for that.  I get where you\u0027re coming from, but I don\u0027t want to flip the pattern so we have to do extra work when we have the lock.  We should *always* have the lock, and in the one or two places where we decided it\u0027s okay not to, we should be really careful and explicit about it.  I don\u0027t want us thinking that the default is working with it unlocked because it is very much not safe.","commit_id":"00e84cd125859b5fec5fbe1ae8d108612997a7b2"}],"zuul/scheduler.py":[{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"ef9a2ef17b8f6d66a0705e8ddffd358d335ec4f9","unresolved":true,"context_lines":[{"line_number":2254,"context_line":"                    except ZKObjectNotInitializedError:"},{"line_number":2255,"context_line":"                        # If the pipeline hasn\u0027t been initialized,"},{"line_number":2256,"context_line":"                        # there probably aren\u0027t any changes in it, so"},{"line_number":2257,"context_line":"                        # we can ignore it."},{"line_number":2258,"context_line":"                        continue"},{"line_number":2259,"context_line":"                    change_keys \u003d pipeline.change_list.getChangeKeys()"},{"line_number":2260,"context_line":"                    relevant_changes \u003d pipeline.manager.resolveChangeKeys("}],"source_content_type":"text/x-python","patch_set":3,"id":"f12e9447_fbcb392d","line":2257,"updated":"2023-02-07 12:53:30.000000000","message":"I think it\u0027s not safe to assume that the pipeline is not yet initialized when we get a `ZKObjectNotInitializedError` aka. `NoNodeError` (see my comment in model.py).\n\nContinuing here in case of error might also lead to the cleanup of cached changes that are still relevant (if they are also older than `max_age\u003d2h`).","commit_id":"f9e3082b31c7030ade5e9778a008b6796010b00b"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"3fa7bbd21a4b0100e86dd5aeb5522015474a8bf8","unresolved":false,"context_lines":[{"line_number":2254,"context_line":"                    except ZKObjectNotInitializedError:"},{"line_number":2255,"context_line":"                        # If the pipeline hasn\u0027t been initialized,"},{"line_number":2256,"context_line":"                        # there probably aren\u0027t any changes in it, so"},{"line_number":2257,"context_line":"                        # we can ignore it."},{"line_number":2258,"context_line":"                        continue"},{"line_number":2259,"context_line":"                    change_keys \u003d pipeline.change_list.getChangeKeys()"},{"line_number":2260,"context_line":"                    relevant_changes \u003d pipeline.manager.resolveChangeKeys("}],"source_content_type":"text/x-python","patch_set":3,"id":"26bc5244_d7e0b6f9","line":2257,"in_reply_to":"f12e9447_fbcb392d","updated":"2023-02-07 19:12:33.000000000","message":"Agreed.  I was hoping to improve this case, as well as the other one below.  But I can\u0027t find a way to distinguish \"uninitialized\" from \"write in progress\" without significantly more complexity, so I think we\u0027ll have to live with the current guarantees (or lack thereof).","commit_id":"f9e3082b31c7030ade5e9778a008b6796010b00b"}]}