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

Return a proper type from mod_muc:node_cleanup_for_host_type/3 #4122

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

arcusfelis
Copy link
Contributor

This PR addresses MIM-2039 - Process crashes when another node stops during the upgrade.

Error:

when=2023-09-11T12:12:54.123796+00:00 level=error reason="{{case_clause,#{}},[{gen_hook,run_hook,4,[{file,\"/home/circleci/project/src/gen_hook.erl\"},{line,237}]},{mongoose_hooks,run_fold,4,[{file,\"/home/circleci/project/src/hooks/mongoose_hooks.erl\"},{line,1388}]},{mongoose_cleaner,'-run_node_cleanup/1-lc$^0/1-0-',2,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,92}]},{mongoose_cleaner,'-run_node_cleanup/1-fun-1-',1,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,92}]},{timer,tc,1,[{file,\"timer.erl\"},{line,235}]},{mongoose_cleaner,run_node_cleanup,1,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,90}]},{global,trans,4,[{file,\"global.erl\"},{line,477}]},{mongoose_cleaner,cleanup_modules,1,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,79}]}]}" pid=<0.652.0> at=gen_server:error_info/8:1402 state={state} name=mongoose_cleaner log= last_message={nodedown,'mongooseim@mongooseim-0.mongooseim.default.svc.cluster.local'} label={gen_server,terminate} client_info=undefined
when=2023-09-11T12:12:54.125557+00:00 level=error pid=<0.652.0> at=proc_lib:crash_report/4:539 report="[[{initial_call,{mongoose_cleaner,init,['Argument__1']}},{pid,<0.652.0>},{registered_name,mongoose_cleaner},{error_info,{error,{case_clause,#{}},[{gen_hook,run_hook,4,[{file,\"/home/circleci/project/src/gen_hook.erl\"},{line,237}]},{mongoose_hooks,run_fold,4,[{file,\"/home/circleci/project/src/hooks/mongoose_hooks.erl\"},{line,1388}]},{mongoose_cleaner,'-run_node_cleanup/1-lc$^0/1-0-',2,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,92}]},{mongoose_cleaner,'-run_node_cleanup/1-fun-1-',1,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,92}]},{timer,tc,1,[{file,\"timer.erl\"},{line,235}]},{mongoose_cleaner,run_node_cleanup,1,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,90}]},{global,trans,4,[{file,\"global.erl\"},{line,477}]},{mongoose_cleaner,cleanup_modules,1,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,79}]}]}},{ancestors,[ejabberd_sup,<0.333.0>]},{message_queue_len,0},{messages,[]},{links,[<0.648.0>]},{dictionary,[{rand_seed,{#{bits => 58,jump => #Fun<rand.3.34006561>,next => #Fun<rand.0.34006561>,type => exsss,uniform => #Fun<rand.1.34006561>,uniform_n => #Fun<rand.2.34006561>},[31893854213874963|241807333728056958]}}]},{trap_exit,false},{status,running},{heap_size,6772},{stack_size,28},{reductions,43280}],[]]" label={proc_lib,crash}
when=2023-09-11T12:12:54.126140+00:00 level=error pid=<0.648.0> at=supervisor:do_restart/3:736 report="[{supervisor,{local,ejabberd_sup}},{errorContext,child_terminated},{reason,{{case_clause,#{}},[{gen_hook,run_hook,4,[{file,\"/home/circleci/project/src/gen_hook.erl\"},{line,237}]},{mongoose_hooks,run_fold,4,[{file,\"/home/circleci/project/src/hooks/mongoose_hooks.erl\"},{line,1388}]},{mongoose_cleaner,'-run_node_cleanup/1-lc$^0/1-0-',2,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,92}]},{mongoose_cleaner,'-run_node_cleanup/1-fun-1-',1,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,92}]},{timer,tc,1,[{file,\"timer.erl\"},{line,235}]},{mongoose_cleaner,run_node_cleanup,1,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,90}]},{global,trans,4,[{file,\"global.erl\"},{line,477}]},{mongoose_cleaner,cleanup_modules,1,[{file,\"/home/circleci/project/src/mongoose_cleaner.erl\"},{line,79}]}]}},{offender,[{pid,<0.652.0>},{id,mongoose_cleaner},{mfargs,{mongoose_cleaner,start_link,[]}},{restart_type,permanent},{significant,false},{shutdown,5000},{child_type,worker}]}]" label={supervisor,child_terminated}

