Skip to content

Conversation

@gcs278
Copy link
Contributor

@gcs278 gcs278 commented May 22, 2024

Manual cherry-pick of #28710

Add "The HAProxy router converges when multiple routers are writing conflicting upgrade validation status" test which validates router converge when writing conflicting status in a scenario that uses multiple conditions.

Previously, we tested conflicting status fields (hostname), but don't have a test for conflicting status. This test add logic that exercises new logic in the router for the Upgrade Validation plugin.

Also, this updates all of the router stress.go status tests write watch logic to use an informer, which allows for comparison of the old and new route objects along with the ability to start the informer in a separate go routine.

HOW I CREATED THIS PR:

  1. Create commit to vendor [release-4.15] OCPBUGS-28928: Add UnservableInFutureVersions route status condition type api#1751
    $ go get github.com/openshift/api@93d6bda14341f1d18319774367829c3278c171e7
    $ go mod tidy
    $ go mod vendor
    $ git commit 
    [...commit message...]
    
  2. Cherrypick all commits from OCPBUGS-26498: Add test for UpgradeValidation contention #28710. 3a0e98a is the merge commit.
    # Get all commits in this merge commit:
    $ git log --oneline  3a0e98a94f0b5cf5a89d9bf02ecf01f0ad827df1^..3a0e98a94f0b5cf5a89d9bf02ecf01f0ad827df1
    3a0e98a94f Merge pull request #28710 from gcs278/OCPBUGS-26498-upgradeable-status
    3341e1c534 Add E2E test for UpgradeValidation contention
    32934f1587 Refactor router status tests to use informer
    
    # Cherry-Pick the non-merge commits
    $ git cherry-pick 32934f1587 3341e1c534
    [...no conflicts...]
    

@gcs278 gcs278 changed the title Ocpbugs 26498 upgradeable status 4.15 OCPBUGS-28928: Add test for UpgradeValidation contention May 22, 2024
@openshift-ci-robot openshift-ci-robot added jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels May 22, 2024
@openshift-ci-robot
Copy link

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

7 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.15.z) matches configured target version for branch (4.15.z)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)
  • release note text is set and does not match the template
  • dependent bug Jira Issue OCPBUGS-26498 is in the state Verified, which is one of the valid states (VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), CLOSED (DONE-ERRATA))
  • dependent Jira Issue OCPBUGS-26498 targets the "4.16.0" version, which is one of the valid target versions: 4.16.0
  • bug has dependents

Requesting review from QA contact:
/cc @ShudiLi

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

Details

In response to this:

Manual cherry-pick of #28710

Add "The HAProxy router converges when multiple routers are writing conflicting upgrade validation status" test which validates router converge when writing conflicting status in a scenario that uses multiple conditions.

Previously, we tested conflicting status fields (hostname), but don't have a test for conflicting status. This test add logic that exercises new logic in the router for the Upgrade Validation plugin.

Also, this updates all of the router stress.go status tests write watch logic to use an informer, which allows for comparison of the old and new route objects along with the ability to start the informer in a separate go routine.

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 gcs278 changed the title OCPBUGS-28928: Add test for UpgradeValidation contention [WIP] OCPBUGS-28928: Add test for UpgradeValidation contention May 22, 2024
@openshift-ci openshift-ci bot requested a review from ShudiLi May 22, 2024 16:24
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 22, 2024
@openshift-ci openshift-ci bot requested review from knobunc and simonpasquier May 22, 2024 16:37
@gcs278 gcs278 force-pushed the OCPBUGS-26498-upgradeable-status-4.15 branch from 415f006 to 76880df Compare May 22, 2024 20:11
@openshift-ci openshift-ci bot added the vendor-update Touching vendor dir or related files label May 22, 2024
@gcs278
Copy link
Contributor Author

gcs278 commented May 29, 2024

/assign @frobware

gcs278 added 3 commits May 30, 2024 13:02
Bump openshift/api to get new route UnservableInFutureVersions
status condition type.

  go get github.com/openshift/api@93d6bda14341f1d18319774367829c3278c171e7
  go mod tidy
  go mod vendor

Vendors openshift/api#1751
This updates the router status test write watch logic to use
an informer, which allows for comparison of the old and new
route objects along with the ability to start the informer
in a separate go routine.

The ability to compare the incoming change allows us to add
filtering to make sure only the updates for our test router are counted.
This greatly simplifies the logic because previously we couldn't filter
out other router updates. As a result, we can remove the logic which
waits for other routers to write status to our test routes.

The informer is now started asynchronously before the routes are
created. This allows us to accurately count all route updates.
Previously, the test routes would be created and then the watch routine
would start immediately after, which leads to the watch missing some
updates.
Add "The HAProxy router converges when multiple routers are
writing conflicting upgrade validation status" test which validates
router converge when writing conflicting status in a scenario that uses
multiple conditions.

Previously, we tested conflicting status fields (hostname), but don't
have a test for conflicting status. This test add logic that exercises
new logic in the router for the Upgrade Validation plugin.
@gcs278 gcs278 force-pushed the OCPBUGS-26498-upgradeable-status-4.15 branch from 76880df to 6132a62 Compare May 30, 2024 17:03
@frobware
Copy link
Contributor

frobware commented Jun 3, 2024

Fabulous instructions. I created a branch, applied your steps, and see zero differences between this PR and my ephemeral branch.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2024
@gcs278
Copy link
Contributor Author

gcs278 commented Jun 3, 2024

Now that openshift/router#585 has merged, this is no longer WIP.
/retest

