fix(cpu): Early return for s390x in GetCPUVendor to avoid ghw errors by ashokpariya0 · Pull Request #982 · k8snetworkplumbingwg/sriov-network-operator

ashokpariya0 · 2025-12-15T09:42:15Z

On s390x, ghw.CPU() returns empty Processors slice, causing errors and warnings. Add early return based on runtime.GOARCH to skip ghw call and return correct vendor.

This keeps existing workflow intact for x86/arm64 while fixing s390x detection.

Why we need this change??

We use ghw library(https://github.com/jaypipes/ghw) for cpu vendor detection which does not support s390x arch.

sriov-network-operator/pkg/host/internal/cpu/cpu.go

Line 21 in 2f98be3

cpuInfo, err := c.ghwLib.CPU()

and we see below warning/error in generic plugin log

WARNING: expected to find 'processor' key in /proc/cpuinfo attributes
WARNING: expected to find 'processor' key in /proc/cpuinfo attributes

WARNING: failed to find attributes for logical processor 0
WARNING: failed to find attributes for logical processor 1
WARNING: failed to find attributes for logical processor 10

wrong CPU information retrieved: cpu (0 physical packages, 0 cores, 0 hardware threads)

This is because it expect vendor_id in /proc/cpuinfo which is not available in s390x case like below

cpu number      : 0
physical id     : 2
core id         : 0
book id         : 2
drawer id       : 2
dedicated       : 0
address         : 0
siblings        : 12
cpu cores       : 6
version         : 00
identification  : 271F08
machine         : 9175
cpu MHz dynamic : 5508
cpu MHz static  : 5508

On s390x, the format is different (global vendor_id, # processors, then per-processor blocks with cpu number, book id, drawer id)

# cat /proc/cpuinfo 
vendor_id       : IBM/S390
# processors    : 38
bogomips per cpu: 28901.00
max thread id   : 1

cpu number      : 0
physical id     : 2
core id         : 0
book id         : 2
drawer id       : 2
dedicated       : 0
address         : 0
siblings        : 12
cpu cores       : 6
version         : 00
identification  : 271F08
machine         : 9175
cpu MHz dynamic : 5508
cpu MHz static  : 5508

cpu number      : 1

ghw fails to parse processors and returns empty Processors slice

Summary by CodeRabbit

Bug Fixes
- Improved CPU vendor detection for s390x architecture systems.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

On s390x, ghw.CPU() returns empty Processors slice, causing errors and warnings. Add early return based on runtime.GOARCH to skip ghw call and return correct vendor. This keeps existing workflow intact for x86/arm64 while fixing s390x detection Signed-off-by: Ashok Pariya <ashok.pariya@ibm.com>

github-actions · 2025-12-15T09:42:24Z

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

coderabbitai · 2025-12-15T09:42:27Z

Walkthrough

The change modifies CPU vendor detection for s390x architecture by adding an early return based on GOARCH instead of deriving it from ghw CPU data. A runtime import was added and the "IBM/S390" case mapping was removed.

Changes

Cohort / File(s)	Summary
s390x CPU vendor detection refactoring `pkg/host/internal/cpu/cpu.go`	Added `runtime` import. Modified `GetCPUVendor` to early-return `CPUVendorS390X` when `GOARCH` is "s390x", replacing reliance on ghw CPU data. Removed case branch mapping "IBM/S390" to `CPUVendorS390X`. Comment restructuring around s390x handling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Verify the runtime import is correctly used and necessary
Confirm the early return logic for s390x is correct and doesn't introduce regressions
Ensure removal of the "IBM/S390" case doesn't break existing behavior for other architectures
Check that non-s390x code paths remain unaffected

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: an early return for s390x architecture in GetCPUVendor to avoid ghw library errors.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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

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

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

pkg/host/internal/cpu/cpu.go (1)

21-27: s390x early return cleanly avoids ghw issues

The runtime.GOARCH == "s390x" guard correctly short-circuits ghw on s390x and returns types.CPUVendorS390X, matching the PR’s intent and avoiding empty-processor errors while leaving other architectures unchanged. As the codebase accumulates more s390x-specific behavior, you might optionally consider a //go:build s390x-specific implementation file instead of a runtime check, but this is not required for correctness here.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f98be3 and 5c245c6.

📒 Files selected for processing (1)

pkg/host/internal/cpu/cpu.go (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/host/internal/cpu/cpu.go (1)

pkg/host/types/interfaces.go (1)

CPUVendorS390X (208-208)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Golangci-lint
GitHub Check: test
GitHub Check: test-coverage
GitHub Check: Analyze (go)
GitHub Check: build

🔇 Additional comments (1)

pkg/host/internal/cpu/cpu.go (1)

3-9: Runtime import usage is correct

The new runtime import is required for the GOARCH check and is properly used, so there’s no unused-import risk.

SchSeba

Hi @ashokpariya0 thanks for the contribution, can I ask if you can add support to the GHW lib?

I think that will be a much better solution.

If that will not be possible (rejected by the package owner) I am fine merging this one

ashokpariya0 · 2026-02-06T09:13:34Z

Hi @ashokpariya0 thanks for the contribution, can I ask if you can add support to the GHW lib?

I think that will be a much better solution.

If that will not be possible (rejected by the package owner) I am fine merging this one

I analyzed the changes needed in the GHW library to add proper CPU info detection(not just vendor_id), and it looks like this will take some time, and we would prefer to handle it in a follow-up PR.

For the current use case, we only need the vendor_id, which is fixed for s390 (IBM/S390).
We are not using any other fields from /proc/cpuinfo.

We can add something similar to what NFD does (for example:
https://github.com/kubernetes-sigs/node-feature-discovery/blob/master/source/cpu/cpu.go#L293-L328
), but that would introduce additional and unnecessary parsing complexity for this specific case.

so, i think the existing changes are sufficient for the current case and provide a minimal fix.
We can work on adding full entire CPU detection support in the GHW library in a follow-up PR on- https://github.com/jaypipes/ghw

ashokpariya0 · 2026-02-12T04:59:34Z

@SchSeba We have added changes in ghw, we can merge pr-#1029 and close this pr.

coderabbitai bot reviewed Dec 15, 2025

View reviewed changes

SchSeba reviewed Dec 24, 2025

View reviewed changes

ashokpariya0 mentioned this pull request Feb 6, 2026

Guidance needed for validating SR-IOV Operator on single-node Kubernetes cluster (Mellanox ConnectX-6 Lx) #968

Open

ashokpariya0 mentioned this pull request Feb 19, 2026

build(deps): bump the general-dependencies group across 1 directory with 2 updates #1037

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

fix(cpu): Early return for s390x in GetCPUVendor to avoid ghw errors#982

fix(cpu): Early return for s390x in GetCPUVendor to avoid ghw errors#982
ashokpariya0 wants to merge 1 commit intok8snetworkplumbingwg:masterfrom
ashokpariya0:fix-cpu-vendor-s390x

ashokpariya0 commented Dec 15, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

github-actions bot commented Dec 15, 2025

Uh oh!

coderabbitai bot commented Dec 15, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

SchSeba left a comment

Uh oh!

ashokpariya0 commented Feb 6, 2026

Uh oh!

ashokpariya0 commented Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

ashokpariya0 commented Dec 15, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why we need this change??

Summary by CodeRabbit

Uh oh!

github-actions bot commented Dec 15, 2025

Uh oh!

coderabbitai bot commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

SchSeba left a comment

Choose a reason for hiding this comment

Uh oh!

ashokpariya0 commented Feb 6, 2026

Uh oh!

ashokpariya0 commented Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ashokpariya0 commented Dec 15, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 15, 2025 •

edited

Loading