T-BC tests

[nocturnalastro]

Summary by CodeRabbit

• New Features

  • Added support for Telco Boundary Clock (T-BC) PTP mode, including receiver/transmitter profile generation
  • Introduced PTP announce data monitoring with clock quality tracking for locked/holdover transition validation

• Improvements

  • Enhanced clock-class/profile verification across additional boundary clock scenarios and improved config/profile discovery fallbacks
  • Improved E810 plugin configuration handling, restart/mutation workflow, and interface pin selection
  • Updated environment tooling to propagate KUBE_RBAC_PROXY_IMAGE into generated manager env configuration

• Documentation

  • Documented MAX_IN_SPEC_OFFSET_NS in test environment variables

• Tests

  • Expanded T-BC synchronization and GM-announcement transition tests

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly expands the conformance test suite by introducing detailed scenarios for Telco Boundary Clock (T-BC) operation, including its behavior during various holdover states and re-synchronization. It also enhances the test framework's configurability with a new environment variable for E810 plugin settings and improves the robustness of PTP-related helper functions and logging mechanisms.

Highlights

• Telco Boundary Clock (T-BC) Conformance Tests: Introduced comprehensive new test cases for Telco Boundary Clock (T-BC) functionality, covering GM announcements, holdover transitions (in-spec, out-of-spec, freerun), and re-locking scenarios.
• New Environment Variable for E810 Plugin: Added MAX_IN_SPEC_OFFSET_NS environment variable to configure the maximum in-spec offset for the E810 plugin, allowing fine-grained control over holdover specification thresholds.
• PTP Helper Function Refactoring and Robustness: Refactored several PTP helper functions (GetProfileLogID, GetClockIDMaster, GetClockIDForeign, WaitForClockIDForeign) to improve pod discovery, configuration retrieval, and error handling, enhancing overall test reliability.
• PMC Monitoring Utility: Implemented a new pmc package for continuously monitoring and parsing PTP Management Client (PMC) output, enabling detailed verification of PTP announce messages and clock state transitions during tests.
• Enhanced Test Logging: Integrated logging.WriteStep calls throughout the test suite to provide clearer, step-by-step progress markers in test logs, aiding in debugging and understanding test flow.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[nocturnalastro]

/gemini review

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[sebsoto]

Submitting this now because I'm taking too long :P
Still trying to wrap my head around the actual logic being introduced here, would appreciate someone else's review as well.

[sebsoto]

@nocturnalastro changes LGTM
Can approve after you rebase

[coderabbitai]

Warning

Review limit reached

@nocturnalastro, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 37 minutes and 4 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 05ca9ed9-5fc1-4e9d-8c3c-7c0526926a9d

📥 Commits

Reviewing files that changed from the base of the PR and between ea949c6 and 91b7e0b.

📒 Files selected for processing (12)

• Makefile
• test/README.md
• test/conformance/serial/ptp.go
• test/pkg/clean/clean.go
• test/pkg/consts.go
• test/pkg/logging/collector.go
• test/pkg/metrics/metrics.go
• test/pkg/pmc/pmc.go
• test/pkg/ptphelper/ptphelper.go
• test/pkg/ptptesthelper/ptptesthelper.go
• test/pkg/testconfig/testconfig.go
• test/pkg/testconfig/testconfig_test.go

Walkthrough

This PR adds end-to-end test support for the Telco Boundary Clock (T-BC) PTP mode. It introduces a new pmc monitoring package, e810 plugin mutation helpers, WPC interface grouping, profile-name qualification, T-BC solver definitions and PTP config creation, and a new holdover-transition test. Several existing utilities are updated for T-BC compatibility.

Changes

Telco Boundary Clock test support

