[389-devel] Re: Deref plugin entries == NULL #4525

[thierry bordaz]

On 1/15/21 11:53 AM, Pierre Rogier wrote:

Hi Thierry,

I was rather thinking about the key and value duplication when 
querying the DB:


When using bdb functions that is done implicitly.
  bdb either copies the values in the DBT buffer or it alloc/realloc it
When mimicking bdb behavior with LDBM we will have to do that 
explicitly in the LDBM plugin:

    LDMB returns a memory mapped address that may be ummapped
   once the txn is ended. So we must copy the result before 
closing the txn.
If we have a read txn that protects the full operation lifespan, then 
we could directly use the mapped address without needing to duplicate 
them.

ah okay ! nice.
Just a question, if we have a txn covering a search with large candidate 
list (unindexed), does that mean that by default each db key/value will 
remain mapped until txn commit ?


thanks
thierry


Pierre

On Fri, Jan 15, 2021 at 10:53 AM thierry bordaz <mailto:tbor...@redhat.com>> wrote:




On 1/14/21 12:32 PM, Pierre Rogier wrote:

Hi William,

> It's a scenario we will need to fix via your BE work because of
the MVCC transaction model that
> LMDB will force us to adopt :)
As I see things in the early phases the lmdb read txn will
probably only be managed at the db plugin level rather than at
backend level. That means that we will have the same
inconsistency risk than today (i.e as if using bdb and the
implicit txn).
The txn model redesign you are speaking about should only occur
in one of the last phases (once bdb does no more coexists with lmdb).
It must be done because it could provide a serious performance
boost for read operations (IMHO, In most cases we could avoid to
duplicate the db data)

Pierre, what duplicate are you thinking of ? str2entry ?


But we should not do it while bdb is still around because of the
risk of lock issue and excessive retries.

Note I put a phasing section in

https://directory.fedoraproject.org/docs/389ds/design/backend-redesign-phase3.html#phasing
explaining that. But I guess I should move it within Ludwig's
document that englobs it.

Pierre

On Thu, Jan 14, 2021 at 12:01 AM William Brown mailto:wbr...@suse.de>> wrote:



> On 13 Jan 2021, at 21:24, Pierre Rogier mailto:prog...@redhat.com>> wrote:
>
> Thank you Willian,
> So far your scenario (entry found when reading base entry
but no more existing when computing the candidates) is the
only one that matches the symptoms.

It's a scenario we will need to fix via your BE work because
of the MVCC transaction model that LMDB will force us to
adopt :)

> And that triggered a thought:
>  We cannot do anything for SUBTREE and ONE_LEVEL searches
>   because the fact that the base entry id is not in the
candidate may be normal
>  but IMHO we should improve the BASE search case.
> In this case the candidate list is directly set to the base
entry id
>  ==> if the candidate entry (in
ldbm_back_next_search_entry) is not found and the scope is
BASE then we should return a LDAP_NO_SUCH_ENTRY error ..

I suspect that Mark has seen this email and submitted a PR to
resolve this exact case :)


>
>        Pierre
>
>
> On Wed, Jan 13, 2021 at 1:45 AM William Brown
mailto:wbr...@suse.de>> wrote:
> Hey there,
>
> https://github.com/389ds/389-ds-base/pull/4525/files
>
> I had a look and I can see a few possible contributing
factors, but without a core and the exact state I can't be
sure if this is correct. It's all just hypothetical from
reading the code.
>
>
> The crash is in deref_do_deref_attr() which is called as
part of deref_pre_entry(). This is the
SLAPI_PLUGIN_PRE_ENTRY_FN which is called by
"./ldap/servers/slapd/result.c:1488:    rc =
plugin_call_plugins(pb, SLAPI_PLUGIN_PRE_ENTRY_FN);"
>
>
> I think what's important here is that the search is
conducted in ./ldap/servers/slapd/opshared.c:818 rc =
(*be->be_search)(pb);  Is *not* in a transaction. That means
that while the single search in be_search() is consistent due
to an implied transaction, the subsequent search in
deref_pre_entry() is likely conducted in a seperate
transaction. This allows for other operations to potentially
interleave and cause changes - modrdn or delete would
certainly be candidates to cause a DN to be remove between
these two points. It would be extremely hard to reproduce as
a race condition of course.
>
 

[389-devel] Re: Deref plugin entries == NULL #4525

[thierry bordaz]

On 1/14/21 12:32 PM, Pierre Rogier wrote:

Hi William,

> It's a scenario we will need to fix via your BE work because of the 
MVCC transaction model that

> LMDB will force us to adopt :)
As I see things in the early phases the lmdb read txn will 
probably only be managed at the db plugin level rather than at backend 
level. That means that we will have the same inconsistency risk than 
today (i.e as if using bdb and the implicit txn).
The txn model redesign you are speaking about should only occur in one 
of the last phases (once bdb does no more coexists with lmdb).
It must be done because it could provide a serious performance boost 
for read operations (IMHO, In most cases we could avoid to duplicate 
the db data)

