Skip to content

Conversation

mloiseleur
Copy link
Collaborator

What does it do ?

It reverts

Motivation

As @abursavich suspected:

If we don't have matching generations when generating the endpoints, the sync could delete the DNS records.

More

  • Yes, this PR title follows Conventional Commits
  • Yes, I added unit tests
  • Yes, I updated end user documentation accordingly

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. source labels Jun 1, 2025
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 1, 2025
@mloiseleur
Copy link
Collaborator Author

@kashalls Can you confirm that your issue was fixed by #5241 ? And so we can safely revert ?
@mrozentsvayg Can you confirm that you do not have your issue anymore with this revert ?

@kashalls
Copy link
Contributor

kashalls commented Jun 1, 2025

@kashalls Can you confirm that your issue was fixed by #5241 ? And so we can safely revert ?

Is there a built image with this commit? And also, if the commit is bad please revert it. If 5241 solves this, then we should be good. Apologies for the bad PR.

Copy link
Contributor

@ivankatliarchuk ivankatliarchuk left a comment

Choose a reason for hiding this comment

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

lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2025
@abursavich
Copy link
Contributor

LGTM

@mloiseleur
Copy link
Collaborator Author

Is there a built image with this commit?

@kashalls See how to test a PR ?

@mloiseleur
Copy link
Collaborator Author

mloiseleur commented Jun 3, 2025

@kashalls did how to test a PR ? answer your question ?

@kashalls
Copy link
Contributor

kashalls commented Jun 3, 2025

@kashalls did how to test a PR ? answer your question ?

I haven't had an opportunity to test this. Please feel free to revert my PR if it is causing issues for others - @tholinka Do you have a moment to test this?

@tholinka
Copy link
Contributor

tholinka commented Jun 3, 2025

I tested this revert branch (image here), and I can't recreate #5348.

So I think #5241 fixes this, and this revert should be good.

As a bit of retrospective as well, I think it would have been a valid solution to use the max value of status.parents[].conditions[].observedGeneration instead of using metadata.generation, as then we use the newest observed generation instead of the newest generation. Though if there is no new generation (e.g. parentRef is edited to an invalid value), that wouldn't have detected it, whereas #5241 does detect it.

On that topic, I do want to note one thing about #5241 I observed while testing this. If you change the parentRef name or namespace, #5241 causes immediate deletion of the existing route, even before the new parentRef is in a status. I would assume that's correct (especially in the case I mentioned above, where the will be no new generation because the new parentRef is invalid), but I just wanted to note it, as #5486 might still see their issue even with this revert.

@mloiseleur
Copy link
Collaborator Author

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivankatliarchuk, mloiseleur

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:
  • OWNERS [ivankatliarchuk,mloiseleur]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2025
@mloiseleur
Copy link
Collaborator Author

thanks for this detailed test @tholinka 👍

@k8s-ci-robot k8s-ci-robot merged commit 7fb0ed0 into kubernetes-sigs:master Jun 4, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants