CA-369899: Add NTP options DHCP|Default|Manual|None #35

GeraldEV · 2024-03-13T10:04:27Z

These changes implement NTP control in xsconsole to match the NTP options available in the host-installer

To keep the xsconsole time up to date, the console must use monotonic time (via ncurses) to ensure the console doesn't wait for any negative timesteps before updating (there is an open PR for this behaviour in XenServer) (the console will also update the time upon receiving user input if monotonic time is not used)

Also remove the testing option for the NTP feature as ntpstat is no longer present.

DHCP - Use DHCP server as NTP server
Default - Use the default centos pool NTP servers
Manual - Use manually specified NTP servers
None - Set time manually, no NTP

Avoids authentication timeouts and session timeouts when the time jumps forward, either by chrony (NTP) or manual time changes

ntpstat is no longer present by default on installations

XSConsoleData.py

bernhardkaindl

I reviewed this PR and found no issue, only suggestions.

My suggestions are not of functional nature. They are only of cosmetic preference and some optional improvements for consistent PEP8 4-character indentation and have no urgency or need.

Therefore, I'm submitting my review as an Approval!

Signed-off-by: Bernhard Kaindl <[email protected]>

Co-authored-by: Bernhard Kaindl <[email protected]>

XSConsoleData.py

plugins-base/XSFeatureNTP.py

XSConsoleData.py

GeraldEV · 2024-03-19T10:01:22Z

Pushed a squashed commit to resolve(?) the precommit issues (removed empty line at the end of XSConsoleConstants.py)

bernhardkaindl · 2024-03-19T11:03:51Z

XSConsoleData.py

