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 2 commits 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 Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a one-shot Kubernetes Job that labels eligible vSphere nodes. The change adds synchronization logic and tests, builds and packages the Job binary, and deploys it with feature-gate and security settings. ChangesvSphere node label synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant node-label-sync
participant FeatureGateAccessor
participant KubernetesAPI
participant SyncVSphereNodeLabels
node-label-sync->>FeatureGateAccessor: Observe feature-gate state
FeatureGateAccessor->>KubernetesAPI: Read OpenShift configuration
node-label-sync->>SyncVSphereNodeLabels: Start one-time synchronization
SyncVSphereNodeLabels->>KubernetesAPI: List and patch eligible Nodes
SyncVSphereNodeLabels-->>node-label-sync: Return synchronization result
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (12 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. |
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
|
/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 `@manifests/0000_90_cloud-controller-manager-operator_00_job.yaml`:
- Around line 33-44: Add topology-aware node placement to the Job spec near
hostNetwork, ensuring it runs only on nodes that provide
/etc/kubernetes/apiserver-url.env while supporting both single-node
control-plane deployments and hosted control planes. Mirror the operator
Deployment’s established nodeSelector and toleration symbols/configuration
rather than allowing scheduling onto arbitrary workers.
🪄 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: c18c9679-f217-4aa9-80c5-8b60f38d7a5f
📒 Files selected for processing (7)
DockerfileMakefilecmd/vsphere-node-label-sync-job/main.gomanifests/0000_90_cloud-controller-manager-operator_00_job.yamlpkg/cloud/vsphere/vsphere.gopkg/controllers/vsphere_node_label_sync.gopkg/controllers/vsphere_node_label_sync_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- Makefile
- Dockerfile
- pkg/cloud/vsphere/vsphere.go
- pkg/controllers/vsphere_node_label_sync.go
- pkg/controllers/vsphere_node_label_sync_test.go
- cmd/vsphere-node-label-sync-job/main.go
| spec: | ||
| priorityClassName: system-node-critical | ||
| serviceAccountName: cluster-cloud-controller-manager | ||
| restartPolicy: OnFailure | ||
| # This Job may run before pod networking (CNI/OVN service routing) is functional on a given | ||
| # node -- e.g. right after a control-plane node reboots during an upgrade. hostNetwork plus | ||
| # sourcing /etc/kubernetes/apiserver-url.env (mirroring the operator Deployment in | ||
| # 0000_26_cloud-controller-manager-operator_50_deployment.yaml) lets the container reach the | ||
| # API server directly instead of through the "kubernetes" Service ClusterIP, which requires | ||
| # kube-proxy/OVN to already be wired up. That file is only populated on control-plane nodes, | ||
| # hence the master nodeSelector/tolerations. | ||
| hostNetwork: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
job='manifests/0000_90_cloud-controller-manager-operator_00_job.yaml'
deployment='manifests/0000_26_cloud-controller-manager-operator_50_deployment.yaml'
echo '== Job placement and host-access configuration =='
rg -n -C 8 'nodeSelector|affinity|tolerations|hostNetwork|host-etc-kube|apiserver-url\.env' "$job"
echo
echo '== Operator Deployment placement and host-access configuration =='
rg -n -C 8 'nodeSelector|affinity|tolerations|hostNetwork|host-etc-kube|apiserver-url\.env' "$deployment"Repository: openshift/cluster-cloud-controller-manager-operator
Length of output: 6101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
job='manifests/0000_90_cloud-controller-manager-operator_00_job.yaml'
deployment='manifests/0000_26_cloud-controller-manager-operator_50_deployment.yaml'
echo '== Job manifest =='
cat -n "$job"
echo
echo '== References to URL_ONLY_KUBECONFIG and node-label-sync =='
rg -n -C 5 'URL_ONLY_KUBECONFIG|vsphere-node-label-sync-job|node-label-sync' .Repository: openshift/cluster-cloud-controller-manager-operator
Length of output: 19651
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Job client configuration and failure handling =='
rg -n -C 8 'URL_ONLY_KUBECONFIG|rest\.InClusterConfig|InClusterConfig|kubeconfig|NewConfig|BuildConfig|config' \
cmd pkg vendor 2>/dev/null | head -n 240
echo
echo '== Related historical Job manifest placement =='
fd -i '*node*label*sync*.yaml' manifests -x sh -c 'echo "FILE: $1"; rg -n -C 5 "nodeSelector|tolerations|hostNetwork|URL_ONLY_KUBECONFIG|apiserver-url" "$1"' sh {}Repository: openshift/cluster-cloud-controller-manager-operator
Length of output: 20822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Related vSphere label-sync Job manifests =='
fd -i -g '*node*label*sync*.yaml' manifests --exec sh -c '
for f do
echo "FILE: $f"
rg -n -C 6 "nodeSelector|tolerations|hostNetwork|URL_ONLY_KUBECONFIG|apiserver-url" "$f" || true
done
' sh
echo
echo '== vSphere label-sync client setup =='
cat -n cmd/vsphere-node-label-sync-job/main.go | sed -n '55,180p'Repository: openshift/cluster-cloud-controller-manager-operator
Length of output: 3612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
old='manifests/0000_26_cloud-controller-manager-operator_45_job-vsphere-node-label-sync.yaml'
if [ -f "$old" ]; then
echo "== Existing vSphere node-label-sync Job =="
cat -n "$old"
else
echo "Missing: $old"
fd -i -g '*job*vSphere*' manifests
fiRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 286
Constrain the Job to nodes that provide /etc/kubernetes/apiserver-url.env.
The Job has no placement constraints, unlike the operator Deployment. A worker uses the /etc/kubernetes/kubeconfig fallback, which does not guarantee API access before service routing is ready. Apply topology-aware placement that supports SNO and hosted control planes.
🧰 Tools
🪛 Trivy (0.72.0)
[error] 33-93: Default security context configured
job node-label-sync in openshift-cloud-controller-manager-operator namespace is using the default security context, which allows root privileges
Rule: KSV-0118
(IaC/Kubernetes)
🤖 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 `@manifests/0000_90_cloud-controller-manager-operator_00_job.yaml` around lines
33 - 44, Add topology-aware node placement to the Job spec near hostNetwork,
ensuring it runs only on nodes that provide /etc/kubernetes/apiserver-url.env
while supporting both single-node control-plane deployments and hosted control
planes. Mirror the operator Deployment’s established nodeSelector and toleration
symbols/configuration rather than allowing scheduling onto arbitrary workers.
Source: Coding guidelines
|
/test e2e-vsphere-ovn |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-vsphere-ovn-upgrade |
|
@vr4manta: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a6111700-8d1a-11f1-9607-9d87cc977858-0 |
|
/test ? |
|
@vr4manta: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
|
@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
Bug Fixes
Tests