[Freeipa-devel] [PATCH] manage replication topology in the shared tree

[Ludwig Krispenz]

On 04/20/2015 06:00 PM, thierry bordaz wrote:
> On 04/13/2015 10:56 AM, Ludwig Krispenz wrote:
>> Hi,
>>
>> in the attachment you find the latest state of the "topology plugin", 
>> it implements what is defined in the design page: 
>> http://www.freeipa.org/page/V4/Manage_replication_topology (which is 
>> also waiting for a reviewer)
>>
>> It contains the plugin itself and  a core of ipa commands to manage a 
>> topology. to be really applicable, some work outside is required, eg 
>> the management of the domain level and a decision where the binddn 
>> group should be maintained.
>>
>> Thanks,
>> Ludwig
>>
>>
> Hello Ludwig,
>
> Quite long review to do. So far I only looked at the startup phase and 
> I have only few questions and comments.
Thanks for your time, and I'm looking forward to your review of the 
other parts, you raise some valid points.
I'll try to answer some of them inline, but will integrate some into a 
next version of the patch
>
> In ipa_topo_start, do you need to get argc/argv as you are not using 
> plugin-argxx attributes ?
no. It was a leftover from a "standard" plugin
>
>
> topo_plugin_conf configuration parameters are not freed when the 
> plugin is closed. Is it closed only at shutdown ?
> Also I would initiatlize it to {NULL}.
So far it is not planned to be dynamic, but I will addres the memory 
management
>
> In case the config does not contain any 
> nsslapd-topo-plugin-shared-replica-root, I wonder if 
> ipa_topo_apply_shared_config may crash as shared_replica_root will be 
> NULL.
> or at least in 
> ipa_topo_apply_shared_replica_config/ipa_topo_util_get_replica_conf.
>
> Also if nsslapd-topo-plugin-shared-replica-root contains an invalid 
> root suffix (typo), topoRepl remains NULL and 
> ipa_topo_util_get_replica_conf/ipa_topo_cfg_replica_add can crash.
for the two comments above, I was assuming that plugin conf and shared 
tree would be setup by ipa tools and server setup, so assuming only 
valid data, but you are right, checking for bad data doesn't hurt.
>
> In ipa_topo_util_segment_from_entry, if the config entry has no 
> direction/left/right it will crash. Shouldn't it return an error if 
> the config is invalid.
adding a segment should be done with the ipa command 'ipa 
topologysegment-add ...' and this always provides a direction (param or 
default). If you try to add a segment directly, direction is a required 
attribute of teh segment objectclass, so it should be rejected-
>
> The update of domainLevel may start the plugin. If two mods update the 
> domainLevel they could be done in parallele.
yes :-(
>
>
> In ipa_topo_util_update_agmt_list, if there is a marked agmnt but no 
> segment it deletes the agreement.
> Is it possible there is a segment but no agmnt ? For example, if the 
> server were stopped or crashed after the segment was created but 
> before the local config was updated.
then it should be created from the segment
>
>
> Hosts are taken from shared config tree (cn=masters,<cfg>), is it 
> possible to have a replica agreement to a host that is not under 
> 'cn=masters,<cfg>'
yes, it will be ignored by the plugin
>
>
> thanks
> thierry
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150421/294c09d8/attachment.htm>