[WPB 3796] Fix background worker for federation policy allowAll #3526

fisx · 2023-08-22T12:28:52Z

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

Checklist

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

services/background-worker/src/Wire/BackgroundWorker/Env.hs

(To pull remotes from rabbitMQ, allowing us to quit talking to brig about this.)

(This was only needed to get federation remote configs, but we get those from rabbitMQ now, and without bugs in allowAll.)

…round-worker-for-allowAll-fed-policy

akshaymankar · 2023-08-24T07:34:31Z

services/background-worker/src/Wire/BackendNotificationPusher.hs

-        ensureConsumers consumersRef chan $ domain <$> chanRemotes.remotes
+        remoteDomains <- getRemoteDomains
+        ensureConsumers consumers chan remoteDomains
+        threadDelay (round $ 1_000_000 * fromMaybe 60 env.backendNotificationsConfig.remotesRefreshInterval)


Why is this a Maybe, we can put default values in all the places, i.e. helm chart and integration test configs? IMO this is unnecessary complexity.

akshaymankar · 2023-08-24T07:36:47Z

services/background-worker/src/Wire/BackgroundWorker/Options.hs

-    pushBackoffMaxWait :: Int
+    pushBackoffMaxWait :: Int,
+    -- | Number of seconds between two calls to `getRemoteDomains`.
+    remotesRefreshInterval :: Maybe Double


Instead of using Double can we just take number of milliseconds? We won't need rounding like this. I think we can safely assume people running wire are capable of understanding different units of time than seconds.
This also makes it more in line with the config one line above.

i get the last point, but i just think doubles are more ergonomic. anyway i'll align it with the rest.

(from before this PR)

akshaymankar · 2023-08-24T12:22:24Z

changelog.d/0-release-notes/WPB-3796

@@ -0,0 +1,5 @@
+New config value `background-worker.backendNotificationPusher.remotesRefreshIntervalMs`.  If in doubt, set to 30000 ms.  (This controls the delay between backend-to-backend notification queue update pulls from rabbitMQ to background-worker.)
+
+Remote federation domains in the config file are no longer supported (deprecated since Charts...).  Before deploying this release, make sure you have completed the migration steps from [the CHANGELOG of that release](...).


There was never a release which supported this, it came and went away between releases. So, this line is not necessary.

incorrect. we released this last week or so.

akshaymankar · 2023-08-24T12:23:34Z

changelog.d/0-release-notes/WPB-3796

+
+Remote federation domains in the config file are no longer supported (deprecated since Charts...).  Before deploying this release, make sure you have completed the migration steps from [the CHANGELOG of that release](...).
+
+If you have enabled federation and therefore rabbitMQ before this upgrade, you may need to remove all queues called `backend-notifications.delete-federation` manually, or this release won't start.


Suggested change

If you have enabled federation and therefore rabbitMQ before this upgrade, you may need to remove all queues called `backend-notifications.delete-federation` manually, or this release won't start.

If you have enabled federation and therefore rabbitMQ before this upgrade, you should remove the queue called `backend-notifications.delete-federation` manually.

I think the release still starts, just keeps printing warning logs, if not something is not right.

the thing that fails is the line below the one where the queue is created: it chokes on the fact that the queue has an invalid domain name and gets trapped in a loop, i think. but you are right, my wording was inaccurate.

akshaymankar · 2023-08-24T12:24:13Z

changelog.d/3-bug-fixes/WPB-3796

@@ -0,0 +1 @@
+[background-worker] Get federation remote domains from rabbitMQ, not from brig (this fixes a bug in federation policy `allowAll`)


There was no release made with this bug, so you can also just drop this changelog.

same as above.

akshaymankar · 2023-08-24T12:25:05Z

charts/background-worker/values.yaml

@@ -26,6 +26,7 @@ config:
  backendNotificationPusher:
    pushBackoffMinWait: 10000 # in microseconds, so 10ms
    pushBackoffMaxWait: 300000000 # microseconds, so 300s
+    remotesRefreshIntervalMs: 30000


Adding Ms here has two problems:

Capital M means Mega not milli or micro.

The things above do not have this suffix, so it is inconsistent.

sigh

i really don't like the way we do config files. very brittle and fragile at the same time.

i'll remove the Ms.

akshaymankar · 2023-08-24T12:27:44Z

