Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mod_bosh config in a map #3540

Merged
merged 8 commits into from
Feb 17, 2022
Merged

mod_bosh config in a map #3540

merged 8 commits into from
Feb 17, 2022

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Feb 11, 2022

Main changes:

  • Put mod_bosh options in a map with defaults.
  • Use mongoose_backend in mod_bosh. Make it global as the only backend (mnesia) uses Mnesia globally and the node_cleanup hook is global as well. Start it only once to avoid warnings for duplicated hooks.
  • Put backend in the config spec for mod_bosh and mod_auth_token even if there is only one backend to avoid setting it in multiple places (system metrics, backend module). Document it as well - for consistency and to simplify adding new custom backends.

Other changes:

  • Create a directory for helpers shared between small and big tests. Use it for default module config.
  • Make mod_bosh config match the docs (time units, positive vs. non-negative integers).
  • Fix mongoose_module_metrics to work with module opts in maps and use the defaults.
  • Reject BOSH connections to host types for which the module is not enabled.
  • Make BOSH tests more stable - the first test can fail under specific circumstances (coverage disabled, freshly started node).

@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #3540 (0ceb574) into master (7f04df9) will increase coverage by 0.00%.
The diff coverage is 93.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3540   +/-   ##
=======================================
  Coverage   81.08%   81.09%           
=======================================
  Files         419      420    +1     
  Lines       32284    32285    +1     
=======================================
+ Hits        26176    26180    +4     
+ Misses       6108     6105    -3     
Impacted Files Coverage Δ
src/gen_mod.erl 78.57% <ø> (ø)
src/mod_bosh_mnesia.erl 100.00% <ø> (ø)
src/mod_auth_token.erl 82.06% <50.00%> (+0.56%) ⬆️
src/mod_bosh.erl 95.62% <90.90%> (+1.25%) ⬆️
src/mod_auth_token_backend.erl 100.00% <100.00%> (ø)
src/mod_bosh_backend.erl 100.00% <100.00%> (ø)
src/mod_bosh_socket.erl 78.57% <100.00%> (ø)
src/system_metrics/mongoose_module_metrics.erl 100.00% <100.00%> (+16.66%) ⬆️
...stem_metrics/mongoose_system_metrics_collector.erl 93.81% <100.00%> (ø)
src/mongoose_tcp_listener.erl 76.59% <0.00%> (-4.26%) ⬇️
... and 7 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 7f04df9...0ceb574. Read the comment docs.

@chrzaszcz chrzaszcz force-pushed the mod-bosh-config-map branch 2 times, most recently from 3b240d3 to a8baf76 Compare February 16, 2022 07:47
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz marked this pull request as ready for review February 16, 2022 16:01
big_tests/tests/bosh_SUITE.erl Show resolved Hide resolved
src/mod_auth_token.erl Outdated Show resolved Hide resolved
@mongoose-im

This comment was marked as outdated.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Feb 16, 2022

small_tests_24 / small_tests / 978d117
Reports root / small


small_tests_23 / small_tests / 978d117
Reports root / small


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 978d117
Reports root/ big
OK: 2718 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 978d117
Reports root/ big
OK: 2718 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / 978d117
Reports root/ big
OK: 2701 / Failed: 0 / User-skipped: 256 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 978d117
Reports root/ big
OK: 2718 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / 978d117
Reports root/ big
OK: 1507 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 978d117
Reports root/ big
OK: 1507 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / 978d117
Reports root/ big
OK: 1548 / Failed: 0 / User-skipped: 349 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / 978d117
Reports root/ big
OK: 3092 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 978d117
Reports root/ big
OK: 3092 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / 978d117
Reports root/ big
OK: 3087 / Failed: 0 / User-skipped: 253 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 978d117
Reports root/ big
OK: 1841 / Failed: 0 / User-skipped: 365 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / 978d117
Reports root/ big
OK: 3092 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / 978d117
Reports root/ big
OK: 1687 / Failed: 0 / User-skipped: 364 / Auto-skipped: 0

@chrzaszcz chrzaszcz force-pushed the mod-bosh-config-map branch 2 times, most recently from c4ea8ab to bfd1f23 Compare February 16, 2022 17:08
- Fix broken 'catch' - the generated tuple would lead to a crash
- Get options from a map or a list
- Make it possible to use the defaults from the config spec
@mongoose-im

This comment was marked as outdated.

Make it global because:
- There is only one implementation, which stores all sessions in one table
- The node_cleanup hook is global
This way it is set only once for mongoose_backend and system metrics
Also:
- Use mod_bosh_backend
- Avoid duplicate hook registration, which caused startup warnings
- Set backend in the config spec even if there is only one
- Respond with host-unknown when mod_bosh is disabled for a host type

Motivation: set in only once, use it in system metrics
Also:
  - make it possible to reuse the same helpers in small and big tests
  - check backend for mod_bosh and mod_auth_token in tests
- Test the case when mod_bosh is disabled for a host type
- Handle the case when server sends stream features separately
  This happens when the 'accumulate' timeout (10 msec) passes.
  It does not happen on CI and happens locally only if the test
  is the first one (they are shuffled) and the server has just started.
  It is not a bug.
- Restore module config properly
This option can be useful when developing a custom backend.
It is also better not to have undocumented options.

Also: update time unit
@esl esl deleted a comment from mongoose-im Feb 16, 2022
@esl esl deleted a comment from mongoose-im Feb 16, 2022
@esl esl deleted a comment from mongoose-im Feb 16, 2022
@mongoose-im
Copy link
Collaborator

mongoose-im commented Feb 16, 2022

small_tests_24 / small_tests / 0ceb574
Reports root / small


small_tests_23 / small_tests / 0ceb574
Reports root / small


dynamic_domains_mysql_redis_24 / mysql_redis / 0ceb574
Reports root/ big
OK: 2701 / Failed: 0 / User-skipped: 256 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 0ceb574
Reports root/ big
OK: 2718 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 0ceb574
Reports root/ big
OK: 2718 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 0ceb574
Reports root/ big
OK: 2718 / Failed: 0 / User-skipped: 239 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / 0ceb574
Reports root/ big
OK: 1507 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 0ceb574
Reports root/ big
OK: 1507 / Failed: 0 / User-skipped: 390 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / 0ceb574
Reports root/ big
OK: 1548 / Failed: 0 / User-skipped: 349 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 0ceb574
Reports root/ big
OK: 3092 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / 0ceb574
Reports root/ big
OK: 3087 / Failed: 0 / User-skipped: 253 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 0ceb574
Reports root/ big
OK: 1841 / Failed: 0 / User-skipped: 365 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / 0ceb574
Reports root/ big
OK: 3092 / Failed: 0 / User-skipped: 248 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / 0ceb574
Reports root/ big
OK: 1687 / Failed: 0 / User-skipped: 364 / Auto-skipped: 0

@mongoose-im
Copy link
Collaborator

riak_mnesia_24 / riak_mnesia / bfd1f23
Reports root/ big
OK: 1687 / Failed: 0 / User-skipped: 364 / Auto-skipped: 0

Copy link
Contributor

@gustawlippa gustawlippa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! I like the backend added, and the common test helper especially.

@gustawlippa gustawlippa merged commit 6ca2443 into master Feb 17, 2022
@gustawlippa gustawlippa deleted the mod-bosh-config-map branch February 17, 2022 09:41
@Premwoik Premwoik added this to the 5.1.0 milestone May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants