OCPBUGS-100052: Created new job to update vSphere nodes to have vsphere label - #497
OCPBUGS-100052: Created new job to update vSphere nodes to have vsphere label#497vr4manta wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-important
|
@vr4manta: This pull request references Jira Issue OCPBUGS-100052, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
WalkthroughThe change adds a feature-gated Kubernetes Job that builds and runs a one-time synchronization pass, labeling eligible vSphere nodes. It introduces shared vSphere constants, reconciliation logic and tests, binary packaging, and deployment configuration. ChangesvSphere node label synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Job as vsphere-node-label-sync-job
participant Config as OpenShift config informers
participant Sync as SyncVSphereNodeLabels
participant API as Kubernetes API
Job->>Config: Wait for initial feature-gate state
Job->>Sync: Start synchronization
Sync->>API: Fetch Infrastructure and list Nodes
Sync->>API: Patch eligible vSphere node labels
Sync-->>Job: Return synchronization result
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
@vr4manta: This pull request references Jira Issue OCPBUGS-100052, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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.
Inline comments:
In `@cmd/vsphere-node-label-sync-job/main.go`:
- Line 75: Add an application-level timeout context derived from the context
returned by ctrl.SetupSignalHandler(), using a deadline safely below the
manifest’s 300-second limit, and ensure its cancel function is released. Pass
this context through the job’s initialization and synchronization calls so API
operations observe both termination cancellation and the application timeout;
use context.Context throughout the affected function signatures.
In
`@manifests/0000_26_cloud-controller-manager-operator_45_job-vsphere-node-label-sync.yaml`:
- Around line 46-47: Remove the hard-coded control-plane nodeSelector from the
Job manifest so scheduling is not restricted to the legacy
node-role.kubernetes.io/master label. Update the deployment scheduling
configuration to remain topology-aware and compatible with HyperShift-style
clusters, without requiring control-plane placement.
- Around line 71-97: Add a restrictive container securityContext to the
vsphere-node-label-sync container, requiring runAsNonRoot, disabling privilege
escalation, enabling a read-only root filesystem, and dropping ALL Linux
capabilities; preserve the existing host-etc-kube read-only mount and add any
other writable mounts or capabilities only if the job explicitly requires them.
- Around line 89-93: Update the container resources block in the Job manifest to
add bounded cpu and memory limits alongside the existing requests. Ensure every
container in this Kubernetes manifest defines both resource limit fields, using
values appropriate for this system-node-critical workload.
- Line 45: Remove the hostNetwork setting from the Job manifest so the
hostaccess workload uses ordinary Kubernetes Service connectivity. Ensure the
manifest retains no hostNetwork configuration while leaving its existing host
Kubernetes configuration mount unchanged.
- Line 26: Remove the ttlSecondsAfterFinished field from the Job manifest so the
completed create-only Job remains present and is not recreated on later CVO
syncs.
In `@pkg/controllers/vsphere_node_label_sync.go`:
- Line 89: Update the logging statement in the node label sync flow to avoid
emitting node.Name at info level. Replace the per-node hostname-containing
message with an aggregate/count message or a redacted identifier, while
preserving the label operation and its existing informational intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e2850ae9-15f2-41e7-bdec-611918f5192d
📒 Files selected for processing (7)
DockerfileMakefilecmd/vsphere-node-label-sync-job/main.gomanifests/0000_26_cloud-controller-manager-operator_45_job-vsphere-node-label-sync.yamlpkg/cloud/vsphere/vsphere.gopkg/controllers/vsphere_node_label_sync.gopkg/controllers/vsphere_node_label_sync_test.go
|
/retest |
|
I noticed the azure job had the following in it: I am thinking the change coderabbit suggested for changing the run definition of the job's container may be causing this. Need to check vsphere log and retest. |
|
/test e2e-vsphere-ovn |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@cmd/vsphere-node-label-sync-job/main.go`:
- Around line 118-123: Add a ctx.Done() case to the feature-gate wait select in
main, returning through the existing graceful cancellation path when the context
is canceled while preserving the observation and timeout behavior. Ensure the
wait uses the existing context established by
ctrl.SetupSignalHandler()/jobTimeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7850ebc2-bcbb-4bf0-af28-d6e65bc74855
📒 Files selected for processing (7)
DockerfileMakefilecmd/vsphere-node-label-sync-job/main.gomanifests/0000_26_cloud-controller-manager-operator_45_job-vsphere-node-label-sync.yamlpkg/cloud/vsphere/vsphere.gopkg/controllers/vsphere_node_label_sync.gopkg/controllers/vsphere_node_label_sync_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pkg/controllers/vsphere_node_label_sync.go`:
- Around line 75-83: Only backfill the platform-type label when it is absent:
update the label check in the sync controller to test map membership and return
for any existing value, including non-vSphere values. In
pkg/controllers/vsphere_node_label_sync.go lines 75-83, preserve existing
labels; in pkg/controllers/vsphere_node_label_sync_test.go lines 37-75, add a
vSphere-providerID Node with a non-vSphere platform label and assert it remains
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3d060cde-f34e-4e30-9950-e7738cfc517a
📒 Files selected for processing (7)
DockerfileMakefilecmd/vsphere-node-label-sync-job/main.gomanifests/0000_26_cloud-controller-manager-operator_45_job-vsphere-node-label-sync.yamlpkg/cloud/vsphere/vsphere.gopkg/controllers/vsphere_node_label_sync.gopkg/controllers/vsphere_node_label_sync_test.go
| if node.Labels[vsphere.NodePlatformTypeLabelKey] == vsphere.NodePlatformTypeLabelValueVSphere { | ||
| return nil | ||
| } | ||
|
|
||
| patch := client.MergeFrom(node.DeepCopy()) | ||
| if node.Labels == nil { | ||
| node.Labels = map[string]string{} | ||
| } | ||
| node.Labels[vsphere.NodePlatformTypeLabelKey] = vsphere.NodePlatformTypeLabelValueVSphere |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only backfill an absent platform-type label.
The sync is documented as repairing missing labels, but it overwrites any existing value other than vsphere. That can replace an established Node scheduling label.
pkg/controllers/vsphere_node_label_sync.go#L75-L83: check map membership and return whennode.openshift.io/platform-typealready exists, regardless of its value.pkg/controllers/vsphere_node_label_sync_test.go#L37-L75: add a vSphere-providerID Node with an existing non-vSphere value and assert that it remains unchanged.
Proposed controller fix
- if node.Labels[vsphere.NodePlatformTypeLabelKey] == vsphere.NodePlatformTypeLabelValueVSphere {
+ if _, hasPlatformTypeLabel := node.Labels[vsphere.NodePlatformTypeLabelKey]; hasPlatformTypeLabel {
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if node.Labels[vsphere.NodePlatformTypeLabelKey] == vsphere.NodePlatformTypeLabelValueVSphere { | |
| return nil | |
| } | |
| patch := client.MergeFrom(node.DeepCopy()) | |
| if node.Labels == nil { | |
| node.Labels = map[string]string{} | |
| } | |
| node.Labels[vsphere.NodePlatformTypeLabelKey] = vsphere.NodePlatformTypeLabelValueVSphere | |
| if node.Labels[vsphere.NodePlatformTypeLabelKey] == vsphere.NodePlatformTypeLabelValueVSphere { | |
| return nil | |
| } | |
| patch := client.MergeFrom(node.DeepCopy()) | |
| if _, hasPlatformTypeLabel := node.Labels[vsphere.NodePlatformTypeLabelKey]; hasPlatformTypeLabel { | |
| return nil | |
| } | |
| node.Labels[vsphere.NodePlatformTypeLabelKey] = vsphere.NodePlatformTypeLabelValueVSphere |
📍 Affects 2 files
pkg/controllers/vsphere_node_label_sync.go#L75-L83(this comment)pkg/controllers/vsphere_node_label_sync_test.go#L37-L75
🤖 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/controllers/vsphere_node_label_sync.go` around lines 75 - 83, Only
backfill the platform-type label when it is absent: update the label check in
the sync controller to test map membership and return for any existing value,
including non-vSphere values. In pkg/controllers/vsphere_node_label_sync.go
lines 75-83, preserve existing labels; in
pkg/controllers/vsphere_node_label_sync_test.go lines 37-75, add a
vSphere-providerID Node with a non-vSphere platform label and assert it remains
unchanged.
|
/test e2e-vsphere-ovn |
|
@vr4manta: The following tests 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. |
2 participants
OCPBUGS-100052
Changes
node.openshift.io/platform-type=vsphereNotes
Idea is to have this only be attempted to run once and only on first upgrade with this change. Following upgrades should not rerun this job.
Summary by CodeRabbit
New Features
vsphere-node-label-sync) to backfill missing vSphere node labels (node.openshift.io/platform-type=vsphere).VSphereMixedNodeEnvis enabled, targets master nodes, and patches only nodes whoseproviderIDindicates vSphere (leaving existing labels unchanged).Tests
providerID, and when the feature gate is disabled.Chores
vsphere-node-label-sync-jobbinary to the build and included it in the runtime image.