SPLAT-2745: Changed vSphere cloud config to be read from openshift-config-managed#1495
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-2745 which is a valid jira issue. DetailsIn response to this:
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. |
WalkthroughThe vSphere actuator now reads OpenShift cloud config from a fixed managed ConfigMap ( ChangesCloud Config Source Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/vsphere/util.go (1)
55-63: ⚡ Quick winWrap ConfigMap read failures with object context.
Returning the raw
c.Geterror here loses which ConfigMap/namespace was attempted, which slows incident triage.Proposed patch
if err := c.Get(context.Background(), cmName, cm); err != nil { - return nil, err + return nil, fmt.Errorf("failed to get ConfigMap %s/%s: %w", cmName.Namespace, cmName.Name, 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/controller/vsphere/util.go` around lines 55 - 63, The ConfigMap read error from c.Get loses object context; update the error return in the block that calls c.Get(context.Background(), cmName, cm) to wrap the original error with the ConfigMap name and namespace (use cmName.Name or OpenshiftConfigManagedConfigMap and cmName.Namespace/configNamespace) so callers see "failed to get ConfigMap <name> in namespace <ns>: <err>" — modify the code around cm, cmName and the c.Get call to return a wrapped error instead of the raw err.
🤖 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/controller/vsphere/util.go`:
- Around line 55-63: The ConfigMap read error from c.Get loses object context;
update the error return in the block that calls c.Get(context.Background(),
cmName, cm) to wrap the original error with the ConfigMap name and namespace
(use cmName.Name or OpenshiftConfigManagedConfigMap and
cmName.Namespace/configNamespace) so callers see "failed to get ConfigMap <name>
in namespace <ns>: <err>" — modify the code around cm, cmName and the c.Get call
to return a wrapped error instead of the raw err.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c2249583-be08-4c68-95da-74766ccc810f
📒 Files selected for processing (2)
cmd/vsphere/main.gopkg/controller/vsphere/util.go
|
/retest |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controller/vsphere/util_test.go (1)
32-46: ⚡ Quick winAdd coverage for the new fallback and error paths.
This only exercises the happy path. The behavior change here is also in the empty-namespace fallback and the missing
cloud.conferror, so a regression there would currently pass unnoticed.🤖 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/controller/vsphere/util_test.go` around lines 32 - 46, TestGetVSphereConfig currently only verifies the happy path; add unit tests to cover the new fallback and error paths by creating additional test cases that (1) simulate the empty-namespace fallback by invoking getVSphereConfig with an empty namespace (or a configmap in a different namespace) and asserting it returns the expected fallback behavior, and (2) simulate a missing cloud.conf by building a fake client without OpenshiftConfigManagedCloudConfigKey (or with it empty) and asserting getVSphereConfig returns the appropriate error; reference the existing TestGetVSphereConfig, getVSphereConfig, OpenshiftConfigManagedConfigMap, OpenshiftConfigManagedCloudConfigKey, and openshiftConfigNamespaceForTest to locate and extend the tests.pkg/controller/vsphere/util.go (1)
61-69: ⚡ Quick winInclude the ConfigMap identity in config-load errors.
After moving to a fixed managed ConfigMap, the raw
Geterror and the missing-key error no longer tell operators which namespace/name was attempted. Wrapping both withcmName.NamespaceandcmName.Namewill make missing RBAC/config/sync issues much easier to diagnose from logs.♻️ Proposed diff
if err := c.Get(context.Background(), cmName, cm); err != nil { - return nil, err + return nil, fmt.Errorf("failed to get vSphere cloud config ConfigMap %s/%s: %w", cmName.Namespace, cmName.Name, err) } cloudConfig, found := cm.Data[OpenshiftConfigManagedCloudConfigKey] if !found { - return nil, fmt.Errorf("cloud-config ConfigMap has no %q key", - OpenshiftConfigManagedCloudConfigKey, - ) + return nil, fmt.Errorf("vSphere cloud config ConfigMap %s/%s has no %q key", + cmName.Namespace, + cmName.Name, + OpenshiftConfigManagedCloudConfigKey, + ) }🤖 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/controller/vsphere/util.go` around lines 61 - 69, Wrap both the c.Get error and the missing-key error with the ConfigMap identity to aid debugging: when c.Get(context.Background(), cmName, cm) returns err, return a wrapped error including cmName.Namespace and cmName.Name (use fmt.Errorf and %w to preserve the original err), and when the key lookup for OpenshiftConfigManagedCloudConfigKey fails, include cmName.Namespace and cmName.Name in the returned fmt.Errorf message so operators can see which ConfigMap was read (reference c.Get, cmName, cm, and OpenshiftConfigManagedCloudConfigKey).
🤖 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/controller/vsphere/util_test.go`:
- Around line 32-46: TestGetVSphereConfig currently only verifies the happy
path; add unit tests to cover the new fallback and error paths by creating
additional test cases that (1) simulate the empty-namespace fallback by invoking
getVSphereConfig with an empty namespace (or a configmap in a different
namespace) and asserting it returns the expected fallback behavior, and (2)
simulate a missing cloud.conf by building a fake client without
OpenshiftConfigManagedCloudConfigKey (or with it empty) and asserting
getVSphereConfig returns the appropriate error; reference the existing
TestGetVSphereConfig, getVSphereConfig, OpenshiftConfigManagedConfigMap,
OpenshiftConfigManagedCloudConfigKey, and openshiftConfigNamespaceForTest to
locate and extend the tests.
In `@pkg/controller/vsphere/util.go`:
- Around line 61-69: Wrap both the c.Get error and the missing-key error with
the ConfigMap identity to aid debugging: when c.Get(context.Background(),
cmName, cm) returns err, return a wrapped error including cmName.Namespace and
cmName.Name (use fmt.Errorf and %w to preserve the original err), and when the
key lookup for OpenshiftConfigManagedCloudConfigKey fails, include
cmName.Namespace and cmName.Name in the returned fmt.Errorf message so operators
can see which ConfigMap was read (reference c.Get, cmName, cm, and
OpenshiftConfigManagedCloudConfigKey).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c08c6b67-e586-4d52-aeaf-d8e28bd08b74
📒 Files selected for processing (6)
cmd/vsphere/main.gopkg/controller/vsphere/actuator_test.gopkg/controller/vsphere/machine_scope_test.gopkg/controller/vsphere/reconciler_test.gopkg/controller/vsphere/util.gopkg/controller/vsphere/util_test.go
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
/approve
/lgtm
Let's run some testing
|
/test ? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/test e2e-vsphere-operator |
|
/retest |
|
@vr4manta: all tests passed! 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. |
|
/verified by @vr4manta |
openshift-ci-robot
added
the
verified
|
@vr4manta: This PR has been marked as verified by DetailsIn response to this:
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. |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
3 participants
SPLAT-2745
Goal
Update the Machine API Operator (MAO) to use the new version of cloud config located in the openshift-config-managed namespace, and ensure the operator picks up the latest changes after any updates to the cluster Infrastructure spec.
Background / Problem
MAO currently consumes a cloud-config source that is being superseded by a new version published in openshift-config-managed. In addition, changes to Infrastructure.spec should trigger MAO to refresh/reconcile its effective cloud-config so it stays aligned with cluster-level infra settings.
Changes
Summary by CodeRabbit