[Freeipa-devel] [PATCH 0073] Remove support for IPA deployments with no persistent search

[Martin Kosek]

On 07/15/2013 06:28 PM, Simo Sorce wrote:
> On Mon, 2013-07-15 at 16:41 +0200, Petr Spacek wrote:
>> On 15.7.2013 16:15, Simo Sorce wrote:
>>> On Mon, 2013-07-15 at 15:57 +0200, Martin Kosek wrote:
>>>> On 07/15/2013 03:44 PM, Petr Spacek wrote:
>>>>> On 15.7.2013 15:31, Martin Kosek wrote:
>>>>>> On 07/11/2013 05:10 PM, Tomas Babej wrote:
>>>>>>> On Thursday 11 of July 2013 16:10:33 Ana Krivokapic wrote:
>>>>>>>
>>>>>>>> On 07/11/2013 11:20 AM, Tomas Babej wrote:
>>>>>>>
>>>>>>>>> boolean_var = {}
>>>>>>>
>>>>>>>>> - for var in ('persistent_search', 'serial_autoincrement'):
>>>>>>>
>>>>>>>>> + for var in ('serial_autoincrement'):
>>>>>>>
>>>>>>>> This won't work - a one element tuple needs a comma at the end:
>>>>>>>
>>>>>>>> ('serial_autoincrement', )
>>>>>>>
>>>>>>>>> boolean_var[var] = "yes" if getattr(self, var, False) else "no"
>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>>> self.sub_dict = dict(FQDN=self.fqdn,
>>>>>>>
>>>>>>>>> @@ -607,9 +604,8 @@ class BindInstance(service.Service):
>>>>>>>
>>>>>>>>> SUFFIX=self.suffix,
>>>>>>>
>>>>>>>>> OPTIONAL_NTP=optional_ntp,
>>>>>>>
>>>>>>>>> ZONEMGR=self.zonemgr,
>>>>>>>
>>>>>>>>> - ZONE_REFRESH=self.zone_refresh,
>>>>>>>
>>>>>>>>> IPA_CA_RECORD=ipa_ca,
>>>>>>>
>>>>>>>>> - PERSISTENT_SEARCH=boolean_var['persistent_search'],
>>>>>>>
>>>>>>>>> + PERSISTENT_SEARCH="yes",
>>>>>>>
>>>>>>>>> SERIAL_AUTOINCREMENT=boolean_var['serial_autoincrement'],)
>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>> But anyway, I think this piece of code is unnecessarily complicated, I
>>>>>>>> don't see
>>>>>>>
>>>>>>>> a need for the 'boolean_var' dict here. I would suggest replacing it with
>>>>>>>
>>>>>>>> something like:
>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>> serial_autoincrement = "yes" if self.serial_autoincrement else "no"
>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>> and then pass serial_autoincrement to self.sub_dict = dict(...)
>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Attached patch refactored the relevant part of the code.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Tomas
>>>>>>>
>>>>>>
>>>>>> Thanks for patches! I am just thinking, should we also hide the respective
>>>>>> option from ipa global DNS configuration? That's idnszonerefresh attribute.
>>>>>>
>>>>>> We may want to mark the attribute as invisible in CLI + remove it from Web UI.
>>>>>> Petr - what is your take on this? Do you plan to remove idnszonerefresh
>>>>>> attribute support in the future (Fedora 20) as persistent search will be
>>>>>> mandatory in that time?
>>>>>
>>>>> Yes, you are right. We completely forgot to web UI. And yes - please remove the
>>>>> option from web UI.
>>>>
>>>> Ok, Tomas please do the changes as proposed above.
>>>>
>>>>>
>>>>> The latest development shows that persistent search will be replaced by RFC
>>>>> 4533 (known as 'syncrepl'), but from user's point of view it doesn't matter.
>>>>> All options related to persistent search and zone_refresh will simply
>>>>> disappear. Syncrepl itself doesn't require explicit configuration.
>>>>
>>>> Ah, so this means that "psearch" option will be also removed from
>>>> bind-dyndb-ldap? In Fedora 19 we just plan to hard-code it to "yes", will that
>>>> cause issues with Fedora 20? Should we already avoid using the "psearch" option
>>>> and assume that bind-dyndb-ldap in Fedora 19 is using persistent search by default?
>>>
>>> Won't the new bind-dyndb-ldap simply ignore the psearch option when it
>>> moves to syncrepl ?
>>
>> I can do it, but I think that cleanest way is to remove the 'psearch' option 
>> in upgrade script.
> 
> Sure, but if the upgrade, for whatever reason, fails to remove it
> bind-dyndb-ldap should just ignore.
> 
>> Another option is to release new bind-dyndb-ldap to Fedora 19 and change 
>> default values to 'psearch yes' right now. Do you agree?
> 
> Too much churn, I think it is ok to change it when we are done with
> syncrepl and upgrade config, with the fallback failsafe that even if
> upgrade doesn't remove the option bind-dyndb-ldap will simply ignore it.
> 
> This will be safer even for people using stuff like cfengine/puppet to
> manage configurations and not realizing we changed the conf on upgrade,
> their confsystems will push again a conf file with psearch yes but
> bnid-dyndb-ldap won't break.
> 
> Simo.
> 

Hmm, that's right, it should be safer. Can bind-dyndb-ldap just yell to error
log that there is an unknown configuration option? (if it does not do that
already).

Martin