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

[Jan Cholasta]

Dne 13.3.2015 v 12:08 Petr Spacek napsal(a):
> On 13.3.2015 12:01, Martin Basti wrote:
>> On 13/03/15 11:55, Petr Spacek wrote:
>>> 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?
>>>
>> So instead of DNS commands logic, which can be used in plugin, we should
>> reimplement the dnzone commands in upgrade plugin, to get modlist? And keep
>> watching it and do same modifications for upgrade plugin as are done in DNS
>> plugin.

Yes, this is how it currently works and how it is done in other update 
plugins. For proper command support some serious changes will have to be 
done in the framework.

>
> Again, I think we are investing to much effort into an option which is almost
> never used. Let it die in Git history. After all, it can be revived at any
> time when needed or when we have proper support in DS.
>

+1

-- 
Jan Cholasta