)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"672093cb7f9e0bd157189a06b99339dd3733b964","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Matthew Oliver \u003cmatt@oliver.net.au\u003e"},{"line_number":5,"context_line":"CommitDate: 2021-09-17 16:52:49 +1000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"sharder: update shard storage_policy_index if roots changes - Simple"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If there is a change to the root container\u0027s storage_policy_index (e.g."},{"line_number":10,"context_line":"after it\u0027s deleted and recreated), the reconciler will kick in and"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"48b11df0_3d6d383a","line":7,"updated":"2021-09-17 15:56:46.000000000","message":"I like how this is coming on, adding the tests in the follow up [1] onto here I\u0027m gaining a lot of confidence that the relationship between the STATE (the spi) and TIMESTAMP-METADATA-FOR-THAT-STATE (which is ALL 3 put, delete, status timestamps) must follow in the shards the same as the root if we want replication and sharding to work together to propogate a consistent view.\n\nI\u0027m not sure I\u0027d call it \"Simple\"\n\n1. https://review.opendev.org/c/openstack/swift/+/805389","commit_id":"b1c3b8976884d22aa18f98956a4f8b6a9d6bb48f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"672093cb7f9e0bd157189a06b99339dd3733b964","unresolved":true,"context_lines":[{"line_number":19,"context_line":"same fragment."},{"line_number":20,"context_line":"Because they come and go (as things shrink or cleave again) I\u0027m not sure"},{"line_number":21,"context_line":"how well locking the root put and delete TS to the shards would really"},{"line_number":22,"context_line":"play out."},{"line_number":23,"context_line":"So this patch goes the other way, which simiplies the whole thing."},{"line_number":24,"context_line":"We already have landed a patch that in essence makes the shards storage"},{"line_number":25,"context_line":"for whatever objects the root puts in at whatever policy the root supplies."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"90556adf_c1220b4b","line":22,"updated":"2021-09-17 15:56:46.000000000","message":"turns out pretty great IMHO","commit_id":"b1c3b8976884d22aa18f98956a4f8b6a9d6bb48f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"672093cb7f9e0bd157189a06b99339dd3733b964","unresolved":true,"context_lines":[{"line_number":35,"context_line":"sync.. but they should all colalease in the end."},{"line_number":36,"context_line":"Further in previous discussions, we\u0027ve shown how out of character a SPI"},{"line_number":37,"context_line":"is and If we end up enqueue-ing a bunch of objects (incorrectly) and then"},{"line_number":38,"context_line":"flap back then the reconciler will just noop them in the queue right?"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"Change-Id: Id8ce5630718895a27ecfaff02e3ba3e20d3eb421"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"3ab2ac94_c1e81596","line":38,"updated":"2021-09-17 15:56:46.000000000","message":"I think none of this applies anymore - there\u0027s no flapping - it\u0027s just big buckets of WIN all over the place!","commit_id":"b1c3b8976884d22aa18f98956a4f8b6a9d6bb48f"}],"swift/container/backend.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c01f94af314d3018e4e345ae414aee80a25cde70","unresolved":true,"context_lines":[{"line_number":1023,"context_line":"            conn.execute(\u0027\u0027\u0027"},{"line_number":1024,"context_line":"                UPDATE container_stat"},{"line_number":1025,"context_line":"                SET storage_policy_index \u003d ?,"},{"line_number":1026,"context_line":"                    status_changed_at \u003d MAX(?, status_changed_at),"},{"line_number":1027,"context_line":"                    reconciler_sync_point \u003d 0"},{"line_number":1028,"context_line":"                WHERE storage_policy_index \u003c\u003e ?"},{"line_number":1029,"context_line":"            \u0027\u0027\u0027, (policy_index, timestamp, policy_index))"}],"source_content_type":"text/x-python","patch_set":1,"id":"e3d14e26_8fc8296b","line":1026,"updated":"2021-09-16 21:11:40.000000000","message":"on un-recreated databases the oldest status_changed_at storage policy index wins","commit_id":"bae6aa04118edef5becf1215a16386fb5c8c694c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9fcc24919dbdca2b81bdcd21a6963932279448e6","unresolved":true,"context_lines":[{"line_number":1008,"context_line":"            # only one policy_stat row"},{"line_number":1009,"context_line":"            return False"},{"line_number":1010,"context_line":""},{"line_number":1011,"context_line":"    def set_storage_policy_index(self, policy_index, timestamp\u003dNone):"},{"line_number":1012,"context_line":"        \"\"\""},{"line_number":1013,"context_line":"        Update the container_stat policy_index and status_changed_at."},{"line_number":1014,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"d235923a_5b853f62","line":1011,"range":{"start_line":1011,"start_character":53,"end_line":1011,"end_character":68},"updated":"2021-09-23 13:47:54.000000000","message":"I can\u0027t find a usage that does not pass timestamp, so perhaps we should eliminate the optional default to simplify understanding of this method","commit_id":"a6a92b66232254b923ab0fb6563a74bc01bcad8d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9fcc24919dbdca2b81bdcd21a6963932279448e6","unresolved":true,"context_lines":[{"line_number":1023,"context_line":"            conn.execute(\u0027\u0027\u0027"},{"line_number":1024,"context_line":"                UPDATE container_stat"},{"line_number":1025,"context_line":"                SET storage_policy_index \u003d ?,"},{"line_number":1026,"context_line":"                    status_changed_at \u003d ?,"},{"line_number":1027,"context_line":"                    reconciler_sync_point \u003d 0"},{"line_number":1028,"context_line":"                WHERE storage_policy_index \u003c\u003e ?"},{"line_number":1029,"context_line":"            \u0027\u0027\u0027, (policy_index, timestamp, policy_index))"}],"source_content_type":"text/x-python","patch_set":4,"id":"fd245dd5_59c36c12","line":1026,"updated":"2021-09-23 13:47:54.000000000","message":"This is a significant change but is not covered by any unit test (a probe test does fail if I revert this change)\n\nI actually think this may be accidentally (but fortuitously) part of fixing a different existing bug with replication (independent of the topic of this patch)...\n\nWhen the replicator calls this method [1], it passes \u0027now\u0027 as the timestamp. This method is only called if incorrect_policy_index is true (or the container has been recreated). I wonder if the replicator was forced to pass \u0027now\u0027 as the new status_changed_at just in order for the status_changed_at to be applied in this method, because this method was *broken*. With this change, the replicator calls could pass the status_changed_at time that came with the correct policy index, and that status_changed_at would now be applied. That would make more sense.\n\nThis is rooted in the fact that spi anomalously obeys an \u0027oldest trumps\u0027 rule, unless deleted. Looking at merge_timestamps, it is easy to see how newest wins could have been accidentally applied to status_changed_at.\n\nSo, existing replication of policy indexes may be broken:\n\nconsider a split brain new container with replicas R0, R1 and R2:\n\n  R0 has spi\u003d0, sca\u003dt0 (sca \u003d\u003d status_changed_at)\n  R1, R2 have spi\u003d1, sca\u003dt1\n\nthis should resolve to all replicas having spi\u003d0\n\n  R0 syncs with R1 at t2:\n    R0 has spi 0 at sca t0, R1 has spi 1 at sca t1\n    -\u003e R1 has incorrect policy index so R1 sets spi\u003d0, sca\u003dt2\n\n  R1 syncs with R2 at t3:\n    R1 has spi 0 at sca t2, R3 has spi 1 at sca t1\n    R1 has incorrect policy index so R1 sets spi\u003d1, sca\u003dt3. \u003c- BAD DECISION!\n\nand so on... (obviously a test to prove/reproduce that would be great!)\n\nIf I am correct (replication spi sync is broken) then I\u0027d advocate for this change moving to a parent patch that fixes replication, before we try to fix root-shard consistency.\n\n[1] in container replicator _handle_sync_response() and _handle_sync_request()-\u003e_get_synced_replication_info():","commit_id":"a6a92b66232254b923ab0fb6563a74bc01bcad8d"}],"swift/container/sharder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c01f94af314d3018e4e345ae414aee80a25cde70","unresolved":true,"context_lines":[{"line_number":1107,"context_line":"        if root_sp_index !\u003d broker.storage_policy_index:"},{"line_number":1108,"context_line":"            self.logger.info(\"Migrating shard %s to storage_policy_index \""},{"line_number":1109,"context_line":"                             \"%d\", broker.db_file, int(root_sp_index))"},{"line_number":1110,"context_line":"            broker.set_storage_policy_index(root_sp_index)"},{"line_number":1111,"context_line":""},{"line_number":1112,"context_line":"    def _audit_shard_container(self, broker):"},{"line_number":1113,"context_line":"        self._increment_stat(\u0027audit_shard\u0027, \u0027attempted\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9ec82109_8f780fb2","line":1110,"updated":"2021-09-16 21:11:40.000000000","message":"even using a timestamp of now, this is not sufficient to keep replicators from overwriting this - you could even get in a cycle that never settles where each shard replica independently discovers the new spi and everytime it tries to replicate gets kicked back","commit_id":"bae6aa04118edef5becf1215a16386fb5c8c694c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"672093cb7f9e0bd157189a06b99339dd3733b964","unresolved":true,"context_lines":[{"line_number":1116,"context_line":"                info[\u0027created_at\u0027],"},{"line_number":1117,"context_line":"                remote_info[\u0027put_timestamp\u0027],"},{"line_number":1118,"context_line":"                remote_info[\u0027delete_timestamp\u0027],"},{"line_number":1119,"context_line":"            )"},{"line_number":1120,"context_line":""},{"line_number":1121,"context_line":"    def _audit_shard_container(self, broker):"},{"line_number":1122,"context_line":"        self._increment_stat(\u0027audit_shard\u0027, \u0027attempted\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9295f66f_86803691","line":1119,"updated":"2021-09-17 15:56:46.000000000","message":"I think this order of operatoins is robust to a crash between set-spi and merge-ts\n\nIf we wanted to add some metadata that includes the broker-id and the local info timestamps \"just in case\" I\u0027d be totally fine with that - I expect draining and recreating sharded buckets to be uncommon; but when it does happen I\u0027m most glad to know it will be *correct*","commit_id":"b1c3b8976884d22aa18f98956a4f8b6a9d6bb48f"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9fcc24919dbdca2b81bdcd21a6963932279448e6","unresolved":true,"context_lines":[{"line_number":1108,"context_line":"            self.logger.info("},{"line_number":1109,"context_line":"                \"Migrating shard %s to storage_policy_index %d\","},{"line_number":1110,"context_line":"                broker.db_file, root_sp_index)"},{"line_number":1111,"context_line":"            # NOTE(clayg): I think this order of operatoins is robust to a"},{"line_number":1112,"context_line":"            # crash between set-spi and merge-ts."},{"line_number":1113,"context_line":"            # If we wanted to add some metadata that includes the broker-id"},{"line_number":1114,"context_line":"            # and the local info timestamps \"just in case\" I\u0027d be totally"}],"source_content_type":"text/x-python","patch_set":4,"id":"c38084bd_97dfc34f","line":1111,"range":{"start_line":1111,"start_character":49,"end_line":1111,"end_character":60},"updated":"2021-09-23 13:47:54.000000000","message":"nit: s/operatoins/operations/","commit_id":"a6a92b66232254b923ab0fb6563a74bc01bcad8d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9fcc24919dbdca2b81bdcd21a6963932279448e6","unresolved":true,"context_lines":[{"line_number":1162,"context_line":"                        own_shard_range_from_root \u003d shard_range"},{"line_number":1163,"context_line":"                        # Only potentially update storage_policy_index if"},{"line_number":1164,"context_line":"                        # we find the SR with the root."},{"line_number":1165,"context_line":"                        if root_resp:"},{"line_number":1166,"context_line":"                            self._update_storage_policy_index(broker,"},{"line_number":1167,"context_line":"                                                              root_resp)"},{"line_number":1168,"context_line":"                        break"}],"source_content_type":"text/x-python","patch_set":4,"id":"83aacaf4_19306397","line":1165,"range":{"start_line":1165,"start_character":24,"end_line":1165,"end_character":36},"updated":"2021-09-23 13:47:54.000000000","message":"this condition is always True \n\nif shard_ranges \u003d\u003d True -\u003e root_resp is not None","commit_id":"a6a92b66232254b923ab0fb6563a74bc01bcad8d"}],"test/probe/test_sharder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"c01f94af314d3018e4e345ae414aee80a25cde70","unresolved":true,"context_lines":[{"line_number":2277,"context_line":"        self.assert_container_listing(subset_obj_names)"},{"line_number":2278,"context_line":""},{"line_number":2279,"context_line":"        # running sharders will migrate the SPI and update the root stats"},{"line_number":2280,"context_line":"        self.sharders.once()"},{"line_number":2281,"context_line":"        self.assert_container_listing(subset_obj_names)"},{"line_number":2282,"context_line":"        self.assert_container_object_count(len(subset_obj_names))"},{"line_number":2283,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"d11bb695_baebfcf6","line":2280,"updated":"2021-09-16 21:11:40.000000000","message":"this isn\u0027t realistic, you\u0027d have to stop the replicators to get all the shards to update from the roots at the same time or else any older \"un-recreated\" shard\u0027s policy would trump during replication (which is a fast two way sync!)","commit_id":"bae6aa04118edef5becf1215a16386fb5c8c694c"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"69acddf030ef3b3cd6443f90abaa0179a35e6b37","unresolved":true,"context_lines":[{"line_number":2277,"context_line":"        self.assert_container_listing(subset_obj_names)"},{"line_number":2278,"context_line":""},{"line_number":2279,"context_line":"        # running sharders will migrate the SPI and update the root stats"},{"line_number":2280,"context_line":"        self.sharders.once()"},{"line_number":2281,"context_line":"        self.assert_container_listing(subset_obj_names)"},{"line_number":2282,"context_line":"        self.assert_container_object_count(len(subset_obj_names))"},{"line_number":2283,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ecd4e526_b183ea5a","line":2280,"in_reply_to":"d11bb695_baebfcf6","updated":"2021-09-16 23:45:40.000000000","message":"Ahh very good point. Just went and looked at the replication code around SPIs and yeah. ouch.","commit_id":"bae6aa04118edef5becf1215a16386fb5c8c694c"}]}