Support autodetection of GCE managed instance groups by name prefix #462
Conversation
k8s-ci-robot
added
the
size/L
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
@k8s-ci-robot Signed. |
k8s-ci-robot
added
cncf-cla: yes
|
Some elaboration on the use case for this PR: at Planet Labs we deploy a cluster on GCE as a regional managed instance group of N master nodes, with one or more zonal worker managed instance group per zone in the region. We've previously used the AWS implementation of I've tested this by deploying the following: I see the following in the logs: The autoscaler then proceeded to scale my idle node pools down to zero as expected. |
|
Hi @negz, This alternative approach, that we think is the way to go, is using CloudProvider.Refresh() method to poll cloudprovider and update the list of NodeGroups. We already use this approach for GKE, so we know CA can handle NodeGroups dynamically changing this way (this was not true at the time PollingAutoscaler was implemented). We believe this is a better approach than re-creating StaticAutoscaler every loop, which is expensive and potentially has hard to predict side effects. Also this way we avoid losing all internal state once the config actually changes (think resetting unneeded nodes timers, but also losing internal caches impacting performance). Implementing this feature would require changing your code so it doesn't use PollingAutoscaler, storing I'm happy to have a more detailed discussion, or help you in any way required. Feel free to ping me on slack if you want to have a chat (warning: me and all other CA maintainers are in CET timezone, so we may have limited overlap). |
|
@MaciekPytel That sounds like a better implementation. I'm happy to take on migrating both the existing AWS and proposed GCE autodetection code to your proposed pattern. I'll do so in a separate commit in this PR for now. |
k8s-ci-robot
added
size/XXL
negz
force-pushed
the
gcedisco
branch
2 times, most recently
from
e7e4a11 to
45317a6
Compare
|
@MaciekPytel I've removed the PollingAutoscaler and moved both the existing AWS and proposed GCE node group autodiscovery code into the All node group autodiscovery ( I've tested this on both AWS and GCE. I see that node groups are autodiscovered and/or explicitly registered as configured, and are successfully scaled up/down as needed. |
|
@negz I've started reading this PR, but it will take me a few days given the size of it. Also can you describe your testing in some more detail (an example of what I mean is in FAQ: https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/FAQ.md#how-should-i-test-my-code-before-submitting-pr). @mumoshu Do you want to review this as well? |
|
@MaciekPytel Understood. Testing wise I've completed steps 2 and 3 of the steps you linked. In more detail:
In GCE I ran the CA with these flags: In AWS I used: I used the following dummy deployment to trigger scale up: |
|
Here's some logs from AWS demonstrating the node groups being discovered using a combination of Here's an AWS scale-up using just I'm afraid I don't have the logs from my GCE tests in my scrollback, but it's not too difficult to run them again if they'd be useful. |
| return nil, err | ||
| } | ||
|
|
||
| cfgs, err := discoveryOpts.ParseASGAutoDiscoverySpecs() |
There was a problem hiding this comment.
nit: specs rather than cfgs for consistency?
There was a problem hiding this comment.
Sure. I was looking for something to differentiate from the --nodes 'spec', which is a string, but they're both specs in a way. I don't feel strongly - will change.
| interrupt: make(chan struct{}), | ||
| service: *service, | ||
| asgs: asgs, | ||
| autoASGs: cfgs, |
There was a problem hiding this comment.
nit: asgAutoDiscoverySpecs or autoDiscoverySpecs rather than autoASGs for consistency?
| @@ -80,41 +87,179 @@ func createAWSManagerInternal(configReader io.Reader, service *autoScalingWrappe | |||
| } | |||
| } | |||
|
|
|||
| asgs, err := newAutoScalingGroups(*service) | |||
There was a problem hiding this comment.
Perhaps autoScalingGroups and newAutoScalingGroups should be renamed to cachedAutoScalingGroups and newCachedAutoScalingGroups respectively?
Because I occasionally confuse autoScalingGroups as a cache with a list of asgs 😉
| m.notInRegisteredAsg = make(map[AwsRef]bool) | ||
| m.instanceToAsgMutex.Unlock() | ||
| } | ||
|
|
||
| func (m *autoScalingGroups) regenerateCache() error { |
There was a problem hiding this comment.
If you agree with renaming autoScalingGroups to cachedAutoScalingGroups, I guess we can rename this func to just regenerate which looks like cachedAsgs.regenerate() as a whole when called.
There was a problem hiding this comment.
I like this - will do.
|
|
||
| func (m *autoScalingGroups) get() []*asgInformation { | ||
| m.registeredAsgsMutex.RLock() | ||
| defer m.registeredAsgsMutex.RUnlock() |
There was a problem hiding this comment.
Just curious but what does this RLock prevents us from?
At glance, this lock seemed to do almost nothing as we don't mutate registeredAsgs in this func.
There was a problem hiding this comment.
You're correct that we don't mutate registeredAsgs here, but we do elsewhere (i.e. in Register and Unregister). We need to take a lock when reading memory - not only when writing it. This RLock is intended to prevent us reading registeredAsgs while another goroutine is writing to it.
For example if play.go were as follows:
package main
import (
"fmt"
"sync"
"time"
)
func main() {
s := make([]int, 0)
m := sync.RWMutex{}
go func() {
for i := 0; ; i++ {
m.Lock()
s = append(s, i)
m.Unlock()
time.Sleep(1 * time.Second)
}
}()
for {
fmt.Println(s)
time.Sleep(1 * time.Second)
}
}$ go run -race play.go
[]
==================
WARNING: DATA RACE
Write at 0x00c420092020 by goroutine 6:
main.main.func1()
/Users/negz/control/go/src/play.go:15 +0xf4
Previous read at 0x00c420092020 by main goroutine:
main.main()
/Users/negz/control/go/src/play.go:21 +0x158
Goroutine 6 (running) created at:
main.main()
/Users/negz/control/go/src/play.go:12 +0x147
================== Adding an m.RLock() and m.RUnlock() around fmt.Println(s) prevents the data race.
Note that maybe I'm missing something and these methods are in fact only ever called by a single goroutine, in which case all this locking is pointless. :)
There was a problem hiding this comment.
It seems like I had not understood the use-case of RLock/RUnlock.
Thanks for the clear explanation!
| // Register ASG. Returns true if the ASG was registered. | ||
| func (m *autoScalingGroups) Register(asg *Asg) bool { | ||
| m.registeredAsgsMutex.Lock() | ||
| defer m.registeredAsgsMutex.Unlock() |
There was a problem hiding this comment.
After reading throughout code, I began to wonder if what we wanted was a single mutex for all the set of internal states including registeredAsgs, instanceToAsg, notInRegisteredAsgs?
For me migrating to the single mutex would result in cleaner code around locking and less functions(regenerateCacheWithoutLock would become unnecesary)? 🤔
I suppose the single mutex could be renamed stateMutex.
There was a problem hiding this comment.
I agree a single mutex would make the code cleaner, but on the surface it seems that less granular locking could result in situations where a caller is waiting for a lock unnecessarily, for example not being able to find an ASG for an instance while also registering an ASG.
That said, I mostly added a second mutex in imitation of the two-mutex pattern in the GCE manager. It's quite possible this could be premature optimisation.
Currently we use the following mutexes in the 'autoscaling group cache':
registeredAsgMutex:
- Taken by a single goroutine when registering an ASG
- Taken by a single goroutine when unregistering an ASG
- Taken by one or more goroutines when listing ASGs
- Taken by one or more goroutines when regenerating the instance to ASG cache
instanceToAsgMutex:
- Taken by a single goroutine when finding the ASG for an instance
- Taken by a single goroutine when invalidating the instance to ASG cache
- Taken by a single goroutine when regenerating the instance to ASG cache
As far as I know there's only two goroutines competing for these mutexes, namely the one used by the RunOnce loop, and the one we spawn to regenerate the instance to ASG cache hourly. I'm new to the codebase so it's not unlikely that there's other goroutines I'm unaware of.
At the end of the day this is not something I feel strongly about. I'm curious if anyone else has an opinion?
There was a problem hiding this comment.
Note that I don't think using a single mutex would remove the need for regenerateCacheWithoutLock. Even with a single mutex we'd still have two cases in which we need to regenerate the cache:
- From inside
FindForInstance, where we have already taken a lock and thus can't take it again. - From everywhere else (i.e. registering an instance, unregistering an instance, regenerating the cache hourly).
Previously we dealt with this by all the 'everywhere else' cases taking the mutex explicitly. i.e. The callers would do:
m.registeredAsgsMutex.Lock()
defer m.registeredAsgsMutex.Unlock()
m.regenerateCache()This puts the responsibility of dealing with locking on the callers rather than the method that actually needs the lock. This is especially messy when the caller is in the AWS manager, not the autoscaling group cache struct - it leaks implementation details of the autoscaling group cache up to the AWS manager to the point that it's almost pointless for them to be two different things.
There was a problem hiding this comment.
Thanks for clarifying again!
for example not being able to find an ASG for an instance while also registering an ASG.
Yes - indeed that sounds problematic in larger clusters.
I'm now inclined to have fine-grained locks as you've done 👍
There was a problem hiding this comment.
I ended up reverting to a single lock after all. :) I ran into a deadlock bug in the AWS provider and after spending some time trying to debug it I was convinced that a single mutex would make life easier.
| glog.Errorf("Error while regenerating Asg cache: %v", err) | ||
| } | ||
| }, time.Hour, manager.interrupt) | ||
| if err := manager.fetchExplicitAsgs(discoveryOpts.NodeGroupSpecs); err != nil { |
There was a problem hiding this comment.
For me, it wasn't very straight-forward to understand why we have to pass auto discovery specs and "explicit" discovery specs separately like this.
Can we pass the discoverySpecs.NodeGroupSpecs while creating the AwsManager struct above, so that it looks more consistent between auto vs explicit discovery specs in regard to those two are both about discovery?
There was a problem hiding this comment.
We certainly could store discoverySpecs.NodeGroupSpecs in the AwsManager, but it would only be for consistency's sake because we need fetch 'explicit' node groups only once - at the creation of the AwsManager.
I think you're suggesting an alternative like this:
func createAWSManagerInternal(...) (*AwsManager, error) {
manager := &AwsManager{
service: *service,
asgs: asgs,
autoASGs: cfgs,
explicitASGs: discoveryOpts.NodeGroupSpecs,
neverUnregister: make(map[AwsRef]bool),
}
manager.fetchExplicitAsgs()
}To me this alternative makes it less obvious that discoveryOpts.NodeGroupSpecs is only consumed once, by fetchExplicitAsgs().
In this PR we have three 'flavours' of spec:
- A regular
--nodes' 'min:max:namestyle spec.NodeGroupSpecsis a[]stringslice of these specs. Each of these specs is passed todynamic.SpecFromStringwhenfetchExplicitAsgsis called. - A
--node-group-autodiscoveryASG spec, likeasg:tag=coolTag. This is parsed into an[]ASGAutoDiscoveryConfigslice bydiscoveryOpts.ParseASGAutoDiscoverySpecs(). - A
-node-group-autodiscoveryMIG spec likemix:prefix=coolPrefix,minNodes=0,maxNodes=100. This is parsed into a[]MigAutoDiscoveryConfigslice bydiscoveryOpts.ParseMIGAutoDiscoverySpecs().
These three can never be completely consistent, because they do slightly different things:
- Contains everything we need to know to register in ASG/MIG, and is only consumed once.
- and 3. instead contain inputs to discover ASGs/MIGs to register, and are consumed repeatedly over the life of the CA.
Talking through this, I wonder whether dynamic.NodeGroupSpec should be merged with NodeGroupDiscoveryOptions. This would move all of the flag/'spec' parsing code up to NodeGroupDiscoveryOptions.
This would look something like:
func (o NodeGroupDiscoveryOptions) ParseExplicitSpecs() ([]NodeGroupSpec, error) {}
func (m *AwsManager) fetchExplicitAsgs(specs cloudprovider.NodeGroupSpec[]) error {}What do you think? Or is there some other approach we could take to make this clearer?
cluster-autoscaler/main.go
Outdated
| flag.Var(&nodeGroupAutoDiscoveryFlag, "node-group-auto-discovery", "One or more definition(s) of node group auto-discovery. "+ | ||
| "A definition is expressed `<name of discoverer>:[<key>[=<value>]]`. "+ | ||
| "The `aws` and `gce` cloud providers are currently supported. AWS matches by ASG tags, e.g. `asg:tag=tagKey,anotherTagKey`. "+ | ||
| "GCE matches by IG prefix, and requires you to specify min and max nodes per IG, e.g. `mig:prefix=pfx,min=0,max=10` "+ |
There was a problem hiding this comment.
Could we make it look like mig:namePrefix=... rather than mig:prefix=...?
The context is that I've initially made asg:tag=... as so because clearly it was about asgs' tag.
On the other hand, for me, mig:prefix=... seems a bit unclear what the prefix is about.
Of course it is the prefix of MIG name but if you'd like to make it even more clearer, I guess it will be good idea to name it something like namePrefix. Just my two cents.
There was a problem hiding this comment.
Good idea - clearer is better.
(Except for all the Java style LongVariableNames in this codebase, which make baby Gophers cry. 
)
|
@MaciekPytel FYI, I'm going to hold off on addressing @mumoshu 's comments until I hear from the Google side of the world so that I can put some time aside to address all comments at once. |
|
@negz Sure, makes sense. I'm halfway through reading, I'll try to finish by tomorrow end of day. Sorry for the delay. |
|
Just rebased to account for the reintroduced Azure provider. |
| glog.Fatalf("Failed to create GCE Manager: %v", err) | ||
| } | ||
|
|
||
| p, err := gce.BuildGceCloudProvider(m, rl) |
There was a problem hiding this comment.
I'd rather avoid one letter variable names. manager is not very long and makes it obvious what is getting passed to where.
There was a problem hiding this comment.
Done.
I tend to follow https://github.com/golang/go/wiki/CodeReviewComments#variable-names, but I realise it was a bit cheeky to stray from the established pattern in this codebase. :)
There was a problem hiding this comment.
You make a good point, but, as you also pointed out, I'd rather keep the codebase consistent.
And, personally, it's one of thing in golang that I can't get myself to accept. And I've never been a java developer :)
| // Technically we're both ProviderNameGCE and ProviderNameGKE... | ||
| // Perhaps we should return a different name depending on | ||
| // gce.gceManager.getMode()? | ||
| return ProviderNameGCE |
There was a problem hiding this comment.
I don't think we even use this anywhere. That being said your comment sounds like the right thing to do.
There was a problem hiding this comment.
Ack. I'll leave it as is for now given it's unused unless you feel strongly.
| if mig, found := m.migCache[*instance]; found { | ||
| return mig, nil | ||
| } | ||
| return nil, fmt.Errorf("Instance %+v does not belong to any configured MIG", *instance) |
There was a problem hiding this comment.
There is no difference in what this function does before and after your changes. While I fully agree that your version is more readable I'd rather avoid drive-by fixes (especially in a PR that is already as big as this one).
| return m.regenerateCacheWithoutLock() | ||
| } | ||
|
|
||
| func (m *gceManagerImpl) regenerateCacheWithoutLock() error { |
There was a problem hiding this comment.
Same comment as above regarding drive-by changes, except in this case I'm also less in favor of the change itself. Arguably if you have 2 versions of a method, which differ by whether they take a lock or not, you're not actually hiding the implementation. The user still needs to think about the lock and most likely look into the implementation to figure out which lock it is and whether they're holding it already (also the fact that cache even exists is an implementation detail and so regenerateCache shouldn't need to be called from outside this file).
I feel sort of neutral regarding where the mutex is handled and I wouldn't mind it in new code. But I don't feel it's worth the increase in PR size to change it.
There was a problem hiding this comment.
Fair enough - will revert.
| @@ -809,62 +819,169 @@ func (m *gceManagerImpl) getTemplates() *templateBuilder { | |||
| } | |||
|
|
|||
| func (m *gceManagerImpl) Refresh() error { | |||
| if m.mode == ModeGCE { | |||
| if m.lastRefresh.Add(refreshInterval).After(time.Now()) { | |||
There was a problem hiding this comment.
This is purely out of curiosity, no change required. Previously it was:
if condition {
return action()
}
return nil
Your version is:
if !condition {
return nil
}
return action()
Any particular reason for changing it? As I said I'm fine with either version, just curious.
There was a problem hiding this comment.
I'm afraid I don't recall. 🤔 It's likely I switched it around while experimenting with a different implementation and it ended up getting left like that.
|
|
||
| for _, arg := range strings.Split(tokens[1], ",") { | ||
| kv := strings.Split(arg, "=") | ||
| k, v := kv[0], kv[1] |
| name: "PrefixDoesNotCompileToRegexp", | ||
| specs: []string{"mig:prefix=a),min=1"}, | ||
| wantErr: true, | ||
| }, |
There was a problem hiding this comment.
Add error case for missing fields (eg. "mig:min=3,max=8") and maybe for min > max?
| return cfg, fmt.Errorf("unsupported key \"%s\" is specified for discoverer \"%s\". Supported keys are \"%s\"", k, discoverer, validMIGAutoDiscovererKeys) | ||
| } | ||
| } | ||
| return cfg, nil |
There was a problem hiding this comment.
I think we should do a few more checks on the input:
- make sure prefix is actually specified
- make sure max > 0 and max >= min (i'm mainly thinking about the case of someone not providing max)
| return cfg, fmt.Errorf("Unsupported discoverer specified: %s", discoverer) | ||
| } | ||
| param := tokens[1] | ||
| paramTokens := strings.Split(param, "=") |
There was a problem hiding this comment.
check len(paramTokens)
| map[string]int64{cloudprovider.ResourceNameCores: options.MaxCoresTotal, cloudprovider.ResourceNameMemory: options.MaxMemoryTotal}, | ||
| ), | ||
| ) | ||
| expanderStrategy, err := factory.ExpanderStrategyFromString(options.ExpanderName, cloudProvider, listerRegistry.AllNodeLister()) |
There was a problem hiding this comment.
Why change this formatting?
There was a problem hiding this comment.
Unintentional, I think. Pretty sure I had split it out a bit so I could better follow what it was doing and forgot to put it back. Happy to revert.
|
@negz Really sorry it took me that long :( Overall it looks good, though I'd prefer to avoid (or at least significantly limit) code style drive-by fixes in a PR that is already so large and complex (as much as I agree with most of the clean ups). |
| } | ||
| glog.V(2).Infof("Refreshed NodePool list, next refresh after %v", nextRefreshAfter) | ||
| } | ||
| m.lastRefresh = time.Now() |
There was a problem hiding this comment.
I don't like logging one timestamp and than actually using a different one - it will be really confusing to someone debugging this in the future. We're doing multiple API calls in the meantime, so the difference is likely to be noticeable.
Maybe we can just log something like "Finished refreshing config, next refresh after" at the end? We can still keep old log messages, just without the "next refresh after" part.
There was a problem hiding this comment.
I figured this wasn't too bad since technically it said the next refresh would happen "after" the time, not "at" the time. ;) Fixed anyhow.
|
Just gave this a final test in AWS and GCE. As best I can tell everything is working as expected. I setup CA on the AWS and GCE cloud providers with node group autodiscovery. I also configured one of the candidate node groups for autodiscovery explicitly using
AWS config: - ./cluster-autoscaler
- --v=4
- --stderrthreshold=info
- --cloud-provider=aws
- --skip-nodes-with-local-storage=false
- --expander=least-waste
- --balance-similar-node-groups=true
- --node-group-auto-discovery=asg:tag=k8s.io/cluster-autoscaler/enabled,kubernetes.io/cluster/$(CLUSTER_NAME)
- --nodes=1:10:kk-c8a9-pool-worker-f821-AutoScalingGroup-8AQYAA49ML9XGCE config: AWS autodiscovering stuff: GCE autodiscovering stuff: AWS autoscaler status: GCE autoscaler status: AWS scaling up: GCE scaling up: AWS scaling down: GCE scaling down: AWS unregistering a deleted ASG: GCE unregistering a deleted MIG: Note that when testing unregistration I deleted the node group explicitly configured using |
| continue | ||
| } | ||
| updated = append(updated, existing) | ||
| changed = true |
There was a problem hiding this comment.
Just noticed that during final reading - shouldn't changed=true happen inside if, just before continue?
|
4 commits is fine. Also I wish all our contributors provided such a good description of performed tests. Added one more comment I noticed during final re-reading though - can you take a look if I'm right? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@MaciekPytel Thanks for the LGTM! What's the next step to get this merged? |
|
Sorry to leave you without comment like that. Normally I just merge PRs after lgtm-ing them, but in this case we're already very late into 1.1.0 testing and this is a very large PR. After discussing with @mwielgus we'd prefer to merge this after we release 1.1.0 and instead include it in 1.2.0-beta1, which we're planning to release next week. Is that ok with you? |
|
We decided that there would be too little time to properly test the PR after merging to approve it for final K8S 1.9 version (CA1.1.0), which was cut today. Anyway, we really like your PR and it will be released soon. |
|
Sounds good! I'm not really in a hurry to be honest, just itching to close out my internal issue tracking this work. ;) |
negz
force-pushed
the
gcedisco
branch
2 times, most recently
from
d70ecbb to
ef05fe9
Compare
|
Just rebased on master to resolve some merge conflicts. |
This commit adds a new usage of the --node-group-auto-discovery flag intended for use with the GCE cloud provider. GCE instance groups can be automatically discovered based on a prefix of their group name. Example usage: --node-group-auto-discovery=mig:prefix=k8s-mig,minNodes=0,maxNodes=10 Note that unlike the existing AWS ASG autodetection functionality we must specify the min and max nodes in the flag. This is because MIGs store only a target size in the GCE API - they do not have a min and max size we can infer via the API. In order to alleviate this limitation a little we allow multiple uses of the autodiscovery flag. For example to discover two classes (big and small) of instance groups with different size limits: ./cluster-autoscaler \ --node-group-auto-discovery=mig:prefix=k8s-a-small,minNodes=1,maxNodes=10 \ --node-group-auto-discovery=mig:prefix=k8s-a-big,minNodes=1,maxNodes=100 Zonal clusters (i.e. multizone = false in the cloud config) will detect all managed instance groups within the cluster's zone. Regional clusters will detect all matching (zonal) managed instance groups within any of that region's zones.
The Build method was getting pretty big, this hopefully makes it a little more readable. It also fixes a few minor error shadowing bugs.
Node group discovery is now handled by cloudprovider.Refresh() in all cases. Additionally, explicit node groups can now be used alongside autodiscovery.
|
Checking in now that 1.9 is out. :) Again, no huge pressure from my end. I mostly just want to get this in before I have to rebase again. |
|
/lgtm |
| cacheMutex sync.Mutex | ||
| instancesNotInManagedAsg map[AwsRef]struct{} | ||
| service autoScalingWrapper | ||
| const scaleToZeroSupported = false |
There was a problem hiding this comment.
Why was scale to zero disabled for AWS?
There was a problem hiding this comment.
Capturing discussion we had elsewhere: this was totally accidental - sorry!
Kueue performance tests
This commit adds a new usage of the
--node-group-auto-discoveryflag intended for use with the GCE cloud provider. GCE instance groups can be automatically discovered based on a prefix of their group name. Example usage:Note that unlike the existing AWS ASG autodetection functionality we must specify the min and max nodes in the flag. This is because MIGs store only a target size in the GCE API - they do not have a min and max size we can infer via the API.
In order to alleviate this limitation a little we allow multiple uses of the autodiscovery flag. For example to discover two classes (big and small) of instance groups with different size limits:
Zonal clusters (i.e.
multizone = falsein the cloud config) will detect all managed instance groups within the cluster's zone. Regional clusters will detect all matching (zonal) managed instance groups within any of the cluster's region's zones.