[Azure VMs pool] Introducing agentpool client #6831

wenxuan0923 · 2024-05-14T17:46:46Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

We will introduce a new vmType: vms to support the autoscaling for single instance vm pool. This new agentpool type will rely on AKS rp for scaling up and down instead of CRP call, which requires the agentpool client, cluster name and cluster resource group info.

In this PR, I'm adding the required info to the config and adding agentpool client into azClient. So far, the construction error is ignored so that no existing behavior will be impacted by this change.

See the PR targeting the 1.29 release: #6685

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2024-05-14T17:46:55Z

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-sigs/prow repository.

tallaxes

LGTM, with minor suggestions

tallaxes · 2024-05-24T22:30:54Z

cluster-autoscaler/cloudprovider/azure/azure_config.go

+	// ARMBaseURL is the URL to use for operations for the VMs pool.
+	// It can override the default public ARM endpoint for VMs pool scale operations.
+	ARMBaseURL string `json:"armBaseURL" yaml:"armBaseURL"`


This is used only by AgentPool client, which may not be obvious to those reading the code (especially since cloud-provider client configuration supports overriding RM endpoint for non-public clouds). Should we reflect this in the field name? Maybe something with "AKS" and/or "AP" in the name?

Related: At some point, please document the new configuration options in cloudprovider/azure/README.md

This variable prefix with "ARM" which indicates Azure Resource Manager, so I don't think we need to prefix with AKS/AP here if that makes sense

All existing clients talk to ARM, this field affects only AKS AgentPool client. Ideally the intended use of the variable should be clear from its name. Comments are bonus, but don't replace unambiguous naming.

Updated the name to ARMBaseURLForAPClient

tallaxes · 2024-05-30T01:45:45Z

/lgtm

jackfrancis · 2024-05-30T02:55:43Z

@tallaxes does AKS support VM (not VMSS) node pools natively now? I can add an additional test scenario to gather functional coverage for this new scenario.

We don't have to block progress of this PR on that btw.

jackfrancis · 2024-05-30T03:00:16Z

cluster-autoscaler/cloudprovider/azure/azure_config.go

@@ -87,8 +87,16 @@ type Config struct {
 	Location       string `json:"location" yaml:"location"`
 	TenantID       string `json:"tenantId" yaml:"tenantId"`
 	SubscriptionID string `json:"subscriptionId" yaml:"subscriptionId"`
-	ResourceGroup  string `json:"resourceGroup" yaml:"resourceGroup"`
-	VMType         string `json:"vmType" yaml:"vmType"`
+	ClusterName    string `json:"clusterName" yaml:"clusterName"`


These changes to the Azure provider config essentially make this an AKS-native provider. Historically this was not the case as it worked in both AKS and non-AKS (e.g., aks-engine, or terraform) modes. I'm not aware of anyone using these non-AKS modes any longer, but we probably want to err on the side of caution and make these new AKS-specific Config properties optional.

I'd suggest making ClusterName andClusterResourceGroup optional.

Thoughts?

This is a good point. They are already expected to be absent from azure.json (and thus should not need json/yaml tags?), would be good to ensure (and test) that nothing else in the code treats them as required.

Thanks for bringing this to my attention. Yes, I will make sure these new config values are optional - only required when we are operating on VMs pools.

tallaxes · 2024-05-30T03:11:58Z

@jackfrancis For the state of the VM nodepool support in AKS proper, @wenxuan0923 would be the best source. From autoscaler perspective, this PR is one in a sequence, more coming, and there won't be much to test until everything is in place.

wenxuan0923 · 2024-05-30T04:00:47Z

@tallaxes does AKS support VM (not VMSS) node pools natively now? I can add an additional test scenario to gather functional coverage for this new scenario.

We don't have to block progress of this PR on that btw.

Thanks for reviewing @jackfrancis, as Alex mentioned, the autoscaling feature for single instance pool is still under development. The changes including autoscaler upstream, AKS-rp and overlaymgr. I will let you know once the E2E flow is ready and then we can add more test scenarios for it.

feiskyer · 2024-06-04T03:16:44Z

/ok-to-test

wenxuan0923 · 2024-06-04T04:26:11Z

/retest

wenxuan0923 · 2024-06-04T05:00:32Z

/retest

tallaxes · 2024-06-13T21:49:33Z

/retest

feiskyer

/retest
/approve

k8s-ci-robot · 2024-06-14T20:48:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, 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/OWNERS~~ [feiskyer]

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 · 2024-06-14T22:05:34Z

@wenxuan0923: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-autoscaler-e2e-azure	`731045d`	link	false	`/test pull-cluster-autoscaler-e2e-azure`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

[Azure VMs pool] Introducing agentpool client

4c206d0

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 14, 2024

k8s-ci-robot requested review from feiskyer and jackfrancis May 14, 2024 17:46

k8s-ci-robot added area/cluster-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/provider/azure Issues or PRs related to azure provider needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 14, 2024

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

tallaxes approved these changes May 24, 2024

View reviewed changes

tallaxes reviewed May 24, 2024

View reviewed changes

Rename ARM_BASE_URL

731045d

k8s-ci-robot assigned tallaxes May 30, 2024

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

jackfrancis reviewed May 30, 2024

View reviewed changes

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 Jun 4, 2024

feiskyer reviewed Jun 14, 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 Jun 14, 2024

k8s-ci-robot merged commit 131d12b into kubernetes:master Jun 14, 2024
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Azure VMs pool] Introducing agentpool client #6831

