From 470f02809d8f93303bf5e7d732904ccace332ad4 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Tue, 30 Apr 2024 20:03:21 -0400 Subject: [PATCH] Remove lease.Extend from performIngressConditionRemoval Since performIngressConditionUpdate already extends the lease, we should not extend the lease in performIngressConditionRemoval. The lease gets over extended because the UpgradeValidation plugin and the Status plugin both attempt to update route status in two separate lease work items and both are extending the lease when there is no work to be done. In a scenario where the Status plugin needs to admit a route, but the UpgradeValidation has no change to UnservableInFutureVersions condition, the UpgradeValidation lease work item will unnecessarily extend the lease, leading to an extra delay in the Status plugin admitted the route. Also add a TODO item to document current limitations of doing multiple status updates in the same route reconcile loop for future developers. --- pkg/router/controller/status.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/pkg/router/controller/status.go b/pkg/router/controller/status.go index 5b7c930ed..42cd48cf7 100644 --- a/pkg/router/controller/status.go +++ b/pkg/router/controller/status.go @@ -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) @@ -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 }