Proposed changes include:

  • Return a proper type from mod_muc:node_cleanup_for_host_type/3
  • Add cleaner_SUITE testcase
  • Sadly, dialyzer didn't caught the bug

@mongoose-im
Copy link
Collaborator

mongoose-im commented Sep 11, 2023

elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / e5373fc
Reports root/ big
OK: 369 / Failed: 0 / User-skipped: 38 / Auto-skipped: 0


small_tests_24 / small_tests / e5373fc
Reports root / small


small_tests_25_arm64 / small_tests / e5373fc
Reports root / small


small_tests_25 / small_tests / e5373fc
Reports root / small


ldap_mnesia_24 / ldap_mnesia / e5373fc
Reports root/ big
OK: 2277 / Failed: 0 / User-skipped: 833 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / e5373fc
Reports root/ big
OK: 4206 / Failed: 0 / User-skipped: 116 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / e5373fc
Reports root/ big
OK: 2277 / Failed: 0 / User-skipped: 833 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / e5373fc
Reports root/ big
OK: 4238 / Failed: 0 / User-skipped: 84 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / e5373fc
Reports root/ big
OK: 4238 / Failed: 0 / User-skipped: 84 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / e5373fc
Reports root/ big
OK: 4235 / Failed: 0 / User-skipped: 87 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / e5373fc
Reports root/ big
OK: 2423 / Failed: 0 / User-skipped: 687 / Auto-skipped: 0


pgsql_cets_25 / pgsql_cets / e5373fc
Reports root/ big
OK: 4597 / Failed: 0 / User-skipped: 121 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / e5373fc
Reports root/ big
OK: 4607 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / e5373fc
Reports root/ big
OK: 4627 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / e5373fc
Reports root/ big
OK: 4627 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / e5373fc
Reports root/ big
OK: 4624 / Failed: 0 / User-skipped: 94 / Auto-skipped: 0

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03% 🎉

Comparison is base (4ec1d63) 83.90% compared to head (5ea7ec5) 83.93%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4122      +/-   ##
==========================================
+ Coverage   83.90%   83.93%   +0.03%     
==========================================
  Files         552      552              
  Lines       33630    33630              
==========================================
+ Hits        28217    28228      +11     
+ Misses       5413     5402      -11     
Files Changed Coverage Δ
src/mod_muc.erl 76.65% <100.00%> (+0.98%) ⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arcusfelis arcusfelis marked this pull request as ready for review September 11, 2023 19:57
Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

The code looks good, but why not put the tests in mongoose_cleanup_SUITE (small tests), which was actually created for the purpose of testing node cleanup?

@arcusfelis
Copy link
Contributor Author

@chrzaszcz we could test it in mongoose_cleanup_SUITE, just would require a lot of mocking. We could even mock the hook handler, but in this case that test would pass without the fix :)

mod_muc is not the module which is easy to start with no other deps (metrics, CETS, hooks, probably needs some storage (mnesia?) too. We could simplify the list by just maybe starting one hook or only hooks, but that would be easy to break and synthetic).

Main reason there was no mod_muc small tests, so to write that test I would probably have to write a couple of sanity tests that when we start the mocked mod_muc, stuff kinda makes sense. And it is more of a regression test, not test to ensure data is actually removed (should be also done in the future ideally).

@arcusfelis arcusfelis force-pushed the fix-node-cleanup-case-clause-error branch from e5373fc to 009b120 Compare September 13, 2023 08:07
Add small testcase
Sadly, dialyzer didn't caught the bug
@arcusfelis arcusfelis force-pushed the fix-node-cleanup-case-clause-error branch from 009b120 to 5ea7ec5 Compare September 13, 2023 08:09
@mongoose-im
Copy link
Collaborator

mongoose-im commented Sep 13, 2023

elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 5ea7ec5
Reports root/ big
OK: 369 / Failed: 0 / User-skipped: 38 / Auto-skipped: 0


small_tests_24 / small_tests / 5ea7ec5
Reports root / small


