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

[Rob Crittenden]

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.

I always intended that some plugins would need to directly write to LDAP
(or via internal commands). This is in the spirit of "you can't think of
everything" and providing enough future flexibility that we can get out
of a sticky situation.

>>
>> 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

More effort is being spent trying to kill the option than it would take
to fix the usage.

rob