<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 04/29/2015 11:18 AM, Ludwig Krispenz
wrote:<br>
</div>
<blockquote cite="mid:5540A1D5.2000301@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi, thanks again, so there is some work to do, but see some
answers inline<br>
<div class="moz-cite-prefix">On 04/27/2015 10:18 AM, thierry
bordaz wrote:<br>
</div>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 04/21/2015 10:49 AM, Ludwig
Krispenz wrote:<br>
</div>
<blockquote cite="mid:55360F10.7010804@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<br>
<div class="moz-cite-prefix">On 04/20/2015 06:00 PM, thierry
bordaz wrote:<br>
</div>
<blockquote cite="mid:553522A2.9090007@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 04/13/2015 10:56 AM, Ludwig
Krispenz wrote:<br>
</div>
<blockquote cite="mid:552B84C5.80300@redhat.com" type="cite">Hi,
<br>
<br>
in the attachment you find the latest state of the
"topology plugin", it implements what is defined in the
design page: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://www.freeipa.org/page/V4/Manage_replication_topology">http://www.freeipa.org/page/V4/Manage_replication_topology</a>
(which is also waiting for a reviewer) <br>
<br>
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. <br>
<br>
Thanks, <br>
Ludwig <br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<font face="Times New Roman, Times, serif">Hello Ludwig,<br>
<br>
Quite long review to do. So far I only looked at the
startup phase and I have only few questions and comments.<br>
</font></blockquote>
<font face="Times New Roman, Times, serif">Thanks for your
time, and I'm looking forward to your review of the other
parts, you raise some valid points.<br>
I'll try to answer some of them inline, but will integrate
some into a next version of the patch<br>
</font>
<blockquote cite="mid:553522A2.9090007@redhat.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
In ipa_topo_start, do you need to get argc/argv as you are
not using plugin-argxx attributes ?<br>
</font></blockquote>
<font face="Times New Roman, Times, serif">no. It was a
leftover from a "standard" plugin</font><br>
<blockquote cite="mid:553522A2.9090007@redhat.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
<br>
topo_plugin_conf configuration parameters are not freed
when the plugin is closed. Is it closed only at shutdown ?<br>
Also I would initiatlize it to {NULL}.<br>
</font></blockquote>
<font face="Times New Roman, Times, serif">So far it is not
planned to be dynamic, but I will addres the memory
management</font><br>
<blockquote cite="mid:553522A2.9090007@redhat.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
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.<br>
or at least in
ipa_topo_apply_shared_replica_config/ipa_topo_util_get_replica_conf.<br>
<br>
Also if </font><font face="Times New Roman, Times, serif"><font
face="Times New Roman, Times, serif">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.</font><br>
</font></blockquote>
<font face="Times New Roman, Times, serif">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.</font><br>
<blockquote cite="mid:553522A2.9090007@redhat.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
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.<br>
</font></blockquote>
<font face="Times New Roman, Times, serif">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-</font><br>
<blockquote cite="mid:553522A2.9090007@redhat.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
The update of domainLevel may start the plugin. If two
mods update the domainLevel they could be done in
parallele.<br>
</font></blockquote>
<font face="Times New Roman, Times, serif">yes :-(</font><br>
<blockquote cite="mid:553522A2.9090007@redhat.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
<br>
In ipa_topo_util_update_agmt_list, if there is a marked
agmnt but no segment it deletes the agreement.<br>
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.<br>
</font></blockquote>
<font face="Times New Roman, Times, serif">then it should be
created from the segment</font><br>
<blockquote cite="mid:553522A2.9090007@redhat.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
<br>
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>'<br>
</font></blockquote>
<font face="Times New Roman, Times, serif">yes, it will be
ignored by the plugin</font><br>
<blockquote cite="mid:553522A2.9090007@redhat.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
<br>
thanks<br>
thierry<br>
<br>
</font> </blockquote>
<br>
</blockquote>
<br>
<br>
<font face="Times New Roman, Times, serif">Hi Ludwig,<br>
<br>
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.<br>
<br>
I went through the design and digging the related parts in the
code. So far I need to review the rest starting at <a
moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://www.freeipa.org/page/V4/Manage_replication_topology#connectivity_management">http://www.freeipa.org/page/V4/Manage_replication_topology#connectivity_management</a>.<br>
<br>
I think I did ~50% design but may be more than 50% of the
code.<br>
<br>
Here are additional points:<br>
<br>
</font><tt><br>
</tt>
<blockquote><tt>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)</tt><br>
</blockquote>
</blockquote>
this just records the actual domain level, I don't think we need
to log it every time, only if it is changed should be sufficient
(to verify)<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <br>
<tt>ipa_topo_be_state_change is called for any backend going
online. </tt><br>
<tt>Domain level and start should be done only for a backend
mapping a shared-replica-root.</tt><br>
</blockquote>
</blockquote>
<tt>yes, the comment is already there, but the actual check isn't,
so it would be recreated at online init of any backend</tt>,
think it is not too bad, but will change it<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <tt>Also the plugin can be started many times
(each online init), </tt><br>
<tt> ipa_topo_util_start is not protected by a lock</tt><br>
<tt> Some fields will leak (in ipa_topo_init_shared_config)</tt><br>
<tt>Also I wonder if you reinit several times the same
replica-root, its previous config will leak.
(replica->repl_segments)</tt><br>
</blockquote>
</blockquote>
you shouldn't :-), but needs to be made safer<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <br>
<tt>In ipa_topo_apply_shared_replica_config, </tt><br>
<tt>I do not see where replica_config is kept (leak ?)</tt><br>
</blockquote>
</blockquote>
<tt>it is created and set to the shared_conf data, but if it
aready exists, </tt>it will leak (that's probably the case
above), it should be freed when the plugin is stopped<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <br>
<tt>ipa_topo_util_start/ipa_topo_apply_shared_config is called
at startup or during online-init.</tt><br>
<tt>For online-init, if the plugin was already active, what is
the need of calling ipa_topo_util_start ?</tt><br>
</blockquote>
</blockquote>
you don't know if the data in the shared tree are teh same as
before<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <tt>For online-init, It initializes all the
replica-root. Could it init only the reinitialized
replic-root ?</tt><br>
</blockquote>
</blockquote>
yes, it could (sjhould).<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <br>
<tt>in <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://www.freeipa.org/page/V4/Manage_replication_topology#shared_configuration:_database">http://www.freeipa.org/page/V4/Manage_replication_topology#shared_configuration:_database</a>,
it mentions ipaReplTopoConfigMaster.</tt><br>
<tt>Is it implemented ?</tt><br>
</blockquote>
</blockquote>
<tt>it is there as a concept, not completed</tt><br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <br>
<tt>in <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://www.freeipa.org/page/V4/Manage_replication_topology#shared_configuration:_segment">http://www.freeipa.org/page/V4/Manage_replication_topology#shared_configuration:_segment</a>,
what happens if a server under cn=masters is removed ?</tt><br>
</blockquote>
</blockquote>
<tt>for all its manages suffixes, the marked segments connecting
it are removed, the ldap principal is removed form the binddn
groups</tt><br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <br>
<tt>in <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://www.freeipa.org/page/V4/Manage_replication_topology#shared_configuration:_example">http://www.freeipa.org/page/V4/Manage_replication_topology#shared_configuration:_example</a>.</tt><br>
<tt>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' .</tt><br>
<tt>What prevents vm-102 topology plugins to create the
segment cn=102_to_111 ?</tt><br>
</blockquote>
</blockquote>
<tt>the pre add check should prevent this, but if you do it
simultaneously, two segments will be added and one will be
ignored when the internal segments are updated (double check)</tt><br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <br>
<tt>in ipa_topo_post_mod.</tt><br>
<tt>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.</tt><br>
<tt>In ipa_topo_util_get_replica_conf, it is looking like the
entry can exist before the related config is added.</tt><br>
<tt>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.</tt><br>
<br>
<tt>In ipa_topo_post_mod</tt><br>
<tt>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</tt><br>
</blockquote>
</blockquote>
<tt>I'm no longer sure if we have a valid scenario, need to think
about it again</tt><br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <br>
<tt>ipaReplTopoSegmentStatus: can not find it in the design</tt><br>
</blockquote>
</blockquote>
it was forgotten when the method to marjk agreements was last
chnged :-)<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <br>
<tt>in ipa_topo_util_existing_agmts_update, my understanding
is that a host only updates its local replica agreement.</tt><br>
</blockquote>
</blockquote>
that's the only agreements it can update with internal ops<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <tt>So even if the segment update is replicated,
others hosts will not skip updates where ->origin is not
themself.</tt><br>
</blockquote>
</blockquote>
you mean will skip/ignore ?<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <tt>I think you may add a comment about this as it
looks an important thing.</tt><br>
<tt>Also I did not find this comment in the design but may be
I missed it.</tt><br>
</blockquote>
</blockquote>
ok<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> <br>
<tt>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.</tt><br>
<br>
<tt>in ipa_topo_agmt_mod, it does nothing when deleting a
managed attribute ?</tt><br>
<br>
<tt>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</tt><br>
<br>
<tt>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.</tt><br>
<br>
<tt>in ipa_topo_post_mod, I do not see 'domainLevel' in the
schema. Is it stored in an extensible object ?</tt><br>
</blockquote>
</blockquote>
the mod of an agreeement via the plugin was a bit neglected in my
tests, will check again and answer later<br>
<blockquote cite="mid:553DF0C6.30403@redhat.com" type="cite">
<blockquote> </blockquote>
<tt><br>
</tt><font face="Times New Roman, Times, serif"><br>
</font> </blockquote>
<br>
</blockquote>
<br>
<br>
<font face="Times New Roman, Times, serif">Hi Ludwig,<br>
<br>
Last (late) part of the review <span class="moz-smiley-s1"><span>
:-) </span></span><br>
<br>
In ipa_topo_agmt_setup, the 'cn' of the replica agreement need to
be freed.<br>
<br>
My understanding is that ipa_topo_apply_shared_config will not be
called twice in parallel. Correct ?<br>
<br>
In ipa_topo_util_update_agmt_list, 'filter' will leak<br>
<br>
In ipa_topo_util_update_agmt_list, shouldn't 'smods' be freed ?<br>
<br>
In ipa_topo_util_segment_merge,.. difficult to understand but nice
<span class="moz-smiley-s1"><span> :-) </span></span><br>
When merging, depending on which server the merge will occur, we
may decide to duplicate the first replica_agreement managed
attributes<br>
or to keep the values given in the added segment. (at least it is
what I understand <span class="moz-smiley-s7"><span> :-\ </span></span>)<br>
</font><font face="Times New Roman, Times, serif"><br>
<br>
In ipa_topo_util_update_segments_for_host, when adding a replica
we lookup all the RA toward that replica. But if RA is related to
a not managed repl_root, I think it may crash in
ipa_topo_util_segment_write/ipa_topo_segment_dn (conf=NULL)<br>
<br>
<br>
ipa_topo_connection_fanout returns allocated targets/node_fanout.
I do not see where it is freed in
ipa_topo_check_disconnect_reject.<br>
<br>
In design, it is said that in preop internal operation by
topology_plugin are successful. Where is it tested ?<br>
<br>
Regarding the design:<br>
I have not found the list of operation that the admnistrator has
to do before activating the plugin. For example, if a non marked
replica agreement exists it is deleted if there is no segment to
the replicahost. Does that mean that all segments must be created
or RA marked before activating the plugin ?<br>
<br>
If some attributes are skipped from replication (reduce
replication trafic or DMZ), the checking of the connectivity
become complex.<br>
For example if a replica receives only parts of the attributes and
an other all the attributes. The decision to create a replica
agreement to a new replica depends if we want fractional/full
replication.<br>
<br>
Thanks<br>
thierry<br>
</font>
</body>
</html>