Bug 2005480: [4.8z] Remove waiting for namespace and namespace lock contention #760

trozet · 2021-09-17T21:00:43Z

Removes waiting for namespace event to show up. Now it is just ensured when needed. Also changes nsinfo to use a RWMutex so that pod handlers can all simultaneously RLock and not be blocked trying to add many pods for a single namespace.

openshift-ci · 2021-09-17T21:00:48Z

@trozet: This pull request references Bugzilla bug 2005480, which is invalid:

expected dependent Bugzilla bug 2005464 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead
expected dependent Bugzilla bug 2005476 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead
expected dependent Bugzilla bug 2005464 to target a release in 4.9.0, but it targets "4.8.z" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2005480: [4.8z] Remove waiting for namespace and namespace lock contention

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.

trozet · 2021-09-17T21:02:24Z

First 3 commits are part of #743 and this change depends on them merging first.

/hold

dcbw · 2021-09-18T19:09:12Z

/retest
#743 merged

The pod handlers and egressgw code would wait for the namespace add event before being able to actually create things. This wait could last up to 10 seconds before failure, and possibly even longer depending on how backed up the namespace handler is. This patch removes waiting where it is unnecessary. Those cases are the following: - Pod add only cares about the address sets for the namespace being available. We already create the address set if we detect it is nil during pod add so there is nothing we are really waiting for the ns event. - Egress GW waits for namespace to see what routes the annotation may have on the namespace for gateways. There is really no reason to wait as we can add the routes we know about, and then let the namespace add event cover any routes that were missing when it arrives. - Multicast enablement. This is configured via the namespace annotation. If it is missing because we get the pod add event first, the add namespace event will cover enabling multicast for all pods in the namespace. Patch does not cover one case of waiting for namespace lock: - Network Policy ACL logging. The ACL logging levels for deny and allow are configured in the namespace. If the network policy does not wait, and the namespace event comes later, we have no method to iterate through all affected policies for that namespace and set there correct logging levels. Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit ea5a757)

nsInfo is one of the 2 main sources of contention during heavy pod add operations. Moving this to an RW Mutex greatly improves performance because there in most places we are able to only use an RLock for nsInfo, which allows the multiple parallel pod handlers to be blocked less often. Signed-off-by: Tim Rozet <trozet@redhat.com> (cherry picked from commit 98e02b2)

Egress firewall gets namespace lock still in 4.8z. Updated those calls to aquire the lock appropriately. In pods.go, exgw calls and hybrid overlay exgw calls also need to get the lock. For hybrid overlay exgw, we still use waitForNamespace because we cannot update a pod's routes from the namespace watcher, so we must wait for the namespace in the pod handler. Signed-off-by: Tim Rozet <trozet@redhat.com>

trozet · 2021-09-20T15:47:30Z

/hold cancel

trozet · 2021-09-20T15:48:10Z

go-controller/pkg/ovn/egressfirewall.go

@@ -179,12 +179,12 @@ func (oc *Controller) syncEgressFirewall(egressFirwalls []interface{}) {

 func (oc *Controller) addEgressFirewall(egressFirewall *egressfirewallapi.EgressFirewall) error {
 	klog.Infof("Adding egressFirewall %s in namespace %s", egressFirewall.Name, egressFirewall.Namespace)
-	nsInfo, err := oc.waitForNamespaceLocked(egressFirewall.Namespace)
+	nsInfo, nsUnlock, err := oc.ensureNamespaceLocked(egressFirewall.Namespace, false)


@JacobTanenbaum PTAL at all of the changes in this commit

@trozet looks good to me for the changes to egressFirewall

trozet · 2021-09-20T15:48:26Z

@dcbw PTAL at the CARRY commit

trozet · 2021-09-21T18:01:54Z

/retest

trozet · 2021-09-21T21:51:44Z

/retest-required

trozet · 2021-09-22T20:18:07Z

/bugzilla refresh

openshift-ci · 2021-09-22T20:18:13Z

@trozet: This pull request references Bugzilla bug 2005480, which is invalid:

expected dependent Bugzilla bug 2005464 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead
expected dependent Bugzilla bug 2005464 to target a release in 4.9.0, but it targets "4.8.z" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

trozet · 2021-09-23T21:04:27Z

/retest

trozet · 2021-09-24T13:05:54Z

/bugzilla refresh

openshift-ci · 2021-09-24T13:06:59Z

@trozet: This pull request references Bugzilla bug 2005480, which is invalid:

expected dependent Bugzilla bug 2005464 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is ON_QA instead
expected dependent Bugzilla bug 2005464 to target a release in 4.9.0, but it targets "4.8.z" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

trozet · 2021-09-24T13:15:57Z

/bugzilla refresh

openshift-ci · 2021-09-24T13:17:02Z

@trozet: This pull request references Bugzilla bug 2005480, which is invalid:

expected dependent Bugzilla bug 2005464 to target a release in 4.9.0, but it targets "4.8.z" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

trozet · 2021-09-24T13:24:55Z

/bugzilla refresh

openshift-ci · 2021-09-24T13:24:59Z

@trozet: This pull request references Bugzilla bug 2005480, which is invalid:

expected dependent Bugzilla bug 2005464 to target a release in 4.9.0, but it targets "4.8.z" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

trozet · 2021-09-24T13:59:36Z

/bugzilla refresh

openshift-bot · 2021-09-29T01:32:00Z

/retest-required