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

fix: managed workflow result is available even when exception is thrown #2552

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: managed workflow result is available even when exception is thrown #2552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 4 additions & 17 deletions ...sdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@

@SuppressWarnings("rawtypes")
public class DefaultManagedDependentResourceContext implements ManagedDependentResourceContext {

private WorkflowReconcileResult workflowReconcileResult;
private WorkflowCleanupResult workflowCleanupResult;
public static final Object RECONCILE_RESULT_KEY = new Object();
public static final Object CLEANUP_RESULT_KEY = new Object();
private final ConcurrentHashMap attributes = new ConcurrentHashMap();

@Override
Expand All @@ -37,25 +36,13 @@ public <T> T getMandatory(Object key, Class<T> expectedType) {
+ ") is missing or not of the expected type"));
}

public DefaultManagedDependentResourceContext setWorkflowExecutionResult(
WorkflowReconcileResult workflowReconcileResult) {
this.workflowReconcileResult = workflowReconcileResult;
return this;
}

public DefaultManagedDependentResourceContext setWorkflowCleanupResult(
WorkflowCleanupResult workflowCleanupResult) {
this.workflowCleanupResult = workflowCleanupResult;
return this;
}

@Override
public Optional<WorkflowReconcileResult> getWorkflowReconcileResult() {
return Optional.ofNullable(workflowReconcileResult);
return get(RECONCILE_RESULT_KEY, WorkflowReconcileResult.class);
Copy link
Collaborator

@csviri csviri Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should solve this this way (with the context map), only if there is no other way around this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that it's not super nice but the current way is not great either (casting to the implementation, which is not a given, especially during tests since it breaks with mocks). I actually had a different implementation initially but I would like to remove the reliance on a specific implementation and a cast to it because I don't think it's a nice design.

}