Pierre, what duplicate are you thinking of ? str2entry ?

But we should not do it while bdb is still around because of the 
risk of lock issue and excessive retries.


Note I put a phasing section in
https://directory.fedoraproject.org/docs/389ds/design/backend-redesign-phase3.html#phasing
explaining that. But I guess I should move it within Ludwig's document 
that englobs it.


Pierre

On Thu, Jan 14, 2021 at 12:01 AM William Brown > wrote:




> On 13 Jan 2021, at 21:24, Pierre Rogier mailto:prog...@redhat.com>> wrote:
>
> Thank you Willian,
> So far your scenario (entry found when reading base entry but no
more existing when computing the candidates) is the only one that
matches the symptoms.

It's a scenario we will need to fix via your BE work because of
the MVCC transaction model that LMDB will force us to adopt :)

> And that triggered a thought:
>  We cannot do anything for SUBTREE and ONE_LEVEL searches
>   because the fact that the base entry id is not in the
candidate may be normal
>  but IMHO we should improve the BASE search case.
> In this case the candidate list is directly set to the base entry id
>  ==> if the candidate entry (in ldbm_back_next_search_entry) is
not found and the scope is BASE then we should return a
LDAP_NO_SUCH_ENTRY error ..

I suspect that Mark has seen this email and submitted a PR to
resolve this exact case :)


>
>        Pierre
>
>
> On Wed, Jan 13, 2021 at 1:45 AM William Brown mailto:wbr...@suse.de>> wrote:
> Hey there,
>
> https://github.com/389ds/389-ds-base/pull/4525/files
>
> I had a look and I can see a few possible contributing factors,
but without a core and the exact state I can't be sure if this is
correct. It's all just hypothetical from reading the code.
>
>
> The crash is in deref_do_deref_attr() which is called as part of
deref_pre_entry(). This is the SLAPI_PLUGIN_PRE_ENTRY_FN which is
called by "./ldap/servers/slapd/result.c:1488:    rc =
plugin_call_plugins(pb, SLAPI_PLUGIN_PRE_ENTRY_FN);"
>
>
> I think what's important here is that the search is conducted in
./ldap/servers/slapd/opshared.c:818  rc = (*be->be_search)(pb); 
Is *not* in a transaction. That means that while the single search
in be_search() is consistent due to an implied transaction, the
subsequent search in deref_pre_entry() is likely conducted in a
seperate transaction. This allows for other operations to
potentially interleave and cause changes - modrdn or delete would
certainly be candidates to cause a DN to be remove between these
two points. It would be extremely hard to reproduce as a race
condition of course.
>
>
> A question you asked is why don't we get a "no such entry" error
or similar? I think that this is because build_candidate_list in
ldbm_search.c doesn't actually create an error if the
base_candidates list is empty, because an IDL is allocated with a
value of 0 (no matching entries). this allows the search to
proceed, and there are no errors, and the result set is set to
NULL with size 0. I can't see where LDAP_NO_SUCH_OBJECT is set in
this process, but without looking further into it, my suspicion is
that entries of size 0 WONT return an error condition to
internal_search_pb, so it's valid for this to be empty.
>
> Anyway, again, this is just reading the code for 20 minutes, and
is not a complete in depth investigation, but maybe it's some
ideas about what happened?
>
> Hope it helps :)
>
>
>
> —
> Sincerely,
>
> William Brown
>
> Senior Software Engineer, 389 Directory Server
> SUSE Labs, Australia
> ___
> 389-devel mailing list -- 389-de...@lists.fedoraproject.org

> To unsubscribe send an email to
389-devel-le...@lists.fedoraproject.org

> Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines:
https:

[389-devel] Re: Deref plugin entries == NULL #4525

[thierry bordaz]

On 1/13/21 1:44 AM, William Brown wrote:

Hey there,

https://github.com/389ds/389-ds-base/pull/4525/files

I had a look and I can see a few possible contributing factors, but without a 
core and the exact state I can't be sure if this is correct. It's all just 
hypothetical from reading the code.


The crash is in deref_do_deref_attr() which is called as part of deref_pre_entry(). This 
is the SLAPI_PLUGIN_PRE_ENTRY_FN which is called by 
"./ldap/servers/slapd/result.c:1488:rc = plugin_call_plugins(pb, 
SLAPI_PLUGIN_PRE_ENTRY_FN);"


I think what's important here is that the search is conducted in 
./ldap/servers/slapd/opshared.c:818  rc = (*be->be_search)(pb);  Is *not* in a 
transaction. That means that while the single search in be_search() is consistent 
due to an implied transaction, the subsequent search in deref_pre_entry() is 
likely conducted in a seperate transaction. This allows for other operations to 
potentially interleave and cause changes - modrdn or delete would certainly be 
candidates to cause a DN to be remove between these two points. It would be 
extremely hard to reproduce as a race condition of course.


Hi William, Pierre,

Thanks for your feedback. I realize how complex it is to think to a 
possible explanation and I really appreciate.

