Rewrite of the ntp-client #1327

MarcFontaine · 2019-12-05T13:43:36Z

This PR started as a refactoring of the ntp-client but evolved into a re-implementation.
(Therefore it does not make sense to review it as a diff)

ntp-client/src/Network/NTP/Client.hs

ntp-client/ntp-client.cabal

ntp-client/src/Network/NTP/Packet.hs

ntp-client/src/Network/NTP/Client.hs

coot · 2020-01-24T13:05:43Z

ntp-client/src/Network/NTP/Client.hs

+            Right _ -> traceWith tracer NtpTracePacketSent
+            Left e  -> do
+                traceWith tracer $ NtpTracePacketSentError e
+                ioError e


I would use a onException (or catch if you only want to act on IOExceptions). This will make transition to MonadCatcheasier (it does not have the low levelSystem.IO` exception control flow functions).

lets leave that for a new PR

ntp-client/src/Network/NTP/Client.hs

ntp-client/ntp-client.cabal

ntp-client/src/Network/NTP/Query.hs

coot · 2020-02-03T10:36:48Z

ntp-client/src/Network/NTP/Query.hs

+        case (firstAddr AF_INET addr, firstAddr AF_INET6 addr) of
+            (Nothing, Nothing) -> do
+                traceWith tracer $ NtpTraceLookupServerFailed server
+                ioError $ userError $ "lookup NTP server failed " ++ server


Why not use a custom exception rather than IOError? Is there something specific to IOError in the rest of the code base that relies on the fact that it is an IOException? (e.g. an error handler that must catch it).

Yes, to keep it simple the rest of the code only handles IOErrors explicitly and only IOErrors
and Async exceptions are expected to happen. All other exception are propagated.
All other exceptions are probably caused by bugs in the program.

ntp-client/src/Network/NTP/Query.hs

ntp-client/src/Network/NTP/Trace.hs

ntp-client/src/Network/NTP/Client.hs

It is not needed outside of `Query` module

GHC will be able to free memory taken by the tread earlier.

The test suite does not need rts, -threaded and -N options set.

* use explicit recursion * queryLoop - it must write the returned value by 'ntpQuery', otherwise a thread waiting for the update will not receive it. It is up the the application to decide what to do if 'NtpSyncUnavailable' is received. * improve documentation * return type is 'IO Void', which clearly indicates that this is an infinite loop.

When triggering a forceful update, check if the client is not already enaged in ntp protocols. If not trigger update, otherwise wait for completion of running ntp protocol instances.

This matches other test suites in 'ouroboros-network' package.

* mask async exception to guarantee that all threads are cancelled * no need to use `waitAnyCancel` inside `withAsync`` callback, `waitAny` is enough for threads started with `withAsync`.

coot · 2020-02-05T08:14:41Z

bors r+

1327: Rewrite of the ntp-client r=coot a=MarcFontaine This PR started as a refactoring of the ntp-client but evolved into a re-implementation. (Therefore it does not make sense to review it as a diff) Co-authored-by: MarcFontaine <[email protected]> Co-authored-by: MarcFontaine <[email protected]> Co-authored-by: Marcin Szamotulski <[email protected]>

iohk-bors · 2020-02-05T09:20:58Z

Build succeeded

MarcFontaine

I would expect the following:

client runs OK for a while and then error
retry in 5 10 20 40... seconds
client runs OK for a while (expected this resets retry sequence) , and then error
EXPECTED : retry in 5 10 20 40 ... seconds
IMPLEMENTATION : retry in 80 160 320 .. seconds

coot · 2020-02-05T12:40:31Z

@MarcFontaine could you elaborate. The client will use 5 10 20 ... up to 600 intervals between restarts: https://github.com/input-output-hk/ouroboros-network/blob/master/ntp-client/src/Network/NTP/Client.hs#L98

coot · 2020-02-05T12:41:24Z

arg, there's a mistake, it should be:

2 * delay `min` 600

not max :D

MarcFontaine · 2020-02-05T15:07:20Z

I created an issue we can discuss it there:
#1583

MarcFontaine

This does not work:
getAddrInfo needs a Just value for at least one of the HostName or ServiceName arguments

MarcFontaine · 2020-02-05T18:10:05Z

ntp-client/src/Network/NTP/Query.hs

 udpLocalAddresses :: IO [AddrInfo]
--                                      Hints        Host    Service
-udpLocalAddresses = Socket.getAddrInfo (Just hints) Nothing (Just $ show port)
+udpLocalAddresses = Socket.getAddrInfo (Just hints) Nothing Nothing
  where
    hints = Socket.defaultHints
          { addrFlags = [AI_PASSIVE]
          , addrSocketType = Datagram
          }
-    port = Socket.defaultPort


This does not work:
getAddrInfo needs at least one Just argument.

mark-stopka · 2020-02-25T16:08:42Z

Hi,
is there an option to disable built-in ntp-client and use system time instead?

coot · 2020-02-25T16:16:07Z

Hi @mark-stopka
The reason for having this component is to check that system clock time is in sync with the rest of the world. It is not used to set or read time.

It's worth to mention that this is not to substitute a proper ntp client run as a system service.

mark-stopka · 2020-02-25T19:27:53Z

Hi @coot, thanks for your feedback,
I was just asking, because Linux Kernel patches introducing Time Namespaces is maybe going to be introduced in Linux 5.6.0. Lot of people running pools on the ITNv1 with Jormungandr were following official guides, which included steps to configure Chrony NTP client and they were obviously failing for all those who did not have real VMs, but rather LXC containers. I'd expect Linux 5.6.x to be LTS distributions in few years time =).

Thanks for confirming,
I'll have a look around a logic that you use your application specific time or it's delta with system time for.

MarcFontaine added networking technical debt labels Dec 5, 2019

MarcFontaine self-assigned this Dec 5, 2019

MarcFontaine force-pushed the mafo/client2 branch from 1bf4617 to aba0a82 Compare December 17, 2019 10:47

mrBliss removed the maintenance label Jan 6, 2020

MarcFontaine mentioned this pull request Jan 10, 2020

Refactoring the ntp-client #1362

Closed

5 tasks

MarcFontaine changed the title ~~Remove use of time-units package~~ Rewrite of the ntp-client Jan 13, 2020

MarcFontaine force-pushed the mafo/client2 branch from c75e318 to ac8616d Compare January 15, 2020 08:12

This was referenced Jan 15, 2020

NTP IPv6 support #1428

Closed

NTP-client inline sendTo into sendPacket #1190

Closed

coot requested changes Jan 15, 2020

View reviewed changes

coot added the ntp-client Issues related to ntp-client label Jan 16, 2020

coot requested changes Jan 24, 2020

View reviewed changes

Replace the old ntp-client with a new implementation.

047a376

MarcFontaine force-pushed the mafo/client2 branch from 9c6423d to 047a376 Compare January 24, 2020 17:17

MarcFontaine and others added 13 commits January 27, 2020 15:49

fix response timeout

362fbdc

split Client.hs module

82f535e

remove unused Language Options

ccb74fd

remove ReceivedPacket

b183d50

unfold firstAddr into call-sites

f2b91ba

rename NtpClientSettings

5479180

add blocking query

dfcf10e

don't link the NTP client thread to the application thread by default

58a3ffa

address some reviews

b62f411

use backround client thread for ntpQueryBlocking

a5f0237

cleanup NtpTrace type

0ee4f78

no special case for no replies

02a6ce8

fix spelling and import

9535da3

coot requested changes Feb 3, 2020

View reviewed changes

MarcFontaine requested a review from coot February 4, 2020 09:28

coot reviewed Feb 4, 2020

View reviewed changes

ntp-client/src/Network/NTP/Client.hs Outdated Show resolved Hide resolved

improve comments

f94e503

MarcFontaine requested a review from coot February 4, 2020 18:46

coot and others added 11 commits February 4, 2020 19:48

Do not export minimumOfThree

a960771

It is not needed outside of `Query` module

Remove forever loop which keeps a thread

e58e25b

GHC will be able to free memory taken by the tread earlier.

ntpQuery - add documentation

d5f1e93

Improved documentation and layout of NTP.Query module

3dc5bb9

ntp-client ghc-options

029f0bc

The test suite does not need rts, -threaded and -N options set.

npt-client - improve documentation

e112387

Forceful ntp update

35a6b8e

When triggering a forceful update, check if the client is not already enaged in ntp protocols. If not trigger update, otherwise wait for completion of running ntp protocol instances.

Rename the test suite as test-ntp-client

9da24ac

This matches other test suites in 'ouroboros-network' package.

run nix/regenerate.sh

520049c

runNtpQueries - more robust thread managment

981abc5

* mask async exception to guarantee that all threads are cancelled * no need to use `waitAnyCancel` inside `withAsync`` callback, `waitAny` is enough for threads started with `withAsync`.

coot approved these changes Feb 5, 2020

View reviewed changes

iohk-bors bot merged commit 981abc5 into master Feb 5, 2020

iohk-bors bot deleted the mafo/client2 branch February 5, 2020 09:21

MarcFontaine commented Feb 5, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite of the ntp-client #1327

Rewrite of the ntp-client #1327

MarcFontaine commented Dec 5, 2019 •

edited

Loading

coot Jan 24, 2020

MarcFontaine Jan 30, 2020

coot Feb 3, 2020

MarcFontaine Feb 4, 2020

coot commented Feb 5, 2020

iohk-bors bot commented Feb 5, 2020

MarcFontaine left a comment

coot commented Feb 5, 2020

coot commented Feb 5, 2020

MarcFontaine commented Feb 5, 2020

MarcFontaine left a comment

MarcFontaine Feb 5, 2020

mark-stopka commented Feb 25, 2020

coot commented Feb 25, 2020

mark-stopka commented Feb 25, 2020 •

edited

Loading

Rewrite of the ntp-client #1327

Rewrite of the ntp-client #1327

Conversation

MarcFontaine commented Dec 5, 2019 • edited Loading

coot Jan 24, 2020

Choose a reason for hiding this comment

MarcFontaine Jan 30, 2020

Choose a reason for hiding this comment

coot Feb 3, 2020

Choose a reason for hiding this comment

MarcFontaine Feb 4, 2020

Choose a reason for hiding this comment

coot commented Feb 5, 2020

iohk-bors bot commented Feb 5, 2020

Build succeeded

MarcFontaine left a comment

Choose a reason for hiding this comment

coot commented Feb 5, 2020

coot commented Feb 5, 2020

MarcFontaine commented Feb 5, 2020

MarcFontaine left a comment

Choose a reason for hiding this comment

MarcFontaine Feb 5, 2020

Choose a reason for hiding this comment

mark-stopka commented Feb 25, 2020

coot commented Feb 25, 2020

mark-stopka commented Feb 25, 2020 • edited Loading

MarcFontaine commented Dec 5, 2019 •

edited

Loading

mark-stopka commented Feb 25, 2020 •

edited

Loading