SubchannelsLoadBalancer.UpdateChannelState() with the same Addresses but different BalancerAddress.Attributes doesn't trigger UpdateBalancingState

[kolonist]

Hi, @JamesNK

It seems that there is an issue in SubchannelsLoadBalancer.UpdateChannelState().

When I call SubchannelsLoadBalancer.UpdateChannelState(state) with the state which consists of the same addresses but with different attributes as in previous call then UpdateChannelState does not update subchannels at all.

Reproduction

1. Create N addresses with some attributes:

       // create 2 addresses with some attributes
       var address1 = new BalancerAddress(host1, port);
       address1.Attributes.TryAdd(attributeKey, 20); // <-- attribute
   
       var address2 = new BalancerAddress(host2, port);
       address2.Attributes.TryAdd(attributeKey, 80); // <-- attribute
   

2. Create state based on these addresses:

   var state1 = new ChannelState(
       status:              new Status(),
       addresses:           [address1, address2],
       loadBalancingConfig: null,
       attributes:          new BalancerAttributes());
   

3. Update channel state:

   balancer.UpdateChannelState(state1);
   

4. Create another N addresses with the same hosts and ports but another attributes:

   var address3 = new BalancerAddress(host1, port);
   address3.Attributes.TryAdd(attributeKey, 40); // <-- difference
   
   var address4 = new BalancerAddress(host2, port);
   address4.Attributes.TryAdd(attributeKey, 60); // <-- difference
   

5. Create new state based on new addresses:

   var state2 = new ChannelState(
       status:              new Status(),
       addresses:           [address3, address4],
       loadBalancingConfig: null,
       attributes:          new BalancerAttributes());
   

6. Update channel state:

   balancer.UpdateChannelState(state2);
   

7. Make sure that channels left unchanged

Where is the problem?

I believe that the problem is in channel updates check code:

public override void UpdateChannelState(ChannelState state)
{
    ...

    var allUpdatedSubchannels = new List<AddressSubchannel>();
    var newSubchannels = new List<Subchannel>();
    var currentSubchannels = _addressSubchannels.ToList();

    foreach (var address in state.Addresses)
    {
        var i = FindSubchannelByAddress(currentSubchannels, address); // <-- Found because addresses are the same

        AddressSubchannel newOrCurrentSubchannel;

        if (i != null) // <-- Not null because addresses are the same
        {
            // <-- Step into this block because addresses are the same

            newOrCurrentSubchannel = currentSubchannels[i.Value];

            currentSubchannels.RemoveAt(i.Value); // <-- Always remove

            // <-- True because attributes are different
            if (!BalancerAddressEqualityComparer.Instance.Equals(address, newOrCurrentSubchannel.Address))
            {
                // <-- Step into this block because attributes are different

                newOrCurrentSubchannel = new AddressSubchannel(
                    newOrCurrentSubchannel.Subchannel,
                    address,
                    newOrCurrentSubchannel.LastKnownState);
                newOrCurrentSubchannel.Subchannel.UpdateAddresses(new[] { address });
            }

            ...
        }
        else
        {
            // <-- Never step into this block
            ...
        }

        allUpdatedSubchannels.Add(newOrCurrentSubchannel);
    }

    var removedSubConnections = currentSubchannels; // <-- Empty because addresses are the same

    // <-- `newSubchannels` is empty because we newer touched it in the code above
    if (removedSubConnections.Count == 0 && newSubchannels.Count == 0)
    {
        // <-- Always step here because `removedSubConnections` and `newSubchannels` are empty

        // <-- Do nothing except some logging
        ...

        return; // <-- Always return here
    }

    ....

    // <-- Newer came here
    UpdateBalancingState(state.Status);

I created simple test in your repository fork to shot the problem: https://github.com/kolonist/grpc-dotnet/blob/SubchannelsLoadBalancerTests/test/Grpc.Net.Client.Tests/Infrastructure/Balancer/SubchannelsLoadBalancerTests.cs

Main test is the first - UpdateChannelState_Should_Update_channel_state_with_different_attributes_only.

Another two tests show that different count of addresses leads to real update of balancing state.

System info

What version of gRPC and what language are you using? - All versions, including 2.66.x

What operating system (Linux, Windows,...) and version? - All versions. Tested on MacOS arm64 and Linux intel x64

What runtime / compiler are you using (e.g. .NET Core SDK version dotnet --info)

SDK:
 Version:           8.0.401
 Commit:            811edcc344
 Workload version:  8.0.400-manifests.57f7c351
 MSBuild version:   17.11.4+37eb419ad

Environment:
 OS Name:     Mac OS X
 OS Version:  14.4
 OS Platform: Darwin
 RID:         osx-arm64
 Base Path:   /usr/local/share/dotnet/sdk/8.0.401/

Host:
  Version:      8.0.8
  Architecture: arm64

.NET SDKs installed:
  8.0.401 [/usr/local/share/dotnet/sdk]

.NET runtimes installed:
  Microsoft.NETCore.App 8.0.8 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]

Other architectures found:
  x64   [/usr/local/share/dotnet/x64]
    registered at [/etc/dotnet/install_location_x64]

Environment variables:
  Not set

global.json file:
  Not found

[kalduzov]

Hi, @JamesNK

Please take a look at this issue. Its importance is that it is impossible to correctly balance a weighted round robin when the endpoint weights are dynamically changing.

[JamesNK]

PR with what I think is the fix you're looking for: #2553

I modified one of thes test from #2550 and included it.