I am still missing some parts to understand how it happened.
In the current crash there was no transaction at all "protecting" the 
initial search or nested searches. So yes we can imagine the entry got 
deleted between the base lookup and candidate list build but it is not 
related to txn.

Note that the logs do not contain direct delete of the entry.
Also during base search, the base entry is lookup. It was successful 
else it would have return a search failure. In such case the candidate 
list is not empty, it contains the base search entry ID (e->ep_id).
Finally, the candidates are evaluated against the filter 
(objectclass=*). It could be that phase that is failing if the entry was 
cleared from the entry cache and ep_id lookup failed.


regards
thierry



A question you asked is why don't we get a "no such entry" error or similar? I 
think that this is because build_candidate_list in ldbm_search.c doesn't actually create 
an error if the base_candidates list is empty, because an IDL is allocated with a value 
of 0 (no matching entries). this allows the search to proceed, and there are no errors, 
and the result set is set to NULL with size 0. I can't see where LDAP_NO_SUCH_OBJECT is 
set in this process, but without looking further into it, my suspicion is that entries of 
size 0 WONT return an error condition to internal_search_pb, so it's valid for this to be 
empty.

Anyway, again, this is just reading the code for 20 minutes, and is not a 
complete in depth investigation, but maybe it's some ideas about what happened?

Hope it helps :)



—
Sincerely,

William Brown

Senior Software Engineer, 389 Directory Server
SUSE Labs, Australia
___
389-devel mailing list -- 389-de...@lists.fedoraproject.org
To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org
Fedora Code of Conduct: 
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/389-de...@lists.fedoraproject.org

___
389-devel mailing list -- 389-de...@lists.fedoraproject.org
To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org
Fedora Code of Conduct: 
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/389-de...@lists.fedoraproject.org

[389-devel] Please review lib389 48984: support environment variable to define defaults.inf

[thierry bordaz]

While testing in a prefix install I found it useful: 
https://fedorahosted.org/389/attachment/ticket/48984/0003-Ticket-48984-support-of-environment-variable-for-def.patch

___
389-devel mailing list -- 389-de...@lists.fedoraproject.org
To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org

[389-devel] 389-DS ACI improvement to control MODDN

[thierry bordaz]

Hello,

   Ticket https://fedorahosted.org/389/ticket/47553, is a 389-ds
   enhancement to allow a finer access control during a MODDN (new
   superior) operation. The use case being to allow/deny a bound user
   to move an entry from one specified part of the DIT to an other part.
   This without the need to grant the ADD permission.

   I started a design of it
   http://port389.org/wiki/Access_control_on_trees_specified_in_MODDN_operation.
   Comments are welcomed.

   regards
   thierry

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

Re: [389-devel] Design review: Access control on entries specified in MODDN operation (ticket 47553)

[thierry bordaz]

On 02/24/2014 11:35 PM, Rich Megginson wrote:

On 02/24/2014 02:47 PM, Noriko Hosoi wrote:

Rich Megginson wrote:

On 02/24/2014 09:00 AM, thierry bordaz wrote:

Hello,

IPA team filled this ticket
https://fedorahosted.org/389/ticket/47553.

It requires an ACI improvement so that during a MODDN a given
user is only allowed to move an entry from one specified part
of the DIT to an other specified part of the DIT. This without
the need to grant the ADD permission.

Here is the design of what could be implemented to support this
need
http://port389.org/wiki/Access_control_on_trees_specified_in_MODDN_operation

regards
thierry



Since this not related to any Red Hat internal or customer 
information, we should move this discussion to the 389-devel list.



Hi Thierry,

Your design looks good.  A minor question.  The doc does not mention 
about "deny".  For instance, in your example DIT, can I allow 
"moddn_to" and "moddn_from" on the top "dc=example,dc=com" and deny 
them on "cn=tests".  Then, I can move an entry between cn=accounts 
and staging, but not to/from cn=tests?  Or "deny" is not supposed to 
use there?


In which entry do you set these ACIs?

Do you set
aci: (target="ldap:///cn=staging,dc=example,dc=com";)(version 3.0; acl 
"MODDN from"; allow (moddn_from))

 userdn="ldap:///uid=admin_accounts,dc=example,dc=com"; ;)
in the cn=accounts,dc=example,dc=com entry?

Do you set
aci: (target="ldap:///cn=accounts,dc=example,dc=com";)(version 3.0; acl 
"MODDN to"; allow (moddn_to))

 userdn="ldap:///uid=admin_accounts,dc=example,dc=com"; ;)
in the cn=staging,dc=example,dc=com entry?



Hi Rich,

   Yes that is correct, I forgot to mention where those aci are stored.

   They can be defined  at upper level but with a target rule that
   restrict the scope to the desire subtree, or they can be set
   directly at the subtree level without target rule.

   I updated the document to better describe that
   
http://port389.org/wiki/Access_control_on_trees_specified_in_MODDN_operation#ACI_scope_and_targets

   In that case we want to only allow a given user to move entries from
   staging to production (accounts). My preferred solution would be to add:

 * moddn_from at the entry cn=staging,dc=example,dc=com (without
   target rule)
 * moddn_to at the entry cn=accounts,dc=example,dc=com (without
   target rule).

