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.

Sign up for GitHub

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

Jump to bottom

Error deleting a custom resources #2679

Open
jalonsomagnolia opened this issue Feb 4, 2025 · 5 comments · May be fixed by #2680
Open

Error deleting a custom resources #2679

jalonsomagnolia opened this issue Feb 4, 2025 · 5 comments · May be fixed by #2680
Assignees
@csviri

Comments

@jalonsomagnolia
Copy link

Bug Report

After upgrading to the java-operator-sdk 5.0, we have the following error when we try to delete a custom resource:

09:43:21 ERROR [io.ja.op.pr.ev.EventProcessor] (ReconcilerExecutor-subscriptionreconciler-46) Error during event processing ExecutionScope{ resource id: ResourceID{name='postman', namespace='paas-admin-api'}, version: 861762903}: io.javaoperatorsdk.operator.OperatorException: io.javaoperatorsdk.operator.OperatorException: java.lang.UnsupportedOperationException: Implement this
	at io.javaoperatorsdk.operator.processing.Controller.cleanup(Controller.java:216)
	at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleCleanup(ReconciliationDispatcher.java:245)
	at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleDispatch(ReconciliationDispatcher.java:91)
	at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleExecution(ReconciliationDispatcher.java:66)
	at io.javaoperatorsdk.operator.processing.event.EventProcessor$ReconcilerExecutor.run(EventProcessor.java:444)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: io.javaoperatorsdk.operator.OperatorException: java.lang.UnsupportedOperationException: Implement this
	at io.javaoperatorsdk.operator.monitoring.micrometer.MicrometerMetrics.lambda$timeControllerExecution$0(MicrometerMetrics.java:151)
	at io.micrometer.core.instrument.composite.CompositeTimer.record(CompositeTimer.java:69)
	at io.javaoperatorsdk.operator.monitoring.micrometer.MicrometerMetrics.timeControllerExecution(MicrometerMetrics.java:147)
	at io.javaoperatorsdk.operator.processing.Controller.cleanup(Controller.java:164)
	... 7 more
Caused by: java.lang.UnsupportedOperationException: Implement this
	at io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow.cleanup(Workflow.java:20)
	at io.javaoperatorsdk.operator.processing.Controller$2.execute(Controller.java:199)
	at io.javaoperatorsdk.operator.processing.Controller$2.execute(Controller.java:165)
	at io.javaoperatorsdk.operator.monitoring.micrometer.MicrometerMetrics.lambda$timeControllerExecution$0(MicrometerMetrics.java:149)
	... 10 more

Everything worked fine before moving to 5.0.

Our controller is configured with something like this:

@Slf4j
@RequiredArgsConstructor
@ControllerConfiguration(
    informer = @Informer(
        namespaces = WATCH_ALL_NAMESPACES,
    ),
    generationAwareEventProcessing = false
)
public class SubscriptionReconciler implements Reconciler<K8Subscription>, Cleaner<K8Subscription> {

       //// more code here

      @Override
    public DeleteControl cleanup(K8Subscription subscription, Context<K8Subscription> context) {
        log.info("cleanup {}", subscription);
   }
}

The code fails before calling our cleanup.

Checking the controller code, where the error is being thrown, I can see

// The cleanup is called also when explicit invocation is true, but the cleaner is not
              // implemented
              if (managedWorkflow.hasCleaner() || !explicitWorkflowInvocation) {
                workflowCleanupResult = managedWorkflow.cleanup(resource, context);
              }

Shouldn't be insteadif (managedWorkflow.hasCleaner() || explicitWorkflowInvocation) { (so only when explicitWorkflowInvocation = true). By default @Workflow has explicitWorkflowInvocation = false, so I understand that unless you want to implement a workflow, this code shouldn't be called.
@csviri
Copy link
Collaborator

csviri commented Feb 4, 2025 •
edited
Loading

Hi @jalonsomagnolia , thx for reporting, taking a look!

@csviri csviri self-assigned this Feb 4, 2025
@csviri
Copy link
Collaborator

