-
Notifications
You must be signed in to change notification settings - Fork 4.3k
CA: Update Azure SDK to v2 #8784
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
base: master
Are you sure you want to change the base?
Conversation
|
/cc @jackfrancis |
|
/cc @nojnhuh |
e3ba9bb to
7047504
Compare
jackfrancis
left a comment
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.
/lgtm
/hold for review from @tallaxes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, mboersma 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:
Approvers can indicate their approval by writing |
|
/release-note-edit |
|
|
||
| klog "k8s.io/klog/v2" | ||
|
|
||
| "sigs.k8s.io/cloud-provider-azure/pkg/azureclients/deploymentclient" |
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.
The Azure code was using these client interfaces exported by cloud-provider-azure, but they didn't support Azure SDK v2 and have been removed in current releases.
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.
They have been replaced in cloud-provider-azure with Azure SDK v2 clients (and mocks), in azclient package (https://github.com/kubernetes-sigs/cloud-provider-azure/tree/master/pkg/azclient) That's what cluster-autoscaler should be migrating to, I think, unless there is a really good reason to use alternative clients.
The two exceptions without existing v2 clients in cloud-provider-azure would be agentpool (autoscaler already has one of its own - though generating one in cloud-provider-azure and using it instead would be better) and resourcesku.
| github.com/Azure/azure-sdk-for-go/sdk/azcore v1.20.0 | ||
| github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.0 | ||
| github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v7 v7.1.0 | ||
| github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/containerservice/armcontainerservice/v5 v5.1.0-beta.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.
| github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v7 v7.1.0 | ||
| github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/containerservice/armcontainerservice/v5 v5.1.0-beta.2 | ||
| github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v7 v7.1.0 | ||
| github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources/v2 v2.1.0 |
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.
The current API version for armresources is v3, but it has some breaking changes and seems to lack DeploymentExtended, so I went with the previous version.
|
ping @tallaxes, if we want to get this included in 1.35 |
tallaxes
left a comment
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.
Can we use v2 clients from cloud-provider-azure? Among other things, this would ensure high compatibility with client configuration such as auth methods, cloud endpoints, retry and backoff policies, rate limiting, polling frequency, etc.
|
|
||
| klog "k8s.io/klog/v2" | ||
|
|
||
| "sigs.k8s.io/cloud-provider-azure/pkg/azureclients/deploymentclient" |
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.
They have been replaced in cloud-provider-azure with Azure SDK v2 clients (and mocks), in azclient package (https://github.com/kubernetes-sigs/cloud-provider-azure/tree/master/pkg/azclient) That's what cluster-autoscaler should be migrating to, I think, unless there is a really good reason to use alternative clients.
The two exceptions without existing v2 clients in cloud-provider-azure would be agentpool (autoscaler already has one of its own - though generating one in cloud-provider-azure and using it instead would be better) and resourcesku.
| // Get v2 credentials for all Azure SDK v2 clients | ||
| cred, err := getAgentpoolClientCredentials(cfg) | ||
| if err != 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.
This replaces (the now unused) newAuthorizer; do we have confidence it supports the same options and configuration as newAuthorizer?
(Also, should not be named Agentpool if it is now used for everything?)
| azClientConfig := cfg.getAzureClientConfig(authorizer, env) | ||
| azClientConfig.UserAgent = getUserAgentExtension() | ||
| // Create common client options for all v2 clients | ||
| clientOptions := &policy.ClientOptions{ |
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.
These won't respect client retry/backoff/rate limiting config?
| resp, err := scaleSet.manager.azClient.virtualMachineScaleSetsClient.Get(ctx, scaleSet.manager.config.ResourceGroup, scaleSet.Name, nil) | ||
| if err != nil { | ||
| klog.Errorf("failed to get information for VMSS: %s, error: %v", scaleSet.Name, err) | ||
| return -1, newGetVMSSFailedError(err, false) |
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.
Shouldn't this still be checking for not found? (There are helpers in Azure/azure-sdk-for-go-extentions ...)
What type of PR is this?
/kind cleanup
/area provider/azure
What this PR does / why we need it:
Updates the Azure cluster-autoscaler backend to use Azure SDK v2.
Which issue(s) this PR fixes:
Fixes #8145
Special notes for your reviewer:
Does this PR introduce a user-facing change?