Refactor AzureMachineReconciler to inject AzuremachineService as dependency

[shysank]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
Refactors AzureMachineReconciler to inject AzuremachineService as dependency
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes#1047 (partially)

Special notes for your reviewer:

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:

Refactor AzureMachineReconciler to inject AzuremachineService as dependency

[k8s-ci-robot]

Hi @shysank. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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]

/ok-to-test

[shysank]

apidiff:

sigs.k8s.io/cluster-api-provider-azure/controllers
  Incompatible changes:
  - AzureMachineReconciler: old is comparable, new is not
  Compatible changes:
  - NewAzureMachineReconciler: added

[CecileRobertMichon]

/retest

[devigned]

I'm not clear that the azureMachineServiceFactory will be set anywhere besides tests. If the ability to inject an azureMachineServiceFactory for testing purposes, perhaps, that can be done simply by having a test within the same package as this struct and set the struct field directly.

[shysank]

@devigned I've made some small changes based on the feedback I received for a similar refactor here. I've also removed the variadic options to keep it simple. wdyt?

/cc @CecileRobertMichon

[devigned]

lgtm

@CecileRobertMichon wdyt?

[CecileRobertMichon]

this looks very similar to https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/cloud/interfaces.go#L28, I wonder if we should consider combining them.

[shysank]

Should we just make azuremachineService also be of type azure.Service? As in it still does reconcile and delete like other cloud/services except that it is kind of a composite service which orchestrates other individual services?

[devigned]

If I recall, @shysank made azuremachineService a azure.Service initially. I advised against that because I didn't want anyone to infer the azuremachineService was actually a cloud service, one of the services that lives ./cloud/services.

I think the Service interface is really an abstract concept that overflows the bounds of ./cloud/services. It is a description of the reconcile behavior, reconcile to state or delete.

@CecileRobertMichon do you remember talking about how a reconciler could be a composition of reconiling services? I think this is that pattern is surfacing. azuremachineService is a reconciling service composed of a collection of reconciling services, in some order of operations.

Perhaps, we just use azure.Service for now, and look to lift the interface from the azure package into a more generic package in a refactor.

Sorry if I've going into the bike shed on this one, but I just want to ensure we are making abstractions at the right levels.

[CecileRobertMichon]

Perhaps, we just use azure.Service for now, and look to lift the interface from the azure package into a more generic package in a refactor.

SGTM. When I say combine them, I mostly meant the cloud/services should be a composition of reconcilers. I think the interface should live outside cloud/ as well.

[shysank]

@devigned @CecileRobertMichon For now, I've made azureMachineReconciler to use azure.service. Filed an issue to move azure.Service outside of cloud/, PTAL.

[CecileRobertMichon]

Overall PR lgtm, I like this second iteration a lot better. Thanks @shysank!

[CecileRobertMichon]

/lgtm

[shysank]

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

[k8s-ci-robot]

@shysank: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	dcc68d6	link	/test pull-cluster-api-provider-azure-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[shysank]

@CecileRobertMichon Just noticed that I missed to use createAzureMachineService in Delete method. Sorry, lgtm got removed. PTAL, whenever you get a chance.

[CecileRobertMichon]

/lgtm
/approve

[k8s-ci-robot]

[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