Updated ntp sources handling #173

mchf · 2022-05-02T07:25:05Z

Problem

For several reasons ntp sources handling is not much user friendly.

For example in OpenSUSE installation the dialog looks this way

There is no possibility to change listed ntp sources (add / remove).
The code doesn't distinguish between ntp server and pool.
What is less visible is that sources are written into "wrong" config file.
Behavior in SLE is not exactly the same but similar enough.

What are major caveats?

the dialog shown in the screenshot is not implemented in one module but is spread in two modules (y2-country and y2-ntp-client)
similarly to dialog creation, the handling is done in similar way. So, there is bunch of ui_ prefixed methods in y2-ntp-client's proposal client. Whenever is some operation done in the ntp part of dialog then it is proposed by client's api. It means that timezone dialog's loop handler checks if the event comes from ntp part of dialog, then it calls an ntp-client handler via proposal client's api and receives the response in the same way. It has some drawbacks regarding handling current dialog state.

Solution

Purpose of this PR was to keep current functionality and open possibilities to implement new behavior. Mainly support to really add / remove ntp sources and distinguish them to pool / servers.

So, as first step the dialog was redesigned to

It allows to add / remove ntp sources and perform real configuration, not only using what installation sets for the user with no possibility to change anything.

It shows knowledge of difference between ntp server / pool.
List of ntp sources offered by default in the combobox was taken from SLE. User is allowed to write own ntp source.

Ntp sources are also read from the control file and from dhcp. Dhcp sources are not proposed by default, but available for selection in combobox. One of control file sources is randomly picked and proposed. See also previous yast2-country PR for some details.

Pools / servers are written into /etc/chrony.d/pool.conf. The pool.conf is provided by chrony-pool-* package(s) (product specific). We have to overwrite it otherwise yast's setup could be corrupted. The pool.conf is deployed by a package by design for needs of image based (no yast) installations.

Testing

Tested manually

Screenshots

See above

coveralls · 2022-05-02T07:26:48Z

Coverage: 64.369% (+0.6%) from 63.778% when pulling 62e89d8 on mchf:removed_old_code into 99c8e0e on yast:master.

src/lib/y2ntp_client/widgets/sources_table.rb

teclator · 2022-07-25T11:57:56Z

Would be nice to see also how it looks like in ncurses. Therefore some screenshot would be appreciated

mchf · 2022-10-06T08:15:11Z

Would be nice to see also how it looks like in ncurses. Therefore some screenshot would be appreciated

As you can see in the following picture, the dialog is a bit messy in current implementation (TW installer)

This patch turns it to

teclator · 2022-10-11T07:38:50Z

There is a type column but there is no information about the source type at all but in general do not look bad although for UX I would ask @dgdavid ;)

mchf · 2022-11-14T18:59:31Z

This time this works:

combo box is pre-filed with values obtained from dhcp, control file, ....
some servers are picked by default - e.g. those obtained from dhcp
user is allowed to pick whether the source is pool or server
sources are written to the target at the end of installation

Major changes:

the dialog is re-entrant. Before it behaved differently depending whether entered first time, or something was selected by user before and some other circumstances
user is allowed to store the source as server or pool
sources selected by default are as before

teclator · 2022-11-22T09:53:56Z

This time this works:

combo box is pre-filed with values obtained from dhcp, control file, ....

some servers are picked by default - e.g. those obtained from dhcp

user is allowed to pick whether the source is pool or server

sources are written to the target at the end of installation

Major changes:

the dialog is re-entrant. Before it behaved differently depending whether entered first time, or something was selected by user before and some other circumstances

user is allowed to store the source as server or pool

sources selected by default are as before

¿What about the servers defined in the rpm? I guess we should not provide default configuration based on rpms anymore in order to not collide with the configured ones or at least we should select the chrony-pool-empty package in the proposal

src/clients/ntp-client_proposal.rb

mchf · 2022-11-23T09:57:42Z

¿What about the servers defined in the rpm? I guess we should not provide default configuration based on rpms anymore in order to not collide with the configured ones or at least we should select the chrony-pool-empty package in the proposal

