fix(operator): Enable request timeout after "300s"#881
Conversation
8R0WNI3
added
kind/bugfix
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTwo Kubernetes watch streams now use a 300-second timeout, and the operator’s top-level loop now owns watch restart handling by tracking ChangesWatch Timeout and Restart Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MainLoop
participant Reconcile
participant KubernetesWatch
MainLoop->>Reconcile: call reconcile(resource_version)
Reconcile->>KubernetesWatch: stream(..., timeout_seconds=300)
KubernetesWatch-->>Reconcile: watch events or timeout
Reconcile-->>MainLoop: return / raise watch-related error
MainLoop->>MainLoop: reset resource_version on GONE or ProtocolError
MainLoop->>Reconcile: retry with updated resource_version
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/odg_operator/__main__.py (1)
571-596: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate
resource_versionacross watch restarts.reconcile()updates it locally, but the outerwhile Truecallsreconcile()again with the default'', so each 300s timeout restarts the watch from the initial state instead of resuming from the last seen event. Return the latest RV or thread it through the caller.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odg_operator/__main__.py` around lines 571 - 596, The watch restart in reconcile() is losing the latest resource_version because the outer caller starts each new reconcile() invocation with the default empty value, so the stream resumes from the beginning instead of the last seen event. Update the control flow around reconcile() and the watch loop to carry the latest resource_version forward across timeout restarts, using the reconcile() function and its caller to persist and pass the returned RV into the next stream invocation.
🧹 Nitpick comments (1)
src/odg_operator/__main__.py (1)
595-595: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared
300timeout to a named constant.Same literal
300is duplicated insrc/backlog_controller.pyand here for the same underlying reason (avoid stale idle watch connections). A shared, named constant (e.g.WATCH_TIMEOUT_SECONDS) would document intent and avoid divergence if the value needs tuning later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odg_operator/__main__.py` at line 595, The watch timeout literal is duplicated and should be centralized to avoid drift. Extract the shared 300-second value into a named constant such as WATCH_TIMEOUT_SECONDS in the shared module or a common constants location, then update both __main__. and backlog_controller to use that constant for the idle watch connection timeout. Keep the existing timeout behavior unchanged while making the intent explicit through the shared symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/odg_operator/__main__.py`:
- Around line 571-596: The watch restart in reconcile() is losing the latest
resource_version because the outer caller starts each new reconcile() invocation
with the default empty value, so the stream resumes from the beginning instead
of the last seen event. Update the control flow around reconcile() and the watch
loop to carry the latest resource_version forward across timeout restarts, using
the reconcile() function and its caller to persist and pass the returned RV into
the next stream invocation.
---
Nitpick comments:
In `@src/odg_operator/__main__.py`:
- Line 595: The watch timeout literal is duplicated and should be centralized to
avoid drift. Extract the shared 300-second value into a named constant such as
WATCH_TIMEOUT_SECONDS in the shared module or a common constants location, then
update both __main__. and backlog_controller to use that constant for the idle
watch connection timeout. Keep the existing timeout behavior unchanged while
making the intent explicit through the shared symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac2f9759-263b-4cd0-95ea-567d207cf2cd
📒 Files selected for processing (2)
src/backlog_controller.pysrc/odg_operator/__main__.py
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)
src/odg_operator/__main__.py (1)
580-581: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
resource_versionupdate insidereconcile()never reaches the caller — watch relists on every idle timeout.
reconcile()mutates its localresource_versionparameter (Line 646) but never returns it. Since strings are passed by value, the__main__loop'sresource_version(Lines 977-989) is only ever updated inside theexceptbranches (GONE/ProtocolError/MaxRetryError). On the expected/common path — a clean 300s timeout with no exception —reconcile()simply returnsNone, and the next iteration re-callsreconcile()with the staleresource_version. This means every idle timeout now causes a full relist/reprocessing of all ODG resources, which is exactly what the PR states it intends to avoid ("the last processedresource_versionis preserved so already processed events are not reprocessed").🐛 Proposed fix: return and capture the updated resource_version
def reconcile( extension_definitions: collections.abc.Iterable[odgm.ExtensionDefinition], required_extensions: list[str], cluster_identity: str, component_descriptor_lookup: ocm.ComponentDescriptorLookup, kubernetes_api: k8s.util.KubernetesApi, oci_client: oci.client.Client, group: str = odgm.ODGMeta.group, plural: str = odgm.ODGMeta.plural, resource_version: str = '', -): +) -> str: """ watches for events of ODG custom-resource creates, updates and deletes ODG installations using managed-resources """ ... logger.error(e) + + return resource_versionwhile True: try: - reconcile( + resource_version = reconcile( extension_definitions=extension_definitions, required_extensions=odg_operator_cfg.required_extension_names, cluster_identity=odg_operator_cfg.cluster_identity, component_descriptor_lookup=component_descriptor_lookup, oci_client=oci_client, kubernetes_api=kubernetes_api, resource_version=resource_version, )Also applies to: 646-646, 806-806, 977-989
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odg_operator/__main__.py` around lines 580 - 581, The updated resource_version is being lost because reconcile() only mutates its local parameter and does not pass the final value back to the caller. Update reconcile() to return the latest resource_version on the normal timeout path, then capture that return value in the __main__ loop where reconcile() is invoked so the next iteration reuses it instead of relisting. Keep the existing exception handling for GONE, ProtocolError, and MaxRetryError, but ensure the common no-error path preserves the last processed resource_version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/odg_operator/__main__.py`:
- Around line 580-581: The updated resource_version is being lost because
reconcile() only mutates its local parameter and does not pass the final value
back to the caller. Update reconcile() to return the latest resource_version on
the normal timeout path, then capture that return value in the __main__ loop
where reconcile() is invoked so the next iteration reuses it instead of
relisting. Keep the existing exception handling for GONE, ProtocolError, and
MaxRetryError, but ensure the common no-error path preserves the last processed
resource_version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdb714eb-963c-4169-9aa5-d9e34335844c
📒 Files selected for processing (2)
src/backlog_controller.pysrc/odg_operator/__main__.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backlog_controller.py
The timeout is necessary to prevent keeping an unnoticed stale connection which blocks further events from being processed. If the idle timeout is reached, the watch stream breaks (without error) and will be re-instantiated by the outer `while True` loop. Already processed events will not be processed again because the last processed `resource_version` is kept. Signed-off-by: Jonas Brand (8R0WNI3) <j.brand@sap.com>
The handled exceptions here are all related to the Kubernetes API watch, however, previously they were handled within the watch loop itself (which is not working). Therefore, moved the error handling and added an extra exception to align behaviour with the backlog-controller. Signed-off-by: Jonas Brand (8R0WNI3) <j.brand@sap.com>
zkdev
left a comment
zkdev
left a comment
There was a problem hiding this comment.
wow, I am surprised we even have the handling for the backlog controller in place already
/lgtm
2 participants
What this PR does / why we need it:
The timeout is necessary to prevent keeping an unnoticed stale connection which blocks further events from being processed. If the idle timeout is reached, the watch stream breaks (without error) and will be re-instantiated by the outer
while Trueloop. Already processed events will not be processed again because the last processedresource_versionis kept.Which issue(s) this PR fixes:
Fixes open-component-model/open-delivery-gear#147
Special notes for your reviewer:
Release note: