feat: Support overriding parameters when retry/resubmit workflows

[terrytangyuan]

Signed-off-by: Yuan Tang terrytangyuan@gmail.com

[jessesuen]

How does overriding parameters work when retrying a workflow which already had Sucessful steps that completed? Since we don't rerun the Successful steps with the new parameters, the retried workflow might have non-deterministic results, right?

[terrytangyuan]

How does overriding parameters work when retrying a workflow which already had Sucessful steps that completed? Since we don't rerun the Successful steps with the new parameters, the retried workflow might have non-deterministic results, right?

Good catch. Actually the parameter overriding was only supposed to work correctly when --restart-successful and --node-field-selector are used when retrying/resubmitting, e.g. nodes that are selected as well as their child nodes will be re-run with the new parameters even if they succeeded.

Is that what you meant? I can add the checks on those two flags.

[jessesuen]

Yes. That's what I meant. I think a warning might be good enough because I think we need to prevent the expectation that parameter overriding may not work with retry. Something like:

Overriding parameters on retried or memoized resubmitted workflows may have unexpected results

[jessesuen]

Changes LGTM but I would request a warning is printed when we detect they are retrying part of the workflow, or doing a memoized resubmit with overridden parameters.

[agilgur5]

I'm inclined to agree with Jesse's logic here regarding retries -- I don't think you should be able to override your parameters, as that makes things non-deterministic and no longer idempotent either. A Workflow only has 1 set of parameters at the top level, and this would make a retried Workflow now potentially have 2 parameters sets; some steps were ran with the initial parameters and others were ran with the overridden parameters.

A memoized resubmit more correctly matches the intent here.

I'm considering removing the retry overriding feature entirely as such.

I mentioned this confusing property of retry overrides in #12734 (review) as well