@Override
public Optional<WorkflowCleanupResult> getWorkflowCleanupResult() {
return Optional.ofNullable(workflowCleanupResult);
return get(CLEANUP_RESULT_KEY, WorkflowCleanupResult.class);
}
}
9 changes: 1 addition & 8 deletions operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceNotFoundException;
import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceProvider;
import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceReferencer;
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedDependentResourceContext;
import io.javaoperatorsdk.operator.health.ControllerHealthInfo;
import io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow;
import io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowCleanupResult;
Expand Down Expand Up @@ -145,10 +144,7 @@ public Map<String, Object> metadata() {
public UpdateControl<P> execute() throws Exception {
initContextIfNeeded(resource, context);
if (!managedWorkflow.isEmpty()) {
var res = managedWorkflow.reconcile(resource, context);
((DefaultManagedDependentResourceContext) context.managedDependentResourceContext())
.setWorkflowExecutionResult(res);
res.throwAggregateExceptionIfErrorsPresent();
managedWorkflow.reconcile(resource, context);
}
return reconciler.reconcile(resource, context);
}
Expand Down Expand Up @@ -191,9 +187,6 @@ public DeleteControl execute() {
WorkflowCleanupResult workflowCleanupResult = null;
if (managedWorkflow.hasCleaner()) {
workflowCleanupResult = managedWorkflow.cleanup(resource, context);
((DefaultManagedDependentResourceContext) context.managedDependentResourceContext())
.setWorkflowCleanupResult(workflowCleanupResult);
workflowCleanupResult.throwAggregateExceptionIfErrorsPresent();
}
if (isCleaner) {
var cleanupResult = ((Cleaner<P>) reconciler).cleanup(resource, context);
Expand Down
7 changes: 6 additions & 1 deletion .../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected;
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedDependentResourceContext;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource;

/**
Expand Down Expand Up @@ -79,7 +80,7 @@ private Map<String, DependentResourceNode> toMap(Set<DependentResourceNode> node
}
map.put(node.getName(), node);
}
if (topLevelResources.size() == 0) {
if (topLevelResources.isEmpty()) {
throw new IllegalStateException(
"No top-level dependent resources found. This might indicate a cyclic Set of DependentResourceNode has been provided.");
}
Expand All @@ -91,6 +92,8 @@ public WorkflowReconcileResult reconcile(P primary, Context<P> context) {
WorkflowReconcileExecutor<P> workflowReconcileExecutor =
new WorkflowReconcileExecutor<>(this, primary, context);
var result = workflowReconcileExecutor.reconcile();
context.managedDependentResourceContext()
.put(DefaultManagedDependentResourceContext.RECONCILE_RESULT_KEY, result);
if (throwExceptionAutomatically) {
result.throwAggregateExceptionIfErrorsPresent();
}
Expand All @@ -102,6 +105,8 @@ public WorkflowCleanupResult cleanup(P primary, Context<P> context) {
WorkflowCleanupExecutor<P> workflowCleanupExecutor =
new WorkflowCleanupExecutor<>(this, primary, context);
var result = workflowCleanupExecutor.cleanup();
context.managedDependentResourceContext()
.put(DefaultManagedDependentResourceContext.CLEANUP_RESULT_KEY, result);
if (throwExceptionAutomatically) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't rather only this throw removed?
It seems to be redundant. If I remember correctly I was solving this for v5, there it seems to have different structure now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's slightly different in v5 but the problem is still present in a somewhat different form, though.

result.throwAggregateExceptionIfErrorsPresent();
}
Expand Down
3 changes: 1 addition & 2 deletions .../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
@SuppressWarnings({"rawtypes", "unchecked"})
public class WorkflowBuilder<P extends HasMetadata> {

private final Map<String, DependentResourceNode<?, P>> dependentResourceNodes =
new HashMap<>();
private final Map<String, DependentResourceNode<?, P>> dependentResourceNodes = new HashMap<>();
private boolean throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT;
private DependentResourceNode currentNode;
private boolean isCleaner = false;
Expand Down
10 changes: 5 additions & 5 deletions ...o/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.ManagedDependentResourceContext;
import io.javaoperatorsdk.operator.processing.event.EventSourceRetriever;
import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;

import static io.javaoperatorsdk.operator.processing.dependent.workflow.ExecutionAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static org.mockito.Mockito.*;

class WorkflowCleanupExecutorTest extends AbstractWorkflowExecutorTest {

Expand All @@ -29,8 +29,7 @@ class WorkflowCleanupExecutorTest extends AbstractWorkflowExecutorTest {
protected TestDeleterDependent dd3 = new TestDeleterDependent("DR_DELETER_3");
protected TestDeleterDependent dd4 = new TestDeleterDependent("DR_DELETER_4");
@SuppressWarnings("unchecked")

Context<TestCustomResource> mockContext = mock(Context.class);
Context<TestCustomResource> mockContext = spy(Context.class);
ExecutorService executorService = Executors.newCachedThreadPool();

@BeforeEach
Expand All @@ -45,7 +44,8 @@ void setup() {
when(eventSourceContextMock.getControllerConfiguration()).thenReturn(mockControllerConfig);
when(mockControllerConfig.getConfigurationService())
.thenReturn(mock(ConfigurationService.class));

when(mockContext.managedDependentResourceContext())
.thenReturn(mock(ManagedDependentResourceContext.class));
when(mockContext.getWorkflowExecutorService()).thenReturn(executorService);
when(mockContext.eventSourceRetriever()).thenReturn(eventSourceRetrieverMock);
}
Expand Down
5 changes: 3 additions & 2 deletions ...javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,26 @@

import io.javaoperatorsdk.operator.AggregatedOperatorException;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.ManagedDependentResourceContext;
import io.javaoperatorsdk.operator.processing.event.EventSourceRetriever;
import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;

import static io.javaoperatorsdk.operator.processing.dependent.workflow.ExecutionAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.*;

@SuppressWarnings("rawtypes")
class WorkflowReconcileExecutorTest extends AbstractWorkflowExecutorTest {

@SuppressWarnings("unchecked")
Context<TestCustomResource> mockContext = mock(Context.class);
Context<TestCustomResource> mockContext = spy(Context.class);
ExecutorService executorService = Executors.newCachedThreadPool();

TestDependent dr3 = new TestDependent("DR_3");
TestDependent dr4 = new TestDependent("DR_4");

@BeforeEach
void setup() {
when(mockContext.managedDependentResourceContext()).thenReturn(mock(ManagedDependentResourceContext.class));
when(mockContext.getWorkflowExecutorService()).thenReturn(executorService);
when(mockContext.eventSourceRetriever()).thenReturn(mock(EventSourceRetriever.class));
}
Expand Down
Loading