[MadLittleMods]

Make it more clear how shared_extra_conf is combined in our Docker configuration scripts

Original context for why it was changed this way: matrix-org/synapse#14921 (comment)

Previously, this code made me question two things:

1. Do we actually use worker_config["shared_extra_conf"] in the templates?
   • At first glance, I couldn't see why we're updating shared_extra_conf here. It's not used in the worker.yaml.j2 template so all of this seemed a bit pointless.
   • Turns out, updating shared_extra_conf itself is pointless and it's being done as a convenient place to mix the objects to get things right in shared_config (confusing).
2. Does it actually do anything?
   • Because shared_config starts out as an empty object, my first glance made me think we we're just updating with an empty object and then just re-assigning. But because we're in a loop, we actually accumulate the shared_extra_conf from each worker.

I'm not sure whether I'm capturing my confusion well enough here but basically, this made me spend time trying to figure out what/why we're doing things this way and we can use a more clear pattern to accomplish the same thing.

---

This change is spawning from looking at the docker/configure_workers_and_start.py script in order to add a metrics listener (upcoming PR).

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.
• Code style is correct (run the linters)

[MadLittleMods]

I can't figure out why it matters that we don't want to overwrite existing keys here. The original context doesn't explain why either: matrix-org/synapse#14921 (comment)

synapse/docker/configure_workers_and_start.py

Lines 865 to 868 in f79acff

	# Update the shared config with any worker_type specific options. The first of a
	# given worker_type needs to stay assigned and not be replaced.
	worker_config["shared_extra_conf"].update(shared_config)
	shared_config = worker_config["shared_extra_conf"]

As far as I can tell, things would work just fine if we overwrote things.

For example, in this case, if we had multiple media workers, it doesn't matter if the first or last worker is the media_instance_running_background_jobs,

synapse/docker/configure_workers_and_start.py

Lines 127 to 131 in f79acff

	# The first configured media worker will run the media background jobs
	"shared_extra_conf": {
	"enable_media_repo": False,
	"media_instance_running_background_jobs": WORKER_PLACEHOLDER_NAME,
	},

(same story for all of the other worker types that define shared_extra_conf)

---

Am I missing something?

If not, we can update this to be even more standard.

If it's just for sanity sake (always use the first worker), that's a fine reason to document.

[anoadragon453]

I also don't see a reason for it. Let's simplify and check that CI still passes?

[MadLittleMods]

If it's just for sanity sake (always use the first worker), that's a fine reason to document.

I've just gone with this as it reads the easiest. Explaining the nuance the other way is clunky and ultimately doesn't matter.

[anoadragon453]

Thanks for the great documentation improvements!

[anoadragon453]

I also don't see a reason for it. Let's simplify and check that CI still passes?

[MadLittleMods]

For reference, this PR used to have this whole change in the diff.

But #19324 was merged first which introduced parts of it already.

Here is what this code used to look like:

synapse/docker/configure_workers_and_start.py

Lines 865 to 868 in 566670c

	# Update the shared config with any worker_type specific options. The first of a
	# given worker_type needs to stay assigned and not be replaced.
	worker_config["shared_extra_conf"].update(shared_config)
	shared_config = worker_config["shared_extra_conf"]

[MadLittleMods]

Thanks for the review @anoadragon453 🐡