I walked through the code very closely, seems like a good architecture, but it may change as more complex operators are ported. I feel like this will remain WIP until at least 3 operators are ported, that said - I tested created resource groups and it worked splendidly, so I will approve

agreed, we will need to ensure our tests are a bit more stable if we are going to confidently tweak this controller after other resources start to rely on it

I haven't dug in much further since last week, but I am reasonably familiar with the code 🙂

There is no way for the the generic reconcile loop to have any assuredness that this is being done correctly

This is generally true in all cases, and it's the developers job to test. This is true of every Kubernetes core controller -- there's no guarantee the Deployment controller does the right thing, except that it does and people have tested the behavior they expect. Internally, all of these reconcile requests will be fed to the controller by a workqueue of comingled objects anyway, so this aligns reasonably well with how the Kubernetes/controller-runtime internals function.

Controllers should never store any state in memory they cannot reconstruct on a fresh reconcile run. Trying to create a state machine inside the controller is going to cause problems. This has bitten the Kubernetes community repeatedly when dealing with status fields in particular, but has been a thorn generally. There is no concept of an "operation" in the Kubernetes API, everything is simply a level triggered response. I'd argue in the scenarios you hit, the correct decision is always to requeue. Reconciliation loop itself should check for whatever preconditions are necessary.

See discussion at kubernetes/kubernetes#34363 (comment) among others as context for this stance.

The problem is here, you don't know what updates have been made to the object, so there is not way of reapplying it.

The controller shouldn't need to care whether it's a re-reconcile of a previous attempt, or the first run of a fresh object on new boot. If it does or needs to store this as state, it's an anti-pattern.

I think one point @szoio's PR captures thoroughly that this does not touch on at all is dependencies. There's somewhat of a blurry line between service/resource, but I would imagine we could have service implementations for all RPs and CRD/controller for each resource type, and e.g. requiring something as input/output would be a matter of, e.g.:

if err := ensureA(ctx, obj); err != nil {
    return ctrl.Result{}, err
}

if err := ensureB(ctx, obj); err != nil {
    return ctrl.Result{}, err
}

if err := ensureC(ctx, obj); err != nil {
    return ctrl.Result{}, err
}

return ctrl.Result{}, nil

I think this starts to mirror the dependency information captured in #397 so maybe one path forward is starting with this, and building up the interface (e.g. pre/post reconcile resources) as we have more concrete use cases for some of those needs and find out what works before trying to abstract too much.

Thanks @alexeldeib for the comments:

Controllers should never store any state in memory they cannot reconstruct on a fresh reconcile run. Trying to create a state machine inside the controller is going to cause problems. This has bitten the Kubernetes community repeatedly when dealing with status fields in particular, but has been a thorn generally. There is no concept of an "operation" in the Kubernetes API, everything is simply a level triggered response. I'd argue in the scenarios you hit, the correct decision is always to requeue. Reconciliation loop itself should check for whatever preconditions are necessary.

Totally agree that the controller itself should not store any state. However the controller does update state of kubernetes objects, and this new state gets fed to the reconcile loop next time round. So the controller itself is stateless, though I don't think this is exactly what you mean.

I also like the idea that "The controller shouldn't need to care whether it's a re-reconcile of a previous attempt, or the first run of a fresh object on new boot" wherever possible.

There are however quite a few edge cases that one may encounter where some state can be somewhat helpful.

For example, say you have a service with an asynchronous deletion, and the GET operation for that service only returns a status code, say 200 if it exists, and 404 if it doesn't. I have found that we can't always rely on the status fields returned by the Azure management SDK.

Then let's say the manifest is updated and reapplied, such that the resource can't be patched, it needs to be deleted and recreated.

The first time it reconciles the manifest it deletes the object. Because it is an asynchronous delete, it requeues. The next time it reconciles, it attempts to verify the object. In this case suppose it gets a 200. Then it somehow needs to know that the object is busy being deleted. If we don't, we'll just take the 200 to mean that we have got to the end and the resource has finished reconciling successfully.

Having a richer set of possible states can be very helpful in resolving these edge cases.
There are a number of these kind of edge cases, and when these are combined, they make the implementation quite tricky, as each AsyncClient implementation will have to think about all these edge cases, and know how to deal with them, and set appropriate status for every possible scenario.

If we push the onus of responsibility onto the implementer to update these states, the implementations will become increasingly more confusing and tricky, especially if we want to refine the operators and provide better support for full lifecycle workflows such as update vs. delete+recreate.

For that reason I have been suggesting moving the this into the generic loop code, and only expect the specific operator implementation i.e. the AsyncClient (or ResourceManagerClient in #397) to give very concrete known facts about the interaction with Azure back to the caller, and not dictate how the reconcile loop should interpret it (by setting properties on the runtime.Object - which will determine how the loop behaves on the next reconcile).

I think i've misunderstood your motivation (between your comment and perusing the updates to your PR). I think I actually agree entirely with capturing the interaction more cleanly, but without more operators, it's tough for me to tell what the right level of abstraction/separation is (both technically and organizationally).

I'd be curious to see how you envision things like multiple service clients or composition of clients for higher level reconcilers (i'll drop a comment on the PR).