From 43e5228bc1dd0b33f1c4ee9669749a4054911304 Mon Sep 17 00:00:00 2001 From: nocturnalastro Date: Wed, 22 Jul 2026 16:35:11 +0100 Subject: [PATCH 1/6] Default missing T-BC E1 to FREERUN for E3 after reconfig Prefer the T-BC stats key for T-BC profiles and treat unset E1 as FREERUN so a lone phc2sys LOCKED sample cannot make OS-clock E3 LOCKED too early after CEP restart or profile apply. Generated-by: Cursor --- plugins/ptp_operator/metrics/manager.go | 17 ++++++ plugins/ptp_operator/metrics/metrics.go | 16 ++++-- plugins/ptp_operator/metrics/tbc_test.go | 66 ++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 5 deletions(-) diff --git a/plugins/ptp_operator/metrics/manager.go b/plugins/ptp_operator/metrics/manager.go index 79abdd17..fc40ce85 100644 --- a/plugins/ptp_operator/metrics/manager.go +++ b/plugins/ptp_operator/metrics/manager.go @@ -662,6 +662,23 @@ func (p *PTPEventManager) GetProfileType(name string) ptp4lconf.PtpProfileType { return ptp4lconf.NONE } +// GetProfileTypeByConfigName returns the cached ProfileType for a ptp4l config +// name (e.g. "ptp4l.0.config"). Returns NONE if the config is unknown. +func (p *PTPEventManager) GetProfileTypeByConfigName(configName types.ConfigName) ptp4lconf.PtpProfileType { + p.lock.RLock() + cfg, ok := p.Ptp4lConfigInterfaces[configName] + p.lock.RUnlock() + if !ok || cfg == nil { + return ptp4lconf.NONE + } + if cfg.ProfileType != ptp4lconf.NONE { + return cfg.ProfileType + } + // ProfileType may still be NONE if the config was registered before + // TBCProfiles was populated; resolve from the profile name. + return p.GetProfileType(cfg.Profile) +} + // RefreshProfileTypes re-evaluates the cached ProfileType on all registered // PTP4lConfigs from the current TBCProfiles list. Call this after // UpdatePTPSetting() has populated TBCProfiles so that configs registered diff --git a/plugins/ptp_operator/metrics/metrics.go b/plugins/ptp_operator/metrics/metrics.go index 8f384f5c..459c9775 100644 --- a/plugins/ptp_operator/metrics/metrics.go +++ b/plugins/ptp_operator/metrics/metrics.go @@ -289,13 +289,19 @@ func (p *PTPEventManager) ExtractMetrics(msg string) { // phc2sys offset is within threshold. // GetMainClockName() returns the correct key per profile type: // T-BC → "T-BC", T-GM → "GM", OC/BC → "master" + // Prefer the profile-type key for T-BC/T-GM so a missing T-BC + // entry after reconfig is not mistaken for OC "master". mainClockKey := ptpStats.GetMainClockName() - if e1Stat, ok := ptpStats[mainClockKey]; ok { - e1State := e1Stat.LastSyncState() - if e1State == ptp.FREERUN || e1State == ptp.HOLDOVER { - syncState = OverallState(syncState, e1State) - } + if ptp4lCfg.ProfileType == ptp4lconf.TBC { + mainClockKey = types.IFace(stats.TBCMainClockName) + } + // Missing or unset E1 defaults to FREERUN so a lone OS-clock + // LOCKED sample cannot make E3 LOCKED early after reconfig. + e1State := ptp.FREERUN + if e1Stat, ok := ptpStats[mainClockKey]; ok && e1Stat.LastSyncState() != "" { + e1State = e1Stat.LastSyncState() } + syncState = OverallState(syncState, e1State) // for HA we can not rely on master ;since there will be 2 or more leaders; this condition will be skipped // ptpStats clock realtime has its own stats objects if r, ok := ptpStats[master]; ok && r.Role() == types.SLAVE { // publish event only if the master role is active diff --git a/plugins/ptp_operator/metrics/tbc_test.go b/plugins/ptp_operator/metrics/tbc_test.go index 4ac747a8..27413f75 100644 --- a/plugins/ptp_operator/metrics/tbc_test.go +++ b/plugins/ptp_operator/metrics/tbc_test.go @@ -118,6 +118,31 @@ func TestGetProfileType(t *testing.T) { } } +func TestGetProfileTypeByConfigName(t *testing.T) { + eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) + eventManager.PtpConfigMapUpdates.TBCProfiles = []string{"tbc-profile-1"} + + tbcCfg := "ptp4l.0.config" + ocCfg := "ptp4l.1.config" + staleCfg := "ptp4l.2.config" + + eventManager.AddPTPConfig(types.ConfigName(tbcCfg), &ptp4lconf.PTP4lConfig{ + Name: tbcCfg, Profile: "tbc-profile-1", ProfileType: ptp4lconf.TBC, + }) + eventManager.AddPTPConfig(types.ConfigName(ocCfg), &ptp4lconf.PTP4lConfig{ + Name: ocCfg, Profile: "oc-profile", ProfileType: ptp4lconf.NONE, + }) + // Cached as NONE, but profile is in TBCProfiles — live fallback should resolve TBC. + eventManager.AddPTPConfig(types.ConfigName(staleCfg), &ptp4lconf.PTP4lConfig{ + Name: staleCfg, Profile: "tbc-profile-1", ProfileType: ptp4lconf.NONE, + }) + + assert.Equal(t, ptp4lconf.TBC, eventManager.GetProfileTypeByConfigName(types.ConfigName(tbcCfg))) + assert.Equal(t, ptp4lconf.NONE, eventManager.GetProfileTypeByConfigName(types.ConfigName(ocCfg))) + assert.Equal(t, ptp4lconf.TBC, eventManager.GetProfileTypeByConfigName(types.ConfigName(staleCfg))) + assert.Equal(t, ptp4lconf.NONE, eventManager.GetProfileTypeByConfigName(types.ConfigName("ptp4l.99.config"))) +} + // TestTBCPtp4lMasterOffsetNoHoldover tests that TBC profiles don't enter HOLDOVER from ptp4l master offset func TestTBCPtp4lMasterOffsetNoHoldover(t *testing.T) { eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) @@ -715,3 +740,44 @@ func TestTBCProcessDownEventFiresFreerun(t *testing.T) { assert.Equal(t, ptp.LOCKED, ptpStats[tbcKey].LastSyncState(), "T-BC stats should be LOCKED after T-BC-STATUS s2") } + +// TestE3DefaultsMissingE1ToFreerun ensures that after reconfig, when only +// phc2sys has reported LOCKED and T-BC/E1 has not reported yet, E3 stays +// FREERUN instead of locking early from OS clock alone. +func TestE3DefaultsMissingE1ToFreerun(t *testing.T) { + cfgName := "ptp4l.1.config" + tbcProfile := "t-bc_tbc-tr" + + eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) + eventManager.MockTest(true) + eventManager.PtpConfigMapUpdates.TBCProfiles = []string{tbcProfile} + + ptp4lCfg := &ptp4lconf.PTP4lConfig{ + Name: cfgName, + Profile: tbcProfile, + ProfileType: ptp4lconf.TBC, + Interfaces: []*ptp4lconf.PTPInterface{ + {Name: "ens2f0", PortID: 1, PortName: "port 1", Role: types.SLAVE}, + }, + } + eventManager.AddPTPConfig(types.ConfigName(cfgName), ptp4lCfg) + + ptpStats := eventManager.GetStats(types.ConfigName(cfgName)) + ptpStats[metrics.MasterClockType] = stats.NewStats(cfgName) + ptpStats[metrics.MasterClockType].SetAlias("ens2fx") + ptpStats[metrics.MasterClockType].SetRole(types.SLAVE) + + eventManager.ResetMockEvent() + phc2sysLocked := fmt.Sprintf( + "phc2sys[600840.357]: [%s] CLOCK_REALTIME phc offset -34 s2 freq -4040 delay 529", + cfgName, + ) + eventManager.ExtractMetrics(phc2sysLocked) + + cStat, ok := ptpStats[metrics.ClockRealTime] + assert.True(t, ok, "CLOCK_REALTIME stats should exist") + assert.Equal(t, ptp.FREERUN, cStat.LastSyncState(), + "E3 must stay FREERUN when E1/T-BC has not reported yet") + assert.Contains(t, eventManager.GetMockEvent(), ptp.OsClockSyncStateChange, + "should publish OS-clock FREERUN (not LOCKED) when E1 is unset") +} From 794d313ddb51f968a543009aa6af9276f3a86e62 Mon Sep 17 00:00:00 2001 From: nocturnalastro Date: Tue, 28 Jul 2026 13:33:17 +0100 Subject: [PATCH 2/6] Address PR review: resolve stale T-BC profile type for E3 Use GetProfileTypeByConfigName when selecting the E1 key, harden the missing-E1 FREERUN test for cached NONE ProfileType, and clear goconst literals in T-BC tests. Generated-by: Cursor --- plugins/ptp_operator/metrics/metrics.go | 2 +- plugins/ptp_operator/metrics/tbc_test.go | 19 ++++++++++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/plugins/ptp_operator/metrics/metrics.go b/plugins/ptp_operator/metrics/metrics.go index 459c9775..d84ca7b3 100644 --- a/plugins/ptp_operator/metrics/metrics.go +++ b/plugins/ptp_operator/metrics/metrics.go @@ -292,7 +292,7 @@ func (p *PTPEventManager) ExtractMetrics(msg string) { // Prefer the profile-type key for T-BC/T-GM so a missing T-BC // entry after reconfig is not mistaken for OC "master". mainClockKey := ptpStats.GetMainClockName() - if ptp4lCfg.ProfileType == ptp4lconf.TBC { + if p.GetProfileTypeByConfigName(types.ConfigName(configName)) == ptp4lconf.TBC { mainClockKey = types.IFace(stats.TBCMainClockName) } // Missing or unset E1 defaults to FREERUN so a lone OS-clock diff --git a/plugins/ptp_operator/metrics/tbc_test.go b/plugins/ptp_operator/metrics/tbc_test.go index 27413f75..0cd5a083 100644 --- a/plugins/ptp_operator/metrics/tbc_test.go +++ b/plugins/ptp_operator/metrics/tbc_test.go @@ -17,7 +17,8 @@ import ( ) const ( - cfgName = "ptp4l.0.config" + cfgName = "ptp4l.0.config" + tbcPtp4l1CfgName = "ptp4l.1.config" ) // TestTBCProfileDetection tests that TBC profiles are correctly identified @@ -122,8 +123,8 @@ func TestGetProfileTypeByConfigName(t *testing.T) { eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) eventManager.PtpConfigMapUpdates.TBCProfiles = []string{"tbc-profile-1"} - tbcCfg := "ptp4l.0.config" - ocCfg := "ptp4l.1.config" + tbcCfg := ntpPtp4lCfgName + ocCfg := tbcPtp4l1CfgName staleCfg := "ptp4l.2.config" eventManager.AddPTPConfig(types.ConfigName(tbcCfg), &ptp4lconf.PTP4lConfig{ @@ -684,7 +685,7 @@ func TestTBCProcessDownEventFiresFreerun(t *testing.T) { eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) eventManager.MockTest(true) - configName = "ptp4l.1.config" + configName = tbcPtp4l1CfgName tbcProfile := "tbc-tr" eventManager.PtpConfigMapUpdates.TBCProfiles = []string{tbcProfile} @@ -714,7 +715,7 @@ func TestTBCProcessDownEventFiresFreerun(t *testing.T) { ptpStats[tbcKey].SetLastSyncState(ptp.LOCKED) // Step 1: Simulate ptp4l process down (PTP_PROCESS_STATUS:0) - downLog := "ptp4l[1780430740]:[ptp4l.1.config] PTP_PROCESS_STATUS:0" + downLog := fmt.Sprintf("ptp4l[1780430740]:[%s] PTP_PROCESS_STATUS:0", configName) eventManager.ResetMockEvent() eventManager.ExtractMetrics(downLog) @@ -745,17 +746,19 @@ func TestTBCProcessDownEventFiresFreerun(t *testing.T) { // phc2sys has reported LOCKED and T-BC/E1 has not reported yet, E3 stays // FREERUN instead of locking early from OS clock alone. func TestE3DefaultsMissingE1ToFreerun(t *testing.T) { - cfgName := "ptp4l.1.config" + cfgName := tbcPtp4l1CfgName tbcProfile := "t-bc_tbc-tr" eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) eventManager.MockTest(true) eventManager.PtpConfigMapUpdates.TBCProfiles = []string{tbcProfile} + // ProfileType intentionally NONE (stale cache after reconfig); TBCProfiles + // still lists the profile so GetProfileTypeByConfigName resolves TBC. ptp4lCfg := &ptp4lconf.PTP4lConfig{ Name: cfgName, Profile: tbcProfile, - ProfileType: ptp4lconf.TBC, + ProfileType: ptp4lconf.NONE, Interfaces: []*ptp4lconf.PTPInterface{ {Name: "ens2f0", PortID: 1, PortName: "port 1", Role: types.SLAVE}, }, @@ -766,6 +769,8 @@ func TestE3DefaultsMissingE1ToFreerun(t *testing.T) { ptpStats[metrics.MasterClockType] = stats.NewStats(cfgName) ptpStats[metrics.MasterClockType].SetAlias("ens2fx") ptpStats[metrics.MasterClockType].SetRole(types.SLAVE) + // Locked master without a T-BC key must not make E3 LOCKED for T-BC. + ptpStats[metrics.MasterClockType].SetLastSyncState(ptp.LOCKED) eventManager.ResetMockEvent() phc2sysLocked := fmt.Sprintf( From c0b714ed02a967e6d9f7b9dd3bb3d8282b7b66b5 Mon Sep 17 00:00:00 2001 From: nocturnalastro Date: Tue, 28 Jul 2026 13:44:43 +0100 Subject: [PATCH 3/6] test: expand E3/T-BC coverage for stale profile-type paths Table-drive missing-E1 FREERUN cases, cover RefreshProfileTypes and IsTBCProfile, and assert non-T-BC profiles still use master for E3. Generated-by: Cursor --- plugins/ptp_operator/metrics/tbc_test.go | 130 ++++++++++++++++++++--- 1 file changed, 118 insertions(+), 12 deletions(-) diff --git a/plugins/ptp_operator/metrics/tbc_test.go b/plugins/ptp_operator/metrics/tbc_test.go index 0cd5a083..ef6bc13f 100644 --- a/plugins/ptp_operator/metrics/tbc_test.go +++ b/plugins/ptp_operator/metrics/tbc_test.go @@ -744,20 +744,130 @@ func TestTBCProcessDownEventFiresFreerun(t *testing.T) { // TestE3DefaultsMissingE1ToFreerun ensures that after reconfig, when only // phc2sys has reported LOCKED and T-BC/E1 has not reported yet, E3 stays -// FREERUN instead of locking early from OS clock alone. +// FREERUN instead of locking early from OS clock alone. Also covers E1 +// HOLDOVER/LOCKED once the T-BC key exists, with a stale ProfileType=NONE cache. func TestE3DefaultsMissingE1ToFreerun(t *testing.T) { cfgName := tbcPtp4l1CfgName tbcProfile := "t-bc_tbc-tr" + tests := []struct { + name string + setupTBC bool + e1State ptp.SyncState + expectedE3 ptp.SyncState + }{ + { + name: "missing T-BC key defaults E1 to FREERUN", + setupTBC: false, + expectedE3: ptp.FREERUN, + }, + { + name: "T-BC HOLDOVER keeps E3 HOLDOVER despite phc2sys LOCKED", + setupTBC: true, + e1State: ptp.HOLDOVER, + expectedE3: ptp.HOLDOVER, + }, + { + name: "T-BC LOCKED allows E3 LOCKED with phc2sys LOCKED", + setupTBC: true, + e1State: ptp.LOCKED, + expectedE3: ptp.LOCKED, + }, + { + name: "T-BC empty LastSyncState defaults E1 to FREERUN", + setupTBC: true, + e1State: "", + expectedE3: ptp.FREERUN, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) + eventManager.MockTest(true) + eventManager.PtpConfigMapUpdates.TBCProfiles = []string{tbcProfile} + + // ProfileType intentionally NONE (stale cache after reconfig); TBCProfiles + // still lists the profile so GetProfileTypeByConfigName resolves TBC. + ptp4lCfg := &ptp4lconf.PTP4lConfig{ + Name: cfgName, + Profile: tbcProfile, + ProfileType: ptp4lconf.NONE, + Interfaces: []*ptp4lconf.PTPInterface{ + {Name: "ens2f0", PortID: 1, PortName: "port 1", Role: types.SLAVE}, + }, + } + eventManager.AddPTPConfig(types.ConfigName(cfgName), ptp4lCfg) + + ptpStats := eventManager.GetStats(types.ConfigName(cfgName)) + ptpStats[metrics.MasterClockType] = stats.NewStats(cfgName) + ptpStats[metrics.MasterClockType].SetAlias("ens2fx") + ptpStats[metrics.MasterClockType].SetRole(types.SLAVE) + // Locked master without a T-BC key must not make E3 LOCKED for T-BC. + ptpStats[metrics.MasterClockType].SetLastSyncState(ptp.LOCKED) + + if tt.setupTBC { + tbcKey := types.IFace(stats.TBCMainClockName) + ptpStats[tbcKey] = stats.NewStats(cfgName) + ptpStats[tbcKey].SetAlias("ens2fx") + ptpStats[tbcKey].SetLastSyncState(tt.e1State) + } + + eventManager.ResetMockEvent() + phc2sysLocked := fmt.Sprintf( + "phc2sys[600840.357]: [%s] CLOCK_REALTIME phc offset -34 s2 freq -4040 delay 529", + cfgName, + ) + eventManager.ExtractMetrics(phc2sysLocked) + + cStat, ok := ptpStats[metrics.ClockRealTime] + assert.True(t, ok, "CLOCK_REALTIME stats should exist") + assert.Equal(t, tt.expectedE3, cStat.LastSyncState()) + assert.Contains(t, eventManager.GetMockEvent(), ptp.OsClockSyncStateChange) + }) + } +} + +// TestRefreshProfileTypes updates cached NONE ProfileTypes from TBCProfiles. +func TestRefreshProfileTypes(t *testing.T) { + eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) + eventManager.PtpConfigMapUpdates.TBCProfiles = []string{"tbc-profile-1"} + + tbcCfg := ntpPtp4lCfgName + ocCfg := tbcPtp4l1CfgName + eventManager.AddPTPConfig(types.ConfigName(tbcCfg), &ptp4lconf.PTP4lConfig{ + Name: tbcCfg, Profile: "tbc-profile-1", ProfileType: ptp4lconf.NONE, + }) + eventManager.AddPTPConfig(types.ConfigName(ocCfg), &ptp4lconf.PTP4lConfig{ + Name: ocCfg, Profile: "oc-profile", ProfileType: ptp4lconf.NONE, + }) + + assert.Equal(t, ptp4lconf.NONE, eventManager.GetPTPConfig(types.ConfigName(tbcCfg)).ProfileType) + eventManager.RefreshProfileTypes() + assert.Equal(t, ptp4lconf.TBC, eventManager.GetPTPConfig(types.ConfigName(tbcCfg)).ProfileType) + assert.Equal(t, ptp4lconf.NONE, eventManager.GetPTPConfig(types.ConfigName(ocCfg)).ProfileType) +} + +func TestIsTBCProfile(t *testing.T) { + eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) + eventManager.PtpConfigMapUpdates.TBCProfiles = []string{"tbc-a", "tbc-b"} + + assert.True(t, eventManager.IsTBCProfile("tbc-a")) + assert.True(t, eventManager.IsTBCProfile("tbc-b")) + assert.False(t, eventManager.IsTBCProfile("oc-profile")) + assert.False(t, eventManager.IsTBCProfile("")) +} + +// TestE3NonTBCUsesMasterE1 ensures OC/BC profiles still derive E3 from master +// when the config is not a T-BC profile (GetProfileTypeByConfigName → NONE). +func TestE3NonTBCUsesMasterE1(t *testing.T) { + cfgName := tbcPtp4l1CfgName eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) eventManager.MockTest(true) - eventManager.PtpConfigMapUpdates.TBCProfiles = []string{tbcProfile} - // ProfileType intentionally NONE (stale cache after reconfig); TBCProfiles - // still lists the profile so GetProfileTypeByConfigName resolves TBC. ptp4lCfg := &ptp4lconf.PTP4lConfig{ Name: cfgName, - Profile: tbcProfile, + Profile: "oc-profile", ProfileType: ptp4lconf.NONE, Interfaces: []*ptp4lconf.PTPInterface{ {Name: "ens2f0", PortID: 1, PortName: "port 1", Role: types.SLAVE}, @@ -769,7 +879,6 @@ func TestE3DefaultsMissingE1ToFreerun(t *testing.T) { ptpStats[metrics.MasterClockType] = stats.NewStats(cfgName) ptpStats[metrics.MasterClockType].SetAlias("ens2fx") ptpStats[metrics.MasterClockType].SetRole(types.SLAVE) - // Locked master without a T-BC key must not make E3 LOCKED for T-BC. ptpStats[metrics.MasterClockType].SetLastSyncState(ptp.LOCKED) eventManager.ResetMockEvent() @@ -779,10 +888,7 @@ func TestE3DefaultsMissingE1ToFreerun(t *testing.T) { ) eventManager.ExtractMetrics(phc2sysLocked) - cStat, ok := ptpStats[metrics.ClockRealTime] - assert.True(t, ok, "CLOCK_REALTIME stats should exist") - assert.Equal(t, ptp.FREERUN, cStat.LastSyncState(), - "E3 must stay FREERUN when E1/T-BC has not reported yet") - assert.Contains(t, eventManager.GetMockEvent(), ptp.OsClockSyncStateChange, - "should publish OS-clock FREERUN (not LOCKED) when E1 is unset") + cStat := ptpStats[metrics.ClockRealTime] + assert.Equal(t, ptp.LOCKED, cStat.LastSyncState(), + "non-T-BC E3 may LOCKED when master E1 is LOCKED") } From dcb7db988e53280d679cf5814ff367176bfbbf0a Mon Sep 17 00:00:00 2001 From: nocturnalastro Date: Tue, 28 Jul 2026 15:43:14 +0100 Subject: [PATCH 4/6] Isolate pubsub test stores and ports for parallel go test Packages sharing STORE_PATH and fixed API ports raced on pub.json and CreatePublisher during parallel package runs. Generated-by: Cursor --- cmd/main_test.go | 36 ++++++++++----- pkg/plugins/handler_test.go | 39 +++++++++++++--- .../ptp_operator/ptp_operator_plugin_test.go | 44 +++++++++++++++++-- 3 files changed, 98 insertions(+), 21 deletions(-) diff --git a/cmd/main_test.go b/cmd/main_test.go index 744e0dd3..98f8d729 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -5,18 +5,19 @@ package main import ( "fmt" + "net" + "os" + "sync" + "testing" + v2 "github.com/cloudevents/sdk-go/v2" "k8s.io/utils/pointer" - "os" "github.com/redhat-cne/cloud-event-proxy/pkg/common" ceEvent "github.com/redhat-cne/sdk-go/pkg/event" "github.com/redhat-cne/sdk-go/pkg/event/ptp" "github.com/redhat-cne/sdk-go/pkg/types" - "sync" - "testing" - "github.com/redhat-cne/sdk-go/pkg/channel" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -40,11 +41,27 @@ func storeCleanUp() { } } +func freeTCPPort() (int, error) { + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + return 0, err + } + port := l.Addr().(*net.TCPAddr).Port + _ = l.Close() + return port, nil +} + func TestSidecar_Main(t *testing.T) { - apiPort = 8990 + port, err := freeTCPPort() + assert.NoError(t, err) + apiPort = port - // Create a unique temporary directory for this test run to avoid conflicts - tempDir, err := os.MkdirTemp("", "sidecar-test-*") + // Unique store under STORE_PATH (if set) so parallel packages do not share pub.json. + base := os.TempDir() + if sPath, ok := os.LookupEnv("STORE_PATH"); ok && sPath != "" { + base = sPath + } + tempDir, err := os.MkdirTemp(base, "sidecar-test-*") assert.NoError(t, err) defer func() { storeCleanUp() @@ -52,10 +69,7 @@ func TestSidecar_Main(t *testing.T) { }() wg := &sync.WaitGroup{} - var storePath = tempDir - if sPath, ok := os.LookupEnv("STORE_PATH"); ok && sPath != "" { - storePath = sPath - } + storePath := tempDir scConfig = &common.SCConfiguration{ EventInCh: make(chan *channel.DataChan, channelBufferSize), EventOutCh: make(chan *channel.DataChan, channelBufferSize), diff --git a/pkg/plugins/handler_test.go b/pkg/plugins/handler_test.go index 49812669..bee196cc 100644 --- a/pkg/plugins/handler_test.go +++ b/pkg/plugins/handler_test.go @@ -19,11 +19,11 @@ package plugins import ( "fmt" + "net" + "os" "sync" "testing" - "os" - "github.com/redhat-cne/cloud-event-proxy/pkg/common" "github.com/redhat-cne/sdk-go/pkg/channel" v1pubsub "github.com/redhat-cne/sdk-go/v1/pubsub" @@ -38,17 +38,41 @@ var ( channelBufferSize int = 10 ) -func init() { - var storePath = "." +func freeTCPPort() (int, error) { + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + return 0, err + } + port := l.Addr().(*net.TCPAddr).Port + _ = l.Close() + return port, nil +} + +func uniqueStorePath(prefix string) (string, error) { + base := os.TempDir() if sPath, ok := os.LookupEnv("STORE_PATH"); ok && sPath != "" { - storePath = sPath + base = sPath + } + return os.MkdirTemp(base, prefix) +} + +func TestMain(m *testing.M) { + storePath, err := uniqueStorePath("plugins-handler-*") + if err != nil { + fmt.Fprintf(os.Stderr, "unique store path: %v\n", err) + os.Exit(1) + } + apiPort, err := freeTCPPort() + if err != nil { + fmt.Fprintf(os.Stderr, "free tcp port: %v\n", err) + os.Exit(1) } scConfig = &common.SCConfiguration{ EventInCh: make(chan *channel.DataChan, channelBufferSize), EventOutCh: make(chan *channel.DataChan, channelBufferSize), CloseCh: make(chan struct{}), - APIPort: 8989, + APIPort: apiPort, APIPath: "/api/cne/", PubSubAPI: v1pubsub.GetAPIInstance("../.."), SubscriberAPI: subscriberApi.GetAPIInstance(storePath), @@ -62,6 +86,9 @@ func init() { Err: nil, }, } + code := m.Run() + _ = os.RemoveAll(storePath) + os.Exit(code) } func TestLoadPTPPlugin(t *testing.T) { diff --git a/plugins/ptp_operator/ptp_operator_plugin_test.go b/plugins/ptp_operator/ptp_operator_plugin_test.go index 3cc12988..254b8196 100644 --- a/plugins/ptp_operator/ptp_operator_plugin_test.go +++ b/plugins/ptp_operator/ptp_operator_plugin_test.go @@ -66,8 +66,37 @@ var ( nodeName = "test_node" ) +func freeTCPPort() (int, error) { + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + return 0, err + } + port := l.Addr().(*net.TCPAddr).Port + _ = l.Close() + return port, nil +} + +func uniqueStorePath(prefix string) (string, error) { + base := os.TempDir() + if sPath, ok := os.LookupEnv("STORE_PATH"); ok && sPath != "" { + base = sPath + } + return os.MkdirTemp(base, prefix) +} + func TestMain(m *testing.M) { defer cleanUP() + var err error + storePath, err = uniqueStorePath("ptp-operator-plugin-*") + if err != nil { + log.Fatalf("unique store path: %v", err) + } + defer os.RemoveAll(storePath) + + apiPort, err = freeTCPPort() + if err != nil { + log.Fatalf("free tcp port: %v", err) + } scConfig = &common.SCConfiguration{ EventInCh: make(chan *channel.DataChan, channelBufferSize), EventOutCh: make(chan *channel.DataChan, channelBufferSize), @@ -79,9 +108,9 @@ func TestMain(m *testing.M) { StorePath: storePath, TransportHost: &common.TransportHost{ Type: common.HTTP, - URL: "localhost:8990", + URL: fmt.Sprintf("localhost:%d", apiPort), Host: "localhost", - Port: 8990, + Port: apiPort, Err: nil, }, BaseURL: nil, @@ -95,8 +124,15 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } func cleanUP() { - _, _ = scConfig.SubscriberAPI.DeleteAllSubscriptions() - _ = scConfig.PubSubAPI.DeleteAllPublishers() + if scConfig == nil { + return + } + if scConfig.SubscriberAPI != nil { + _, _ = scConfig.SubscriberAPI.DeleteAllSubscriptions() + } + if scConfig.PubSubAPI != nil { + _ = scConfig.PubSubAPI.DeleteAllPublishers() + } } // ProcessInChannel will be called if Transport is disabled From abb80ad1af640b0b4832d30e0ad1b25d095be2f6 Mon Sep 17 00:00:00 2001 From: nocturnalastro Date: Wed, 29 Jul 2026 10:36:47 +0100 Subject: [PATCH 5/6] Clean up TestMain store explicitly before os.Exit os.Exit skips deferred cleanup, so run cleanUP and RemoveAll before exiting while keeping defers for panic paths. Generated-by: Cursor --- plugins/ptp_operator/ptp_operator_plugin_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/plugins/ptp_operator/ptp_operator_plugin_test.go b/plugins/ptp_operator/ptp_operator_plugin_test.go index 254b8196..17f2aefd 100644 --- a/plugins/ptp_operator/ptp_operator_plugin_test.go +++ b/plugins/ptp_operator/ptp_operator_plugin_test.go @@ -121,7 +121,11 @@ func TestMain(m *testing.M) { common.StartPubSubService(scConfig) pubsubTypes = InitPubSubTypes() scConfig.RestAPI.SetOnStatusReceiveOverrideFn(getMockOverrideFn()) - os.Exit(m.Run()) + code := m.Run() + // os exit skips defer + cleanUP() + _ = os.RemoveAll(storePath) + os.Exit(code) } func cleanUP() { if scConfig == nil { From 986fa985a706212f0064b8e7c432810d6bbde0fc Mon Sep 17 00:00:00 2001 From: nocturnalastro Date: Tue, 4 Aug 2026 14:34:11 +0100 Subject: [PATCH 6/6] Scope missing-E1 FREERUN default to T-BC/T-GM for E3 OC/BC set E1 from ptp4l, so leave unset E1 alone and only force FREERUN for profiles that rely on delayed X-STATUS. Also hold the profile-type lookup lock for the full GetProfileTypeByConfigName call. Generated-by: Cursor --- plugins/ptp_operator/metrics/manager.go | 2 +- plugins/ptp_operator/metrics/metrics.go | 27 ++++---- plugins/ptp_operator/metrics/tbc_test.go | 82 ++++++++++++++++-------- 3 files changed, 72 insertions(+), 39 deletions(-) diff --git a/plugins/ptp_operator/metrics/manager.go b/plugins/ptp_operator/metrics/manager.go index fc40ce85..064513f5 100644 --- a/plugins/ptp_operator/metrics/manager.go +++ b/plugins/ptp_operator/metrics/manager.go @@ -666,8 +666,8 @@ func (p *PTPEventManager) GetProfileType(name string) ptp4lconf.PtpProfileType { // name (e.g. "ptp4l.0.config"). Returns NONE if the config is unknown. func (p *PTPEventManager) GetProfileTypeByConfigName(configName types.ConfigName) ptp4lconf.PtpProfileType { p.lock.RLock() + defer p.lock.RUnlock() cfg, ok := p.Ptp4lConfigInterfaces[configName] - p.lock.RUnlock() if !ok || cfg == nil { return ptp4lconf.NONE } diff --git a/plugins/ptp_operator/metrics/metrics.go b/plugins/ptp_operator/metrics/metrics.go index d84ca7b3..cd20437b 100644 --- a/plugins/ptp_operator/metrics/metrics.go +++ b/plugins/ptp_operator/metrics/metrics.go @@ -284,24 +284,27 @@ func (p *PTPEventManager) ExtractMetrics(msg string) { switch interfaceName { //note: this is not interface type case ClockRealTime: // CLOCK_REALTIME is active slave interface // O-RAN O-Cloud API v04.00 Table 37: E3 = worst_of(phc2sys_state, E1_state). - // phc2sys is the single publisher — but if the upstream PHC is not - // traceable (E1 != LOCKED), E3 cannot be LOCKED even when - // phc2sys offset is within threshold. - // GetMainClockName() returns the correct key per profile type: - // T-BC → "T-BC", T-GM → "GM", OC/BC → "master" - // Prefer the profile-type key for T-BC/T-GM so a missing T-BC - // entry after reconfig is not mistaken for OC "master". + // phc2sys publishes E3, but E3 cannot be LOCKED unless E1 is also LOCKED. + + // E1 lookup key: T-BC → "T-BC"; T-GM/OC/BC → "master". + // T-BC/T-GM set E1 from delayed X-STATUS lines (T-BC-STATUS / T-GM-STATUS), + // so missing E1 defaults to FREERUN. OC/BC set E1 from ptp4l, so leave it unset. + var e1State ptp.SyncState mainClockKey := ptpStats.GetMainClockName() - if p.GetProfileTypeByConfigName(types.ConfigName(configName)) == ptp4lconf.TBC { + switch p.GetProfileTypeByConfigName(types.ConfigName(configName)) { + case ptp4lconf.TBC: mainClockKey = types.IFace(stats.TBCMainClockName) + e1State = ptp.FREERUN + case ptp4lconf.TGM: + e1State = ptp.FREERUN } - // Missing or unset E1 defaults to FREERUN so a lone OS-clock - // LOCKED sample cannot make E3 LOCKED early after reconfig. - e1State := ptp.FREERUN if e1Stat, ok := ptpStats[mainClockKey]; ok && e1Stat.LastSyncState() != "" { e1State = e1Stat.LastSyncState() } - syncState = OverallState(syncState, e1State) + if e1State != "" { + syncState = OverallState(syncState, e1State) + } + // for HA we can not rely on master ;since there will be 2 or more leaders; this condition will be skipped // ptpStats clock realtime has its own stats objects if r, ok := ptpStats[master]; ok && r.Role() == types.SLAVE { // publish event only if the master role is active diff --git a/plugins/ptp_operator/metrics/tbc_test.go b/plugins/ptp_operator/metrics/tbc_test.go index ef6bc13f..feea8436 100644 --- a/plugins/ptp_operator/metrics/tbc_test.go +++ b/plugins/ptp_operator/metrics/tbc_test.go @@ -858,37 +858,67 @@ func TestIsTBCProfile(t *testing.T) { assert.False(t, eventManager.IsTBCProfile("")) } -// TestE3NonTBCUsesMasterE1 ensures OC/BC profiles still derive E3 from master -// when the config is not a T-BC profile (GetProfileTypeByConfigName → NONE). +// TestE3NonTBCUsesMasterE1 ensures OC/BC profiles do not default missing E1 +// to FREERUN (unlike T-BC/T-GM). E3 follows master when set, else phc2sys alone. func TestE3NonTBCUsesMasterE1(t *testing.T) { cfgName := tbcPtp4l1CfgName - eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) - eventManager.MockTest(true) - - ptp4lCfg := &ptp4lconf.PTP4lConfig{ - Name: cfgName, - Profile: "oc-profile", - ProfileType: ptp4lconf.NONE, - Interfaces: []*ptp4lconf.PTPInterface{ - {Name: "ens2f0", PortID: 1, PortName: "port 1", Role: types.SLAVE}, + tests := []struct { + name string + setupMaster bool + masterE1 ptp.SyncState + expectedE3 ptp.SyncState + }{ + { + name: "master LOCKED allows E3 LOCKED", + setupMaster: true, + masterE1: ptp.LOCKED, + expectedE3: ptp.LOCKED, + }, + { + name: "missing master E1 does not default FREERUN", + setupMaster: false, + expectedE3: ptp.LOCKED, + }, + { + name: "empty master LastSyncState does not default FREERUN", + setupMaster: true, + masterE1: "", + expectedE3: ptp.LOCKED, }, } - eventManager.AddPTPConfig(types.ConfigName(cfgName), ptp4lCfg) - ptpStats := eventManager.GetStats(types.ConfigName(cfgName)) - ptpStats[metrics.MasterClockType] = stats.NewStats(cfgName) - ptpStats[metrics.MasterClockType].SetAlias("ens2fx") - ptpStats[metrics.MasterClockType].SetRole(types.SLAVE) - ptpStats[metrics.MasterClockType].SetLastSyncState(ptp.LOCKED) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + eventManager := metrics.NewPTPEventManager("", initPubSubTypes(), "testnode", &common.SCConfiguration{StorePath: "/tmp/store"}) + eventManager.MockTest(true) - eventManager.ResetMockEvent() - phc2sysLocked := fmt.Sprintf( - "phc2sys[600840.357]: [%s] CLOCK_REALTIME phc offset -34 s2 freq -4040 delay 529", - cfgName, - ) - eventManager.ExtractMetrics(phc2sysLocked) + ptp4lCfg := &ptp4lconf.PTP4lConfig{ + Name: cfgName, + Profile: "oc-profile", + ProfileType: ptp4lconf.NONE, + Interfaces: []*ptp4lconf.PTPInterface{ + {Name: "ens2f0", PortID: 1, PortName: "port 1", Role: types.SLAVE}, + }, + } + eventManager.AddPTPConfig(types.ConfigName(cfgName), ptp4lCfg) - cStat := ptpStats[metrics.ClockRealTime] - assert.Equal(t, ptp.LOCKED, cStat.LastSyncState(), - "non-T-BC E3 may LOCKED when master E1 is LOCKED") + ptpStats := eventManager.GetStats(types.ConfigName(cfgName)) + if tt.setupMaster { + ptpStats[metrics.MasterClockType] = stats.NewStats(cfgName) + ptpStats[metrics.MasterClockType].SetAlias("ens2fx") + ptpStats[metrics.MasterClockType].SetRole(types.SLAVE) + ptpStats[metrics.MasterClockType].SetLastSyncState(tt.masterE1) + } + + eventManager.ResetMockEvent() + phc2sysLocked := fmt.Sprintf( + "phc2sys[600840.357]: [%s] CLOCK_REALTIME phc offset -34 s2 freq -4040 delay 529", + cfgName, + ) + eventManager.ExtractMetrics(phc2sysLocked) + + cStat := ptpStats[metrics.ClockRealTime] + assert.Equal(t, tt.expectedE3, cStat.LastSyncState()) + }) + } }