CronWorkflow suspend: incorrect manifest

[DanCardin]

Pre-bug-report checklist

1. This bug can be reproduced using pure Argo YAML

• [ ] Yes 👉 Please report a bug to the Argo Workflows GitHub 👈
• [x] No

If yes, it is more likely to be an Argo bug unrelated to Hera. Please double check before submitting an issue to Hera.

2. This bug occurs in Hera when...

• [x] exporting to YAML
• [x] submitting to Argo
• [ ] running on Argo with the Hera runner
• [ ] other:

Bug report

Describe the bug

CronWorkflow(
    name="foo",
    schedule="* * * * *",
    workflow_template_ref=models.WorkflowTemplateRef(name="bar"),
    suspend=True,
)

Generates:

apiVersion: argoproj.io/v1alpha1
kind: CronWorkflow
metadata:
  name: foo
spec:
  schedule: 0 */6 * * *
  workflowSpec:
    suspend: true
    workflowTemplateRef:
      name: bar

This does not prevent the workflow from being scheduled, as expected. Instead, it regularly schedules the workflows, but the workflow itself is suspended and stays running forever.

Expected behavior As far as i can tell from the docs (and from clicking the suspend checkbox inside Workflows), it should generate

apiVersion: argoproj.io/v1alpha1
kind: CronWorkflow
metadata:
  name: foo
spec:
  schedule: 0 */6 * * *
  suspend: true
  workflowSpec:
    workflowTemplateRef:
      name: bar

Environment

• Hera Version: 5.15.1
• Python Version: 3.10
• Argo Version: 3.5.1

---

As far as I can tell, this stems from https://github.com/argoproj-labs/hera/blob/main/src/hera/workflows/cron_workflow.py#L153 not special casing the suspend field, like it does the schedule field.

It seems like it could be fixed by something alike:

        model_workflow = cast(_ModelWorkflow, super().build())
+       model_workflow.spec.suspend = None
        model_cron_workflow = _ModelCronWorkflow(
            metadata=model_workflow.metadata,
            spec=CronWorkflowSpec(
                schedule=self.schedule,
+               suspend=self.suspend,
                workflow_spec=model_workflow.spec,
            ),
        )

to exactly produce the above yaml. With the caveat, that if you did want the suspend inside workflowSpec, this makes that impossible, for whatever that's worth...

Im happy to supply a PR if this makes sense?

[DanCardin]

for what it's worth, I ran into the relative complexity caused by CronWorkflow subclassing Workflow in https://github.com/argoproj-labs/hera/pull/942.

imo, while subclassing Workflow for the DRYness of its fields is convenient, the complexity added downstream ends up being more confusing due to fields like this.

It's perhaps beyond the scope of this specific bug report, but it might be better to construct a mixin that lets them both share the methods that are mutually applicable, while redefining the actual attributes on CronWorkflow and properly assigning the spec path, so that the build method doesn't need to pretend it's a workflow and individually handle the colliding attributes.

[DanCardin]

🥹 there's a whole separate cron_suspend?! gah. Well that probably solves this bug-wise...

I still probably/maybe think something about this issue indicates something maybe ought to change...

[elliotgunton]

This is now more prominently documented in the fixed API docs (from #1415) at the top of the CronWorkflow page.

I don't think there's anything to do in Hera here, the unfortunate naming collision is a problem from the Argo Workflows spec itself, and I don't see how/why CronWorkflow should not a subclass of Workflow when looking at it from the Python perspective (it's a signal to developers that it will act like a workflow, but with a cron schedule). It's either use

• cron_suspend for spec.suspend, and suspend for the workflowSpec.suspend; or
• suspend for spec.suspend which is now doing something different (suspending the CronWorkflow vs the generated Workflow itself) and (something like) workflow_spec_suspend for the workflowSpec.suspend

Overall not a great situation but I prefer cron_suspend.

[DanCardin]

I personally would prefer something like CronWorkflow(cron='', suspend=True, workflow=Workflow(...)), because i feel like that actually more closely reflects the actual specs. Realizing of course that it'd either be a breaking change, require a 2nd impl, or have very weird semantics; so it's not like it's a free and easy choice either.

And for the record in the current impl, i do think suspend == cron.suspend and workflow_suspend == workflowSpec.suspend makes more sense, even if it's not a good idea because of how breaking a change it'd be. Firstly, because you're describing a CronWorkflow object, not a workflow, so imo all conflicting attribute to namespace the workflow-side, not the cron-side; and secondly because suspending the workflow inside the cron (afaict?) has no practical use.

I'd have to look, but i assume labels/annotations would be similarly conflicting and "incorrectly" map to the underlying workflow definition rather than the CronWorkflow.

With all that said, it's not a great situation because "fixing" it in any direction is a breaking change or a duplicate API for the same thing.

[elliotgunton]

something like CronWorkflow(cron='', suspend=True, workflow=Workflow(...))

Yeah I was trying to imagine something like this while maintaining the context behaviour, I guess users would do:

with Workflow(...) as w:
    # stuff

cron_workflow = CronWorkflow(..., workflow=w)

more closely reflects the actual specs

Yes there's a careful balance to be had. I think both approaches (the existing vs the above) have merits, and can potentially live side-by-side.

suspending the workflow inside the cron (afaict?) has no practical use

Yeah I came to the same conclusion - I don't think it has any practical use at submission time for normal workflows, it's just a way for K8s to track a suspended Workflow (whether by CLI, UI or suspend template).

---

Just wanted to say I appreciate the discussion and great ideas! 🙏 🚀 I'm starting to think about collating a list of breaking changes to take into a v6 release (although no concrete plans right now) so we can fix any mistakes from v5.