chore!: remove interval field from resource CR#2091
Closed
frewilhelm wants to merge 6 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Resource
participant Status
participant Workqueue
rect rgba(200,200,255,0.5)
Note over Controller: Old flow (before this PR)
Controller ->> Resource: Reconcile()
Controller ->> Resource: GetRequeueAfter()
Resource -->> Controller: duration
Controller ->> Status: MarkReady(...)
Controller ->> Workqueue: RequeueAfter(duration)
end
rect rgba(200,255,200,0.5)
Note over Controller: New flow (after this PR)
Controller ->> Resource: Reconcile()
Controller ->> Status: MarkReady(...)
Controller --x Workqueue: (no interval-based requeue)
Note over Workqueue: Subsequent reconciles rely on events/predicates
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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 |
piotrjanik
reviewed
Mar 27, 2026
piotrjanik
left a comment
Contributor
There was a problem hiding this comment.
LGTM, but isn't that a breaking change?
a22d401 to
590c923
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kubernetes/controller/internal/controller/deployer/deployer_controller.go (1)
373-377:⚠️ Potential issue | 🟠 MajorConfig and credential rotations lose their last enqueue path.
After this returns without requeueing, the controller is only driven by
Deployer,Resource, worker-pool, and dynamic resource events. It still reloads effective OCM config duringDownloadResourceWithOCM(), but none of the referenced Secret/ConfigMap/OCM config objects are watched here, so config or credential changes can leave the applied state stale until some unrelated event happens. Please add watches for those refs before removing the fallback requeue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/controller/internal/controller/deployer/deployer_controller.go` around lines 373 - 377, The controller currently returns without requeueing after status.MarkReady(...) which removes the fall-back requeue and leaves config/credential rotations un-watched; update the controller setup to add watches for any Secret, ConfigMap and OCM config objects referenced by resources so changes trigger reconciles before removing the fallback requeue—specifically ensure the reconciler that calls DownloadResourceWithOCM() has Informers/Watch registrations for the Secret and ConfigMap refs (and the OCM config resource) and tie those watches to enqueue the corresponding Deployer/Resource reconcile (or use EnqueueRequestForOwner) so rotations of credentials or config will requeue the affected resource prior to deleting the existing requeue behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kubernetes/controller/internal/controller/resource/resource_controller.go`:
- Around line 453-455: The controller removed periodic requeues but
SetupWithManager() still only watches Resource, Component, Deployer deletions
and the worker-pool source, so rotations/changes to referenced config/credential
objects (Secrets, ConfigMaps, OCM objects) won't enqueue Resources and Deployers
remain stale; update SetupWithManager() to add Watches (or field/index-based
event handlers) for the Resource's config/credential references and the
referenced Secret / ConfigMap / OCM types and create indices (e.g., via
mgr.GetFieldIndexer().IndexField) so changes to those objects map back to
affected Resource objects and enqueue reconcile requests for Resource (and
thereby Deployer) reconciliations, ensuring functions like SetupWithManager(),
and the controller's event handlers and indexing code for
Resource/Component/Deployer reflect these extra watches/indices.
- Around line 178-183: The deferred call currently hardcodes
status.UpdateBeforePatch(..., 0, err) which hides real retry intent; introduce a
local variable (e.g., requeueInt := 0) captured by the defer and set requeueInt
= 1 (or the appropriate positive value) in any retry branches (such as the
Requeue: true after finalizer installation and the "effective ocm config
changed" retry branches) before returning so status.UpdateBeforePatch(resource,
r.EventRecorder, requeueInt, err) reflects the real intent; alternatively, if
you prefer to suppress success events on retry, set a flag (e.g.,
suppressSuccessEvent) and pass that information to UpdateBeforePatch, but the
simplest fix is to thread and update a captured requeue integer and use it in
the deferred status.UpdateBeforePatch call.
---
Outside diff comments:
In `@kubernetes/controller/internal/controller/deployer/deployer_controller.go`:
- Around line 373-377: The controller currently returns without requeueing after
status.MarkReady(...) which removes the fall-back requeue and leaves
config/credential rotations un-watched; update the controller setup to add
watches for any Secret, ConfigMap and OCM config objects referenced by resources
so changes trigger reconciles before removing the fallback requeue—specifically
ensure the reconciler that calls DownloadResourceWithOCM() has Informers/Watch
registrations for the Secret and ConfigMap refs (and the OCM config resource)
and tie those watches to enqueue the corresponding Deployer/Resource reconcile
(or use EnqueueRequestForOwner) so rotations of credentials or config will
requeue the affected resource prior to deleting the existing requeue behavior.
🪄 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: Pro
Run ID: 29afa583-a1f1-452b-acd5-df9aa156c179
📒 Files selected for processing (31)
kubernetes/controller/api/v1alpha1/resource_types.gokubernetes/controller/api/v1alpha1/zz_generated.deepcopy.gokubernetes/controller/chart/templates/crd/resources.delivery.ocm.software.yamlkubernetes/controller/config/crd/bases/delivery.ocm.software_resources.yamlkubernetes/controller/config/samples/resource.delivery.ocm.software_sample.yamlkubernetes/controller/examples/applyset-pruning/bootstrap.yamlkubernetes/controller/examples/helm-configuration-localization/bootstrap.yamlkubernetes/controller/examples/helm-configuration-localization/rgd.yamlkubernetes/controller/examples/helm-nested-signed/bootstrap.yamlkubernetes/controller/examples/helm-nested-signed/rgd.yamlkubernetes/controller/examples/helm-nested/bootstrap.yamlkubernetes/controller/examples/helm-nested/rgd.yamlkubernetes/controller/examples/helm-signing/bootstrap.yamlkubernetes/controller/examples/helm-signing/rgd.yamlkubernetes/controller/examples/helm-simple-nested-status/bootstrap.yamlkubernetes/controller/examples/helm-simple-nested-status/rgd.yamlkubernetes/controller/examples/helm-simple/bootstrap.yamlkubernetes/controller/examples/helm-simple/rgd.yamlkubernetes/controller/examples/k8s-manifest-simple/bootstrap.yamlkubernetes/controller/examples/kustomize-configuration-localization/bootstrap.yamlkubernetes/controller/examples/kustomize-configuration-localization/rgd.yamlkubernetes/controller/examples/kustomize-simple/bootstrap.yamlkubernetes/controller/examples/kustomize-simple/rgd.yamlkubernetes/controller/internal/controller/deployer/deployer_controller.gokubernetes/controller/internal/controller/deployer/deployer_controller_test.gokubernetes/controller/internal/controller/resource/resource_controller.gokubernetes/controller/internal/controller/resource/resource_controller_test.gokubernetes/controller/test/e2e/testdata/basic-auth/bootstrap.yamlkubernetes/controller/test/e2e/testdata/basic-auth/rgd.yamlkubernetes/controller/test/e2e/testdata/docker-config-json/bootstrap.yamlkubernetes/controller/test/e2e/testdata/docker-config-json/rgd.yaml
💤 Files with no reviewable changes (28)
- kubernetes/controller/examples/helm-nested-signed/rgd.yaml
- kubernetes/controller/examples/helm-signing/bootstrap.yaml
- kubernetes/controller/examples/helm-simple-nested-status/bootstrap.yaml
- kubernetes/controller/examples/helm-nested-signed/bootstrap.yaml
- kubernetes/controller/examples/helm-simple/bootstrap.yaml
- kubernetes/controller/examples/helm-signing/rgd.yaml
- kubernetes/controller/examples/helm-configuration-localization/bootstrap.yaml
- kubernetes/controller/config/crd/bases/delivery.ocm.software_resources.yaml
- kubernetes/controller/examples/helm-nested/bootstrap.yaml
- kubernetes/controller/examples/applyset-pruning/bootstrap.yaml
- kubernetes/controller/examples/k8s-manifest-simple/bootstrap.yaml
- kubernetes/controller/examples/helm-nested/rgd.yaml
- kubernetes/controller/examples/helm-simple/rgd.yaml
- kubernetes/controller/test/e2e/testdata/docker-config-json/bootstrap.yaml
- kubernetes/controller/test/e2e/testdata/basic-auth/bootstrap.yaml
- kubernetes/controller/examples/kustomize-configuration-localization/rgd.yaml
- kubernetes/controller/config/samples/resource.delivery.ocm.software_sample.yaml
- kubernetes/controller/examples/kustomize-simple/bootstrap.yaml
- kubernetes/controller/internal/controller/deployer/deployer_controller_test.go
- kubernetes/controller/test/e2e/testdata/docker-config-json/rgd.yaml
- kubernetes/controller/examples/kustomize-configuration-localization/bootstrap.yaml
- kubernetes/controller/examples/kustomize-simple/rgd.yaml
- kubernetes/controller/api/v1alpha1/resource_types.go
- kubernetes/controller/examples/helm-simple-nested-status/rgd.yaml
- kubernetes/controller/internal/controller/resource/resource_controller_test.go
- kubernetes/controller/test/e2e/testdata/basic-auth/rgd.yaml
- kubernetes/controller/examples/helm-configuration-localization/rgd.yaml
- kubernetes/controller/api/v1alpha1/zz_generated.deepcopy.go
Signed-off-by: Frederic Wilhelm <frederic.wilhelm@sap.com>
590c923 to
0e16c2a
Compare
… chart Signed-off-by: Frederic Wilhelm <frederic.wilhelm@sap.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
6 tasks
Signed-off-by: Frederic Wilhelm <frederic.wilhelm@sap.com>
Contributor
Author
|
Currently, the Conformance test is failing but it is also passing. I believe the failure is because it does not use the Helm Chart build previously: In comparison to the successful one where it downloads the chart artifact that was built previously (with the adjusted CRDs): |
Contributor
|
do we need to update the conformance test with |
Signed-off-by: Frederic Wilhelm <frederic.wilhelm@sap.com>
Contributor
Author
I don't think so. The problem of the Conformance Test that ran without the "Kubernetes Controller" context is missing the adjusted CRDs. It take the default Helm Chart from our registry that does not have the changes yet. The Conformance Test that is ran in the context of "Kubernetes Controller" is successful because it takes the Helm Chart that was build previously and is available as workflow artifact. So, the problem is not the rgd. It is the Helm Chart that is taken for tests. But when I think about this, we will always have a problem when we change the CRDs. The Conformance test workflow triggered by the "Kubernetes Controller" workflow will expect the changed CRDs. The Conformance test workflow triggered by changes in |
Contributor
Author
|
After thinking about this, the single Conformance Test workflow should only run with the pinned versions. I probably broke the tests by changing something in |
morri-son
pushed a commit
that referenced
this pull request
Mar 30, 2026
## Summary - Remove the `interval` field from `ResourceSpec` and the `GetRequeueAfter()` method from the `Resource` type - Update the Resource and Deployer controllers to no longer requeue on a timer - Remove `interval` from the Resource CRD schema (base + Helm chart) and drop it from the `required` list - Clean up all tests, samples, examples, and documentation accordingly ## Motivation The `interval` field on the Resource API is redundant with the existing event-driven reconciliation model. Unlike Repository and Component, the Resource controller does not poll an external system directly. Its reconciliation is already triggered by: 1. **Spec changes** — via the `GenerationChangedPredicate` on the Resource itself. 2. **Component updates** — via a watch on the referenced Component object, which re-enqueues all Resources that reference it. 3. **Resolution completions** — via the worker pool event source, which notifies the controller when an async resolution finishes. The Component controller is the one that actually polls the OCM repository on its own interval. When a new component version is discovered, the Component's status update propagates through the watch chain: Component → Resource → Deployer. Adding a separate polling interval on the Resource was therefore unnecessary overhead that could also mask the true source of updates and make debugging harder. Removing it makes the Resource controller purely reactive and aligns it with the principle that only the outermost controllers (Repository, Component) that interact with external systems should own polling intervals. --- This is a replacement for #2091 (see [comment](#2091 (comment))) Signed-off-by: Frederic Wilhelm <frederic.wilhelm@sap.com> Co-authored-by: Fabian Burth <fabian.burth@sap.com>
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
4 participants
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
intervalfield fromResourceSpecand theGetRequeueAfter()method from theResourcetypeintervalfrom the Resource CRD schema (base + Helm chart) and drop it from therequiredlistMotivation
The
intervalfield on the Resource API is redundant with the existing event-drivenreconciliation model. Unlike Repository and Component, the Resource controller does
not poll an external system directly. Its reconciliation is already triggered by:
GenerationChangedPredicateon the Resource itself.re-enqueues all Resources that reference it.
the controller when an async resolution finishes.
The Component controller is the one that actually polls the OCM repository on its
own interval. When a new component version is discovered, the Component's status
update propagates through the watch chain: Component → Resource → Deployer. Adding
a separate polling interval on the Resource was therefore unnecessary overhead that
could also mask the true source of updates and make debugging harder.
Removing it makes the Resource controller purely reactive and aligns it with the
principle that only the outermost controllers (Repository, Component) that interact
with external systems should own polling intervals.