[WIP] CNF-24539: T-BC TR ptp4l kill fires HOLDOVER instead of FREERUN - #695
Open
jzding wants to merge 1 commit into
Open
[WIP] CNF-24539: T-BC TR ptp4l kill fires HOLDOVER instead of FREERUN#695jzding wants to merge 1 commit into
jzding wants to merge 1 commit into
Conversation
When the TR (time receiver) ptp4l is killed on a T-BC with DPLL hardware, the DPLL stays locked (LHAQ) and T-BC-STATUS never leaves s2. The clock is in holdover, not freerun. - processDownEvent: distinguish TR (has phc2sys) from TT via Phc2SysEnabled(). TR fires HOLDOVER + starts holdover timer. TT fires FREERUN (downstream clients lose sync). - TR ptp4l kill suppresses OsClockSyncStateChange — DPLL keeps PHC locked, CLOCK_REALTIME stays synced. - Add startTBCHoldoverTimer/handleTBCHoldOverState: if ptp4l doesn't recover before timeout, transitions to FREERUN. - ParseTBCLogs: cancel holdover timer on HOLDOVER→LOCKED recovery. - Tests: TestTBCProcessDownEventTR expects HOLDOVER, new TestTBCProcessDownEventTT expects FREERUN. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jack Ding <jackding@gmail.com>
|
@jzding: This pull request references CNF-24539 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jzding The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR implements T-BC holdover timer management for Telecom Grandmaster clocks. When a T-BC's ptp4l process dies and phc2sys is enabled, a timer starts in HOLDOVER state; if not recovered to LOCKED before timeout, it auto-transitions to FREERUN. Process-down logic now distinguishes TR (phc2sys) from non-TR profiles, suppressing spurious clock sync events for TR. ChangesT-BC Holdover Timer
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/ptp_operator/metrics/metrics.go (1)
394-420:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTR down path can silently skip HOLDOVER + timer and still early-return.
At Line 396, HOLDOVER/timer startup depends on
ptpStats[tbcKey]existing with a non-empty alias; at Line 419, the function still returns for TR. If that T-BC stat key isn’t initialized yet, TRptp4ldown emits nothing and no timer is started.Suggested fix
if profileType == ptp4lconf.TBC && processName == ptp4lProcessName { tbcKey := types.IFace(stats.TBCMainClockName) - if tbcStat, ok := ptpStats[tbcKey]; ok && tbcStat.Alias() != "" { - masterResource := fmt.Sprintf("%s/%s", tbcStat.Alias(), MasterClockType) - ptpOpts := p.PtpConfigMapUpdates.LookupPtpProcessOpts(profileName) - isTR := ptpOpts != nil && ptpOpts.Phc2SysEnabled() - if isTR { - p.GenPTPEvent(profileName, tbcStat, masterResource, FreeRunOffsetValue, ptp.HOLDOVER, ptp.PtpStateChange) - p.startTBCHoldoverTimer(ptpOpts, configName, profileName, tbcStat.Alias()) - } else { - p.GenPTPEvent(profileName, tbcStat, masterResource, FreeRunOffsetValue, ptp.FREERUN, ptp.PtpStateChange) - } - } + ptpStats.CheckSource(tbcKey, configName, ts2phcProcessName) + tbcStat := ptpStats[tbcKey] + if tbcStat.Alias() == "" { + if m, ok := ptpStats[master]; ok && m.Alias() != "" { + tbcStat.SetAlias(m.Alias()) + } + } + if tbcStat.Alias() != "" { + masterResource := fmt.Sprintf("%s/%s", tbcStat.Alias(), MasterClockType) + ptpOpts := p.PtpConfigMapUpdates.LookupPtpProcessOpts(profileName) + isTR := ptpOpts != nil && ptpOpts.Phc2SysEnabled() + if isTR { + p.GenPTPEvent(profileName, tbcStat, masterResource, FreeRunOffsetValue, ptp.HOLDOVER, ptp.PtpStateChange) + p.startTBCHoldoverTimer(ptpOpts, configName, profileName, tbcStat.Alias()) + } else { + p.GenPTPEvent(profileName, tbcStat, masterResource, FreeRunOffsetValue, ptp.FREERUN, ptp.PtpStateChange) + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ptp_operator/metrics/metrics.go` around lines 394 - 420, The TR early-return is happening unconditionally for profileType == ptp4lconf.TBC which can skip the tbcKey branch that should start HOLDOVER and startTBCHoldoverTimer; change the control flow so you lookup ptpOpts (via p.PtpConfigMapUpdates.LookupPtpProcessOpts(profileName)) and only perform an early return after you have attempted the tbcKey handling — i.e., ensure the existing tbcKey block (using tbcKey := types.IFace(stats.TBCMainClockName), ptpStats[tbcKey], p.GenPTPEvent(..., ptp.HOLDOVER...), and p.startTBCHoldoverTimer(...)) always runs before you return for TR, or alternatively move the ptpOpts.Phc2SysEnabled() check earlier so you can invoke the HOLDOVER + startTBCHoldoverTimer when tbcStat exists and only return if TR has been handled (do not return silently when tbcStat/alias was missing).
🧹 Nitpick comments (1)
plugins/ptp_operator/metrics/tbc_test.go (1)
405-471: ⚡ Quick winTR test currently bypasses the new timer path.
Line 407 enables mock mode, and
startTBCHoldoverTimershort-circuits in mock mode, so this test does not verify auto-expire-to-FREERUN or cancellation-on-LOCKED recovery. Add a non-mock unit with a short timeout (or directhandleTBCHoldOverStateinvocation) to cover the new timer behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ptp_operator/metrics/tbc_test.go` around lines 405 - 471, TestTBCProcessDownEventTR currently sets eventManager.MockTest(true) which causes startTBCHoldoverTimer to short-circuit and skip the new timer path; change the test to exercise the timer by either creating a non-mock eventManager (MockTest(false) or remove the mock call) and configuring a short holdover timeout before calling startTBCHoldoverTimer so the timer will auto-expire to FREERUN, or keep the mock but explicitly invoke handleTBCHoldOverState with the tbcKey after a small sleep to simulate timer expiry; ensure assertions check for FREERUN on expiry and that a subsequent ParseTBCLogs/LOCKED cancels the timer and returns to LOCKED (referencing TestTBCProcessDownEventTR, eventManager, startTBCHoldoverTimer, handleTBCHoldOverState, and tbcKey).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ptp_operator/metrics/logparser.go`:
- Around line 496-505: The current loop over p.PtpConfigMapUpdates.TBCProfiles
cancels the first found TR event threshold (via
LookupEventThreshold(...).SafeClose()) rather than the threshold for the
specific recovering profile, risking cancelling the wrong timer; change the
logic to target only the recovering profile instead of iterating all TBCProfiles
— i.e., use the recovering profile identifier (the profile that just entered
LOCKED recovery) and call
p.PtpConfigMapUpdates.LookupPtpProcessOpts(recoveringProfile).Phc2SysEnabled()
and p.PtpConfigMapUpdates.LookupEventThreshold(recoveringProfile) and, if
non-nil, call SafeClose() on that threshold; keep use of LookupPtpProcessOpts,
Phc2SysEnabled, LookupEventThreshold and SafeClose to locate and close only the
matching timer.
---
Outside diff comments:
In `@plugins/ptp_operator/metrics/metrics.go`:
- Around line 394-420: The TR early-return is happening unconditionally for
profileType == ptp4lconf.TBC which can skip the tbcKey branch that should start
HOLDOVER and startTBCHoldoverTimer; change the control flow so you lookup
ptpOpts (via p.PtpConfigMapUpdates.LookupPtpProcessOpts(profileName)) and only
perform an early return after you have attempted the tbcKey handling — i.e.,
ensure the existing tbcKey block (using tbcKey :=
types.IFace(stats.TBCMainClockName), ptpStats[tbcKey], p.GenPTPEvent(...,
ptp.HOLDOVER...), and p.startTBCHoldoverTimer(...)) always runs before you
return for TR, or alternatively move the ptpOpts.Phc2SysEnabled() check earlier
so you can invoke the HOLDOVER + startTBCHoldoverTimer when tbcStat exists and
only return if TR has been handled (do not return silently when tbcStat/alias
was missing).
---
Nitpick comments:
In `@plugins/ptp_operator/metrics/tbc_test.go`:
- Around line 405-471: TestTBCProcessDownEventTR currently sets
eventManager.MockTest(true) which causes startTBCHoldoverTimer to short-circuit
and skip the new timer path; change the test to exercise the timer by either
creating a non-mock eventManager (MockTest(false) or remove the mock call) and
configuring a short holdover timeout before calling startTBCHoldoverTimer so the
timer will auto-expire to FREERUN, or keep the mock but explicitly invoke
handleTBCHoldOverState with the tbcKey after a small sleep to simulate timer
expiry; ensure assertions check for FREERUN on expiry and that a subsequent
ParseTBCLogs/LOCKED cancels the timer and returns to LOCKED (referencing
TestTBCProcessDownEventTR, eventManager, startTBCHoldoverTimer,
handleTBCHoldOverState, and tbcKey).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 1c0f7f98-fbf9-4d7b-86e2-0faf0fa3099f
📒 Files selected for processing (4)
plugins/ptp_operator/metrics/logparser.goplugins/ptp_operator/metrics/metrics.goplugins/ptp_operator/metrics/ptp4lParse.goplugins/ptp_operator/metrics/tbc_test.go
Comment on lines
+496
to
+505
| for _, tbcProfile := range p.PtpConfigMapUpdates.TBCProfiles { | ||
| ptpOpts := p.PtpConfigMapUpdates.LookupPtpProcessOpts(tbcProfile) | ||
| if ptpOpts != nil && ptpOpts.Phc2SysEnabled() { | ||
| if t := p.PtpConfigMapUpdates.LookupEventThreshold(tbcProfile); t != nil { | ||
| log.Infof("T-BC: cancelling holdover timer on LOCKED recovery: profile=%s", tbcProfile) | ||
| t.SafeClose() | ||
| } | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Timer cancellation is not scoped to the recovering profile.
At Line 496, cancellation iterates all TBCProfiles and closes the first TR threshold found. On nodes with multiple TR T-BC profiles, this can cancel the wrong timer and let the real one expire into a false FREERUN.
Suggested fix
if lastClockState == ptp.HOLDOVER && clockState.State == ptp.LOCKED {
- for _, tbcProfile := range p.PtpConfigMapUpdates.TBCProfiles {
- ptpOpts := p.PtpConfigMapUpdates.LookupPtpProcessOpts(tbcProfile)
- if ptpOpts != nil && ptpOpts.Phc2SysEnabled() {
- if t := p.PtpConfigMapUpdates.LookupEventThreshold(tbcProfile); t != nil {
- log.Infof("T-BC: cancelling holdover timer on LOCKED recovery: profile=%s", tbcProfile)
- t.SafeClose()
- }
- break
- }
- }
+ cfg := p.GetPTPConfig(types.ConfigName(configName))
+ if cfg != nil {
+ tbcProfile := cfg.Profile
+ if ptpOpts := p.PtpConfigMapUpdates.LookupPtpProcessOpts(tbcProfile); ptpOpts != nil && ptpOpts.Phc2SysEnabled() {
+ if t := p.PtpConfigMapUpdates.LookupEventThreshold(tbcProfile); t != nil {
+ log.Infof("T-BC: cancelling holdover timer on LOCKED recovery: profile=%s", tbcProfile)
+ t.SafeClose()
+ }
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/ptp_operator/metrics/logparser.go` around lines 496 - 505, The
current loop over p.PtpConfigMapUpdates.TBCProfiles cancels the first found TR
event threshold (via LookupEventThreshold(...).SafeClose()) rather than the
threshold for the specific recovering profile, risking cancelling the wrong
timer; change the logic to target only the recovering profile instead of
iterating all TBCProfiles — i.e., use the recovering profile identifier (the
profile that just entered LOCKED recovery) and call
p.PtpConfigMapUpdates.LookupPtpProcessOpts(recoveringProfile).Phc2SysEnabled()
and p.PtpConfigMapUpdates.LookupEventThreshold(recoveringProfile) and, if
non-nil, call SafeClose() on that threshold; keep use of LookupPtpProcessOpts,
Phc2SysEnabled, LookupEventThreshold and SafeClose to locate and close only the
matching timer.
jzding
changed the title
Contributor
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the TR (time receiver) ptp4l is killed on a T-BC with DPLL hardware, the DPLL stays locked (LHAQ) and T-BC-STATUS never leaves s2. The clock is in holdover, not freerun.