csviri commented Feb 4, 2025

@jalonsomagnolia could you pls provide a simple reproducer?

this works correctly in integration tests like this

That logic is actually right, since in this case it should be basically an empty no-op workflow there.

@jalonsomagnolia
Copy link
Author

Thanks for the response @csviri . I'll provide you the reproducer asap. But in the meantime, let me understand something:

// The cleanup is called also when explicit invocation is true, but the cleaner is not
              // implemented
              if (managedWorkflow.hasCleaner() || !explicitWorkflowInvocation) {
                workflowCleanupResult = managedWorkflow.cleanup(resource, context);
              }

We don't want to invoke this managedWorkflow.cleanup, because the result is the error we are seeing. Then, the condition has to be false. managedWorkflow.hasCleaner()is false, which it's ok. And explicitWorkflowInvocation is false, which makes the if condition true, executing the code that produces the error.

default WorkflowCleanupResult cleanup(P primary, Context<P> context) {
    throw new UnsupportedOperationException("Implement this");
  }

Then, if you say that the ìf` condition is correct, then maybe the initialiser is incorrect:

explicitWorkflowInvocation =
        configuration.getWorkflowSpec().map(WorkflowSpec::isExplicitInvocation)
            .orElse(false);

In our case, configuration.getWorkflowSpec() is null, making explicitWorkflowInvocation = false. Should the default value be true then?

Finally, notice that the code in version 4.9.7, which works fine, was like this:

Controller.this.initContextIfNeeded(resource, context);
                    WorkflowCleanupResult workflowCleanupResult = null;
                    if (Controller.this.managedWorkflow.hasCleaner()) {
                        workflowCleanupResult = Controller.this.managedWorkflow.cleanup(resource, context);
                    }

The change is adding that new explicitWorkflowInvocation variable. To me it looks that something is wrong related to that variable.

@csviri csviri linked a pull request Feb 4, 2025 that will close this issue
fix: explicit workflow invovation cleanup issue #2680
Draft
@csviri
Copy link
Collaborator

csviri commented Feb 4, 2025 •
edited
Loading

Hi right there is an issue, your right, it is a bit more complex than it first seems, pls take a look on attached PR if makes sense.
#2680

Will add unit tests soon.
However would be nice to have a reproducer since there are other reasons that this should not failed nevertheless. thx

@jalonsomagnolia
Copy link
Author

Good morning @csviri . Here it is the reproducer project. As you can see it's a really simple operator, but if you create a CR and then tries to delete, you'll have this exception

2025-02-04 17:20:06,899 ERROR [io.jav.ope.pro.eve.EventProcessor] (ReconcilerExecutor-mgnlsimplereconciler-166) Error during event processing ExecutionScope{ resource id: ResourceID{name='test', namespace='test'}, version: 861967378}: io.javaoperatorsdk.operator.OperatorException: java.lang.UnsupportedOperationException: Implement this
        at io.javaoperatorsdk.operator.processing.Controller.cleanup(Controller.java:216)
        at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleCleanup(ReconciliationDispatcher.java:245)
        at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleDispatch(ReconciliationDispatcher.java:91)
        at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleExecution(ReconciliationDispatcher.java:66)
        at io.javaoperatorsdk.operator.processing.event.EventProcessor$ReconcilerExecutor.run(EventProcessor.java:444)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
        at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: java.lang.UnsupportedOperationException: Implement this
        at io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow.cleanup(Workflow.java:20)
        at io.javaoperatorsdk.operator.processing.Controller$2.execute(Controller.java:199)
        at io.javaoperatorsdk.operator.processing.Controller$2.execute(Controller.java:165)
        at io.javaoperatorsdk.operator.api.monitoring.Metrics.timeControllerExecution(Metrics.java:165)
        at io.javaoperatorsdk.operator.processing.Controller.cleanup(Controller.java:164)
        ... 7 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@csviri csviri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: explicit workflow invovation cleanup issue operator-framework/java-operator-sdk
2 participants
@csviri @jalonsomagnolia