Mongoose service #1792

bartekgorny · 2018-04-05T14:08:56Z

This PR addresses feature #18914

Introduces mongoose_service behaviour, with dependencies between services and modules requiring certain services.

There will be another PR with slightly modified interface.

codecov · 2018-04-06T10:26:58Z

Codecov Report

Merging #1792 into master will decrease coverage by 0.13%.
The diff coverage is 76.82%.

@@            Coverage Diff             @@
##           master    #1792      +/-   ##
==========================================
- Coverage   74.48%   74.35%   -0.14%     
==========================================
  Files         290      291       +1     
  Lines       26996    27061      +65     
==========================================
+ Hits        20109    20122      +13     
- Misses       6887     6939      +52

Impacted Files	Coverage Δ
src/service_admin_extra_stats.erl	`84.61% <ø> (ø)`
src/service_admin_extra_srg.erl	`0% <ø> (ø)`
src/service_admin_extra_last.erl	`66.66% <ø> (ø)`
src/service_admin_extra_stanza.erl	`96.66% <ø> (ø)`
src/service_admin_extra_roster.erl	`88.19% <ø> (ø)`
src/service_admin_extra.erl	`100% <ø> (ø)`
src/service_admin_extra_vcard.erl	`92.06% <ø> (ø)`
src/service_admin_extra_node.erl	`20% <ø> (ø)`
src/service_admin_extra_private.erl	`81.48% <ø> (ø)`
src/service_admin_extra_sessions.erl	`86.79% <ø> (ø)`
... and 25 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 497ede9...d9daa35. Read the comment docs.

fenek

Missing documentation
I'd transform logs into ?[level]("event='something_happened',...", [...]) format

fenek · 2018-04-09T14:55:07Z

test.disabled/ejabberd_tests/tests/mongoose_service_big_SUITE.erl

+%% See the License for the specific language governing permissions and
+%% limitations under the License.
+%%==============================================================================
+-module(mongoose_service_big_SUITE).


Duplicate in old directory.

fenek · 2018-04-09T23:26:35Z

src/mod_admin_extra.erl

@@ -27,9 +27,9 @@
 -module(mod_admin_extra).


I think it should be renamed, so it would be clear it's no longer a module. E.g. service_admin_extra or srv_admin_extra

fenek · 2018-04-09T23:27:15Z

big_tests/tests/mongoose_service_big_SUITE.erl

+%% See the License for the specific language governing permissions and
+%% limitations under the License.
+%%==============================================================================
+-module(mongoose_service_big_SUITE).


I'm not sure if we need such suite. If ejabberdctl_SUITE works, then admin extra as a service works. For mongoose_service alone there is a dedicated small test.

fenek · 2018-04-09T23:35:44Z

src/mongoose_service.erl

+
+-spec loaded_services_with_opts() -> [{service(), options()}].
+loaded_services_with_opts() ->
+    [Z || [Z] <- ets:match(?ETAB, '$1')].


ets:tab2list?

fenek · 2018-04-10T13:34:47Z

src/gen_mod.erl

