Use region specific API calls with VMSS #1850

devigned · 2021-11-10T22:22:06Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
When listing VMSSes in a resource group, Azure responds with values it has in the regional cache. Unfortunately, there are sometimes spikes in regional replication for these caches. To avoid cross region replication issues, this PR introduces a region specific client for AzureManageMachinePools by transforming the client baseURI from something like https://management.azure.com to https://{region}.management.azure.com to ensure the request is directed to the region where the VMSSes should be.

fixes #1720

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

use a region specific Azure client for listing VMSS in AzureManagedClusters

devigned · 2021-11-10T22:45:47Z

success 1, 20mins

/test pull-cluster-api-provider-azure-e2e-exp

devigned · 2021-11-10T23:09:37Z

success 2, 22mins

/test pull-cluster-api-provider-azure-e2e-exp

devigned · 2021-11-11T00:21:59Z

success 3, 23mins

/test pull-cluster-api-provider-azure-e2e-exp

alexeldeib · 2021-11-11T00:27:07Z

exp/controllers/azuremanagedmachinepool_reconciler.go

+	if err != nil {
+		return "", errors.Wrap(err, "failed to parse the base URI of client")
+	}
+


devigned · 2021-11-11T01:03:11Z

success 4, 23mins

/test pull-cluster-api-provider-azure-e2e-exp

alexeldeib · 2021-11-11T01:12:28Z

23 min is right around what I’d expect for the total duration of this test. Looking good!

alexeldeib · 2021-11-11T01:13:57Z

Actually, with the latest bits to reconcile pools all together on cluster create, I wonder if we can’t squeeze that lower. Delete is kind of slow though, so we’ll see what makes sense (unrelated to this PR)

devigned · 2021-11-11T02:13:30Z

5 passes, 5th 23mins

devigned · 2021-11-11T13:49:17Z

@CecileRobertMichon the PR history is now solid green for the exp job. I think we are solid. So solid that we may want to consider adding the AKS related tests to the regular pr e2e test.

devigned · 2021-11-11T13:58:12Z

Actually, with the latest bits to reconcile pools all together on cluster create, I wonder if we can’t squeeze that lower. Delete is kind of slow though, so we’ll see what makes sense (unrelated to this PR)

I think with the transient error / requeue stuff that I added to the AzureManagedMachinePool reconciler, I think we are close to as good as we can get. The initial reconcile of AMMP only starts when control plane transitions to ready. Once ready, the reconciler will try to reach goal state persistently with requeues if it reaches what is deemed a transient error, a 404 for the VMSS other similar errors that will be resolved as infra comes online. I don't think this is the most elegant solution, but it seems to be resilient.

devigned · 2021-11-11T14:56:47Z

success 7?, 22mins.

alexeldeib · 2021-11-11T14:59:30Z

azure/regional_baseuri.go

+		return a.aliasAuth.BaseURI()
+	}
+
+	sansScheme := path.Join(fmt.Sprintf("%s.%s", a.Region, a.parsedURL.Host), a.parsedURL.Path)


Do we actually want path.join() (os filepath separator) here, versus a hardcoded slash? Wouldn’t this now be platform dependent?

Nvm, I might be thinking filepath.join()

I think we are good: https://cs.opensource.google/go/go/+/refs/tags/go1.17.3:src/path/path.go;l=174

alexeldeib · 2021-11-11T15:01:10Z

Before the PR which reconciled all AMMP with cluster creation, we would create the cluster, wait for it to reconcile fully, then trigger creation of nodepools which takes ~5 min. With that approach I would expect to see around 25min runs based on my previous testing.

since we don’t need that extra 5min anymore (all pools should be created), I wonder if we have room to squeeze the times lower somewhere, or if that’s just as good as it gets.

alexeldeib · 2021-11-11T15:01:52Z

Nice work with the URL scheme/host/path parsing

/lgtm

devigned · 2021-11-11T18:06:55Z

Before the PR which reconciled all AMMP with cluster creation, we would create the cluster, wait for it to reconcile fully, then trigger creation of nodepools which takes ~5 min. With that approach I would expect to see around 25min runs based on my previous testing.