@gcs278 gcs278 changed the title [WIP] OCPBUGS-28928: Add test for UpgradeValidation contention OCPBUGS-28928: Add test for UpgradeValidation contention Jun 3, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2024
@gcs278
Copy link
Contributor Author

gcs278 commented Jun 3, 2024

Unrelated
/test e2e-metal-ipi-ovn-ipv6
/test e2e-metal-ipi-sdn

@gcs278
Copy link
Contributor Author

gcs278 commented Jun 3, 2024

/test e2e-metal-ipi-ovn-ipv6
/test e2e-metal-ipi-sdn

@gcs278
Copy link
Contributor Author

gcs278 commented Jun 3, 2024

/retest-required

@Miciah
Copy link
Contributor

Miciah commented Jun 3, 2024

/approve

@gcs278
Copy link
Contributor Author

gcs278 commented Jun 3, 2024

Run multi-stage test e2e-metal-ipi-ovn-ipv6 - e2e-metal-ipi-ovn-ipv6-baremetalds-devscripts-gather container test unrelated:
/retest-required

@frobware
Copy link
Contributor

frobware commented Jun 4, 2024

/test e2e-metal-ipi-ovn-ipv6

@gcs278
Copy link
Contributor Author

gcs278 commented Jun 4, 2024

@deads2k or @knobunc could you please provide an /approve since this touches go.mod?

@gcs278
Copy link
Contributor Author

gcs278 commented Jun 4, 2024

/retest

@xueqzhan
Copy link
Contributor

xueqzhan commented Jun 6, 2024

/lgtm

@gcs278
Copy link
Contributor Author

gcs278 commented Jun 6, 2024

@xueqzhan thanks for the LGTM! I actually need approve and backport-risk-assessed. Are you able to do that?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 6, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, gcs278, Miciah, xueqzhan

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2024
@xueqzhan
Copy link
Contributor

xueqzhan commented Jun 6, 2024

@xueqzhan thanks for the LGTM! I actually need approve and backport-risk-assessed. Are you able to do that?

approved is applied. I believe backport-risk-assessed needs to be applied by a staff engineer. It will be @jupierce or @deads2k

@gcs278
Copy link
Contributor Author

gcs278 commented Jun 6, 2024

approved is applied.

Ah thanks @xueqzhan, sorry the for confusion. I suppose this repo has lgtm_acts_as_approve: true on.

@deads2k deads2k added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Jun 7, 2024
@deads2k deads2k added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jun 7, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 7, 2024

@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/e2e-aws-ovn-single-node-upgrade 6132a62 link false /test e2e-aws-ovn-single-node-upgrade
ci/prow/e2e-aws-etcd-recovery 6132a62 link false /test e2e-aws-etcd-recovery
ci/prow/e2e-aws-ovn-single-node-serial 6132a62 link false /test e2e-aws-ovn-single-node-serial
ci/prow/e2e-metal-ipi-sdn 6132a62 link false /test e2e-metal-ipi-sdn

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 2320880 and 2 for PR HEAD 6132a62 in total

@openshift-merge-bot openshift-merge-bot bot merged commit c13a01e into openshift:release-4.15 Jun 7, 2024
@openshift-ci-robot
Copy link

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

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

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-28928 has not been moved to the MODIFIED state.

Details

In response to this:

Manual cherry-pick of #28710

Add "The HAProxy router converges when multiple routers are writing conflicting upgrade validation status" test which validates router converge when writing conflicting status in a scenario that uses multiple conditions.

Previously, we tested conflicting status fields (hostname), but don't have a test for conflicting status. This test add logic that exercises new logic in the router for the Upgrade Validation plugin.

Also, this updates all of the router stress.go status tests write watch logic to use an informer, which allows for comparison of the old and new route objects along with the ability to start the informer in a separate go routine.

HOW I CREATED THIS PR:

  1. Create commit to vendor [release-4.15] OCPBUGS-28928: Add UnservableInFutureVersions route status condition type api#1751
$ go get github.com/openshift/api@93d6bda14341f1d18319774367829c3278c171e7
$ go mod tidy
$ go mod vendor
$ git commit 
[...commit message...]
  1. Cherrypick all commits from OCPBUGS-26498: Add test for UpgradeValidation contention #28710. 3a0e98a is the merge commit.
# Get all commits in this merge commit:
$ git log --oneline  3a0e98a94f0b5cf5a89d9bf02ecf01f0ad827df1^..3a0e98a94f0b5cf5a89d9bf02ecf01f0ad827df1
3a0e98a94f Merge pull request #28710 from gcs278/OCPBUGS-26498-upgradeable-status
3341e1c534 Add E2E test for UpgradeValidation contention
32934f1587 Refactor router status tests to use informer

# Cherry-Pick the non-merge commits
$ git cherry-pick 32934f1587 3341e1c534
[...no conflicts...]

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
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build openshift-enterprise-tests-container-v4.15.0-202406100906.p0.gc13a01e.assembly.stream.el8 for distgit openshift-enterprise-tests.
All builds following this will include this PR.

@openshift-merge-robot
Copy link
Contributor

Fix included in accepted release 4.15.0-0.nightly-2024-06-12-223752

LiangquanLi930 pushed a commit to LiangquanLi930/origin that referenced this pull request Jul 12, 2024
…ble-status-4.15

OCPBUGS-28928: Add test for UpgradeValidation contention
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. vendor-update Touching vendor dir or related files

Projects

None yet

Development

Successfully merging this pull request may close these issues.