Skip to content
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

fix: ignore agentpool label when looking for similar node groups with Azure provider #2171

Merged
merged 3 commits into from
Jul 23, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Modify the code as the simple solution proposed by MaciekPytel.
  • Loading branch information
t-qini committed Jul 18, 2019
commit f7c563ab06c1825642c4db2afc718620fde35c6e
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,8 @@ import (
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
"k8s.io/autoscaler/cluster-autoscaler/config"
"k8s.io/autoscaler/cluster-autoscaler/config/dynamic"
"k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupset"
"k8s.io/autoscaler/cluster-autoscaler/utils/errors"
"k8s.io/klog"
schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
)

const (
Expand Down Expand Up @@ -231,9 +229,3 @@ func BuildAlicloud(opts config.AutoscalingOptions, do cloudprovider.NodeGroupDis
}
return cloudProvider
}

// IsNodeInfoSimilar compares if two nodes should be considered part of the
// same NodeGroupSet.
func (ali *aliCloudProvider) IsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
return nodegroupset.IsNodeInfoSimilar(n1, n2)
}
7 changes: 0 additions & 7 deletions cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
"k8s.io/autoscaler/cluster-autoscaler/config"
"k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupset"
"k8s.io/autoscaler/cluster-autoscaler/utils/errors"
"k8s.io/klog"
schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
Expand Down Expand Up @@ -358,9 +357,3 @@ func BuildAWS(opts config.AutoscalingOptions, do cloudprovider.NodeGroupDiscover
}
return provider
}