libs/wire-api-federation/src/Wire/API/Federation/BackendNotifications.hs

+-- | FUTUREWORK: This really doesn't belong here in a module about pushing backend-to-backend
+-- notifications.  Maybe "Wire.API.Federation.Defederate"?  (Same for `ensureTLQueue`.)


Why not create the module right now?

akshaymankar · 2023-08-24T12:28:33Z

libs/wire-api-federation/src/Wire/API/Federation/BackendNotifications.hs

-ensureQueue :: Q.Channel -> Text -> IO ()
-ensureQueue chan queue = do
+ensureTLQueue :: Q.Channel -> Text -> IO ()
+ensureTLQueue chan queue = do


What does TL stand for?

"top level". not great, yes.

do you have a suggestion what to use for a name?

i'll think of something...

This reverts commit 3ac378b.

…ound-worker-for-allowAll-fed-policy' into WPB-3796/fix-background-worker-for-allowAll-fed-policy

…round-worker-for-allowAll-fed-policy

(for some reason, remotes listed in config files don't do it any more. stealing this fix from #3526)

elland · 2023-09-27T07:55:45Z

charts/brig/templates/tests/configmap.yaml

@@ -58,6 +58,8 @@ data:
      cert: /etc/wire/integration-secrets/provider-publiccert.pem
      botHost: https://brig-integration

+    originDomain: 612c6a18-4e3e-11ee-a205-97507f092421.example.com


Having this hard-coded like this raises an eyebrow. Why is this not dynamic?

elland · 2023-09-27T07:56:17Z

charts/integration/templates/integration-integration.yaml

@@ -207,3 +207,7 @@ spec:
        secretKeyRef:
          name: brig
          key: rabbitmqPassword
+    - name: TEST_INCLUDE


Should probably remove this before merging? :)

fisx · 2023-09-27T08:22:00Z

@akshaymankar this can be closed, right? i'll close it! #3588

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

akshaymankar reviewed Aug 22, 2023

View reviewed changes

services/background-worker/src/Wire/BackgroundWorker/Env.hs Outdated Show resolved Hide resolved

fisx force-pushed the WPB-3796/fix-background-worker-for-allowAll-fed-policy branch from 189af78 to f524514 Compare August 22, 2023 13:52

fisx requested a review from akshaymankar August 22, 2023 13:52

fisx marked this pull request as ready for review August 22, 2023 13:52

fisx added 7 commits August 23, 2023 15:33

nit-pick.

9823fd0

nit-pick.

a347a75

Call getRemoteDomains in a loop (again).

88f202f

(To pull remotes from rabbitMQ, allowing us to quit talking to brig about this.)

Remove dead code.

8752761

Don't call brig from background-worker.

3d4ffc1

(This was only needed to get federation remote configs, but we get those from rabbitMQ now, and without bugs in allowAll.)

Changelog.

3998753

make sanitize-pr

06e2f3f

fisx force-pushed the WPB-3796/fix-background-worker-for-allowAll-fed-policy branch from f524514 to 06e2f3f Compare August 23, 2023 13:34

fisx changed the title ~~[WPB 3796] Fix background worker for allow all fed policy~~ [WPB 3796] Fix background worker for federation policy allowAll Aug 23, 2023

Merge remote-tracking branch 'origin/develop' into WPB-3796/fix-backg…

de782bf

…round-worker-for-allowAll-fed-policy

akshaymankar reviewed Aug 24, 2023

View reviewed changes

fisx added 7 commits August 24, 2023 11:43

Make config value required, make it milliseconds and Int.

90477ca

rename.

21c5770

Fixups

ff59df1

Fix bug about ensuring rabbitMQ queues and namespaces.

3e2a4c3

(from before this PR)

Fix integration tests.

07042e7

FUTUREWORK.

7265f2d

TODO

cc820b8

akshaymankar reviewed Aug 24, 2023

View reviewed changes

fisx added 6 commits August 25, 2023 09:42

Move BackgroundNotifications module to a more suitlable place.

3252b16

Move BackgroundNotifications module (cleanup).

a6230d3

Clean up ensureQueue mess.

de0cabd

Cleanup remotesRefreshInterval unit inconsistency.

1277282

make sanitize-pr

883c063

Remove seemless-migration-to-database-remotes hack.

f76a1c6

fisx and others added 23 commits September 6, 2023 14:39

