Cluster-api-provider-azure: Errors during AKS cluster provisioning

[devigned]

/kind bug

What steps did you take and what happened:

• run EXP_MACHINE_POOL=true EXP_AKS=true make tilt-up
• click on "worker-aks" in Tilt UI to deploy an AKS managed cluster
• during reconciliation, the following errors are seen repeatedly, but the infra eventually reconciles successfully

[manager] I0730 16:18:44.872984       9 azuremanagedmachinepool_reconciler.go:87] controllers/AzureManagedMachinePool "msg"="reconciling machine pool" "infraPool"="agentpool1" "namespace"="default" "ownerCluster"="aks-template" 
[manager] E0730 16:18:47.836734       9 controller.go:248] controller-runtime/controller "msg"="Reconciler error" "error"="error creating AzureManagedMachinePool default/agentpool1: failed to reconcile machine pool agentpool1: failed to create or update agent pool: failed to begin operation: containerservice.AgentPoolsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=\u003cnil\u003e \u003cnil\u003e" "controller"="azuremanagedmachinepool" "name"="agentpool1" "namespace"="default"

What did you expect to happen:
I expect in the normal case there would be no errors in the capz-controller logs when deploying an AKS managed cluster

Anything else you would like to add:
I'm betting this is caused by the AzureManagedMachinePools calling CreateOrUpdate to create a new agent pool prior to the successful, initial creation of the AKS cluster.

Discovered during: #798

[alexeldeib]

I was confused by these because I thought we already did a Get on the managed cluster in the reconciler, but looks like we only check if the agent pool exists: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/928f0c492b8dccf78703b343c7bbe543a5861210/cloud/services/agentpools/agentpools.go#L66

Since the managed pool reconciler and the cluster reconciler are separate, seems like your theory is right.

Fix should be pretty straightforward, get the cluster and check for a succeeded (or simply terminal?) provisioning state before attempting more operations

/help-wanted
/good-first-issue

[k8s-ci-robot]

@alexeldeib:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

I was confused by these because I thought we already did a Get on the managed cluster in the reconciler, but looks like we only check if the agent pool exists: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/928f0c492b8dccf78703b343c7bbe543a5861210/cloud/services/agentpools/agentpools.go#L66

Since the managed pool reconciler and the cluster reconciler are separate, seems like your theory is right.

Fix should be pretty straightforward, get the cluster and check for a succeeded (or simply terminal?) provisioning state before attempting more operations

/help-wanted
/good-first-issue

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.

[devigned]

It works, but it's just not optimal. I didn't want to hold back the changes in #798.

[CecileRobertMichon]

Are not blocking on the managed cluster create in the service that creates it like we do for other services currently?

"get the cluster and check for a succeeded (or simply terminal?) provisioning state before attempting more operations" is fine for now but I'm a bit concerned about generalizing this across services when we start not blocking on long running create operations because it would add a lot of API get calls if we get the cluster at each agent pool reconcile. Perhaps we could instead get the managed cluster in the managed cluster service itself and have some sort of status like providerID or provisioning state be written and then have any other services that have a dependency on it look for that status before attempting a createOrUpdate so that the Get only happens in one place (as opposed to every service that has a dependency having to do the Get) and only once after the cluster is in ready state.

This is out of scope for this issue, just thinking out loud about to generalize this solution in the future.

[alexeldeib]

We block on create in the AzureManagedControlPlane reconciler, the issue is we have a separate AzureManagedMachinePool reconciler for agent pools. This fails to reconcile when the AKS resource is in a non-terminal state and it tries to PUT a new pool.

I would probably rather look at the k8s object to figure out dependencies, we might even be able to cheat and use the existing readiness that the control plane reconciler sets.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[CecileRobertMichon]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[CecileRobertMichon]

/remove-lifecycle stale

[nprokopic]

I believe that I also hit these errors in my recent testing, if nobody is working on this you can assign it to me.

Fixing this before adding a basic AKS e2e test would be good.

[CecileRobertMichon]

/assign @nprokopic

[nprokopic]

Small update, apparently this does not happen always.

In multiple attempts, when the AKS cluster is created with 2 node pools where both have only a single replica, there are no errors.
When I increased the replica count to 3 for both node pools, the error occurred. I assume it happens as the initial AKS provisioning (with first system node pool deployment, larger in this case) takes longer.

And for the possible fix: AzureManagedControlPlane is already obtained in AzureManagedMachinePoolReconciler.Reconcile(req) func, so checking the AzureManagedControlPlane.Status.Initialized and/or AzureManagedControlPlane.Status.Ready, before proceeding with further reconciliation for non-system node pools, might help. I'll try this.

[nprokopic]

Here is a PR https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/1205 that fixes the AKS cluster provisioning errors that are mentioned in this issue, plus few more I stumbled upon.