Layer / File(s)	Summary
Shared constants, metrics, cleanup, and Makefile test/pkg/consts.go, test/pkg/metrics/metrics.go, test/pkg/clean/clean.go, test/pkg/logging/collector.go, test/README.md, Makefile	Adds PTPWPCTBCPolicyName constant and MaxInSpecOffsetNs/MaxInSpecOffsetDefaultNs metrics variables; extends Configs() cleanup to delete the new TBC policy; moves WriteStep to a later position in the file with a comment header; documents MAX_IN_SPEC_OFFSET_NS in the test README; and adds OS-specific sed update for KUBE_RBAC_PROXY_IMAGE to the Makefile update-env-yaml target.
PMC monitor utility test/pkg/pmc/pmc.go	New package defining AnnounceData and ClockQuality exported structs with String() and CheckSame() comparison methods that perform case-insensitive field matching and accumulate mismatches. Implements a ticker-driven Monitor type that executes sequential PMC commands against a Kubernetes pod, parses responses into AnnounceData, optionally refreshes the pod on errors, and delivers results over non-blocking buffered channels. Includes a Query() one-shot helper.
ptphelper: profile naming and config discovery test/pkg/ptphelper/ptphelper.go	Adds QualifyProfileName to prefix profile names with crName_, updates GetProfileName to accept a receiverOnly bool parameter and qualify returned names, adds GetConfigForProfile to resolve profile config paths by normalizing log-derived run-ids or /var/run scanning, adds GetConfigForProfileFromVarRun to scan daemon pod /var/run for ptp4l.*.config files matching a profile regex, and updates GetProfileLogID to use the new fallback path when log parsing fails.
ptphelper: e810 mutation, WPC interface grouping, and clock ID test/pkg/ptphelper/ptphelper.go	Adds ApplyE810PluginMutation to fetch the in-cluster PtpConfig, mutate e810 plugin settings (holdover parameters and pins), update the config, restart the daemon pod, and return a cleanup closure. Adds mutateE810PluginSettings to parse e810 plugin JSON/YAML, update holdover fields, and populate pins from WPC interface discovery. Adds WPC interface grouping helpers to select interfaces by PHC and pinning status. Adds GetLocalClockID to compute a node's PTP clock identity from e810 pins and L2 discovery data.
NICInfo config path refactoring test/pkg/ptptesthelper/ptptesthelper.go	Replaces NICInfo.ConfigFile string with ConfigDir, ConfigName, and PMCConfigName fields; adds ConfigPath() and PMCConfigPath() helper methods to compute full paths. Updates DiscoverNICInfo to derive ConfigName by trimming DefaultConfigDir, updates BasicClockSyncCheck and VerifyNICClockClass to use the new path helpers and the updated GetProfileName signature with receiverOnly parameter.
T-BC mode enum, solver definitions, and discovery test/pkg/testconfig/testconfig.go	Adds TelcoBoundaryClock to the PTPMode enum and TelcoBoundaryClockString constant; defines AlgoTelcoBC* solver algorithm constants; extends enabledProblems and initAndSolveProblems to include T-BC solver constraints and role-to-algorithm mappings; updates GetDesiredConfig to recognize T-BC mode; and extends discoverMode to detect T-BC by verifying both tbc-tr and tbc-tt profiles are present in the recovered config.
T-BC PTP config creation and WPC GM updates test/pkg/testconfig/testconfig.go, test/pkg/testconfig/testconfig_test.go	Adds CreatePtpConfigTelcoBoundaryClock() to build receiver/transmitter profiles with ts2phc, phc2sys, and E810 plugin YAML generation. Adds PtpConfigTelcoBC() to select solver solution, validate NIC constraints, create the T-BC config, and optionally create a local GM. Adds createTelcoBCConfig() helper to assemble the multi-profile PtpConfig CR. Updates CreatePtpConfigWPCGrandMaster to use dynamic values from ptptestconfig.GetPtpTestConfig() and metrics, fixes CreatePtpConfigGrandMaster scheduling-priority argument, and adds MAX_IN_SPEC_OFFSET_NS initialization. Adds TelcoBoundaryClock mock test case with helper functions generateTelcoBCConfig, generateTs2phcConfig, and mockTelcoBCPlugins.
T-BC test scenario updates and holdover transition test test/conformance/serial/ptp.go	Updates clock sync loop to use a tbc-(tr|tt) regex for T-BC profile matching. Reworks downstream sync test to qualify profile names using QualifyProfileName() for T-BC. Adds T-BC case to higher-priority-profile switch using PTPWPCTBCPolicyName. Adds fallback config resolution via GetConfigForProfileFromVarRun when logs lack profile info. Switches gmCapable verification to use ConfigPath() instead of ConfigFile. Changes FIFO scheduling to use qualified profile names. Adds a new comprehensive holdover-transition test for T-BC that mutates e810 settings, runs a PMC monitor, verifies locked-mode announcement field consistency, triggers holdover by disabling upstream interfaces, and asserts the full clock-class transition sequence (6→135→165→248→6).

Sequence Diagram(s)

sequenceDiagram
  rect rgba(135, 100, 200, 0.5)
    Note over HoldoverTest,ClockClassSeq: T-BC Holdover Transition Test
  end
  participant HoldoverTest as Holdover Test
  participant ApplyE810PluginMutation as ApplyE810PluginMutation
  participant PMCMonitor as pmc.Monitor
  participant UpstreamIface as Upstream Interface
  participant ClockClassSeq as Clock-class Verifier

  HoldoverTest->>ApplyE810PluginMutation: mutate e810 holdover settings (offset, timeout, maxInSpec)
  ApplyE810PluginMutation-->>HoldoverTest: cleanup func
  HoldoverTest->>PMCMonitor: NewMonitor(pod, configFile, interval)
  HoldoverTest->>PMCMonitor: Start() → DataChan
  loop Locked state wait
    PMCMonitor-->>HoldoverTest: AnnounceData (ClockClass=6)
  end
  HoldoverTest->>HoldoverTest: CheckSame TT vs TR AnnounceData
  HoldoverTest->>UpstreamIface: disable upstream slave interfaces
  loop Holdover/out-of-spec/freerun transitions
    PMCMonitor-->>HoldoverTest: AnnounceData (ClockClass 135, 165, 248)
    ClockClassSeq->>ClockClassSeq: assert ordered transitions 6→135→165→248
  end
  HoldoverTest->>UpstreamIface: re-enable upstream interfaces
  loop Re-lock wait
    PMCMonitor-->>HoldoverTest: AnnounceData (ClockClass=6)
    ClockClassSeq->>ClockClassSeq: assert re-lock (248→6)
  end
  HoldoverTest->>ApplyE810PluginMutation: cleanup (restore original PtpConfig)

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.94% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title "T-BC tests" is vague and generic, using a non-descriptive acronym without conveying the scope or primary purpose of the comprehensive changes across multiple files.	Improve the title to be more descriptive and specific, such as "Add Telco Boundary Clock test mode and PMC monitoring" to better reflect the main changes.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 7

