[Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Petr Spacek]

On 13.3.2015 11:34, Jan Cholasta wrote:
> Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):
>> On 03/13/2015 11:00 AM, Petr Spacek wrote:
>>> On 13.3.2015 10:42, Alexander Bokovoy wrote:
>>>> On Fri, 13 Mar 2015, Petr Spacek wrote:
>>>>> On 13.3.2015 10:18, Martin Kosek wrote:
>>>>>> On 03/12/2015 05:10 PM, Rob Crittenden wrote:
>>>>>>> Petr Spacek wrote:
>>>>>>>> On 12.3.2015 16:23, Rob Crittenden wrote:
>>>>>>>>> David Kupka wrote:
>>>>>>>>>> On 03/06/2015 06:00 PM, Martin Basti wrote:
>>>>>>>>>>> Upgrade plugins which modify LDAP data directly should not be
>>>>>>>>>>> executed
>>>>>>>>>>> in --test mode.
>>>>>>>>>>>
>>>>>>>>>>> This patch is a workaround, to ensure update with --test
>>>>>>>>>>> option will not
>>>>>>>>>>> modify any LDAP data.
>>>>>>>>>>>
>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3448
>>>>>>>>>>>
>>>>>>>>>>> Patch attached.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Ideally we want to fix all plugins to dry-run the upgrade not
>>>>>>>>>> just skip
>>>>>>>>>> when there is '--test' option but it is a good first step.
>>>>>>>>>> Works for me, ACK.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I agree that this breaks the spirit of --test and think it
>>>>>>>>> should be
>>>>>>>>> fixed before committing.
>>>>>>>>
>>>>>>>> Considering how often is the option is used ... I do not think
>>>>>>>> that this
>>>>>>>> requires 'proper' fix now. It was broken for *years* so this
>>>>>>>> patch is a huge
>>>>>>>> improvement and IMHO should be commited in current form. We can
>>>>>>>> re-visit it
>>>>>>>> later on, open a ticket :-)
>>>>>>>>
>>>>>>>
>>>>>>> No. There is no rush for this, at least not for the promise of a
>>>>>>> future
>>>>>>> fix that will never come.
>>>>>>
>>>>>> I checked the code and to me, the proper fix looks like instrumenting
>>>>>> ldap.update_entry calls in upgrade plugins with
>>>>>>
>>>>>> if options.test:
>>>>>>     log message
>>>>>> else
>>>>>>     do the update
>>>>>>
>>>>>> right? I see just couple places that would need to be updated:
>>>>>>
>>>>>> $ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
>>>>>> ipaserver/install/plugins/dns.py:
>>>>>> ldap.update_entry(container_entry)
>>>>>> ipaserver/install/plugins/fix_replica_agreements.py:
>>>>>> repl.conn.update_entry(replica)
>>>>>> ipaserver/install/plugins/fix_replica_agreements.py:
>>>>>> repl.conn.update_entry(replica)
>>>>>> ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
>>>>>> ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
>>>>>> ipaserver/install/plugins/update_managed_permissions.py:
>>>>>> ldap.update_entry(base_entry)
>>>>>> ipaserver/install/plugins/update_managed_permissions.py:
>>>>>> ldap.update_entry(entry)
>>>>>> ipaserver/install/plugins/update_pacs.py:
>>>>>> ldap.update_entry(entry)
>>>>>> ipaserver/install/plugins/update_referint.py:
>>>>>> ldap.update_entry(entry)
>>>>>> ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
>>>>>> ipaserver/install/plugins/update_uniqueness.py:
>>>>>> ldap.update_entry(uid_uniqueness_plugin)
>>>>>>
>>>>>>
>>>>>> So from my POV, very quick fix. In that case, I would also prefer a
>>>>>> fix now
>>>>>> than a ticket that would never be done.
>>>>>
>>>>> I really dislike this approach because I consider it flawed by
>>>>> design. Plugin
>>>>> author has to think about it all the time and do not forget to add if
>>>>> otherwise ... too bad.
>>>>>
>>>>> I can see two 'safer' ways to do that:
>>>>> - LDAP transactions :-)
>>>>> - 'mock_writes=True' option in LDAP backend which would print
>>>>> modlists instead
>>>>> of applying them (and return success to the caller).
>>>>>
>>>>> Both cases eliminate the need to scatter 'ifs' all over update
>>>>> plugins and do
>>>>> not add risk of forgetting about one of them when adding/changing
>>>>> plugin code.
>>>> I like idea about mock_writes=True. However, I think we still need to
>>>> make
>>>> sure plugin writers rely on options.test value to see that we aren't
>>>> going to write the data. The reason for it is that we might get into
>>>> configurations where plugins would be doing updates based on earlier
>>>> performed tasks.  If task configuration is not going to be written, its
>>>> status will never be correct and plugin would get an error.
>>>
>>> That is exactly why I mentioned LDAP transactions. There is no other
>>> way how
>>> to test complex plugins which actually read own writes (except mocking
>>> the
>>> whole LDAP interface somewhere).
>>
>> While this may be a good idea long term, I do not think any of us is
>> considering implementing the LDAP transaction support within work on
>> this refactoring.
>>
>> So in this thread, let us focus on how to fix options.test mid-term. I
>> currently see 2 proposed ways:
>> - Making the plugins aware of options.test
>> - Make ldap2 write operations only print the update and not do it.
>> Although thinking of this approach, I think it may make some plugins
>> like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
>>    - search for all unfixed DNS zones
>>    - fix them with ldap update
>>    - was the search truncated, i.e.  are there more zones to update?
>>      if yes, go back to start
> 
>  - Make the plugins not call {add,update,delete}_entry themselves but rather
> return the updates like they should. This is what the ticket
> (<https://fedorahosted.org/freeipa/ticket/3448>) requests and what should be
> done to make --test work for them.

How do you propose to handle iterative updates like the DNS upgrade mentioned
by Martin^1? Return set of updates along with boolean 'call me again'?
Something else?

-- 
Petr^2 Spacek