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 5 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:
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 |
|
@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
|
/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 ? |
|
Installs are now working; however, both ovn test runs hit the following: This was recommended by coderabbitai. Need to see what we should do for this. |
|
/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/83abd4a0-8f2e-11f1-8291-6a9e64f42511-0 |
|
All install and upgrade jobs have passed. The payload-job test for upgrade is also passing which is what reported the initial bug. |
|
/assign @damdo |
18b34d8 to
a4d895b
Compare
|
/test e2e-vsphere-ovn |
a4d895b to
ee4624a
Compare
|
/test e2e-vsphere-ovn |
b5cf1aa to
765ed18
Compare
|
/retest |
|
/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/b026cd10-90d6-11f1-815d-b29a0b9a8058-0 |
|
/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. |
|
/assign @damdo @theobarberbany |
| currentFeatureGates, err := featureGateAccess.CurrentFeatureGates() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get current feature gates: %w", err) | ||
| } | ||
| if !currentFeatureGates.Enabled(features.FeatureGateVSphereMixedNodeEnv) { | ||
| klog.V(2).Infof("%s feature gate is disabled, skipping node label sync", features.FeatureGateVSphereMixedNodeEnv) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Optimization nit, we could move this above the infra get to avoid doing the get in the first place if the featureGateAccess.CurrentFeatureGates() (checking the feature gate first is cheaper then getting the infra (in-memory lookup))
There was a problem hiding this comment.
Ok, I can make that change.
| - key: node.cloudprovider.kubernetes.io/uninitialized | ||
| effect: NoSchedule | ||
| value: "true" |
There was a problem hiding this comment.
Is this a duplicate of the one at line 62?
There was a problem hiding this comment.
IMO no. This is the same as what you'll find in the deployment yaml in this same manifest directory. These were added based on recommendations from CodeRabbitAI above.
There was a problem hiding this comment.
Made some adjustments. Hope this is better.
| func init() { | ||
| utilruntime.Must(clientgoscheme.AddToScheme(scheme)) | ||
| utilruntime.Must(configv1.AddToScheme(scheme)) | ||
| } |
There was a problem hiding this comment.
We are not registering these for the fake client in the tests
There was a problem hiding this comment.
We could do this in a TestMain or at the top of the test, matching what the real main.go does in its init()
| # CNI relies on CCM to fill in IP information on Node objects. | ||
| # Therefore we must schedule before the CNI can mark the Node as ready. |
There was a problem hiding this comment.
Indentation of the comment is wrong here if it refers to the key below
| configInformers := configinformers.NewSharedInformerFactory(configClient, 10*time.Minute) | ||
| controllerRef, err := events.GetControllerReferenceForCurrentPod(ctx, kubeClient, managedNamespace, nil) | ||
| if err != nil { | ||
| klog.Warningf("unable to get owner reference (falling back to namespace): %v", err) | ||
| } | ||
|
|
||
| featureGateAccessor := featuregates.NewFeatureGateAccess( | ||
| controllers.GetReleaseVersion(), missingVersion, | ||
| configInformers.Config().V1().ClusterVersions(), configInformers.Config().V1().FeatureGates(), | ||
| events.NewKubeRecorder(kubeClient.CoreV1().Events(managedNamespace), recorderName, controllerRef, clock.RealClock{}), | ||
| ) | ||
| featureGateAccessor.SetChangeHandler(func(featuregates.FeatureChange) {}) | ||
| go featureGateAccessor.Run(ctx) | ||
| go configInformers.Start(ctx.Done()) | ||
|
|
||
| select { | ||
| case <-featureGateAccessor.InitialFeatureGatesObserved(): | ||
| case <-ctx.Done(): | ||
| setupLog.Error(ctx.Err(), "context canceled while waiting for FeatureGate detection") | ||
| os.Exit(1) | ||
| case <-time.After(featureGateWaitTimeout): | ||
| setupLog.Error(errors.New("timed out waiting for FeatureGate detection"), "unable to determine feature gate state") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| if err := controllers.SyncVSphereNodeLabels(ctx, k8sClient, featureGateAccessor); err != nil { | ||
| setupLog.Error(err, "failed to sync vSphere node labels") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| setupLog.Info("vSphere node label sync completed successfully") | ||
| } |
There was a problem hiding this comment.
The job creates three K8s clients and a full informer framework to check one feature gate before doing a straightforward list+patch and for a run-once Job that feels a bit heavy.
So I looked at whether we could replace the informer with a Get() on the FeatureGate resource + NewHardcodedFeatureGateAccessFromFeatureGate, but that won't work cleanly as NewFeatureGateAccess has version logic.
That said, you could still simplify things by moving the feature gate check into main.go.
Do the informer dance there, resolve the gate, then pass a plain bool (or just don't call SyncVSphereNodeLabels at all if the gate is off). That way the function signature drops from (ctx, client, featureGateAccess) to just (ctx, client)
There was a problem hiding this comment.
ok, i'll look into this.
| # 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.
Could we make it run strictly only after CNI/OVN is up so we drop the hostNet?
There was a problem hiding this comment.
Well not that it is run much later in the deployment, I can look into adjust this.
- Move feature gate check from SyncVSphereNodeLabels into main.go, simplifying the function signature to (ctx, client) - Remove duplicate node.cloudprovider.kubernetes.io/uninitialized toleration from job manifest - Fix comment indentation for CNI toleration in job manifest - Register configv1 scheme in test init() to match main.go's init() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
removed
the
verified
|
@damdo I have pushed changes addressing your feedback. Please review when you have time. |
|
@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. |
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
/approve
/assign @theobarberbany
for LGTM
@vr4manta Feel free to add verified when all the manual and CI payload testing has been done you are happy with. Thanks!
|
[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
|
/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/c7aa8340-94dd-11f1-97b2-a787b0b809e9-0 |
|
/test e2e-vsphere-ovn |
4 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