From 88bfac178ac1468804f5d690ed04193c411bd6b4 Mon Sep 17 00:00:00 2001 From: Shriram Sharma Date: Fri, 20 May 2022 14:57:11 -0700 Subject: [PATCH 1/5] Fixes #219 Added mutex to sync access to the SidecarEgressMap Signed-off-by: Shriram Sharma --- admiral/pkg/controller/common/types.go | 17 +++++++- admiral/pkg/controller/common/types_test.go | 47 +++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/admiral/pkg/controller/common/types.go b/admiral/pkg/controller/common/types.go index 7a51f7ab..a05418df 100644 --- a/admiral/pkg/controller/common/types.go +++ b/admiral/pkg/controller/common/types.go @@ -177,7 +177,10 @@ func (s *SidecarEgressMap) Put(identity string, namespace string, fqdn string, c } func (s *SidecarEgressMap) Get(key string) map[string]SidecarEgress { - return s.cache[key] + s.mutex.Lock() + val := s.cache[key] + s.mutex.Unlock() + return val } func (s *SidecarEgressMap) Delete(key string) { @@ -186,6 +189,18 @@ func (s *SidecarEgressMap) Delete(key string) { delete(s.cache, key) } +// Map func returns a map of identity to namespace:SidecarEgress map +// Iterating through the returned map is not implicitly thread safe, +// use (s *SidecarEgressMap) Range() func instead. func (s *SidecarEgressMap) Map() map[string]map[string]SidecarEgress { return s.cache } + +// Range is a thread safe iterator to iterate through the SidecarEgress map +func (s *SidecarEgressMap) Range(fn func(k string, v map[string]SidecarEgress)) { + s.mutex.Lock() + for k, v := range s.cache { + fn(k, v) + } + s.mutex.Unlock() +} diff --git a/admiral/pkg/controller/common/types_test.go b/admiral/pkg/controller/common/types_test.go index ebb33d2c..cd7fdfa6 100644 --- a/admiral/pkg/controller/common/types_test.go +++ b/admiral/pkg/controller/common/types_test.go @@ -191,3 +191,50 @@ func TestMapRange(t *testing.T) { assert.Equal(t, 3, numOfIter) } + +func TestSidecarEgressGet(t *testing.T) { + + egressMap := NewSidecarEgressMap() + egressMap.Put("pkey1", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"}) + + ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(3*time.Second)) + defer cancel() + + go func(ctx context.Context) { + for { + select { + case <-ctx.Done(): + return + default: + egressMap.Put("pkey1", string(uuid.NewUUID()), "fqdn", map[string]string{"pkey2": "pkey2"}) + } + } + }(ctx) + + for { + select { + case <-ctx.Done(): + return + default: + assert.NotNil(t, egressMap.Get("pkey1")) + } + } + +} + +func TestTestSidecarEgressRange(t *testing.T) { + + egressMap := NewSidecarEgressMap() + egressMap.Put("pkey1", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"}) + egressMap.Put("pkey2", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"}) + egressMap.Put("pkey3", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"}) + + numOfIter := 0 + egressMap.Range(func(k string, v map[string]SidecarEgress) { + assert.NotNil(t, v) + numOfIter++ + }) + + assert.Equal(t, 3, numOfIter) + +} From 1a0d30c62bbea56fa03b017033607ff6dc4a5875 Mon Sep 17 00:00:00 2001 From: Shriram Sharma Date: Fri, 20 May 2022 14:59:53 -0700 Subject: [PATCH 2/5] fixed the test name Signed-off-by: Shriram Sharma --- admiral/pkg/controller/common/types_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/admiral/pkg/controller/common/types_test.go b/admiral/pkg/controller/common/types_test.go index cd7fdfa6..a22599b7 100644 --- a/admiral/pkg/controller/common/types_test.go +++ b/admiral/pkg/controller/common/types_test.go @@ -222,7 +222,7 @@ func TestSidecarEgressGet(t *testing.T) { } -func TestTestSidecarEgressRange(t *testing.T) { +func TestSidecarEgressRange(t *testing.T) { egressMap := NewSidecarEgressMap() egressMap.Put("pkey1", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"}) From 62ea06e349f0ad30d100945982aabefa93dd5f68 Mon Sep 17 00:00:00 2001 From: Shriram Sharma Date: Mon, 23 May 2022 15:03:26 -0700 Subject: [PATCH 3/5] used defer to release the mutex Signed-off-by: Shriram Sharma --- admiral/pkg/controller/common/types.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/admiral/pkg/controller/common/types.go b/admiral/pkg/controller/common/types.go index a05418df..91e20a26 100644 --- a/admiral/pkg/controller/common/types.go +++ b/admiral/pkg/controller/common/types.go @@ -177,10 +177,9 @@ func (s *SidecarEgressMap) Put(identity string, namespace string, fqdn string, c } func (s *SidecarEgressMap) Get(key string) map[string]SidecarEgress { + defer s.mutex.Unlock() s.mutex.Lock() - val := s.cache[key] - s.mutex.Unlock() - return val + return s.cache[key] } func (s *SidecarEgressMap) Delete(key string) { @@ -198,9 +197,9 @@ func (s *SidecarEgressMap) Map() map[string]map[string]SidecarEgress { // Range is a thread safe iterator to iterate through the SidecarEgress map func (s *SidecarEgressMap) Range(fn func(k string, v map[string]SidecarEgress)) { + defer s.mutex.Unlock() s.mutex.Lock() for k, v := range s.cache { fn(k, v) } - s.mutex.Unlock() } From 53a6b558ac11084999d3a023c4d37757f7e521b5 Mon Sep 17 00:00:00 2001 From: Shriram Sharma Date: Wed, 25 May 2022 08:29:04 -0700 Subject: [PATCH 4/5] changes made as per review comments Signed-off-by: Shriram Sharma --- admiral/pkg/controller/common/types_test.go | 24 +++++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/admiral/pkg/controller/common/types_test.go b/admiral/pkg/controller/common/types_test.go index a22599b7..9c6f785f 100644 --- a/admiral/pkg/controller/common/types_test.go +++ b/admiral/pkg/controller/common/types_test.go @@ -3,6 +3,7 @@ package common import ( "context" "strings" + "sync" "testing" "time" @@ -200,10 +201,14 @@ func TestSidecarEgressGet(t *testing.T) { ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(3*time.Second)) defer cancel() + var wg sync.WaitGroup + wg.Add(2) + // Producer go routine go func(ctx context.Context) { for { select { case <-ctx.Done(): + wg.Done() return default: egressMap.Put("pkey1", string(uuid.NewUUID()), "fqdn", map[string]string{"pkey2": "pkey2"}) @@ -211,15 +216,20 @@ func TestSidecarEgressGet(t *testing.T) { } }(ctx) - for { - select { - case <-ctx.Done(): - return - default: - assert.NotNil(t, egressMap.Get("pkey1")) + // Consumer go routine + go func(ctx context.Context) { + for { + select { + case <-ctx.Done(): + wg.Done() + return + default: + assert.NotNil(t, egressMap.Get("pkey1")) + } } - } + }(ctx) + wg.Wait() } func TestSidecarEgressRange(t *testing.T) { From e330fdec6dacfbabd5cb8cee00b6074d2a8ab122 Mon Sep 17 00:00:00 2001 From: Shriram Sharma Date: Wed, 25 May 2022 08:33:22 -0700 Subject: [PATCH 5/5] used defer Signed-off-by: Shriram Sharma --- admiral/pkg/controller/common/types_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/admiral/pkg/controller/common/types_test.go b/admiral/pkg/controller/common/types_test.go index 9c6f785f..741a0d41 100644 --- a/admiral/pkg/controller/common/types_test.go +++ b/admiral/pkg/controller/common/types_test.go @@ -205,10 +205,10 @@ func TestSidecarEgressGet(t *testing.T) { wg.Add(2) // Producer go routine go func(ctx context.Context) { + defer wg.Done() for { select { case <-ctx.Done(): - wg.Done() return default: egressMap.Put("pkey1", string(uuid.NewUUID()), "fqdn", map[string]string{"pkey2": "pkey2"}) @@ -218,10 +218,10 @@ func TestSidecarEgressGet(t *testing.T) { // Consumer go routine go func(ctx context.Context) { + defer wg.Done() for { select { case <-ctx.Done(): - wg.Done() return default: assert.NotNil(t, egressMap.Get("pkey1"))