OCPBUGS-26498: Optimize Upgrade Validation plugin by skipping unnecessary changes

[gcs278]

Both performIngressConditionUpdate and performIngressConditionRemoval functions add tasks to the writerlease queue even if no work needed to be done. This commit optimizes the Upgrade Validation plugin by ensuring that tasks for updating UnservableInFutureVersions are only added to the queue when changes are required.

This reduction in unnecessary work significantly lowers the frequency of lease extensions. Previously, excessive lease extensions could delay route status updates under certain conditions, such as during temporary contention periods
where a router pod gets demoted to a follower. After the contention is resolved, the pod’s subsequent retry will be delayed more than necessary.

Additionally, this update provides a clearer indication of when updates are actually required, but have been completed by another router pod. The prior logic did not clearly distinguish between unnecessary updates and updates that were completed by another pod.

The selective updates are only added to the interface used by Upgrade Validation plugin to avoid perturbing existing Admitted condition logic. This means nominal clusters without SHA1 routes should not be impacted by the Upgrade
Validation plugin, minimizing the risk associated with its introduction.

Fixes TestRouteAdmissionPolicy Flake by reducing the number of unnecessary lease extensions.

[openshift-ci-robot]

@gcs278: This pull request references Jira Issue OCPBUGS-26498, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.16.0) matches configured target version for branch (4.16.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Previously, both performIngressConditionUpdate and performIngressConditionRemoval functions were adding tasks to the writerlease queue even if no work needed to be done. This commit optimizes the logic by ensuring that tasks are only added to the queue when changes are required.

This reduction in unnecessary work significantly lowers the frequency of lease extensions. Previously, excessive lease extensions could delay route status updates under certain conditions, such as during temporary contention periods
where a router pod gets demoted to a follower. After the contention is resolved, the pod’s subsequent retry will be delayed more than necessary.

Additionally, this update provides a clearer indication of when updates are actually required, but have been completed by another router pod. The prior logic did not clearly distinguish between unnecessary updates and updates that were completed by another pod.

This PR also add some logging in leasewriter and the plugin chain for assisting future debugging efforts.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[gcs278]

Though this is a valid improvement, I'm closing in favor of #588 as I now don't think this PR fixes the TestRouteAdmissionPolicy flake.

/close

[openshift-ci]

@gcs278: Closed this PR.

In response to this:

Though this is a valid improvement, I'm closing in favor of #588 as I now don't think this PR fixes the TestRouteAdmissionPolicy flake.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@gcs278: This pull request references Jira Issue OCPBUGS-26498. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In response to this:

Previously, both performIngressConditionUpdate and performIngressConditionRemoval functions were adding tasks to the writerlease queue even if no work needed to be done. This commit optimizes the logic by ensuring that tasks are only added to the queue when changes are required.

This reduction in unnecessary work significantly lowers the frequency of lease extensions. Previously, excessive lease extensions could delay route status updates under certain conditions, such as during temporary contention periods
where a router pod gets demoted to a follower. After the contention is resolved, the pod’s subsequent retry will be delayed more than necessary.

Additionally, this update provides a clearer indication of when updates are actually required, but have been completed by another router pod. The prior logic did not clearly distinguish between unnecessary updates and updates that were completed by another pod.

Fixes TestRouteAdmissionPolicy Flake by reducing the number of unnecessary lease extensions.

This PR also add some logging in writerlease and the plugin chain for assisting future debugging efforts.

WIP because it needs testing.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[gcs278]

TestRouteAdmissionPolicy is still flaking, and this may be a reasonable fix.
/reopen

[openshift-ci]

@gcs278: Reopened this PR.

In response to this:

TestRouteAdmissionPolicy is still flaking, and this may be a reasonable fix.
/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@gcs278: This pull request references Jira Issue OCPBUGS-26498, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.16.0) matches configured target version for branch (4.16.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Previously, both performIngressConditionUpdate and performIngressConditionRemoval functions were adding tasks to the writerlease queue even if no work needed to be done. This commit optimizes the logic by ensuring that tasks are only added to the queue when changes are required.

This reduction in unnecessary work significantly lowers the frequency of lease extensions. Previously, excessive lease extensions could delay route status updates under certain conditions, such as during temporary contention periods
where a router pod gets demoted to a follower. After the contention is resolved, the pod’s subsequent retry will be delayed more than necessary.

Additionally, this update provides a clearer indication of when updates are actually required, but have been completed by another router pod. The prior logic did not clearly distinguish between unnecessary updates and updates that were completed by another pod.

Fixes TestRouteAdmissionPolicy Flake by reducing the number of unnecessary lease extensions.

This PR also add some logging in writerlease and the plugin chain for assisting future debugging efforts.

WIP because it needs testing.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[Miciah]

Please add a comment to note that route.DeepCopy() is necessary because recordIngressCondition actually modifies route.

[Miciah]

Consider refactoring to use findCondition instead of recordIngressCondition to save some overhead from deepcopies etc.

[gcs278]

Done.

[gcs278]

Putting a hold to wait for testing confirmation.
/hold

[Miciah]

Reading this and the unit test makes me realize that both the StatusAdmitter plugin and the UpgradeValidation plugin update the status ingress entry in case one of these fields (Host, WildcardPolicy, or RouterCanonicalHostname) needs to be updated.

Suppose the route has a host name that conflicts with another route's, and then the route is updated to have a non-conflicting host name. UpgradeValidation runs first in the plugin chain, so it updates the status ingress entry first, and the StatusAdmitter plugin updates the status condition second. Do I have that right?

Would it be safer for RecordRouteUnservableInFutureVersions to ignore changes to the status ingress entry other than the UnservableInFutureVersions status condition?

[gcs278]

Suppose the route has a host name that conflicts with another route's, and then the route is updated to have a non-conflicting host name. UpgradeValidation runs first in the plugin chain, so it updates the status ingress entry first, and the StatusAdmitter plugin updates the status condition second. Do I have that right?

Correct. Both UpgradeValidation and StatusAdmitter try to update the new host name as part of the ingress fields.

Would it be safer for RecordRouteUnservableInFutureVersions to ignore changes to the status ingress entry other than the UnservableInFutureVersions status condition?

Hmmm. Yea it might be slightly safer. If the route is also going from Rejected to Admitted, you'll have this sequence:

1. Route is updated to fix hostname
2. UpgradeValidation updates status to fix hostname
3. StatusAdmitter updates status from Admitted=False to Admitted=True

If we limit UpgradeValidation to just changes in UnservableInFutureVersions, we should only get one status update:

1. Route is updated to fix hostname
2. UpgradeValidation does nothing since it only cares about UnservableInFutureVersions
3. StatusAdmitter updates status to fix hostname and sets Admitted=False to Admitted=True

So yes, I agree. I can update it so we only care about conditions changes. That way, if it's just a hostname change, the UpgradeValidation plugin will let StatusAdmitter take care of that.

[Miciah]

Thanks!
/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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 [Miciah]

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

[openshift-ci]

@gcs278: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[gcs278]

I've ran nearly a hundred times with my e2e test in openshift/origin#28710 and no issues. Reviewed the router logs and lease extends are drastically reduced.
/hold cancel

[openshift-ci-robot]

@gcs278: Jira Issue OCPBUGS-26498: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

• openshift/cluster-ingress-operator#1014 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-26498 has not been moved to the MODIFIED state.

In response to this:

Both performIngressConditionUpdate and performIngressConditionRemoval functions add tasks to the writerlease queue even if no work needed to be done. This commit optimizes the Upgrade Validation plugin by ensuring that tasks for updating UnservableInFutureVersions are only added to the queue when changes are required.

This reduction in unnecessary work significantly lowers the frequency of lease extensions. Previously, excessive lease extensions could delay route status updates under certain conditions, such as during temporary contention periods
where a router pod gets demoted to a follower. After the contention is resolved, the pod’s subsequent retry will be delayed more than necessary.

Additionally, this update provides a clearer indication of when updates are actually required, but have been completed by another router pod. The prior logic did not clearly distinguish between unnecessary updates and updates that were completed by another pod.

The selective updates are only added to the interface used by Upgrade Validation plugin to avoid perturbing existing Admitted condition logic. This means nominal clusters without SHA1 routes should not be impacted by the Upgrade
Validation plugin, minimizing the risk associated with its introduction.

Fixes TestRouteAdmissionPolicy Flake by reducing the number of unnecessary lease extensions.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-bot]

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-haproxy-router-base-container-v4.16.0-202405020546.p0.g5610ac8.assembly.stream.el9 for distgit ose-haproxy-router-base.
All builds following this will include this PR.