Bug 689123

Created attachment 486393 [details]
Add more params to totem

hi Vladislav,

something does not look right in the patch:

seqno_nchanged_const default is 30, but if you don´t find the key you set it to 2500 ?

I am also a bit puzzled.. why do we need to set defaults again?

Once you add those values to the <totem section in cluster.conf, they should be propagated automatically down to totem in objdb.

In theory, allowing the config validation should be enough.

In cmanpre_readconfig:

                ret = copy_tree_to_root(objdb, "totem", 0);

that copies pristine

<cluster>
 <totem...>

to the equivalent of:

totem {

in corosync.conf

My guess (sorry I don´t have time to do testing right now) is that validating the config bits should be enough.

Fabio

> seqno_nchanged_const default is 30, but if you don´t find the key you set it to
> 2500 ?

Oops, just a blind copy from existing code ;)

> In theory, allowing the config validation should be enough.
> 
> In cmanpre_readconfig:
> 
>                 ret = copy_tree_to_root(objdb, "totem", 0);

I dug into cman code just for the first time, so wasn't aware of this one.

> My guess (sorry I don´t have time to do testing right now) is that validating
> the config bits should be enough.

Do you mean that patch against cluster.rng.in is enough?

(In reply to comment #3)
> > seqno_nchanged_const default is 30, but if you don´t find the key you set it to
> > 2500 ?
> 
> Oops, just a blind copy from existing code ;)

eheh.. yeah I noticed the similarity from the few lines above :)

> 
> > In theory, allowing the config validation should be enough.
> > 
> > In cmanpre_readconfig:
> > 
> >                 ret = copy_tree_to_root(objdb, "totem", 0);
> 
> I dug into cman code just for the first time, so wasn't aware of this one.

No worries, I am very glad somebody is contributing stuff back.

> 
> > My guess (sorry I don´t have time to do testing right now) is that validating
> > the config bits should be enough.
> 
> Do you mean that patch against cluster.rng.in is enough?

In theory it should be more than enough. I would assume you have a setup that requires those changes and therefor you should be able to verify that easily?

It could be easily verified with corosync-objctl, so yes.

I do not promise to do it quickly, only with next restart of my testing cluster in cman mode (it now runs on openais stack). It is not yet scheduled, so it could take some days.

(In reply to comment #5)
> It could be easily verified with corosync-objctl, so yes.
> 
> I do not promise to do it quickly, only with next restart of my testing cluster
> in cman mode (it now runs on openais stack). It is not yet scheduled, so it
> could take some days.

I am in no hurry personally. Take your time. 3.1.2 is nowhere ready to be released.

Created attachment 487043 [details]
Fixed version

Hi Fabio,

attached is trimmed version.

BTW, shouldn't I specify that default value should be consulted in corosync.conf(5)?

(In reply to comment #7)
> Created attachment 487043 [details]
> Fixed version
> 
> Hi Fabio,
> 
> attached is trimmed version.
> 
> BTW, shouldn't I specify that default value should be consulted in
> corosync.conf(5)?

Hi Vladislav,

Yes sure, there is also a specific flag for the schema to point to a man page.

Assuming you tested the patch, it should be good to go in the tree. Please let me know if you want to update it more to add references to the man pages.

Fabio

Created attachment 487071 [details]
The same without defaults

I just dropped defaults from patch (inline) assuming they may change in corosync. Whole totem section should refer to man page, not every option.
Previous patch is tested to work, this one should apply cleanly too (nothing is functionally changed).

http://git.fedorahosted.org/git/?p=cluster.git;a=commitdiff;h=e7d91758422f354a3488db5f9d375484b4be4535

The patch is slightly different than submitted one. There was a typo fix in one of the entries and added rha:default values.