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

priority: release references to child policies which are removed #5682

Merged
merged 3 commits into from Oct 6, 2022

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Oct 4, 2022

Summary of changes:

  • priority balancer
    • remove child policy wrapper from the children map when the child is removed from the configuration
    • stop caching balancer state from child policies which are not started
    • store the balancer name in the child policy wrapper instead of the balancer.Builder and pass the former to the balancergroup when creating child policies
    • wrap the balancer.ClientConn passed in from the parent before passing it to the children
      • this wrapper ignores ResolveNow() calls from children who are configured with the IgnoreReresolutionRequests field set
      • this wrapper also supports updating this field based on changes in configuration
  • balancergroup:
    • add a new API to add child policies to the group. Once other lb policies are updated to use this API, it will replace the existing Add API
  • clusterresolver:
    • release reference to cached child policy state in Close()

RELEASE NOTES:

  • priority: fix a bug where unreleased references to removed child policies (and associated state) was causing a memory leak

@easwars easwars added this to the 1.50 Release milestone Oct 4, 2022
@easwars easwars requested a review from dfawley October 4, 2022 00:06
@dfawley dfawley modified the milestones: 1.50 Release, 1.51 Release Oct 4, 2022
Comment on lines +305 to +306
//
// TODO: Get rid of the existing Add() API and replace it with this.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In internal I'm probably fine with this, but be cautious about putting TODOs in godoc strings.

var sbc *subBalancerWrapper
// If outgoingStarted is true, search in the cache. Otherwise, cache is
// guaranteed to be empty, searching is unnecessary.
if bg.outgoingStarted {
if old, ok := bg.balancerCache.Remove(id); ok {
sbc, _ = old.(*subBalancerWrapper)
if sbc != nil && sbc.builder != builder {
if sbc != nil && sbc.builder.Name() != builder.Name() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pointer comparison should be fine here, right? Especially if both were pulled from the registry?

String comparisons are slower, so it's probably better to still use the pointer, but this is pretty minor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, and since we are planning to change the balancergroup API to take the name instead of a balancer.Builder, I thought name comparison was still OK here instead of the pointer. Do you have any scenarios in mind where things can go south if we do name comparisons until we switch the balancergroup API?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't matter, as the balancer should always come from the registry, and if it doesn't, it should also have a unique name for the builder instance. I'm not sure if we're following that in all places where we use it, but my guess is the last item is the one least likely to be followed, which would also break this version of the code. The pointer comparison is more optimized and results in simpler code (possibly? it's shorter, anyway), so I do think it would be better to keep it how it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted.

@@ -162,6 +162,9 @@ func (b *priorityBalancer) handleChildStateUpdate(childName string, s balancer.S
b.logger.Warningf("priority: child balancer not found for child %v", childName)
return
}
if !child.started {
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe at least an info log about receiving an update from a child we stopped? It's a race but could be of interest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I added it as a warning log because:
a. it is not a very common event, so won't flood the logs
b. if at all we are looking at a problem that has already happened and we have logs, there is more chance of this statement showing up as a warning log than as an info log.

//
// `ignore` can be updated later by `updateIgnoreResolveNow`, and the update
// will be propagated to all the old and new balancers built with this.
func newIgnoreResolveNowBalancerBuilder(bb balancer.Builder, ignore bool) *ignoreResolveNowBalancerBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so much more direct now! ➕💯

@dfawley dfawley assigned easwars and unassigned dfawley Oct 6, 2022
@easwars easwars assigned dfawley and unassigned easwars Oct 6, 2022
@easwars easwars merged commit c03925d into grpc:master Oct 6, 2022
1 check passed
@easwars easwars deleted the directpath_prober_memory_leak branch October 6, 2022 20:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants