Skip to content
Merged
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
17 changes: 17 additions & 0 deletions api/hypershift/v1beta1/hostedcluster_conditions.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,15 @@ const (
// This condition is used to track the status of the recovery process and to determine if the HostedCluster
// is ready to be used after restoration.
HostedClusterRestoredFromBackup ConditionType = "HostedClusterRestoredFromBackup"

// DataPlaneConnectionAvailable indicates whether the control plane has a successful
// network connection to the data plane components.
// **True** means the control plane can successfully reach the data plane nodes.
// **False** means there are network connection issues preventing the control plane from reaching the data plane.
// A failure here suggests potential issues such as: network policy restrictions,
// firewall rules, missing data plane nodes, or problems with infrastructure
// components like the konnectivity-agent workload.
DataPlaneConnectionAvailable ConditionType = "DataPlaneConnectionAvailable"
)

// Reasons.
Expand Down Expand Up @@ -250,7 +259,15 @@ const (

RecoveryFinishedReason = "RecoveryFinished"

ReconcileErrorReason = "ReconcileError"

CloudResourcesCleanupSkippedReason = "CloudResourcesCleanupSkipped"

DataPlaneConnectionNoKonnectivityAgentPodsNotFoundReason = "KonnectivityAgentPodsNotFound"

DataPlaneConnectionLogsAccessFailedReason = "LogsAccessFailed"

DataPlaneConnectionNoWorkerNodesAvailableReason = "NoWorkerNodesAvailable"
)

