You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2020/02/05 19:14:19 UTC
[kudu] branch master updated (e722084 -> cfe589c)
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git.
from e722084 [clock] simplify system time source
new 6f626f2 [clock] fix on SystemNtp::Init()
new cfe589c [tests] enable TestNtpDiagnostics for 'system' time source
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
src/kudu/clock/CMakeLists.txt | 2 +-
src/kudu/clock/hybrid_clock-test.cc | 4 ++--
src/kudu/clock/system_ntp.cc | 38 +++++++++++++++++++++----------------
3 files changed, 25 insertions(+), 19 deletions(-)
[kudu] 01/02: [clock] fix on SystemNtp::Init()
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 6f626f2baef0519cbae6546ec15f939419c489b6
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Tue Feb 4 21:27:56 2020 -0800
[clock] fix on SystemNtp::Init()
Do not require the clock to be synchronized to read the clock frequency
tolerance via ntp_adjtime().
This is a follow-up to e72208436a625391739217394c67d783e992367a.
Change-Id: I0b4a64d422d417e86e621d0d42d715d7046fc0b8
Reviewed-on: http://gerrit.cloudera.org:8080/15163
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
src/kudu/clock/system_ntp.cc | 38 ++++++++++++++++++++++----------------
1 file changed, 22 insertions(+), 16 deletions(-)
diff --git a/src/kudu/clock/system_ntp.cc b/src/kudu/clock/system_ntp.cc
index a7c1e6b..b4977a1 100644
--- a/src/kudu/clock/system_ntp.cc
+++ b/src/kudu/clock/system_ntp.cc
@@ -49,22 +49,19 @@ namespace clock {
namespace {
+// Convert the clock state returned by ntp_gettime() into Status.
Status NtpStateToStatus(int rc) {
+ // According to http://man7.org/linux/man-pages/man3/ntp_gettime.3.html,
+ // ntp_gettime() never fails if given correct pointer to the output argument.
+ PCHECK(rc >= 0);
switch (rc) {
case TIME_OK:
return Status::OK();
- case -1: // generic error
- {
- int err = errno;
- // From 'man 2 adjtimex', ntp_adjtime failure implies an improper 'tx'.
- return Status::InvalidArgument(
- "Error reading clock. ntp_adjtime() failed", ErrnoToString(err));
- }
case TIME_ERROR:
return Status::ServiceUnavailable(
- PREDICT_FALSE(FLAGS_inject_unsync_time_errors) ?
- "Injected clock unsync error" :
- "Error reading clock. Clock considered unsynchronized");
+ PREDICT_FALSE(FLAGS_inject_unsync_time_errors)
+ ? "Injected clock unsync error"
+ : "Error reading clock. Clock considered unsynchronized");
default:
// TODO(dralves): what to do about leap seconds? see KUDU-146
KLOG_FIRST_N(ERROR, 1)
@@ -109,11 +106,22 @@ SystemNtp::SystemNtp()
Status SystemNtp::Init() {
timex tx;
tx.modes = 0; // set mode to 0 for read-only query
- RETURN_NOT_OK(NtpStateToStatus(ntp_adjtime(&tx)));
+ int rc = ntp_adjtime(&tx);
+ // From http://man7.org/linux/man-pages/man2/adjtimex.2.html,
+ // the failure implies invalid pointer for 'tx', which cannot be the case.
+ // However, there were reports that with old version of Docker it might return
+ // EPERM even if 'tx.mode' is set to 0; see KUDU-2000 for details.
+ if (PREDICT_FALSE(rc == -1)) {
+ int err = errno;
+ return Status::RuntimeError("ntp_adjtime() failed", ErrnoToString(err));
+ }
+
// The unit of the reported tolerance is ppm with 16-bit fractional part:
// 65536 is 1 ppm (see http://man7.org/linux/man-pages/man3/ntp_adjtime.3.html
// for details).
skew_ppm_ = tx.tolerance / 65536;
+ VLOG(1) << "ntp_adjtime(): tolerance is " << tx.tolerance;
+
return Status::OK();
}
@@ -121,12 +129,10 @@ Status SystemNtp::WalltimeWithError(uint64_t* now_usec, uint64_t* error_usec) {
if (PREDICT_FALSE(FLAGS_inject_unsync_time_errors)) {
return NtpStateToStatus(TIME_ERROR);
}
- // Read the time. This will return an error if the clock is not synchronized.
+ // Read the clock and convert its state into status. This will return an error
+ // if the clock is not synchronized.
ntptimeval tv;
- const int rc = ntp_gettime(&tv);
- // ntp_gettime() never fails according to its manual page.
- PCHECK(rc != -1);
- RETURN_NOT_OK(NtpStateToStatus(rc));
+ RETURN_NOT_OK(NtpStateToStatus(ntp_gettime(&tv)));
uint64_t now = static_cast<uint64_t>(tv.time.tv_sec) * 1000000;
#ifdef __APPLE__
now += tv.time.tv_nsec / 1000;
[kudu] 02/02: [tests] enable TestNtpDiagnostics for 'system' time
source
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit cfe589ca8dc4e6ead0b49ed372ea06f63bacda52
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Tue Feb 4 17:28:35 2020 -0800
[tests] enable TestNtpDiagnostics for 'system' time source
Enable or disable HybridClockTest.TestNtpDiagnostics depending
on whether the 'system' time source is available.
This allows for running the test scenaio on macOS High Sierra
and newer releases where the NTP kernel API is supported.
This is a follow-up for e72208436a625391739217394c67d783e992367a.
Change-Id: I2f2efd5de6a18b0af86429ed63ba2df7c65a1e84
Reviewed-on: http://gerrit.cloudera.org:8080/15160
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
src/kudu/clock/CMakeLists.txt | 2 +-
src/kudu/clock/hybrid_clock-test.cc | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/kudu/clock/CMakeLists.txt b/src/kudu/clock/CMakeLists.txt
index 190698f..3ed2665 100644
--- a/src/kudu/clock/CMakeLists.txt
+++ b/src/kudu/clock/CMakeLists.txt
@@ -26,7 +26,7 @@ set(CLOCK_SRCS
# Darwin 17 (macOS High Sierra, ver. 10.13.6) and higher supports get_ntptime()
if (NOT APPLE OR ${CMAKE_SYSTEM_VERSION} VERSION_GREATER_EQUAL "17.0.0")
set(CLOCK_SRCS ${CLOCK_SRCS} system_ntp.cc)
- set_source_files_properties(hybrid_clock.cc
+ set_source_files_properties(hybrid_clock.cc hybrid_clock-test.cc
PROPERTIES COMPILE_DEFINITIONS "KUDU_HAS_SYSTEM_TIME_SOURCE=1")
endif()
diff --git a/src/kudu/clock/hybrid_clock-test.cc b/src/kudu/clock/hybrid_clock-test.cc
index 3873b59..422daf4 100644
--- a/src/kudu/clock/hybrid_clock-test.cc
+++ b/src/kudu/clock/hybrid_clock-test.cc
@@ -352,7 +352,7 @@ TEST_F(HybridClockTest, TestRideOverNtpInterruption) {
ASSERT_LT(timestamps[1].ToUint64(), timestamps[2].ToUint64());
}
-#ifndef __APPLE__
+#if defined(KUDU_HAS_SYSTEM_TIME_SOURCE)
TEST_F(HybridClockTest, TestNtpDiagnostics) {
FLAGS_time_source = "system";
clock_.reset(new HybridClock);
@@ -365,7 +365,7 @@ TEST_F(HybridClockTest, TestNtpDiagnostics) {
ASSERT_STR_MATCHES(s, "(ntp_gettime\\(\\) returns code |chronyc -n tracking)");
ASSERT_STR_MATCHES(s, "(ntpq -n |chronyc -n sources)");
}
-#endif
+#endif // #if defined(KUDU_HAS_SYSTEM_TIME_SOURCE) ...
} // namespace clock
} // namespace kudu