Conversation
cyclictest measures real-time scheduler wake-up latency, so its pass/fail verdict is only meaningful on a PREEMPT_RT kernel, where the worst-case latency is bounded and repeatable. On a non-RT kernel the worst-case latency is unbounded and non-deterministic, so no threshold is both correct and stable and raising it does not help. The result is a flaky, meaningless pass/fail. This is the case on aarch64 today, which has no RT kernel available. Add IsRealTimeKernelEnabled, a profile-package helper that reads the setting directly from the PerformanceProfile spec, and use it to skip cyclictest when the profile does not enable the real-time kernel. The check runs before the pod is created. oslat and hwlatdetect are unaffected: they measure hardware and firmware jitter, which is bounded regardless of the kernel. Signed-off-by: Tomer Avital <[email protected]>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughThe change adds a helper that reports whether a performance profile enables PREEMPT_RT. The cyclictest e2e setup skips execution when PREEMPT_RT is disabled or unspecified. Unit tests cover the helper’s supported configurations. ChangesReal-time kernel gating
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change gates cyclictest to real-time kernel profiles while leaving other latency tests unaffected, with no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
Full details: Test Structure And QualityExplanation The four new Ginkgo Resolution Add a diagnostic message to each new boolean assertion. For example:
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/RHsyseng/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/docker/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-logr/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/google/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/jaypipes/[email protected]: i ... [truncated 19684 characters] ... 8s.io/kubectl: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/kubelet: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tgithub.com/onsi/ginkgo/v2: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tavital The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest-required |
|
okd-scos-images fails with "nevra" issues, see #1633 |
|
/cc @MarSik @mrniranjan |
|
PR closed because customers in fact use non-rt with cyclictest (they should raise the max latency manually for cyclictest if reached). |
|
PR closed because customers in fact use non-rt with cyclictest (they should raise the max latency manually for cyclictest if reached). |
|
@tavital: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
3 participants
cyclictest measures real-time scheduler wake-up latency, so its pass/fail verdict is only meaningful on a PREEMPT_RT kernel, where the worst-case latency is bounded and repeatable.
On a non-RT kernel the worst-case latency is unbounded and non-deterministic, so no threshold is both correct and stable and raising it does not help. The result is a flaky, meaningless pass/fail. This is the case on aarch64 today, which has no RT kernel available.
Add IsRealTimeKernelEnabled, a profile-package helper that reads the setting directly from the PerformanceProfile spec, and use it to skip cyclictest when the profile does not enable the real-time kernel. The check runs before the pod is created. oslat and hwlatdetect are unaffected: they measure hardware and firmware jitter, which is bounded regardless of the kernel.
Summary by CodeRabbit
Bug Fixes
Tests