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

[Rob Crittenden]

Petr Viktorin wrote:
> 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
>

I left it in for now. Updated patch attached.

rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1088-4-dnarange.patch
Type: text/x-diff
Size: 30356 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130311/0fcdbc7d/attachment.bin>