feat: add containerset retry strategy. Fixes #7290

[NikeNano]

Fixes: #7290

Don't bother creating a PR until you've done this:

• Run make pre-commit -B to fix codegen, lint, and commit message problems.

Create your PR as a draft.

• Your PR needs to pass the required checks before it can be approved. If the check is not required (e.g. E2E tests) it
  does not need to pass.
• Once required tests have passed, you can make it "Ready for review".
• Say how how you tested your changes. If you changed the UI, attach screenshots.

Tips:

• If changes were requested, and you've made them, then dismiss the review to get it looked at again.
• Add you organization to USERS.md if you like.
• You can ask for help!

[alexec]

initial thoughts:

• I don't think you need limit, you already have steps in backoff.
• Do you think we want one strategy for all containers in the set? Or different ones for different containers?

[NikeNano]

• I don't think you need limit, you already have steps in backoff.

Yes, it is not needed, will stick to the steps.

• Do you think we want one strategy for all containers in the set? Or different ones for different containers?

To keep it simpler I think we should have a general one to start of with also based upon the feedback here it seems to solve the most urgent use case.

[alexec]

To keep it simpler I think we should have a general one to start of with also based upon the feedback here it seems to solve the most urgent use case.

Sure. So we can add that in a v2.

[NikeNano]

initial thoughts:

• I don't think you need limit, you already have steps in backoff.

I mixed up it up with wait.Backoff the current implementation of retryStrategy dont have steps in the implementation. The current implementation (not using step) is also similar to the workflow retry strategy that have limit separated:

argo-workflows/pkg/apis/workflow/v1alpha1/workflow_types.go

Line 1538 in 00e2837

type RetryStrategy struct {

. Thus I think we should keep the current implementation not using step but limit separate from the backoff. WDYT @alexec

[NikeNano]

@sarabala1979 could you take a look?

[alexec]

I think there are ways to simplify the code.

[alexec]

more changes requested I'm afraid

[NikeNano]

I have realised that the parsing from v1alpha1.Backoff to wait.Backoff is not correct. The reason for that is that the fields actually don't map 1 to 1(miss understood the docs before) . I tried to use the wait.Backoff type as well in the

type ContainerSetTemplate struct {}

but the generation then fails:

2022/01/27 20:32:26 k8s.io/api/core/v1/generated.proto:12:1: warning: Import k8s.io/apimachinery/pkg/runtime/schema/generated.proto is unused.
2022/01/27 20:32:28 k8s.io/api/policy/v1beta1/generated.proto:11:1: warning: Import k8s.io/apimachinery/pkg/runtime/generated.proto is unused.
k8s.io/api/policy/v1beta1/generated.proto:12:1: warning: Import k8s.io/apimachinery/pkg/runtime/schema/generated.proto is unused.
2022/01/27 20:32:28 github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1/generated.proto:1912:18: "duration" is already defined in "github.com.argoproj.argo_workflows.v3.pkg.apis.workflow.v1alpha1.Backoff".
github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1/generated.proto:1918:19: "factor" is already defined in "github.com.argoproj.argo_workflows.v3.pkg.apis.workflow.v1alpha1.Backoff".
github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1/generated.proto:1910:9: "Backoff" is already defined in "github.com.argoproj.argo_workflows.v3.pkg.apis.workflow.v1alpha1".
github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1/generated.proto:8:1: warning: Import github.com/gogo/protobuf/gogoproto/gogo.proto is unused.
github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1/generated.proto:12:1: warning: Import k8s.io/apimachinery/pkg/runtime/generated.proto is unused.
github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1/generated.proto:13:1: warning: Import k8s.io/apimachinery/pkg/runtime/schema/generated.proto is unused.
2022/01/27 20:32:28 protoc -I . -I /go/src -I /go/src --gogo_out=/go/src /go/src/github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1/generated.proto
2022/01/27 20:32:28 Unable to generate protoc on github.com.argoproj.argo_workflows.v3.pkg.apis.workflow.v1alpha1: exit status 1

I guess we could recreate a new type to solve the issue but that feels a bit hacky. WDYT @alexec ?

[alexec]

You can't have two types named Backoff, so don't use wait.Backoff as a field.

[alexec]

I think we could do with a comment for users here as this gets surfaced in docs. How does the retry work for example?

[NikeNano]

Will update with docs.

[NikeNano]

Thanks for all the feedback @alexec appreciate it, have a hard time to get time off from work so things take slightly longer than I wish for :)