refactor: upstream most of Azure managed CAS changes in cloudprovider/azure #7003

comtalyst · 2024-07-04T02:02:57Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

"Refactor" as a part of fork-upstream (managed-selfhosted) realignment. Should not have any breaking changes.
This codebases realignment will simplify the logistics between the two, cutting a significant portion of maintenance cost.

There will be a separate effort focusing on improve code quality, rather than realigning codebase.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

All are in cloudprovider/azure except go.mod, go.sum, and vendor.
Preferrably review 1.29 PR first, then compare it to here.
No significant differences.

Overall:

Bump cloud-provider-azure library to v1.29.4
Trivial code improvements (e.g., using constants for reused string literals, method interfaces, etc.)
Support cloud provider AAD certificate authentication
Add a new GPU label, with the old one being deprecated/switch out soon
Temporarily adding new configuration options; primarily for managed offering, not being supported for self-hosted yet so don't rely on them until documented: EnableDetailedCSEMessage, GetVmssSizeRefreshPeriod

Does this PR introduce a user-facing change?

- Azure: support cloud provider AAD certificate authentication

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

k8s-ci-robot · 2024-07-04T02:03:06Z

Hi @comtalyst. 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.

comtalyst · 2024-07-04T02:04:36Z

cluster-autoscaler/cloudprovider/azure/azure_scale_set.go


-	if vmssInfo.ExtendedLocation != nil {


Not present on older versions due to #6447. But seemingly justified(?) due to fork code differences, see createOrUpdateInstances() for after-fork location.

This feature itself and the after-fork location is already in the fork from past versions, but #6447 upstream it in master.

comtalyst · 2024-07-04T02:04:45Z

cluster-autoscaler/cloudprovider/azure/azure_scale_set.go


-	if scaleSet.enableForceDelete && isOperationNotAllowed(rerr) {


Not present on older versions due to #6447. But seemingly justified(?) due to fork code differences, see createOrUpdateInstances() for after-fork location.

This feature itself and the after-fork location is already in the fork from past versions, but #6447 upstream it in master.

comtalyst · 2024-07-04T02:05:00Z

cluster-autoscaler/cloudprovider/azure/azure_scale_set.go

 	if rerr != nil {
-		klog.Errorf("virtualMachineScaleSetsClient.DeleteInstancesAsync for instances %v failed: %v", requiredIds.InstanceIds, rerr)


Not present on older versions due to fork 1.30 have this white space for unknown reason... No commit differences. Probably a merge mistake.

comtalyst · 2024-07-04T02:05:26Z

cluster-autoscaler/cloudprovider/azure/azure_scale_set.go

@@ -767,6 +837,42 @@ func (scaleSet *ScaleSet) getOrchestrationMode() (compute.OrchestrationMode, err
 	return vmss.OrchestrationMode, nil
 }

-func isOperationNotAllowed(rerr *retry.Error) bool {


Not present on older versions due to #6447. But seemingly justified(?) due to fork code differences, see azure_force_delete_scale_set.go for after-fork location.

This feature itself and the after-fork location is already in the fork from past versions, but #6447 upstream it in master.

…/azure

tallaxes · 2024-07-24T17:34:56Z

/ok-to-test

comtalyst · 2024-07-26T20:28:06Z

/test pull-cluster-autoscaler-e2e-azure

tallaxes · 2024-07-31T00:39:44Z

/lgtm
/approve

feiskyer · 2024-07-31T03:06:15Z

/approve

k8s-ci-robot · 2024-07-31T03:06:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: comtalyst, feiskyer, tallaxes

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 4, 2024

k8s-ci-robot requested a review from aleksandra-malinowska July 4, 2024 02:03

k8s-ci-robot added the area/cluster-autoscaler label Jul 4, 2024

k8s-ci-robot requested a review from nilo19 July 4, 2024 02:03

k8s-ci-robot added 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 Jul 4, 2024

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 4, 2024

comtalyst commented Jul 4, 2024

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 10, 2024

rakechill mentioned this pull request Jul 19, 2024

refactor: upstream most of Azure managed CAS changes in cloudprovider/azure for 1.29 #6991

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2024

comtalyst mentioned this pull request Jul 20, 2024

refactor: upstream most of Azure managed CAS changes in cloudprovider/azure for 1.30 #7076

Merged

refactor: upstream most of Azure managed CAS changes in cloudprovider…

2eb6cbe

…/azure

comtalyst force-pushed the comtalyst/azure-changes-from-managed-master branch from 55979ec to 2eb6cbe Compare July 21, 2024 18:44

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2024

comtalyst marked this pull request as ready for review July 21, 2024 18:45

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 Jul 21, 2024

k8s-ci-robot requested a review from tallaxes July 21, 2024 18:45

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 Jul 24, 2024

k8s-ci-robot assigned tallaxes Jul 31, 2024

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

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

k8s-ci-robot merged commit 45dfba4 into kubernetes:master Jul 31, 2024
7 checks passed

comtalyst mentioned this pull request Aug 12, 2024

REQUEST: New membership for comtalyst kubernetes/org#5099

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: upstream most of Azure managed CAS changes in cloudprovider/azure #7003

refactor: upstream most of Azure managed CAS changes in cloudprovider/azure #7003

comtalyst commented Jul 4, 2024 •

edited

Loading

k8s-ci-robot commented Jul 4, 2024

comtalyst Jul 4, 2024

comtalyst Jul 4, 2024

comtalyst Jul 4, 2024

comtalyst Jul 4, 2024

tallaxes commented Jul 24, 2024

comtalyst commented Jul 26, 2024

tallaxes commented Jul 31, 2024

feiskyer commented Jul 31, 2024

k8s-ci-robot commented Jul 31, 2024


		if scaleSet.enableForceDelete && isOperationNotAllowed(rerr) {

		if rerr != nil {
		klog.Errorf("virtualMachineScaleSetsClient.DeleteInstancesAsync for instances %v failed: %v", requiredIds.InstanceIds, rerr)

refactor: upstream most of Azure managed CAS changes in cloudprovider/azure #7003

refactor: upstream most of Azure managed CAS changes in cloudprovider/azure #7003

Conversation

comtalyst commented Jul 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 Jul 4, 2024

comtalyst Jul 4, 2024

Choose a reason for hiding this comment

comtalyst Jul 4, 2024

Choose a reason for hiding this comment

comtalyst Jul 4, 2024

Choose a reason for hiding this comment

comtalyst Jul 4, 2024

Choose a reason for hiding this comment

tallaxes commented Jul 24, 2024

comtalyst commented Jul 26, 2024

tallaxes commented Jul 31, 2024

feiskyer commented Jul 31, 2024

k8s-ci-robot commented Jul 31, 2024

comtalyst commented Jul 4, 2024 •

edited

Loading