// Messages.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ import (
"k8s.io/apimachinery/pkg/types"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/sets"
clientset "k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
"k8s.io/utils/ptr"
Expand Down Expand Up @@ -127,6 +128,7 @@ exec /bin/azure-cloud-node-manager \
type reconciler struct {
client client.Client
uncachedClient client.Client
clientSet *clientset.Clientset
upsert.CreateOrUpdateProvider
platformType hyperv1.PlatformType
rootCA string
Expand All @@ -144,6 +146,18 @@ type reconciler struct {
operateOnReleaseImage string
ImageMetaDataProvider util.ImageMetadataProvider
cleanupTracker *util.CleanupTracker

// exposed for unit test since GetLogs looks hard to be mocked
GetPodLogs func(context context.Context, clientset *clientset.Clientset, namespace, name, container string) ([]byte, error)
}

func getPodLogs(ctx context.Context, clientSet *clientset.Clientset, namespace, name, container string) ([]byte, error) {
limit := int64(1024)
opts := &corev1.PodLogOptions{
Container: container,
LimitBytes: &limit,
}
return clientSet.CoreV1().Pods(namespace).GetLogs(name, opts).DoRaw(ctx)
}

// eventHandler is the handler used throughout. As this controller reconciles all kind of different resources
Expand Down Expand Up @@ -181,9 +195,15 @@ func Setup(ctx context.Context, opts *operator.HostedClusterConfigOperatorConfig
return fmt.Errorf("failed to create kubevirt infra uncached client: %w", err)
}

clientset, err := clientset.NewForConfig(opts.Manager.GetConfig())
if err != nil {
return fmt.Errorf("failed to initialize kubeClient from config: %w", err)
}

c, err := controller.New(ControllerName, opts.Manager, controller.Options{Reconciler: &reconciler{
client: opts.Manager.GetClient(),
uncachedClient: uncachedClient,
clientSet: clientset,
CreateOrUpdateProvider: opts.TargetCreateOrUpdateProvider,
platformType: opts.PlatformType,
rootCA: opts.InitialCA,
Expand All @@ -201,6 +221,7 @@ func Setup(ctx context.Context, opts *operator.HostedClusterConfigOperatorConfig
operateOnReleaseImage: opts.OperateOnReleaseImage,
ImageMetaDataProvider: opts.ImageMetaDataProvider,
cleanupTracker: util.NewCleanupTracker(),
GetPodLogs: getPodLogs,
}})
if err != nil {
return fmt.Errorf("failed to construct controller: %w", err)
Expand Down Expand Up @@ -539,6 +560,11 @@ func (r *reconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result
errs = append(errs, fmt.Errorf("failed to reconcile konnectivity agent: %w", err))
}

log.Info("reconciling control-Plane to data-Plane status conditions")
if err := r.reconcileControlPlaneDataPlaneConnectivityConditions(ctx, hcp, log); err != nil {
errs = append(errs, fmt.Errorf("failed to update ControlPlaneToDataPlaneConnectivity condition: %w", err))
}
Comment on lines +563 to +566

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Align connectivity condition semantics when konnectivity-agent pod listing fails

The core logic of reconcileControlPlaneDataPlaneConnectivityConditions is solid (nodes → pods → logs), but the pod-list failure branch currently misreports the reason:

if err := r.uncachedClient.List(...); err != nil {
    condition.Reason = hyperv1.ControlPlaneToDataPlaneReasonNoKonnectivityAgentPodsFound
    condition.Message = "Couldn't list konnectivity-agent PODs in kube-system namespace: " + err.Error()
    return patchHCPWithCondition(hcp, condition)
}

Here, we don’t know whether pods exist; we just failed to list them (RBAC, connectivity, API outage, etc.). Surfacing this as NoKonnectivityAgentPodsFound is misleading for operators and any automation keying on Reason.

To better reflect the failure mode and match how other “can’t determine connectivity” cases are handled:

  • Treat this as an Unknown connectivity state.
  • Use a reason that reflects inability to inspect the data-plane (reusing ControlPlaneToDataPlaneReasonLogAccessFailed is less misleading than claiming “no pods found”).

Concrete tweak:

	var podList corev1.PodList
	if err := r.uncachedClient.List(ctx, &podList,
		client.MatchingLabels{"app": "konnectivity-agent"}, client.InNamespace("kube-system")); err != nil {
-		condition.Reason = hyperv1.ControlPlaneToDataPlaneReasonNoKonnectivityAgentPodsFound
-		condition.Message = "Couldn't list konnectivity-agent PODs in kube-system namespace: " + err.Error()
-		return patchHCPWithCondition(hcp, condition)
+		condition.Status = metav1.ConditionUnknown
+		condition.Reason = hyperv1.ControlPlaneToDataPlaneReasonLogAccessFailed
+		condition.Message = "Failed to list konnectivity-agent PODs in kube-system namespace: " + err.Error()
+		return patchHCPWithCondition(hcp, condition)
	}

This keeps the rest of your paths intact:

  • Unknown / NoWorkerNodesAvailable for zero workers.
  • False / NoKonnectivityAgentPodsFound for zero running pods.
  • False / LogAccessFailed when running pods exist but logs can’t be read or are empty.
  • True / AsExpected + AllIsWellMessage when at least one running pod has readable logs.

If you adopt this, it’s worth adding a small test case in Test_reconciler_reconcileControlPlaneDataPlaneConnectivityConditions that exercises the “list pods fails” path (e.g., by wrapping r.uncachedClient with a client that returns an error on List) to lock the semantics in.

Also applies to: 1418-1485

🤖 Prompt for AI Agents
In
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
around lines 563-566 (and the similar branch at 1418-1485), the pod-list failure
branch currently sets the connectivity condition to indicate
"NoKonnectivityAgentPodsFound"; instead, when r.uncachedClient.List(...) returns
an error you should treat connectivity as Unknown: set condition.Status =
metav1.ConditionUnknown, set condition.Reason =
hyperv1.ControlPlaneToDataPlaneReasonLogAccessFailed, set condition.Message to
include the list error (e.g. "Couldn't list konnectivity-agent PODs in
kube-system namespace: "+err.Error()), and then call patchHCPWithCondition(hcp,
condition) as before; also add a unit test in
Test_reconciler_reconcileControlPlaneDataPlaneConnectivityConditions that
simulates a List error (wrap uncachedClient to return an error on List) to lock
in the new semantics.


log.Info("reconciling openshift apiserver apiservices")
if err := r.reconcileOpenshiftAPIServerAPIServices(ctx, hcp); err != nil {
errs = append(errs, fmt.Errorf("failed to reconcile openshift apiserver service: %w", err))
Expand Down Expand Up @@ -1389,6 +1415,79 @@ func (r *reconciler) reconcileClusterVersion(ctx context.Context, hcp *hyperv1.H
return nil
}

func (r *reconciler) reconcileControlPlaneDataPlaneConnectivityConditions(ctx context.Context, hcp *hyperv1.HostedControlPlane, log logr.Logger) error {
patchHCPWithCondition := func(hcp *hyperv1.HostedControlPlane, condition *metav1.Condition) error {
originalHCP := hcp.DeepCopy()
if !meta.SetStatusCondition(&hcp.Status.Conditions, *condition) {
return nil // No status change; avoid unnecessary API call.
}
if err := r.cpClient.Status().Patch(ctx, hcp, client.MergeFrom(originalHCP)); err != nil {
return fmt.Errorf("failed to update HostedControlPlane status with %s condition: %w", condition.Type, err)
}
log.Info(string(hyperv1.DataPlaneConnectionAvailable) + " updated")
return nil
}

condition := &metav1.Condition{
Type: string(hyperv1.DataPlaneConnectionAvailable),
Status: metav1.ConditionFalse, // False by default
}
totalNodes, err := util.CountAvailableNodes(ctx, r.client)
if err != nil {
condition.Status = metav1.ConditionUnknown
condition.Reason = hyperv1.ReconcileErrorReason
condition.Message = "Unable to count worker nodes: " + err.Error()
return patchHCPWithCondition(hcp, condition)
}
if totalNodes == 0 {
condition.Status = metav1.ConditionUnknown
condition.Reason = hyperv1.DataPlaneConnectionNoWorkerNodesAvailableReason
condition.Message = "No worker nodes available"
return patchHCPWithCondition(hcp, condition)
}
var podList corev1.PodList
if err := r.uncachedClient.List(ctx, &podList,
client.MatchingLabels{"app": "konnectivity-agent"}, client.InNamespace("kube-system")); err != nil {
condition.Reason = hyperv1.ReconciliationErrorReason
condition.Message = "Couldn't list konnectivity-agent PODs in kube-system namespace: " + err.Error()
return patchHCPWithCondition(hcp, condition)
}

logsFound := false
runningPodsFound := false
for _, pod := range podList.Items {
if pod.Status.Phase != corev1.PodRunning {
continue
}
runningPodsFound = true
data, err := r.GetPodLogs(ctx, r.clientSet, pod.Namespace, pod.Name, "konnectivity-agent")
if err != nil {
log.Error(err,
fmt.Sprintf("failed to get logs for konnectivity-agent pod %s/%s", pod.Namespace, pod.Name))
continue
}
if len(data) > 0 {
logsFound = true
break
}
}
if !runningPodsFound {
condition.Reason = hyperv1.DataPlaneConnectionNoKonnectivityAgentPodsNotFoundReason
condition.Message = "Couldn't find any konnectivity-agent running in data plane"
} else {
if !logsFound {
condition.Reason = hyperv1.DataPlaneConnectionNoKonnectivityAgentPodsNotFoundReason
condition.Message = "failed to read konnectivity-agent logs from data plane"
} else {
condition.Status = metav1.ConditionTrue
condition.Reason = hyperv1.AsExpectedReason
condition.Message = hyperv1.AllIsWellMessage
}
}
Comment on lines +1474 to +1486

@coderabbitai coderabbitai Bot Nov 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix semantic mismatch: pods found but logs unreadable should use LogsAccessFailedReason

When runningPodsFound is true but logsFound is false (lines 1478-1480), the code sets:

  • Reason: DataPlaneConnectionNoKonnectivityAgentPodsNotFoundReason
  • Message: "failed to read konnectivity-agent logs from data plane"

This is misleading—the reason says "pods not found" but the scenario is "running pods exist, logs couldn't be read." The reason should reflect the actual failure mode.

Apply this diff:

 	if !runningPodsFound {
 		condition.Reason = hyperv1.DataPlaneConnectionNoKonnectivityAgentPodsNotFoundReason
 		condition.Message = "Couldn't find any konnectivity-agent running in data plane"
 	} else {
 		if !logsFound {
-			condition.Reason = hyperv1.DataPlaneConnectionNoKonnectivityAgentPodsNotFoundReason
+			condition.Reason = hyperv1.DataPlaneConnectionLogsAccessFailedReason
 			condition.Message = "failed to read konnectivity-agent logs from data plane"
 		} else {
 			condition.Status = metav1.ConditionTrue
 			condition.Reason = hyperv1.AsExpectedReason
 			condition.Message = hyperv1.AllIsWellMessage
 		}
 	}
🤖 Prompt for AI Agents
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
around lines 1474-1486: when runningPodsFound is true but logsFound is false the
code erroneously sets Reason to
hyperv1.DataPlaneConnectionNoKonnectivityAgentPodsNotFoundReason; change that
Reason to hyperv1.DataPlaneConnectionLogsAccessFailedReason (or the correct
LogsAccessFailedReason constant from hyperv1) while keeping the existing message
("failed to read konnectivity-agent logs from data plane"), so the condition
Reason accurately reflects logs access failure rather than pods-not-found.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@coderabbitai ignore it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sdminonne Understood, I'll skip this suggestion. Let me know if you need anything else!


return patchHCPWithCondition(hcp, condition)
}
Comment on lines +1418 to +1489

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Typo in reason constant and pod-list error semantics

There are two issues in reconcileControlPlaneDataPlaneConnectivityConditions:

  1. Reason constant typo (compile-time blocker)
    Line 1451 uses hyperv1.ReconciliationErrorReason, but the conditions API exposes ReconcileErrorReason. This will not compile.

  2. Status for pod-list failures should be Unknown, not False
    When listing konnectivity-agent pods fails, we don’t know the connectivity state; leaving Status at its default False is misleading compared to the “we couldn’t evaluate” message.

You can fix both with a small change:

	var podList corev1.PodList
	if err := r.uncachedClient.List(ctx, &podList,
		client.MatchingLabels{"app": "konnectivity-agent"}, client.InNamespace("kube-system")); err != nil {
-		condition.Reason = hyperv1.ReconciliationErrorReason
-		condition.Message = "Couldn't list konnectivity-agent PODs in kube-system namespace: " + err.Error()
+		condition.Status = metav1.ConditionUnknown
+		condition.Reason = hyperv1.ReconcileErrorReason
+		condition.Message = "Couldn't list konnectivity-agent PODs in kube-system namespace: " + err.Error()
		return patchHCPWithCondition(hcp, condition)
	}

This both restores compilation (correct constant) and keeps the condition semantics consistent with the earlier CountAvailableNodes error branch.

🤖 Prompt for AI Agents
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
lines 1418-1489: fix two issues — replace the incorrect constant
hyperv1.ReconciliationErrorReason with hyperv1.ReconcileErrorReason to match the
exported API (restore compilation), and when the List of konnectivity-agent pods
fails set condition.Status = metav1.ConditionUnknown (not left at False) along
with condition.Reason = hyperv1.ReconcileErrorReason and condition.Message
describing the list failure so the condition reflects an unknown/evaluation
error; then return patchHCPWithCondition(hcp, condition).


func (r *reconciler) reconcileOpenshiftAPIServerAPIServices(ctx context.Context, hcp *hyperv1.HostedControlPlane) error {
rootCA := cpomanifests.RootCASecret(hcp.Namespace)
if err := r.cpClient.Get(ctx, client.ObjectKeyFromObject(rootCA), rootCA); err != nil {
Expand Down
Loading