regards
thierrry



Thanks,
--noriko




--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel




--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel


--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review (take #2) ticket 47676: Replication of the schema fails 'master branch' -> 1.2.11 or 1.3.1

[thierry bordaz]

In the previous review, the policies (to be applied during replication 
of the schema) were hardcoded.
This new review, is to define them into specific entries under 
'cn=replSchema,cn=config" so that it will be configurable.


https://fedorahosted.org/389/attachment/ticket/47676/0002-Ticket-ticket47676-Replication-of-the-schema-fails-m.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

Re: [389-devel] Please review: various lib389 cleanups

[thierry bordaz]

On 11/21/2013 04:22 PM, Rich Megginson wrote:

On 11/21/2013 01:58 AM, thierry bordaz wrote:

Hi,

The changes look  very good.

I have a question regarding start/stop in Replica class. Why do not 
you use the function self.agreement.schedule(agmdn, interval='start') 
and self.agreement.schedule(agmdn, interval='stop') ?


I will change it to use schedule().



about the function 'agreement_dn(basedn, other)' why not putting it 
into the Agreement class ?
Note that it uses the functions 'agreements' that is Replica but I 
would expect it to be in Agreement class as well (renamed in 'list' ?).


Yes.  I think I will change the names also, to pause() and resume().

I think the following methods should be moved to the Agreement class: 
check_init start_and_wait wait_init start_async keep_in_sync 
agreement_dn (rename to "dn") agreements (rename to "list") (also, is 
it a problem that we have a method name "list" that is the same as a 
python keyword/built-in?)


Hi Rich,

Yes that is good. When fixing https://fedorahosted.org/389/ticket/47590 
I noticed that with the new Agreement class these functions also needed 
to be moved. I opened https://fedorahosted.org/389/ticket/47600 for 
that. 'dn' and 'list' are very good.


When eclipse was complaining with names (function or variable) same as 
built-in keyword, I tried to change the name. Now I like 'list' name and 
it was not a problem with self.replica.list(), so I would vote to use 
'list'.


regards
thierry




Regards
thierry

On 11/21/2013 03:21 AM, Rich Megginson wrote:




--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel






--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review ticket 47595 (lib389) - fail to detect/reinit already existing instance/backup

[thierry bordaz]

https://fedorahosted.org/389/attachment/ticket/47595/0001-Ticket-47595-fail-to-detect-reinit-already-existing-.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

Re: [389-devel] Please review: various lib389 cleanups

[thierry bordaz]

On 11/21/2013 05:34 PM, Rich Megginson wrote:

On 11/21/2013 08:36 AM, thierry bordaz wrote:

On 11/21/2013 04:22 PM, Rich Megginson wrote:

On 11/21/2013 01:58 AM, thierry bordaz wrote:

Hi,

The changes look  very good.

I have a question regarding start/stop in Replica class. Why do not 
you use the function self.agreement.schedule(agmdn, 
interval='start') and self.agreement.schedule(agmdn, interval='stop') ?


I will change it to use schedule().



about the function 'agreement_dn(basedn, other)' why not putting it 
into the Agreement class ?
Note that it uses the functions 'agreements' that is Replica but I 
would expect it to be in Agreement class as well (renamed in 'list' ?).


Yes.  I think I will change the names also, to pause() and resume().

I think the following methods should be moved to the Agreement 
class: check_init start_and_wait wait_init start_async keep_in_sync 
agreement_dn (rename to "dn") agreements (rename to "list") (also, 
is it a problem that we have a method name "list" that is the same 
as a python keyword/built-in?)


Hi Rich,

Yes that is good. When fixing 
https://fedorahosted.org/389/ticket/47590 I noticed that with the new 
Agreement class these functions also needed to be moved. I opened 
https://fedorahosted.org/389/ticket/47600 for that. 'dn' and 'list' 
are very good.


When eclipse was complaining with names (function or variable) same 
as built-in keyword, I tried to change the name. Now I like 'list' 
name and it was not a problem with self.replica.list(), so I would 
vote to use 'list'.

New patch attached


Yes pause/resume  always/never are good names.
ack.

regards
theirry




regards
thierry




Regards
thierry

On 11/21/2013 03:21 AM, Rich Megginson wrote:




--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel










--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review bug 47586: CI tests: test case for 47490

[thierry bordaz]

This second review takes differs from the previous one with:

 * use common.py instead of _common.py
 * initialization of the topology. This has been widely rewritten from
   previous review in order to run whatever it exists or not instance
   (started or not) and exists backups.

https://fedorahosted.org/389/attachment/ticket/47586/0002-Ticket-47586-CI-tests-test-case-for-47490.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review ticket 47590 (take #3): add/split functions around replication

[thierry bordaz]

This review takes into account the recommendations of previous review:

 * Create a Agreement class in brooker
 * mv init/status/schedule/create in that new class
 * mv createDefaultReplMgr into the brooker replica class with the
   function create_repl_manager()
 * Handling of error condition with exceptions

What is not implemented in that review, that will be implemented with 
https://fedorahosted.org/389/ticket/47600:


 * Cleanup of createAgreement (use of exception). This function will
   likely be replace by agreement.create().
 * Cleanup of enableReplication (use of exception, move under
   replica.create())

https://fedorahosted.org/389/attachment/ticket/47590/0003-Ticket-47590-CI-tests-add-split-functions-around-rep.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review bug 47586: CI tests: test case for 47490

[thierry bordaz]

https://fedorahosted.org/389/attachment/ticket/47586/0001-Ticket-47586-CI-tests-test-case-for-47490.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review ticket 47590: add/split functions around replication

[thierry bordaz]

In order to implement the first CI test with replication instances, I 
reorganised lib389 functions  related to replication setup.


https://fedorahosted.org/389/attachment/ticket/47590/0001-Ticket-47590-CI-tests-add-split-functions-around-rep.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review lib389 ticket 47584 (take #4): CI tests: add backup/restore of an instance

[thierry bordaz]

This review takes into account the following recommendations

 * skip verbose option (use of log.level).
 * use glob.glob to retrieve files matching rather that os.walk
 * fix a bug in instanceBackupFS that did not return an already
   existing backup
 * add two functions to handle instance backup: clearInstanceBackupFS
   (to remove one or all backups of an instance), _infoInstanceBackupFS
   (just to keep path/pattern info of backup into a single routine)
 * do not hard code '/tmp' as directory to store the backups. A new
   optional argument 'backupdir' for createInstance, sets the attribute
   'dirsrv.backupdir'. By default it takes '/tmp'. The caller of
   createInstance (the test case), may provide 'backupdir' from an
   environment variable (like it was done with $PREFIX)

https://fedorahosted.org/389/attachment/ticket/47584/0004-Ticket-47584-CI-tests-add-backup-restore-of-an-insta.patch


--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review lib389 ticket 47584 (take #3): CI tests: add backup/restore of an instance

[thierry bordaz]

This review differs from previous with:

 * skip verbose option (use of log.level).
 * use glob.glob to retrieve files matching rather that os.walk
 * fix a bug in instanceBackupFS that did not return an already
   existing backup
 * add two functions to handle instance backup: clearInstanceBackupFS
   (to remove one or all backups of an instance), _infoInstanceBackupFS
   (just to keep path/pattern info of backup into a single routine)

https://fedorahosted.org/389/attachment/ticket/47584/0003-Ticket-47584-CI-tests-add-backup-restore-of-an-insta.patch

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

Re: [389-devel] Please review lib389 ticket 47584 (take #2): CI tests: add backup/restore of an instance

[thierry bordaz]

Hi Roberto,

   Yes absolutely this is a better choice !! thanks for the tips, I
   will change the fix

regards
thierry

On 11/06/2013 06:35 PM, Roberto Polli wrote:

You may consider replacing the whole function checkInstanceBackupFS
with the simpler:
  backup_pattern = os.path.join(backupDir, "backup*.tar.gz")
  return glob.glob(backup_pattern)

It should return an empty list in case of unexistent path

On Wednesday 06 November 2013 18:31:40 thierry bordaz wrote:

Thanks Roberto and Rich for reviewing this.
This second patch takes into account your recommendation to let the
logging mechanism to choose rather that to use 'verbose' flag.

https://fedorahosted.org/389/attachment/ticket/47584/0002-Ticket-47584-CI-te
sts-add-backup-restore-of-an-insta.patch


--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review lib389 ticket 47584 (take #2): CI tests: add backup/restore of an instance

[thierry bordaz]

Thanks Roberto and Rich for reviewing this.
This second patch takes into account your recommendation to let the 
logging mechanism to choose rather that to use 'verbose' flag.


https://fedorahosted.org/389/attachment/ticket/47584/0002-Ticket-47584-CI-tests-add-backup-restore-of-an-insta.patch
--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review lib389 ticket 47578 (take #2): removal of 'sudo' and absolute path in lib389

[thierry bordaz]

This review enhances the previous fix with the handling of non SELinux 
platforms.


https://fedorahosted.org/389/attachment/ticket/47578/0002-Ticket-47578-CI-tests-removal-of-sudo-and-absolute-p.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review (take #2) ticket 47575: add test case for ticket47560

[thierry bordaz]

Hi All,

   This review is new proposal to implement CI test cases inside 389-DS.
   The layout of the tests will be:

   /dirsrvtests/
tickets/
ticket_abc_test.py
ticket_xyz_test.py

...
   finalizer

testsuites/
acl_test.py
replication_test.py
...

   Each ticket_xyz_test.py will contain "framework" functions that will
   allocate a fresh installed instance then the test functions:
   "framework" functions are _ds_create_standalone, DSInstance class
   and ds_instance pytest fixture.
   tests functions are named 'test_ticket_xyz' with 'ds_instance'
   fixture as argument.

   py.test will call the test functions 'test_ticket_xyz'. Its argument
   is an instance (ds_instance) of DSInstance where
   ds_instance.instance is a connection to the instance.

   The first instance of DSInstance (first ticket being tested), get a
   newly created instance (e.g. slapd-standalone) and creates a backup
   of the instance (under /tmp/slapd-standalone.bck/backup_HHMMSS.tar.gz).
   Then when running others  ticket test cases, the instance is
   reinitialized from the backup.
   So each ticket test case will have an instance almost like it was
   just created.

   py.test will discover all the ticket_xxx_test.py files and run them.
   In order to remove the created instances, py.test is called a second
   time to run finalizer.py.
   So a jenkins job script, will do something like:

   cd /dirsrvtests/tickets
   py.test -v   # that will run all the ticket_xxx_tests and create
   the instance
   py.test -v finalizer.py


   Note: in order to check/backup/restore instance, new functions are
   added in lib389 (
   checkInstanceBackupFS, instanceBackupFS, instanceRestoreFS), I will
   send an other review for them as it is not in 389-DS repos.

https://fedorahosted.org/389/attachment/ticket/47575/0002-Ticket-47575-CI-test-add-test-case-for-ticket47560.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

Re: [389-devel] Please review lib389 ticket 47578: removal of 'sudo' and absolute path in lib389

[thierry bordaz]

Ok I realise it is not a good idea. I will reimplement it and send an 
other review :-)

Thanks for all your feedbacks.

thierry

On 10/30/2013 08:11 PM, Rich Megginson wrote:

On 10/30/2013 12:56 PM, Jan Rusnacko wrote:

Hello Thierry,

layout OK.

As for tests - instead of reinventing the wheel by defing class 
Test_standAlone

to set up instance, use py.test fixture.

+1


Also, you should not force setup, test, teardown execution for each 
test by
specifying sub-methods for each test. Testing framework (py.test) 
should be
doing that. I think this will make your tests fail badly if some 
exception
occurs - if _test_ticket47560_setup raises exception, it will 
propagate back to

py.test and cleanup method will never be executed for that ticket.

+1


Also, I believe each ticket should have its own file which contains 
one or more

testcases. I think that would reasonably group relevant things together.

+1


On 10/30/2013 05:57 PM, thierry bordaz wrote:
https://fedorahosted.org/389/attachment/ticket/47578/0001-Ticket-47578-CI-tests-removal-of-sudo-and-absolute-p.patch 





--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel


--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel




--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

Re: [389-devel] Please review lib389 ticket 47578: removal of 'sudo' and absolute path in lib389

[thierry bordaz]

On 10/30/2013 07:56 PM, Jan Rusnacko wrote:

Hello Thierry,

layout OK.

As for tests - instead of reinventing the wheel by defing class Test_standAlone
to set up instance, use py.test fixture.

Also, you should not force setup, test, teardown execution for each test by
specifying sub-methods for each test. Testing framework (py.test) should be
doing that. I think this will make your tests fail badly if some exception
occurs - if _test_ticket47560_setup raises exception, it will propagate back to
py.test and cleanup method will never be executed for that ticket.

Hi Jan,

thanks for you comments.
py.test will call for for each test_ticketxxx

   setup
   test_ticketxxx
   teardown


setup will create the instance if it does not already exist, else it 
provide a dirsrv to it.
teardown will remove the instance if the test_ticketxx is not able to 
properly clean it up (if test_ticketxx raise the clean_please flag)
test_ticketxxx will start it executions with clean_please=true (set by 
setup), if it succeeds it set clean_please=false. Unless it fails to 
properly clean up the instance. In that case it sets clean_please to true.


What would be the advantages to make the setup/teardown sub-methods of 
the test_ticketxx ?
At least a "small" drawback is that the person that write test_ticketxxx 
will have to write them, instead of using the "standard" one.




Also, I believe each ticket should have its own file which contains one or more
testcases. I think that would reasonably group relevant things together.


Right, it was my first intention. But then I had this idea to group the 
tickets per deployment module.

I don't know if it is a good idea, but it seems to be confusing :-[


On 10/30/2013 05:57 PM, thierry bordaz wrote:

https://fedorahosted.org/389/attachment/ticket/47578/0001-Ticket-47578-CI-tests-removal-of-sudo-and-absolute-p.patch



--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel



--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

Re: [389-devel] Please review lib389 ticket 47575: add test case for ticket47560

[thierry bordaz]

On 10/30/2013 06:59 PM, Rich Megginson wrote:

On 10/30/2013 10:47 AM, thierry bordaz wrote:

Hello,

This tickets implement a test case and propose a layout of the CI
tests in the 389-ds.
The basic idea is to put CI tests under:
/dirsrvtests/
tickets/
standalone_test.py
m1c1_test.py
m2_c1_test.py
...


Does "tickets" in this case mean "tickets for issues in the 389 trac"?

Yes in my mind, this directory would contains test cases for 389 tickets.



testsuites/
acl_test.py
replication_test.py
...

For example, test_standalone.py would setup a standalone topology
and will contain all ticket test cases that are applicable on
standalone topology.

https://fedorahosted.org/389/attachment/ticket/47575/0001-Ticket-47575-CI-test-add-test-case-for-ticket47560.patch


So we would just keep adding tests to the single file 
standalone_test.py, every time we add a test for a trac ticket that 
deals with a standalone server?

Yes, if we have a test case for a ticket_xyz, we may add a new class method

   class Test_standAlone(object):
def setup(self):
...
def teardown(self):
...

def test_ticket_xyz(self):
def _test_ticket_xyx_setup():
   
def _test_ticket_xyz_teardown():


_test_ticket_xyz_setup()



_test_ticket_xyz_teardown()





def test_ticket_abc(self)
...

def test_final(self)










regards
thierry


--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel




--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

Re: [389-devel] Proof of concept: mocking DS in lib389

[thierry bordaz]

On 10/29/2013 02:18 PM, Jan Rusnacko wrote:

Hello Thierry,

I am not rewriting ldapadd,...  methods of "real" DS class, I am in fact
creating MockDS class with custom ldapadd,... methods, _just_ like you suggest 
:)

Furthermore, you can view it as a subclass of "real_ds" - even though it is not
a proper Python subclass, it inherits all functions from repl module just like
"real_ds" would (again through ModuleProxy mechanism). So, methods that are
defined in repl are the same for "real_ds" class and for MockDS class, but
ldap.. methods are different. So, basically exactly what you suggest :)


Hi Jan,

   Sorry my question was not clear. For example an other approach could be

   Class DSInstance (object):
def __init__(self):
...

def ldapadd_r(self, input):
# real call to pythonldap.add_s

   Class MockDSInstance(DSInstance):
def __init__(self):
...

def ldapadd_r(self, input):
input = input.strip()
entry = dict(e.strip().split(': ') for e in
   input.split('\n'))
self.dit[entry['dn']] = entry



   My understanding is that both approach would allow us to call
   inherited methods, just ldap method are different.
   What are the advantages of the approach you described compare to the
   one above ?

best regards
thierry


Code of the whole class along with all methods is in file
tests/test_dsmodules/conftest.py line 7.

Thank you,
Jan

On 10/28/2013 12:02 PM, thierry bordaz wrote:

Hi Jan,

 That is very impressive POC, far above my skill in python. Thanks for
 sharing this.
 I have a novice question.
 This implementation overwrites the basic ldapadd,ldapsearch... function of
 the "real" DS.
 An other approach is to write a 'mock_ds' class being a subclass of
 'real_ds' and to overwrite the ldapadd,ldapsearch in mock_ds class (to 
store
 data into a dict). What would be the advantages of your approach ?

best regards
thierry

On 10/25/2013 09:36 PM, Jan Rusnacko wrote:

Hello Roberto and Thierry,

as I promised, I am sending you a proof-of-concept code that demonstrates, how
we can mock DS in unit tests for library function (see attachment). You can run
tests just by executing py.test in tests directory.

Only 3 files are of interest here:

lib389/dsmodules/repl.py - this is a Python module with functions - they expect
DS instance as the first argument. Since they are functions, not methods, I can
just mock DS and pass that fake one as the first argument to them in unit tests.

tests/test_dsmodules/conftest.py - this file contains definition of mock DS
class along with py.test fixture, that returns it.

tests/test_dsmodules/test_repl.py - this contains unit tests for functions from
repl.py.

What I do is quite simple - I override ldapadd, ldapdelete .. methods of mock DS
class, so that instead of sending command to real DS instance, they just store
the data in 'dit' dictionary (which represents content stored in DS). This way,
I can check that when I call e.g. function enable_changelog(..), in the end DS
will have correct changelog entry.

To put it very bluntly - enable_changelog(..) function just adds correct
changelog entry to whatever is passed to it as the first argument. In unit
tests, it is mock DS, otherwise it would be real DS class that sends real ldap
commands to real DS instance behind.

Now I can successfully test that enable_changelog really works, without going
into trouble defining DSInstance or ldap calls at all. Also, I believe this
approach would work for 95% of all functions in lib389. Another benefit is that
unit tests are much faster, than on real DS instance.

Sidenote: even though everything is defined in separate namespace of 'repl'
module as function, in runtime they can be used as normal methods of class
DSInstance. That is handled by DSModuleProxy. We already went through this, but
not with Roberto.

Hopefully, now with some code in our hands, we will be able to understand each
other on this 'mocking' issue and come to conclusions more quickly.

Let me know what you think.

Thank you,
Jan


--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

Re: [389-devel] Please review lib389 ticket 47568: Rename DSAdmin class (2nd)

[thierry bordaz]

On 10/25/2013 11:38 AM, Roberto Polli wrote:

On Friday 25 October 2013 11:18:53 thierry bordaz wrote:

lib389/brooker.py:795: python variable naming convention: I would get
stick
with the "_" instead of camelCase  and change whenever possible.

If you prefer to use '_' also for local variable, I am fine.

Using camel just for classes is more explicative, and I find that "_"  are
easier to read and replace with sed ;)


tests/dsadmin_test.py: I renamed it lib389_test.py, you can merge my
changes tests/dsadmin_test.py:39: why remove the addbackend_harn?

Humm, to be honest... I do not know how to rename files :-)

