Merge pull request #143 from sapcc/fix-141

Fix an index out of range when running shorter than multiple measurements duration, Treat high drift as an absolute value

CHANGELOG.md

@@ -1,11 +1,18 @@
 ## v2.8.0 - TBD
 
+Bug fixes:
+- Fixed an index out of range when the clock drift is high when running shorter than the multiple measurement duration
+- Fixed high drift not considering negative drift
+
 Changes:
+- Updated all go dependencies to their latest versions.
 
 ## v2.7.1 - 2024-09-27
 
+Bug fixes:
+- Fixed a server error being returned if the optional `high_drift` parameter is not given.
+
 Changes:
-- Fix a server error being returned if the optional `high_drift` parameter is not given.
 - Updated all go dependencies to their latest versions.
 
 ## v2.7.0 - 2024-09-10

collector.go

@@ -23,6 +23,7 @@ package main
 import (
 	"fmt"
 	"log"
+	"math"
 	"sort"
 	"time"
 
@@ -183,8 +184,9 @@ func (c Collector) measure() error {
 		return fmt.Errorf("couldn't get NTP measurement: %w", err)
 	}
 
-	// if clock drift is unusually high (e.g. >10ms): repeat measurements for 30 seconds and submit median value
-	if measurement.clockOffset > c.NtpHighDrift.Seconds() {
+	// if absolute clock drift is unusually high (e.g. >10ms):
+	// repeat measurements for the configured amount of seconds and submit median value
+	if math.Abs(measurement.clockOffset) > c.NtpHighDrift.Seconds() {
 		// arrays of measurements used to calculate median
 		var measurementsClockOffset []float64
 		var measurementsStratum []float64
@@ -197,7 +199,7 @@ func (c Collector) measure() error {
 		var measurementsLeap []float64
 
 		log.Printf("WARN: clock drift is above %.3fs, taking multiple measurements for %.2f seconds", c.NtpHighDrift.Seconds(), c.NtpMeasurementDuration.Seconds())
-		for time.Since(begin) < c.NtpMeasurementDuration {
+		for next := true; next; next = time.Since(begin) < c.NtpMeasurementDuration {
 			nextMeasurement, err := c.getClockOffsetAndStratum()
 			if err != nil {
 				c.serverReachable.WithLabelValues(c.NtpServer).Set(0)

main.go

@@ -67,7 +67,7 @@ func main() {
 		}
 
 		if ntpProtocolVersion < 2 || ntpProtocolVersion > 4 {
-			log.Fatalf("invalid NTP protocol version %d; must be 2, 3, or 4", ntpProtocolVersion)
+			logger.Fatalf("invalid NTP protocol version %d; must be 2, 3, or 4", ntpProtocolVersion)
 		}
 	}
 
@@ -86,8 +86,8 @@ func init() {
 	flag.StringVar(&metricsPath, "web.telemetry-path", "/metrics", "Path under which to expose metrics.")
 	flag.StringVar(&ntpServer, "ntp.server", "", "NTP server to use (required).")
 	flag.IntVar(&ntpProtocolVersion, "ntp.protocol-version", 4, "NTP protocol version to use.")
-	flag.DurationVar(&ntpMeasurementDuration, "ntp.measurement-duration", 30*time.Second, "Duration of measurements in case of high (>10ms) drift.")
-	flag.DurationVar(&ntpHighDrift, "ntp.high-drift", 10*time.Millisecond, "High drift threshold.")
+	flag.DurationVar(&ntpMeasurementDuration, "ntp.measurement-duration", 30*time.Second, "Duration of measurements in case of high drift.")
+	flag.DurationVar(&ntpHighDrift, "ntp.high-drift", 10*time.Millisecond, "Absolute high drift threshold.")
 	flag.StringVar(&ntpSource, "ntp.source", "cli", "source of information about ntp server (cli / http).")
 	flag.Parse()
 }
@@ -100,6 +100,13 @@ func handlerMetrics(w http.ResponseWriter, r *http.Request) {
 	d := ntpMeasurementDuration
 	hd := ntpHighDrift
 
+	if d < 0 {
+		logger.Fatal("ntp.measurement-duration cannot be negative")
+	}
+	if hd < 0 {
+		logger.Fatal("ntp.high-drift cannot be negative")
+	}
+
 	if ntpSource == "http" {
 		query := r.URL.Query()
 		for _, i := range []string{"target", "protocol", "duration"} {
@@ -122,6 +129,10 @@ func handlerMetrics(w http.ResponseWriter, r *http.Request) {
 		}
 
 		if t, err := time.ParseDuration(query.Get("duration")); err == nil {
+			if t < 0 {
+				http.Error(w, "duration cannot be negative", http.StatusBadRequest)
+				return
+			}
 			d = t
 		} else {
 			http.Error(w, "while parsing duration: "+err.Error(), http.StatusBadRequest)
@@ -130,6 +141,10 @@ func handlerMetrics(w http.ResponseWriter, r *http.Request) {
 
 		if query.Get("high_drift") != "" {
 			if u, err := time.ParseDuration(query.Get("high_drift")); err == nil {
+				if u < 0 {
+					http.Error(w, "high_drift cannot be negative", http.StatusBadRequest)
+					return
+				}
 				hd = u
 			} else {
 				http.Error(w, "while parsing high_drift: "+err.Error(), http.StatusBadRequest)