CNTRLPLANE-1956: add logic to check control-plane to data-plane connectivity - #7260
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sdminonne: This pull request references CNTRLPLANE-1956 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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. |
WalkthroughAdds control-plane → data-plane connectivity checks: new reconciliation logic inspects worker nodes and konnectivity-agent pod logs, exposes a GetPodLogs hook, updates HostedControlPlane status with a DataPlaneConnectionAvailable condition, propagates it to HostedCluster, extends API condition types and test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
support/konnectivityproxy/proxy_dialer.go (1)
12-12: Comment contains incorrect package name; fix the typo on line 20.The import on line 12 correctly uses
golang.org/x/net/proxy, but the comment on line 20 incorrectly referencesgolang.org/x/xnet/proxy. The official Go module isgolang.org/x/net; there is nogolang.org/x/xnet. Update the comment to match the actual import:-// as the golang.org/x/xnet/proxy package only supports socks5 proxies, but does allow registering additional protocols. +// as the golang.org/x/net/proxy package only supports socks5 proxies, but does allow registering additional protocols.docs/content/reference/api.md (2)
3459-3463: Azure NodePool subnetID paragraph references the wrong fieldText says “The subscriptionId in the encryptionSetID must be a valid UUID” within the subnetID section. That should reference subnetID, not encryptionSetID. This is a user-facing correctness issue; please fix in the source comments and regenerate.
5729-5736: Expander descriptions appear swappedIn ExpanderString:
- Priority should select the highest user-defined priority (not “least idle resources”).
- Random should select randomly (not “highest priority”).
Please correct in source comments and regenerate.
🧹 Nitpick comments (4)
docs/content/reference/api.md (1)
293-321: Fix recurring typos in generated text (update source docs and regenerate)Examples:
- “Hypersfhit Operator” → “HyperShift Operator”
- “surce policies” → “surge policies”
- “dicate” → “dictate”
- “DiskEncyptionSet” → “DiskEncryptionSet”
- “Ephmeral” → “Ephemeral”
- “integrate with an eternally managed etcd cluster” → “externally managed”
Please correct in code/docstrings so make api-docs regenerates cleanly.
Also applies to: 3318-3327, 3341-3346, 6215-6241, 13028-13031
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (1)
39-2456: Gomega migration and new connectivity test look soundThe switch to explicit
gomega.NewWithT/NewGomegaWithTand fully-qualified matchers is consistent across the file, and the new helpers plusTest_reconciler_updateControlPlaneDatapPlaneConnectivityConditionscorrectly exercise the no‑pod and pod‑with‑logs cases via a fake client and injectableGetPodLogs. No test‑level blockers from these changes.control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (2)
1414-1449: Field selector + full log fetch may cause runtime and performance issuesTwo aspects here are worth tightening up:
Field selector on
status.phasewith cached clientif err := r.client.List(ctx, &podList, &client.ListOptions{ LabelSelector: labels.SelectorFromValidatedSet(labels.Set{"app": "konnectivity-agent"}), FieldSelector: fields.SelectorFromSet(fields.Set{"status.phase": "Running"}), Namespace: "kube-system", }); err != nil {Using a non-default field selector like
status.phaseagainst a controller-runtime cached client typically requires a registered field index for that field; otherwiseListmay return an error at runtime. Tests inject an index withWithIndex(...), butSetuphere does not configure such an index.If there is no global indexer elsewhere, either:
- register a pod
status.phaseindex on the manager’s cache, or- drop the field selector and filter for
pod.Status.Phase == corev1.PodRunningin-process after listing by label.Fetching entire pod logs on every reconcile
data, err := r.GetPodLogs(ctx, r.clientSet, pod.Namespace, pod.Name, "konnectivity-agent") if len(data) > 0 { ... }
getPodLogscurrently callsDoRawwithout anyLimitBytesorTailLines, so every reconcile can download the full log stream for eachkonnectivity-agentpod just to check “non-empty”. On busy clusters this can mean large payloads and extra load on the API server.You can significantly reduce overhead by constraining the log request, e.g.:
func getPodLogs(ctx context.Context, cs *clientset.Clientset, ns, name, container string) ([]byte, error) { opts := &corev1.PodLogOptions{ Container: container, TailLines: ptr.To[int64](1), // or small LimitBytes } return cs.CoreV1().Pods(ns).GetLogs(name, opts).DoRaw(ctx) }or by relying on pod
Runningphase alone if log inspection isn’t strictly required.Would you double-check whether a pod
status.phasefield index is already registered for this manager, and whether limiting log size still satisfies your connectivity signal requirements?
194-221: Avoid shadowing theclientsetimport for clarityIn
Setupyou have:clientset, err := clientset.NewForConfig(opts.Config) ... clientSet: clientset, GetPodLogs: getPodLogs,This works, but the local variable name shadows the imported
clientsetalias, which makes the line harder to read and easier to misinterpret in future edits.Consider renaming the variable (and corresponding struct field) to something like
kubeClientSetorguestClientSet:- clientset, err := clientset.NewForConfig(opts.Config) + kubeClientSet, err := clientset.NewForConfig(opts.Config) @@ - client: opts.Manager.GetClient(), - uncachedClient: uncachedClient, - clientSet: clientset, + client: opts.Manager.GetClient(), + uncachedClient: uncachedClient, + clientSet: kubeClientSet,This keeps the package alias and variable name distinct and improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcontrolplane_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
api/hypershift/v1beta1/hosted_controlplane.go(0 hunks)api/hypershift/v1beta1/hostedcontrolplane_conditions.go(1 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go(7 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go(43 hunks)docs/content/reference/api.md(4 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go(1 hunks)support/konnectivityproxy/proxy_dialer.go(1 hunks)
💤 Files with no reviewable changes (1)
- api/hypershift/v1beta1/hosted_controlplane.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
support/konnectivityproxy/proxy_dialer.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.godocs/content/reference/api.mdapi/hypershift/v1beta1/hostedcontrolplane_conditions.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
🔇 Additional comments (3)
api/hypershift/v1beta1/hostedcontrolplane_conditions.go (1)
1-43: LGTM: Well-documented condition constantsThe new connectivity health condition types are clearly named and thoroughly documented. The comments appropriately describe both the True and False states, and provide useful context about potential causes of connectivity failures (network policies, firewall rules, infrastructure problems).
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
828-829: LGTM: Condition propagation follows established patternThe new connectivity health conditions are properly added to the propagation list alongside other infrastructure health conditions (EtcdAvailable, KubeAPIServerAvailable, etc.). The implementation correctly follows the existing pattern where conditions are copied from HostedControlPlane to HostedCluster status, with appropriate handling for missing conditions.
docs/content/reference/api.md (1)
4908-4920: Connectivity conditions verified and properly implementedBoth conditions are correctly defined in the API, actively managed by controllers, and propagated through the HostedCluster status hierarchy. Confirmed that
ControlPlaneToDataPlaneConnectivityHealthyis set in the control-plane-operator (resources.go), both conditions are included in the HCP-to-HC condition propagation list (hostedcluster_controller.go), and tests validate their behavior.
b70c0e1 to
0953fe6
Compare
|
@sdminonne: This pull request references CNTRLPLANE-1956 which is a valid jira issue. 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. |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
|
@jparrill many many thanks! |
7133360 to
3a56acd
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (2)
68-96: Tighten pod test scaffolding and index behaviorTwo small cleanups could make these tests clearer and closer to production behavior:
initialObjectsincludes an empty&corev1.Pod{}(Line 94), but no test appears to rely on a nameless/namelesspod being present. Unless there is a specificGetagainst this object, you can likely drop it to avoid confusion about its role.- The various
indexFuncdefinitions for"status.phase"(Lines 153–155 and 1648–1650) always return[]string{"Running"}regardless of the pod’s actual phase. Since production code indexes bystatus.phaseto findRunningpods, it would be more representative to implement these helpers based onpod.Status.Phase(e.g., return"Running"only whenPhase == corev1.PodRunning, otherwisenil). That keeps the fake index semantics aligned with the real ones without making the tests more complex.These are non-blocking, but addressing them will improve test readability and fidelity.
Also applies to: 153-160, 1648-1652
2358-2454: Make connectivity condition test more robust (scheme + GetPodLogs default)The new connectivity test is valuable, but a couple of small adjustments would make it more resilient:
- The fake client for pods is built as
fake.NewClientBuilder().WithLists(podList).WithIndex(&corev1.Pod{}, "status.phase", indexFunc).Build()(Lines 2416–2423) without an explicit scheme. Elsewhere in this file you consistently useWithScheme(api.Scheme), which is the typical pattern for controller‑runtime’s fake client and avoids surprises with type registration and indexing. Consider adding.WithScheme(api.Scheme)here as well.- In the first table entry,
mockedGetPodLogsis leftnil(Line 2387), and the test currently relies onupdateControlPlaneDatapPlaneConnectivityConditionsnever callingGetPodLogswhen there are no pods. To guard against future refactors that might invokeGetPodLogsunconditionally, it would be safer to always setr.GetPodLogsto a no‑op stub in tests (returning, for example,nil, nil) and override it only when you need specific log content.Both changes are test‑only and should not affect production behavior, but they reduce the risk of brittle tests as the reconciler evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
api/hypershift/v1beta1/hostedcontrolplane_conditions.go(1 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go(7 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go(6 hunks)docs/content/reference/api.md(4 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/content/reference/api.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.goapi/hypershift/v1beta1/hostedcontrolplane_conditions.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-release-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
🔇 Additional comments (2)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
817-850: Connectivity conditions propagation looks consistentAdding
DataPlaneToControlPlaneConnectivityHealthyandControlPlaneToDataPlaneConnectivityHealthytohcpConditionsand bubbling them like the other HostedControlPlane conditions is consistent and correct; no additional handling seems required here.api/hypershift/v1beta1/hostedcontrolplane_conditions.go (1)
7-43: Condition constant definitions are clear and well-scopedCentralizing the HostedControlPlane condition
ConditionTypevalues here (including the new connectivity conditions) with concise comments is a good API cleanup; names and strings match the intended semantics and align with existing usage in controllers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (1)
1449-1454: Fix constant name typo and set status to Unknown on pod list failureTwo issues when pod listing fails:
Incorrect constant name (line 1451): Uses
hyperv1.ReconciliationErrorReasonbut the API definesReconcileErrorReason(without "ation"). This will not compile.Missing status assignment: When we cannot list pods, we don't know the connectivity state. The condition should be
Unknown, but it remains at the defaultFalseset on line 1433.Apply this diff:
var podList corev1.PodList if err := r.uncachedClient.List(ctx, &podList, client.MatchingLabels{"app": "konnectivity-agent"}, client.InNamespace("kube-system")); err != nil { + condition.Status = metav1.ConditionUnknown - condition.Reason = hyperv1.ReconciliationErrorReason + condition.Reason = hyperv1.ReconcileErrorReason condition.Message = "Couldn't list konnectivity-agent PODs in kube-system namespace: " + err.Error() return patchHCPWithCondition(hcp, condition) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
api/hypershift/v1beta1/hostedcluster_conditions.go(2 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go(7 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go(3 hunks)docs/content/reference/api.md(1 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/content/reference/api.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.goapi/hypershift/v1beta1/hostedcluster_conditions.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
🧬 Code graph analysis (3)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
api/hypershift/v1beta1/hostedcluster_conditions.go (1)
DataPlaneConnectionAvailable(206-206)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (3)
api/hypershift/v1beta1/hosted_controlplane.go (1)
HostedControlPlane(28-39)api/hypershift/v1beta1/hostedcluster_conditions.go (6)
DataPlaneConnectionAvailable(206-206)ReconcileErrorReason(262-262)DataPlaneConnectionNoWorkerNodesAvailableReason(270-270)DataPlaneConnectionNoKonnectivityAgentPodsNotFoundReason(266-266)AsExpectedReason(212-212)AllIsWellMessage(276-276)support/util/util.go (1)
CountAvailableNodes(735-755)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (4)
support/api/scheme.go (1)
Scheme(60-60)control-plane-operator/hostedclusterconfigoperator/api/scheme.go (1)
Scheme(37-37)api/hypershift/v1beta1/hosted_controlplane.go (1)
HostedControlPlane(28-39)api/hypershift/v1beta1/hostedcluster_conditions.go (5)
DataPlaneConnectionAvailable(206-206)DataPlaneConnectionNoWorkerNodesAvailableReason(270-270)DataPlaneConnectionNoKonnectivityAgentPodsNotFoundReason(266-266)AsExpectedReason(212-212)AllIsWellMessage(276-276)
🔇 Additional comments (9)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
819-849: BubblingDataPlaneConnectionAvailableup to HostedCluster is correctly wired.Including
hyperv1.DataPlaneConnectionAvailablein the propagatedhcpConditionsslice follows the existing pattern for other HCP conditions (Unknown by default, overridden when present on HCP,ObservedGenerationupdated). This cleanly exposes the new connectivity status onHostedClusterwithout special‑casing.control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (2)
41-42: Test wiring for status updates and pod‑log mocking looks sound.Using a dedicated
clientsetimport solely to type theGetPodLogsmock, plus addingWithStatusSubresource(&hyperv1.HostedControlPlane{})to the fakecpClientinTestReconcileErrorHandling, keeps the tests aligned with how the reconciler updates HCP status while avoiding fake‑client status subresource quirks. No further changes needed here.Also applies to: 161-201
2353-2556: Connectivity condition test is comprehensive and correctly isolates scenarios.
newConditionkeeps expectations focused on type/reason/status/message, andTest_reconciler_reconcileControlPlaneDataPlaneConnectivityConditionscleanly exercises all key paths: no workers, no/running/pending konnectivity pods, and both successful/failed log retrieval, using separate fake clients for nodes, pods, and HCP status plus an injectableGetPodLogs. The setup and assertions look correct and maintainable.api/hypershift/v1beta1/hostedcluster_conditions.go (2)
198-206: LGTM: Well-documented connectivity conditionThe new
DataPlaneConnectionAvailablecondition type is clearly documented with appropriate True/False semantics and helpful guidance on potential failure scenarios.
262-270: LGTM: Reason constants are well-definedThe new reason constants follow naming conventions and provide clear semantics for the different connectivity failure modes.
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (4)
74-74: LGTM: Clean infrastructure additionsThe clientset import, reconciler field, and mockable GetPodLogs function field are properly structured to support the connectivity checks.
Also applies to: 131-131, 150-152
154-161: LGTM: Efficient log fetching with size limitThe 1024-byte limit on log fetches is appropriate for a connectivity check—it verifies logs exist without pulling large volumes on every reconcile.
198-201: LGTM: Proper initialization in SetupThe clientset and GetPodLogs function are correctly initialized and wired in the Setup function.
Also applies to: 206-206, 224-224
563-566: LGTM: Clean integration into Reconcile loopThe connectivity condition reconciliation is properly integrated with appropriate logging and error collection.
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@sdminonne Understood, I'll skip this suggestion. Let me know if you need anything else!
This condition follows the True=healthy, False=unhealthy pattern and provide visibility into connectivity issues that may require external intervention. The control plane to data plance connection is detected through an oc logs to konnectivity-agent PODs running in control plane kube-system namespace. Before executing the logs command it checks if nodes are available The reconciliation set the condition to Unknown in case no healthy nodes are present. In case logs is succesfull condition value is set to True otherwise is set to False. Fixes CNTRLPLANE-1956 Signed-off-by: Salvatore Dario Minonne <sminonne@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
58f5ba6 to
0d5f3d6
Compare
openshift-ci-robot
removed
the
verified
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (1)
1418-1489: Fix typo in reason constant and clarify status for pod‑list failuresThe overall structure of
reconcileControlPlaneDataPlaneConnectivityConditionsis good (nodes → pods → logs, with a singlepatchHCPWithConditionhelper), but there are two issues worth addressing:
- Compile‑time error: wrong reason constant
Line 1451 uses
hyperv1.ReconciliationErrorReason, but the API definesReconcileErrorReason(seehostedcluster_conditions.go). This will fail to compile.
- Pod‑list failure should report an Unknown state, not a definitive False
When listing konnectivity‑agent pods fails, the condition currently:
- Leaves
Statusat the defaultFalse.- Sets
Reasonto the (misspelled) reconciliation error reason and a message about failing to list pods.In that scenario, we don’t actually know if the control plane can reach the data plane; we just couldn’t evaluate the pods. This is closer to the “Unable to count worker nodes” branch where you already set
Status=Unknownand useReconcileErrorReason.A minimal fix that corrects both issues would be:
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:
- Fixes the constant name so the code builds.
- Aligns the semantics of this error path with the earlier node‑count error: the condition reports “Unknown” with a reconcile error reason when connectivity can’t be evaluated at all.
Everything else in the function (no workers → Unknown/NoWorkerNodes, no running pods → False/NoKonnectivityAgentPodsNotFound, running pod with logs → True/AsExpected) looks coherent.
🧹 Nitpick comments (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (1)
2354-2556: Connectivity condition tests are solid; optional: add coverage for pod‑list failuresThe
newConditionhelper and the table inTest_reconciler_reconcileControlPlaneDataPlaneConnectivityConditionsexercise the main happy/edge paths (no nodes, no pods, pending pods, running pods with/without logs) and align with the controller logic.If you want to harden behavior further, consider adding one more case where
r.uncachedClient.Listfails so theReconcileErrorReason/ “couldn’t list konnectivity-agent PODs” path is locked in by tests. Not required, but would fully cover the new condition semantics.control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (1)
128-161: GetPodLogs wiring is reasonable; consider a defensive default for future call‑sitesUsing a typed
clientset.Clientsetand injectingGetPodLogsas a function field is a good way to keep the log access logic mockable while avoiding controller‑runtime client limitations. The boundedLimitBytesingetPodLogsalso keeps per‑reconcile overhead in check.One thing to keep in mind is that
GetPodLogsis nil for anyreconcilerconstructed outsideSetupunless explicitly set (tests do this for the new connectivity test). If future code paths constructreconcilermanually and callreconcileControlPlaneDataPlaneConnectivityConditionswithout wiringGetPodLogs, it will panic on a nil func call.Consider a small guard, e.g. in
reconcileControlPlaneDataPlaneConnectivityConditions:func (r *reconciler) reconcileControlPlaneDataPlaneConnectivityConditions(ctx context.Context, hcp *hyperv1.HostedControlPlane, log logr.Logger) error { + if r.GetPodLogs == nil { + return fmt.Errorf("GetPodLogs is not configured on reconciler") + }or assigning a default implementation if
GetPodLogsis nil inSetup, to make the behavior more robust to future refactors.Also applies to: 172-225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
api/hypershift/v1beta1/hostedcluster_conditions.go(2 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go(7 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go(3 hunks)docs/content/reference/api.md(1 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
- docs/content/reference/api.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.goapi/hypershift/v1beta1/hostedcluster_conditions.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
🧬 Code graph analysis (2)
api/hypershift/v1beta1/hostedcluster_conditions.go (1)
api/hypershift/v1beta1/hosted_controlplane.go (1)
ConditionType(294-294)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (1)
api/hypershift/v1beta1/hostedcluster_conditions.go (6)
DataPlaneConnectionAvailable(206-206)ReconcileErrorReason(262-262)DataPlaneConnectionNoWorkerNodesAvailableReason(270-270)DataPlaneConnectionNoKonnectivityAgentPodsNotFoundReason(266-266)AsExpectedReason(212-212)AllIsWellMessage(276-276)
🔇 Additional comments (2)
api/hypershift/v1beta1/hostedcluster_conditions.go (1)
199-207: New connectivity condition and reasons look consistent with existing API patterns
DataPlaneConnectionAvailableand the associated reason constants follow the existing condition/reason conventions and provide the right expressiveness for the controller logic; no blocking issues here.Also applies to: 262-270
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (1)
563-566: Placement of connectivity condition reconciliation in the main Reconcile loop looks appropriateRunning
reconcileControlPlaneDataPlaneConnectivityConditionsimmediately after konnectivity agent reconciliation, and aggregating any patch errors into the existingerrsslice, integrates the new condition cleanly into the existing flow without changing other behaviors.
|
/lgtm |
|
/test e2e-aks |
|
/test e2e-aks-4-20 |
|
/test e2e-aks |
|
/test e2e-aks-4-20 |
|
/test e2e-aks |
|
/test e2e-aks-4-20 |
|
/retest-required |
|
/test e2e-aks-4-20 |
1 similar comment
|
/test e2e-aks-4-20 |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/verified by @mgencur |
openshift-ci-robot
added
the
verified
|
@sdminonne: 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. |
|
/retest-required |
|
@sdminonne: 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. |
…condition to HostedCluster Add ControlPlaneConnectionAvailable to the list of conditions copied from HostedControlPlane to HostedCluster status. This follows the same pattern established by DataPlaneConnectionAvailable in PR openshift#7260. The condition monitors whether data plane workloads can reach control plane components via per-node DaemonSet-based connectivity checking. Signed-off-by: Salvatore Dario Minonne <sminonne@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
6 participants
What this PR does / why we need it:
We need this PR to create and update a status.condition in hosted-control-plane CR to show the status of connection between control-plane and data-plane.
It lists the konnecivity-agent PODs and it get the logs.
Which issue(s) this PR fixes:
Fixes CNTRLPLANE-1956
Special notes for your reviewer:
Checklist: