[Azure VMs Pool] Support mixed agentpool types in Azure Cache #6689

wenxuan0923 · 2024-04-04T03:44:42Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Use Azure cache to track VMs pools. As of today, CAS can only be used for single type of agentpool - either vmss or standard. We need to support vms pool and mixed agentpool types (vmss + vms). During the cache generation, we will fetch all resources regardless of the vmType, and also track the vms pools using a set (implemented with golang map) so we can distinguish between vmss vm instances and vms instances.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Right now the file azure_vms_pool.go is just a place holder without real implementation.

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2024-04-04T03:44:51Z

Hi @wenxuan0923. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Bryce-Soghigian · 2024-04-04T05:08:58Z

cluster-autoscaler/cloudprovider/azure/azure_vms_pool.go

+// GetOptions returns NodeGroupAutoscalingOptions that should be used for this particular
+// NodeGroup. Returning a nil will result in using default options.
+func (agentPool *VMsPool) GetOptions(defaults config.NodeGroupAutoscalingOptions) (*config.NodeGroupAutoscalingOptions, error) {
+	// TODO(wenxuan): Implement this method
+	return nil, cloudprovider.ErrNotImplemented
+}


Yes I'm aware. This whole file is now mostly place holders without any real implementation. I will add the required methods in the following PRs

Same concern. Thank you.

Bryce-Soghigian · 2024-04-04T05:37:02Z

cluster-autoscaler/cloudprovider/azure/azure_manager.go

@@ -137,7 +137,24 @@ func (m *AzureManager) fetchExplicitNodeGroups(specs []string) error {
 	return nil
 }

+// this pattern is for vms pool only "<minSize>:<maxSize>:<name>:vms"


is the name of the nodepool?

ah i see from reading tests

Removed this spec change so we don't expose AKS internal terms to the public

Bryce-Soghigian · 2024-04-04T05:40:48Z

cluster-autoscaler/config/dynamic/node_group_spec.go

