OCPBUGS-11453: [release-4.10] Batch potentially big transaction on egress firewall ACLs migration by npinaeva · Pull Request #1641 · openshift/ovn-kubernetes

npinaeva · 2023-04-14T16:00:05Z

Backport of #1629
Had to make some changes, this one requires a proper review

openshift-ci · 2023-04-14T16:00:08Z

@npinaeva: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

OCPBUGS-11453: [release-4.10] Batch potentially big transaction on egress firewall ACLs migration

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 · 2023-04-14T16:00:14Z

@npinaeva: This pull request references Jira Issue OCPBUGS-11453, which is valid. The bug has been moved to the POST state.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.10.z) matches configured target version for branch (4.10.z)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)
dependent bug Jira Issue OCPBUGS-11110 is in the state Verified, which is one of the valid states (VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE))
dependent Jira Issue OCPBUGS-11110 targets the "4.11.z" version, which is one of the valid target versions: 4.11.0, 4.11.z
bug has dependents

Requesting review from QA contact:
/cc @huiran0826

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

Details

In response to this:

Backport of #1629
Had to make some changes, this one requires a proper review

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.

npinaeva · 2023-04-17T14:09:34Z

/retest-required

jcaamano · 2023-04-19T09:31:58Z

@npinaeva any reason to skip the batch test?

npinaeva · 2023-04-19T09:51:15Z

@npinaeva any reason to skip the batch test?

because the tests were written for int, and now Batch only works for ACLs, so it would require re-writing the whole test. So I though I can cheat and say we know the function works based on the tests in the previous versions :P

jcaamano · 2023-04-19T10:21:32Z

The batching & tests could be []interface{} based instead and retain some of its generic nature?
That might be interesting as we might end up backporting other batching.

npinaeva · 2023-04-19T10:43:27Z

The batching & tests could be []interface{} based instead and retain some of its generic nature? That might be interesting as we might end up backporting other batching.

Yeah I considered this option, but that would require copying the slice twice to convert types back and forth, so I thought we would just copy the Batch function for other types in the future if needed.
We can go the []interface{} way, or try to rewrite tests for acls (which is not that difficult), both approaches have their pros and cons, wdyt @jcaamano ?

jcaamano · 2023-04-19T10:56:09Z

I see. The other only thing I can think of is a batch function that gives you the indexes of the array you need to work with:

// the meat goes here
func batch(batchSize int, dataSize int, eachFn func(from, to int)) error