small_tests_25 / small_tests / 5ea7ec5
Reports root / small


small_tests_25_arm64 / small_tests / 5ea7ec5
Reports root / small


ldap_mnesia_24 / ldap_mnesia / 5ea7ec5
Reports root/ big
OK: 2276 / Failed: 0 / User-skipped: 833 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / 5ea7ec5
Reports root/ big
OK: 2276 / Failed: 0 / User-skipped: 833 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 5ea7ec5
Reports root/ big
OK: 4237 / Failed: 0 / User-skipped: 84 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 5ea7ec5
Reports root/ big
OK: 4237 / Failed: 0 / User-skipped: 84 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 5ea7ec5
Reports root/ big
OK: 4234 / Failed: 0 / User-skipped: 87 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 5ea7ec5
Reports root/ big
OK: 4205 / Failed: 0 / User-skipped: 116 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 5ea7ec5
Reports root/ big
OK: 2422 / Failed: 0 / User-skipped: 687 / Auto-skipped: 0


pgsql_cets_25 / pgsql_cets / 5ea7ec5
Reports root/ big
OK: 4596 / Failed: 0 / User-skipped: 121 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / 5ea7ec5
Reports root/ big
OK: 4606 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 5ea7ec5
Reports root/ big
OK: 4626 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 5ea7ec5
Reports root/ big
OK: 4626 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / 5ea7ec5
Reports root/ big
OK: 4622 / Failed: 1 / User-skipped: 94 / Auto-skipped: 0

muc_SUITE:register:user_unregisters_nick
{error,
  {{assertion_failed,assert,is_iq_result,
     [{xmlel,<<"iq">>,
        [{<<"type">>,<<"set">>},
         {<<"id">>,<<"43c45e6a50f8cd504ec08b6a082792f9">>},
         {<<"to">>,<<"muc.localhost">>}],
        [{xmlel,<<"query">>,
           [{<<"xmlns">>,<<"jabber:iq:register">>}],
           [{xmlel,<<"x">>,
            [{<<"xmlns">>,<<"jabber:x:data">>},
             {<<"type">>,<<"submit">>}],
            [{xmlel,<<"field">>,
               [{<<"type">>,<<"hidden">>},
                {<<"var">>,<<"FORM_TYPE">>}],
               [{xmlel,<<"value">>,[],
                  [{xmlcdata,<<"jabber:iq:register">>}]}]},
             {xmlel,<<"field">>,
               [{<<"type">>,<<"text-single">>},
                {<<"var">>,<<"nick">>}],
               [{xmlel,<<"value">>,[],
                  [{xmlcdata,
                     <<"thirdwitch1room-2d37787211">>}]}]}]}]}]}],
     {xmlel,<<"iq">>,
       [{<<"from">>,<<"muc.localhost">>},
        {<<"to">>,<<"alice_user_unregisters_nick_2741@localhost/res1">>},
        {<<"type">>,<<"error">>},
        {<<"id">>,<<"43c45e6a50f8cd504ec08b6a082792f9">>}],
       [{xmlel,<<"query">>,
          [{<<"xmlns">>,<<"jabber:iq:register">>}],
          [{xmlel,<<"x">>,
             [{<<"xmlns">>,<<"jabber:x:data">>},
            {<<"type">>,<<"submit">>}],
             [{xmlel,<<"field">>,
              [{<<"type">>,<<"hidden">>},
               {<<"var">>,<<"FORM_TYPE">>}],
              [{xmlel,<<"value">>,[],
                 [{xmlcdata...

Report log


small_tests_24 / small_tests / 5ea7ec5
Reports root / small


mssql_mnesia_25 / odbc_mssql_mnesia / 5ea7ec5
Reports root/ big
OK: 4623 / Failed: 0 / User-skipped: 94 / Auto-skipped: 0

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

👍

@chrzaszcz chrzaszcz merged commit 7d5726a into master Sep 13, 2023
4 checks passed
@chrzaszcz chrzaszcz deleted the fix-node-cleanup-case-clause-error branch September 13, 2023 15:04
@chrzaszcz chrzaszcz added this to the 6.2.0 milestone Dec 11, 2023
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.

3 participants