Zone aware routing

[RomanDzhabarov]

@lyft/network-team for review

[mattklein123]

default to on

[mattklein123]

circumvent all of this logic if there is a single zone

[RomanDzhabarov]

made default to on.
single zone should be fine as we either return healthyHosts or localZoneHealthyHosts (should be the same in case of single zone).

[mattklein123]

Yes, but you will do a lot of runtime lookups for no reason. I would definitely short circuit for 1 zone which will be very common.

[RomanDzhabarov]

Yes, that make sense, have not really though that someone will run in one zone

[mattklein123]

let's pick reasonable default here for people without runtime. 10% ? 15% ?

[RomanDzhabarov]

Yep, will set that to something like 3%.
Consider simple case of 1, 2, 2 hosts, we'll get 20%, 40%, 40% and target of 33%.
I'll make that default on a safe side, but we can tune via runtime.

[mattklein123]

We are going to be moving to % scale up and scale down internally, so it's possible we might be more than 3% out of sync. Sure we can start with a small default and then tune.

[mattklein123]

in a perfect world, we don't need to do this calculation every time we LB, only when there is a host set update. Might consider just adding a method on the host set: bool localZoneBalanced() which has this calculation already done.

[RomanDzhabarov]

Agree recalculating is not optimal, but if we do not do recalculation then runtime change will be in effect only after membership changes.
I think calculations are not that intensive, for now extracting logic into method in HostSet::localZoneHealthyBalanced(), then we can easily make that lazy calculated if needed.

[RomanDzhabarov]

i though about this a little bit more, i think the right way would be to have map: zone -> list of hosts instead of localZone array (easy to find out total number of zones, can get distribution of any zone in asg, etc)
if i'd move localZoneHealthyBalanced to HostSet then we'd need a way to know total number of zones (taking that from stat seems a bit convoluted, as HostSet knows about all of the hosts already).
I'd prefer to leave calculations in load_balancer and make bigger refactoring in hostset later.

let me know what you think

[mattklein123]

Yup let's just leave this here for now. I would like to get this into production and start experimenting with it. We can move it out later to gain a bit of perf.

[mattklein123]

minor: invert this if statement, and return all healthy hosts, will reduce one level if logic. This code is already complicated would like to simplify as much as possible

[mattklein123]

I would also short circuit id the # of local zone hosts is zero. This happens for all non-SDS cluster types, and if SDS is misconfigured.

[RomanDzhabarov]

will simplify and add early exit with healthy hosts

[mattklein123]

IIRC unless you cast one of the division arguments to a double, I don't think you are actually getting any floating point accuracy, so you might as well just stick with integer division everywhere.

[RomanDzhabarov]

it's floating point as 100.0 makes the div to be operating on floats. i double checked that.

[mattklein123]

OK I didn't notice the 100.0. Personally, I think the cast is more clear, but whichever way please make the math consistent in this function. So either change the stuff you added or change how the division is done above.

[RomanDzhabarov]

i removed cast in the above, that seem to make the line shorter and reads well

[mattklein123]

same here. See how division is done above for the global panic threshold

[RomanDzhabarov]

this works fine

[mattklein123]

I think this line should be in the if statement below around line 199. Basically only insert when you push into final_hosts?

[RomanDzhabarov]

yep, fixed it.

[mattklein123]

👍