UPSTREAM: <carry>: Fix nil pointer dereference in VirtualMachine methods

[RadekManak]

Summary

• Fix panic in EnsureHostInPool when getVmssVM returns ErrorNotVmssInstance: the code fell through to vm.GetInstanceViewStatus() with vm == nil, causing a nil pointer dereference
• Add nil receiver guards to IsVirtualMachine(), IsVirtualMachineScaleSetVM(), and ManagedByVMSS() as defense in depth
• Add unit tests for nil receiver behavior

Root Cause

In EnsureHostInPool (azure_vmss.go:1038-1042), when getVmssVM returned ErrorNotVmssInstance, the negated condition !errors.Is(err, ErrorNotVmssInstance) evaluated to false, so the function did not return — it fell through to vm.GetInstanceViewStatus() with vm == nil, panicking at IsVirtualMachine() on the nil receiver.

Fix

• Call site fix: Changed EnsureHostInPool to skip the node (return nil error) when ErrorNotVmssInstance is encountered, matching the pattern already used in GetInstanceIDByNodeName
• Defensive fix: Added vm != nil guards to IsVirtualMachine(), IsVirtualMachineScaleSetVM(), and ManagedByVMSS() so a nil receiver returns false instead of panicking

Test plan

• Added TestNilVirtualMachine unit test covering all nil receiver methods
• Existing TestEnsureHostInPool passes
• Full pkg/provider/... test suite passes (no failures)

Fixes: https://issues.redhat.com/browse/OCPBUGS-62712

Summary by CodeRabbit

• Bug Fixes

  • Improved resilience of virtual machine identification to avoid crashes when VM data is absent.
  • Refined handling of scale-set VM lookups to return empty results and log appropriately when a VM is not from a scale set, improving reliability.

• Tests

  • Added unit tests validating nil-instance behavior and provisioning-state handling.

[coderabbitai]

Walkthrough

Added nil-receiver guards to two VirtualMachine methods to avoid nil dereference and adjusted VMSS error handling to explicitly handle ErrorNotVmssInstance. A unit test verifying nil VirtualMachine behavior was added.

Changes

Nil Guard Defensive Improvements

Layer / File(s)	Summary
Core Implementation pkg/provider/virtualmachine/virtualmachine.go	IsVirtualMachine() and IsVirtualMachineScaleSetVM() now check if vm == nil { return false } before performing existing Variant comparisons.
Tests pkg/provider/virtualmachine/virtualmachine_test.go	Adds TestNilVirtualMachine asserting nil VirtualMachine returns false for IsVirtualMachine, IsVirtualMachineScaleSetVM, ManagedByVMSS; GetInstanceViewStatus returns nil; GetProvisioningState equals ProvisioningStateUnknown.

VMSS Error Handling Adjustment

Layer / File(s)	Summary
Core Logic pkg/provider/azure_vmss.go	In EnsureHostInPool, treat ErrorNotVmssInstance as a non-fatal condition (log and return empty results) and propagate any other errors instead of the prior inverted handling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 11 | ❌ 1

❌ 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%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title directly addresses the main change: adding nil pointer guards to VirtualMachine methods to prevent nil dereference, which is reflected in all three files modified.
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	PR contains only standard Go test using TestNilVirtualMachine name. No Ginkgo tests found. Test name is static, descriptive, and contains no dynamic information.
Test Structure And Quality	✅ Passed	The test uses standard Go testing (testing.T), not Ginkgo. The check requires reviewing Ginkgo test code, making it not applicable to this PR which follows pkg/provider's standard testing pattern.
Microshift Test Compatibility	✅ Passed	No Ginkgo e2e tests were added in this PR. The new test file uses standard Go unit testing (testing package), not Ginkgo patterns. Check is not applicable.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No Ginkgo e2e tests added. The new test file (virtualmachine_test.go) contains only a standard Go unit test (TestNilVirtualMachine with testing.T). No e2e test files were modified.
Topology-Aware Scheduling Compatibility	✅ Passed	PR contains only Go source code changes to fix nil pointer bugs. No deployment manifests, pod scheduling constraints, PodDisruptionBudgets, or controller configurations are modified.
Ote Binary Stdout Contract	✅ Passed	No OTE Binary Stdout Contract violations detected. All changes are to regular instance methods and test functions, not process-level code (main/init/TestMain/suite setup). No unmanaged stdout writes.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The PR adds standard Go unit tests only, not Ginkgo e2e tests. No IPv4 assumptions or external connectivity requirements detected.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[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 nrb 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

[RadekManak]

/hold clanker

[coderabbitai]

🧹 Nitpick comments (1)

pkg/provider/azure_vmss.go (1)

1038-1043: ⚡ Quick win

Avoid error-level logging for the expected ErrorNotVmssInstance path.

At Line 1038, logger.Error(...) runs before the ErrorNotVmssInstance check, so skipped non-VMSS nodes are logged as errors. Reordering avoids false error noise.

Proposed change

 	if err != nil {
 		if errors.Is(err, cloudprovider.InstanceNotFound) {
 			klog.Infof("EnsureHostInPool: skipping node %s because it is not found", vmName)
 			return "", "", "", nil, nil
 		}
-
-		logger.Error(err, "failed to get vmss vm", "vmName", vmName)
 		if errors.Is(err, ErrorNotVmssInstance) {
 			klog.Infof("EnsureHostInPool: skipping node %s because it is not a VMSS instance", vmName)
 			return "", "", "", nil, nil
 		}
+		logger.Error(err, "failed to get vmss vm", "vmName", vmName)
 		return "", "", "", nil, err
 	}

🤖 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 `@pkg/provider/azure_vmss.go` around lines 1038 - 1043, The current code logs
every error with logger.Error before checking for ErrorNotVmssInstance, causing
expected non-VMSS nodes to be emitted as errors; update the EnsureHostInPool
error handling around the vmss get call so that you first check if
errors.Is(err, ErrorNotVmssInstance) and handle that path (klog.Infof and return
nils) without calling logger.Error, and only call logger.Error(err, "failed to
get vmss vm", "vmName", vmName) for unexpected errors; locate the block
referencing ErrorNotVmssInstance and logger.Error in azure_vmss.go and reorder
or gate the log call accordingly.

🤖 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.

Nitpick comments:
In `@pkg/provider/azure_vmss.go`:
- Around line 1038-1043: The current code logs every error with logger.Error
before checking for ErrorNotVmssInstance, causing expected non-VMSS nodes to be
emitted as errors; update the EnsureHostInPool error handling around the vmss
get call so that you first check if errors.Is(err, ErrorNotVmssInstance) and
handle that path (klog.Infof and return nils) without calling logger.Error, and
only call logger.Error(err, "failed to get vmss vm", "vmName", vmName) for
unexpected errors; locate the block referencing ErrorNotVmssInstance and
logger.Error in azure_vmss.go and reorder or gate the log call accordingly.

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: fb21d883-c5c8-4c49-a87c-54491b2c3882

📥 Commits

Reviewing files that changed from the base of the PR and between 57047c4 and bdf1e64.

📒 Files selected for processing (3)

• pkg/provider/azure_vmss.go
• pkg/provider/virtualmachine/virtualmachine.go
• pkg/provider/virtualmachine/virtualmachine_test.go

[openshift-ci]

@RadekManak: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/verify-git-history	bdf1e64	link	false	/test verify-git-history
ci/prow/e2e-azure-ovn	bdf1e64	link	true	/test e2e-azure-ovn
ci/prow/e2e-azurestack-ipi	bdf1e64	link	true	/test e2e-azurestack-ipi

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.