Cluster-api-provider-azure: Clean up ./cloud/services

[devigned]

/kind cleanup

Describe the solution you'd like
A while back, the code used to interact with Azure under ./cloud/services was interfaced to allow some separation of concern between the controller / scopes and the types from the Azure SDK. With the advent of the azure.Service interface, Azure SDK / client code doesn't leak into the controllers. In fact, there should not longer be dependencies on the code that directly interacts with Azure. We should use this as an opportunity to crystallize this design goal of refactoring to azure.Service by cleaning up the service internals.

Current structure:

cloud/services/scalesets/
├── client.go
├── mock_scalesets
│   ├── client_mock.go
│   ├── doc.go
│   └── scalesets_mock.go
├── service.go
├── vmss.go
└── vmss_test.go

Proposed structure:

cloud/services/scalesets/
├── client.go  <-- don't export this; make it private
├── mock_scalesets
│   ├── client_mock.go
│   ├── doc.go
│   └── scaleset_mock.go
├── scaleset.go <-- includes service and scope
└── scaleset_test.go

The only thing exported from the new structure is the Service struct and the Scope interface for each service. This will ensure that the encapsulation will not be violated and guide future contributors.

[CecileRobertMichon]

/help

[k8s-ci-robot]

@CecileRobertMichon:
This request has been marked as needing help from a contributor.

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-help command.

In response to this:

/help

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.

[shysank]

/assign

[devigned]

@shysank please don't hesitate to ping me if you have any questions. Might be best to refactor one service in a PR before refactoring all at the same time.

[alexeldeib]

any thoughts on how to replace code like https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/e92bba9b95d18578e09a4eaad1c1150c0c6f5516/exp/controllers/azuremangedcontrolplane_reconciler.go#L219-L263 ?

notably, I don't see many other reconcilers which write data back into the CRD?

[alexeldeib]

that blurb is long -- referencing managedClustersSvc.Get and managedClustersSvc.GetCredentials to reconcile endpoint/kubeconfig

[alexeldeib]

I see: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/e92bba9b95d18578e09a4eaad1c1150c0c6f5516/cloud/services/virtualmachines/virtualmachines.go#L69

[devigned]

Ahh... the code for *MachinePool and the rest in ./exp have not been refactored to the use the scope pattern. See https://github.com/kubernetes-sigs/cluster-api-provider-azure/issues/757.

I would focus on a service not used by the ./exp packages.

[alexeldeib]

I was thinking about how to refactor them in line with the same pattern (or if we should avoid that?) seems like the above solution would be one approach

[devigned]

We should refactor them. I've been remiss in not getting to them.

[shysank]

@devigned I'm trying to refactor disks service, and have a couple of questions:

1. Should we also make service struct private, and only export the scope interface, and the service initialization function?
2. Should we divide azure.Service into azure.Reconciler, and azure.Deleter to avoid the no-op in disks.Reconcile?

[devigned]

Great questions, @shysank.

1. I generally refrain from returning interfaces, but in this situation, it might be a good idea to ensure that a consumer of the API can only expect an azure.Service.
2. I think this will include a lot of work as you would then need to introduce disks.NewReconciler() azure.Reconciler and disks.NewDeleter azure.Deleter. I haven't thought through all of the implications of this change. I lean toward adding the no-op for now, returning an azure.Service, and possibly revisit in the future.

@CecileRobertMichon & @nader-ziada wdyt about this?

[nader-ziada]

Great questions, @shysank.

1. I generally refrain from returning interfaces, but in this situation, it might be a good idea to ensure that a consumer of the API can only expect an azure.Service.
2. I think this will include a lot of work as you would then need to introduce disks.NewReconciler() azure.Reconciler and disks.NewDeleter azure.Deleter. I haven't thought through all of the implications of this change. I lean toward adding the no-op for now, returning an azure.Service, and possibly revisit in the future.

@CecileRobertMichon & @nader-ziada wdyt about this?

what is the reason to return an interface? its usually not a best practice and I worry we might be complicating things by adding extra abstractions

[shysank]

what is the reason to return an interface? its usually not a best practice and I worry we might be complicating things by adding extra abstractions

I guess my main concern was to avoid code like this disks.Service{} for initialization, and only use the exported api's. But, I agree that it's not a good idea to return an interface, although in this case, we will still return the actual type except that it can only be inferred.

[devigned]

I was recommending an interface since at some point in the future, we will likely replace the cloud code to use Azure Service Operator to reconcile Azure resources rather than the cloud services directly.

As long as Reconcile and Delete are the only funcs exported, I'm 💯 behind return in a struct rather than an interface.

[alexeldeib]

we will likely replace the cloud code to use Azure Service Operator to reconcile Azure resources rather than the cloud services directly

Just a thought, I'm noticing several places where exporting Reconcile/Delete and handing that off to another implementer is not really sufficient. There are several places where the services write back into crds, several of which have bugs or awkward ergonomics right now. I would not expect ASO or another project to implement some of these things as part of reconcile a resource, since they are CAPI/CAPZ specific.

examples:

• https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/272261b395983b18247850dfb979ec4c41717632/cloud/services/networkinterfaces/networkinterfaces.go#L93 doesn't actually persist into the CRD
• https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/272261b395983b18247850dfb979ec4c41717632/cloud/services/virtualmachines/virtualmachines.go#L330-L339 should be similar to above: when diffDiskSettings is nil, we should validate the VM size either has a large enough cache or temp disk, and enable ephemeral os by default (I think!). This would require writing back into the CRD, which will not work cleanly in the same way as acceleratednetworking.
• https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/272261b395983b18247850dfb979ec4c41717632/cloud/services/virtualnetworks/virtualnetworks.go#L74 won't work for managed control plane
• https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/272261b395983b18247850dfb979ec4c41717632/cloud/services/scalesets/vmss.go#L81 might be broken same as for NIC. at the very least this should be done via setters or something.

other random opportunities for cleanup:

• vmspecs probably shouldn't be a slice: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/272261b395983b18247850dfb979ec4c41717632/cloud/scope/machine.go#L92
• vnetspecs probably shouldn't be a slice: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/272261b395983b18247850dfb979ec4c41717632/cloud/scope/cluster.go#L206

[nader-ziada]

we still need to refactor the other services

/reopen

[k8s-ci-robot]

@nader-ziada: Reopened this issue.

In response to this:

we still need to refactor the other services

/reopen

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.

[nader-ziada]

@shysank are you planning on working on the other services as well?

[shysank]

@nader-ziada Yeah, I'll take on the rest. Just caught up on some pending work in capi prs. Will start working on this once that is done.

[shysank]

/assign