Multi tenancy iq handlers #3118

DenysGonchar · 2021-05-14T22:10:03Z

this PR introduces iq handling for dynamic domains. here is the list of all changes:

introduction of mongoose_iq_handlers
enforcing map as type of packet_handler's extra field and extending it with host_type for dynamic domains
introducing mongoose_lazy_routing module that controls dynamic domains/subdomains routing and iq handlers registration.
splitting gen_iq_handler into 3 different modules:
- iq handler registration API
- iq component behaviour
- iq handlers' workers
introducing demo module

TODO: introduce unit testing for mongoose_lazy_routing module

codecov · 2021-05-14T22:27:41Z

Codecov Report

Merging #3118 (ca2fcc6) into master (8c597bc) will increase coverage by 0.01%.
The diff coverage is 74.03%.

@@            Coverage Diff             @@
##           master    #3118      +/-   ##
==========================================
+ Coverage   79.22%   79.24%   +0.01%     
==========================================
  Files         389      394       +5     
  Lines       31959    32139     +180     
==========================================
+ Hits        25321    25467     +146     
- Misses       6638     6672      +34

Impacted Files	Coverage Δ
src/ejabberd_sup.erl	`85.71% <ø> (ø)`
src/mod_dynamic_domains_test.erl	`0.00% <0.00%> (ø)`
src/mod_vcard.erl	`76.95% <ø> (ø)`
src/mongoose_hooks.erl	`92.85% <ø> (+0.54%)`	⬆️
src/muc_light/mod_muc_light.erl	`84.54% <ø> (ø)`
src/mongoose_iq_handler.erl	`61.11% <61.11%> (ø)`
src/mod_muc_iq.erl	`60.00% <62.50%> (-5.00%)`	⬇️
src/mongoose_iq_worker.erl	`72.72% <72.72%> (ø)`
src/gen_iq_handler.erl	`73.91% <77.27%> (+16.13%)`	⬆️
src/ejabberd_sm.erl	`83.61% <80.00%> (+0.90%)`	⬆️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c597bc...ca2fcc6. Read the comment docs.

…ts for mongoose_lazy_routing

not sure why iq handlers are registered asynchronously, but keeping that as is (in case that is done to resolve some deadlocks)

…subhosts

big_tests/tests/dynamic_domains_pm_SUITE.erl

arcusfelis

added some style comments, but good

big_tests/tests/dynamic_domains_pm_SUITE.erl

src/ejabberd_local.erl

src/gen_iq_handler.erl

chrzaszcz

Looks good and quite complicated at the same time... There are some gen_servers introduced, which can always become bottlenecks. Please double-check that all the calls really need to be synchronized this way.

chrzaszcz · 2021-05-18T13:30:58Z

src/mongoose_packet_handler.erl

+    %% KV pairs from the OldExtra map will remain unchanged, only
+    %% the new keys from Extra map will be added to the NewExtra map


Suggested change

%% KV pairs from the OldExtra map will remain unchanged, only

%% the new keys from Extra map will be added to the NewExtra map

%% KV pairs from the OldExtra map will remain unchanged,

%% only the new keys from Extra map will be added to the NewExtra map

I find it much more readable when all the lines of the block comment have more or less the same length.

I am trying to follow the ideas in https://sembr.org/ which really help with reading the comments. It's mostly for markup languages, but it helps me a lot with comments. I don't see a benefit of that stray "only" at the end of the sentence as it can be interpreted differently without seeing the next line (as an adverb instead of a conjunction).

src/mongoose_packet_handler.erl

src/mod_muc_iq.erl

src/domain/mongoose_subdomain_core.erl

src/gen_iq_component.erl

src/mongoose_router_dynamic_domains.erl

src/domain/mongoose_lazy_routing.erl

chrzaszcz · 2021-05-19T07:30:35Z

src/domain/mongoose_lazy_routing.erl

+            ejabberd_local:register_host(Domain),
+            ok;
+        false ->
+            %% we should never get here, but it's ok to just ignore this.


So if this gets called multiple times (because of a bug), we would silently ignore this fact.

chrzaszcz · 2021-05-19T07:33:46Z

big_tests/tests/dynamic_domains_pm_SUITE.erl

@@ -69,6 +74,59 @@ auth_domain_removal_is_triggered_on_hook(_Config) ->
    1 = rpc(mim(), meck, num_calls, [ejabberd_auth_dummy, remove_domain, Params]),
    rpc(mim(), meck, unload, [ejabberd_auth_dummy]).

+test_routing(Config) ->


Please split into 3 test cases.

big_tests/tests/dynamic_domains_pm_SUITE.erl

NelsonVides · 2021-05-19T10:57:39Z

I've seen that ejabberd_local has now its ets table as protected, why can't we do the same for ejabberd_sm' ets table? 🤔

DenysGonchar · 2021-05-19T11:07:35Z

I've seen that ejabberd_local has now its ets table as protected, why can't we do the same for ejabberd_sm' ets table? 🤔

it is protected. see http://erlang.org/doc/man/ets.html#new-2 :

protected

    The owner process can read and write to the table. Other processes can only read the table. This is the default setting for the access rights.

NelsonVides · 2021-05-19T11:59:29Z

