feat(ex/notifications/notification_server): send notification to all connected Skate instances

[firestack]

To be able to synchronize multiple Skate instances in production, we need to setup
distributed Elixir.

Following the way https://github.com/mbta/ride_along did it, this uses DNSCluster
to find the other Skate instances, and if the variable is not present, it does nothing
so this is safe to deploy to production before the corresponding DevOps PR.

Depends on:

• fix: update tzdata  #2841
• fix: split up build and deploy steps #2838

[firestack]

I uhh, didn't mean to put this up for review just yet, as I was looking into if we could add tests. But we could merge without tests because I'm unsure about writing tests for distributed Elixir, I've been looking at https://elixirschool.com/en/lessons/advanced/otp_distribution#conditionally-excluding-tests-with-tags-11 but am still not certain how to do so.

[hannahpurcell]

Ok! Iiiiii think this makes sense to me and can't think of any feedback, so let's see it in action?

[paulswartz]

suggestion: you shouldn't need Node.spawn here. You can target the named server on another node with {process_name, node}: https://github.com/mbta/ride_along/blob/1d393329af28c00f080d37b730b1ad725e70a59d/lib/ride_along/rider_notifier.ex#L60

server = Keyword.get(options, :server, default_name())
for node <- Node.list() do
  GenServer.cast({server, node}, {:detour_activated_notification, notification_id})
end

[firestack]

Awesome, that works!
https://github.com/mbta/skate/compare/c9cf2debf1e309c4140e4b43753b2d8b26356a97..37026f15ff15bba582f0b85b6ad6e158bba2142d

If you have any thoughts on how to automatically test this, let me know?
I was looking at https://elixirschool.com/en/lessons/advanced/otp_distribution#conditionally-excluding-tests-with-tags-11 but that didn't mention anything about automatically setting up a second test instance, and when I tried to do so, it seemed like I'd have to run a second real instance of Skate for the tests to run against.

[paulswartz]

I don't have any good answers for this, unfortunately. For RideAlong, I tested the code paths manually and tried to keep them simple.

If you don't have something already, you could test calling the cast and making sure is has the right effect?

[firestack]

If you have any thoughts on how to automatically test this, let me know?

I don't have any good answers for this, unfortunately. For RideAlong, I tested the code paths manually and tried to keep them simple.

This is what I've been doing locally

PORT=4001 iex \
	--sname 1@localhost \
	--cookie test \
	-S mix phx.server

iex \
	--sname 2@localhost \
	--cookie test \
	-S mix phx.server

And then run this on 2@localhost shell

Node.connect(:"1@localhost")

(which means I've not been able to test DNSCluster, because it gets mad when I don't run it in release mode, as well as I don't think I can put a socket address (IP + Port) into what would be returned from DNS for localhost)

extra

or of course the corresponding call for the other server

Node.connect(:"2@localhost")

And then I open a browser window to both servers and create and activate a detour and confirm that the other browser window on the other server.

[firestack]

Oh hmmm, this will always return default_name() because options isn't passed from the caller on ln#111. We don't store the server name in the state, and we don't technically know the name of the server on the other node, we only know it because we haven't used a different name, but I think this would fail in tests?

Should this just be removed for now, or plumb the value through the original call to detour_activated or seomthing else?

I had originally thought I needed a "public" "caller function" (Something that calls GenServer.cast like detour_activated, and I then realized I didn't), so this is technically leftover code.

[paulswartz]

It needs to know the name of the server somehow, but there are bunch of ways:

• require a static name
• store the name in app config
• store the name in the GenServer state

[firestack]

Since ideally we'd move to using Phoenix Channel PubSub anyway, I'll put it in the app config, since then tests could change the value.

[firestack]

Changed my mind, I think this is better https://github.com/mbta/skate/compare/37026f15ff15bba582f0b85b6ad6e158bba2142d..d4601dcbf421efc6fe8097d8cda8f9b112739755

[paulswartz]

Code looks good (modulo the formatting issue) but I think it'd be good to put it up in one of the dev environments as well to make sure it works before merging.

[firestack]

Code looks good (modulo the formatting issue) but I think it'd be good to put it up in one of the dev environments as well to make sure it works before merging.

We don't currently have multiple instances in our dev envs, maybe it's time to change that with this change?

[firestack]

Checked dev-blue and confirmed no changes to the functionality of sending notifications, but I've not been able to test and confirm (in a deployment environment using DNSCluster) that multiple instances work.

[hannahpurcell]

👍