+            self.data['ntp']['method'] = "Manual"
+
+            servers = self.data['ntp']['servers']
+            if len(servers) == 4 and all(


Some coding standards consider the use of magic numbers a defect, I tried to get some references, because I don't understand the meaning of 4 in this context:

Today, I got feedback from a code review and learned something new. It was about "magic numbers", which are unique values with unexplained meaning or multiple occurrences which could (preferably) be replaced with named constants.
https://dev.to/anthonyjdella/magic-numbers-in-programming-19h0

Initially, skipped adding a review comment about it, but now I recognize that the meaning 4 in this case is not obvious to me. Hence, it may deserve an explanatory comment or (and?) the use of a symbolic constant (if that is sufficient)

bernhardkaindl · 2024-03-19T11:16:28Z

XSConsoleData.py

+        Auth.Inst().AssertAuthenticated()
+
+        Data.Inst().NTPServersSet(
+            ["%d%s" % (i, DEFAULT_NTP_DOMAINS[0]) for i in range(4)]


Here, I see the magic number 4 again, it is more clear here.
Here, [0] in DEFAULT_NTP_DOMAINS[0] looks like another non-obvious number to me.

Checking above, I see it means ".centos.pool.ntp.org", so the code would be more obvious to read this way:

CENTOS_NTP_POOL_DOMAIN = ".centos.pool.ntp.org" DEFAULT_NTP_DOMAINS = [CENTOS_NTP_POOL_DOMAIN, ".xenserver.pool.ntp.org"]

["%d%s" % (ntp_host, CENTOS_NTP_POOL_DOMAIN) for ntp_host in range(4)]

Or it could be nice to move the "." to the format string of the host:

CENTOS_NTP_POOL_DOMAIN = "centos.pool.ntp.org" DEFAULT_NTP_DOMAINS = [CENTOS_NTP_POOL_DOMAIN, "xenserver.pool.ntp.org"]

["%d.%s" % (ntp_host, CENTOS_NTP_POOL_DOMAIN) for ntp_host in range(4)]

I know this also affects inDefaultNTPDomains(), but not significantly.

bernhardkaindl · 2024-03-19T11:49:11Z

plugins-base/XSFeatureNTP.py

        if inKey == 'KEY_ENTER':
            inputValues = pane.GetFieldValues()
            Layout.Inst().PopDialogue()
+            Layout.Inst().TransientBanner(Lang("Adding %s NTP Server..." % inputValues['name']))
            try:
                IPUtils.AssertValidNetworkName(inputValues['name'])
+
                data=Data.Inst()
+                if data.ntp.method("") == "Default":
+                    data.NTPServersSet([server for server in data.ntp.servers([]) if not inDefaultNTPDomains(server)])


I don't understand this section 100% clearly: It is in HandleKeyADD to Add NTP servers, AFAICS.

When data.ntp.method("") == "Default":, data.NTPServersSet( is called with the return values (NTP servers) from data.ntp.servers([]) that are not in the NTP default domains, correct?

I've seen that "Default" means that the default NTP servers shall be configured, so I'm a bit at a loss what this last line is used for, thanks!

"Default" means to use the initial/default NTP servers ([0-3].centos.pool.ntp.org)
"Manual" means to use servers specified by the user
In the case where a user is in "Default" mode but switches to "Manual" mode, as a convenience we remove the default servers from the list
In theory this could backfire if the user manually adds the default servers to the list, then adding a new default server could remove their existing ones
The alternative here is to not remove any servers from the list when a user switches from "Default" to "Manual" mode
What do you think?

bernhardkaindl · 2024-03-19T11:59:41Z

XSConsoleData.py

+            self.data['ntp']['method'] = "Manual"
+
+            servers = self.data['ntp']['servers']
+            if len(servers) == 4 and all(


Suggested change

if len(servers) == 4 and all(

# If 4 NTP servers are set and they are default NTP servers, set method to Default:

if len(servers) == 4 and all(

bernhardkaindl · 2024-03-19T12:13:37Z

XSConsoleConstants.py

@@ -0,0 +1,2 @@
+# Default NTP domains are <int>.[centos|xenserver].pool.ntp.org
+DEFAULT_NTP_DOMAINS = [".centos.pool.ntp.org", ".xenserver.pool.ntp.org"]


Suggestion to move the static definition of the default NTP server domain to the constants:

Suggested change

DEFAULT_NTP_DOMAINS = [".centos.pool.ntp.org", ".xenserver.pool.ntp.org"]

CENTOS_NTP_DOMAIN = "centos.pool.ntp.org"

DEFAULT_NTP_DOMAINS = [CENTOS_NTP_DOMAIN, "xenserver.pool.ntp.org"]

DEFAULT_NTP_SERVERS = ["%d.%s" % (i, CENTOS_NTP_DOMAIN) for i in range(4)]

bernhardkaindl · 2024-03-19T12:22:04Z

XSConsoleData.py

+        Data.Inst().NTPServersSet(
+            ["%d%s" % (i, DEFAULT_NTP_DOMAINS[0]) for i in range(4)]


And then use the new constant:

Suggested change

Data.Inst().NTPServersSet(

["%d%s" % (i, DEFAULT_NTP_DOMAINS[0]) for i in range(4)]

Data.Inst().NTPServersSet(DEFAULT_NTP_SERVERS)

…changing servers

Gerald Elder-Vass added 3 commits March 13, 2024 09:25

Use monotonic time in Python3 to avoid timeouts during NTP changes

c202279

Avoids authentication timeouts and session timeouts when the time jumps forward, either by chrony (NTP) or manual time changes

CA-369899: DHCP|Default|Manual|None NTP config

a992e30

Remove NTPStatus and the testing hook

003ece8

ntpstat is no longer present by default on installations

GeraldEV requested review from rosslagerwall, ashwin9390 and bernhardkaindl March 13, 2024 10:04

bernhardkaindl mentioned this pull request Mar 15, 2024

Apply selected suggestions courtesy of darker (from black) #36

Merged

bernhardkaindl reviewed Mar 15, 2024

View reviewed changes

XSConsoleData.py Outdated Show resolved Hide resolved

bernhardkaindl reviewed Mar 15, 2024

View reviewed changes

XSConsoleData.py Outdated Show resolved Hide resolved

bernhardkaindl reviewed Mar 15, 2024

View reviewed changes

XSConsoleData.py Outdated Show resolved Hide resolved

bernhardkaindl mentioned this pull request Mar 15, 2024

XSFeatureNTP: Apply selected suggestions courtesy of darker(from black) #37

Merged

bernhardkaindl approved these changes Mar 15, 2024

View reviewed changes

bernhardkaindl and others added 3 commits March 18, 2024 09:49

XSFeatureNTP: Apply selected suggestions courtesy of darker(from black)

af66f76

Signed-off-by: Bernhard Kaindl <[email protected]>

Apply selected suggestions courtesy of darker(from black)

5830764

Signed-off-by: Bernhard Kaindl <[email protected]>

Improved comments for chronyd one-shot update

27d076b

Co-authored-by: Bernhard Kaindl <[email protected]>

GeraldEV force-pushed the private/geralde/CA-369899 branch from 7c2bf15 to 27d076b Compare March 18, 2024 10:48

rosslagerwall reviewed Mar 18, 2024

View reviewed changes

rosslagerwall approved these changes Mar 19, 2024

View reviewed changes

GeraldEV force-pushed the private/geralde/CA-369899 branch from a56152c to ee123b2 Compare March 19, 2024 10:00

Improved comments and pythonisms from PR review

ee123b2

bernhardkaindl reviewed Mar 19, 2024

View reviewed changes

GeraldEV force-pushed the private/geralde/CA-369899 branch from db0e7e9 to f8a6181 Compare March 19, 2024 13:56

Use hardcoded default NTP servers, don't remove default servers when …

f8a6181

…changing servers

GeraldEV merged commit 13d1e3e into master Mar 19, 2024
2 checks passed

bernhardkaindl mentioned this pull request Mar 19, 2024

CA369899: Add docstrings to added/changed functions in XSFeatureNTP.py #39

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CA-369899: Add NTP options DHCP|Default|Manual|None #35

CA-369899: Add NTP options DHCP|Default|Manual|None #35

GeraldEV commented Mar 13, 2024

bernhardkaindl left a comment •

edited

Loading

GeraldEV commented Mar 19, 2024

bernhardkaindl Mar 19, 2024

bernhardkaindl Mar 19, 2024 •

edited

Loading

bernhardkaindl Mar 19, 2024

GeraldEV Mar 19, 2024

bernhardkaindl Mar 19, 2024 •

edited

Loading

bernhardkaindl Mar 19, 2024

bernhardkaindl Mar 19, 2024

	if len(servers) == 4 and all(
	# If 4 NTP servers are set and they are default NTP servers, set method to Default:
	if len(servers) == 4 and all(

		@@ -0,0 +1,2 @@
		# Default NTP domains are <int>.[centos\|xenserver].pool.ntp.org
		DEFAULT_NTP_DOMAINS = [".centos.pool.ntp.org", ".xenserver.pool.ntp.org"]

-DEFAULT_NTP_DOMAINS = [".centos.pool.ntp.org", ".xenserver.pool.ntp.org"]
+CENTOS_NTP_DOMAIN = "centos.pool.ntp.org"
+DEFAULT_NTP_DOMAINS = [CENTOS_NTP_DOMAIN, "xenserver.pool.ntp.org"]
+DEFAULT_NTP_SERVERS = ["%d.%s" % (i, CENTOS_NTP_DOMAIN) for i in range(4)]

		Data.Inst().NTPServersSet(
		["%d%s" % (i, DEFAULT_NTP_DOMAINS[0]) for i in range(4)]

	Data.Inst().NTPServersSet(
	["%d%s" % (i, DEFAULT_NTP_DOMAINS[0]) for i in range(4)]
	Data.Inst().NTPServersSet(DEFAULT_NTP_SERVERS)

CA-369899: Add NTP options DHCP|Default|Manual|None #35

CA-369899: Add NTP options DHCP|Default|Manual|None #35

Conversation

GeraldEV commented Mar 13, 2024

bernhardkaindl left a comment • edited Loading

Choose a reason for hiding this comment

GeraldEV commented Mar 19, 2024

bernhardkaindl Mar 19, 2024

Choose a reason for hiding this comment

bernhardkaindl Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

bernhardkaindl Mar 19, 2024

Choose a reason for hiding this comment

GeraldEV Mar 19, 2024

Choose a reason for hiding this comment

bernhardkaindl Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

bernhardkaindl Mar 19, 2024

Choose a reason for hiding this comment

bernhardkaindl Mar 19, 2024

Choose a reason for hiding this comment

bernhardkaindl left a comment •

edited

Loading

bernhardkaindl Mar 19, 2024 •

edited

Loading

bernhardkaindl Mar 19, 2024 •

edited

Loading