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

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

Reply via email to