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

[Martin Basti]

On 13/03/15 12:50, Jan Cholasta wrote:
> 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
>
Updated patch attached.

-- 
Martin Basti

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0208.2-Server-Upgrade-respect-test-option-in-plugins.patch
Type: text/x-patch
Size: 15371 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150313/f0d2f36e/attachment.bin>