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

Cleanup old NS name format #543

Merged
merged 1 commit into from
Jul 20, 2021
Merged

Cleanup old NS name format #543

merged 1 commit into from
Jul 20, 2021

Conversation

k0da
Copy link
Collaborator

@k0da k0da commented Jul 12, 2021

We changed nameserver name format, but for migration puposes we need to
take care and drop entries with an old format. Each cluster will take
care of its own entries.

Signed-off-by: Dinar Valeev [email protected]

We changed nameserver name format, but for migration puposes we need to
take care and drop entries with an old format. Each cluster will take
care of its own entries.

Signed-off-by: Dinar Valeev <[email protected]>
Copy link
Collaborator

@kuritka kuritka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @k0da , thanks for the PR. The implemented mechanism filters the original NS names from the EdgeDNS returned records per each reconciliation. The code looks good but I would consider different approach.

The problem I see is that the records will remain in EdgeDNS forever. Imagine someone without knowing the context will go through infoblox records and will ask why all of them are there?? Similarly, the filtering mechanism remaining in the code forever, which without knowing the historical context will confuse contributors.

Maybe it would be better to consider the migration as a separate k8gb class / package, which would contain the current and future migrations (separation of concerns).
The migrations would be run once when the application is started (or redeployed) and would make permanent changes.

Now we have chain:
read config -> logger -> reconciler + scheme builder -> provider -> start operator

So we can extend it like this:

read config -> logger -> reconciler + scheme builder -> provider -> migration (at this point we have everything important) -> start operator

I understand that the change I'm describing requires a bit more coding, although the result will be exactly the same, but in the long run we will find it easier to keep the system migration together.

@k0da
Copy link
Collaborator Author

k0da commented Jul 12, 2021

I see your concern. But cleaning up old naming should go away in one release. We don't have anything else yet to put into "migration machinery". This affects only infoblox implementation. The rest is handled by DNSEndpoint NS object, which shouldn't be affected by this.

@kuritka
Copy link
Collaborator

kuritka commented Jul 13, 2021

approving based on plan we made during meeting, good job @k0da !

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k0da , @kuritka pls confirm, based on discussion, this should not go to master

@k0da
Copy link
Collaborator Author

k0da commented Jul 13, 2021

@somaritane yeah

@k0da k0da closed this Jul 13, 2021
@kuritka
Copy link
Collaborator

kuritka commented Jul 13, 2021

@somaritane , sure, thx. Let's keep it blocked/closed pls to not accidently push into master..

@k0da k0da reopened this Jul 20, 2021
@k0da k0da mentioned this pull request Jul 20, 2021
@k0da k0da requested review from somaritane and kuritka July 20, 2021 13:35
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion, we agreed to merge it in with the long term strategy of removal of clean up code as part of the 0.9 release/milestone

@ytsarev ytsarev dismissed somaritane’s stale review July 20, 2021 14:57

we agreed to proceed with the merge with further code deprecation

@k0da k0da merged commit 7ce9ba4 into master Jul 20, 2021
@kuritka kuritka deleted the dropOldFormat branch July 22, 2021 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants