Add status.conditions to DiscoveredCluster#1988
Add status.conditions to DiscoveredCluster#1988openshift-merge-bot[bot] merged 12 commits intostolostron:mainfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRefactors DiscoveredCluster conditions from an enum to string-based types, removes LastUpdateTime, adds Message/Reason/ObservedGeneration, updates CRD schemas and generated deepcopy logic, bumps bundle metadata, and adds controller logic/tests to compute and reconcile status conditions each reconcile loop. Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler as Reconciler
participant API as Kubernetes API
participant Status as Status Subresource
rect rgba(200,230,201,0.5)
Reconciler->>API: GET DiscoveredCluster
API-->>Reconciler: DiscoveredCluster object
Reconciler->>Reconciler: buildStatusConditions(spec, meta)
Reconciler->>API: GET latest DiscoveredCluster (fresh copy)
API-->>Reconciler: fresh object
Reconciler->>Status: compare & PATCH status (if changed)
Status-->>Reconciler: 200 OK / conflict error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 49 minutes and 35 seconds.Comment |
Implemented proper Kubernetes status subresource with conditions to track cluster state. Replaces custom condition type with standard metav1.Condition for better K8s ecosystem compatibility. Changes: - Replaced custom DiscoveredClusterCondition with metav1.Condition - Added condition types: Available, Managed - Added condition reasons: RecentTelemetry, StaleTelemetry, ImportedAsManagedCluster, NotImported - Updated DiscoveredClusterStatus to use []metav1.Condition - Added updateStatus() to populate conditions in reconcile loop - Available condition tracks Active/Stale state from OCM with timestamp details - Managed condition tracks whether cluster is imported as ManagedCluster - Regenerated deepcopy and CRDs Benefits: - Explains WHY cluster is Active/Stale (no recent telemetry) - Shows WHEN last seen (timestamp in message) - Tracks managed state separately from availability - kubectl describe shows full troubleshooting context - Compatible with kubectl wait --for=condition Signed-off-by: dislbenn <dbennett@redhat.com>
Added comprehensive test coverage for the new status condition functions: - Test_Reconciler_buildStatusConditions: Tests condition generation for Active/Stale and Managed/NotManaged states - Test_conditionEqual: Tests condition comparison logic Updated bundle/manifests/discovery.open-cluster-management.io_discoveredclusters.yaml to include the new status.conditions schema. All tests passing. Signed-off-by: dislbenn <dbennett@redhat.com>
Signed-off-by: dislbenn <dbennett@redhat.com>
e48bb6e to
251d89b
Compare
When a DiscoveredCluster is deleted during reconciliation, the updateStatus function should not treat it as an error. This can happen in test scenarios where resources are cleaned up while reconcile is in progress. Changes: - Added NotFound error handling when fetching fresh resource copy - Added NotFound error handling when updating status subresource Fixes test failure in: Test_DiscoveredCluster_Reconciler_Reconcile/should_create_auto_import_Secret_object Signed-off-by: dislbenn <dbennett@redhat.com>
metav1.Condition has required fields (type, status, reason, message, lastTransitionTime) baked into the upstream Kubernetes API definition. To allow all condition fields to be optional, we need a custom type. Changes: - Defined custom DiscoveredClusterCondition struct with +optional markers - Replaced []metav1.Condition with []DiscoveredClusterCondition in status - Updated controller buildStatusConditions to return custom type - Updated conditionEqual helper function signature - Updated all tests to use custom condition type - Regenerated CRDs and bundle manifests Result: No required fields in conditions schema Signed-off-by: dislbenn <dbennett@redhat.com>
Kubernetes requires that fields used in x-kubernetes-list-map-keys must either be required or have a default value. Since 'type' is the key field for merging conditions, it must be required. Changes: - Removed +optional marker from DiscoveredClusterCondition.Type - Removed omitempty from Type json tag - Regenerated CRDs with required: [type] This fixes the KinD test failure: 'this property is in x-kubernetes-list-map-keys, so it must have a default or be a required property' Signed-off-by: dislbenn <dbennett@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/discoveredcluster_controller.go`:
- Around line 137-150: The updateStatus function currently computes
newConditions from the stale reconcile snapshot dc; re-read the resource into
fresh and use that fresh object when calling r.buildStatusConditions and when
setting ObservedGeneration/status fields so the status reflects the latest
spec/generation; update all references in updateStatus from dc to fresh (e.g.,
call r.buildStatusConditions(ctx, fresh) and use
fresh.Generation/ObservedGeneration) to avoid writing stale reasons/messages.
- Around line 149-177: The new conditions currently replace
fresh.Status.Conditions wholesale (built by buildStatusConditions) and reset
lastTransitionTime for every condition; change the logic so that after obtaining
newConditions you iterate over newConditions and for each entry find the
matching condition by Type in fresh.Status.Conditions and, if the Status value
is identical, copy the existing condition's LastTransitionTime into the new
condition (only update LastTransitionTime when the Status value actually
changes). You can implement this either inside buildStatusConditions (pass the
old slice to reuse timestamps) or immediately after calling
buildStatusConditions before assigning fresh.Status.Conditions; refer to
buildStatusConditions, fresh.Status.Conditions and the equality check
(conditionEqual) to locate the relevant code paths.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: e1374216-9d2c-4a87-a927-9b39fc993765
📒 Files selected for processing (9)
api/v1/discoveredcluster_types.goapi/v1/zz_generated.deepcopy.gobundle.Dockerfilebundle/manifests/discovery.clusterserviceversion.yamlbundle/manifests/discovery.open-cluster-management.io_discoveredclusters.yamlbundle/metadata/annotations.yamlconfig/crd/bases/discovery.open-cluster-management.io_discoveredclusters.yamlcontrollers/discoveredcluster_controller.gocontrollers/discoveredcluster_controller_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 the current code and only fix it if needed.
Inline comments:
In `@api/v1/discoveredcluster_types.go`:
- Around line 114-132: The condition fields Status, LastTransitionTime, Message
and Reason are incorrectly marked optional; remove the "+optional" markers and
the `omitempty` json tags for Status, LastTransitionTime, Message and Reason in
the condition struct (the fields named Status, LastTransitionTime, Message,
Reason on the condition type in discoveredcluster_types.go) so they are required
per KEP-1623; keep ObservedGeneration as appropriate, then regenerate
CRDs/bundle (run the project's codegen/crd generation) to reflect the required
fields.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 45f36a22-ba3b-45e0-942c-aa254a18bef1
📒 Files selected for processing (4)
api/v1/discoveredcluster_types.gobundle/manifests/discovery.clusterserviceversion.yamlbundle/manifests/discovery.open-cluster-management.io_discoveredclusters.yamlconfig/crd/bases/discovery.open-cluster-management.io_discoveredclusters.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- bundle/manifests/discovery.clusterserviceversion.yaml
Regenerated bundle to include Provenance, SupportLevel, and Usage fields added in stolostron#1987. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: dislbenn <dbennett@redhat.com>
- Use fresh resource copy in buildStatusConditions instead of stale dc - Preserve LastTransitionTime when condition Status unchanged - Only update LastTransitionTime when Status actually changes Addresses CodeRabbit review comments on PR stolostron#1988. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: dislbenn <dbennett@redhat.com>
9ce1f8c to
5fcb433
Compare
Signed-off-by: dislbenn <dbennett@redhat.com>
Previously, status.conditions were only populated on ROSA clusters because the controller's event filter limited reconciliation to ROSA and MCE-HCP types. This left OCP, ARO, OSD, and other cluster types without status conditions. Changes: - Removed ShouldReconcile predicate and WithEventFilter from SetupWithManager - Controller now reconciles all DiscoveredCluster types for status updates - Auto-import logic remains safely restricted to ROSA/MCE-HCP via: * Webhook validation (rejects importAsManagedCluster on unsupported types) * Reconcile guard (if ImportAsManagedCluster check) * Switch statement (default case skips non-ROSA/MCE-HCP) - Removed Test_Reconciler_ShouldReconcile (function deleted) - Removed unused event/predicate imports - Updated function docstrings from "..." placeholders Result: All cluster types now receive Available/Managed status conditions Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: dislbenn <dbennett@redhat.com>
Changed if-else chain to tagged switch for clearer auth method handling. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: dislbenn <dbennett@redhat.com>
b744337 to
4a4e240
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cameronmwall, dislbenn 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 |
Description
Implements proper Kubernetes status subresource with conditions to provide detailed observability for DiscoveredCluster state. Replaces custom condition types with standard
metav1.Conditionfor better K8s ecosystem compatibility.Related Issue
N/A - Enhancement to provide better status observability
Changes Made
Status Structure
Replaced custom
DiscoveredClusterConditionwith standardmetav1.Condition:Before:
After:
Condition Types
Available - Tracks cluster availability based on OCM telemetry
Status=True+Reason=RecentTelemetry: Cluster is ActiveStatus=False+Reason=StaleTelemetry: Cluster is StaleMessage: Includes last telemetry timestamp for troubleshootingManaged - Tracks import status
Status=True+Reason=ImportedAsManagedCluster: Cluster importedStatus=False+Reason=NotImported: Not importedController Updates
updateStatus()method to populate conditions during reconcilebuildStatusConditions()to construct conditions from cluster stateExample Status
Benefits
✅ Better observability - Clear explanation of why cluster is Active/Stale
✅ Troubleshooting context - Timestamp shows when last seen
✅ Multiple states - Tracks availability AND managed status separately
✅ K8s best practice - Uses standard metav1.Condition
✅ Automation friendly -
kubectl wait --for=condition=Available✅ kubectl describe - Rich status information visible to users
Checklist
Additional Notes
This change maintains backward compatibility -
spec.statusstill exists and is populated from OCM. The status.conditions provide additional context and details.Users can now run:
Reviewers
/cc @ngraham20 @cameronmwall
Definition of Done
Summary by CodeRabbit
New Features
Chores
Tests