git mv dsadmin_test.py lib389_test.py ;)

:-[ !! so easy :-D





tests/replica_test.py:119: you're using Backend.delete in a class that
should test just Replica. I would use harness and the standard
python-ldap methods in setup/teardown, so that we can change the Backend
and Replica class without at least breaking the tests.

I miss your point. It is calling in teardown conn.backend.delete, is
that the call that is not correct ?

That's just an IMHO: see those cases:
1- I change the Backend class and break the replica test: I'll look for errors
in Replica while the issue is in Backend
2- somebody works on the Backend class, I work on the Replica one: he can
break my tests.

Splitting the test stuff in an harness module will reduce the impact of all
that. As an example, I could even agree the setup process be done populating
entries via an LDIF. If I test Replica, Backend or Suffix I shouldn't have
other dependencies distracting me.


Is that related to Mock. For example in Replica, we need a suffix and a 
replica but do not want to rely on them.
If instead of creating a real suffix/backend, we have mock of them we 
could develop the replica tests without any concerns regarding further 
changes in suffix/backend.

Is that your concern ?

regards
thierry

Let me know + Peace,
R.
   



--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review lib389 ticket 47568: Rename DSAdmin class

[thierry bordaz]

https://fedorahosted.org/389/attachment/ticket/47568/0001-Ticket-47568-Rename-DSAdmin-class.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review Ticket 47566: Initial import of DSadmin into lib389 repos

[thierry bordaz]

lib389 implements a python library for 389-ds administrative operations. 
This review is the push of DSadmin (

https://github.com/richm/dsadmin ) into lib389 with few adaptation fixes.

https://fedorahosted.org/389/attachment/ticket/47566/0001-Ticket-ticket47566-Initial-import-of-DSadmin-into-38.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

Re: [389-devel] new python management library for 389

[thierry bordaz]

On 10/18/2013 10:14 AM, Roberto Polli wrote:

Hi @all,

I've just check out https://git.fedorahosted.org/cgit/389/lib389.git/ and I'll
check the layout nkinder set up.

Then I'll ask you how to import the first tests using py.test.


Peace,
R:

Hi Roberto,

   After some differents evaluations we found dsadmin to be most
   advanced implemented of what will be lib389.
   I opened ticket https://fedorahosted.org/389/ticket/47566 to import
   dsadmin in lib389 to be our base lib.
   I checkout dsadmin branch, merged locally
   https://github.com/richm/dsadmin/pull/5 and did some tests.
   It worked very well but I had to do some slight modifications to
   create master and allow instance removal.
   I was about to push it into lib389. An other possibility is that I
   do a pull request on dsadmin for those modifications (or send you
   the patch) and we will populate lib389 from dsadmin after. Just let
   me know what is your preferred option.

   lib389 is really at a starting phase. Jan and I have been testing
   differents ideas and finally choose to use pytest framework.  Jan
   implemented a mechanism to catch any IOs (from to print to
   subprocess print) into a logger and that could be an enhancement
   bring to lib389.

best regards
thierry
--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review ticket 47398: memberOf on a user is converted to lowercase

[thierry bordaz]

https://fedorahosted.org/389/attachment/ticket/47398/0002-Ticket-47398-memberOf-on-a-user-is-converted-to-lowe.patch

https://fedorahosted.org/389/ticket/47398
--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review 2nd (fix regression) ticket 512: improve performance of vattr code

[thierry bordaz]

This review takes into account remarks done by Ludwig and Rich

https://fedorahosted.org/389/attachment/ticket/512/0006-Ticket-512-improve-performance-of-vattr-code.patch

https://fedorahosted.org/389/ticket/512
--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] Please review ticket 512: improve performance of vattr code

[thierry bordaz]

https://fedorahosted.org/389/ticket/512

https://fedorahosted.org/389/attachment/ticket/512/0001-Ticket-512-improve-performance-of-vattr-code.patch
--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] please review: Ticket 618 - Crash at shutdown while stopping replica agreements

[thierry bordaz]

https://fedorahosted.org/389/ticket/618

https://fedorahosted.org/389/attachment/ticket/618/0001-Ticket-618-Crash-at-shutdown-while-stopping-replica-.patch
--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] please review: Ticket 616 - High contention on computed attribute lock

[thierry bordaz]

https://fedorahosted.org/389/ticket/616

https://fedorahosted.org/389/attachment/ticket/616/0001-Ticket-616-High-contention-on-computed-attribute-loc.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel

[389-devel] please review: Ticket 600 - Server should return unavailableCriticalExtension when processing a badly formed critical control

[thierry bordaz]

https://fedorahosted.org/389/ticket/600

https://fedorahosted.org/389/attachment/ticket/600/0001-Ticket-600-Server-should-return-unavailableCriticalE.patch 

--
389-devel mailing list
389-de...@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/389-devel