Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete the DNSEndpoint resource when VS is deleted & Ratelimit requeues on errors. #4504

Merged
merged 2 commits into from Nov 17, 2023

Conversation

ciarams87
Copy link
Member

Proposed changes

When a VirtualServer with ExternalDNS enabled is deleted, the corresponding DNSEndpoint resource is not cleaned up. Additionally, we are endlessly re-queuing VS objects with ExternalDNS enabled with errors without rate limiting.
Also, we are not checking if the VS has been deleted before re-queuing, in both the external dns controller, and the cert-manager controller.

This commit rectifies these issues by:

  1. Not blocking owner deletion on DNSEndpoints
  2. When there is an error processing a DNSEndpoint sync for VirtualServer, the VS is re-added to the queue using rate limiting, with an eventual timeout
  3. When the error processing the item is due to the VS no longer existing, don't return an error and exit quietly.

Checklist

Closes #4418

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@ciarams87 ciarams87 requested a review from a team as a code owner October 11, 2023 15:12
@github-actions github-actions bot added the enhancement Pull requests for new features/feature enhancements label Oct 11, 2023
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (c7680a4) 51.99% compared to head (5a8ddf2) 51.94%.

Files Patch % Lines
internal/externaldns/controller.go 0.00% 6 Missing ⚠️
internal/certmanager/cm_controller.go 0.00% 4 Missing ⚠️
internal/externaldns/sync.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4504      +/-   ##
==========================================
- Coverage   51.99%   51.94%   -0.05%     
==========================================
  Files          59       59              
  Lines       16960    16972      +12     
==========================================
- Hits         8818     8816       -2     
- Misses       7847     7859      +12     
- Partials      295      297       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ciarams87 ciarams87 force-pushed the feat/delete-extdns-vs-deleted branch 3 times, most recently from 17a0305 to 0835235 Compare October 19, 2023 11:02
@ciarams87 ciarams87 force-pushed the feat/delete-extdns-vs-deleted branch 2 times, most recently from 448bdfd to 6941b64 Compare October 24, 2023 15:05
@ciarams87 ciarams87 force-pushed the feat/delete-extdns-vs-deleted branch from 6941b64 to 0528c50 Compare November 7, 2023 09:23
@brianehlert brianehlert added this to the v3.4.0 milestone Nov 8, 2023
@brianehlert brianehlert added the backlog Pull requests/issues that are backlog items label Nov 8, 2023
@danielnginx danielnginx removed this from the v3.4.0 milestone Nov 15, 2023
@shaun-nx shaun-nx merged commit 6d85f27 into main Nov 17, 2023
62 checks passed
@shaun-nx shaun-nx deleted the feat/delete-extdns-vs-deleted branch November 17, 2023 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Pull requests/issues that are backlog items enhancement Pull requests for new features/feature enhancements
Projects
Status: Done 🚀
Development

Successfully merging this pull request may close these issues.

Deleting VirtualServer with ExternalDNS enabled causes CPU usage to increase
5 participants