[release-4.10] Bug 2104454: improve performance of service sync #1173

jcaamano · 2022-07-06T10:39:59Z

This is a backport of #1110, containing:

CARRY: cf3a846 libovsdb: passthrough ops in ModelClient
CARRY: 73e491c libovsdb: refactor duplicate detection in modelClient
1aaad50 libovsdbops: only one wait per txn
0573fe5 Don't lookup LBs that don't exist in cache

Conflicts:

Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com> (cherry picked from commit cf3a846)

openshift-ci · 2022-07-06T10:40:54Z

@jcaamano: This pull request references Bugzilla bug 2104454, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.z) matches configured target release for branch (4.10.z)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 2070674 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 2070674 targets the "4.11.0" release, which is one of the valid target releases: 4.11.0
bug has dependents

Requesting review from QA contact:
/cc @anuragthehatter

In response to this:

[release-4.10] Bug 2104454: improve performance of service sync

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.

Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com> (cherry picked from commit 73e491c)

Transactions is an all or nothing thing so we just need one wait per transaction to check against duplicates. Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com> (cherry picked from commit 1aaad50) (cherry picked from commit e490188)

Running the LB predicate that matches on name takes a long time if the LB table has many LBs. For example, looking up ~40 LBs in a table with ~200k rows took aproximately 3s. The service controller has a second level cache and knows which LBs need to be added and which need to be updated. Avoid this lookup for LBs that are to be added. Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com> (cherry picked from commit 0573fe5) (cherry picked from commit c4a539b)

tssurya · 2022-07-07T16:10:57Z

/cc

openshift-ci · 2022-07-07T18:02:52Z

@jcaamano: 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-openstack-ovn	`3369d38`	link	false	`/test e2e-openstack-ovn`
ci/prow/okd-e2e-gcp-ovn	`3369d38`	link	false	`/test okd-e2e-gcp-ovn`

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.

tssurya

/lgtm

tssurya · 2022-07-22T16:27:36Z

go-controller/pkg/libovsdbops/model_client.go

