HYPERFLEET-707 - doc: update adapter configs to add time-based stability precondition#20
Open
86254860 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
Open
HYPERFLEET-707 - doc: update adapter configs to add time-based stability precondition#2086254860 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
86254860 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
Conversation
…dition for resource deletion recovery
WalkthroughTwo Helm adapter task configuration files were modified to replace string-based ready status checks with boolean and TTL-based preconditions. Each file renames the readyConditionStatus precondition (to clusterNotReady and nodepoolNotReady), removes the previous structured conditions block, adds a TTL precondition (clusterReadyTTL / nodepoolReadyTTL) that computes seconds since the Ready condition's lastTransitionTime (defaulting to now()), and updates validationCheck to require the not-ready boolean OR the TTL expiry (300s). Ready evaluation now treats non-True as not-ready and defaults to true when data is insufficient. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 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 64-67: Update the inline comment above the precondition expression
to reflect the actual comparison used in the expression: change the text to
indicate ">= TTL_EXPIRY_THRESHOLD seconds" (i.e., passing when stable for
greater than or equal to the threshold) so the comment matches the logic using
readyCondition, readyCondition.last_transition_time and TTL_EXPIRY_THRESHOLD in
the expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fa61e23-328c-4483-9b6c-bb7dd38ec389
📒 Files selected for processing (2)
helm/adapter1/adapter-task-config.yamlhelm/adapter3/adapter-task-config.yaml
Comment on lines
+64
to
+67
| # Precondition passes if cluster is NOT Ready OR if cluster is Ready and stable for >TTL_EXPIRY_THRESHOLD seconds since last transition (enables self-healing) | ||
| expression: | | ||
| readyConditionStatus == "False" | ||
| readyCondition == null || readyCondition.status != "True" || | ||
| (timestamp(currentTime) - timestamp(readyCondition.last_transition_time)).getSeconds() >= TTL_EXPIRY_THRESHOLD |
There was a problem hiding this comment.
Minor comment/code mismatch.
The comment states >TTL_EXPIRY_THRESHOLD seconds but the expression uses >=. The code behavior (passing at exactly the threshold) is likely the intended behavior, so consider updating the comment to >=TTL_EXPIRY_THRESHOLD for accuracy.
🤖 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 64 - 67, Update the
inline comment above the precondition expression to reflect the actual
comparison used in the expression: change the text to indicate ">=
TTL_EXPIRY_THRESHOLD seconds" (i.e., passing when stable for greater than or
equal to the threshold) so the comment matches the logic using readyCondition,
readyCondition.last_transition_time and TTL_EXPIRY_THRESHOLD in the expression.
…gs for more readable
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 `@helm/adapter1/adapter-task-config.yaml`:
- Around line 42-53: clusterNotReady and clusterReadyTTL assume
status.conditions and last_transition_time exist; make the CEL expressions
null-safe by first checking status.conditions != null and that the filtered list
has >0 elements before indexing, and for clusterReadyTTL also check the
existence of last_transition_time and fall back to now() if missing. Update the
clusterNotReady expression used in the "clusterNotReady" rule to:
status.conditions != null && status.conditions.filter(c, c.type ==
"Ready").size() > 0 ? status.conditions.filter(c, c.type == "Ready")[0].status
!= "True" : true; and update "clusterReadyTTL" to compute the timestamp using a
guarded last_transition_time: (timestamp(now()) - timestamp(status.conditions !=
null && status.conditions.filter(c, c.type == "Ready").size() > 0 &&
status.conditions.filter(c, c.type == "Ready")[0].last_transition_time != null ?
status.conditions.filter(c, c.type == "Ready")[0].last_transition_time :
now())).getSeconds() >= 300 so accesses in clusterReadyTTL and clusterNotReady
(status.conditions and last_transition_time) are protected.
In `@helm/adapter3/adapter-task-config.yaml`:
- Around line 47-58: The CEL expressions for nodepoolNotReady and
nodepoolReadyTTL assume status.conditions and last_transition_time exist; make
them defensive by first checking status and conditions are present and non-empty
and that last_transition_time is present before indexing or calling timestamp.
Update the nodepoolNotReady expression to use a guard like status != null &&
status.conditions != null && status.conditions.size() > 0 ?
status.conditions.filter(c, c.type == "Ready")[0].status != "True" : true, and
update nodepoolReadyTTL to compute the reference time only when
status/conditions and last_transition_time exist (fallback to now() otherwise),
e.g. check status != null && status.conditions != null &&
status.conditions.size() > 0 && status.conditions.filter(c, c.type ==
"Ready")[0].last_transition_time != null ? timestamp(status.conditions.filter(c,
c.type == "Ready")[0].last_transition_time) : timestamp(now()), then compare
getSeconds() >= 300; apply these guards in the nodepoolNotReady and
nodepoolReadyTTL expressions to avoid runtime errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6898181-f2c9-48b0-9b31-0822173bc160
📒 Files selected for processing (2)
helm/adapter1/adapter-task-config.yamlhelm/adapter3/adapter-task-config.yaml
Comment on lines
+42
to
+53
| - name: "clusterNotReady" | ||
| expression: | | ||
| status.conditions.filter(c, c.type == "Ready").size() > 0 | ||
| ? status.conditions.filter(c, c.type == "Ready")[0].status | ||
| : "False" | ||
| # Structured conditions with valid operators | ||
| conditions: | ||
| - field: "readyConditionStatus" | ||
| operator: "equals" | ||
| value: "False" | ||
| ? status.conditions.filter(c, c.type == "Ready")[0].status != "True" | ||
| : true | ||
| - name: "clusterReadyTTL" | ||
| expression: | | ||
| (timestamp(now()) - timestamp( | ||
| status.conditions.filter(c, c.type == "Ready").size() > 0 | ||
| ? status.conditions.filter(c, c.type == "Ready")[0].last_transition_time | ||
| : now() | ||
| )).getSeconds() >= 300 |
There was a problem hiding this comment.
Guard status.conditions/last_transition_time access to avoid brittle preconditions.
Line 44 and Line 50 assume status.conditions is always present, and Line 51 assumes last_transition_time exists. Partial API responses can make these expressions fail at runtime instead of cleanly falling back.
Suggested hardening
- name: "clusterNotReady"
expression: |
- status.conditions.filter(c, c.type == "Ready").size() > 0
- ? status.conditions.filter(c, c.type == "Ready")[0].status != "True"
+ status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0
+ ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].status != "True"
: true
- name: "clusterReadyTTL"
expression: |
(timestamp(now()) - timestamp(
- status.conditions.filter(c, c.type == "Ready").size() > 0
- ? status.conditions.filter(c, c.type == "Ready")[0].last_transition_time
+ status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0 &&
+ has(status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time)
+ ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time
: now()
)).getSeconds() >= 300As per coding guidelines, **: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helm/adapter1/adapter-task-config.yaml` around lines 42 - 53, clusterNotReady
and clusterReadyTTL assume status.conditions and last_transition_time exist;
make the CEL expressions null-safe by first checking status.conditions != null
and that the filtered list has >0 elements before indexing, and for
clusterReadyTTL also check the existence of last_transition_time and fall back
to now() if missing. Update the clusterNotReady expression used in the
"clusterNotReady" rule to: status.conditions != null &&
status.conditions.filter(c, c.type == "Ready").size() > 0 ?
status.conditions.filter(c, c.type == "Ready")[0].status != "True" : true; and
update "clusterReadyTTL" to compute the timestamp using a guarded
last_transition_time: (timestamp(now()) - timestamp(status.conditions != null &&
status.conditions.filter(c, c.type == "Ready").size() > 0 &&
status.conditions.filter(c, c.type == "Ready")[0].last_transition_time != null ?
status.conditions.filter(c, c.type == "Ready")[0].last_transition_time :
now())).getSeconds() >= 300 so accesses in clusterReadyTTL and clusterNotReady
(status.conditions and last_transition_time) are protected.
Comment on lines
+47
to
+58
| - name: "nodepoolNotReady" | ||
| expression: | | ||
| status.conditions.filter(c, c.type == "Ready").size() > 0 | ||
| ? status.conditions.filter(c, c.type == "Ready")[0].status | ||
| : "False" | ||
| # Structured conditions with valid operators | ||
| conditions: | ||
| - field: "readyConditionStatus" | ||
| operator: "equals" | ||
| value: "False" | ||
| ? status.conditions.filter(c, c.type == "Ready")[0].status != "True" | ||
| : true | ||
| - name: "nodepoolReadyTTL" | ||
| expression: | | ||
| (timestamp(now()) - timestamp( | ||
| status.conditions.filter(c, c.type == "Ready").size() > 0 | ||
| ? status.conditions.filter(c, c.type == "Ready")[0].last_transition_time | ||
| : now() | ||
| )).getSeconds() >= 300 |
There was a problem hiding this comment.
Handle missing readiness fields defensively to prevent CEL runtime failures.
Line 49 and Line 55 assume status.conditions exists, and Line 56 assumes last_transition_time exists. If either is missing in API payloads, precondition evaluation can error and block execution instead of falling back.
Suggested hardening
- name: "nodepoolNotReady"
expression: |
- status.conditions.filter(c, c.type == "Ready").size() > 0
- ? status.conditions.filter(c, c.type == "Ready")[0].status != "True"
+ status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0
+ ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].status != "True"
: true
- name: "nodepoolReadyTTL"
expression: |
(timestamp(now()) - timestamp(
- status.conditions.filter(c, c.type == "Ready").size() > 0
- ? status.conditions.filter(c, c.type == "Ready")[0].last_transition_time
+ status.?conditions.orValue([]).filter(c, c.type == "Ready").size() > 0 &&
+ has(status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time)
+ ? status.?conditions.orValue([]).filter(c, c.type == "Ready")[0].last_transition_time
: now()
)).getSeconds() >= 300As per coding guidelines, **: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 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 47 - 58, The CEL
expressions for nodepoolNotReady and nodepoolReadyTTL assume status.conditions
and last_transition_time exist; make them defensive by first checking status and
conditions are present and non-empty and that last_transition_time is present
before indexing or calling timestamp. Update the nodepoolNotReady expression to
use a guard like status != null && status.conditions != null &&
status.conditions.size() > 0 ? status.conditions.filter(c, c.type ==
"Ready")[0].status != "True" : true, and update nodepoolReadyTTL to compute the
reference time only when status/conditions and last_transition_time exist
(fallback to now() otherwise), e.g. check status != null && status.conditions !=
null && status.conditions.size() > 0 && status.conditions.filter(c, c.type ==
"Ready")[0].last_transition_time != null ? timestamp(status.conditions.filter(c,
c.type == "Ready")[0].last_transition_time) : timestamp(now()), then compare
getSeconds() >= 300; apply these guards in the nodepoolNotReady and
nodepoolReadyTTL expressions to avoid runtime errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Configuration Updates
Improvements