Cluster-api-provider-azure: [managedcluster] should check provisioning status before attempting changes

[alexeldeib]

/kind bug

Describe the solution you'd like
https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/4b049939e18f0ba735735c41ac3fa9f828cd5236/cloud/services/managedclusters/managedclusters.go#L181

This code unconditionally sends the cluster to Azure for updates. We need to do a Get() first on the cluster and ensure the provisioning state is in a terminal mode (succeeded, failed, etc -- NOT updating, deleting, or otherwise in progress).

The field is ProvisioningState in this struct from the response: https://godoc.org/github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-10-01/containerservice#ManagedClusterProperties

agentpools client does the get(), but doesn't check provisioning state: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/4b049939e18f0ba735735c41ac3fa9f828cd5236/cloud/services/agentpools/agentpools.go#L67-L70

• [ ] fix agent pools
• [ ] fix managed clusters

cc @serbrech

/help-wanted
/good-first-issue

[muang0]

Hi @alexeldeib I would like to take this one assuming that's cool :)

Got a couple questions for ya--
what action should be taken if the cluster is in a non-terminal mode?
Any advice on testing this one? Should I create unit tests for the parent functions inside the agentpools and managedclusters packages?

[alexeldeib]

Go for it! 👍

We don't want to send any additional CreateOrUpdate calls if the cluster is already in a non-terminal state.

For testing, we could create some mocks and then test the behavior of the logic around the actual service calls -- e.g. if we do a get, we can mock returning a cluster in "Provisioning" state, and we should expect no calls to CreateorUpdate.

Picking disks as a random example for mocks:

https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/4b049939e18f0ba735735c41ac3fa9f828cd5236/cloud/services/disks/mock_disks/doc.go

https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/4b049939e18f0ba735735c41ac3fa9f828cd5236/cloud/services/disks/disks_test.go

[alexeldeib]

/remove-help

[muang0]

@alexeldeib Hi Alex, hope you're having a good weekend! I'm facing an interesting issue, hoping I can take your advice on how I should approach this one :)

I am building my unit tests, but am getting an assignment error on a mock function response. In the agentPools unit test, I am mocking the Get() call to respond with an AgentPool struct. However, mock (I am using v1.4.3 latest) is throwing the following error:
agentpools_test.go:61: wrong type of argument 0 to Return for *mock_agentpools.MockClient.Get: containerservice.AgentPool is not assignable to containerservice.AgentPool

I have taken a couple approaches so far:

1. Compared my Get() mock to the 'groups' unit test, specifically TestDeleteGroups where line 311 is returning resources.Group struct in their mock
2. Sanity check by doing my own assignment between variables of type containerservice.AgentPool (line 46 here)
3. Stepped through gomock down to gomock/call.go, in their Return() function where I believe AssignableTo() should be evaluating as true (but is false). Return() validates that the return arguments of the mock are assignable in application (my next approach is to dive deeper into this one, but I am struggling a bit here)

Thanks again for the help, and let me know if there is another approach to debugging this that I am missing :)

[devigned]

The error is probably caused by one import being different, but having the same type name. This is an error that is often found when using a different api version for an Azure SDK for Go type.

[alexeldeib]

Nice progress! David beat me to it, but:

https://github.com/jroden/cluster-api-provider-azure/blob/87b339be45a22d13602ba2bd241532e5f10806d7/cloud/services/agentpools/agentpools_test.go#L24

And

https://github.com/jroden/cluster-api-provider-azure/blob/87b339be45a22d13602ba2bd241532e5f10806d7/cloud/services/agentpools/agentpools.go#L23

Need to match.

(Btw, I usually go by Ace :) )

[muang0]

Oops, sorry Ace!
Thanks for the help guys, that solved my issue :sweat_smile:

[muang0]

Hi guys, any advice on which issue I should tackle next?

[devigned]

@jroden, love the enthusiasm!! Come over and join us in https://slack.k8s.io #cluster-api-azure. It's easier having these kind of conversations in chat.

If that doesn't work out for you, then see if any of these are interesting. Thank you!