since we don’t need that extra 5min anymore (all pools should be created), I wonder if we have room to squeeze the times lower somewhere, or if that’s just as good as it gets.

I like where this is going! I think we need to do some tinkering now that tests are more stable.

CecileRobertMichon

looks great, just one comment about using the Location() func

exp/controllers/azuremanagedmachinepool_reconciler.go

CecileRobertMichon · 2021-11-12T18:16:44Z

exp/controllers/azuremanagedmachinepool_reconciler.go

 	return &azureManagedMachinePoolService{
 		scope:         scope,
 		agentPoolsSvc: agentpools.New(scope),
-		scaleSetsSvc:  scalesets.NewClient(scope),
-	}
+		scaleSetsSvc:  scalesets.NewClient(authorizer),


CecileRobertMichon

/lgtm
/approve

k8s-ci-robot · 2021-11-12T21:30:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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:

~~OWNERS~~ [CecileRobertMichon]

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

CecileRobertMichon · 2021-11-18T22:46:58Z

/cherry-pick release-1.0

k8s-infra-cherrypick-robot · 2021-11-18T22:47:40Z

@CecileRobertMichon: #1850 failed to apply on top of branch "release-1.0":

Applying: use a regional base uri for making VMSS requests
Using index info to reconstruct a base tree...
M	exp/controllers/azuremanagedmachinepool_controller.go
M	exp/controllers/azuremanagedmachinepool_reconciler.go
M	templates/test/ci/cluster-template-prow-aks-multi-tenancy.yaml
M	templates/test/ci/prow-aks-multi-tenancy/kustomization.yaml
A	templates/test/ci/prow-aks-multi-tenancy/patch_location.yaml
Falling back to patching base and 3-way merge...
Auto-merging exp/controllers/azuremanagedmachinepool_reconciler.go
Auto-merging exp/controllers/azuremanagedmachinepool_controller.go
CONFLICT (content): Merge conflict in exp/controllers/azuremanagedmachinepool_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 use a regional base uri for making VMSS requests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.0

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.

CecileRobertMichon · 2021-11-18T23:13:33Z

Ok that's fair @ cherry-pick bot :)

@devigned do you think we should cherry-pick this to release-1.0?

devigned · 2021-11-19T16:38:39Z

@devigned do you think we should cherry-pick this to release-1.0?

With the next release coming very soon and managed clusters being experimental, I lean toward not dealing with the cherry pick merge conflict.

jackfrancis · 2022-02-02T18:51:31Z

/cherry-pick release-0.5

k8s-infra-cherrypick-robot · 2022-02-02T18:52:14Z

@jackfrancis: #1850 failed to apply on top of branch "release-0.5":

Applying: use a regional base uri for making VMSS requests
Using index info to reconstruct a base tree...
M	exp/controllers/azuremanagedmachinepool_controller.go
M	exp/controllers/azuremanagedmachinepool_reconciler.go
M	templates/test/ci/cluster-template-prow-aks-multi-tenancy.yaml
M	templates/test/ci/prow-aks-multi-tenancy/kustomization.yaml
A	templates/test/ci/prow-aks-multi-tenancy/patch_location.yaml
Falling back to patching base and 3-way merge...
Auto-merging templates/test/ci/cluster-template-prow-aks-multi-tenancy.yaml
Auto-merging exp/controllers/azuremanagedmachinepool_reconciler.go
Auto-merging exp/controllers/azuremanagedmachinepool_controller.go
CONFLICT (content): Merge conflict in exp/controllers/azuremanagedmachinepool_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 use a regional base uri for making VMSS requests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.5

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.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 10, 2021

k8s-ci-robot requested review from CecileRobertMichon and nader-ziada November 10, 2021 22:22

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 10, 2021

alexeldeib reviewed Nov 11, 2021

View reviewed changes

devigned force-pushed the regional-vmss-client branch 3 times, most recently from c87eec2 to d47efd7 Compare November 11, 2021 13:47

alexeldeib reviewed Nov 11, 2021

View reviewed changes

k8s-ci-robot assigned alexeldeib Nov 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2021

CecileRobertMichon reviewed Nov 12, 2021

View reviewed changes