// adapter function per type that uses the previous function
func BatchACLs(batchSize int, data []nbdb.ACL, eachFn func([]nbdb.ACL) error) error {

Then we can just test batch.

The default transaction timeout is 10 seconds, it can be reached when we delete all egress firewall acls during migration to port groups from switches. Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com> (cherry picked from commit 1896e16) (cherry picked from commit 7fb527e) (cherry picked from commit 88ecd8b) Conflicts: go-controller/pkg/ovn/egressfirewall.go - egressFirewallACLPriorityKey is not used in 4.11, because logging for egress firewall is not implemented (cherry picked from commit 5a64c5b) Conflicts: go-controller/pkg/ovn/egressfirewall.go Update Batch to be typed, since generics are not availbale in go 1.17 Update Batch tests to use ACLs instead of ints. Use RemoveACLsFromAllSwitches instead of RemoveACLsFromLogicalSwitchesWithPredicate, use nbdb.ACL instead of *nbdb.ACL

first argument is nil. Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com> (cherry picked from commit 11283d6) (cherry picked from commit 74f95e9) (cherry picked from commit 8ce4aa4) (cherry picked from commit 6173c7b)

stale acls. Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com> (cherry picked from commit 81acdc2) (cherry picked from commit 34eb562) Conflict; egressfirewall.go - update apimachinery.sets to the previous version (cherry picked from commit 0bc8f14) (cherry picked from commit dc46aa9) Conflicts: go-controller/pkg/ovn/egressfirewall.go had to move the fix to libovsdbops/switch

npinaeva · 2023-04-19T13:10:30Z

I see. The other only thing I can think of is a batch function that gives you the indexes of the array you need to work with:
// the meat goes here
func batch(batchSize int, dataSize int, eachFn func(from, to int)) error

// adapter function per type that uses the previous function
func BatchACLs(batchSize int, data []nbdb.ACL, eachFn func([]nbdb.ACL) error) error {
Then we can just test batch.

I didn't want to introduce any new logic (which switching to indexes will require), I prefer to stay as close to the initial implementation as possible when it comes to backporting. So the most straightforward way for now I would say it to just make Batch type pre-defined. I updated the tests, since it is not that many changes I believe, lmk what you think

jcaamano · 2023-04-19T15:04:10Z

/lgtm
/approve
/label backport-risk-assessed

openshift-ci · 2023-04-19T15:09:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcaamano, npinaeva

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:

~~OWNERS~~ [jcaamano]

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

huiran0826 · 2023-04-20T03:23:47Z

/label qe-approved
/label cherry-pick-approved

openshift-ci-robot · 2023-04-20T03:58:39Z

/retest-required

Remaining retests: 0 against base HEAD f84b8d0 and 2 for PR HEAD e0b95a5 in total

npinaeva · 2023-04-20T08:25:52Z

/retest-required

openshift-ci · 2023-04-20T11:31:00Z

@npinaeva: 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-serial	`e0b95a5`	link	false	`/test e2e-aws-ovn-serial`
ci/prow/e2e-vsphere-windows	`e0b95a5`	link	false	`/test e2e-vsphere-windows`
ci/prow/okd-e2e-gcp-ovn	`e0b95a5`	link	false	`/test okd-e2e-gcp-ovn`
ci/prow/e2e-vsphere-ovn	`e0b95a5`	link	false	`/test e2e-vsphere-ovn`

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

openshift-ci-robot · 2023-04-20T11:34:00Z

@npinaeva: Jira Issue OCPBUGS-11453: All pull requests linked via external trackers have merged:

openshift/ovn-kubernetes#1641

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

Details

In response to this:

Backport of #1629
Had to make some changes, this one requires a proper review

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-merge-robot · 2023-04-22T09:18:55Z

Fix included in accepted release 4.10.0-0.nightly-2023-04-21-212037

openshift-bot · 2023-05-01T15:05:59Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-ovn-kubernetes-container-v4.10.0-202305011254.p0.g7215581.assembly.stream for distgit ose-ovn-kubernetes.
All builds following this will include this PR.

openshift-ci bot requested review from JacobTanenbaum, huiran0826 and tssurya April 14, 2023 16:00

npinaeva added 3 commits April 19, 2023 15:05

npinaeva force-pushed the ocpbugs-11453 branch from 666f52a to e0b95a5 Compare April 19, 2023 13:07

openshift-ci bot added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Apr 19, 2023

openshift-ci bot assigned anuragthehatter, asood-rh, huiran0826, jechen0648, mffiedler, qiowang721, rbbratta and weliang1 Apr 19, 2023

openshift-ci bot assigned yingwang-0320 and zhaozhanqi Apr 19, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2023

openshift-ci bot added qe-approved Signifies that QE has signed off on this PR cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Apr 20, 2023

openshift-merge-robot merged commit 7215581 into openshift:release-4.10 Apr 20, 2023

npinaeva deleted the ocpbugs-11453 branch April 20, 2023 16:19

Conversation

npinaeva commented Apr 14, 2023

Uh oh!

openshift-ci bot commented Apr 14, 2023

Uh oh!

openshift-ci-robot commented Apr 14, 2023

Uh oh!

npinaeva commented Apr 17, 2023

Uh oh!

jcaamano commented Apr 19, 2023

Uh oh!

npinaeva commented Apr 19, 2023

Uh oh!

jcaamano commented Apr 19, 2023

Uh oh!

npinaeva commented Apr 19, 2023

Uh oh!

jcaamano commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

npinaeva commented Apr 19, 2023

Uh oh!

jcaamano commented Apr 19, 2023

Uh oh!

openshift-ci bot commented Apr 19, 2023

Uh oh!

huiran0826 commented Apr 20, 2023

Uh oh!

openshift-ci-robot commented Apr 20, 2023

Uh oh!

npinaeva commented Apr 20, 2023

Uh oh!

openshift-ci bot commented Apr 20, 2023

Uh oh!

openshift-ci-robot commented Apr 20, 2023

Uh oh!

openshift-merge-robot commented Apr 22, 2023

Uh oh!

openshift-bot commented May 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

jcaamano commented Apr 19, 2023 •

edited

Loading