Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ FROM registry.ci.openshift.org/ocp/5.0:base-rhel9
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/bin/cluster-controller-manager-operator .
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/bin/config-sync-controllers .
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/bin/azure-config-credentials-injector .
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/bin/vsphere-node-label-sync-job .
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/manifests manifests
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/openshift-tests/bin/cloud-controller-manager-aws-tests-ext.gz /usr/bin/cloud-controller-manager-aws-tests-ext.gz
COPY --from=builder /go/src/github.com/openshift/cluster-cloud-controller-manager-operator/openshift-tests/bin/cloud-controller-manager-operator-tests-ext.gz /usr/bin/cloud-controller-manager-operator-tests-ext.gz
Expand Down
5 changes: 4 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ unit:

# Build operator binaries
.PHONY: build
build: operator config-sync-controllers azure-config-credentials-injector cloud-controller-manager-aws-tests-ext cluster-cloud-controller-manager-operator-tests-ext
build: operator config-sync-controllers azure-config-credentials-injector vsphere-node-label-sync-job cloud-controller-manager-aws-tests-ext cluster-cloud-controller-manager-operator-tests-ext

operator:
go build -o bin/cluster-controller-manager-operator cmd/cluster-cloud-controller-manager-operator/main.go
Expand All @@ -50,6 +50,9 @@ config-sync-controllers:
azure-config-credentials-injector:
go build -o bin/azure-config-credentials-injector cmd/azure-config-credentials-injector/main.go

vsphere-node-label-sync-job:
go build -o bin/vsphere-node-label-sync-job cmd/vsphere-node-label-sync-job/main.go