Proposals are currently taken only from dhcp and control file. In my POV, any "default" servers provided by an rpm package should not be used as it is somehow against what the proposal in installer does. However, this is for a discussion somewhere else. We're having a bit of chicken egg problem here, however I'd say that we need to have working installer proposal accepting user modifications and then we can ask for some more changes elsewhere.

teclator · 2022-11-23T10:16:45Z

Proposals are currently taken only from dhcp and control file. In my POV, any "default" servers provided by an rpm package should not be used as it is somehow against what the proposal in installer does.

I agree, and it was part of the PBI to discuss it first before any implementation.

However, this is for a discussion somewhere else.

+1

We're having a bit of chicken egg problem here, however I'd say that we need to have working installer proposal accepting user modifications and then we can ask for some more changes elsewhere.

Not at all, we need to define behavior and implement accordingly, if we not show the user the current servers then it could be problematic and a mess which is exactly what is currently happening unless the user deselect the default rpm and selects the empty pools one, so this could be part of the current proposal until the product patterns or chrony package recommends are adapted.

BTW, checking the current implementation with a DUD ends with a long list of servers (known ones if I'm not wrong)

I expected the ones from the control file but that is not the case, is there some change in yast2-country needed by this PR?

teclator · 2022-11-23T12:35:10Z

src/clients/ntp-client_proposal.rb

 module Yast
  # This is used as the general interface between yast2-country
  # (time,timezone) and yast2-ntp-client.
  class NtpClientProposalClient < Client
    include Yast::Logger

+    @sources_table = nil


I would omit this initialization and move it to a method...

Suggested change

@sources_table = nil

def sources_table

@sources_table_widget ||= Y2NtpClient::Widgets::SourcesTable.new(NtpClient.GetUsedNtpSources)

end

mchf · 2022-11-25T10:34:00Z

Minor fix in default initialization of ntp sources dialog

dgdavid · 2022-11-30T09:39:43Z

Just a minor suggestion, slightly out of the scope of this PR: why not take the opportunity to put Current time and Current date inline?

I mean, when selecting Manually see something like

Date        Time
▒▒▒▒▒▒▒▒▒   ▒▒▒▒▒▒▒▒▒

instead of the current

   
Current Time
▒▒▒▒▒▒▒▒▒  

Current Date     
▒▒▒▒▒▒▒▒▒

mchf · 2022-11-30T11:12:35Z

Just a minor suggestion, slightly out of the scope of this PR: why not take the opportunity to put Current time and Current date inline?

I mean, when selecting Manually see something like
Date        Time
▒▒▒▒▒▒▒▒▒   ▒▒▒▒▒▒▒▒▒
instead of the current
   
Current Time
▒▒▒▒▒▒▒▒▒  

Current Date     
▒▒▒▒▒▒▒▒▒

it is in different module (yast2-country) see the description how it works to get into fun with this dialog ;-)

List of servers to be stored is by default initialized by dhcp provided sources only. Timezone based proposal is used for combo together with dhcp ones, to be used as hint of available sources for user.

teclator

LGTM

yast-bot · 2023-01-25T10:24:15Z

✔️ Public Jenkins job #49 successfully finished
✔️ Created OBS submit request #1060832

yast-bot · 2023-01-25T10:27:38Z

✔️ Internal Jenkins job #31 successfully finished
✔️ Created IBS submit request #288844

mchf added 5 commits February 4, 2014 08:55

Merge remote-tracking branch 'upstream/master'

f984aca

Merge remote-tracking branch 'upstream/master'

105e2f1

Merge remote-tracking branch 'upstream/master'

8434698

Merge remote-tracking branch 'upstream/master'

096b02a

Merge remote-tracking branch 'upstream/master'

7f638e0

teclator reviewed Jul 22, 2022

View reviewed changes

src/lib/y2ntp_client/widgets/sources_table.rb Outdated Show resolved Hide resolved

teclator reviewed Jul 22, 2022

View reviewed changes

src/lib/y2ntp_client/widgets/sources_table.rb Outdated Show resolved Hide resolved

teclator reviewed Jul 22, 2022

View reviewed changes

src/lib/y2ntp_client/widgets/sources_table.rb Outdated Show resolved Hide resolved

teclator reviewed Jul 22, 2022

View reviewed changes

src/lib/y2ntp_client/widgets/sources_table.rb Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master'

4c5a5d4

