gce: concurrent zonal List()s + opportunistic basename fill

[bpineau]

FetchAllMigs (unfiltered InstanceGroupManagers.List) is costly as it's not
bounded to MIGs attached to the current cluster, but rather lists all MIGs
in the project/zone, and therefore equally affects all clusters in that
project/zone. Running the calls concurrently over the region's zones (so at
most, 4 concurrent API calls, about once per minute) contains that impact.

findMigsInRegion might be scoped to the current cluster (name pattern),
but also benefits from the same improvement, as it's also costly and
called at each refreshInterval (1mn).

Also: we're calling out GCE mig.Get() API again for each MIG (at ~300ms per
API call, in my tests), sequentially and with the global cache lock held
(when updateClusterState -> ...-> GetMigForInstance kicks in). Yet we
already get that bit of information (MIG's basename) from any other
mig.Get or mig.List call, like the one fetching target sizes. Leveraging
this helps significantly on large fleets (for instance this shaves 8mn
startup time on the large cluster I tested on).

[MaciekPytel]

Opportunistic basename fill only improves the duration of the very first loop I think, because we never invalidate the basename cache. Is that correct? Or am I missing some path where we actually do invalidate it?

The code looks good to me, but I'd like to ask how you tested it and what level of improvement have you seen outside of the first loop (or is it 8m every loop? in that case I'd like to understand why it helps so much).

[bpineau]

That's correct, the baseName change only helps at startup time. Then that cache lives forever.

As for the tests: I first ran this (go -race build) on a pair of small clusters (then filled them with hundreds MIGs) for about a week; and now run that for over 5 days on a dozen clusters ranging from a few dozen nodes to a few thousands.

About the level of improvement: I manually measured startup time benefits (helped by both basename change, and concurrency) for a large cluster having 2k MIGs+IT attached, but didn't check other clusters (I can do if you want me to?).
Non-startup improvements (at every loop, due to concurrent refreshes, not basename) looks like so for instance:

On a 1k nodes / 600 MIGs, not very busy cluster (in a project hosting ~5k MIGs overall, 3 zones, where InstanceGroupManagers.List() takes 10-12s for each zone):

On a 3k+ nodes / 2k MIGs, quite active cluster (also bumped concurrency at the end of 05/11, but that only helped p99):

By the way, the higher percentiles remains high and unstable on the larger clusters: that's probably due any single MIG change causing (at the next minute tick) a complete and blocking instanceRefToMigRef cache invalidation followed by a full list instances refresh for each MIG. That's via Refresh->forceRefresh->fetchAutoMigs->[changed]->RegenerateInstancesCache()->gc.instanceRefToMigRef=make(...).

Perhaps RegenerateInstancesCache() could take a []GceRef slice as argument, and update just those (and gc entries pointing to a MIG unknown by gc.getMigRefs()) only after it fetched instances rather than starting by a full invalidation? The background hourly RegenerateInstancesCache() goroutine spawn by CreateGceManager could then pass all known migs. Unless you suggest otherwise, I'm considering looking into this too.

[MaciekPytel]

/lgtm
/approve

Thanks, that seems like some thorough testing. I don't think faster startup is all that critical, but the loop time improvement is very nice.

Regarding your other ideas - I'd need to dig into code some more, but the overall idea seems reasonable. We've optimized GCE provider while testing it with something like 100 MIGs, but I'm not aware of any prior optimization work targeting a case of 1k+ MIGs. So the impact of forced cache refresh was much lower and there was probably not enough incentive to optimize it.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bpineau, MaciekPytel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• cluster-autoscaler/OWNERS [MaciekPytel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment