This is an automated email from the ASF dual-hosted git repository.

alexey pushed a commit to branch branch-1.18.x
in repository https://gitbox.apache.org/repos/asf/kudu.git


The following commit(s) were added to refs/heads/branch-1.18.x by this push:
     new 81a5e294f [thirdparty] upgrade chrony up to 4.6.1 version
81a5e294f is described below

commit 81a5e294fed7c369e8e8737fb10663603d912add
Author: Alexey Serbin <ale...@apache.org>
AuthorDate: Mon Dec 9 19:01:09 2024 -0800

    [thirdparty] upgrade chrony up to 4.6.1 version
    
    With chrony 4.6.1, the following has changed at the Kudu side:
      * The chrony-no-superuser.patch is no longer needed since a new '-U'
        command line option is now available to run chronyd when running
        tests under regular (non-super-user) accounts.
      * The SO_REUSEPORT socket option is now utilized by chronyd itself for
        its own purposes, so the chrony-reuseport.patch removes some
        limitations to let chronyd run under Kudu test scaffolding.
      * It seems the behavior of finding suitable (a.k.a. selectable)
        sources among configured NTP servers has changed, and chronyd 4.6.1
        is less picky than chronyd 3.5.  To address that, the 'minsources'
        directive is added when running chronyd in client-only mode to allow
        for existing test scenarios run unmodified.
      * Apparently, chronyd's behavior w.r.t. determining selectable time
        sources has changed between 3.5 and 4.6.1, and that manifests itself
        in a few existing test scenarios when clocks of reference NTP
        servers are far apart from each other or some even unsynchronized.
        To address that, corresponding test scenarios have been updated
        as necessary.
      * chronyd 4.6.1 correctly reports an error in its configuration when
        seeing multiple servers that would occupy the same configuration
        slot. Since it's not possible to bind and listen at arbitrary
        loopback IP addresses (127.0.0.0/8) on macOS, various test scenarios
        involving multiple chronyd servers aren't run on macOS anymore.
      * Since we aren't interested in exploring IPv6 support in chrony 4.6.1
        at this particular moment, the '-4' option is added to both chronyd
        and chronyc command lines to work only in IPv4 domain to avoid
        unexpected behavior when running tests on machines with IPv6 stack
        enabled.
    
    Change-Id: I3d2dc0145f4af0ff5488bef95d0ab6544038285e
    Reviewed-on: http://gerrit.cloudera.org:8080/22187
    Tested-by: Alexey Serbin <ale...@apache.org>
    Reviewed-by: Abhishek Chennaka <achenn...@cloudera.com>
    (cherry picked from commit d64533ffadc399182ce6910f5bf2f783ff693554)
    Reviewed-on: http://gerrit.cloudera.org:8080/22204
---
 src/kudu/clock/ntp-test.cc                   | 27 +++++++++------
 src/kudu/clock/test/mini_chronyd-test.cc     | 16 ++++++---
 src/kudu/clock/test/mini_chronyd.cc          | 23 ++++++++++---
 thirdparty/download-thirdparty.sh            |  3 +-
 thirdparty/patches/chrony-no-superuser.patch | 31 -----------------
 thirdparty/patches/chrony-reuseport.patch    | 50 +++++++++++++++++-----------
 thirdparty/vars.sh                           |  2 +-
 7 files changed, 78 insertions(+), 74 deletions(-)

