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

[thierry bordaz]

On 04/21/2015 10:49 AM, Ludwig Krispenz wrote:
>
> 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
>>
>


Hi Ludwig,

I continued the review of the design/topology plugin code. This is 
really an interesting plugin but unfortunately I have not yet reviewed 
all the parts.

I went through the design and digging the related parts in the code. So 
far I need to review the rest starting at 
http://www.freeipa.org/page/V4/Manage_replication_topology#connectivity_management.

I think I did ~50% design but may be more than 50% of the code.

Here are additional points:


    in ipa_topo_set_domain_level, you may record the new Domain level
    value as FATAL (it is already recorded in case of oneline import)

    ipa_topo_be_state_change is called for any backend going online.
    Domain level and start should be done only for a backend mapping a
    shared-replica-root.
    Also the plugin can be started many times (each online init),
         ipa_topo_util_start is not protected by a lock
         Some fields will leak (in ipa_topo_init_shared_config)
    Also I wonder if you reinit several times the same replica-root, its
    previous config will leak. (replica->repl_segments)

    In ipa_topo_apply_shared_replica_config,
    I do not see where replica_config is kept (leak ?)

    ipa_topo_util_start/ipa_topo_apply_shared_config is called at
    startup or during online-init.
    For online-init, if the plugin was already active, what is the need
    of calling ipa_topo_util_start ?
    For online-init, It initializes all the replica-root. Could it init
    only the reinitialized replic-root ?

    in
    http://www.freeipa.org/page/V4/Manage_replication_topology#shared_configuration:_database,
    it mentions ipaReplTopoConfigMaster.
    Is it implemented  ?

    in
    http://www.freeipa.org/page/V4/Manage_replication_topology#shared_configuration:_segment,
    what happens if a server under cn=masters is removed ?

    in
    http://www.freeipa.org/page/V4/Manage_replication_topology#shared_configuration:_example.
    There is a segment cn=111_to_102. For example it was created by
    vm-111 when topology plugin starts with 'dc=example,dc=com' .
    What prevents vm-102 topology plugins to create the segment
    cn=102_to_111 ?

    in ipa_topo_post_mod.
    Is it 100% that if we have 'cn=replica
    example,cn=topology,dc=example,dc=com' then it exists the related
    config in topo_shared_conf.replicas.
    In ipa_topo_util_get_replica_conf, it is looking like the entry can
    exist before the related config is added.
    In that case when modifying a segment
    ipa_topo_util_get_conf_for_segment will return 'tconf' config that
    is not linked in topo_shared_conf.replicas  tconf will leak and I am
    unsure the post_mod is fully processed.

    In ipa_topo_post_mod
    in ipa_topo_util_segment_update if the
    segment.ipaReplTopoSegmentDirection was "none" and MOD set it to
    "both", segment->right/left are not set but it is said to be
    bidirectional

    ipaReplTopoSegmentStatus: can not find it in the design

    in ipa_topo_util_existing_agmts_update, my understanding is that a
    host only updates its local replica agreement.
    So even if the segment update is replicated, others hosts will not
    skip updates where ->origin is not themself.
    I think you may add a comment about this as it looks an important thing.
    Also I did not find this comment in the design but may be I missed it.

    in ipa_topo_util_existing_agmts_update, it applies the mods on left
    or on right. That means we do not support serveral instance on the
    same machine. I also missed that point in the design.

    in ipa_topo_agmt_mod, it does nothing when deleting a managed
    attribute ?

    in ipa_topo_agmt_mod, if update of the replica agreement fails
    (ipa_topo_agreement_dn or ipa_topo_util_modify) you may log a message

    in ipa_topo_agmt_mod, if the mod is not related to any managed
    attribute, there is no replica agreement update but the 'dn' is not
    freed.

    in ipa_topo_post_mod, I do not see 'domainLevel' in the schema. Is
    it stored in an extensible object ?



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