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: standalone workflows now fail on top-level cycles #1997

Merged
merged 5 commits into from
Aug 7, 2023

Conversation

yarinkos
Copy link
Contributor

@yarinkos yarinkos commented Aug 6, 2023

Workflow toplevel resoruces size

The next looks like a runtime deadlock issue , and I would like to suggest a fix
There is no point for a workflow builder with zero size toplevel resources ( A bit illogically ).

The following builder isn't common , but it demonstrates how wrong workflow configuration could lead for a deadlock state (topLevelResources.size() == 0, cycle detection still does not implemented )

  initDependentResources(kubernetesClient);
  workflow = new WorkflowBuilder<WebPage>()
        .addDependentResource(configMapDR)
        .addDependentResource(deploymentDR).dependsOn(configMapDR)
        .addDependentResource(serviceDR).dependsOn(deploymentDR)
        .addDependentResource(ingressDR).dependsOn(serviceDR)
            .addDependentResource(configMapDR).dependsOn(serviceDR)
            .build();

For cases where the initializations aren't logical, this exception throw can comes in
Hope is following the guidelines and the project roadmap.
r.m.k

@openshift-ci openshift-ci bot requested review from adam-sandor and metacosm August 6, 2023 18:21
@csviri
Copy link
Collaborator

csviri commented Aug 7, 2023

Hi @yarinkos , thank you, this makes sense to some extent, just few points.

  1. please run mvn clean install it will format the code, that is why the build is failing.
  2. please add unit test for this

Note that this not necessarily detects cycles in all cases.

@csviri
Copy link
Collaborator

csviri commented Aug 7, 2023

There is cycle detection for managed case: https://github.com/java-operator-sdk/java-operator-sdk/blob/077bbc2f224ee7fcc9da933aefd6c46aa51f95dd/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowSupport.java#L59-L59

We could also properly do cycle detection to standalone mode. Still consider this as a good first step.

@@ -79,6 +79,9 @@ private Map<String, DependentResourceNode> toMap(Set<DependentResourceNode> node
}
map.put(node.getName(), node);
}
if (topLevelResources.size() == 0) {
throw new IllegalStateException("Top level dependent resources set size must be bigger than 0 .");
Copy link
Collaborator

Choose a reason for hiding this comment

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

A more explicit message would be better as this doesn't really help the user fix the problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here just wanted to comment that :)
But basically as mentioned below, this just detects in some cases the cycles. But yes would check here if there is more than 0 resources, and if there is no top then it's a cycle.

@@ -79,6 +79,9 @@ private Map<String, DependentResourceNode> toMap(Set<DependentResourceNode> node
}
map.put(node.getName(), node);
}
if (topLevelResources.size() == 0) {
throw new IllegalStateException("Top level dependent resources set size must be bigger than 0 .");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here just wanted to comment that :)
But basically as mentioned below, this just detects in some cases the cycles. But yes would check here if there is more than 0 resources, and if there is no top then it's a cycle.

@yarinkos yarinkos requested review from csviri and metacosm August 7, 2023 12:18
@metacosm metacosm changed the title feat(): validate toplevel size fix: standalone workflows now fail on top-level cycles Aug 7, 2023
Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

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

Thank you!

@csviri csviri merged commit d2fdf38 into operator-framework:main Aug 7, 2023
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.

3 participants