mchf force-pushed the removed_old_code branch from 4abe031 to d5118fe Compare October 6, 2022 07:41

mchf force-pushed the removed_old_code branch from b7adbf6 to d653e86 Compare October 21, 2022 11:13

mchf force-pushed the removed_old_code branch from fc9db29 to 3c54095 Compare November 10, 2022 17:51

teclator reviewed Nov 22, 2022

View reviewed changes

src/clients/ntp-client_proposal.rb Show resolved Hide resolved

teclator reviewed Nov 23, 2022

View reviewed changes

mchf force-pushed the removed_old_code branch from d74eb01 to f39bb06 Compare November 30, 2022 10:37

mchf force-pushed the removed_old_code branch 3 times, most recently from dd7e708 to c112f50 Compare December 5, 2022 08:55

mchf added 23 commits January 23, 2023 19:53

Tweaking preselection

cd8bd21

Documentation

a0f6ab2

Write pools even servers at the end of installation

1527bb5

Refactoring of SourcesTable

3cdfa4a

Tunning ntp sources write workflow

ab2960e

A hack to keep client proposal API untouched

2630290

Code cleanup

554be36

Happy rubocop

f43930e

Tuned ntp sources offered by default

fcda4e5

List of servers to be stored is by default initialized by dhcp provided sources only. Timezone based proposal is used for combo together with dhcp ones, to be used as hint of available sources for user.

Updated testsuite

cdcdfd0

Switch writing configuration into pool.conf

5d818ae

Temporarily disable some tests

dfe10df

Updated testsuite

c53222b

Happy rubocop

126002b

Fixed proposal re-entrancy

9b00e1f

Use a source for initial sync from sources table instead of the old way

ed889db

Tuned default servers proposal

d0638d3

Updated testsuite

01a89ee

Happy rubocop

3bea579

Changed combobox label

d464217

Changes according to CR

3b796f3

Updated testsuite

7a8b28c

Updated changelog

62e89d8

mchf force-pushed the removed_old_code branch from 84978c7 to 62e89d8 Compare January 23, 2023 18:54

mchf requested a review from teclator January 24, 2023 09:56

teclator approved these changes Jan 25, 2023

View reviewed changes

mchf merged commit 1e87efd into yast:master Jan 25, 2023

mchf deleted the removed_old_code branch January 25, 2023 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated ntp sources handling #173

Updated ntp sources handling #173

mchf commented May 2, 2022 •

edited

Loading

coveralls commented May 2, 2022 •

edited

Loading

teclator commented Jul 25, 2022

mchf commented Oct 6, 2022

teclator commented Oct 11, 2022

mchf commented Nov 14, 2022

teclator commented Nov 22, 2022

mchf commented Nov 23, 2022

teclator commented Nov 23, 2022

teclator Nov 23, 2022

mchf commented Nov 25, 2022

dgdavid commented Nov 30, 2022 •

edited

Loading

mchf commented Nov 30, 2022

teclator left a comment

yast-bot commented Jan 25, 2023

yast-bot commented Jan 25, 2023

-    @sources_table = nil
+    def sources_table
+      @sources_table_widget ||= Y2NtpClient::Widgets::SourcesTable.new(NtpClient.GetUsedNtpSources)
+    end

Updated ntp sources handling #173

Updated ntp sources handling #173

Conversation

mchf commented May 2, 2022 • edited Loading

Problem

Solution

Testing

Screenshots

coveralls commented May 2, 2022 • edited Loading

teclator commented Jul 25, 2022

mchf commented Oct 6, 2022

teclator commented Oct 11, 2022

mchf commented Nov 14, 2022

teclator commented Nov 22, 2022

mchf commented Nov 23, 2022

teclator commented Nov 23, 2022

teclator Nov 23, 2022

Choose a reason for hiding this comment

mchf commented Nov 25, 2022

dgdavid commented Nov 30, 2022 • edited Loading

mchf commented Nov 30, 2022

teclator left a comment

Choose a reason for hiding this comment

yast-bot commented Jan 25, 2023

yast-bot commented Jan 25, 2023

mchf commented May 2, 2022 •

edited

Loading

coveralls commented May 2, 2022 •

edited

Loading

dgdavid commented Nov 30, 2022 •

edited

Loading