Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Cluster-autoscaler: fix for multi-mig autoscaling #1282

Merged
merged 1 commit into from
Jun 28, 2016
Merged

Cluster-autoscaler: fix for multi-mig autoscaling #1282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cluster-autoscaler/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,9 @@ func CheckMigsAndNodes(nodes []*kube_api.Node, gceManager *gce.GceManager) error
if err != nil {
return err
}
if migConfig == nil {
continue
}
url := migConfig.Url()
count, _ := migCount[url]
migCount[url] = count + 1
Expand Down
48 changes: 39 additions & 9 deletions cluster-autoscaler/utils/gce/gce.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package gce
import (
"fmt"
"io"
"strings"
"sync"
"time"

Expand All @@ -40,9 +41,14 @@ const (
operationPollInterval = 100 * time.Millisecond
)

type migInformation struct {
config *config.MigConfig
basename string
}

// GceManager is handles gce communication and data caching.
type GceManager struct {
migs []*config.MigConfig
migs []*migInformation
service *gce.Service
migCache map[config.InstanceConfig]*config.MigConfig
cacheMutex sync.Mutex
Expand Down Expand Up @@ -74,8 +80,15 @@ func CreateGceManager(migs []*config.MigConfig, configReader io.Reader) (*GceMan
return nil, err
}

migInfos := make([]*migInformation, 0, len(migs))
for _, mig := range migs {
migInfos = append(migInfos, &migInformation{
config: mig,
})
}

manager := &GceManager{
migs: migs,
migs: migInfos,
service: gceService,
migCache: map[config.InstanceConfig]*config.MigConfig{},
}
Expand Down Expand Up @@ -164,13 +177,22 @@ func (m *GceManager) GetMigForInstance(instance *config.InstanceConfig) (*config
if mig, found := m.migCache[*instance]; found {
return mig, nil
}
if err := m.regenerateCache(); err != nil {
return nil, fmt.Errorf("Error while looking for MIG for instance %+v, error: %v", *instance, err)
}
if mig, found := m.migCache[*instance]; found {
return mig, nil

for _, mig := range m.migs {
if mig.config.Project == instance.Project &&
mig.config.Zone == instance.Zone &&
strings.HasPrefix(instance.Name, mig.basename) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During the first run of this loop basename will be empty and it will always be a prefix and we'll return a prefix. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache is 'regenerated' in the beginning, so this could happen only in case of race condition (with very low probability).

if err := m.regenerateCache(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be readable if you restructure the code such that:

  1. in a for loop check if we have any corresponding loop
  2. if we have a corresponding mig regenerate cache and return mig
  3. if don't have it return nils

return nil, fmt.Errorf("Error while looking for MIG for instance %+v, error: %v", *instance, err)
}
if mig, found := m.migCache[*instance]; found {
return mig, nil
}
return nil, fmt.Errorf("Instance %+v does not belong to any configured MIG", *instance)
}
}
return nil, fmt.Errorf("Instance %+v does not belong to any known MIG", *instance)
// Instance doesn't belong to any configured mig.
Copy link
Contributor

@fgrzadkowski fgrzadkowski Jun 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain why it's ok.

return nil, nil
}

func (m *GceManager) regenerateCacheIgnoreError() {
Expand All @@ -184,8 +206,16 @@ func (m *GceManager) regenerateCacheIgnoreError() {
func (m *GceManager) regenerateCache() error {
newMigCache := map[config.InstanceConfig]*config.MigConfig{}

for _, mig := range m.migs {
for _, migInfo := range m.migs {
mig := migInfo.config
glog.V(4).Infof("Regenerating MIG information for %s %s %s", mig.Project, mig.Zone, mig.Name)

instanceGroupManager, err := m.service.InstanceGroupManagers.Get(mig.Project, mig.Zone, mig.Name).Do()
if err != nil {
return err
}
migInfo.basename = instanceGroupManager.BaseInstanceName

instances, err := m.service.InstanceGroupManagers.ListManagedInstances(mig.Project, mig.Zone, mig.Name).Do()
if err != nil {
glog.V(4).Infof("Failed MIG info request for %s %s %s: %v", mig.Project, mig.Zone, mig.Name, err)
Expand Down