Origin: confused lock usage in router

[adohe-zz]

I am learning origin code, when I scan through router implementation, I found this lock usage https://github.com/openshift/origin/blob/master/pkg/router/template/router.go#L233 very confused. From the lock description, it is used to prevent concurrent router reloads, but I really can't find concurrent router reloads in code, this reload is only invoked by rateLimitedFunction, it handles events serially. @smarterclayton

[smarterclayton]

The lock comment is probably not accurate - we want to prevent events from
the cache reflector from mutating the state map while we are recalculating
the route, so we lock the r.state object. I.e. if endpoints are removed
while we are iterating over the r.state map, we don't want another
goroutine adding or removing keys from r.state (we'll get a concurrent
mutation error from the Go map).

We've had a long overdue item to refactor the router controller to be an
indexed cache store of routes/ingresses, services, and endpoints. When we
do that, we'll likely make a list call to each cache (which is under each
cache store lock) and then use that list to directly generate the config,
so the template would have access to the list of services / routes /
endpoints, rather than transforming those objects into an intermediate form.

On Thu, Jun 9, 2016 at 11:39 PM, TonyAdo [email protected] wrote:

I am learning origin code, when I scan through router implementation, I
found this lock usage
https://github.com/openshift/origin/blob/master/pkg/router/template/router.go#L233
very confused. From the lock description, it is used to prevent concurrent
router reloads, but I really can't find concurrent router reloads in code,
this reload is only invoked by rateLimitedFunction, it handles events
serially. @smarterclayton https://github.com/smarterclayton

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/issues/9266, or mute the thread
https://github.com/notifications/unsubscribe/ABG_p6Th2gA2tUnpEL5wqRzYgFyOYUxVks5qKNxigaJpZM4IymN4
.

[kargakis]

We've had a long overdue item to refactor the router controller to be an indexed cache store of routes/ingresses, services, and endpoints. When we do that, we'll likely make a list call to each cache (which is under each cache store lock) and then use that list to directly generate the config, so the template would have access to the list of services / routes / endpoints, rather than transforming those objects into an intermediate form.

+1 for this. Do we have an issue to track?

[smarterclayton]

Yes, there are cards on the networking board.

[adohe-zz]

@smarterclayton thanks for detailed explanation.

[openshift-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.

/lifecycle stale