Allow infinite retry of actions until timeout

[deepthidevaki]

To wait for a scaling operation to completed, we port-forward to a gateway and repeatedly query the status. However, it sometimes failed because the port-forwarding disconnected but it was not retried until the action is retried. By default the actions are retried 3 times. So we have configured a long timeout (around 30 minutes) for the query and wait. That means, when the port-forwarding disconnected the action was still running for 30 minutes, with out a successful query response. If we fail the action early, it might result in an incident because the retries is hard-coded to 3.

Instead, as a generic solution for retries, this PR allows infinite retry of all actions until timeout. This can simplify operations that has to repeatedly query and wait. For example, verify readiness or cluster wait, we can in theory remove the loop with in the code and instead can be retried by just adjusting the timeout in the chaos action provider parameters.

PS:- Although with these changes implementation of verify readiness can be simplified, it is not done in this PR to reduce the impact of these changes.

closes #516

[Zelldon]

Thanks for your changes and I understand the idea behind it. Unfortunately, I'm not that happy with the changes yet (where we did the change).

I would like to see it as a parameter either on the cluster subcommand or if you need at the root level which you can use like zbchaos cluster scale --runUntilTimeout, with that we don't change the specifications and have this feature also available in the cli (useful for further experimenting and manual testing).

I hope this makes sense to you :)

[Zelldon]

❌ That should be part of the arguments array, so we don't change the specification which is defined here https://chaostoolkit.org/reference/api/experiment/#action-or-probe-provider

[Zelldon]

I would propose you make it part of the arguments and support this also in the CLI as new parameter, because otherwise we only have a feature now supporting for the worker but not for the cli which I would like to avoid. Especially for testing and trying out the cli is useful.

[deepthidevaki]

Do we have to stick to chaostoolkit spec? 💭

Unfortunately adding the parameter in the command does not solve this problem. We already have a retry implemented in the command. The problem is that port-forwarding is not re-connected until the job is retried. We can only fail the job 3 times because after that it raises an incident and the test fails.

[deepthidevaki]

We can probably create and close port-forwarding in each iteration of the retry loop. But that doesn't look like an optimal solution.

[deepthidevaki]

Also right now, because the default job timeout is 5 minutes there are multiple workers running for the same job if the action takes longer than 5 minutes. For actions similar to scale up, it always takes more than 5 minutes. This is why I want to provide a more generic solution at the chaos worker.

[deepthidevaki]

If we want to stick to the chaostoolkit spec, I think it would also make sense to set "retryUntilTimeout" as the default behaviour. Right now the commands implementation is not respecting the timeout specified anyway. Each command has its own way of handling retry and different timeout. Configured timeout is actually ignored at the command level.

[Zelldon]

That is a long time :D

[Zelldon]

Alternatively would it make sense to maybe change your loop to contain more parts like the port-forwarding? Like you mentioned here?

To wait for a scaling operation to completed, we port-forward to a gateway and repeatedly query the status. However, it sometimes failed because the port-forwarding disconnected but it was not retried until the action is retried.

Or is maybe some error not handled correctly? It should fail if the connection is closed right?

[deepthidevaki]

Alternatively would it make sense to maybe change your loop to contain more parts like the port-forwarding? Like you mentioned here?

To wait for a scaling operation to completed, we port-forward to a gateway and repeatedly query the status. However, it sometimes failed because the port-forwarding disconnected but it was not retried until the action is retried.

Or is maybe some error not handled correctly? It should fail if the connection is closed right?

We intentionally do not fail the command for network errors because if the job fails for 3 times, it raises an incident and causes the test to fail.

[Zelldon]

Would you be open when I take a look at the code whether there is an alternative?

Is it currently causing trouble in our executions?

[Zelldon]

And changing the default retries to more than 3 is not an option?

[deepthidevaki]

And changing the default retries to more than 3 is not an option?

This can be changed only globally, not for a particular action.

Changing it globally would be an alternate solution. #519 (comment)

[deepthidevaki]

Is it currently causing trouble in our executions?

Yes. There was atleast one flaky e2e run because of this.

[Zelldon]

What do you think should we go with that? Feels like the simplest and also acceptable as it should be fine to retry (which is only done on failing actions anyway) and we still have the timeout which get us covered right?

Feels more natural to use the built-in mechanics

[deepthidevaki]

What do you think should we go with that? Feels like the simplest and also acceptable as it should be fine to retry (which is only done on failing actions anyway) and we still have the timeout which get us covered right?

Feels more natural to use the built-in mechanics

Do you mean changing it globally - always retry until timeout for all actions? I like that. I was initially hesitant to do it because I was not sure how it impacts existing chaos experiments. But for me it sounds natural and fitting to our experiment process.

[Zelldon]

Then lets do that 👍🏼 I think you only need to change the action.bpmn process model then :)

[deepthidevaki]

Why change actions.bpmn? My thought was to replace the following the line chaos_worker.go

_, _ = client.NewFailJobCommand().JobKey(job.Key).Retries(job.Retries - 1).Send(ctx)

to

// do not reduce retry count
_, _ = client.NewFailJobCommand().JobKey(job.Key).Retries(job.Retries).Send(ctx)

Is there a way to specify the same behavior in the service task spec?

[Zelldon]

Ah ok :) I was thinking of defining a high retries count https://docs.camunda.io/docs/next/components/modeler/bpmn/service-tasks/#task-definition

<zeebe:taskDefinition type="action" retries="50" />

and then make even use of the backoff stuff maybe 🤷🏼 https://docs.camunda.io/docs/next/apis-tools/go-client/job-worker/#backoff-configuration

[Zelldon]

But I think we could also do it like you have proposed :) we can always change it if we see any issues with it.

[deepthidevaki]

Thanks. Then I will make the following changes:

1. Remove the new parameters
2. Infinite retry until timeout for all actions
3. May be also set a default backoff for all jobs (10s?)

[Zelldon]

Sounds great thanks @deepthidevaki 🙇🏼 👍🏼 Please also add a comment with the reasoning why we have this :)

[Zelldon]

Great thanks @deepthidevaki 🚀

[Zelldon]

👍🏼