cloud-controller-manager-aws-tests-ext:
cd openshift-tests/ccm-aws-tests && \
mkdir -p ../bin && \
Expand Down
134 changes: 134 additions & 0 deletions cmd/vsphere-node-label-sync-job/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
/*
Copyright 2021.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

// Command vsphere-node-label-sync-job runs to completion once, backfilling the
// node.openshift.io/platform-type=vsphere label onto vSphere nodes that are missing it. It is
// installed as a Kubernetes Job by the CVO (see
// manifests/0000_26_cloud-controller-manager-operator_45_job-vsphere-node-label-sync.yaml) rather
// than run as an ongoing in-process controller.
package main

import (
"context"
"errors"
"flag"
"os"
"time"

"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"

// Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.)
// to ensure that exec-entrypoint and run can make use of them.
"k8s.io/client-go/kubernetes"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
_ "k8s.io/client-go/plugin/pkg/client/auth"
"k8s.io/klog/v2"
"k8s.io/utils/clock"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"

configv1 "github.com/openshift/api/config/v1"
configv1client "github.com/openshift/client-go/config/clientset/versioned"
configinformers "github.com/openshift/client-go/config/informers/externalversions"
"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
"github.com/openshift/library-go/pkg/operator/events"

"github.com/openshift/cluster-cloud-controller-manager-operator/pkg/controllers"
)

var (
scheme = runtime.NewScheme()
setupLog = ctrl.Log.WithName("setup")
)

const (
recorderName = "cloud-controller-manager-operator-vsphere-node-label-sync-job"
missingVersion = "0.0.1-snapshot"
managedNamespace = controllers.DefaultManagedNamespace
featureGateWaitTimeout = 1 * time.Minute
// jobTimeout bounds the process's overall runtime safely below the Job manifest's
// activeDeadlineSeconds (300s), so it can log a clear error and exit cleanly instead of
// being killed by the kubelet once the deadline is hit.
jobTimeout = 4 * time.Minute
)

func init() {
utilruntime.Must(clientgoscheme.AddToScheme(scheme))
utilruntime.Must(configv1.AddToScheme(scheme))
}

func main() {
klog.InitFlags(flag.CommandLine)
flag.Parse()

ctrl.SetLogger(klog.NewKlogr().WithName("VSphereNodeLabelSyncJob"))

ctx, cancel := context.WithTimeout(ctrl.SetupSignalHandler(), jobTimeout)
defer cancel()

restConfig := ctrl.GetConfigOrDie()

k8sClient, err := client.New(restConfig, client.Options{Scheme: scheme})
if err != nil {
setupLog.Error(err, "unable to create client")
os.Exit(1)
}

configClient, err := configv1client.NewForConfig(restConfig)
if err != nil {
setupLog.Error(err, "unable to create config client")
os.Exit(1)
}

kubeClient, err := kubernetes.NewForConfig(restConfig)
if err != nil {
setupLog.Error(err, "unable to create kube client")
os.Exit(1)
}

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)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
apiVersion: batch/v1
kind: Job
metadata:
name: vsphere-node-label-sync
namespace: openshift-cloud-controller-manager-operator
annotations:
capability.openshift.io/name: CloudControllerManager
include.release.openshift.io/self-managed-high-availability: "true"
include.release.openshift.io/single-node-developer: "true"
# Only create this Job on clusters where the VSphereMixedNodeEnv feature gate is enabled,
# matching the same gate the vSphere CCM checks before applying the node.openshift.io/platform-type
# label at node-init time (see pkg/cloud/vsphere/vsphere.go). The CVO re-evaluates this against the
# live FeatureGate object on every sync, so the Job is created as soon as the gate turns on.
release.openshift.io/feature-gate: "VSphereMixedNodeEnv"
# This Job must only ever be created once, never updated. Its pod template's container image
# changes on nearly every release, but a Job's pod template is immutable once created, so if
# the CVO ever tried to reconcile this Job in place (instead of only creating it) the update
# would fail on every subsequent upgrade after the first. create-only makes the CVO create it
# once and leave it alone from then on, matching the intended "backfill once" semantics.
release.openshift.io/create-only: "true"
labels:
k8s-app: vsphere-node-label-sync
spec:
backoffLimit: 6
activeDeadlineSeconds: 300
template:
metadata:
labels:
k8s-app: vsphere-node-label-sync
annotations:
target.workload.openshift.io/management: '{"effect": "PreferredDuringScheduling"}'
openshift.io/required-scc: hostaccess
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
Comment thread
coderabbitai[bot] marked this conversation as resolved.
nodeSelector:
node-role.kubernetes.io/master: ""
Comment thread
coderabbitai[bot] marked this conversation as resolved.
tolerations:
- key: node.cloudprovider.kubernetes.io/uninitialized
effect: NoSchedule
value: "true"
- key: "node-role.kubernetes.io/master"
operator: "Exists"
effect: "NoSchedule"
- key: "node.kubernetes.io/unreachable"
operator: "Exists"
effect: "NoExecute"
tolerationSeconds: 120
- key: "node.kubernetes.io/not-ready"
operator: "Exists"
effect: "NoExecute"
tolerationSeconds: 120
- key: "node.cloudprovider.kubernetes.io/uninitialized"
operator: "Exists"
effect: "NoSchedule"
# 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.
- key: "node.kubernetes.io/not-ready"
operator: "Exists"
effect: "NoSchedule"
containers:
- name: vsphere-node-label-sync
image: quay.io/openshift/origin-cluster-cloud-controller-manager-operator
command:
- /bin/bash
- -c
- |
#!/bin/bash
set -o allexport
if [[ -f /etc/kubernetes/apiserver-url.env ]]; then
source /etc/kubernetes/apiserver-url.env
else
URL_ONLY_KUBECONFIG=/etc/kubernetes/kubeconfig
fi
exec /vsphere-node-label-sync-job
env:
- name: RELEASE_VERSION
value: "0.0.1-snapshot"
resources:
requests:
cpu: 10m
memory: 50Mi
limits:
cpu: 100m
memory: 100Mi
terminationMessagePolicy: FallbackToLogsOnError
Comment thread
coderabbitai[bot] marked this conversation as resolved.
# runAsNonRoot is deliberately NOT set here: this image (like the operator's own
# Deployment container, which also runs unguarded) has no USER directive in its
# Dockerfile and defaults to root, and the hostaccess SCC does not reliably assign
# a non-root UID for this pod. Setting runAsNonRoot: true without a matching
# runAsUser makes the kubelet refuse to start the container at all ("container has
# runAsNonRoot and image will run as root"), which is what happened in CI (PR #497,
# e2e-azure-ovn-upgrade): the container crash-looped for the full activeDeadlineSeconds
# window and tripped the "events should not repeat pathologically" invariant test.
securityContext:
allowPrivilegeEscalation: false
readOnlyRootFilesystem: true
capabilities:
drop:
- ALL
volumeMounts:
- mountPath: /etc/kubernetes
name: host-etc-kube
readOnly: true
Comment thread
coderabbitai[bot] marked this conversation as resolved.
volumes:
- name: host-etc-kube
hostPath:
path: /etc/kubernetes
type: Directory
11 changes: 10 additions & 1 deletion pkg/cloud/vsphere/vsphere.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,16 @@ const (
// see manifests/0000_26_cloud-controller-manager-operator_16_credentialsrequest-vsphere.yaml
globalCredsSecretName = "vsphere-cloud-credentials"

vSpherePlatformTypeLabel = "node.openshift.io/platform-type=vsphere"
// NodePlatformTypeLabelKey and NodePlatformTypeLabelValueVSphere are the label the vSphere CCM applies
// (via --node-labels, see additionalLabels below) to nodes when it first initializes them. They are
// exported so that node.openshift.io/platform-type=vsphere can be reapplied to nodes that were
// initialized before the VSphereMixedNodeEnv feature gate was enabled.
NodePlatformTypeLabelKey = "node.openshift.io/platform-type"
NodePlatformTypeLabelValueVSphere = "vsphere"
// NodeProviderIDPrefix is the spec.providerID prefix used by vSphere nodes, e.g. vsphere://4210e24f-...
NodeProviderIDPrefix = "vsphere://"

vSpherePlatformTypeLabel = NodePlatformTypeLabelKey + "=" + NodePlatformTypeLabelValueVSphere
)

var (
Expand Down
91 changes: 91 additions & 0 deletions pkg/controllers/vsphere_node_label_sync.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package controllers

import (
"context"
"errors"
"fmt"
"strings"

configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/api/features"
"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/klog/v2"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/vsphere"
)

// SyncVSphereNodeLabels performs a single pass over all nodes and retroactively applies the
// node.openshift.io/platform-type=vsphere label to vSphere nodes that are missing it. The vSphere
// CCM only ever applies this label (via its --node-labels flag, see pkg/cloud/vsphere) when it
// initializes a node for the first time, so nodes that joined the cluster before the
// VSphereMixedNodeEnv feature gate was enabled never get labeled without this repair pass. Hybrid
// clusters can mix vSphere and bare-metal nodes, so each node's spec.providerID is checked to
// confirm it is actually a vSphere node before labeling it.
//
// This is run to completion once by the vsphere-node-label-sync-job Job that the CVO installs
// (see manifests/0000_26_cloud-controller-manager-operator_45_job-vsphere-node-label-sync.yaml),
// rather than as an ongoing in-process controller.
func SyncVSphereNodeLabels(ctx context.Context, c client.Client, featureGateAccess featuregates.FeatureGateAccess) error {
infra := &configv1.Infrastructure{}
if err := c.Get(ctx, client.ObjectKey{Name: infrastructureResourceName}, infra); err != nil {
if apierrors.IsNotFound(err) {
klog.Infof("infrastructure resource not found, skipping node label sync")
return nil
}
return fmt.Errorf("failed to get infrastructure: %w", err)
}

if infra.Status.PlatformStatus == nil || infra.Status.PlatformStatus.Type != configv1.VSpherePlatformType {
klog.V(2).Infof("platform is not vSphere, skipping node label sync")
return nil
}

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
}

nodeList := &corev1.NodeList{}
if err := c.List(ctx, nodeList); err != nil {
return fmt.Errorf("failed to list nodes: %w", err)
}

var errs []error
for i := range nodeList.Items {
if err := syncVSphereNodeLabel(ctx, c, &nodeList.Items[i]); err != nil {
errs = append(errs, fmt.Errorf("node %s: %w", nodeList.Items[i].Name, err))
}
}

return errors.Join(errs...)
}

func syncVSphereNodeLabel(ctx context.Context, c client.Client, node *corev1.Node) error {
if !strings.HasPrefix(node.Spec.ProviderID, vsphere.NodeProviderIDPrefix) {
return nil
}

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
Comment on lines +75 to +83

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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 when node.openshift.io/platform-type already 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.

Suggested change
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.


if err := c.Patch(ctx, node, patch); err != nil {
return fmt.Errorf("failed to patch label: %w", err)
}

klog.Infof("added %s=%s label to node %s", vsphere.NodePlatformTypeLabelKey, vsphere.NodePlatformTypeLabelValueVSphere, node.Name)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return nil
}
Loading