@@ -115,6 +116,7 @@ start_module_for_host(Host, Module, Opts0) ->
    Opts = clear_opts(Module, Opts0),
    set_module_opts_mnesia(Host, Module, Opts),
    ets:insert(ejabberd_modules, #ejabberd_module{module_host = {Module, Host}, opts = Opts}),
+    lists:map(fun mongoose_service:assert_loaded/1, get_required_services(Host, Module, Opts)),


I'd put it inside try..catch - nicer error handling.

fenek · 2018-04-16T10:37:35Z

doc/advanced-configuration/Services.md

-### Module Description
-This module significantly extends `mongooseimctl` script capabilities. New functionalities appear as additional commands. For detailed description, just run `mongooseimctl` script with no parameters.
+Some functionalities in MongooseIM are provided by "services".
+A service is similar to a module, only it is started only globally, while a module can be global or host-specific.


Module may have global configuration.

fenek · 2018-04-16T10:40:25Z

doc/user-guide/MongooseIM-High-level-Architecture.md

+A wide range of options includes authentication, privacy, storage, backend integration and mobile optimisations.
+See '[Extension Modules](../advanced-configuration/Modules.md)' for a full list.
+
+Modules can be configured and started either globally (for all virutal hosts served by the instance) or individually for each host.


Modules can be configured and started either for all virtual hosts served by the instance or with individual configuration for only some of them.

fenek · 2018-04-16T10:41:03Z

doc/user-guide/MongooseIM-High-level-Architecture.md

+
+Modules can be configured and started either globally (for all virutal hosts served by the instance) or individually for each host.
+Modules may include dependencies on services and on other modules.
+If a module depends on other modules, required modules are started automatically.


... started automatically with a default configuration provided by dependent module.

fenek · 2018-04-16T10:47:05Z

doc/user-guide/MongooseIM-High-level-Architecture.md

+
+Some functionalities which are not related to virtual hosts but rather are provided for the whole instance or for modules started for various hosts are provided by "services".
+Services are configured globally launched on startup, before modules, so that dependencies are satisfied.
+A service can require other services to be operational; required services are started automatically.


Wat? TBH I haven't noticed it in the code. Why should service be started as a dependency (with empty config!) by another service but we deny the same for modules?

The service will not be started with empty config, it will be started with the config from the config file. Since services are singletond you cant give your own config at runtime. We do this for convenience, so that you can start a service without complete knowledge about the whole dependency tree.why we do not allow modules start required services lm not sure, it was decided on tech meeting.

Actually the main advantage of it is that you can define services in the config file in any order and they will resolve their dependencies themselves.

…is now started as a service

* single callback for module dependencies and requirements * unified vocabulary (no requirements, only deps) * fail module start if required services not loaded (a.k.a. no lazy service startup) * Extend error on circular services dependency

gadgetci · 2018-04-16T16:34:56Z

src/mongoose_service.erl

+    catch
+        Class:Reason ->
+            ets:delete(?ETAB, Service),
+            Template = "event=service_startup,status=error,service=~p,options=~p,class=~p,reason=~p~n~p",


According to Elvis:

Line 131 is too long: Template = "event=service_startup,status=error,service=~p,options=~p,class=~~p,reason=pn~~p",.

goddammit · 2018-04-17T08:34:20Z

doc/advanced-configuration/Services.md

-### Module Description
-This module significantly extends `mongooseimctl` script capabilities. New functionalities appear as additional commands. For detailed description, just run `mongooseimctl` script with no parameters.
+Some functionalities in MongooseIM are provided by "services".
+A service is similar to a module, only it is started only once with a global configuration, while a module is started for every virtual host and may have global or host-specific configuration.


A service is similar to a module, but while a module is started for every virtual host and may have global or host-specific configuration, a service is started only once with global configuration.

goddammit · 2018-04-17T08:35:16Z

doc/advanced-configuration/Services.md

+## Service list
+
+As of version 2.2, only one module is a "service provider".
+Over time, part of modules which are not host-specific will be refactored to be services.


Eventually the modules which are not host-specific will be refactored to be services.
?

goddammit · 2018-04-17T08:36:18Z

doc/user-guide/MongooseIM-High-level-Architecture.md

+
+Modules can be configured and started either for all virutal hosts served by the instance or with individual configuration for only some of them.
+Modules may include dependencies on services and on other modules.
+If a module depends on other modules, required modules are started automatically with configuration provided by the dependent module..


If a module depends on other modules, required modules are started automatically with configuration provided by the dependent module.

goddammit · 2018-04-17T08:39:26Z

doc/user-guide/MongooseIM-High-level-Architecture.md

+
+### Services
+
+Some functionalities which are not related to virtual hosts but rather are provided for the whole instance or for modules started for various hosts are provided by "services".


Services provide certain fuctionalities not specific to virtual hosts but rather applied to the whole instance or to modules started for various hosts.
?

goddammit · 2018-04-17T08:39:57Z

doc/user-guide/MongooseIM-High-level-Architecture.md

+### Services
+
+Some functionalities which are not related to virtual hosts but rather are provided for the whole instance or for modules started for various hosts are provided by "services".
+Services are configured globally launched on startup, before modules, so that dependencies are satisfied.


They are configured globally and launched on startup, before modules, so that dependencies are satisfied.

fenek force-pushed the mongoose-service branch from b3e2b56 to 53ed418 Compare April 10, 2018 08:25

bartekgorny mentioned this pull request Apr 10, 2018

Mongoose service+ #1794

Merged

fenek requested changes Apr 10, 2018

View reviewed changes

fenek added the WIP 🚧 label Apr 10, 2018

fenek added this to the 3.0.0 milestone Apr 10, 2018

bartekgorny force-pushed the mongoose-service branch 3 times, most recently from 9b74608 to 668054e Compare April 12, 2018 15:19

fenek requested changes Apr 16, 2018

View reviewed changes

bartekgorny and others added 14 commits April 16, 2018 18:34

fixed app name

266ff18

first version of service module, and test

3f1b1a5

Starting services from ejabberd.cfg; big test.

1c44064

dependency management, starting the whole tree

9c46856

renamed deps to requirements

50dcfc5

service requirements for modules

bb9ea81

big test

01db1d2

fix ejabberdctl test - no use of trying to start mod_admin_extra, it …

13658a7

…is now started as a service

requested and suggested changes

b5064fb

[skip ci] docs for services

9bca51c

bugfixes

240b46a

fixed documentation

d241039

do not start service if not present in configuration file

2437ace

bartekgorny force-pushed the mongoose-service branch from 1040fe7 to 2437ace Compare April 16, 2018 16:34

gadgetci reviewed Apr 16, 2018

View reviewed changes

esl deleted a comment from gadgetci Apr 16, 2018

fenek approved these changes Apr 16, 2018

View reviewed changes

goddammit reviewed Apr 17, 2018

View reviewed changes

[skip ci] spell- and style-check

d9daa35

fenek merged commit 4f76cbc into master Apr 17, 2018

fenek deleted the mongoose-service branch April 17, 2018 09:46

michalwski mentioned this pull request Nov 6, 2018

Host-indenependent modules disappear when hosts config is changed #1679

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mongoose service #1792

Mongoose service #1792

bartekgorny commented Apr 5, 2018

codecov bot commented Apr 6, 2018 •

edited

Loading

fenek left a comment

fenek Apr 9, 2018

fenek Apr 9, 2018

fenek Apr 9, 2018

fenek Apr 9, 2018

fenek Apr 10, 2018

fenek Apr 16, 2018

fenek Apr 16, 2018

fenek Apr 16, 2018

fenek Apr 16, 2018

bartekgorny Apr 16, 2018

bartekgorny Apr 16, 2018

gadgetci Apr 16, 2018

goddammit Apr 17, 2018

goddammit Apr 17, 2018

goddammit Apr 17, 2018

goddammit Apr 17, 2018

goddammit Apr 17, 2018


		### Services

		Some functionalities which are not related to virtual hosts but rather are provided for the whole instance or for modules started for various hosts are provided by "services".

Mongoose service #1792

Mongoose service #1792

Conversation

bartekgorny commented Apr 5, 2018

codecov bot commented Apr 6, 2018 • edited Loading

Codecov Report

fenek 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

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

codecov bot commented Apr 6, 2018 •

edited

Loading