use a regional base uri for making VMSS requests

613a2ab

devigned force-pushed the regional-vmss-client branch from d47efd7 to 613a2ab Compare November 12, 2021 20:59

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2021

CecileRobertMichon reviewed Nov 12, 2021

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Nov 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2021

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

k8s-ci-robot merged commit feba723 into kubernetes-sigs:main Nov 12, 2021

k8s-ci-robot added this to the v1.1 milestone Nov 12, 2021

devigned deleted the regional-vmss-client branch November 13, 2021 16:17

k8s-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Nov 19, 2021

devigned removed the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Nov 19, 2021

CecileRobertMichon mentioned this pull request Feb 1, 2022

[release-0.5] use aks-managed-poolName also for vmss match #2028

Merged

jackfrancis mentioned this pull request Feb 2, 2022

Fix AKS (AzureManagedCluster) cluster create on release-0.5 #2039

Closed

luthermonson mentioned this pull request May 2, 2023

Add AzureEnvironment to AzureManagedControlPlane #3509

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use region specific API calls with VMSS #1850

Use region specific API calls with VMSS #1850

devigned commented Nov 10, 2021 •

edited

Loading

devigned commented Nov 10, 2021 •

edited

Loading

devigned commented Nov 10, 2021

devigned commented Nov 11, 2021

alexeldeib Nov 11, 2021

devigned commented Nov 11, 2021

alexeldeib commented Nov 11, 2021

alexeldeib commented Nov 11, 2021 •

edited

Loading

devigned commented Nov 11, 2021

devigned commented Nov 11, 2021

devigned commented Nov 11, 2021

devigned commented Nov 11, 2021

alexeldeib Nov 11, 2021

alexeldeib Nov 11, 2021

devigned Nov 11, 2021

alexeldeib commented Nov 11, 2021

alexeldeib commented Nov 11, 2021

devigned commented Nov 11, 2021

CecileRobertMichon left a comment

CecileRobertMichon Nov 12, 2021

CecileRobertMichon left a comment

k8s-ci-robot commented Nov 12, 2021

CecileRobertMichon commented Nov 18, 2021

k8s-infra-cherrypick-robot commented Nov 18, 2021

CecileRobertMichon commented Nov 18, 2021

devigned commented Nov 19, 2021 •

edited

Loading

jackfrancis commented Feb 2, 2022

k8s-infra-cherrypick-robot commented Feb 2, 2022

Use region specific API calls with VMSS #1850

Use region specific API calls with VMSS #1850

Conversation

devigned commented Nov 10, 2021 • edited Loading

devigned commented Nov 10, 2021 • edited Loading

devigned commented Nov 10, 2021

devigned commented Nov 11, 2021

alexeldeib Nov 11, 2021

Choose a reason for hiding this comment

devigned commented Nov 11, 2021

alexeldeib commented Nov 11, 2021

alexeldeib commented Nov 11, 2021 • edited Loading

devigned commented Nov 11, 2021

devigned commented Nov 11, 2021

devigned commented Nov 11, 2021

devigned commented Nov 11, 2021

alexeldeib Nov 11, 2021

Choose a reason for hiding this comment

alexeldeib Nov 11, 2021

Choose a reason for hiding this comment

devigned Nov 11, 2021

Choose a reason for hiding this comment

alexeldeib commented Nov 11, 2021

alexeldeib commented Nov 11, 2021

devigned commented Nov 11, 2021

CecileRobertMichon left a comment

Choose a reason for hiding this comment

CecileRobertMichon Nov 12, 2021

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 12, 2021

CecileRobertMichon commented Nov 18, 2021

k8s-infra-cherrypick-robot commented Nov 18, 2021

CecileRobertMichon commented Nov 18, 2021

devigned commented Nov 19, 2021 • edited Loading

jackfrancis commented Feb 2, 2022

k8s-infra-cherrypick-robot commented Feb 2, 2022

devigned commented Nov 10, 2021 •

edited

Loading

devigned commented Nov 10, 2021 •

edited

Loading

alexeldeib commented Nov 11, 2021 •

edited

Loading

devigned commented Nov 19, 2021 •

edited

Loading