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

DefaultManagedWorkflowAndDependentResourceContext.workflowReconcileResult() throws Exception in case DRs have not been reconciled #2625

Closed
ebma16 opened this issue Dec 7, 2024 · 4 comments · Fixed by #2639

Comments

@ebma16
Copy link

ebma16 commented Dec 7, 2024

Bug Report

What did you do?

When reconciling DRs with a ManagedWorkflow and utilizing the Explicit Workflow Incovation feature, there are cases where the DRs or the workflow are not reconciled. This could happen, for instance, when the current state of the CR (Custom Resource) prevents the reconciliation of the DRs.

Despite this, I need to access the context.managedWorkflowAndDependentResourceContext()?.workflowReconcileResult in my reconciler. Prior to v5, this method returned an Optional, so I could easily check for the existence of a workflowReconcileResult.

What did you expect to see?

Since v5, the context.managedWorkflowAndDependentResourceContext().workflowReconcileResult no longer returns an Optional. Instead, I expected it to return either an Object or null if no result was available.

What did you see instead? Under which circumstances?

An IllegalStateException was thrown. The Default Workflow only adds the RECONCILE_RESULT_KEY into the context when the DRs are reconciled (see here).

As a result the getMandatory() method throws an IllegalStateException because the DRs were not reconciled due to an unmet condition in the explicit workflow invocation. The RECONCILE_RESULT_KEY cannot be found in the context, causing the exception.

java.lang.IllegalStateException: Mandatory attribute (key: java.lang.Object@c9edbfc, type: io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowReconcileResult) is missing or not of the expected type
	at io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedWorkflowAndDependentResourceContext.lambda$getMandatory$0(DefaultManagedWorkflowAndDependentResourceContext.java:71) ~[operator-framework-core-5.0.0-beta1.jar:na]
	at java.base/java.util.Optional.orElseThrow(Optional.java:403) ~[na:na]
	at io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedWorkflowAndDependentResourceContext.getMandatory(DefaultManagedWorkflowAndDependentResourceContext.java:70) ~[operator-framework-core-5.0.0-beta1.jar:na]
	at io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedWorkflowAndDependentResourceContext.getWorkflowReconcileResult(DefaultManagedWorkflowAndDependentResourceContext.java:77) ~[operator-framework-core-5.0.0-beta1.jar:na]
	at io.pasx.mes.install.operator.service.genericservice.GenericServiceReconciler.getUpdatedStatus(GenericServiceReconciler.kt:414) ~[main/:na]

Environment

java-operator-sdk: 5.0.0-beta1

Additional context

Could you clarify the intended usage of the context.managedWorkflowAndDependentResourceContext()?.workflowReconcileResult in v5? Specifically, how should the workflowReconcileResult be accessed in situations like this? I would not expect to have to manually wrap this check in a try/catch block or check in advance whether the reconciliation has been explicitly triggered manually.

Best regards
Martin

@csviri
Copy link
Collaborator

csviri commented Dec 16, 2024

Hi @ebma16 ,

one thing is not clear to me from this description, when you get this exception, the workflow was actually called explitily before?

The idea why Optional was removed, is that you know if the workflow was called (either explicitly or implicitly) and only in that case you ask for the results.

thx!

@ebma16
Copy link
Author

ebma16 commented Dec 16, 2024

Hi @csviri,

the exception only occurs if the workflow was not explicitly called before.

From an architectural point of view, introducing a method that throws an exception when a result is not available leads to a suboptimal user experience. In practice, the explicit trigger of the workflow and the subsequent necessary query is often called at different source code locations. To ensure that the method can be called without problems, this information would either have to be routed through several methods or made globally accessible. This leads to additional complexity on the user side.

Although the intention behind this design change may have been to simplify the background logic, I feel that it makes the calling code or user interface unnecessarily complicated and less intuitive.

Of course, this is just my view of things. If you disagree, feel free to close the issue and maybe add a comment to the javadoc or the documentation so that people know that this behavior is intentional.

@csviri
Copy link
Collaborator

csviri commented Dec 16, 2024

In practice, the explicit trigger of the workflow and the subsequent necessary query is often called at different source code locations.

makes sense, kk, I we will consider putting back the Optional, thank you!
cc @metacosm @xstefank

@csviri
Copy link
Collaborator

csviri commented Dec 16, 2024

prepared related PR: #2639

@csviri csviri linked a pull request Dec 16, 2024 that will close this issue
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 a pull request may close this issue.

2 participants