Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add locking to map access #279

Merged
merged 4 commits into from
Apr 27, 2021

Conversation

eytan-avisror
Copy link
Collaborator

Signed-off-by: Eytan Avisror eytan_avisror@intuit.com
Fixes #278

This adds a read lock/unlock when accessing Namespaces map

Eytan Avisror added 2 commits April 26, 2021 17:25
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
@eytan-avisror eytan-avisror requested review from a team as code owners April 27, 2021 00:29
@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #279 (5548cae) into master (088d011) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #279   +/-   ##
=======================================
  Coverage   70.23%   70.23%           
=======================================
  Files          19       19           
  Lines        2970     2970           
=======================================
  Hits         2086     2086           
  Misses        755      755           
  Partials      129      129           
Impacted Files Coverage Δ
controllers/provisioners/config.go 78.34% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 088d011...5548cae. Read the comment docs.

@backjo
Copy link
Collaborator

backjo commented Apr 27, 2021

Is this missing the changes needed in reconcilers.go?

@eytan-avisror
Copy link
Collaborator Author

Is this missing the changes needed in reconcilers.go?

Nope, the write part already locks:

func (r *InstanceGroupReconciler) namespaceReconciler(obj client.Object) []ctrl.Request {
var (
name = obj.GetName()
)
ctrl.Log.Info("namespace watch event", "object", name)
namespacedName := types.NamespacedName{
Namespace: "",
Name: name,
}
r.NamespacesLock.Lock()
defer r.NamespacesLock.Unlock()

@backjo
Copy link
Collaborator

backjo commented Apr 27, 2021

Ah, gotcha. I'm blind :P

Anywho - noticed a compilation failure on local - filed #280 to fix our checks.

vet: ./main.go:153:27: cannot use &(sync.Mutex literal) (value of type *sync.Mutex) as *sync.RWMutex value in struct literal

@eytan-avisror
Copy link
Collaborator Author

eytan-avisror commented Apr 27, 2021

Ah, gotcha. I'm blind :P

Anywho - noticed a compilation failure on local - filed #280 to fix our checks.

vet: ./main.go:153:27: cannot use &(sync.Mutex literal) (value of type *sync.Mutex) as *sync.RWMutex value in struct literal

Hmm, looks like we are bypassing make test by calling go test directly which doesnt run vet

Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
@eytan-avisror eytan-avisror merged commit 522a8d4 into keikoproj:master Apr 27, 2021
@eytan-avisror eytan-avisror mentioned this pull request Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IsNamespaceAnnotated is not thread safe
2 participants