Skip to content

Commit

Permalink
ovn/namespace: fix some namespace locking issues
Browse files Browse the repository at this point in the history
First, updateNamespace() didn't have locking for oc.namespaceMutex
Introduced by c3def15 (PR ovn-kubernetes#885).

Second, getNamespaceLock() didn't protect against concurrent
access of oc.namespaceMutex when it checked it the second time.
That's fishy anyway; we serialize Namespace events from the
watch factory and getNamespaceLock() is only called from event
handler functions, so it doesn't seem possible for the namespace
to be deleted between grabbing these two locks.

Signed-off-by: Dan Williams <[email protected]>
  • Loading branch information
dcbw committed Nov 16, 2019
1 parent e8a701b commit 7ad1bf3
Showing 1 changed file with 11 additions and 18 deletions.
29 changes: 11 additions & 18 deletions go-controller/pkg/ovn/namespace.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,13 @@ func (oc *Controller) multicastDeleteNamespace(ns *kapi.Namespace) {
func (oc *Controller) AddNamespace(ns *kapi.Namespace) {
logrus.Debugf("Adding namespace: %s", ns.Name)
oc.namespaceMutexMutex.Lock()
if oc.namespaceMutex[ns.Name] == nil {
oc.namespaceMutex[ns.Name] = &sync.Mutex{}
mutex, ok := oc.namespaceMutex[ns.Name]
if !ok {
mutex = &sync.Mutex{}
oc.namespaceMutex[ns.Name] = mutex
}

// A big fat lock per namespace to prevent race conditions
// with namespace resources like address sets and deny acls.
oc.namespaceMutex[ns.Name].Lock()
defer oc.namespaceMutex[ns.Name].Unlock()
mutex.Lock()
defer mutex.Unlock()
oc.namespaceMutexMutex.Unlock()

oc.namespaceAddressSet[ns.Name] = make(map[string]string)
Expand Down Expand Up @@ -204,10 +203,11 @@ func (oc *Controller) AddNamespace(ns *kapi.Namespace) {
func (oc *Controller) updateNamespace(old, newer *kapi.Namespace) {
logrus.Debugf("Updating namespace: old %s new %s", old.Name, newer.Name)

// A big fat lock per namespace to prevent race conditions
// with namespace resources like address sets and deny acls.
oc.namespaceMutex[newer.Name].Lock()
defer oc.namespaceMutex[newer.Name].Unlock()
mutex := oc.getNamespaceLock(newer.Name)
if mutex == nil {
return
}
defer mutex.Unlock()

oc.multicastUpdateNamespace(newer)
}
Expand Down Expand Up @@ -244,12 +244,5 @@ func (oc *Controller) getNamespaceLock(ns string) *sync.Mutex {

// lock the individual namespace
mutex.Lock()

// check that the namespace wasn't deleted between getting the two locks
if _, ok := oc.namespaceMutex[ns]; !ok {
mutex.Unlock()
return nil
}

return mutex
}

0 comments on commit 7ad1bf3

Please sign in to comment.