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

[nilo19]

Related to issue #2044 and pull request #2094

Refact the corresponding code in #2094.

The instance of cloudprovider.interface can only be obtained after the cluster autoscaler is built. Therefore, we let each cloud provider implement their own node comparator function, which will later be built in autoscaler in main.go.

[k8s-ci-robot]

Welcome @nilo19!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[nilo19]

/sig azure

[nilo19]

Can you please help to review my code? Thanks.

/assign @piosz

[feiskyer]

/cc @losipiuk @CecileRobertMichon

[k8s-ci-robot]

@feiskyer: GitHub didn't allow me to request PR reviews from the following users: CecileRobertMichon.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @losipiuk @CecileRobertMichon

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.

[feiskyer]

/lgtm

Thanks for the work

[feiskyer]

@losipiuk Could you help to take a look at the PR?

[losipiuk]

I do not believe this is the way to tackle this. Seems too tactical approach to me and if we go this way for any cloud-provider specific customization then CloudProvider interface would get super bloated.

I would very much prefer to have a method:
UpdateAutoscalingProcessors(processors processors.AutoscalingProcessors) processors.AutoscalingProcessors

I do not believe such customization should be put in CloudProvider interface itself but rather in abstraction above (also it just cannot be in CloudProvider because of circular module dependencies).
I would put it on the same level as code which creates the specific CloudProvider implementation.
Currently cluster autoscaler does not have it abstracted particularly well. There are Build* methods called from builder_*.go files but this is single method - not an interface - therefore it is not very extensible.

What I propose is somewhat bigger refactor, but hopefully it will pay out later as more CP specific customizations are needed:

1. in separate module introduce CloudProviderBuilder interface with following methods:
   • NewCloudProvider(opts config.AutoscalingOptions, do cloudprovider.NodeGroupDiscoveryOptions, rl *cloudprovider.ResourceLimiter) cloudprovider.CloudProvider
   • UpdateAutoscalingProcessors(processors processors.AutoscalingProcessors) processors.AutoscalingProcessors
2. In all cloud provider modules (cloudprovider/azure, cloudprovider/aws, ...) create submodule builder exposing single, paramterless method GetCloudProviderBuilder() CloudProviderBuilder
3. Implement the CloudProviderBuilder interface for each cloud provider
4. Make code in cloudprovider/builder use the GetCloudProviderBuilder methods. It probably should just become a proxy allowing calling appropriate GetCloudProviderBuilder method by cloud provider name.
5. In main.go use CloudProviderBuilder instance to
   a) build cloud provider instance
   b) postprocess the processors list

WDYT?
cc: @MaciekPytel

[MaciekPytel]

I agree - the point of having processors is to allow customization without further bloating cloudprovider. What @losipiuk proposes makes sense, but I'm not sure if we really need it.
If all we need is to initialize processors based on which provider is used (and not runtime information in that provider) we could just make names of providers consts in cloudprovider (ie. replace azure.ProviderName with cloudprovider.AzureProviderName). That way we can use them safely in if statements in main.

[feiskyer]

Thanks, @losipiuk @MaciekPytel.

Both proposals are reasonable. But agreed with @MaciekPytel that CloudProviderBuilder/UpdateAutoscalingProcessors() is a little too complicated, while MaciekPytel's suggestion is much simpler.

@losipiuk What do you think about initializing the processors based on provider?

[feiskyer]

Kindly ping @losipiuk @MaciekPytel. What do you think?

[losipiuk]

I agree - the point of having processors is to allow customization without further bloating cloudprovider. What Łukasz Osipiuk proposes makes sense, but I'm not sure if we really need it.
If all we need is to initialize processors based on which provider is used (and not runtime information in that provider) we could just make names of providers consts in cloudprovider (ie. replace azure.ProviderName with cloudprovider.AzureProviderName). That way we can use them safely in if statements in main.

The problem is not only the name, but also (mostly) customization logic which is cloud provider specific. What you propose does not address that in any way - the logic would need to be in core codebase, and would be compiled in no matter which CP is used. For simple customizations - which just parametrize processors that are already in core it would work. But for more in-depth customizations, it would be much better to have processor implementation isolated in cloudprovider module.

I do not like the approach as it bloats main.go with cp specific logic. We can probably use the approach you propose as a stopgap, though. It would be great though to have a commitment to straighten it up as direct followup.

PS. To be fair I am not super sure that with current processor interface the UpdateAutoscalingProcessors interface I proposed would be expressive enough to be easy to use for all the use-cases we would like to use it for. But it is hard to be sure without prooftesting.

[MaciekPytel]

I agree with you in principle. My idea only works if you want to use a set of predetermined processors (or a trivial customization like in this case). That being said so far no one needed anything more advanced - every single time a processor was customized in main.go it was to customize comparison for zone balancing.
Building a more advanced setup seems premature. And, honestly, if you want really complex cp-specific processors I think there will be more to figure out than just how to initialize them.

[losipiuk]

I agree with you in principle. My idea only works if you want to use a set of predetermined processors (or a trivial customization like in this case). That being said so far no one needed anything more advanced - every single time a processor was customized in main.go it was to customize comparison for zone balancing. Building a more advanced setup seems premature.

Fine. Let's just remember that the issue is there. So we do not extend temporary solution for customizations which no longer fit it.

And, honestly, if you want really complex cp-specific processors I think there will be more to figure out than just how to initialize them.

Yeah - that is very much probable.

Let's go with simple approach for now then.

[nilo19]

Does this refactor live up to your expectation? @MaciekPytel

[losipiuk]

Does this refactor live up to your expectation?

I do not believe so. Whole point of this discussion was not to access the azure cloud provider module from core part of cluster-autoscaler codebase.
To achieve that please move the azure.IsNodeInfoSimilar to a self-contained file processors/nodegropuset/azure_nodegroups.go.
Also please drop azure.ProviderName and replace its usages with AzureProviderName.
And do the same for other cloud providers. We want to have same pattern where provider name is defined for all cloud providers.

[nilo19]

What do you think of it know? @losipiuk @MaciekPytel

[losipiuk]

Looks. good. Thanks!
/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: losipiuk

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 [losipiuk]

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

[feiskyer]

@losipiuk Thanks