diff --git a/src/kudu/clock/ntp-test.cc b/src/kudu/clock/ntp-test.cc
index dc3c71322..b55d21ce1 100644
--- a/src/kudu/clock/ntp-test.cc
+++ b/src/kudu/clock/ntp-test.cc
@@ -427,18 +427,14 @@ TEST(TimeIntervalsTest, ThreeResponses) {
 //       requires super-user privileges to bind to.
 //
 // NOTE: Some scenarios and sub-scenarios are disabled on macOS because
-//       of a bug in chronyd: it doesn't differentiate between configured NTP
-//       sources by IP address + port, using only IP address as the key.
+//       chronyd doesn't allow multiple servers on the same IP address,
+//       even if they are listening on different NTP server ports.
 //       On macOS, the same IP address 127.0.0.1 (loopback) is used for all
 //       the test NTP servers since multiple loopback addresses from
 //       the 127.0.0.0/8 range are not available out of the box. So, on macOS,
 //       the end-points of the test NTP servers differ only by their port
-//       number. It means chronyd doesn't see multiple NTP servers and talks
-//       only with the first one as listed in chrony.conf configuration file.
-//       In essence, any scenario which involves multiple NTP servers as source
-//       of true time for chrony doesn't run as expected on macOS.
-//
-// TODO(aserbin): fix the described chrony's bug, at least in thirdparty
+//       number. In essence, any scenario which involves multiple NTP servers
+//       as source of true time for chrony isn't run on macOS.
 class BuiltinNtpWithMiniChronydTest: public KuduTest {
  public:
   BuiltinNtpWithMiniChronydTest()
@@ -500,9 +496,13 @@ TEST_F(BuiltinNtpWithMiniChronydTest, Basic) {
     servers.emplace_back(std::move(chrony));
   }
 
+#ifndef __APPLE__
   // All chronyd servers that use the system clock as a reference lock should
   // present themselves as a set of NTP servers suitable for synchronisation.
+  //
+  // NOTE: see class-wide notes for details on macOS-specific exclusion
   ASSERT_OK(MiniChronyd::CheckNtpSource(servers_endpoints));
+#endif
 
   // chronyd supports very short polling intervals (down to 1/64 second).
   FLAGS_builtin_ntp_poll_interval_ms = 50;
@@ -529,7 +529,7 @@ TEST_F(BuiltinNtpWithMiniChronydTest, Basic) {
 #ifndef __APPLE__
 // Make sure the built-in client doesn't latch on NTP servers if they have 
their
 // true time spaced far away from each other. Basically, when no intersection 
is
-// registered between the true time reference intervals, the build-in NTP 
client
+// registered between the true time reference intervals, the built-in NTP 
client
 // should not synchronize its clock with any of the servers.
 //
 // NOTE: see class-wide notes for details on macOS-specific exclusion
@@ -692,8 +692,13 @@ TEST_F(BuiltinNtpWithMiniChronydTest, 
SyncAndUnsyncReferenceServers) {
     copy(unsync_servers_refs.begin(), unsync_servers_refs.end(),
          back_inserter(refs));
 
-    // Verify chronyd's client itself accepts the set of of NTP sources.
-    ASSERT_OK(MiniChronyd::CheckNtpSource(refs));
+    // Version 3.5 of the chronyd was able to work with such a configuration,
+    // but version 4.6.1 reports an error measuring clock offset because at
+    // least one of the source NTP servers is unsynchronized.
+    const auto s = MiniChronyd::CheckNtpSource(refs);
+    ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
+    ASSERT_STR_CONTAINS(s.ToString(),
+                        "failed measure clock offset from reference NTP 
servers");
 
     FLAGS_builtin_ntp_servers = HostPort::ToCommaSeparatedString(refs);
     BuiltInNtp c(metric_entity_);
diff --git a/src/kudu/clock/test/mini_chronyd-test.cc 
b/src/kudu/clock/test/mini_chronyd-test.cc
index 86dc44bb0..e744bd766 100644
--- a/src/kudu/clock/test/mini_chronyd-test.cc
+++ b/src/kudu/clock/test/mini_chronyd-test.cc
@@ -44,6 +44,16 @@ class MiniChronydTest: public KuduTest {
 // Run chronyd without any reference: neither local reference mode, nor
 // reference NTP server present. Such server cannot be a good NTP source
 // because its clock is unsynchronized.
+//
+// NOTE: Some scenarios and sub-scenarios are disabled on macOS because
+//       chronyd doesn't allow multiple servers on the same IP address,
+//       even if they are listening on different NTP server ports.
+//       On macOS, the same IP address 127.0.0.1 (loopback) is used for all
+//       the test NTP servers since multiple loopback addresses from
+//       the 127.0.0.0/8 range are not available out of the box. So, on macOS,
+//       the end-points of the test NTP servers differ only by their port
+//       number. In essence, any scenario which involves multiple NTP servers
+//       as source of true time for chrony isn't run on macOS.
 TEST_F(MiniChronydTest, UnsynchronizedServer) {
   unique_ptr<MiniChronyd> chrony;
   {
@@ -116,6 +126,7 @@ TEST_F(MiniChronydTest, BasicSingleServerInstance) {
   }
 }
 
+#ifndef __APPLE__
 // This scenario runs multiple chronyd and verifies they can co-exist without
 // conflicts w.r.t. resources such as port numbers and paths to their
 // configuration files, command sockets, and other related files.
@@ -153,9 +164,7 @@ TEST_F(MiniChronydTest, BasicMultipleServerInstances) {
   for (auto& server : servers) {
     MiniChronyd::ServerStats stats;
     ASSERT_OK(server->GetServerStats(&stats));
-#ifndef __APPLE__
     ASSERT_LT(0, stats.ntp_packets_received);
-#endif
     ASSERT_LT(0, stats.cmd_packets_received);
   }
 
@@ -169,7 +178,6 @@ TEST_F(MiniChronydTest, BasicMultipleServerInstances) {
     ASSERT_OK(servers[i]->SetTime(ref_time + i * 10));
   }
 
-#ifndef __APPLE__
   {
     // Now, with contradicting source NTP servers, it should be impossible
     // to synchronize the time.
@@ -177,7 +185,6 @@ TEST_F(MiniChronydTest, BasicMultipleServerInstances) {
     ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
     ASSERT_STR_CONTAINS(s.ToString(), "No suitable source for 
synchronisation");
   }
-#endif
 }
 
 // This scenario runs multi-tier set of chronyd servers: few servers of
@@ -232,6 +239,7 @@ TEST_F(MiniChronydTest, MultiTierBasic) {
     ASSERT_TRUE(s.ok()) << s.ToString();
   }
 }
+#endif // ifndef __APPLE__ ...
 
 } // namespace clock
 } // namespace kudu
diff --git a/src/kudu/clock/test/mini_chronyd.cc 
b/src/kudu/clock/test/mini_chronyd.cc
index 5f4cf6dc5..789acf56d 100644
--- a/src/kudu/clock/test/mini_chronyd.cc
+++ b/src/kudu/clock/test/mini_chronyd.cc
@@ -107,21 +107,26 @@ Status MiniChronyd::CheckNtpSource(const 
vector<HostPort>& servers,
   // packets. Also, it's desirable to use the same NTP protocol version that is
   // used by the Kudu built-in NTP client. Some more details:
   //   * 'minpoll' parameter is set to the smallest possible that is supported
-  //      by chrony (-6, i.e. 1/64 second)
+  //      by chrony (-7, i.e. 1/128 second in chrony 4.6.1)
   //   * 'maxpoll' parameter is set to be at least 2 times longer interval
   //      than 'minpoll' with some margin to accumulate at least 4 samples
   //      with current setting of 'minpoll' interval
-  //   * 'iburst' option allows to send first 4 NTP packets in burst.
+  //   * 'burst' option allows the client sending bursts of up to 4 requests
+  //     when it cannot get a good measurement from the server -- this helps
+  //     stabilize measurements and avoid problems while measuring the offset
+  //     of an NTP server's reference clock
+  //   * 'iburst' option allows to send first 4-8 NTP packets in burst mode.
   //   * 'version' is set to 3 to enforce using NTP v3 since the current
   //     implementation of the Kudu built-in NTP client uses NTPv3 (chrony
   //     supports NTP v4 and would use newer version of protocol otherwise)
   static const string kConfigTemplate =
-      "server $0 port $1 maxpoll -1 minpoll -6 iburst version 3\n";
+      "server $0 port $1 maxpoll -3 minpoll -7 iburst burst version 3\n";
 
   if (servers.empty()) {
     return Status::InvalidArgument("empty set of NTP server endpoints");
   }
-  string cfg;
+  // Require all the reference servers to be selectable for synchronisation.
+  string cfg = Substitute("minsources $0\n", servers.size());
   for (const auto& hp : servers) {
     cfg += Substitute(kConfigTemplate, hp.host(), hp.port());
   }
@@ -129,7 +134,9 @@ Status MiniChronyd::CheckNtpSource(const vector<HostPort>& 
servers,
   RETURN_NOT_OK(MiniChronyd::GetChronydPath(&chronyd_bin));
   const vector<string> cmd_and_args = {
     chronyd_bin,
+    "-4",                               // use only IPv4 addresses
     "-Q",                               // client-only mode without setting 
time
+    "-U",                               // disable super-user privileges check
     "-f", "/dev/stdin",                 // read config file from stdin
     "-t", std::to_string(timeout_sec),  // timeout for clock synchronisation
   };
@@ -211,7 +218,9 @@ Status MiniChronyd::Start() {
 
   process_.reset(new Subprocess({
       server_bin,
+      "-4", // use only IPv4 addresses
       "-f", config_file_path(),
+      "-U", // disable super-user privileges check
       "-x", // do not drive the system clock (server-only mode)
       "-d", // do not daemonize; print logs into standard out
   }));
@@ -470,18 +479,22 @@ Status MiniChronyd::RunChronyCmd(const vector<string>& 
args,
                                  string* out_stderr) const {
   string chronyc_bin;
   RETURN_NOT_OK(GetChronycPath(&chronyc_bin));
+  // The rest of the test scaffolding isn't yet working with IPv6:
+  // let's limit chronyc to the IPv4 stack to avoid unexpected outcomes
+  // if running against on machines with IPv6 or dual IP stack configured.
   // Use '-m' switch to allow for multiple commands to be sent at once: each
   // argument is interpreted as a separate command.
   // Use '-n' switch to avoid resolving IP addresses into DNS names in case if
   // chronyc outputs anything related to to IP addresses/hostnames (e.g.,
   // list of source NTP servers or list of NTP clients server by chronyd, 
etc.).
+  // Use '-N' option to avoid reverse DNS lookups, if any.
   // Since non-default command address is used for chronyd (the 
'bindcmdaddress'
   // option in 'chrony.conf'), it's necessary to specify the address using the
   // '-h' switch (it's a path of the Unix domain socket in case of 
MiniChronyd).
   // See 'man chronyc' for more details on the switches used for 'chronyc'
   // invocation.
   vector<string> cmd_and_args = {
-      chronyc_bin, "-n", "-m", "-h", cmdaddress(), };
+      chronyc_bin, "-4", "-n", "-N", "-m", "-h", cmdaddress(), };
   std::copy(args.begin(), args.end(), std::back_inserter(cmd_and_args));
   return Subprocess::Call(cmd_and_args, "", out_stdout, out_stderr);
 }
diff --git a/thirdparty/download-thirdparty.sh 
b/thirdparty/download-thirdparty.sh
index 7ce511078..c90a93146 100755
--- a/thirdparty/download-thirdparty.sh
+++ b/thirdparty/download-thirdparty.sh
@@ -421,12 +421,11 @@ fetch_and_patch \
  $YAML_SOURCE \
  $YAML_PATCHLEVEL
 
-CHRONY_PATCHLEVEL=2
+CHRONY_PATCHLEVEL=1
 fetch_and_patch \
  $CHRONY_NAME.tar.gz \
  $CHRONY_SOURCE \
  $CHRONY_PATCHLEVEL \
- "patch -p1 < $TP_DIR/patches/chrony-no-superuser.patch" \
  "patch -p1 < $TP_DIR/patches/chrony-reuseport.patch"
 
 GUMBO_PARSER_PATCHLEVEL=1
diff --git a/thirdparty/patches/chrony-no-superuser.patch 
b/thirdparty/patches/chrony-no-superuser.patch
deleted file mode 100644
index 937f5d99d..000000000
--- a/thirdparty/patches/chrony-no-superuser.patch
+++ /dev/null
@@ -1,31 +0,0 @@
-commit ecdaaf222a126ec9aeecb0479e2337f875a9618f
-Author: Alexey Serbin <ale...@apache.org>
-Date:   Tue Aug 20 16:47:02 2019 -0700
-
-    main: allow to run under regular user in server-only mode
-    
-    This patch allows for running chronyd under non-superuser account when
-    no clock control is requested (i.e. when '-d' option is specified).
-    
-    The motivation for this change is to run chronyd in server-only mode
-    (i.e. not driving system's clock) in various development and testing
-    environments.  Of course, it's assumed non-standard ports are used
-    for the NTP and the command endpoints in such a case, otherwise
-    chronyd would fail to bind to the standard NTP (123) and
-    command (323) ports since these are privileged ones.
-
-diff --git a/main.c b/main.c
-index fafca65..d94a3b5 100644
---- a/main.c
-+++ b/main.c
-@@ -501,8 +501,9 @@ int main
-     }
-   }
- 
--  if (getuid() && !client_only)
-+  if (getuid() && !(client_only || !clock_control)) {
-     LOG_FATAL("Not superuser");
-+  }
- 
-   /* Turn into a daemon */
-   if (!nofork) {
diff --git a/thirdparty/patches/chrony-reuseport.patch 
b/thirdparty/patches/chrony-reuseport.patch
index e42d59ce5..b337f8d32 100644
--- a/thirdparty/patches/chrony-reuseport.patch
+++ b/thirdparty/patches/chrony-reuseport.patch
@@ -1,33 +1,43 @@
-commit 7f5475515484885c112dffc51f32db3985acc4d6
+commit 5959f6c914bd5e9e9fbd858c8081ee19845df1b8
 Author: Alexey Serbin <ale...@apache.org>
-Date:   Thu Sep 12 23:48:20 2019 -0700
+Date:   Mon Dec 9 22:03:37 2024 -0800
 
-    ntp: add SO_REUSEPORT option for NTP server socket
+    ntp: adapt SO_REUSEPORT of NTP server socket for Kudu test scaffolding
     
     This patch makes the NTP server socket capable of binding to a port
     which is already bound, but not yet listened to.  This is useful
     in scenarios when it's necessary to make chronyd serving NTP requests
     at a port which has been reserved for chronyd by some other process.
 
-diff --git a/ntp_io.c b/ntp_io.c
-index ab08372..1ab554d 100644
---- a/ntp_io.c
-+++ b/ntp_io.c
-@@ -206,6 +206,17 @@ prepare_socket(int family, int port_number, int 
client_only)
-     LOG(LOGS_ERR, "Could not set %s socket option", "SO_REUSEADDR");
-     /* Don't quit - we might survive anyway */
-   }
-+
+diff --git a/socket.c b/socket.c
+index 5b22db5..e09275b 100644
+--- a/socket.c
++++ b/socket.c
+@@ -456,10 +456,12 @@ bind_ip_address(int sock_fd, IPSockAddr *addr, int flags)
+   if (addr->port > 0 && !SCK_SetIntOption(sock_fd, SOL_SOCKET, SO_REUSEADDR, 
1))
+     ;
+ 
+-#if defined(LINUX) && defined(SO_REUSEPORT)
+-  /* Allow multiple instances to bind to the same port in order to enable load
+-     balancing.  Don't enable this option on non-Linux systems as it has
+-     a slightly different meaning there (with some important implications). */
++#if defined(SO_REUSEPORT)
 +  /* Make the socket capable of binding to a port which is already bound,
 +   * but not yet listened to. This is useful in scenarios when it's necessary
 +   * to make chronyd serving NTP requests at a port which has been reserved
 +   * for chronyd by some other process.
 +   */
-+  if (!client_only && port_number &&
-+      setsockopt(sock_fd, SOL_SOCKET, SO_REUSEPORT, (char *)&on_off, 
sizeof(on_off)) < 0) {
-+    LOG(LOGS_ERR, "Could not set %s socket option", "SO_REUSEPORT");
-+    /* Don't quit yet - we might survive anyway */
-+  }
-   
-   /* Make the socket capable of sending broadcast pkts - needed for NTP 
broadcast mode */
-   if (!client_only &&
+   if (addr->port > 0 && !SCK_SetIntOption(sock_fd, SOL_SOCKET, SO_REUSEPORT, 
1))
+     ;
+ #endif
+@@ -470,10 +472,6 @@ bind_ip_address(int sock_fd, IPSockAddr *addr, int flags)
+     ;
+ #endif
+ 
+-  /* Do not attempt to bind pre-initialised reusable socket */
+-  if (SCK_IsReusable(sock_fd))
+-    return 1;
+-
+   saddr_len = SCK_IPSockAddrToSockaddr(addr, (struct sockaddr *)&saddr, 
sizeof (saddr));
+   if (saddr_len == 0)
+     return 0;
diff --git a/thirdparty/vars.sh b/thirdparty/vars.sh
index 5ba99eba5..a086a0f4f 100644
--- a/thirdparty/vars.sh
+++ b/thirdparty/vars.sh
@@ -216,7 +216,7 @@ YAML_VERSION=0.8.0
 YAML_NAME=yaml-cpp-yaml-cpp-$YAML_VERSION
 YAML_SOURCE=$TP_SOURCE_DIR/$YAML_NAME
 
-CHRONY_VERSION=3.5
+CHRONY_VERSION=4.6.1
 CHRONY_NAME=chrony-$CHRONY_VERSION
 CHRONY_SOURCE=$TP_SOURCE_DIR/$CHRONY_NAME
 

Reply via email to