I've seen that ejabberd_local has now its ets table as protected, why can't we do the same for ejabberd_sm' ets table? 🤔

it is protected. see http://erlang.org/doc/man/ets.html#new-2 :
protected

    The owner process can read and write to the table. Other processes can only read the table. This is the default setting for the access rights.

Ah, protected is the default, I see, thanks!

…ns_SUITE

arcusfelis

k

DenysGonchar added 3 commits May 14, 2021 15:10

fixing mod_muc_iq:unregister_iq_handler/2

d06751d

changing extra field of #packet_handler{} from any() to map()

e0560bf

extending subdomain packet handler extra fields with host_type

04dbcd2

DenysGonchar added the WIP 🚧 label May 14, 2021

reworking gen_iq_handler module

742aa04

DenysGonchar force-pushed the multi-tenancy-iq_handlers branch 2 times, most recently from 57fc23a to bdc1bc2 Compare May 17, 2021 13:04

DenysGonchar added 3 commits May 17, 2021 23:06

adding mongoose_lazy_routing module

c5ef3f4

adding new API for gen_iq_handler

37e39b9

implementing IQ handling API at mongoose_lazy_routing

b48c3e9

DenysGonchar force-pushed the multi-tenancy-iq_handlers branch from bdc1bc2 to b48c3e9 Compare May 18, 2021 00:01

adding mod_dynamic_domains_test demo module and basic integration tes…

25cd9c8

…ts for mongoose_lazy_routing

DenysGonchar force-pushed the multi-tenancy-iq_handlers branch from 7f5bc61 to 25cd9c8 Compare May 18, 2021 10:30

DenysGonchar added 3 commits May 18, 2021 13:04

resolving race condition during lazy registration of iq handlers.

bc36e6f

not sure why iq handlers are registered asynchronously, but keeping that as is (in case that is done to resolve some deadlocks)

cleaning up dynamic_domains_pm_SUITE:test_routing/1 test case

51f5168

removing dependency between mongoose_router_localdomain and mongoose_…

b48b6c2

…subhosts

DenysGonchar added the waiting-for-review label May 18, 2021

DenysGonchar requested review from chrzaszcz and arcusfelis May 18, 2021 12:42

DenysGonchar removed the WIP 🚧 label May 18, 2021

arcusfelis reviewed May 18, 2021

View reviewed changes

big_tests/tests/dynamic_domains_pm_SUITE.erl Outdated Show resolved Hide resolved

arcusfelis reviewed May 18, 2021

View reviewed changes

big_tests/tests/dynamic_domains_pm_SUITE.erl Outdated Show resolved Hide resolved

big_tests/tests/dynamic_domains_pm_SUITE.erl Outdated Show resolved Hide resolved

src/ejabberd_local.erl Show resolved Hide resolved

src/gen_iq_handler.erl Show resolved Hide resolved

chrzaszcz reviewed May 19, 2021

View reviewed changes

DenysGonchar added 2 commits May 19, 2021 17:53

changes according to the review comments

1ab85df

reworking dynamic_domains_pm_SUITE and renaming it into dynamic_domai…

ca2fcc6

…ns_SUITE

arcusfelis approved these changes May 19, 2021

View reviewed changes

arcusfelis merged commit eae2da0 into master May 19, 2021

arcusfelis deleted the multi-tenancy-iq_handlers branch May 19, 2021 19:16

Premwoik added this to the 5.0.0 milestone Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi tenancy iq handlers #3118

Multi tenancy iq handlers #3118

DenysGonchar commented May 14, 2021 •

edited

codecov bot commented May 14, 2021 •

edited

arcusfelis left a comment

chrzaszcz left a comment

chrzaszcz May 18, 2021

DenysGonchar May 19, 2021

chrzaszcz May 19, 2021 •

edited

chrzaszcz May 19, 2021

chrzaszcz May 19, 2021

NelsonVides commented May 19, 2021

DenysGonchar commented May 19, 2021

NelsonVides commented May 19, 2021

arcusfelis left a comment

		%% KV pairs from the OldExtra map will remain unchanged, only
		%% the new keys from Extra map will be added to the NewExtra map

Multi tenancy iq handlers #3118

Multi tenancy iq handlers #3118

Conversation

DenysGonchar commented May 14, 2021 • edited

codecov bot commented May 14, 2021 • edited

Codecov Report

arcusfelis left a comment

Choose a reason for hiding this comment

chrzaszcz left a comment

Choose a reason for hiding this comment

chrzaszcz May 18, 2021

Choose a reason for hiding this comment

DenysGonchar May 19, 2021

Choose a reason for hiding this comment

chrzaszcz May 19, 2021 • edited

Choose a reason for hiding this comment

chrzaszcz May 19, 2021

Choose a reason for hiding this comment

chrzaszcz May 19, 2021

Choose a reason for hiding this comment

NelsonVides commented May 19, 2021

DenysGonchar commented May 19, 2021

NelsonVides commented May 19, 2021

arcusfelis left a comment

Choose a reason for hiding this comment

DenysGonchar commented May 14, 2021 •

edited

codecov bot commented May 14, 2021 •

edited

chrzaszcz May 19, 2021 •

edited