Proposal: Automatically suspend CronWorkflow for repeated errors in child workflows

[terrytangyuan]

Summary

What change needs making?

Automatically suspend a CronWorkflow for repeated errors in its workflows.

Use Cases

When would you use this?

When there are repeated errors observed in the child workflows, future child workflows would likely also fail. In this case, the controller could automatically suspend CronWorkflow from submitting new child workflows to avoid waste of computational resources.

Implementation =-wise, we could:

1. Introduce a field suspendFailedJobsLimit bool (probably need a better name) in CronWorkflow to specify the maximum number of consecutive failed workflows before we suspend the corresponding CronWorkflow.
2. Record statuses of child workflows in CronController.
3. Suspend the corresponding CronWorkflow when the threshold is reached.

---

Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.

[terrytangyuan]

@alexec I can work on this if you think this is a reasonable feature to have. We currently have a hacky workaround and it would be good to implement the logic in the controller itself.

[simster7]

I think this is a good idea

[terrytangyuan]

I created a quick draft PR in #5662 and left some comments so that we can discuss pros and cons of different designs in several places.

[terrytangyuan]

Addressed in #12305

[agilgur5]

Addressed in #12305

Automatically suspend

@terrytangyuan per #12696 (comment), #12305's stopStrategy completely stops a CronWorkflow, it cannot be resumed after. The term suspend in Argo refers to a temporary pause that can be manually resumed again.

So it seems like this is not entirely addressed in #12305, a separate suspendStrategy would be needed

[Joibel]

This feels like just a difference in the words used, not a difference in function.

You could patch the phase away from Stopped and remove the label and it would be resumed. I'm slightly unsure why we have both a label and phase in hindsight now - I feels like we don't need phase.

We don't have a CLI command to patch back to Active, which would be a useful convenience.

[agilgur5]

No the functional difference was very intentional per #12696 (comment), as linked above. I literally asked about the difference and suggested a rename initially.

You could patch

If you put it back to Active, it would stop again if the history were not modified as well.

We don't have a CLI command to patch back to Active, which would be a useful convenience.

The UI also (intentionally) treats it differently

[agilgur5]

Re-opening per my above comments, a suspendStrategy would be needed for this and #7291