Fix: AllowAll has default search policy FullSearch.

3ac378b

Revert "Fix: AllowAll has default search policy FullSearch."

29e3429

This reverts commit 3ac378b.

Fix federator integration tests. All green now?

b246036

hi ci

d47e4df

Fixup

f32375d

Merge remote-tracking branch 'refs/remotes/origin/WPB-3796/fix-backgr…

00fd31a

…ound-worker-for-allowAll-fed-policy' into WPB-3796/fix-background-worker-for-allowAll-fed-policy

Merge remote-tracking branch 'origin/develop' into WPB-3796/fix-backg…

67b7b2e

…round-worker-for-allowAll-fed-policy

Fixup

2a86f02

hi ci

b0f3e4d

Re-align brig-integration config file on ci.

4f246bf

Increase time to wait for events in /integration.

4d6e6da

Fixup?

06e21e2

Merge remote-tracking branch 'origin/develop' into WPB-3796/fix-backg…

f2cf937

…round-worker-for-allowAll-fed-policy

Fixup!

a6927ba

*sigh*

7c8bf45

?!

975f142

...

5f89f96

Merge remote-tracking branch 'origin/develop' into WPB-3796/fix-backg…

3bbcf4d

…round-worker-for-allowAll-fed-policy

Makefile: clean up federation_remotes before make ci-fast.

23b7130

Fix one more.

4c34668

Nit-pick.

5f74ad8

...

e65ef0b

stash

43dd731

fisx added a commit that referenced this pull request Sep 15, 2023

Fix tests.

4d2b605

(for some reason, remotes listed in config files don't do it any more. stealing this fix from #3526)

akshaymankar mentioned this pull request Sep 25, 2023

Fix federationStrategy 'allowAll' #3588

Merged

2 tasks

elland reviewed Sep 27, 2023

View reviewed changes

fisx closed this Sep 27, 2023

echoes-hq bot added the echoes: technical-debt Changes intended at mitigating risks label Jul 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WPB 3796] Fix background worker for federation policy allowAll #3526

[WPB 3796] Fix background worker for federation policy allowAll #3526

fisx commented Aug 22, 2023 •

edited

Loading

akshaymankar Aug 24, 2023

akshaymankar Aug 24, 2023

fisx Aug 24, 2023

akshaymankar Aug 24, 2023

fisx Aug 24, 2023

akshaymankar Aug 24, 2023

fisx Aug 24, 2023

akshaymankar Aug 24, 2023

fisx Aug 24, 2023

akshaymankar Aug 24, 2023 •

edited

Loading

fisx Aug 24, 2023

akshaymankar Aug 24, 2023

akshaymankar Aug 24, 2023

fisx Aug 24, 2023

elland Sep 27, 2023

elland Sep 27, 2023

fisx commented Sep 27, 2023

		@@ -0,0 +1,5 @@
		New config value `background-worker.backendNotificationPusher.remotesRefreshIntervalMs`. If in doubt, set to 30000 ms. (This controls the delay between backend-to-backend notification queue update pulls from rabbitMQ to background-worker.)

		Remote federation domains in the config file are no longer supported (deprecated since Charts...). Before deploying this release, make sure you have completed the migration steps from [the CHANGELOG of that release](...).


		Remote federation domains in the config file are no longer supported (deprecated since Charts...). Before deploying this release, make sure you have completed the migration steps from [the CHANGELOG of that release](...).

		If you have enabled federation and therefore rabbitMQ before this upgrade, you may need to remove all queues called `backend-notifications.delete-federation` manually, or this release won't start.

		@@ -0,0 +1 @@
		[background-worker] Get federation remote domains from rabbitMQ, not from brig (this fixes a bug in federation policy `allowAll`)

		-- \| FUTUREWORK: This really doesn't belong here in a module about pushing backend-to-backend
		-- notifications. Maybe "Wire.API.Federation.Defederate"? (Same for `ensureTLQueue`.)

[WPB 3796] Fix background worker for federation policy allowAll #3526

[WPB 3796] Fix background worker for federation policy allowAll #3526

Conversation

fisx commented Aug 22, 2023 • edited Loading

Checklist

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 Aug 24, 2023 • edited Loading

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

fisx commented Sep 27, 2023

fisx commented Aug 22, 2023 •

edited

Loading

akshaymankar Aug 24, 2023 •

edited

Loading