🤖 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 `@test/conformance/serial/ptp.go`:
- Around line 2877-2878: The calls to QualifyProfileName for the TBC profile
names are incorrectly prefixed with leading underscores. Since
QualifyProfileName already appends an underscore after the configuration name
(crName + "_"), passing "_tbc-tt" and "_tbc-tr" creates double underscores in
the qualified names. Remove the leading underscores from both arguments to
QualifyProfileName at lines 2877-2878, changing them to "tbc-tt" and "tbc-tr"
respectively, to match the actual profile names and align with the correct usage
patterns found elsewhere in the code.
- Around line 547-557: The regex pattern containing the alternation `(r|t)` for
matching both `tbc-tr` and `tbc-tt` is being passed with `isLiteralText=true` to
the GetPodLogsRegex function call within the TelcoBoundaryClock conditional
block. This causes the pattern to be escaped by regexp.QuoteMeta, converting the
regex operators to literal characters and preventing the alternation from
working. Change the `isLiteralText` parameter from `true` to `false` in the
GetPodLogsRegex call for the TBC profile pattern so the regex alternation is
properly interpreted.

In `@test/pkg/ptphelper/ptphelper.go`:
- Around line 400-407: The mutateE810PluginSettings function searches for a
profile by name in a loop and uses the index variable to track its position, but
if no matching profile is found, index remains at its initialized value of 0,
causing the function to silently mutate the first profile instead of the
intended one. After the loop that searches through config.Spec.Profile, add a
check to verify whether a matching profile was actually found; if no profile
matches the profileName, either return early from the function or log a warning
to prevent unintended mutation of the wrong profile.
- Around line 517-537: The function GetConfigForProfileFromVarRun is prepending
`#profile:\s*` to the profileRegex parameter when compiling the regex pattern,
but callers like GetConfigForProfile and GetProfileLogID are already including
`#profile:\s*` in their arguments, creating a malformed double prefix pattern.
Remove the `#profile:\s*` prefix from the fmt.Sprintf call inside
GetConfigForProfileFromVarRun so it only uses the profileRegex as-is, then
update the callers GetConfigForProfile (line 505) and GetProfileLogID (line 116)
to pass the complete pattern including `#profile:\s*` when invoking
GetConfigForProfileFromVarRun.

In `@test/pkg/testconfig/testconfig_test.go`:
- Around line 306-318: The TelcoBoundaryClock test case currently builds a
single profile with a base config name, but downstream Telco logic expects two
qualified dual profiles with suffixes _tbc-tr and _tbc-tt. Refactor the test
setup in the TelcoBoundaryClock case to create two separate profile
configurations instead of one, where each profile is named with the appropriate
qualified suffix and configured with the relevant settings using the existing
helper functions like generateTelcoBCConfig, generateTs2phcConfig, and
mockTelcoBCPlugins to ensure the mock properly models the expected Telco
contract.

In `@test/pkg/testconfig/testconfig.go`:
- Line 1009: The `plugins` variable is initialized with an empty map at line
1009 but is immediately overwritten with another assignment, making the initial
initialization ineffectual and triggering linter warnings. Remove the line that
initializes `plugins := make(map[string]*apiextensions.JSON)` at line 1009.
Apply the same fix at line 1183 where an identical ineffectual initialization of
`plugins` occurs.
- Around line 2194-2216: The T-BC detection logic sets
GlobalConfig.PtpModeDiscovered to TelcoBoundaryClock but does not prevent
subsequent generic detection logic in the function from overwriting this value.
Add a guard condition or return statement after the successful T-BC detection
block (where GlobalConfig.PtpModeDiscovered is set to TelcoBoundaryClock) to
prevent the code from continuing to execute other mode classification logic that
could overwrite the detected mode. This ensures T-BC detection is not
order-dependent and the discovered mode is not inadvertently overwritten by
later detection attempts.

🪄 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: Pro

Run ID: ba98a557-70fb-415c-9300-25d42363aff5

📥 Commits

Reviewing files that changed from the base of the PR and between 21a01d5 and 184453a.

📒 Files selected for processing (12)

• Makefile
• test/README.md
• test/conformance/serial/ptp.go
• test/pkg/clean/clean.go
• test/pkg/consts.go
• test/pkg/logging/collector.go
• test/pkg/metrics/metrics.go
• test/pkg/pmc/pmc.go
• test/pkg/ptphelper/ptphelper.go
• test/pkg/ptptesthelper/ptptesthelper.go
• test/pkg/testconfig/testconfig.go
• test/pkg/testconfig/testconfig_test.go

[sebsoto]

/lgtm

He's re-reviewed since