OCPBUGS-126898: Don't default vSphere numCoresPerSocket to numCPUs

[nikhilprajapati-world]

Summary

• When providerSpec.numCoresPerSocket is omitted (0), clone no longer sets it equal to numCPUs.
• The clone ConfigSpec leaves NumCoresPerSocket unset so vSphere uses the template analogue, which is what the API documents.
• Unit test covers omit vs explicit cores-per-socket.

Fixes OCPBUGS-126898

Test plan

• go test ./pkg/controller/vsphere/ -count=1 -run TestCoresPerSocketForClone
• Lab on OpenShift 5.0.0-rc.2 vSphere: custom MachineSet numCPUs: 8, omit numCoresPerSocket. Before this change, guest lscpu was 8 CPUs / 8 cores/socket / 1 socket (IPI workers 4/4/1). After this change, omitted field must follow the template (4 cores/socket → 2 sockets for 8 vCPU), not 1×8.

Made with Cursor

Summary by CodeRabbit

• Bug Fixes
  • Improved vSphere virtual machine cloning when the cores-per-socket setting is unset.
  • Clone operations now preserve the template’s compatible CPU topology instead of forcing all vCPUs onto one socket, reducing clone failures on incompatible hosts.

[openshift-merge-bot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

[openshift-ci-robot]

@nikhilprajapati-world: This pull request references Jira Issue OCPBUGS-126898, which is invalid:

• expected the bug to target the "5.1.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Summary

• When providerSpec.numCoresPerSocket is omitted (0), clone no longer sets it equal to numCPUs.
• The clone ConfigSpec leaves NumCoresPerSocket unset so vSphere uses the template analogue, which is what the API documents.
• Unit test covers omit vs explicit cores-per-socket.

Fixes OCPBUGS-126898

Test plan

• go test ./pkg/controller/vsphere/ -count=1 -run TestCoresPerSocketForClone
• Lab on OpenShift 5.0.0-rc.2 vSphere: custom MachineSet numCPUs: 8, omit numCoresPerSocket. Before this change, guest lscpu was 8 CPUs / 8 cores/socket / 1 socket (IPI workers 4/4/1). After this change, omitted field must follow the template (4 cores/socket → 2 sockets for 8 vCPU), not 1×8.

Made with Cursor

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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 1c46287d-7498-498f-8bc9-29b47230f10d

📥 Commits

Reviewing files that changed from the base of the PR and between 26d7767 and 6f8799c.

📒 Files selected for processing (2)

• pkg/controller/vsphere/reconciler.go
• pkg/controller/vsphere/reconciler_test.go

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

Walkthrough

The vSphere clone path no longer defaults zero cores per socket to the CPU count. It leaves the clone field unset for zero and preserves explicit values. New tests verify both cases.

Changes

vSphere clone CPU topology

Layer / File(s)	Summary
Conditional cores-per-socket handling and tests pkg/controller/vsphere/reconciler.go, pkg/controller/vsphere/reconciler_test.go	The clone specification uses coresPerSocketForClone. The helper returns nil for zero and a pointer to explicit values. Tests cover zero, 4, and 8.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: vr4manta

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (14 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: vSphere no longer defaults numCoresPerSocket to numCPUs.
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.
Stable And Deterministic Test Names	✅ Passed	The pull request adds only the static Go test name TestCoresPerSocketForClone. It contains no Ginkgo title and no dynamic value such as a pod name, timestamp, UUID, node, namespace, or IP address. T…
Test Structure And Quality	✅ Passed	PASS. The added test is a standard testing.T unit test, not a Ginkgo It block. It exercises one related behavior: zero returns nil, and explicit values remain unchanged. It creates no cluster reso…
Microshift Test Compatibility	✅ Passed	PASS: The pull request adds only a standard Go unit test, TestCoresPerSocketForClone, in pkg/controller/vsphere/reconciler_test.go. The test uses testing.T and contains no Ginkgo constructs such…
Single Node Openshift (Sno) Test Compatibility	✅ Passed	PASS: The pull request adds no new Ginkgo e2e tests. The only added test is the standard Go test TestCoresPerSocketForClone(t *testing.T) in pkg/controller/vsphere/reconciler_test.go; it checks po…
Topology-Aware Scheduling Compatibility	✅ Passed	PASS: The pull request changes only vSphere VM clone CPU topology. It sets ConfigSpec.NumCoresPerSocket to nil when the provider value is zero and preserves explicit values. The changed files introd…
Ote Binary Stdout Contract	✅ Passed	The pull request changes only vSphere clone configuration and adds a unit test. The new code contains no stdout writes, logging setup, main function, suite setup, or TestMain. The test uses testing.T …
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PASS: The pull request adds only TestCoresPerSocketForClone, a standard Go testing.T unit test. The changed test file contains no new Ginkgo It, Describe, Context, or When test. It uses no…
No-Weak-Crypto	✅ Passed	PASS. The PR changes only vSphere CPU topology handling and adds its unit test. The diff removes the numCPUs fallback and adds coresPerSocketForClone, which returns nil or the configured int32. The ad…
Container-Privileges	✅ Passed	PASS. The authoritative PR diff changes only two Go files: vSphere clone logic and its unit test. The patch contains no container or Kubernetes manifest and no occurrence of privileged, hostPID, hostN…
No-Sensitive-Data-In-Logs	✅ Passed	The pull request adds no logging statements and does not add passwords, tokens, API keys, PII, session IDs, hostnames, or customer data to logs. The changed production code only changes the clone spec…

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

Warning

Some tools did not complete. Review the errors below.

🔧 golangci-lint (2.13.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

---

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

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign radekmanak for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jcpowermac]

@nikhilprajapati-world @coderabbitai I think you are forgetting something - we should have less numa nodes not more, and your tests left out the odd number of cores case, which customers' have used in the past.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ast-grep outline pkg/controller/vsphere/reconciler.go --items all --match 'clone|coresPerSocketForClone' || true
ast-grep outline pkg/controller/vsphere/reconciler_test.go --items all --match 'TestCoresPerSocketForClone|clone' || true

printf '\n--- clone helper and focused test context ---\n'
rg -n -A35 -B20 'coresPerSocketForClone|NumCoresPerSocket|TestCoresPerSocketForClone' \
  pkg/controller/vsphere/reconciler.go pkg/controller/vsphere/reconciler_test.go || true

printf '\n--- topology and validation references ---\n'
rg -n -i -C4 'numCoresPerSocket|NumCoresPerSocket|numa|topology|cores.?per.?socket' \
  --glob '*.go' . || true

Length of output: 27134

---

@jcpowermac You are correct that the test does not cover an odd explicit value.

NumCoresPerSocket controls virtual socket topology. It does not directly define the NUMA-node count. With an omitted value, this change lets vCenter retain the template topology. The lab validation must confirm the guest-visible socket and NUMA-node topology. It must not assume that a higher socket count always means a higher NUMA-node count.

[plan_code_changes]

• Add an odd numCoresPerSocket test case.
• Keep the test focused on the clone ConfigSpec contract.
• Update the lab validation to report guest socket and NUMA-node topology for the template-derived case.

---

📝 Learnings were identified and not saved (knowledge base disabled). Enable

You are interacting with an AI system.

[jcpowermac]

Tip

For best results, initiate chat on the files or code changes.
🧩 Analysis chain
@jcpowermac You are correct that the test does not cover an odd explicit value.

NumCoresPerSocket controls virtual socket topology. It does not directly define the NUMA-node count. With an omitted value, this change lets vCenter retain the template topology. The lab validation must confirm the guest-visible socket and NUMA-node topology. It must not assume that a higher socket count always means a higher NUMA-node count.

[plan_code_changes]

• Add an odd numCoresPerSocket test case.
• Keep the test focused on the clone ConfigSpec contract.
• Update the lab validation to report guest socket and NUMA-node topology for the template-derived case.

📝 Learnings were identified and not saved (knowledge base disabled). Enable

You are interacting with an AI system.

@coderabbitai you are wrong, if you increase the number of sockets that directly increases numa nodes, which incurs a latency cost

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🤖 web_search executed:

vSphere NumCoresPerSocket virtual sockets NUMA nodes topology documentation

Length of output: 11776

---

@jcpowermac You are right. My previous statement was incorrect for the vSphere topology behavior relevant to this PR.

When vSphere derives virtual NUMA topology from coresPerSocket, decreasing NumCoresPerSocket increases the virtual socket count and the virtual NUMA-node count. This can cause remote-memory access and added latency.

The validation must therefore confirm that an omitted providerSpec.numCoresPerSocket preserves the template socket and NUMA topology. It must not only confirm that cloning succeeds. The odd-core test remains useful, but it does not address this topology regression.

---

📝 Learnings were identified and not saved (knowledge base disabled). Enable

You are interacting with an AI system.

[jcpowermac]

/test unit
/test e2e-vsphere-ovn

/lgtm
/hold

I think the change is fine, it gets us back to the contract defined in the API. vsphere CI needs to pass, I would like to see the odd case.

[openshift-merge-bot]

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn

[openshift-ci]

@nikhilprajapati-world: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.