[Freeipa-devel] [PATCH] 1088 Recover DNA ranges when deleting a master

[Petr Viktorin]

On 03/07/2013 08:27 PM, Rob Crittenden wrote:
> Petr Viktorin wrote:
>> On 03/06/2013 09:52 PM, Rob Crittenden wrote:
>>> Petr Viktorin wrote:
>> [...]
>>>> On new installs, the ACI on cn=Posix IDs,cn=Distributed Numeric
>>>> Assignment Plugin,cn=plugins,cn=config is added before the entry
>>>> itself.
>>>> I didn't test everything as I didn't get the access.
>>>
>>> It shouldn't make a difference. What isn't working?
>>
>> I get a CRITICAL message in the log:
>>
>> add aci:
>>          (targetattr=dnaNextRange || dnaNextValue ||
>> dnaMaxValue)(version 3.0;acl "permission:Modify DNA Range";allow (write)
>> groupdn = "ldap:///cn=Modify DNA
>> Range,cn=permissions,cn=pbac,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com";)
>>
>>
>> modifying entry "cn=Posix IDs,cn=Distributed Numeric Assignment
>> Plugin,cn=plugins,cn=config"
>>
>> 2013-03-07T11:01:48Z DEBUG stderr=ldap_initialize(
>> ldap://vm-081.idm.lab.eng.brq.redhat.com:389/??base )
>> ldap_modify: No such object (32)
>>
>> 2013-03-07T11:01:48Z CRITICAL Failed to load replica-acis.ldif: Command
>> '/usr/bin/ldapmodify -v -f /tmp/tmpT55upM -H
>> ldap://vm-081.idm.lab.eng.brq.redhat.com:389 -x -D cn=Directory Manager
>> -y /tmp/tmplFeere' returned non-zero exit status 32
>>
>
> Gotcha. I moved where the replica acis are loaded.

Please attach the patch :)

[...]
>>>>> +        failed = 0
>>>>> +        for ent in entries:
>>>>
>>>>
>>>> This loops more than necessary and is somewhat hard to follow. Consider
>>>> using for-else here:
>>>>
>>>> for ...:
>>>>      ...
>>>>      if okay:
>>>>          break
>>>> else:
>>>>      raise error
>>>
>>> I simplified things a bit but a for/else won't work here as we need to
>>> check all ranges all the time. It is perfectly fine to not fit into a
>>> range, as long as it fits into SOME range.
>>
>> Well, that's how for's (not if's) else clause works -- it's executed
>> after all the looping's done if you didn't `break` out.
>> http://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statements-and-else-clauses-on-loops
>>
>>
>> Maybe I'm just used to it and it's too esoteric to the average reader,
>> though.
>
> Thanks for the vote of confidence. Like I said, I wanted it to check all
> the ranges. A for/else quits on the break, which I guess is really
> probably ok assuming we trust that nothing else is going to stuff bad
> ranges in. I can go ahead and make the change.

Your code also breaks out as soon as a good range is found.
Anyway this is a small nitpick; the loop works fine as it is.

>> [...]
>>> Ok, I'll drop this since it doesn't affect things with the new LDAP
>>> backend.
>>
>> Please do, you left it in by mistake.
>
> Yeah, there it is sitting unsquashed in my tree :-(
 >
>>> I also added one change related to the LDAP core changes. In the past if
>>> you did not have a ticket it would prompt for DM password. This was
>>> broken after the updates. I added an additional except in
>>> test_connection().
>>
>> This should also be fixed soon in ipaldap. Thanks for putting up with
>> the changes.
>>
>
> So should I drop this in my patch then? I don't really like having to
> import ldap.

You can if you're fine with waiting until my patches are pushed.
Otherwise it's covered by https://fedorahosted.org/freeipa/ticket/3499

-- 
Petr³