-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Visualization: current version differences for 1.30 - master #3
base: cluster-autoscaler-release-1.30
Are you sure you want to change the base?
Visualization: current version differences for 1.30 - master #3
Conversation
} | ||
// we fetch both sets of resources since CAS may operate on mixed nodepools | ||
m.virtualMachines = vmResult | ||
m.vmsPoolSet = vmsPoolSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do double (VM + VMSS fetches) all the time?
But it is already merged it seems. Might need a bit of reevaluation if to be cherry-picked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This intersting part is already in, so, safe
// couldn't find instance in the cache, assume it's deleted | ||
return false, cloudprovider.ErrNotImplemented | ||
} | ||
|
||
// FindForInstance returns node group of the given Instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HastInstance()
obviously.
@@ -71,6 +71,8 @@ type azureCache struct { | |||
// vmType can be one of vmTypeVMSS (default), vmTypeStandard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Most of this file unless specified) VMs pool
@@ -18,18 +18,31 @@ package azure | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not recognize all in this file. Need evaluation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Match https://github.com/kubernetes/autoscaler/pull/6863/files exactly.
@@ -37,13 +37,13 @@ If you are using `nodeSelector`, you need to tag the VMSS with a node-template | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only trivial changes in this file.
@@ -24,6 +24,16 @@ import ( | |||
"os" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Most of this file unless specified) VMs pool
if !strings.HasPrefix(node.Spec.ProviderID, "azure://") { | ||
return false, fmt.Errorf("invalid azure ProviderID prefix for node: %s, skipped", node.Name) | ||
} | ||
return azure.azureManager.azureCache.HasInstance(node.Spec.ProviderID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HasInstance()
} else { | ||
cfg.GetVmssSizeRefreshPeriod = VmssSizeRefreshPeriodDefault | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetVmssSizeRefreshPeriod
type/env inconsistencies.
// (DEPRECATED, DO NOT USE) GetVmssSizeRefreshPeriod defines how frequently to call GET VMSS API to fetch VMSS info per nodegroup instance | ||
GetVmssSizeRefreshPeriod time.Duration `json:"getVmssSizeRefreshPeriod,omitempty" yaml:"getVmssSizeRefreshPeriod,omitempty"` | ||
// (DEPRECATED, DO NOT USE) GetVmssSizeRefreshPeriod (seconds) defines how frequently to call GET VMSS API to fetch VMSS info per nodegroup instance | ||
GetVmssSizeRefreshPeriod int `json:"getVmssSizeRefreshPeriod,omitempty" yaml:"getVmssSizeRefreshPeriod,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetVmssSizeRefreshPeriod
type/env inconsistencies.
@@ -56,6 +56,9 @@ const ( | |||
rateLimitWriteQPSEnvVar = "RATE_LIMIT_WRITE_QPS" | |||
rateLimitWriteBucketsEnvVar = "RATE_LIMIT_WRITE_BUCKETS" | |||
|
|||
// VmssSizeRefreshPeriodDefault in seconds | |||
VmssSizeRefreshPeriodDefault = 30 | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetVmssSizeRefreshPeriod
type/env inconsistencies.
|
||
// ARMBaseURLForAPClient is the URL to use for operations for the VMs pool. | ||
// It can override the default public ARM endpoint for VMs pool scale operations. | ||
ARMBaseURLForAPClient string `json:"armBaseURLForAPClient" yaml:"armBaseURLForAPClient"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VMs pool
cfg.ClusterName = os.Getenv("CLUSTER_NAME") | ||
cfg.ClusterResourceGroup = os.Getenv("ARM_CLUSTER_RESOURCE_GROUP") | ||
cfg.ARMBaseURLForAPClient = os.Getenv("ARM_BASE_URL_FOR_AP_CLIENT") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VMs pool
@@ -5517,11 +5517,18 @@ var InstanceTypes = map[string]*InstanceType{ | |||
MemoryMb: 921600, | |||
GPU: 8, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Most of this file unless specified) SKUs fetch (addressed).
vmsPoolSet := m.azureCache.getVMsPoolSet() | ||
if _, ok := vmsPoolSet[s.Name]; ok { | ||
return NewVMsPool(s, m), nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VMs pool
} else { | ||
klog.Warningf("ignoring vmss %q because of no maximum size specified for vmss", *scaleSet.Name) | ||
continue | ||
} | ||
if spec.MaxSize < spec.MinSize { | ||
klog.Warningf("ignoring vmss %q because of maximum size must be greater than minimum size: max=%d < min=%d", *scaleSet.Name, spec.MaxSize, spec.MinSize) | ||
klog.Warningf("ignoring vmss %q because of maximum size must be greater than or equal to minimum size: max=%d < min=%d", *scaleSet.Name, spec.MaxSize, spec.MinSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Most of this file unless specified) https://github.com/kubernetes/autoscaler/pull/6863/files
//For now, discourage the customers from using podAffinity to pick the availability zones. | ||
randomZone := failureDomains[rand.Intn(len(failureDomains))] | ||
result[apiv1.LabelTopologyZone] = randomZone | ||
result[azureDiskTopologyKey] = randomZone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.True(t, registered) | ||
assert.Equal(t, len(provider.NodeGroups()), 1) | ||
registered = provider.azureManager.RegisterNodeGroup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasinstance + vms pool together
@@ -691,6 +709,54 @@ func TestGetFilteredAutoscalingGroupsVmss(t *testing.T) { | |||
assert.True(t, assert.ObjectsAreEqualValues(expectedAsgs, asgs), "expected %#v, but found: %#v", expectedAsgs, asgs) | |||
} | |||
|
|||
func TestGetFilteredAutoscalingGroupsVmssWithConfiguredSizes(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that master-only change
@@ -147,6 +147,7 @@ func TestCreateAzureManagerValidConfig(t *testing.T) { | |||
mockVMClient := mockvmclient.NewMockInterface(ctrl) | |||
mockVMSSClient := mockvmssclient.NewMockInterface(ctrl) | |||
mockVMSSClient.EXPECT().List(gomock.Any(), "fakeId").Return([]compute.VirtualMachineScaleSet{}, nil).Times(2) | |||
mockVMClient.EXPECT().List(gomock.Any(), "fakeId").Return([]compute.VirtualMachine{}, nil).Times(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vms pool
No description provided.