// IsNodeInfoSimilar compares if two nodes should be considered part of the
// same NodeGroupSet.
func (aws *awsCloudProvider) IsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
return nodegroupset.IsNodeInfoSimilar(n1, n2)
}
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,14 @@ func nodesFromSameAzureNodePool(n1, n2 *schedulernodeinfo.NodeInfo) bool {
// IsNodeInfoSimilar compares if two nodes should be considered part of the
// same NodeGroupSet. This is true if they either belong to the same Azure agentpool
// or match usual conditions checked by IsNodeInfoSimilar, even if they have different agentpool labels.
func (azure *AzureCloudProvider) IsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
func IsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
if nodesFromSameAzureNodePool(n1, n2) {
return true
}
azureIgnoredLabels := make(map[string]bool)
for k, v := range nodegroupset.IgnoredLabels {
for k, v := range nodegroupset.BasicIgnoredLabels {
azureIgnoredLabels[k] = v
}
azureIgnoredLabels[AzureNodepoolLabel] = true
return nodegroupset.IsNodeInfoSimilarExceptIgnoredLabels(n1, n2, azureIgnoredLabels)
return nodegroupset.IsCloudProviderNodeInfoSimilar(n1, n2, azureIgnoredLabels)
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
"k8s.io/autoscaler/cluster-autoscaler/config"
"k8s.io/autoscaler/cluster-autoscaler/config/dynamic"
"k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupset"
"k8s.io/autoscaler/cluster-autoscaler/utils/errors"
"k8s.io/klog"
schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
Expand Down Expand Up @@ -369,9 +368,3 @@ func (asg *Asg) Delete() error {
func (asg *Asg) Autoprovisioned() bool {
return false
}

// IsNodeInfoSimilar compares if two nodes should be considered part of the
// same NodeGroupSet.
func (baiducloud *baiducloudCloudProvider) IsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
return nodegroupset.IsNodeInfoSimilar(n1, n2)
}
9 changes: 6 additions & 3 deletions cluster-autoscaler/cloudprovider/cloud_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ import (
schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
)

const (
// AzureProviderName gets the azure.ProviderName. To prevent the cyclic importation,
// `azure.ProviderName` cannot be directly used here.
AzureProviderName = "azure"
)

// CloudProvider contains configuration info and functions for interacting with
// cloud provider (GCE, AWS, etc).
type CloudProvider interface {
Expand Down Expand Up @@ -69,9 +75,6 @@ type CloudProvider interface {
// Refresh is called before every main loop and can be used to dynamically update cloud provider state.
// In particular the list of node groups returned by NodeGroups can change as a result of CloudProvider.Refresh().
Refresh() error

// IsNodeInfoSimilar compare if two nodes are similar
IsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool
}

// ErrNotImplemented is returned if a method is not implemented.
Expand Down
7 changes: 0 additions & 7 deletions cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
"k8s.io/autoscaler/cluster-autoscaler/config"
"k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupset"
"k8s.io/autoscaler/cluster-autoscaler/utils/errors"
"k8s.io/klog"
schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
Expand Down Expand Up @@ -367,9 +366,3 @@ func BuildGCE(opts config.AutoscalingOptions, do cloudprovider.NodeGroupDiscover
RegisterMetrics()
return provider
}

// IsNodeInfoSimilar compares if two nodes should be considered part of the
// same NodeGroupSet.
func (gce *GceCloudProvider) IsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
return nodegroupset.IsNodeInfoSimilar(n1, n2)
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,8 @@ import (
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
"k8s.io/autoscaler/cluster-autoscaler/config"
"k8s.io/autoscaler/cluster-autoscaler/config/dynamic"
"k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupset"
"k8s.io/autoscaler/cluster-autoscaler/utils/errors"
"k8s.io/klog"
schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
)

const (
Expand Down Expand Up @@ -209,9 +207,3 @@ func BuildMagnum(opts config.AutoscalingOptions, do cloudprovider.NodeGroupDisco

return provider
}

// IsNodeInfoSimilar compares if two nodes should be considered part of the
// same NodeGroupSet.
func (mcp *magnumCloudProvider) IsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
return nodegroupset.IsNodeInfoSimilar(n1, n2)
}
2 changes: 0 additions & 2 deletions cluster-autoscaler/core/autoscaler.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,6 @@ type Autoscaler interface {
RunOnce(currentTime time.Time) errors.AutoscalerError
// ExitCleanUp is a clean-up performed just before process termination.
ExitCleanUp()

GetCloudProvider() cloudprovider.CloudProvider
}

// NewAutoscaler creates an autoscaler of an appropriate type according to the parameters
Expand Down
5 changes: 0 additions & 5 deletions cluster-autoscaler/core/static_autoscaler.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,6 @@ type StaticAutoscaler struct {
ignoredTaints taintKeySet
}

// GetCloudProvider returns the CloudProvider instance in staticAutoscaler
func (a *StaticAutoscaler) GetCloudProvider() cloudprovider.CloudProvider {
return a.CloudProvider
}

type staticAutoscalerProcessorCallbacks struct {
disableScaleDownForLoop bool
extraValues map[string]interface{}
Expand Down
20 changes: 8 additions & 12 deletions cluster-autoscaler/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,13 @@ import (
"syscall"
"time"

"k8s.io/autoscaler/cluster-autoscaler/cloudprovider/azure"

"github.com/prometheus/client_golang/prometheus"
"github.com/spf13/pflag"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
cloudBuilder "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/builder"
"k8s.io/autoscaler/cluster-autoscaler/config"
"k8s.io/autoscaler/cluster-autoscaler/core"
Expand Down Expand Up @@ -287,6 +290,10 @@ func buildAutoscaler() (core.Autoscaler, error) {

processors := ca_processors.DefaultProcessors()
processors.PodListProcessor = core.NewFilterOutSchedulablePodListProcessor()
if autoscalingOptions.CloudProviderName == cloudprovider.AzureProviderName {
processors.NodeGroupSetProcessor = &nodegroupset.BalancingNodeGroupSetProcessor{
Comparator: azure.IsNodeInfoSimilar}
}

opts := core.AutoscalerOptions{
AutoscalingOptions: autoscalingOptions,
Expand All @@ -299,18 +306,7 @@ func buildAutoscaler() (core.Autoscaler, error) {
metrics.UpdateNapEnabled(autoscalingOptions.NodeAutoprovisioningEnabled)

// Create autoscaler.
ca, err := core.NewAutoscaler(opts)
if ca == nil || err != nil {
return ca, err
}

// Modify the NodeGroupSetProcessor.Comparator in autoscaler
cp := ca.GetCloudProvider()
processors.NodeGroupSetProcessor = &nodegroupset.BalancingNodeGroupSetProcessor{
Comparator: cp.IsNodeInfoSimilar,
}

return ca, err
return core.NewAutoscaler(opts)
}

func run(healthCheck *metrics.HealthCheck) {
Expand Down
15 changes: 8 additions & 7 deletions cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,10 @@ const (
MaxFreeDifferenceRatio = 0.05
)

// IgnoredLabels define a set of basic labels that should be ignored when comparing the similarity
// of two nodes
var IgnoredLabels = map[string]bool{
// BasicIgnoredLabels define a set of basic labels that should be ignored when comparing the similarity
// of two nodes. Customized IgnoredLabels can be implemented in the corresponding codes of
// specific cloud provider and the BasicIgnoredLabels should always be considered part of them.
var BasicIgnoredLabels = map[string]bool{
apiv1.LabelHostname: true,
apiv1.LabelZoneFailureDomain: true,
apiv1.LabelZoneRegion: true,
Expand Down Expand Up @@ -85,12 +86,12 @@ func compareLabels(nodes []*schedulernodeinfo.NodeInfo, ignoredLabels map[string
// are similar enough to likely be the same type of machine and if the set of labels
// is the same (except for a pre-defined set of labels like hostname or zone).
func IsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
return IsNodeInfoSimilarExceptIgnoredLabels(n1, n2, IgnoredLabels)
return IsCloudProviderNodeInfoSimilar(n1, n2, BasicIgnoredLabels)
}

// IsNodeInfoSimilarExceptIgnoredLabels returns true if two NodeInfos are similar while
// ignoring the set of labels provided
func IsNodeInfoSimilarExceptIgnoredLabels(n1, n2 *schedulernodeinfo.NodeInfo, ignoredLabels map[string]bool) bool {
// IsCloudProviderNodeInfoSimilar remains the same logic of IsNodeInfoSimilar with the
// customized set of labels that should be ignored when comparing the similarity of two NodeInfos.
func IsCloudProviderNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo, ignoredLabels map[string]bool) bool {
capacity := make(map[apiv1.ResourceName][]resource.Quantity)
allocatable := make(map[apiv1.ResourceName][]resource.Quantity)
free := make(map[apiv1.ResourceName][]resource.Quantity)
Expand Down