galley: Send on-user-deleted-conversations backend notification through RabbitMQ #3333

akshaymankar · 2023-05-31T12:34:57Z

https://wearezeta.atlassian.net/browse/WPB-200

Also includes some fixups for #3276 (all brig related changes are fixups)

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

…gh RabbitMQ

It was removed in #3276

elland · 2023-06-01T11:40:17Z

services/galley/src/Galley/Intra/BackendNotificationQueue.hs

@@ -0,0 +1,62 @@
+{-# LANGUAGE NumericUnderscores #-}


This is in the cabal file now, right?

fisx

LGTM! just nit-picks.

fisx · 2023-06-02T08:45:54Z

charts/galley/values.yaml

@@ -23,6 +23,11 @@ config:
    host: aws-cassandra
    replicaCount: 3
  enableFederation: false # keep enableFederation default in sync with brig and cargohold chart's config.enableFederation as well as wire-server chart's tags.federation
+  # Not used if enableFederation is false


Suggested change

# Not used if enableFederation is false

# It is an error to set this if enableFederation is false

It is not an error to set the helm value, it just gets ignored. If we want to make it an error it would require us to remove this from here and put it in some docs. Which would make it less discoverable.

fisx · 2023-06-02T08:50:02Z

services/galley/src/Galley/API/Internal.hs

-    leaveRemoteConversations cids = do
+    leaveRemoteConversations cids =


this is creating a lot of noise, we're adding and removing redundant dos at about the same frequency.

fisx · 2023-06-02T09:02:24Z

services/galley/test/integration/API.hs

-      Right udcnD <- pure . eitherDecode . frBody $ dReq
-      sort (fromRange (F.udcvConversations udcnD)) @?= sort [convD1]
-      F.udcvUser udcnD @?= qUnqualified alexDel
+      assertEqual ("expect exactly 4 federated requests in : " <> show fedRequests) 4 (length fedRequests)


ok, the federated calls don't happen here any more, so we can't test for these events. but should/can we test for something else?

related: a half-solution to the MakesFederatedCall tolerance for removing calls without removing the constraint could be that we add another kind of constraint PushesFederatedNotifications. then we would get a type error if we forget to add the new constraint, and that would help us remember to remove the old one. not sure that's worth it, though.

ok, the federated calls don't happen here any more, so we can't test for these events. but should/can we test for something else?

This functionality is already tested in federation end-to-end tests. If we really want to do something we'd have to create a rabbitmq vhost (its like a namespace) and pass that to our test and then we can assert that there is something in the queue. But honestly, seems a bit redundant given there are other test. I am making sure that I don't delete tests without making sure that there are other tests.

related: a half-solution to the MakesFederatedCall tolerance for removing calls without removing the constraint could be that we add another kind of constraint PushesFederatedNotifications. then we would get a type error if we forget to add the new constraint, and that would help us remember to remove the old one. not sure that's worth it, though.

I thought the point behind adding MakesFederatedCall was to ensure that possibility of federation errors shows up in swagger. If we fail to connect to RabbitMQ, it is just a 500 because the backend is not well. Do you think it would help swagger somehow that we add this new annotation? Do the clients need to care that this gets send via RabbitMQ?

fisx · 2023-06-02T09:04:25Z

services/galley/src/Galley/Env.hs

@@ -104,3 +108,15 @@ currentFanoutLimit o = do
  let optFanoutLimit = fromIntegral . fromRange $ fromMaybe defFanoutLimit (o ^. (optSettings . setMaxFanoutSize))
  let maxTeamSize = fromIntegral (o ^. (optSettings . setMaxTeamSize))
  unsafeRange (min maxTeamSize optFanoutLimit)
+
+mkRabbitMqChannel :: Logger -> Opts -> IO (Maybe (MVar Q.Channel))
+mkRabbitMqChannel l (view optRabbitmq -> Just RabbitMqOpts {..}) = do


should this go to wire-api? that would also pull a few other changes there, and clean up the arguments (Opts is not available in wire-api, but RabbitMqOpts should be).

I think wire-api is the wrong place to put it as this has nothing to do with the API. I was thinking of shoving it in the exteded package, but forgot about it. Let's do it in the next PR?

fisx · 2023-06-02T09:09:42Z

services/galley/src/Galley/Intra/BackendNotificationQueue.hs

+      Right <$> recovering policy handlers (const $ go ownDomain chanVar)
+  where
+    logError willRetry (SomeException e) status = do
+      Log.err $


This is something that may well happen every now and then without being an error. eg., if only one backend-pusher is running and needs to be restarted, or during startup. Should the log level be info, and only error once the retry count has been exhausted?

This can only happen when RabbitMQ is down or unreachable. So, this is definitely an error.

…gh RabbitMQ (#3333) * galley: Send on-user-deleted-conversations backend notification through RabbitMQ * charts/galley: Support configuring rabbitmq * brig: Remove unnecessary annotation for on-user-deleted-connections It was removed in #3276 * brig: validate options like galley does

akshaymankar added 3 commits May 31, 2023 14:23

galley: Send on-user-deleted-conversations backend notification throu…

422f94d

…gh RabbitMQ

charts/galley: Support configuring rabbitmq

ae42981

Remove dead code

893cb06

akshaymankar force-pushed the rabbitmq-galley branch from fdbe21f to 893cb06 Compare May 31, 2023 13:22

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label May 31, 2023

akshaymankar added 5 commits May 31, 2023 15:28

brig: Remove unnecessary annotation for on-user-deleted-connections

b3f39f4

It was removed in #3276

k8s-integration-setup: Give rabbitmq creds to galley

a532de4

brig: validate options like galley does

95ac09c

charts/galley: More RABBITMQ creds

f8b4624

Changelog

620ecd6

akshaymankar marked this pull request as ready for review May 31, 2023 15:09

akshaymankar requested review from elland, smatting and fisx May 31, 2023 15:12

elland reviewed Jun 1, 2023

View reviewed changes

fisx approved these changes Jun 2, 2023

View reviewed changes

akshaymankar added 2 commits June 5, 2023 10:36

brig-integration: Properly disable federation

a7c1fe1

galley-integration: Disbale federation properly

b4ee4e0

akshaymankar merged commit 2423d2e into develop Jun 5, 2023

akshaymankar deleted the rabbitmq-galley branch June 5, 2023 10:40

akshaymankar mentioned this pull request Jun 8, 2023

Extract RabbitMQOpts and the function to create channel in extended #3343

Merged

2 tasks

zebot mentioned this pull request Aug 11, 2023

Release 2023-08-11 - (expected chart version 4.36.0) #3493

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

galley: Send on-user-deleted-conversations backend notification through RabbitMQ #3333

galley: Send on-user-deleted-conversations backend notification through RabbitMQ #3333

akshaymankar commented May 31, 2023 •

edited

Loading

elland Jun 1, 2023

fisx left a comment

fisx Jun 2, 2023

akshaymankar Jun 5, 2023

fisx Jun 2, 2023

fisx Jun 2, 2023

akshaymankar Jun 5, 2023

akshaymankar Jun 5, 2023

fisx Jun 2, 2023

akshaymankar Jun 5, 2023

fisx Jun 2, 2023

akshaymankar Jun 5, 2023

	# Not used if enableFederation is false
	# It is an error to set this if enableFederation is false

		leaveRemoteConversations cids = do
		leaveRemoteConversations cids =

galley: Send on-user-deleted-conversations backend notification through RabbitMQ #3333

galley: Send on-user-deleted-conversations backend notification through RabbitMQ #3333

Conversation

akshaymankar commented May 31, 2023 • edited Loading

Checklist

Choose a reason for hiding this comment

fisx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshaymankar commented May 31, 2023 •

edited

Loading