Use a list for Azure services in reconcilers #2146

CecileRobertMichon · 2022-03-04T00:51:08Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it: Since we are reconciling (and deleting in reverse order) Azure services one by one in reconcilers, this is a small optimization to move the reconcile and delete to a list and range through the services instead of making a Reconcile/Delete call for each. This is possible now that we've refactored all services to implement Azure.Reconciler. Also adds unit tests.

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 #

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:

Use a list for Azure services in AzureCluster and AzureMachine reconcilers

CecileRobertMichon · 2022-03-04T00:53:09Z

controllers/azurecluster_reconciler.go

-		tagsSvc:          tags.New(scope),
+		scope: scope,
+		services: []azure.Reconciler{
+			groups.New(scope),


this is the same order as the Reconcile() calls below

CecileRobertMichon · 2022-03-04T00:55:23Z

controllers/azuremachine_reconciler.go

+			inboundnatrules.New(machineScope),
+			networkinterfaces.New(machineScope, cache),
+			availabilitysets.New(machineScope, cache),
+			disks.New(machineScope),


this is the same order as Reconcile() below except for disks that were not present before in Reconcile (it's a no-op) but needs to be deleted after the VM (hence needs to be before VMs in the list)

What was our behavior before this change? Would the disk get deleted anyway?

There was a disk delete call https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2146/files#diff-5a6d880b9a0be00329894fc2f52f15827dd6bdfe6b60aaec660728661023f41fL142 but no disk reconcile call. Disk reconcile is a no-op so adding it does not change behavior.

CecileRobertMichon · 2022-03-04T20:11:59Z

/retest

CecileRobertMichon · 2022-03-04T20:12:14Z

/assign @Jont828 @devigned

devigned

I love this changeset! This improves the readability of the reconciler so much.

/lgtm

mboersma

/lgtm

jackfrancis

/lgtm

/assign @shysank

Jont828 · 2022-03-07T21:26:54Z

controllers/azurecluster_reconciler.go

@@ -152,46 +102,14 @@ func (s *azureClusterService) Delete(ctx context.Context) error {
 	ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.azureClusterService.Delete")
 	defer done()

-	if err := s.groupsSvc.Delete(ctx); err != nil {
+	if err := s.services[0].Delete(ctx); err != nil {


Could we set a variable like groupSvc before placing it in the list so we call that explicitly here?

Or can we make service registration a little bit more elaborate instead of just adding to an array. Something like

type RecocilerService struct { Name string, azure.Reconciler } func Register(serviceName, azure.Reconciler) { }

This would help us easily add new apis to control reconciliation flow. For eg. for the case above, we can have a GetService(name) which iterates over the services list and returns the one with matching name.
Or add a Skip option to the ReconcilerService if we want to skip reconciliation etc. wdyt?

even if we assign it to a variable it wouldn't be accessible from here unless we add it as a separate field on the azureClusterService struct since we only have access to the s receiver from this func. we could assign a new variable here to make it explicit that it's what we expect but the variable would only be used once and it would still make the same assumption on the order so not sure that's an improvement, like this:

groupSvc := s.services[0] if err := s.groupSvc.Delete(ctx); err != nil {

@shysank I like the idea of registering the services. How would you set order and iterate over all the services without a list?

Could we make a pretty simple struct with an array and a map from a name to its element in the array? Something like

type ServiceWrapper struct { Services []azure.Reconciler ServiceMap map[string]azure.Reconciler } func (s *ServiceWrapper) Add(name string, service azure.Reconciler) { s.Services = append(s.Services, service) s.ServiceMap[name] = service } func (s *ServiceWrapper) Get(name string) azure.Reconciler { return s.ServiceMap[name] }

How would you set order and iterate over all the services without a list?

There will still be some collection backing the services, just that it'd be something more than just []azure.Reonciler. Something like []ReconcilerService (or servicesMap in @Jont828's example). If I want to add a new service, I'll just use the Register or Add api without having to care about the underlying collection. For ordering, we could start with the order in which the services were registered (add priorities later if needed). We can extend this further by tagging services from which we can extract groups. For example, there can be a group with services independent of each other, and services in that group can be reconciled concurrently while the group itself will be reconciled serially. (shameless plug of a poc that I did earlier :) ).

Anyways, I'm fine with merging this PR with the current design since there has been so many reviews already, and we can discuss this in a separate issue.

Let me try something out. Concurrency is out of scope and we should discuss it separately but I think I can at least get a service name added so we can log the service name when there's an error to @Jont828's point below.

controllers/azurecluster_reconciler.go

controllers/azurecluster_reconciler_test.go

Jont828 · 2022-03-07T21:33:23Z

controllers/azuremachine_reconciler.go

+			inboundnatrules.New(machineScope),
+			networkinterfaces.New(machineScope, cache),
+			availabilitysets.New(machineScope, cache),
+			disks.New(machineScope),


What was our behavior before this change? Would the disk get deleted anyway?

CecileRobertMichon · 2022-03-09T01:05:41Z

Okay @Jont828 @shysank check out the latest commits. It is a significantly larger refactor but I think it addresses your concerns. I added a new interface called ServiceReconciler which is a Reconciler that also has Name() and IsManaged() . The Name() func allows us to retrieve the service name for better error logging and for identifying the groups service which avoid assuming ordering. After I did this I realized that we were still assuming ordering by skipping the group delete in the AzureCluster reconciler (loop was going from len-1 to 1 instead of 0). So I added another commit on top that adds IsManaged which allows us to check if the group is managed by CAPZ directly from the reconciler. Feel free to tell me this is a scope creep, I kept it unsquashed so we can extract the commit if needed.

I also included AzureMachinePool and AzureManagedControlPlane reconcilers in the changes since they use the same pattern.

I think this gets us closer to what we want and we can discuss adding "groups" of services (or priorities, or a dependency graph) if we want to move towards concurrency as a follow-up. The IsManaged interface is also something I had wanted to do previously in #1684 to allow us to do caching of the vnet tags and standardize the "is resource managed" story.

CecileRobertMichon · 2022-03-09T01:06:17Z

azure/interfaces.go

 type Reconciler interface {
 	Reconcile(ctx context.Context) error
 	Delete(ctx context.Context) error
 }

-// CredentialGetter is a Service which knows how to retrieve credentials for an Azure
-// resource in a resource group.
-type CredentialGetter interface {


this wasn't used anymore

CecileRobertMichon · 2022-03-09T01:08:00Z

azure/services/privatedns/privatedns.go

@@ -219,3 +226,9 @@ func (s *Service) isVnetLinkManaged(ctx context.Context, resourceGroupName, zone
 	tags := converters.MapToTags(zone.Tags)
 	return tags.HasOwned(s.Scope.ClusterName()), nil
 }
+
+// IsManaged returns always returns true.
+// TODO: separate private DNS and VNet links so we can implement the IsManaged method for each.


this one is a TODO for after #1715 is merged

CecileRobertMichon · 2022-03-09T01:11:35Z

azure/services/publicips/publicips.go

@@ -149,3 +156,8 @@ func (s *Service) isIPManaged(ctx context.Context, ipName string) (bool, error)
 	tags := converters.MapToTags(ip.Tags)
 	return tags.HasOwned(s.Scope.ClusterName()), nil
 }
+
+// IsManaged returns always returns true as public IPs are managed on a one-by-one basis.


this one is a little awkward. Each public IP can be either managed or unmanaged depending on its own tags and there is no overall "resource type is managed by CAPZ" or not, which makes it different from how the other services behave. Since we're not using this func anywhere (but it's there to satisfy the interface) I opted to always return true since that's the most cautious outcome: we assume public IPs are managed, but then we check on each one individually (https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2146/files#diff-fe943e74189aa531d871d7b774c371755043f5bd1b7e8ab14ad3d2b5fc023301R148). Open to suggestions if anyone has a better idea.

…ilers

CecileRobertMichon · 2022-03-09T19:23:55Z

looks like e2e tests are failing delete... looking

shysank · 2022-03-09T21:39:40Z

/lgtm

CecileRobertMichon · 2022-03-09T22:13:28Z

should I squash or keep the commits separate?

shysank · 2022-03-10T17:47:27Z

should I squash or keep the commits separate?

I'm fine with keeping the commits as is.

Jont828 · 2022-03-10T18:36:18Z

I like the interface changes!
/lgtm

shysank · 2022-03-10T19:30:17Z

/approve

k8s-ci-robot · 2022-03-10T19:30:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shysank

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~~ [shysank]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2022-03-10T20:48:03Z

@CecileRobertMichon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-apidiff	`2defff0`	link	false	`/test pull-cluster-api-provider-azure-apidiff`
pull-cluster-api-provider-azure-e2e-windows-dockershim	`2defff0`	link	unknown	`/test pull-cluster-api-provider-azure-e2e-windows-dockershim`

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.

k8s-ci-robot requested review from jsturtevant and mboersma March 4, 2022 00:51

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Mar 4, 2022

CecileRobertMichon commented Mar 4, 2022

View reviewed changes

k8s-ci-robot assigned devigned and Jont828 Mar 4, 2022

CecileRobertMichon force-pushed the service-list branch from 3cc357c to eb6756e Compare March 4, 2022 22:45

devigned approved these changes Mar 5, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2022

mboersma approved these changes Mar 7, 2022

View reviewed changes

k8s-ci-robot assigned mboersma Mar 7, 2022

jackfrancis approved these changes Mar 7, 2022

View reviewed changes

k8s-ci-robot assigned shysank and jackfrancis Mar 7, 2022

Jont828 reviewed Mar 7, 2022

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 9, 2022

CecileRobertMichon commented Mar 9, 2022

View reviewed changes

Use a list for Azure services in AzureCluster and AzureMachine reconc…

951e35f

…ilers

CecileRobertMichon force-pushed the service-list branch from cbbe562 to b6b8bd4 Compare March 9, 2022 01:20

CecileRobertMichon changed the title ~~Use a list for Azure services in AzureCluster and AzureMachine reconcilers~~ Use a list for Azure services in reconcilers Mar 9, 2022

Add IsManaged to service reconciler interface

2defff0

CecileRobertMichon force-pushed the service-list branch from b6b8bd4 to 2defff0 Compare March 9, 2022 19:26

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2022

k8s-ci-robot merged commit 348ef60 into kubernetes-sigs:main Mar 10, 2022

k8s-ci-robot added this to the v1.3 milestone Mar 10, 2022

CecileRobertMichon mentioned this pull request Mar 21, 2022

Make private dns reconcile/delete async #2007

Merged

3 tasks

CecileRobertMichon deleted the service-list branch February 17, 2023 23:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a list for Azure services in reconcilers #2146

Use a list for Azure services in reconcilers #2146

CecileRobertMichon commented Mar 4, 2022 •

edited

Loading

CecileRobertMichon Mar 4, 2022

CecileRobertMichon Mar 4, 2022

Jont828 Mar 7, 2022

CecileRobertMichon Mar 7, 2022

CecileRobertMichon commented Mar 4, 2022

CecileRobertMichon commented Mar 4, 2022

devigned left a comment

mboersma left a comment

jackfrancis left a comment

Jont828 Mar 7, 2022

shysank Mar 7, 2022

CecileRobertMichon Mar 7, 2022

CecileRobertMichon Mar 7, 2022

Jont828 Mar 7, 2022

shysank Mar 7, 2022 •

edited

Loading

CecileRobertMichon Mar 8, 2022

Jont828 Mar 7, 2022

CecileRobertMichon commented Mar 9, 2022

CecileRobertMichon Mar 9, 2022

CecileRobertMichon Mar 9, 2022

CecileRobertMichon Mar 9, 2022

CecileRobertMichon commented Mar 9, 2022

shysank commented Mar 9, 2022

CecileRobertMichon commented Mar 9, 2022 •

edited

Loading

shysank commented Mar 10, 2022

Jont828 commented Mar 10, 2022

shysank commented Mar 10, 2022

k8s-ci-robot commented Mar 10, 2022

k8s-ci-robot commented Mar 10, 2022

Use a list for Azure services in reconcilers #2146

Use a list for Azure services in reconcilers #2146

Conversation

CecileRobertMichon commented Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon commented Mar 4, 2022

CecileRobertMichon commented Mar 4, 2022

devigned left a comment

Choose a reason for hiding this comment

mboersma left a comment

Choose a reason for hiding this comment

jackfrancis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shysank Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon commented Mar 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon commented Mar 9, 2022

shysank commented Mar 9, 2022

CecileRobertMichon commented Mar 9, 2022 • edited Loading

shysank commented Mar 10, 2022

Jont828 commented Mar 10, 2022

shysank commented Mar 10, 2022

k8s-ci-robot commented Mar 10, 2022

k8s-ci-robot commented Mar 10, 2022

CecileRobertMichon commented Mar 4, 2022 •

edited

Loading

shysank Mar 7, 2022 •

edited

Loading

CecileRobertMichon commented Mar 9, 2022 •

edited

Loading