[Azure VMs pool] Introducing agentpool client #6831

wenxuan0923 commented May 14, 2024 •

edited

Loading

k8s-ci-robot commented May 14, 2024

tallaxes left a comment

tallaxes May 24, 2024

wenxuan0923 May 29, 2024

tallaxes May 30, 2024

wenxuan0923 May 30, 2024

tallaxes commented May 30, 2024

jackfrancis commented May 30, 2024

jackfrancis May 30, 2024

tallaxes May 30, 2024

wenxuan0923 May 30, 2024

tallaxes commented May 30, 2024 •

edited

Loading

wenxuan0923 commented May 30, 2024 •

edited

Loading

feiskyer commented Jun 4, 2024

wenxuan0923 commented Jun 4, 2024

wenxuan0923 commented Jun 4, 2024

tallaxes commented Jun 13, 2024

feiskyer left a comment

k8s-ci-robot commented Jun 14, 2024

k8s-ci-robot commented Jun 14, 2024

[Azure VMs pool] Introducing agentpool client #6831

[Azure VMs pool] Introducing agentpool client #6831

Conversation

wenxuan0923 commented May 14, 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 May 14, 2024

tallaxes left a comment

Choose a reason for hiding this comment

tallaxes May 24, 2024

Choose a reason for hiding this comment

wenxuan0923 May 29, 2024

Choose a reason for hiding this comment

tallaxes May 30, 2024

Choose a reason for hiding this comment

wenxuan0923 May 30, 2024

Choose a reason for hiding this comment

tallaxes commented May 30, 2024

jackfrancis commented May 30, 2024

jackfrancis May 30, 2024

Choose a reason for hiding this comment

tallaxes May 30, 2024

Choose a reason for hiding this comment

wenxuan0923 May 30, 2024

Choose a reason for hiding this comment

tallaxes commented May 30, 2024 • edited Loading

wenxuan0923 commented May 30, 2024 • edited Loading

feiskyer commented Jun 4, 2024

wenxuan0923 commented Jun 4, 2024

wenxuan0923 commented Jun 4, 2024

tallaxes commented Jun 13, 2024

feiskyer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 14, 2024

k8s-ci-robot commented Jun 14, 2024

wenxuan0923 commented May 14, 2024 •

edited

Loading

tallaxes commented May 30, 2024 •

edited

Loading

wenxuan0923 commented May 30, 2024 •

edited

Loading