-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Handle the migration to the new TXT format: create missing records #2811
Handle the migration to the new TXT format: create missing records #2811
Conversation
239daa2
to
b19c1bd
Compare
45e878d
to
af8dbdd
Compare
/assign @k0da |
@alebedev87: GitHub didn't allow me to assign the following users: k0da. Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
af8dbdd
to
25addee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
/assign @Raffo |
Tested the migration manually e2e. Records created with
Then
|
should fix #2817 |
/assign @njuettner |
/lgtm |
@alebedev87 can you tell us exactly how this is fixing the error and what is the exact error? Not this nor the issue linked are clear. I would also appreciate clear instructions on how to reproduce this so that we can verify. /cc @k0da Are there some situations in which this bug doesn't happen? Trying to understand what was missed during testing. |
/lgtm |
registry/txt.go
Outdated
missingDesiredTXT = append(missingDesiredTXT, desiredTXT) | ||
} | ||
} | ||
if len(desiredTXTs) > len(missingDesiredTXT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guard condition is confusing. It's not possible that "missingDesiredTXT[s]" could be longer than "desiredTXTs." The former can only ever be shorter or the same length. Therefore, this condition is only false when the two slices are of the same length. What does it mean when they're of equal length? That there are no desired TXT records? That all of them are missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If both slices are equal in lengths this means that the record is not managed by externaldns. Because if it wsa managed by externaldns, at least one TXT record would exist. I added additional comment to make this clearer.
However, this made we think of the records owned by different externaldns instances. I think that we should not migrate the TXT records for the other instances. I added a guarding condition for this here.
@Raffo , @seh : what do you think about the migration of other (defferent txt-owner-id) externaldns instances records?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should only manipulate records that are managed by this ExternalDNS instance.
When trying to transfer record ownership between instances (orphaning and adopting), we've written a tool to revise the corresponding TXT records. It would be nice it one could invoke ExternalDNS with a command-line flag (or run it in a different mode, as a one-time tool rather than a long-running service) to rewrite the TXT records from an orphaning ExternalDNS instance to an adopting ExternalDNS instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's something we can think of, in the followup PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all makes sense to me 👍
@alebedev87 I'm good with the explanations and fixes, I'd defer to @seh's comments to be fixed, then I'm happy to approve and trigger the official release process. |
3be36a4
to
16e9250
Compare
16e9250
to
0d3b563
Compare
0d3b563
to
50f196c
Compare
Test after the addition of the check for the owner-id. The state before (unmanaged records, old format TXT records only, TXT records from different owner):
Run of external-dns:
The state after:
|
Until this bug is fixed: kubernetes-sigs/external-dns#2811
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, JAORMX, Raffo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@Raffo Is there any estimate of when this is gonna be officially released? |
@azalenski-branch I'll kick off a release next week. |
I'm still having problems on the v0.12.1 release candidate image:
Could it be because it is trying to both upsert and create the same txt? I only have one service in the cluster with the annotation Is there anything that you think I can debug @alebedev87 ? |
Description
Handle the migration to the new TXT record format automatically by ExternalDNS.
Should help fixing issue #2793.
Checklist