java-operator-sdk icon indicating copy to clipboard operation
java-operator-sdk copied to clipboard

Change Naming of `reconcilePrecondition` for v5

Open csviri opened this issue 2 years ago • 8 comments

It can be confusing that in the precondition there is reconcile included, this not really describes the deletion that happens if this condition does not hold. Maybe it should be just condition ?

csviri avatar May 30 '23 08:05 csviri

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] avatar Jul 30 '23 01:07 github-actions[bot]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] avatar Sep 29 '23 01:09 github-actions[bot]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] avatar Dec 02 '23 01:12 github-actions[bot]

When the reconcileCondition holds true first and then later changes to false, the secondary resource is deleted. This is clearly described in the Javadocs. Two questions regarding this:

  • Why is it deleted at all? The name would indicate that it is just not reconciled.
  • What happens when the reconcileCondition holds false and then changes to true? Is the secondary registered again? This is not stated in the docs.

Maybe instead of renaming the method you could change it is logic. I don't know why it was implemented like that, is there a specific use case this was needed for or is it just an optimization?

Javatar81 avatar Jan 15 '24 10:01 Javatar81

@Javatar81 the idea is that the condition is a barrier for the secondary reconciliation meaning that if the condition is not true then the secondary shouldn't exist and therefore not reconciled (because, if it existed, it certainly should be reconciled to decide whether or not its state is conform to the desired state described by the primary resource). I do agree, though, that this is somewhat confusing.

metacosm avatar Jan 15 '24 14:01 metacosm

Let's assume this scenario:

https://github.com/operator-framework/java-operator-sdk/blob/af7706452752dadcecaccf549640066c795311d3/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageManagedDependentsReconciler.java#L22-L22

We want in the CR a flag to create an Ingress or not. If we want to describe such cases, we need a conditional resource, since some might want to change this flag from true to false. Thus remove the Ingress.

Side note: not reconciling a resource does not make sense, only case when it happens or might happen if a resource B depends on resource A, where A has a ready post condition; and before it already happened that A was ready and B reconciled, but now A is not ready (like new version deployment happening or such), and B is not reconciled until the A is ready. Otherwise workflow reconciles in a run all the resources.

So in this regard the reconcile word in precondition I think is a bit misleading since the condition does not prevent the reconciliation, but basically we are describing if the resource should exist or not.

csviri avatar Jan 15 '24 14:01 csviri

In #2063 we discussed the activationCondition. I needed this because I had the additional requirement that the API for the dependent resource might not be available in the cluster and even with reconcilePrecondition the operator calls the API to access the resource type.

But besides that, what are the differences between activationCondition and reconcilePrecondition? I could replace an reconcilePrecondition by an activationCondition but not vice versa, right?

Javatar81 avatar Jan 16 '24 10:01 Javatar81

The activation condition is very limited atm, for example you cannot make resources properly of same type with activation condition. (Will describe more the limitations in the docs). If you know that the resources will be always present on the cluster you can replace the activationCondition with reconcilePreCondition. Vice versa is not working always, just in some limited use cases.

Technically the differences is how the event source is registered, when a resource has an activation condition, the event source is not registered while the condition is not true. This is much more trickier than it might sound.

If the limitations would be solved the activation condition functionally replace in theory the reconcile condition, but registering dynamically the event sources is less efficient (although the differences might not be noticable).

We will explore more the possibilities of the activation condition. TBH it would be much easier to support it with just the standalone dependent resources.

csviri avatar Jan 16 '24 15:01 csviri

see discussion in the PR, we will stick with the original naming for now.

csviri avatar Mar 25 '24 09:03 csviri