From c00a46ab6c79d887cf59c11990d72ac65856bb21 Mon Sep 17 00:00:00 2001 From: David Elie-Dit-Cosaque Date: Mon, 11 May 2026 17:29:08 -0500 Subject: [PATCH 1/3] ptp_operator: resolve master alias before publishing HOLDOVER event When the HOLDOVER path in ParsePTP4l fires during ptp4l startup, the alias on ptpStats[master] may not yet be populated because no "master offset" log line has been parsed for the new process. This causes the ptp-state-change event to be published on the bare ResourceAddress ".../master" instead of ".../ens3fx/master". Subsequent recovery events use the per-interface address, so the aggregate ".../master" channel is never updated past HOLDOVER. Resolve the alias from the ptp4l config's slave interface (GetAliasByInterface) when the cached value is empty, matching the pattern already used by the clock-class-change and master-offset paths. The resolved alias is cached via SetAlias so that the holdover timeout goroutine also uses the correct value. --- plugins/ptp_operator/metrics/ptp4lParse.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/plugins/ptp_operator/metrics/ptp4lParse.go b/plugins/ptp_operator/metrics/ptp4lParse.go index ae19e7f9d..466f8e731 100644 --- a/plugins/ptp_operator/metrics/ptp4lParse.go +++ b/plugins/ptp_operator/metrics/ptp4lParse.go @@ -137,11 +137,15 @@ func (p *PTPEventManager) ParsePTP4l(processName, configName, profileName, outpu ptpStats[master].SetRole(types.FAULTY) // update slave port as faulty log.Infof("master process name %s and masteroffsetsource %s", ptpStats[master].ProcessName(), masterOffsetSource) if ptpStats[master].ProcessName() == masterOffsetSource { - alias := ptpStats[master].Alias() - masterResource := fmt.Sprintf("%s/%s", alias, MasterClockType) + aliasName := ptpStats[master].Alias() + if aliasName == "" { + aliasName = ptp4lCfg.GetAliasByInterface(ptpInterface) + ptpStats[master].SetAlias(aliasName) + } + masterResource := fmt.Sprintf("%s/%s", aliasName, MasterClockType) ptpStats[master].SetLastSyncState(syncState) p.PublishEvent(syncState, ptpStats[master].LastOffset(), masterResource, ptp.PtpStateChange) - UpdateSyncStateMetrics(ptpStats[master].ProcessName(), alias, syncState) + UpdateSyncStateMetrics(ptpStats[master].ProcessName(), aliasName, syncState) if ptpOpts, ok := p.PtpConfigMapUpdates.PtpProcessOpts[profileName]; ok && ptpOpts != nil { p.maybePublishOSClockSyncStateChangeEvent(ptpOpts, configName, profileName) threshold := p.PtpThreshold(profileName, true) From 6d2104634a0df2f19cd18b3d907e12a006a053a5 Mon Sep 17 00:00:00 2001 From: David Elie-Dit-Cosaque Date: Tue, 12 May 2026 11:32:31 -0500 Subject: [PATCH 2/3] ptp_operator: add unit test for HOLDOVER alias resolution Verify that when ptpStats[master] has no cached alias, the HOLDOVER path in ParsePTP4l resolves it from the ptp4l config's slave interface via GetAliasByInterface and caches it with SetAlias. --- .../ptp_operator/metrics/logparser_test.go | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/plugins/ptp_operator/metrics/logparser_test.go b/plugins/ptp_operator/metrics/logparser_test.go index 5e0b550d6..d32717de1 100644 --- a/plugins/ptp_operator/metrics/logparser_test.go +++ b/plugins/ptp_operator/metrics/logparser_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/redhat-cne/cloud-event-proxy/pkg/common" + "github.com/redhat-cne/cloud-event-proxy/plugins/ptp_operator/alias" "github.com/redhat-cne/cloud-event-proxy/plugins/ptp_operator/event" "github.com/redhat-cne/cloud-event-proxy/plugins/ptp_operator/ptp4lconf" @@ -715,6 +716,46 @@ func TestParsePTP4l(t *testing.T) { } } +func TestParsePTP4l_HoldoverResolvesEmptyAlias(t *testing.T) { + alias.SetAlias("ens5f0", "ens5fx") + + mockFS := &metrics.MockFileSystem{} + metrics.Filesystem = mockFS + ptpEventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) + ptpEventManager.MockTest(true) + + cfgName := "ptp4l.0.config" + profileName := "test-profile" + processName := "ptp4l" + metrics.SetMasterOffsetSource(processName) + + ptp4lCfg := &ptp4lconf.PTP4lConfig{ + Interfaces: []*ptp4lconf.PTPInterface{ + {Name: "ens5f0", PortID: 1, PortName: "port 1", Role: types.SLAVE}, + {Name: "ens5f1", PortID: 2, PortName: "port 2", Role: types.MASTER}, + }, + } + + ptpEventManager.Stats[types.ConfigName(cfgName)] = make(stats.PTPStats) + ptpStats := ptpEventManager.GetStats(types.ConfigName(cfgName)) + ptpStats.CheckSource(metrics.ClockRealTime, cfgName, processName) + ptpStats.CheckSource(metrics.MasterClockType, cfgName, processName) + ptpStats[metrics.MasterClockType].SetLastSyncState(ptp.LOCKED) + + assert.Equal(t, "", ptpStats[metrics.MasterClockType].Alias(), "alias should be empty before ParsePTP4l") + + output := "ptp4l[72444.514]: [ptp4l.0.config:5] port 1 (ens5f0): SLAVE to FAULTY on FAULT_DETECTED (FT_UNSPECIFIED)" + fields := []string{"ptp4l", "1646672953", "ptp4l.0.config", "port", "1", "(ens5f0)", "SLAVE to FAULTY on FAULT_DETECTED (FT_UNSPECIFIED)"} + ptpInterface := ptp4lconf.PTPInterface{Name: "ens5f0"} + + ptpEventManager.ParsePTP4l(processName, cfgName, profileName, output, fields, ptpInterface, ptp4lCfg, ptpStats) + + assert.Equal(t, ptp.HOLDOVER, ptpStats[metrics.MasterClockType].LastSyncState(), + "master should enter HOLDOVER state") + assert.Equal(t, "ens5fx", ptpStats[metrics.MasterClockType].Alias(), + "alias should be resolved from ptp4l config slave interface when empty") +} + func deepCopyPTP4lCfg(src *ptp4lconf.PTP4lConfig) *ptp4lconf.PTP4lConfig { var copied ptp4lconf.PTP4lConfig raw, _ := json.Marshal(src) From 8061118937d936ced0d5adb21ba2ecf9d0e630f6 Mon Sep 17 00:00:00 2001 From: David Elie-Dit-Cosaque Date: Tue, 12 May 2026 12:39:53 -0500 Subject: [PATCH 3/3] refactor: simplify Makefile and update unit test scripts Removed GOPATH and GOROOT checks from the Makefile to streamline the build process. Updated the 'gha' target to directly build plugins and run unit tests with coverage, excluding specific directories for accurate coverage results. Modified the unit-test.sh script to filter coverage output more effectively. Additionally, adjusted the coverage-gate.sh script to ensure proper restoration of the Makefile and unit-test.sh after execution. --- Makefile | 38 ++++--------------- hack/coverage-gate.sh | 4 +- hack/unit-test.sh | 4 +- .../ptp_operator/metrics/logparser_test.go | 22 +++++++++++ 4 files changed, 33 insertions(+), 35 deletions(-) diff --git a/Makefile b/Makefile index 85bb18ac7..3f7f72503 100644 --- a/Makefile +++ b/Makefile @@ -27,15 +27,6 @@ else endif export GOPATH=$(shell go env GOPATH) -# Fix GOPATH == GOROOT for GO111MODULE=off builds (common on servers with Go at ~/go). -_SYS_GOROOT := $(shell GO111MODULE=off go env GOROOT 2>/dev/null) -_SYS_GOPATH := $(shell GO111MODULE=off go env GOPATH 2>/dev/null) -ifeq ($(_SYS_GOPATH),$(_SYS_GOROOT)) -export GOMODCACHE := $(_SYS_GOPATH)/pkg/mod -export GOPATH := /tmp/gopath -endif -# Module-aware GOROOT (may point to an auto-downloaded newer toolchain) -_MOD_GOROOT := $(shell go env GOROOT 2>/dev/null) ##@ Build Dependencies @@ -115,28 +106,15 @@ undeploy-consumer:kustomize cd ./examples/manifests && $(KUSTOMIZE) edit set image cloud-event-consumer=${CONSUMER_IMG} $(KUSTOMIZE) build ./examples/manifests | kubectl delete -f - -# For GitHub Actions CI +# Build plugins and run unit tests with coverage. +# GO111MODULE=on is required so external test packages (package foo_test) +# produce real coverage numbers instead of 0.0%. +# pkg/plugins is excluded because its plugin-loading test is incompatible +# with -coverprofile instrumentation (package signature mismatch). gha: - mkdir -p $(GOPATH)/src/github.com/redhat-cne/cloud-event-proxy - @if [ "$(_SYS_GOPATH)" = "$(_SYS_GOROOT)" ] && [ -n "$(_SYS_GOROOT)" ]; then \ - echo "Cleaning stale vendor copies from GOROOT/src..."; \ - for d in golang.org github.com k8s.io sigs.k8s.io google.golang.org; do \ - rm -rf "$(_SYS_GOROOT)/src/$$d" 2>/dev/null || true; \ - done; \ - fi - @if [ "$$(realpath $(GOPATH)/src/github.com/redhat-cne/cloud-event-proxy)" != "$$(realpath .)" ]; then \ - echo "✅ Safe to delete: cleaning GOPATH workspace..."; \ - rm -rf $(GOPATH)/src/github.com/redhat-cne/cloud-event-proxy/*; \ - cp -r cmd examples pkg plugins test $(GOPATH)/src/github.com/redhat-cne/cloud-event-proxy; \ - cp -r vendor/* $(GOPATH)/src; \ - rm -rf /tmp/sub-store && mkdir -p /tmp/sub-store; \ - else \ - echo "⚠️ Skipping delete: GOPATH is pointing to current working directory!"; \ - fi - - PATH=$(_MOD_GOROOT)/bin:$$PATH GOROOT=$(_MOD_GOROOT) GO111MODULE=off go build -a -o plugins/ptp_operator_plugin.so -buildmode=plugin plugins/ptp_operator/ptp_operator_plugin.go - PATH=$(_MOD_GOROOT)/bin:$$PATH GOROOT=$(_MOD_GOROOT) GO111MODULE=off go build -a -o plugins/mock_plugin.so -buildmode=plugin plugins/mock/mock_plugin.go - PATH=$(_MOD_GOROOT)/bin:$$PATH GOROOT=$(_MOD_GOROOT) GO111MODULE=off STORE_PATH=/tmp/sub-store go test ./... --tags=unittests -coverprofile=cover.out + go build -a -o plugins/ptp_operator_plugin.so -buildmode=plugin plugins/ptp_operator/ptp_operator_plugin.go + go build -a -o plugins/mock_plugin.so -buildmode=plugin plugins/mock/mock_plugin.go + go test -tags=unittests $$(go list ./... | grep -v '/test/' | grep -v 'pkg/plugins') -coverprofile=cover.out docker-build: # make sure build the right target when developer using a Mac diff --git a/hack/coverage-gate.sh b/hack/coverage-gate.sh index 7981933b7..ef999c9f2 100755 --- a/hack/coverage-gate.sh +++ b/hack/coverage-gate.sh @@ -76,9 +76,9 @@ cp "${TMPDIR_SAVE}/unit-test.sh" "${SCRIPT_DIR}/unit-test.sh" rm -rf "${TMPDIR_SAVE}" "${SCRIPT_DIR}/unit-test.sh" BASE_COV=$(go tool cover -func=coverage.out | grep ^total | awk '{print $3}' | tr -d '%') -git checkout -- "${SCRIPT_DIR}/../Makefile" 2>/dev/null || true -rm -f "${SCRIPT_DIR}/unit-test.sh" 2>/dev/null || true git checkout "${CURRENT_BRANCH}" --quiet +git checkout -- "${SCRIPT_DIR}/../Makefile" 2>/dev/null || true +git checkout -- "${SCRIPT_DIR}/unit-test.sh" 2>/dev/null || true if [ "${STASHED}" = true ]; then git stash pop --quiet fi diff --git a/hack/unit-test.sh b/hack/unit-test.sh index b2636d506..72b1aa661 100755 --- a/hack/unit-test.sh +++ b/hack/unit-test.sh @@ -1,11 +1,9 @@ #!/bin/bash # Run unit tests and generate filtered coverage profile. -# This wraps `make gha` which handles GOPATH setup and plugin builds -# required for GO111MODULE=off test execution. set -e REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" cd "${REPO_ROOT}" make gha -grep -vE "/test/|/examples/|/plugins/mock/|vendor/" cover.out > coverage.out +grep -vE "zz_generated|mock_|/test/|vendor/|/bin/" cover.out > coverage.out diff --git a/plugins/ptp_operator/metrics/logparser_test.go b/plugins/ptp_operator/metrics/logparser_test.go index d32717de1..2ee24d018 100644 --- a/plugins/ptp_operator/metrics/logparser_test.go +++ b/plugins/ptp_operator/metrics/logparser_test.go @@ -8,6 +8,7 @@ import ( "github.com/redhat-cne/cloud-event-proxy/pkg/common" "github.com/redhat-cne/cloud-event-proxy/plugins/ptp_operator/alias" + ptpConfig "github.com/redhat-cne/cloud-event-proxy/plugins/ptp_operator/config" "github.com/redhat-cne/cloud-event-proxy/plugins/ptp_operator/event" "github.com/redhat-cne/cloud-event-proxy/plugins/ptp_operator/ptp4lconf" @@ -754,6 +755,27 @@ func TestParsePTP4l_HoldoverResolvesEmptyAlias(t *testing.T) { "master should enter HOLDOVER state") assert.Equal(t, "ens5fx", ptpStats[metrics.MasterClockType].Alias(), "alias should be resolved from ptp4l config slave interface when empty") + + // Subtest: HOLDOVER with alias already set (covers the aliasName != "" branch + // and the PtpProcessOpts/mock holdover path) + ptpStats[metrics.MasterClockType].SetLastSyncState(ptp.LOCKED) + ptpStats[metrics.MasterClockType].SetAlias("ens5fx") + ptp4lCfg.Interfaces[0].Role = types.SLAVE + + ptpEventManager.PtpConfigMapUpdates.PtpProcessOpts[profileName] = &ptpConfig.PtpProcessOpts{} + ptpEventManager.PtpConfigMapUpdates.EventThreshold[profileName] = &ptpConfig.PtpClockThreshold{ + HoldOverTimeout: 5, + MaxOffsetThreshold: 100, + MinOffsetThreshold: -100, + Close: make(chan struct{}), + } + + ptpEventManager.ParsePTP4l(processName, cfgName, profileName, output, fields, ptpInterface, ptp4lCfg, ptpStats) + + assert.Equal(t, ptp.HOLDOVER, ptpStats[metrics.MasterClockType].LastSyncState(), + "master should re-enter HOLDOVER state with pre-set alias") + assert.Equal(t, "ens5fx", ptpStats[metrics.MasterClockType].Alias(), + "alias should remain unchanged when already set") } func deepCopyPTP4lCfg(src *ptp4lconf.PTP4lConfig) *ptp4lconf.PTP4lConfig {