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

[Martin Kosek]

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