fix: provide fallback for 3.4 to 3.5 transition with absent NodeFlag. Fixes #12162

[isubasinghe]

Fixes #12162

Motivation

Modifications

Verification

[agilgur5]

I'm not sure if this would simplify the code, but if we detect a 3.4 Workflow, could we just skip the recursion check on it entirely? Since it did not exist in 3.4

[isubasinghe]

I'm not sure if this would simplify the code, but if we detect a 3.4 Workflow, could we just skip the recursion check on it entirely? Since it did not exist in 3.4

Not really this isn't a recursion check, the PR title is incorrect.
3.4 workflows missed a NodeFlag, 3.5 introduced one.

This fix mandates a NodeFlag to be created now, it takes the lack of a NodeFlag to imply older controller to have created the workflow, then it tries to mimic 3.4 logic.

[agilgur5]

Not really this isn't a recursion check, the PR title is incorrect.
3.4 workflows missed a NodeFlag, 3.5 introduced one.

I changed the title again. I'm not entirely sure I follow the root cause -- your comments in #12162 don't really outline it either.
That issue is about the recursion check added in #11646. NodeFlag was added in #11839 (i'm not entirely certain it's necessary either, since it seems like it could be derived). Are you saying that #11839 caused an infinite loop somewhere that #11646 then detected?

[Joibel]

I'm worried about 3.5 and 3.6 diverging in behaviour with this.

As a less invasive change did you consider scanning the workflow at the beginning of operate to upgrade it?

• If you find a nodeFlags field, abort scan (early dropout for all updated workflows to avoid the cost
• Scan each node and calling CheckHookNode generate and attach a nodeFlags to the nodes as appropriate

This scan would then not be needed in 3.6, but it would be the only difference.

[isubasinghe]

I changed the title again. I'm not entirely sure I follow the root cause -- your comments in #12162 don't really outline it either. That issue is about the recursion check added in #11646. NodeFlag was added in #11839 (i'm not entirely certain it's necessary either, since it seems like it could be derived). Are you saying that #11839 caused an infinite loop somewhere that #11646 then detected?

No worries.

Basically the NodeFlag was added in #11839 along with a function to get all retry nodes by using the NodeFlag field.
The problem is that 3.4 workflows do not have this. This leads to the controller thinking that the retry node was never created. It then throws the controller into an infinite recursion where it tries to executeTemplate recursively.

[isubasinghe]

As a less invasive change did you consider scanning the workflow at the beginning of operate to upgrade it?

No, mostly because the mandating of the NodeFlag effectively provides a blast radius. But I don't think this is needed.
I've re-read the retry logic.

I think I can effectively create a map containing Nodes mapped into their NodeFlags.

Thanks for the suggestion, that is definitely less invasive than this fix.

[isubasinghe]

I am pretty happy with this now actually :)