Controllers in inactive groups still update group ID and replace premature check with controller SDK backoff. - #809
Controllers in inactive groups still update group ID and replace premature check with controller SDK backoff.#809philbrookes wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThe change removes manual DNS record validity tracking and the ChangesDNS reconciliation and registry updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DNSRecordReconciler
participant RateLimiter
participant GroupRegistry
participant DNSProvider
participant KubernetesAPI
DNSRecordReconciler->>RateLimiter: select bounded requeue delay
RateLimiter->>DNSRecordReconciler: return reconcile result
DNSRecordReconciler->>GroupRegistry: apply inactive-group registry updates
GroupRegistry->>DNSProvider: suppress endpoint creates and deletes
DNSRecordReconciler->>KubernetesAPI: persist status and WriteCounter
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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.
Actionable comments posted: 2
🤖 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 `@internal/controller/dnsrecord_controller.go`:
- Around line 439-445: Update DNSRecordReconciler.SetupWithManager to enforce
the supported requeue bounds before assigning defaultRequeueTime,
defaultValidationRequeue, or configuring RateLimiter: reject or clamp minRequeue
values below 5 seconds and maxRequeue values above 15 minutes, while preserving
the existing min/max ordering validation.
- Around line 503-524: Update specOrDeletionChangedPredicate.Update to accept
deletion only on the transition from a nil old deletion timestamp to a non-nil
new timestamp. Keep generation-change handling unchanged, and return false for
subsequent updates while deletion is already in progress so explicit
RequeueAfter results drive the deletion state machine.
🪄 Autofix
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 Plus
Run ID: 558d3689-7f33-48c7-a3ef-a1c5b5062334
📒 Files selected for processing (19)
CLAUDE.mdapi/v1alpha1/dnsrecord_types.goapi/v1alpha1/zz_generated.deepcopy.gobundle/manifests/dns-operator.clusterserviceversion.yamlbundle/manifests/kuadrant.io_dnsrecords.yamlcharts/dns-operator/templates/manifests.yamlcmd/main.goconfig/crd/bases/kuadrant.io_dnsrecords.yamldocs/RFC/distributed-dns/dnsrecord-lifecycle.mddocs/reference/dnsrecord.mdinternal/controller/base_dnsrecord_reconciler.gointernal/controller/dnshealthcheckprobe_reconciler.gointernal/controller/dnsrecord_controller.gointernal/controller/dnsrecord_controller_runtime_reconfiguration_test.gointernal/controller/dnsrecord_controller_test.gointernal/controller/dnsrecord_groups_test.gointernal/controller/helper_test.gointernal/controller/remote_dnsrecord_controller.gointernal/controller/suite_test.go
💤 Files with no reviewable changes (7)
- internal/controller/dnsrecord_controller_runtime_reconfiguration_test.go
- docs/reference/dnsrecord.md
- api/v1alpha1/zz_generated.deepcopy.go
- bundle/manifests/kuadrant.io_dnsrecords.yaml
- charts/dns-operator/templates/manifests.yaml
- config/crd/bases/kuadrant.io_dnsrecords.yaml
- api/v1alpha1/dnsrecord_types.go
6bf5d40 to
6fb3e0d
Compare
philbrookes
changed the title
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
This has the same concern I raised in #733, with recordReceivedPrematurely removed, AwaitingValidation is set and then immediately cleared on the next reconcile (5 seconds later). There's no meaningful validation window anymore. What purpose does AwaitingValidation serve with these changes? |
This is true, I'd completely forgotten that issue. I wonder if we could have the awaiting validation stay true until the last write was over a X time ago and there are no changes made in this reconcile? |
I see that this is exactly what we discussed before. I'm not sure if the validation loop still has enough value to be worth the effort of recoding it that way though. Maybe the better answer here is to remove the concept of a DNSRecord verification step? |
Mike and I discussed this offline, and came to the conclusion that we could implement a backoff using the lastTransitionTime of the ready condition, and avoid using the Requeue: true. I will implement and push up some changes to reflect this. |
Remove recordReceivedPrematurely, exponentialRequeueTime, and the --valid-for flag. Use time.Since(LastTransitionTime) to derive a natural ramp-up from defaultValidationRequeue to defaultRequeueTime, removing the need for explicit backoff state. Add a specOrDeletionChangedPredicate to prevent status-only watch events from bypassing the calculated RequeueAfter. Remove QueuedAt and ValidFor status fields (WriteCounter kept for the dns_provider_write_counter Prometheus metric). Signed-off-by: Phil Brookes <pbrookes@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…ers (#779) Inactive group controllers now call publishRecord to update TXT registry labels (e.g. groupID) without creating, deleting, or modifying DNS record targets. The GroupRegistry filters changes based on its IsActive field — when inactive, only label-change Updates are applied. This ensures unpublishInactiveGroups can correctly identify inactive group records via accurate TXT entries. Signed-off-by: Phil Brookes <pbrookes@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Adds a test that tampers TXT registry group labels on an inactive controller's authoritative record and verifies the inactive controller corrects them via registryOnly applyChanges without modifying zone DNS records. Signed-off-by: Phil Brookes <pbrookes@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
Summary
This PR combines three related changes for the DNS groups feature:
1. Refactor: replace manual reconcile backoff with SDK rate limiter (#778)
nextReconcileAt/lastReconcileAttracking with the SDK's built-inItemExponentialFailureRateLimiterRequeueTimeandWriteCounterfromDNSRecordStatus, simplifying the CRDspecOrDeletionChangedPredicateto filter watch events to spec changes (generation changed) and deleting objects, preventing status-only updates from causing tight reconciliation loopsdefaultValidationRequeueconsistently for pre-deletion status transitions2. Feat: always write groupID to provider registry for inactive controllers (#779)
registryOnlymode toapplyChangesthat strips Creates, Deletes, and target-changing Updates from the plan, allowing only TXT registry label updates (e.g. groupID) to be appliedapplyChanges(registryOnly=true)to write accurate groupID labels to TXT registry entries without modifying DNS recordsunpublishInactiveGroupsfrom correctly identifying and cleaning up inactive group records3. Test: integration test for groupID registry self-correction (#780)
registryOnlyapplyChangestamperRegistryGrouphelper to simulate stale/incorrect group labels by modifying authoritative record TXT endpointsfilterDNSEndpointsto clear Labels for stable endpoint comparisonsFixes #778, #779, #780
Test plan
make test-unit)make test-integration) — all 46 specsSummary by CodeRabbit
Breaking Changes
queuedAtandvalidForfields from DNS record status information and documentation.--valid-forconfiguration option and corresponding environment variable.Improvements