Skip to content

Change Naming of reconcilePrecondition for v5 #1926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
csviri opened this issue May 30, 2023 · 9 comments
Closed

Change Naming of reconcilePrecondition for v5 #1926

csviri opened this issue May 30, 2023 · 9 comments
Labels
api-changes-epic kind/feature Categorizes issue or PR as related to a new feature. workflows
Milestone

Comments

@csviri
Copy link
Collaborator

csviri commented May 30, 2023

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 csviri added kind/feature Categorizes issue or PR as related to a new feature. api-changes-epic labels May 30, 2023
@csviri csviri added this to the 5.0 milestone May 30, 2023
@github-actions
Copy link

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 github-actions bot added the stale label Jul 30, 2023
@csviri csviri removed the stale label Jul 30, 2023
@github-actions
Copy link

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 github-actions bot added the stale label Sep 29, 2023
@csviri csviri removed the stale label Oct 2, 2023
Copy link

github-actions bot commented Dec 2, 2023

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 github-actions bot added the stale label Dec 2, 2023
@csviri csviri removed the stale label Dec 4, 2023
@Javatar81
Copy link

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?

@metacosm
Copy link
Collaborator

@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.

@csviri
Copy link
Collaborator Author

csviri commented Jan 15, 2024

Let's assume this scenario:

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.

@Javatar81
Copy link

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?

@csviri
Copy link
Collaborator Author

csviri commented Jan 16, 2024

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 csviri linked a pull request Mar 21, 2024 that will close this issue
@csviri
Copy link
Collaborator Author

csviri commented Mar 25, 2024

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

@csviri csviri closed this as completed Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes-epic kind/feature Categorizes issue or PR as related to a new feature. workflows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants