Skip to content
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

fix: explicit workflow invocation uses the same resource intance that reconcile api #2686

Merged
merged 3 commits into from
Feb 14, 2025

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Feb 13, 2025

Signed-off-by: Attila Mészáros a_meszaros@apple.com

… reconcile api

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
@csviri csviri requested a review from metacosm February 13, 2025 12:29

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
@csviri csviri requested a review from xstefank February 13, 2025 13:03
@@ -116,4 +116,8 @@ public DefaultContext<P> setRetryInfo(RetryInfo retryInfo) {
this.retryInfo = retryInfo;
return this;
}

public P getPrimaryResource() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of exposing this method only for tests…

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for jumping in here, but if this method is not exposed, how a main reconciliation loop will be aware that the primary resource has changed if in the middle of one reconciliation an explicit dependent resource reconciliation is requested via context.managedWorkflowAndDependentResourceContext().reconcileManagedWorkflow(); It may happen that the primary resource is updated, or either its status, and then the main reconciliation loop will not be using a most up to date version of the primary resource. Or am I wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afalhambra-hivemq usually you get this as a parameter for example here for dependent resources:

It is not part of the interface (Context) just the impl so it is unit testable but you never access this primary resource through this method. I think @metacosm has a point I'm not big fan either, but on the other hand if there is no other way to unit test it I prefer to be just pragmatic.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that's the intent here, though. Also, the primary should probably not changed during a reconciliation loop but be the target of a subsequent reconciliation because otherwise the current one might be operating on inconsistent basis (for example, some dependents might have seen the old primary version and taken decisions based on that, while others might see a new, possibly contradictory version…)

Copy link
Collaborator Author

@csviri csviri Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use SSA should not change, otherwise fine. We should explain that in docs maybe more thoroughly or in a separate blogpost.
Will expand that section in release blogpost (currently there is a TODO 😶‍🌫️ )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your replies both.
Just to clarify our current scenario here.
Currently on JOSDK 4.9.7 we have managed dependent resources where in their reconcile method we update the status of the primary resource.... then the main/primary reconcile method is called afterwards with the primary and context as a parameter. - Did I understand then that updating the status of the primary on the DR renconcile method may lead to inconsistencies?

Am just asking cause we're already planning to upgrade to JOSDK 5.0.1, and would like to use the new feature to call explicitly the reconcile method on the managed dependent resources by calling context.managedWorkflowAndDependentResourceContext().reconcileManagedWorkflow() from the main/primary reconcile method, but then of course, once the method is done and the DR are reconciled, the primary object on this main/primary reconcile method won't be up to date with potential changes from the reconcile calls on the DRs.

Copy link
Collaborator Author

@csviri csviri Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @afalhambra-hivemq ,
currently working on blogpost regarding to this, what you do should to be fine if you set:
ConfigurationService.useSSAToPatchPrimaryResource
to false and this PR change released.

For SSA, basically it is required that you just create a resources where you set fields which you have "opinion". See
https://kubernetes.io/docs/reference/using-api/server-side-apply/

But it is not mandatory to use that in JOSDK, this flag to don't use SSA is not just for making the migration doable in multiple steps, we want to keep it also for the future.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both, I tested it and works like a charm. Even with ConfigurationService.useSSAToPatchPrimaryResource set to true.

@metacosm metacosm changed the title fix: explicit workflow invication uses the same resource intance that reconcile api fix: explicit workflow invocation uses the same resource intance that reconcile api Feb 14, 2025
@csviri csviri merged commit f5f0a60 into main Feb 14, 2025
57 checks passed
@csviri csviri deleted the fix-explicit-invoce-resource branch February 14, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants