Skip to content

[WIP] OCPBUGS-26498: Remove lease.Extend from performIngressConditionRemoval #590

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.

Sign up for GitHub

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

Closed
gcs278 wants to merge 1 commit into from
Closed

[WIP] OCPBUGS-26498: Remove lease.Extend from performIngressConditionRemoval #590

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions pkg/router/controller/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ const (
unservableInFutureVersionsClearAction = string(routev1.RouteUnservableInFutureVersions) + "-Clear"
)

// TODO: Refactor the router to perform a single status update per route reconciliation.
// Currently, executing multiple status updates simultaneously can complicate the writer lease and
// contention tracker logic, increasing the likelihood of write conflicts and contentions. While the
// existing logic is eventually consistent and acceptable for infrequent scenarios, it may lead to
// prolonged status updates and complicate the debugging process.

// RouteStatusRecorder is an object capable of recording why a route status condition changed.
type RouteStatusRecorder interface {
RecordRouteRejection(route *routev1.Route, reason, message string)
Expand Down Expand Up @@ -208,11 +214,8 @@ func performIngressConditionRemoval(action string, lease writerlease.Lease, trac
changed, now, latest, original := removeIngressCondition(route, routerName, condType)
if !changed {
log.V(4).Info("no changes to route needed", "action", action, "namespace", route.Namespace, "name", route.Name)
// Extending the lease ensures a delay in future work ONLY for followers. Unlike in
// performIngressConditionUpdate, it's not logical to invoke findMostRecentIngress here and expect the last
// update to be from our router. This is because performIngressConditionRemoval *removes* a condition
// without providing a LastTransitionTime on a condition for us to track previous actions.
lease.Extend(workKey)
// Previously, we executed lease.Extend here. We found that it led to over-extending the lease in nominal
// clusters that don't actually have any conditions to be removed.
return writerlease.None, false
}

Expand Down