picker: increase range of peer observation

[stevvooe]

To allow the weighting algorithms to work properly, the values of
observation weights need to be selected to be greater than 1. Without
this, the observations, positive or negative, will always converge to 1.
We also relax the smoothing factor to allow the observations to react
faster to changes.

Arguably, we could remove the weight parameter from Observe and then
make the manager state truly discrete but that is a much larger change.

Signed-off-by: Stephen J Day stephen.day@docker.com

cc @LK4D4 @aaronlehmann

[codecov-io]

Current coverage is 54.91% (diff: 16.66%)

Merging #1267 into master will increase coverage by 0.19%

@@             master      #1267   diff @@
==========================================
  Files            78         78          
  Lines         12418      12418          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6795       6819    +24   
+ Misses         4677       4660    -17   
+ Partials        946        939     -7   

Powered by Codecov. Last update 069614f...bbc4243

[aaronlehmann]

Maybe remove the TODO now that we are taking a closer look at this.

[stevvooe]

This is still questionable... and we're probably getting rid of this after the move to grpc.LoadBalancer.

[LK4D4]

LGTM

[LK4D4]

@aaronlehmann true, we should modify test which I wrote to use such ideas, like Observe each twice, then downweight one and see how often it's there.

[aaronlehmann]

I'm wondering if using math.Ceil really makes sense.

For example, when the weight has converged to 10, and we observe with -10, we should end up at 0. But if the math was not exact due to FP rounding, and we ended up with a value like 0.00000001, this would turn into a weight of 1. It wouldn't be a big deal if the domain was large, but since we've limited ourselves to 21 discrete steps, a difference of 1 is quite significant.

I think using floating point weights for internal state would prevent roundoffs from becoming amplified like this.

[aaronlehmann]

Thanks for adding the protection against flakiness. It really sucks when a probablistic test fails 1 in 1000 times because someone felt that was rare enough not to be a problem.

[stevvooe]

This has a probability of 0.5%^20 of failing.

[LK4D4]

@aaronlehmann @stevvooe I'm not sure that I understand algorithm, but test

func TestRemotesDownweight(t *testing.T) {
    peers := []api.Peer{{Addr: "one"}, {Addr: "two"}, {Addr: "three"}}
    index := make(map[api.Peer]struct{}, len(peers))
    remotes := NewRemotes()

    for _, peer := range peers {
        index[peer] = struct{}{}
    }

    for _, p := range peers {
        remotes.Observe(p, 10)
        remotes.Observe(p, 10)
    }

    remotes.Observe(peers[0], -10)

    samples := 100000
    choosen := 0

    for i := 0; i < samples; i++ {
        p, err := remotes.Select()
        if err != nil {
            t.Fatalf("error selecting remote: %v", err)
        }
        if p == peers[0] {
            choosen++
        }
    }
    ratio := float32(choosen) / float32(samples)
    t.Logf("ratio: %f", ratio)
    if ratio > 0.001 {
        t.Fatalf("downweighted peer is choosen too often, ratio: %f", ratio)
    }
}

passes, downweighted peer choosen only in 0.001% cases.

[aaronlehmann]

Should we observe a few times with a positive weight before this downweighting to make sure that we're still unlikely to select this node even if its weight started as > 0 (as it will in practice)?

[stevvooe]

You can either have balanced downweighting have failures immediately reduce selection probability or have less negative than positive to require several observations before it is fully downweighted.

In this case, initial condition is DefaultObservationWeight and we downweight with -DefaultObservationWeight, expecting it to cross zero.

[aaronlehmann]

I missed the initial condition. Where is it set up?

[stevvooe]

The initial loop converges it to DefaultObservationWeight.

[stevvooe]

The tests have been adjusted to reduce failure probability. Another test has been changed to deal with bias reduction, discovered after running 10s of thousands of times.

[aaronlehmann]

comment says 10%

[aaronlehmann]

LGTM