Add all Stream Writer worker types to configure_workers_and_start.py

[realtyem]

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

There are a few missing types of workers that aren't included in the templating to generate configuration that is built into configure_workers_and_start.py for the Dockerfile-workers, specifically five of six kinds of stream writers. These other worker types are documented in the configuration manual. Let's add them to the scripting so if they ever need to be used, it's simple.
🔴 BIG WARNING: Typing and Receipts stream writers do not work correctly at this time as-is and fail Complement tests.

Removed the unnecessary gating from add_sharding_to_shared_config(), commit adb34bc, then rename function to process_sharding_and_stream_writers_for_shared_config as this gating is actually taken care of by worker type inside the subroutine. Unsupported worker types pass right through. A side effect of removing the gate was that a bug was possibly fixed in how pusher_instances and federation_senders_instances was processed.

Signed-off-by: Jason Little realtyem@gmail.com

[realtyem]

I welcome suggestions on what to rename add_sharding_to_shared_config() since it does a little bit more now(and probably should have been all along). Perhaps add_streamwriters_and_sharding_to_shared_config()? Feels kinda long, for a function name. OTOH, have you seen Windows function names? Win64DoThisAndThatAndThoseOthersAndThenResetAllSettingsToDefaultsWithoutAsking_45927()

@anoadragon453 Since you wrote the original function, I feel like yours is the input for this.

[realtyem]

If you will accept the testing from my own repo, which doesn't touch functionality of the core Synapse project, you can see example CI logs of the failing stream writers from before this fix here and the after here. Flakes are fun, but I think these are known. I'm still testing actual usage on my live server to make sure those passes on stream writers aren't a fluke.

[realtyem]

Strange occurrence in my Postgresql logs after live testing this.

2022-10-16 06:41:55.897 CDT [7116] ERROR:  could not serialize access due to concurrent delete
2022-10-16 06:41:55.897 CDT [7116] STATEMENT:
                        DELETE FROM stream_positions
                        WHERE
                            stream_name = 'account_data'
                            AND instance_name != ALL(ARRAY['account_data1'])

2022-10-16 05:53:23.135 CDT [6528] ERROR:  could not serialize access due to concurrent delete
2022-10-16 05:53:23.135 CDT [6528] STATEMENT:
                        DELETE FROM stream_positions
                        WHERE
                            stream_name = 'presence_stream'
                            AND instance_name != ALL(ARRAY['master'])

2022-10-16 06:52:49.281 CDT [276] ERROR:  could not serialize access due to concurrent delete
2022-10-16 06:52:49.281 CDT [276] STATEMENT:
                        DELETE FROM stream_positions
                        WHERE
                            stream_name = 'receipts'
                            AND instance_name != ALL(ARRAY['receipts1'])

Was spammed for several minutes. Is this the system resetting itself after worker reconfiguration? It did seem to stop after a while while it wasn't racing against itself.

Further exceptions looked like this in the synapse log:

********************************************************
 Error during initialisation:
    could not serialize access due to concurrent delete

 There may be more information in the logs.
********************************************************

  File "/usr/local/lib/python3.10/site-packages/synapse/storage/databases/main/media_repository.py", line 148, in __init__
    super().__init__(database, db_conn, hs)
  File "/usr/local/lib/python3.10/site-packages/synapse/storage/databases/main/media_repository.py", line 68, in __init__
    super().__init__(database, db_conn, hs)
  File "/usr/local/lib/python3.10/site-packages/synapse/storage/databases/main/metrics.py", line 68, in __init__
    super().__init__(database, db_conn, hs)
  File "/usr/local/lib/python3.10/site-packages/synapse/storage/databases/main/event_push_actions.py", line 247, in __init__
    super().__init__(database, db_conn, hs)
  File "/usr/local/lib/python3.10/site-packages/synapse/storage/databases/main/receipts.py", line 71, in __init__
    self._receipts_id_gen = MultiWriterIdGenerator(
  File "/usr/local/lib/python3.10/site-packages/synapse/storage/util/id_generators.py", line 368, in __init__
    self._load_current_ids(db_conn, tables)
  File "/usr/local/lib/python3.10/site-packages/synapse/storage/util/id_generators.py", line 393, in _load_current_ids
    cur.execute(sql, (self._stream_name, self._writers))
  File "/usr/local/lib/python3.10/site-packages/synapse/storage/database.py", line 388, in execute
    self._do_execute(self.txn.execute, sql, *args)
  File "/usr/local/lib/python3.10/site-packages/synapse/storage/database.py", line 436, in _do_execute
    return func(sql, *args, **kwargs)
psycopg2.errors.SerializationFailure: could not serialize access due to concurrent delete

Which I assume to be normal for this situation.

Final testing results on live: Everything appears to function(or at least not be a show stopper) except for the receipts worker type which did bring down the whole synapse server. This may be a red herring, as Complement tests still pass and may just be an issue with migrating a live server with existing database. Can anyone provide me with a way to check this?

[H-Shay]

This is still in draft, is it ready for review?

[realtyem]

The only reason I hadn't put it up as ready was because of a subroutine name. If you think it's ok otherwise, then I'll switch it.

…

On Tue, Oct 18, 2022, 2:50 PM Shay ***@***.***> wrote: This is still in draft, is it ready for review? — Reply to this email directly, view it on GitHub <#14197 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMCKHKML2TODRXVWCSGBSLWD35QXANCNFSM6AAAAAARGIYL74> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[H-Shay]

I think if you're ready for feedback go ahead and remove it from draft.

[reivilibre]

This looks good to me.

psycopg2.errors.SerializationFailure: could not serialize access due to concurrent delete

These issues are because the workers all start up very quickly at the same time and hit the database table at the same time, especially if you're using the complement fork launcher.
Probably this case should be handled with a retry loop with a short but random backoff — that is, if we can't figure out how to prevent the race outright.
(Maybe some explicit locking could help.)