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
Fix endpoint reconciler failing to delete masterlease #114122
Fix endpoint reconciler failing to delete masterlease #114122
Conversation
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Thu Nov 24 21:33:01 UTC 2022. |
|
31c238f
to
ea1328c
Compare
/assign @wojtek-t @liggitt @lavalamp @tallclair |
#114109 (comment) |
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 is great - just some minor comments.
Please fix the verify test failures.
add regression test for endpoint reconciler leases
ea1328c
to
4ffca65
Compare
Copying from @liggitt from the original PR cc @kubernetes/release-managers for visibility. We probably want to hold rc1 for this if we're not anticipating an rc2 /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, wojtek-t 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 |
/milestone v1.26 |
Thanks! @pacoxu, will this get auto-picked to release-1.26? For 1.22-1.25, hopefully this will cherry-pick cleanly |
@liggitt we are auto-ff-ing things from master to release-1.26 until we do the code thaw |
That's what I thought but I wanted to double check |
…2-upstream-release-1.25 Automated cherry pick of #114122: Fix endpoint reconciler failing to delete masterlease
@@ -120,7 +120,8 @@ func (s *storageLeases) UpdateLease(ip string) error { | |||
|
|||
// RemoveLease removes the lease on a master IP in storage | |||
func (s *storageLeases) RemoveLease(ip string) error { | |||
return s.storage.Delete(apirequest.NewDefaultContext(), s.baseKey+"/"+ip, &corev1.Endpoints{}, nil, rest.ValidateAllObjectFunc, nil) | |||
key := path.Join(s.baseKey, ip) |
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.
Can you avoid using path.Join for this? I know it's a pain but hiding the call to path.Clean is a problem.
cc @tallclair
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.
at head, in a followup, sure. For backports, I don't think it's worth it. baseKey is hardcoded and IP is a stringified net.IP so it can't contain weird slash/space/backstep chars
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.
Sounds good.
Thanks for fixing this! |
/triage accepted |
Fixes: #114049
This is @wojtek-t 's + a regression test #114109
This was introduced in #113686 so will need to be cherrypicked back to all releases that PR was cherrypicked.
/king bug
/kind regression
/sig api-machinery
/priority critical-urgent
/assign @wojtek-t @liggitt @lavalamp @tallclair