@@ -32,6 +34,36 @@ type NodeGroupSpec struct {
 	MaxSize int `json:"maxSize"`
 	// Specifies whether this node group can scale to zero nodes.
 	SupportScaleToZero bool
+	// specifies the agentpool type, used for vms pool only


Agentpool and vm pool is an AKS specific concept. config/dynamic falls outside of our cloudprovider, so when adding changes we should consider how the behavior will affect other cloud providers.

I wonder if we can call it PoolType instead since it will the the homogenous type of the pool, and that seems more cloud neutral vs vmtype which means something to aks but not anyone else

Oh that makes sense! okay I will use a different approach them.

: change to use azure cache to track vms pools

gandhipr · 2024-04-08T22:50:09Z

cluster-autoscaler/cloudprovider/azure/azure_cache.go

+		vmPoolName := tags[agentpoolNameTag]
+		// fall back to legacy tag name if not found
+		if vmPoolName == nil {
+			vmPoolName = tags["poolName"]


can we add this as a const as well?

gandhipr · 2024-04-08T22:52:03Z

cluster-autoscaler/cloudprovider/azure/azure_cache.go

@@ -51,6 +51,7 @@ type azureCache struct {
 	// Cache content.
 	resourceGroup        string
 	vmType               string
+	vmsPoolMap           map[string]struct{} // track the nodepools that're vms pool


can we have a more defined type as opposed to just using struct? VMpools? I observe that we have defined this inside azure_vm_pool.go

This map[string]struct{} is just being used as set here, since golang doesn't have set data structure. It is a common pattern as you can see many of such usage in rp code base.
The purpose is only to be able to find out if an agentpool is vms pool, given its name, so we don't need to use any specific data type here.

We are setting value like this:

vmsPoolMap[to.String(vmPoolName)] = struct{}{}

https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/go.mod#L40 api machinery should have sets iirc you can use.

Thanks Bryce, the Set in apimachinery is also a map, so I don't think it's necessary:

All go implementations of set will use map[T]EmptyStruct for zero bytes.

https://pkg.go.dev/k8s.io/apimachinery/pkg/util/sets#Set is the location of the set package though that works with all types. Set in pkg/fields isn't one i have heard of before.

rename the field to vmsPoolSet to make it much more clearer?

gandhipr · 2024-04-08T23:00:00Z

cluster-autoscaler/cloudprovider/azure/azure_vms_pool.go

+// GetOptions returns NodeGroupAutoscalingOptions that should be used for this particular
+// NodeGroup. Returning a nil will result in using default options.
+func (agentPool *VMsPool) GetOptions(defaults config.NodeGroupAutoscalingOptions) (*config.NodeGroupAutoscalingOptions, error) {
+	// TODO(wenxuan): Implement this method
+	return nil, cloudprovider.ErrNotImplemented
+}


Same concern. Thank you.

gandhipr · 2024-04-08T23:00:33Z

cluster-autoscaler/cloudprovider/azure/azure_vms_pool.go

+// number is different from the number of nodes registered in Kubernetes.
+func (agentPool *VMsPool) TargetSize() (int, error) {
+	// TODO(wenxuan): Implement this method
+	return -1, cloudprovider.ErrNotImplemented


I am assuming we'll be implementing all of these in follow-up PRs?

Yes that's right, to implement all the methods we will need the agentpool client PR to be merged first.

I'm trying to keep the PR small and self-contained, so it's easier to review. There will be more PRs coming next.

gandhipr · 2024-04-10T03:04:11Z

/lgtm

wenxuan0923 · 2024-04-10T06:19:08Z

/retest

k8s-ci-robot · 2024-04-10T06:19:22Z

@wenxuan0923: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

Bryce-Soghigian · 2024-04-10T07:17:31Z

/ok-to-test

k8s-ci-robot · 2024-04-12T20:01:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tallaxes, wenxuan0923

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/cloudprovider/azure/OWNERS~~ [tallaxes]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 4, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 4, 2024

k8s-ci-robot requested review from nilo19 and x13n April 4, 2024 03:45

k8s-ci-robot added the area/provider/azure Issues or PRs related to azure provider label Apr 4, 2024

Bryce-Soghigian reviewed Apr 4, 2024

View reviewed changes

wenxuan0923 changed the title ~~[Azure VMs Pool] Adding new spec pattern for vms pool~~ [WIP][Azure VMs Pool] Adding new spec pattern for vms pool Apr 4, 2024

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2024

Use cache to track vms pools

d665ce6

wenxuan0923 force-pushed the wenx/add-vms-spec branch from c9255d9 to d665ce6 Compare April 4, 2024 20:15

fx

dba6883

wenxuan0923 changed the title ~~[WIP][Azure VMs Pool] Adding new spec pattern for vms pool~~ [WIP][Azure VMs Pool] Support mixed agentpool types when generating Azure cache Apr 4, 2024

wenxuan0923 changed the title ~~[WIP][Azure VMs Pool] Support mixed agentpool types when generating Azure cache~~ [WIP][Azure VMs Pool] Support mixed agentpool types Apr 4, 2024

Add UTs

cbdd49b

wenxuan0923 changed the title ~~[WIP][Azure VMs Pool] Support mixed agentpool types~~ [WIP][Azure VMs Pool] Support mixed agentpool types in Azure Cache Apr 5, 2024

wenxuan0923 changed the title ~~[WIP][Azure VMs Pool] Support mixed agentpool types in Azure Cache~~ [Azure VMs Pool] Support mixed agentpool types in Azure Cache Apr 5, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2024

Fx boilder plate header

d1aa705

gandhipr reviewed Apr 8, 2024

View reviewed changes

wenxuan0923 added 2 commits April 8, 2024 17:46

Add const

a454720

Rename vmsPoolSet

cca2f18

k8s-ci-robot assigned gandhipr Apr 10, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 10, 2024

tallaxes approved these changes Apr 12, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2024

k8s-ci-robot merged commit a7b4386 into kubernetes:cluster-autoscaler-release-1.29 Apr 12, 2024
7 checks passed

wenxuan0923 mentioned this pull request May 9, 2024

[Azure VMs Pool] Support mixed agentpool types in Azure Cache #6812

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Azure VMs Pool] Support mixed agentpool types in Azure Cache #6689

[Azure VMs Pool] Support mixed agentpool types in Azure Cache #6689

wenxuan0923 commented Apr 4, 2024 •

edited

Loading

k8s-ci-robot commented Apr 4, 2024

Bryce-Soghigian Apr 4, 2024

wenxuan0923 Apr 4, 2024 •

edited

Loading

gandhipr Apr 8, 2024

Bryce-Soghigian Apr 4, 2024

Bryce-Soghigian Apr 4, 2024

wenxuan0923 Apr 4, 2024

Bryce-Soghigian Apr 4, 2024 •

edited

Loading

wenxuan0923 Apr 4, 2024

gandhipr Apr 8, 2024

wenxuan0923 Apr 9, 2024

gandhipr Apr 8, 2024

wenxuan0923 Apr 9, 2024 •

edited

Loading

Bryce-Soghigian Apr 9, 2024

wenxuan0923 Apr 9, 2024

Bryce-Soghigian Apr 9, 2024 •

edited

Loading

zqingqing1 Apr 9, 2024

wenxuan0923 Apr 9, 2024

gandhipr Apr 8, 2024

gandhipr Apr 8, 2024

wenxuan0923 Apr 9, 2024 •

edited

Loading

gandhipr commented Apr 10, 2024

wenxuan0923 commented Apr 10, 2024

k8s-ci-robot commented Apr 10, 2024

Bryce-Soghigian commented Apr 10, 2024

k8s-ci-robot commented Apr 12, 2024

[Azure VMs Pool] Support mixed agentpool types in Azure Cache #6689

[Azure VMs Pool] Support mixed agentpool types in Azure Cache #6689

Conversation

wenxuan0923 commented Apr 4, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Apr 4, 2024

Choose a reason for hiding this comment

wenxuan0923 Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bryce-Soghigian Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenxuan0923 Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bryce-Soghigian Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenxuan0923 Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

gandhipr commented Apr 10, 2024

wenxuan0923 commented Apr 10, 2024

k8s-ci-robot commented Apr 10, 2024

Bryce-Soghigian commented Apr 10, 2024

k8s-ci-robot commented Apr 12, 2024

wenxuan0923 commented Apr 4, 2024 •

edited

Loading

wenxuan0923 Apr 4, 2024 •

edited

Loading

Bryce-Soghigian Apr 4, 2024 •

edited

Loading

wenxuan0923 Apr 9, 2024 •

edited

Loading

Bryce-Soghigian Apr 9, 2024 •

edited

Loading

wenxuan0923 Apr 9, 2024 •

edited

Loading