[AKS] The default MachinePool should be of "system" type, not "user"

[mdufourneaud]

/kind bug

What steps did you take and what happened:
I create an AKS cluster with 2 machine pools.

What did you expect to happen:
The default machine pool should be a "system" node pool.

Anything else you would like to add:
The node pool type should be a property of AzureManagedMachinePool, in order to allow its configuration.

Environment:

• cluster-api-provider-azure version: v0.4.14
• Kubernetes version: (use kubectl version): v1.20.5
• OS (e.g. from /etc/os-release): Ubuntu 18.04.2 LTS

[alexeldeib]

The correct fix here is:

• an SDK update to 2021-03-01 (well, I'm saying that version because it's latest but anything after 2020-03-01 would work)
• mangedcluster service should construct the initial pool with Mode: System
• API for AzureManagedMachinePool should include a mode: {System | User} field
• AzureManagedMachinePool reconciler should pass the mode to agentpools service, which should apply it in Azure...
• BUT either a webhook (ideal) or a reconciler should prevent the default pool reference from being set to "User" -- it should always be set to System (after this update).
  • Note that this should work bidirectionally: if a user changes AzureManagedControlPlane's default pool ref, the hook should validate the new pool is of mode System. If the user changed a machine pool, we may (?) want to map that back to the corresponding AzureManagedControlPlane and avoid permitting the change. However, this will be difficult: the MachinePool has no owner ref at create time, so it can't be mapped back to the AzureManagedControlPlane. Catching updates would not be sufficient since the MachinePool could be updated by something other than setting the owner ref, before the owner ref has been set. So probably check for AzureManagedControlPlane-> MachinePool inside a webhook and MachinePool -> AzureManagedControlPlane inside MachinePool reconciler.

think I forgot something as I was writing out the validation details, but that's the gist of it.

[alexeldeib]

MachinePool has spec.clusterName, we could do something similar with AzureManagedMachinePool (e.g. spec.machinePool) to link bidirectionally...a bit annoying but would make the webhook validation way easier (// possible at all)

[LochanRn]

The correct fix here is:

• an SDK update to 2021-03-01 (well, I'm saying that version because it's latest but anything after 2020-03-01 would work)

• mangedcluster service should construct the initial pool with Mode: System

• API for AzureManagedMachinePool should include a mode: {System | User} field

• AzureManagedMachinePool reconciler should pass the mode to agentpools service, which should apply it in Azure...

• BUT either a webhook (ideal) or a reconciler should prevent the default pool reference from being set to "User" -- it should always be set to System (after this update).

  • Note that this should work bidirectionally: if a user changes AzureManagedControlPlane's default pool ref, the hook should validate the new pool is of mode System. If the user changed a machine pool, we may (?) want to map that back to the corresponding AzureManagedControlPlane and avoid permitting the change. However, this will be difficult: the MachinePool has no owner ref at create time, so it can't be mapped back to the AzureManagedControlPlane. Catching updates would not be sufficient since the MachinePool could be updated by something other than setting the owner ref, before the owner ref has been set. So probably check for AzureManagedControlPlane-> MachinePool inside a webhook and MachinePool -> AzureManagedControlPlane inside MachinePool reconciler.

think I forgot something as I was writing out the validation details, but that's the gist of it.

Hi Alex,
Even I was trying to solve this problem and I came up with a bit of a different solution 😅

Why do we have a defaultPoolRef in capz ?
According to the AKS docs we only have system node pools and user node pools.
I feel capz should also maintain the same. We should remove defaultPoolRef from Azuremanagedcontrolplane.
In AzureManagedMachinePool as you said we should include a mode: {System | User} field. If the mode is set to system we need to add a label kubernetes.azure.com/mode: system just like how AKS adds it to it's nodes.
In the AzureManagedControlplane reconciler we can list all the system AzureManagedMachinePools using the label filter, the pools listed can then be used to pass it to managedClusterSpec.AgentPools. Once we list we can also validate for minimum of 1 system node pool, which is mandatory for AKS.

Advantages:

• Reduces circular dependency and thus reduces the complexity.
• Another advantage of this is we can also support or allow users to update system node pools. According to AKS docs if we have more then one system node pool we can
  • delete the other system node pools
  • change system node pool to a user node pool
  • change user node pool to system node pool

[alexeldeib]

The only reason for the default pool ref is to enforce the invariant that AKS clusters always have at least one (now, system) node pool.

Totally happy to see other solutions to that, but the thinking was we shouldn’t allow the CRD representation of an AKS cluster to enter invalid states, e.g. we should reject attempts to update AzureManagedControlPlane or AzureManagedMachinePools which result in zero system pools existing.

[LochanRn]

Honestly speaking, when i started to think and try to reason out why the existing API has defaultPoolRef even i felt the same. We should maintain a minimum of one system node pool. I agree and it makes sense 👍 .

But another thought I have for the above problem is, can we add a custom admission webhook, where in if it is the last system node pool and if the user tries to delete it we can reject the request. By doing so we can also avoid the possibility of having zero system node pools.

During the creation phase we can make the check at the azuremanagedcontrolplane reconciler. Just like how it happens in the exiting system.

[alexeldeib]

If we can do a webhook which lists all the pools and checks for at least one that works.

If we do that, would you create the cluster with all pools? Or just pick a random system pool for creation and then reconcile the rest in parallel?

[LochanRn]

During the creation time if we have multiple system node pools, I think I will create the cluster with all the system node pools provided.

[alexeldeib]

sgtm. I think we may want to avoid updating the node pools after creation inside the managed cluster service? And let agentpools service/reconciler handle updates after the cluster is ready. We’ll save some calls to Azure that way and avoid some conflicting operations, I think

[LochanRn]

Ok yeah make sense 👍

[LochanRn]

Honestly speaking, when i started to think and try to reason out why the existing API has defaultPoolRef even i felt the same. We should maintain a minimum of one system node pool. I agree and it makes sense 👍 .

But another thought I have for the above problem is, can we add a custom admission webhook, where in if it is the last system node pool and if the user tries to delete it we can reject the request. By doing so we can also avoid the possibility of having zero system node pools.

During the creation phase we can make the check at the azuremanagedcontrolplane reconciler. Just like how it happens in the exiting system.

If the webhook cannot list and figure out if it is a the last system node pool, may be shift the logic in azuremanagedmachinepool_reconciler and make the check there while deletion and prevent it from happening.