search

openshift / router

Ingress controller for OpenShift
Apache License 2.0
74 stars 116 forks source link

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

Closed gcs278 closed 6 months ago

gcs278 commented 6 months ago

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.

openshift-ci-robot commented 6 months ago

@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](https://github.com/openshift/router/pull/590): >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. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Frouter). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 6 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please ask for approval from gcs278. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/openshift/router/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci[bot] commented 6 months ago

@gcs278: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/unit 470f02809d8f93303bf5e7d732904ccace332ad4 link true /test unit
ci/prow/e2e-metal-ipi-ovn-ipv6 470f02809d8f93303bf5e7d732904ccace332ad4 link false /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-aws-serial 470f02809d8f93303bf5e7d732904ccace332ad4 link true /test e2e-aws-serial

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
gcs278 commented 6 months ago

Closing in favor of https://github.com/openshift/router/pull/587 /close

openshift-ci[bot] commented 6 months ago

@gcs278: Closed this PR.

In response to [this](https://github.com/openshift/router/pull/590#issuecomment-2088885237): >Closing in favor of https://github.com/openshift/router/pull/587 >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
openshift-ci-robot commented 6 months ago

@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](https://github.com/openshift/router/pull/590): >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. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Frouter). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.