[Freeipa-devel] [PATCH 0159-0160] emancipate IPA NTP service into role

[Martin Basti]

On 15.06.2016 13:29, Petr Spacek wrote:
> On 15.6.2016 09:57, Martin Basti wrote:
>>
>> On 15.06.2016 09:55, Petr Vobornik wrote:
>>> On 06/14/2016 07:28 PM, Martin Basti wrote:
>>>> On 14.06.2016 18:58, Martin Babinsky wrote:
>>>>> On 06/14/2016 05:06 PM, Martin Basti wrote:
>>>>>> On 12.06.2016 17:37, Martin Babinsky wrote:
>>>>>>> These two patches turn oft-neglected ntp service into a full fledged
>>>>>>> role whose status can be queried centrally. They should also enable
>>>>>>> generation of location-specific _ntp._udp records.
>>>>>>>
>>>>>>> Please note that NTP is LDAP-enabled by additional call after DS
>>>>>>> instance is configured. I was not feeling confident by swapping NTP
>>>>>>> and DS configuration steps as I was afraid it will break things. If
>>>>>>> not, I will happily update the patch accordingly.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/5815
>>>>>>> https://fedorahosted.org/freeipa/ticket/5826
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> Hello, I have a few comments:
>>>>>>
>>>>>> Patch: 159
>>>>>> 1)
>>>>>> +    if ntp.is_configured():
>>>>>> +        ntp.ldap_enable('NTP', fqdn, None, base_dn)
>>>>>> +        ntp.enable()
>>>>>>
>>>>>> All ipa services are in disabled state, ipactl starts them according
>>>>>> configuration in LDAP
>>>>>> IMO it should be something like:
>>>>>> ntp.disable()
>>>>>> if running:
>>>>>>       ntp.start()
>>>>>>
>>>>>> 2)
>>>>>> could you upgrade NTP only once in upgrade.py? Use sysupgrade state
>>>>>>
>>>>>> 3)
>>>>>> +    'NTP': ('ntpd', 42),
>>>>>> I prefer 45, it is easier to put any service before NTP if needed
>>>>>> without huge renumbering
>>>>>>
>>>>>>
>>>>>> Patch 160: LGTM
>>>>>>
>>>>>> Martin^2
>>>>>>
>>>>>>
>>>>> Right, attaching updated patches.
>>>>>
>>>> Patches are good, but I'm curious if there is any chance for NTP to be
>>>> able synchronize time before replication on replica install. If no, IMO
>>>> better is to move NTP service configuration after dirserver to be able
>>>> to configure LDAP entry directly.
>>>> But if there is not time for this, I'm fine with opening ticket and
>>>> fixing it later.
>>>>
>>>> Martin^2
>>> Isn't it already done during Client part of replica installation?
>>>
>> With domain level 1 yes, with domain level 0, no.
> ACK to current version of the patch.
>
> Please keep the order as it is (NTP first) because it ensures proper time sync
> even on domain level 0.
>
master:
* 567f00a59c53aca760336aea95423368ac621032 Add NTP to the list of 
services stored in IPA masters LDAP subtree
* 3e6af238bb695572e462ff49a3096ab0e2e85bc5 Introduce "NTP server" role