refactor(load-biaser): store penalty_secs as a u16

[cratelyn]

this commit is based upon #4537, but is written in
part as preemptive review feedback related to #4546.

in that change, we must remove the Hash, PartialEq, and Eq
derivations on a number of types throughout the outbound proxy, making
that change fairly involved.

see, for example: 8675c7f

this commit proposes an alteration to the load biaser crate that can
avoid the need for these changes, both minimizing the size of the
unified circuit breaker integration, and avoiding the risk of future
changes introducing hashing or equality bugs should we add other fields
to these structures in the future and neglect to update the manual
Hash implementations.

instead, we can change our model of the load biaser's config. by storing
penalty_secs as an unsigned integer, this structure can now be
compared and hashed. this value is then converted to a floating point
f64 only at the site where we need to perform exponential weighted
moving average arithmetic.

finally, this also provides another benefit: by modeling our
configuration in a way that makes illegal states (negative penalty, NaN
penalty) impossible to represent, we no longer need to check for such
states when constructing a load biasing layer.

X-Ref: #4546
X-Ref: #4537
Signed-off-by: katelyn martin kate@buoyant.io

[unleashed]

That's interesting, although it loses precision that an operator might want to use, in case they precisely wanted 2.5 instead of 2 or 3. We could alternatively use a fixed point implementation, which would overcome the issue and allow Eq/Hash, at the cost of yet-another-dependency.

I'm inclined to say that it's probably ok to reduce the precision and reap the benefits. I'll revisit when working through the changes for biaser.

[unleashed]

Hm, we could also store this as a Duration (impls Eq/Hash), or maybe as milliseconds (using u32). What do you think @cratelyn?

[cratelyn]

Hm, we could also store this as a Duration (impls Eq/Hash), or maybe as milliseconds (using u32). What do you think @cratelyn?

i'm less inclined to pursue Duration, since this ends up being used in a floating point calculation in LoadBiaserFuture rather than being passed to a time-based interface like e.g. tokio::time::sleep.

milliseconds seems like a reasonable choice though, and would align with the annotations proposed in linkerd/linkerd2#15317.

[cratelyn]

That's interesting, although it loses precision that an operator might want to use, in case they precisely wanted 2.5 instead of 2 or 3. We could alternatively use a fixed point implementation, which would overcome the issue and allow Eq/Hash, at the cost of yet-another-dependency.

I'm inclined to say that it's probably ok to reduce the precision and reap the benefits. I'll revisit when working through the changes for biaser.

just to confirm my understanding: fractional values like 2.5 aren't permitted in https://github.com/linkerd/linkerd2/pull/15317/changes#diff-ca93f5418ef6acaecd829ca8e13828305fddd066b46e56c34e3b11c09f40934cR2135-R2153.

so if someone wanted a penalty of two-and-a-half seconds, they would write this as 2500ms, not 2.5s?

[unleashed]

so if someone wanted a penalty of two-and-a-half seconds, they would write this as 2500ms, not 2.5s?

Yes, exactly, although at the parsing and validation layer we could do conversions as needed, the only requirement is that we're able to represent the expected values in the chosen type.

[unleashed]

I'm going to take this and integrate it early on, rebuilding branches because this also has an impact later on that helps simplify code that is basically fighting the lack of Hash/Eq. Thanks @cratelyn!

[cratelyn]

I'm going to take this and integrate it early on, rebuilding branches because this also has an impact later on that helps simplify code that is basically fighting the lack of Hash/Eq. Thanks @cratelyn!

you bet! feel free to merge this eagerly, or cherry-pick this and close it. 🫡

[cratelyn]

this was done in ee9e7228a6.