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

KIALI-836 Fix map written concurrently on no_service_checker #232

Merged
merged 3 commits into from May 30, 2018

Conversation

@lucasponce
Copy link
Contributor

commented May 30, 2018

No description provided.

@lucasponce lucasponce force-pushed the lucasponce:FixRaceCondition branch from 8b53d20 to 126675f May 30, 2018
@simonpasquier

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

Instead of passing around maps, it would be more "go-esque" to communicate via channels. I was tempted to do it in #225 but refrained myself 😏

func (in NoServiceChecker) Check() models.IstioValidations {
   var validationc := make(chan models.IstioValidations)
   // snip
   go runRouteRulesCheck(in.IstioDetails.RouteRules, in.Namespace, serviceNames, validationc, &wg)
   // snip
   go func() {
       wg.Wait()
       // closing the channel will stop the range loop below
       close(validationc)
   }()

   for v := range validationc {
       validations.MergeVadlidations(v)
   }
   return validations
}

func runRouteRuleCheck(routeRule kubernetes.IstioObject, namespace string, serviceNames []string, validationc chan models.IstioValidations, wg *sync.WaitGroup) {
   // snip
    validationc <- validations
}

Untested but this is to show the mechanics.

@lucasponce

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@simonpasquier
I thought about channels, too.
But, in some previous comment there was a discussion about the "abuse" use of channels.

With this one, it is ensure that map are not written concurrently, I think this address the previous issue, I can refactor later.

@simonpasquier

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

@lucasponce understood. Note that there's no need to pass *models.IstioValidations, models.IstioValidations should be fine.

The core of the problem is here that MergeValidations() can't be called from different goroutines and it is up to the caller to ensure that it doesn't happen. Another option would be to extend IstioValidations with a mutex.

But, in some previous comment there was a discussion about the "abuse" use of channels.

Sure but in that case, it wouldn't be an over-use IMO.

@lucasponce

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

It's fine, I'm coding the channel approach.
This has been a good opportunity to face a "real" concurrency issue.
So, one identified, is good to improve the fix.

@lucasponce

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

Please @simonpasquier, let me know if the channel approach for the fix is close what you had in mind.
Thanks.


for _, routeRule := range routeRules {
go runRouteRuleCheck(routeRule, namespace, serviceNames, validations, &rrWg)
go runRouteRuleCheck(routeRule, namespace, serviceNames, rrvalidationsc, &rrWg)

This comment has been minimized.

Copy link
@simonpasquier

simonpasquier May 30, 2018

Contributor

You should be able to pass validationsc down to runRouteRuleCheck (eg no need to create an intermediary rrvalidationsc channel) and keep the rest of the function unchanged. The same applies to runDestinationPoliciesCheck, runVirtualServicesCheck and runDestinationRulesCheck. Otherwise LGTM.

@lucasponce

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@simonpasquier updated, let me know if the final approach is good to merge.
Thanks.

Copy link
Contributor

left a comment

👍

@jshaughn

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

@simonpasquier Thanks for the Go lesson :) One question, would this also work using only one waitgroup, where wg.add is called in the goroutines as opposed to using inner waitgroups?

@lucasponce

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@jshaughn perhaps not as reading in the doc

	// This goroutine has set counter to 0 when waiters > 0.
	// Now there can't be concurrent mutations of state:
	// - Adds must not happen concurrently with Wait,
	// - Wait does not increment waiters if it sees counter == 0.

I understand that WaitGroup is not defined to be mutated during the execution of the concurrent tasks shoud monitor.

@jshaughn

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

@lucasponce Yes, I suppose it would be dangerous to assume that all of the adds would happen before the wg was reduced to 0, thus releasing the original wait. And also, the potential danger of concurrent adds.

Copy link
Contributor

left a comment

A good lesson in Go concurrency.

@jshaughn jshaughn merged commit 5498bfe into kiali:master May 30, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lucasponce lucasponce deleted the lucasponce:FixRaceCondition branch Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.