Re: [PATCH] Support for MO direction 'group = smsbox-route' reloading (aka graceful restart)

Am 15.10.2018 17:33, schrieb [email protected]:
> Hi Stipe,
>
> sorry I was not available earlier…

Hi Alex,

> just checked your patch and see man many issues…
>
> 1.
>
> +static void init_smsbox_routes(Cfg *cfg, int reload)
>
> you use continue if config is wrong
>
> but you already destroyed previous configuration
>
> - /* send new config to clients */
> + gw_rwlock_wrlock(smsbox_list_rwlock);
> + dict_destroy(smsbox_by_smsc);
> + dict_destroy(smsbox_by_receiver);
> + dict_destroy(smsbox_by_smsc_receiver);
> + smsbox_by_smsc = dict_create(30, (void(*)(void *)) octstr_destroy);
> + smsbox_by_receiver = dict_create(50, (void(*)(void *)) octstr_destroy);
> + smsbox_by_smsc_receiver = dict_create(50, (void(*)(void *))
> octstr_destroy);
> + init_smsbox_routes(cfg, 1);
> + gw_rwlock_unlock(smsbox_list_rwlock);
>
> That means, you have inconsistency

hmm, yes we do this intentionally actually.

We destroy the structures, then re-parse the 'group = smsbox-route' 
groups from the configuration. If a group has an error, we only dump the 
error but continue to parse the following groups.

A restart would formally break via PANIC, since the config has an issue. 
But we keep on going with all groups that are configured correctly.

I'm assuming you would rather re-parse the new config, and ONLY if it 
has no error swap the new config with the old one. If it contains an 
error rather keep running the old one?

> 2. if I see reload configs without a lock… Uhh I can construct soo many
> cases, e.g. smsc is renamed in new config but still used in the old
> smsbox group, etc. etc.
>
> int bb_graceful_restart(void)
> {
> - return smsc2_graceful_restart();
> + Cfg *cfg;
> +
> + info(0, "Reloading configuration resource `%s'.",
> octstr_get_cstr(cfg_filename));
> + cfg = cfg_create(cfg_filename);
> + if (cfg_read(cfg) == -1) {
> + error(0, "Error processing configuration resource `%s'. Continue with
> existing configuration.",
> + octstr_get_cstr(cfg_filename));
> + return -1;
> + }
> +
> + smsc2_graceful_restart(cfg);
> + smsbox_restart(cfg);
> + return 0;
> }

the smsbox_restart() locks down via

   gw_rwlock_wrlock(smsbox_list_rwlock);

and the MO receiving part do route_incoming_to_boxc(), which sets:

   gw_rwlock_rdlock(smsbox_list_rwlock);

so we SHOULD never be in a race-condition where we would try to get a MO 
routing and the structures are destroyed in the middle of the process. 
Right?

I don't understand the argument with "smsc is renamed in new config but 
still used in the old smsbox group". Could you please explain?

Cheers,
Stipe

-- 
Best Regards,
Stipe Tolj

-------------------------------------------------------------------
Düsseldorf, NRW, Germany

Kannel Foundation                 tolj.org system architecture
http://www.kannel.org/            http://www.tolj.org/

stolj at kannel.org               st at tolj.org
-------------------------------------------------------------------