+func (m *ModelClient) buildOps(ops []ovsdb.Operation, doWhenFound opModelToOpMapper, doWhenNotFound opModelToOpMapper, opModels ...OperationModel) (interface{}, []ovsdb.Operation, error) {
+	if ops == nil {
+		ops = []ovsdb.Operation{}
+	}
 	notfound := []interface{}{}
 	for _, opModel := range opModels {
 		if opModel.ExistingResult == nil && opModel.Model != nil {


commit1: LGTM, same as 4.11 no conflicts from cf3a846

tssurya · 2022-07-22T16:29:34Z

go-controller/pkg/libovsdbops/loadbalancer.go

@@ -105,17 +105,6 @@ func createOrUpdateLoadBalancerOps(nbClient libovsdbclient.Client, ops []libovsd

 	// If LoadBalancer does not exist, create it
 	if err == libovsdbclient.ErrNotFound {
-		timeout := types.OVSDBWaitTimeout


this is partially from ovn-org/ovn-kubernetes@8f17235#diff-685368ea628f10afd438349e793caa717e04ec15ae38a15ac0be406686f0ae5bL97
Moves the wait to model_client.

true. Maybe I should just pick the whole thing.

tssurya · 2022-07-22T16:37:36Z

go-controller/pkg/libovsdbops/loadbalancer.go

-		ops = append(ops, libovsdb.Operation{
-			Op:      libovsdb.OperationWait,
-			Timeout: &timeout,
-			Table:   "Load_Balancer",


self-note: EnsureLBs-> CreateOrUpdateLoadBalancersOps -> createOrUpdateLoadBalancerOps -> modelClient.CreateOrUpdateOps -> modelClient.createOrUpdateOps -> buildFailOnDuplicateOps -> wait ops moved there.

tssurya · 2022-07-22T16:45:48Z

go-controller/pkg/libovsdbops/switch.go

@@ -304,15 +304,9 @@ func AddACLToNodeSwitch(nbClient libovsdbclient.Client, nodeName string, nodeACL
 		Name: nodeName,
 	}

-	aclName := ""


So these names should be unique were added for the wait ops here: 72c9cdb#diff-8f8adc7b3c118c97013725f0f4e69dd32f54e098c4e183f52a39049f34b2ed1bR288; refactored 6d60741#diff-8f8adc7b3c118c97013725f0f4e69dd32f54e098c4e183f52a39049f34b2ed1bL306 so 4.11 doesn't have this function at all. Your buildFailOnDuplicateOps should take care of setting the names...?

buildFailOnDuplicateOps is basically in charge of building the Wait op. A spearate field with the names is no longer needed.

tssurya · 2022-07-22T16:54:40Z

go-controller/pkg/ovn/egressfirewall.go

@@ -132,12 +132,7 @@ func (oc *Controller) syncEgressFirewallRetriable(egressFirewalls []interface{})
 		for i := range egressFirewallACLs {
 			egressFirewallACL := egressFirewallACLs[i]
 			egressFirewallACL.Direction = types.DirectionToLPort
-			aclName := ""
-			if egressFirewallACL.Name != nil {
-				aclName = *egressFirewallACL.Name


forgive me for the stupid question that is unrelated to your PR, but just to understand, we never had waits for ACLs rights (72c9cdb#diff-54373a7069b8883bb780babfb45422a0c8af7211107571b700db55cf17feb863R292)? why do we have code where the name was explicitly set in many places for ACLs and you are removing them now?

I don't know but probably in preparation to actually have waits for ACLs as well in the future.

tssurya · 2022-07-22T16:58:08Z

go-controller/pkg/ovn/gateway_init.go

@@ -782,7 +771,6 @@ func (oc *Controller) createPolicyBasedRoutes(match, priority, nexthops string)
 			},
 		},
 		{
-			Name:           logicalRouter.Name,


self-note: partial revert of 72c9cdb#diff-9c68f154bc9189eb61d17c17d1392ee36c13a497f4f742a98250c2e0a272d48cR61 different because of 8b14898#diff-9c68f154bc9189eb61d17c17d1392ee36c13a497f4f742a98250c2e0a272d48cL68, being present in 4.11

tssurya · 2022-07-22T17:09:08Z

go-controller/pkg/util/util.go

@@ -297,7 +297,6 @@ func UpdateNodeSwitchExcludeIPs(nbClient libovsdbclient.Client, nodeName string,

 	opModels := []libovsdbops.OperationModel{
 		{
-			Name:           logicalSwitchDes.Name,
 			Model:          &logicalSwitchDes,
 			ModelPredicate: func(ls *nbdb.LogicalSwitch) bool { return ls.Name == nodeName },
 			OnModelMutations: []interface{}{


commit2 LGTM: slightly confused around the ACL names, but mostly this is a combo of moving to buildFailOnDuplicateOps and removing the .Name that was added in the initial waitOps commit.

self note:
model.go: exactly same from 73e491c#diff-305e20a256590719c5d0ae03a702e7070b42539c3d38478a99b89893312be20dR322
model_client.go doesn't have 73e491c#diff-54373a7069b8883bb780babfb45422a0c8af7211107571b700db55cf17feb863L467 rest is the same.

tssurya · 2022-07-22T17:12:46Z

go-controller/pkg/libovsdbops/model_client.go

+
+func isGuardOp(op *ovsdb.Operation) bool {
+	return op != nil && op.Op == ovsdb.OperationWait && op.Timeout != nil && *op.Timeout == types.OVSDBWaitTimeout
+}


commit 3: LGTM same as e490188#diff-54373a7069b8883bb780babfb45422a0c8af7211107571b700db55cf17feb863
NOTE: didn't review tests, I am going to open a JIRA card to run unit tests in 4.10 CI

tssurya · 2022-07-22T17:16:14Z

go-controller/pkg/libovsdbops/model_client.go

@@ -188,30 +193,32 @@ func (m *ModelClient) CreateOrUpdateOps(opModels ...OperationModel) (interface{}
 If BulkOp is set, delete or mutate can happen accross multiple models found.
 */
 func (m *ModelClient) Delete(opModels ...OperationModel) error {
-	ops, err := m.DeleteOps(opModels...)
+	ops, err := m.DeleteOps(nil, opModels...)


awesome! :) this is exactly what I need for my cherry-pick :)

tssurya · 2022-07-22T17:19:59Z

go-controller/pkg/libovsdbops/model_client.go

@@ -122,6 +122,10 @@ func (m *ModelClient) WithClient(client client.Client) *ModelClient {
 	return &cl
 }

+func onModelUpdatesNone() []interface{} {


COMMIT4: LGTM
except this bit which came from cleanup, exactly same as c4a539b

trozet

/approve

openshift-ci · 2022-07-25T14:58:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcaamano, trozet, tssurya

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~~ [jcaamano,trozet]

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

trozet · 2022-07-25T14:59:45Z

/label backport-risk-assessed

trozet · 2022-07-25T15:00:59Z

/assign @anuragthehatter

tssurya · 2022-07-29T12:14:38Z

@anuragthehatter PTAL, this blocks a cherry-pick I need for https://bugzilla.redhat.com/show_bug.cgi?id=2105657 (CU bug)

anuragthehatter · 2022-08-01T12:55:42Z

/label cherry-pick-approved

openshift-ci-robot · 2022-08-01T15:50:09Z

/retest-required

Remaining retests: 2 against base HEAD 358801d and 8 for PR HEAD 3369d38 in total

openshift-ci · 2022-08-01T19:44:08Z

@jcaamano: All pull requests linked via external trackers have merged:

openshift/ovn-kubernetes#1173

Bugzilla bug 2104454 has been moved to the MODIFIED state.

In response to this:

[release-4.10] Bug 2104454: improve performance of service sync

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.

libovsdb: passthrough ops in ModelClient

d0083c2

Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com> (cherry picked from commit cf3a846)

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jul 6, 2022

openshift-ci bot requested review from anuragthehatter, dcbw and JacobTanenbaum July 6, 2022 10:40

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2022

jcaamano added 3 commits July 7, 2022 14:44

libovsdb: refactor duplicate detection in modelClient

f2169ef

Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com> (cherry picked from commit 73e491c)

libovsdbops: only one wait per txn

3ce4a28

Transactions is an all or nothing thing so we just need one wait per transaction to check against duplicates. Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com> (cherry picked from commit 1aaad50) (cherry picked from commit e490188)

jcaamano force-pushed the release-4.10 branch from 21d3544 to 3369d38 Compare July 7, 2022 14:50

openshift-ci bot requested a review from tssurya July 7, 2022 16:11

tssurya approved these changes Jul 22, 2022

View reviewed changes

openshift-ci bot assigned tssurya Jul 22, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2022

trozet approved these changes Jul 25, 2022

View reviewed changes

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

openshift-ci bot assigned anuragthehatter Jul 25, 2022

openshift-ci bot added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Aug 1, 2022

openshift-merge-robot merged commit 464b78d into openshift:release-4.10 Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release-4.10] Bug 2104454: improve performance of service sync #1173

[release-4.10] Bug 2104454: improve performance of service sync #1173

jcaamano commented Jul 6, 2022

openshift-ci bot commented Jul 6, 2022

tssurya commented Jul 7, 2022

openshift-ci bot commented Jul 7, 2022

tssurya left a comment

tssurya Jul 22, 2022

tssurya Jul 22, 2022

jcaamano Jul 26, 2022

tssurya Jul 22, 2022

tssurya Jul 22, 2022

jcaamano Jul 26, 2022

tssurya Jul 22, 2022

jcaamano Jul 26, 2022

tssurya Jul 22, 2022

tssurya Jul 22, 2022

tssurya Jul 22, 2022

tssurya Jul 22, 2022

tssurya Jul 22, 2022

trozet left a comment

openshift-ci bot commented Jul 25, 2022

trozet commented Jul 25, 2022

trozet commented Jul 25, 2022

tssurya commented Jul 29, 2022

anuragthehatter commented Aug 1, 2022

openshift-ci-robot commented Aug 1, 2022

openshift-ci bot commented Aug 1, 2022

[release-4.10] Bug 2104454: improve performance of service sync #1173

[release-4.10] Bug 2104454: improve performance of service sync #1173

Conversation

jcaamano commented Jul 6, 2022

openshift-ci bot commented Jul 6, 2022

tssurya commented Jul 7, 2022

openshift-ci bot commented Jul 7, 2022

tssurya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trozet left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 25, 2022

trozet commented Jul 25, 2022

trozet commented Jul 25, 2022

tssurya commented Jul 29, 2022

anuragthehatter commented Aug 1, 2022

openshift-ci-robot commented Aug 1, 2022

openshift-ci bot commented Aug 1, 2022