HYPERFLEET-999 - refactor: standardize adapter conditions for deletions#40
HYPERFLEET-999 - refactor: standardize adapter conditions for deletions#40ldornele wants to merge 3 commits intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR makes three Helm adapter task configs deletion-aware. Each task computes an Sequence Diagram(s)sequenceDiagram
participant Resource as Cluster/Nodepool
participant Adapter as Adapter Task
participant K8s as Kubernetes API (ManifestWork/ConfigMap)
participant Store as Adapter Execution Status
Resource->>Adapter: deleted_time present -> compute is_deleting=true
Adapter->>Adapter: validationCheck allows run when is_deleting
Adapter->>K8s: apply manifests or trigger deletion lifecycle (delete hook / foreground)
alt is_deleting == true
K8s->>K8s: perform deletion (ConfigMap delete / ManifestWork foreground)
K8s-->>Adapter: resource missing or deletion in progress
else
K8s-->>Adapter: resource applied / available
end
Adapter->>Store: update adapter.executionStatus, errorReason/errorMessage
Adapter->>Adapter: evaluate conditions (Applied, Available, Health, Finalized)
Adapter-->>Resource: report status with deletion-aware reasons/messages and Finalized state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/adapter1/adapter-task-config.yaml`:
- Around line 151-163: The current expressions (using is_deleting and
adapter.?executionStatus.orValue("") == "success") gate all deletion semantics
on executionStatus == "success", causing in-flight or failed deletions to be
reported as "ResourceActive"; update the three expressions (the boolean
Finalized expression, reason.expression, and message.expression) to first check
is_deleting and then branch by executionStatus: when is_deleting is false return
ResourceActive/False as before, when is_deleting is true then if
resources.?resource0.hasValue() is false return CleanupConfirmed/True with a
success message, else if adapter.?executionStatus.orValue("") == "failed" return
CleanupFailed/False with an appropriate failure message, else (executionStatus
empty/in-progress) return CleanupInProgress/False with an in-progress message —
implement these changes in the expressions referencing is_deleting,
adapter.?executionStatus, and resources.?resource0.
In `@helm/adapter2/adapter-task-config.yaml`:
- Around line 302-328: The Finalized condition now reports deletion completion
but the existing Applied and Available condition expressions still evaluate as
True during deletion; update the Applied and Available condition expressions
(the same blocks that currently compute "Applied"/"Available") to short-circuit
when is_deleting is true or when adapter.?executionStatus.orValue("") ==
"success" during deletion so they return "False" (and appropriate
reason/message) while cleanup is in progress; ensure you reference the same
resource presence checks (resources.?resource0, resources.?namespace0,
resources.?configmap0) and the adapter.?executionStatus check so
Applied/Available become false during deletion and only Finalized indicates
completion.
In `@helm/adapter3/adapter-task-config.yaml`:
- Around line 149-161: The current logic checks is_deleting &&
adapter.?executionStatus.orValue("") == "success" which hides deletion progress
when executionStatus is non-success (failed/retrying); update the three
expressions (the condition that produces the boolean, the reason, and the
message) to instead check is_deleting && adapter.?executionStatus.orValue("") !=
"" so any executionStatus shows deletion state, then branch on
resources.?resource0.hasValue() to emit "CleanupConfirmed"/"CleanupInProgress"
(or "CleanupFailed" when adapter.?executionStatus.orValue("") != "success") and
include the actual adapter.?executionStatus.orValue("") in the message to
surface failure/retry status rather than always returning "ResourceActive".
🪄 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: 221dae4f-5227-4537-a6c9-fcd89b17cd9a
📒 Files selected for processing (3)
helm/adapter1/adapter-task-config.yamlhelm/adapter2/adapter-task-config.yamlhelm/adapter3/adapter-task-config.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm/adapter2/adapter-task-config.yaml (1)
45-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeletion can still be blocked by the earlier precondition.
Line 45 still hard-requires
readyConditionStatus == "False", so a cluster that isReady=Trueand being deleted will failclusterStatusbefore the newvalidationCheckcan allow cleanup. Foldis_deletinginto that gate, or move the readiness check entirely intovalidationCheck.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/adapter2/adapter-task-config.yaml` around lines 45 - 53, The current precondition under conditions uses readyConditionStatus == "False" which blocks deletion flows because it runs before validationCheck; update the readiness gate so it also allows deletion by including is_deleting in the same check (e.g., change the conditions entry for readyConditionStatus to include an OR with is_deleting) or remove the standalone readiness condition and fold the readiness logic entirely into the validationCheck expression (readyConditionStatus == "False" || is_deleting) so a cluster with Ready=True but is_deleting can pass and proceed to cleanup; adjust the block containing conditions, readyConditionStatus, and validationCheck accordingly.
♻️ Duplicate comments (1)
helm/adapter2/adapter-task-config.yaml (1)
302-334:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Finalizedis deletion-aware, butApplied/Availablecan still stayTrue.This still produces contradictory status during teardown: the new
Finalizedblock reports cleanup progress, while the existingApplied/Availableexpressions at Line 229 onward can continue reflecting the pre-delete state. Those two conditions should also short-circuit onis_deletingand reportFalse/ResourceDeleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/adapter2/adapter-task-config.yaml` around lines 302 - 334, The Applied and Available status expressions must short-circuit on is_deleting to avoid contradictory statuses during teardown: update the existing Applied and Available expression blocks (the same style used in the Finalized block) so they first check !is_deleting and return "True" (or the existing active message) when not deleting, but when is_deleting return status "False" and set reason to "ResourceDeleted" (and message to something like "Resource is being deleted" or "Resource deleted" depending on whether nested resources remain), and reuse adapter.?executionStatus.orValue("") == "failed" to switch to "DeletionFailed"/"Deletion in progress" as appropriate; locate and adjust the Applied and Available expressions that reference resources.?resource0, resources.?namespace0, resources.?configmap0 and adapter.?executionStatus to implement this short-circuiting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@helm/adapter2/adapter-task-config.yaml`:
- Around line 45-53: The current precondition under conditions uses
readyConditionStatus == "False" which blocks deletion flows because it runs
before validationCheck; update the readiness gate so it also allows deletion by
including is_deleting in the same check (e.g., change the conditions entry for
readyConditionStatus to include an OR with is_deleting) or remove the standalone
readiness condition and fold the readiness logic entirely into the
validationCheck expression (readyConditionStatus == "False" || is_deleting) so a
cluster with Ready=True but is_deleting can pass and proceed to cleanup; adjust
the block containing conditions, readyConditionStatus, and validationCheck
accordingly.
---
Duplicate comments:
In `@helm/adapter2/adapter-task-config.yaml`:
- Around line 302-334: The Applied and Available status expressions must
short-circuit on is_deleting to avoid contradictory statuses during teardown:
update the existing Applied and Available expression blocks (the same style used
in the Finalized block) so they first check !is_deleting and return "True" (or
the existing active message) when not deleting, but when is_deleting return
status "False" and set reason to "ResourceDeleted" (and message to something
like "Resource is being deleted" or "Resource deleted" depending on whether
nested resources remain), and reuse adapter.?executionStatus.orValue("") ==
"failed" to switch to "DeletionFailed"/"Deletion in progress" as appropriate;
locate and adjust the Applied and Available expressions that reference
resources.?resource0, resources.?namespace0, resources.?configmap0 and
adapter.?executionStatus to implement this short-circuiting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 91efedfb-ae5d-417f-ada8-d656aaa6350a
📒 Files selected for processing (3)
helm/adapter1/adapter-task-config.yamlhelm/adapter2/adapter-task-config.yamlhelm/adapter3/adapter-task-config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/adapter1/adapter-task-config.yaml
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 `@helm/adapter3/adapter-task-config.yaml`:
- Around line 142-148: The status defaults to "Unknown" while reason/message
still default to "Healthy"/success text; update the expressions for reason
(adapter.?errorReason) and message (adapter.?errorMessage) to return a neutral
in-progress/unknown value when adapter.?executionStatus is neither "success" nor
"failed" so they align with the status. Specifically, change the default
branches used alongside adapter.?executionStatus.orValue("") in the status
expression so that when executionStatus != "success" && != "failed" the reason
becomes e.g. "InProgress" or "Unknown" and the message becomes a matching
neutral text like "Adapter execution in progress or state unknown" instead of
"Healthy" / success text.
🪄 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: 384ca211-ad95-4cd8-9a71-427638496933
📒 Files selected for processing (3)
helm/adapter1/adapter-task-config.yamlhelm/adapter2/adapter-task-config.yamlhelm/adapter3/adapter-task-config.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- helm/adapter1/adapter-task-config.yaml
- helm/adapter2/adapter-task-config.yaml
| adapter.?executionStatus.orValue("") == "success" ? "True" : (adapter.?executionStatus.orValue("") == "failed" ? "False" : "Unknown") | ||
| reason: | ||
| expression: | | ||
| has(resources.resource0.data.nodepoolId) | ||
| ? "ConfigMap data available" | ||
| : "ConfigMap data not yet available" | ||
| adapter.?errorReason.orValue("") != "" ? adapter.?errorReason.orValue("") : "Healthy" | ||
| message: | ||
| expression: | | ||
| toJson(resources.resource0) | ||
| adapter.?errorMessage.orValue("") != "" ? adapter.?errorMessage.orValue("") : "All adapter operations completed successfully" |
There was a problem hiding this comment.
Health status is inconsistent with its own reason/message defaults.
When execution is neither success nor failed, status is "Unknown" but reason/message still default to "Healthy" and a success message. This can mask in-progress/retrying states in monitoring and automation.
Suggested fix
- type: "Health"
status:
expression: |
adapter.?executionStatus.orValue("") == "success" ? "True" : (adapter.?executionStatus.orValue("") == "failed" ? "False" : "Unknown")
reason:
expression: |
- adapter.?errorReason.orValue("") != "" ? adapter.?errorReason.orValue("") : "Healthy"
+ adapter.?executionStatus.orValue("") == "failed"
+ ? adapter.?errorReason.orValue("ExecutionFailed")
+ : (adapter.?executionStatus.orValue("") == "success"
+ ? "Healthy"
+ : "ExecutionInProgress")
message:
expression: |
- adapter.?errorMessage.orValue("") != "" ? adapter.?errorMessage.orValue("") : "All adapter operations completed successfully"
+ adapter.?executionStatus.orValue("") == "failed"
+ ? adapter.?errorMessage.orValue("Adapter execution failed")
+ : (adapter.?executionStatus.orValue("") == "success"
+ ? "All adapter operations completed successfully"
+ : "Adapter execution is still in progress")📝 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.
| adapter.?executionStatus.orValue("") == "success" ? "True" : (adapter.?executionStatus.orValue("") == "failed" ? "False" : "Unknown") | |
| reason: | |
| expression: | | |
| has(resources.resource0.data.nodepoolId) | |
| ? "ConfigMap data available" | |
| : "ConfigMap data not yet available" | |
| adapter.?errorReason.orValue("") != "" ? adapter.?errorReason.orValue("") : "Healthy" | |
| message: | |
| expression: | | |
| toJson(resources.resource0) | |
| adapter.?errorMessage.orValue("") != "" ? adapter.?errorMessage.orValue("") : "All adapter operations completed successfully" | |
| adapter.?executionStatus.orValue("") == "success" ? "True" : (adapter.?executionStatus.orValue("") == "failed" ? "False" : "Unknown") | |
| reason: | |
| expression: | | |
| adapter.?executionStatus.orValue("") == "failed" | |
| ? adapter.?errorReason.orValue("ExecutionFailed") | |
| : (adapter.?executionStatus.orValue("") == "success" | |
| ? "Healthy" | |
| : "ExecutionInProgress") | |
| message: | |
| expression: | | |
| adapter.?executionStatus.orValue("") == "failed" | |
| ? adapter.?errorMessage.orValue("Adapter execution failed") | |
| : (adapter.?executionStatus.orValue("") == "success" | |
| ? "All adapter operations completed successfully" | |
| : "Adapter execution is still in progress") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helm/adapter3/adapter-task-config.yaml` around lines 142 - 148, The status
defaults to "Unknown" while reason/message still default to "Healthy"/success
text; update the expressions for reason (adapter.?errorReason) and message
(adapter.?errorMessage) to return a neutral in-progress/unknown value when
adapter.?executionStatus is neither "success" nor "failed" so they align with
the status. Specifically, change the default branches used alongside
adapter.?executionStatus.orValue("") in the status expression so that when
executionStatus != "success" && != "failed" the reason becomes e.g. "InProgress"
or "Unknown" and the message becomes a matching neutral text like "Adapter
execution in progress or state unknown" instead of "Healthy" / success text.
| status: | ||
| expression: | | ||
| has(resources.resource0.metadata.creationTimestamp) ? "True" : "False" | ||
| is_deleting |
There was a problem hiding this comment.
Being in deletion phase is not a direct indication of Applied not being True
The natural condition is still the resource being found in the discovery phase IMO
It wold be the same for reason and the Available condition values
| !is_deleting | ||
| ? (resources.?resource0.hasValue() && has(resources.resource0.status) && has(resources.resource0.status.conditions) && resources.resource0.status.conditions.filter(c, has(c.type) && c.type == "Applied").size() > 0 ? resources.resource0.status.conditions.filter(c, c.type == "Applied")[0].reason : "ManifestWorkNotDiscovered") | ||
| : (!resources.?resource0.hasValue() | ||
| ? "ResourceDeleted" |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Inconsistency
During deletion, adapter2 reports granular reasons for Applied/Available ("ResourceDeleted" / "DeletionFailed" / "DeletionInProgress"), but adapter1/adapter3 always report just "ResourceDeleted" regardless of actual deletion state. Since the PR title is "standardize adapter conditions," the reasons should be consistent across all three adapters.
If you keep the is_deleting wrapper on Applied/Available (see @rh-amarin's comment above), consider making adapter1/3 match adapter2's granularity — or simplifying adapter2 to match adapter1/3. If you remove the wrapper per Angel's suggestion, this resolves naturally.
3 participants
Summary
Adds deletion lifecycle support to all three adapter task configurations (adapter1, adapter2, adapter3), enabling proper resource cleanup when clusters/nodepools are deleted.
Changes
Preconditions
is_deletingvariable that checks fordeleted_timefield in cluster/nodepool statusvalidationCheckexpression to trigger adapter execution when resource is being deleted (is_deleting || existing_conditions)Resource Lifecycle
lifecycle.deleteconfiguration with propagation policies:BackgroundpropagationForegroundpropagation to ensure nested resources deleted firstBackgroundpropagationis_deletingexpression evaluates to trueStatus Conditions
status: Falsewithreason: ResourceDeletedduring deletionstatus: Falsewithreason: ResourceDeletedduring deletionstatus: Truewithreason: CleanupConfirmedwhen all resources deletedstatus: Falsewithreason: CleanupInProgresswhile deletion ongoingWhy This Matters
Without proper deletion lifecycle handling:
This change ensures:
Test Plan
is_deleting)Summary by CodeRabbit