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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code