Extract owner idle actions into pkg/idler from member operator - #540
Conversation
Share the full kind matrix and on-demand IdleFromPod helper so member-operator and MCP can idle owners the same way without duplicating scale/stop/patch/delete logic. Co-authored-by: Cursor <cursoragent@cursor.com>
Allow the member-operator timeout orchestration path to reuse the shared owner walk and ownership-chain logging helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
WalkthroughAdds a client-backed idler that dispatches kind-specific actions, processes pod owner chains, supports Kubernetes and custom resources, and includes tests for scaling, deletion, stopping, timeout cleanup, and error handling. ChangesResource idling workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pod
participant OwnerFetcher
participant Idler
participant KubernetesAPI
Pod->>OwnerFetcher: resolve owner chain
OwnerFetcher-->>Idler: return owners with GVRs
Idler->>KubernetesAPI: execute kind-specific idle action
KubernetesAPI-->>Idler: return action result or error
Idler-->>Pod: return result and errors
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/idler/idler_test.go (1)
339-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
creationTimestampmap entry.Lines 343-345 set a string timestamp that is immediately superseded by the
Set*calls below; keeping both is confusing.♻️ Cleanup
obj := &unstructured.Unstructured{Object: map[string]any{ "apiVersion": "serving.kserve.io/v1beta1", "kind": "InferenceService", - "metadata": map[string]any{ - "name": name, - "namespace": ns, - "creationTimestamp": metav1.NewTime(time.Now().Add(-age)).Format(time.RFC3339), - }, + "metadata": map[string]any{}, }} - // SetCreationTimestamp is more reliable than map string for fake client obj.SetName(name)🤖 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 `@pkg/idler/idler_test.go` around lines 339 - 353, Remove the redundant creationTimestamp entry from the Object map in the test fixture, keeping the SetCreationTimestamp call as the sole timestamp initialization while leaving the other metadata and Set* calls unchanged.pkg/idler/actions.go (2)
55-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
idleAAPandidleClaware the same function with a different field name.Consider one helper taking the idle field name (
idle_aap/idle) and a label for logging; the two exported behaviors stay identical.🤖 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 `@pkg/idler/actions.go` around lines 55 - 106, Refactor the duplicated logic in idleAAP and idleClaw into a shared helper that accepts the idle field name and logging label as parameters. Have each method delegate to that helper with its existing values ("idle_aap" for AAP and "idle" for Claw), preserving their current parsing, logging, patching, and error behavior.
29-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a direct map lookup instead of scanning values.
The map is keyed by GVK; iterating values and comparing GVR strings loses the kind association and is needlessly indirect. The owner kind is already known at dispatch time.
♻️ Suggested simplification
patch := []byte(`{"spec":{"replicas":0}}`) - for _, groupVersionResource := range SupportedScaleResources { - if groupVersionResource.String() == objectWithGVR.GVR.String() { - logger.Info("Scaling controller owner to zero using the scale subresource") - _, err := i.scalesClient.Scales(object.GetNamespace()).Patch(ctx, *objectWithGVR.GVR, object.GetName(), types.MergePatchType, patch, metav1.PatchOptions{}) - if err != nil { - return err - } - logger.Info("Controller owner scaled to zero using the scale subresource") - return nil - } - } + if _, ok := SupportedScaleResources[object.GetObjectKind().GroupVersionKind()]; ok { + logger.Info("Scaling controller owner to zero using the scale subresource") + if _, err := i.scalesClient.Scales(object.GetNamespace()).Patch(ctx, *objectWithGVR.GVR, object.GetName(), types.MergePatchType, patch, metav1.PatchOptions{}); err != nil { + return err + } + logger.Info("Controller owner scaled to zero using the scale subresource") + return nil + }🤖 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 `@pkg/idler/actions.go` around lines 29 - 40, The scaling logic around SupportedScaleResources should use a direct lookup keyed by the owner’s known GVK instead of iterating map values and comparing GVR strings. Update the dispatch condition to retrieve the corresponding scale resource directly, then pass that resource to scalesClient.Scales while preserving the existing patch, logging, and return behavior.
🤖 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.
Inline comments:
In `@pkg/idler/actions.go`:
- Around line 186-212: Update the InferenceService cleanup loop in the relevant
idler action to delete only resources associated with the target ServingRuntime,
using the established runtime-association field or label rather than owner
references. Preserve the existing age cutoff and deletion-error handling, and
add a test covering multiple runtimes to verify unrelated InferenceServices
remain untouched.
In `@pkg/idler/idler.go`:
- Around line 88-119: Update IdleFromPod’s owner-fetch handling around GetOwners
and the ownerChain loop so a GetOwners error is returned when no owner was
successfully attempted, instead of returning an empty result with nil error.
Preserve the existing behavior for partial chains where at least one owner is
processed, including any accumulated IdleOwner errors.
---
Nitpick comments:
In `@pkg/idler/actions.go`:
- Around line 55-106: Refactor the duplicated logic in idleAAP and idleClaw into
a shared helper that accepts the idle field name and logging label as
parameters. Have each method delegate to that helper with its existing values
("idle_aap" for AAP and "idle" for Claw), preserving their current parsing,
logging, patching, and error behavior.
- Around line 29-40: The scaling logic around SupportedScaleResources should use
a direct lookup keyed by the owner’s known GVK instead of iterating map values
and comparing GVR strings. Update the dispatch condition to retrieve the
corresponding scale resource directly, then pass that resource to
scalesClient.Scales while preserving the existing patch, logging, and return
behavior.
In `@pkg/idler/idler_test.go`:
- Around line 339-353: Remove the redundant creationTimestamp entry from the
Object map in the test fixture, keeping the SetCreationTimestamp call as the
sole timestamp initialization while leaving the other metadata and Set* calls
unchanged.
🪄 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: bab39f60-7ffd-417f-9ceb-4a1b4bcdbeda
📒 Files selected for processing (3)
pkg/idler/actions.gopkg/idler/idler.gopkg/idler/idler_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test
- GitHub Check: GolangCI Lint
- GitHub Check: Verify Dependencies
🧰 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:
pkg/idler/actions.gopkg/idler/idler_test.gopkg/idler/idler.go
🪛 GitHub Check: SonarCloud Code Analysis
pkg/idler/actions.go
[failure] 20-20: Define a constant instead of duplicating this literal "camel.apache.org" 4 times.
🔇 Additional comments (2)
pkg/idler/idler.go (1)
123-139: LGTM!pkg/idler/idler_test.go (1)
306-320: LGTM!Also applies to: 384-450, 484-499
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/idler/idler_test.go (1)
509-513: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a start time that is clearly below the 110% gate.
-3960 * time.Secondequals exactly 110% of 3600s, sopodRunningLongerThan(pod, 3600, 1.10)is also true here. The subtest passes only because the ReplicaSet is not deleting andattemptedstays true. Use a value between 3780s and 3960s, for example 3900s, to test the 105% gate in isolation.🤖 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 `@pkg/idler/idler_test.go` around lines 509 - 513, Update the “SecondOwnerAfterTimeout over 105% idles two owners” test’s start time to a duration strictly between 105% and 110% of 3600 seconds, such as 3900 seconds, so it exercises the 105% gate without also satisfying the 110% condition.pkg/idler/idler.go (1)
128-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the second-owner policy logic to satisfy the SonarCloud gate.
SonarCloud reports cognitive complexity 22 against the allowed 15 for
IdleFromPod, so the quality gate fails. The owner loop mixes three concerns: deleting-owner handling, first/second owner selection, and the timeout gates. Move theSecondOwnerAfterTimeoutdecision into a small helper, for exampleshouldTryNextOwner(pod, opts, err) bool, and keep the loop body to selection and error aggregation. The 1.05 and 1.10 ratios are also good candidates for named constants.🤖 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 `@pkg/idler/idler.go` around lines 128 - 191, Reduce cognitive complexity in IdleFromPod by extracting the SecondOwnerAfterTimeout timeout decision into a helper such as shouldTryNextOwner, preserving the existing 1.05 success gate and 1.10 pod-delete gate behavior. Simplify the owner loop to focus on deleting-owner handling, owner selection, and error aggregation, and replace the timeout ratios with named constants.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@pkg/idler/idler_test.go`:
- Around line 509-513: Update the “SecondOwnerAfterTimeout over 105% idles two
owners” test’s start time to a duration strictly between 105% and 110% of 3600
seconds, such as 3900 seconds, so it exercises the 105% gate without also
satisfying the 110% condition.
In `@pkg/idler/idler.go`:
- Around line 128-191: Reduce cognitive complexity in IdleFromPod by extracting
the SecondOwnerAfterTimeout timeout decision into a helper such as
shouldTryNextOwner, preserving the existing 1.05 success gate and 1.10
pod-delete gate behavior. Simplify the owner loop to focus on deleting-owner
handling, owner selection, and error aggregation, and replace the timeout ratios
with named constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 1b49d430-63d1-4efe-a44b-dffca580e44e
📒 Files selected for processing (2)
pkg/idler/idler.gopkg/idler/idler_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Verify Dependencies
🧰 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:
pkg/idler/idler.gopkg/idler/idler_test.go
🪛 GitHub Check: SonarCloud Code Analysis
pkg/idler/idler.go
[failure] 128-128: Refactor this method to reduce its Cognitive Complexity from 22 to the 15 allowed.
🔇 Additional comments (2)
pkg/idler/idler.go (1)
23-38: LGTM!Also applies to: 48-78, 98-124, 193-207
pkg/idler/idler_test.go (1)
376-394: LGTM!Also applies to: 418-484, 486-574, 576-696
|
@MatousJobanek more code is extracted from the member operator now with additional configuration options. Please take a look. |
| if topOwnerKind == "" && fetchErr != nil { | ||
| return Result{}, fetchErr | ||
| } |
There was a problem hiding this comment.
this is slightly different compared to the previous logic - if nothing was idled and there was an error, the original code tried to delete the pod, which wouldn't do in this particular case if I'm not mistaken
There was a problem hiding this comment.
Right. Previously (current logic) if we failed to fetch the owner of the pod due to some errors then we would delete the pod as a standalone pod. It has some pros and cons. One con is that it could hide some bugs in our code or if it was a temp error in the cluster then we would basically reset the pod idling timeout. With the new logic we always retry in case of an error while fetching the owner(s). I would keep the new logic. But if we see a legit case when an error should be treated as a standalone pod then we can always restore the old logic. WDYT?
|
2 participants
pkg/idleras the shared library for idling Sandbox workload owners (scale/stop/patch/delete by kind).IdleOwnerfor a single known owner and on-demandIdleFromPod(up to two known owners, no timeout policy, no user notifications).Options.TimeoutSeconds(no Idler CR dependency in common).Related PR: codeready-toolchain/member-operator#761
Summary by CodeRabbit
Summary by CodeRabbit