Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail suspended steps after deadline #1704

Merged
merged 4 commits into from
Nov 7, 2019

Conversation

markterm
Copy link
Contributor

Suspended steps would never time out, regardless of the setting of activeDeadlineSeconds.

This change fixes that.

@xianlubird
Copy link
Member

Should we add a example or tests file?

@markterm
Copy link
Contributor Author

There's already an example with activeDeadlineSeconds, but I've added a test.

@sarabala1979
Copy link
Member

looks like it is only working on activeDeadlineSeconds: 0 you need to requeue the wf to keep checking the deadlinseconds. Try the below example.

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: suspend-template1
spec:
  entrypoint: suspend
  activeDeadlineSeconds: 5
  templates:
    - name: suspend
      suspend: {}

@markterm
Copy link
Contributor Author

markterm commented Nov 5, 2019 via email

@sarabala1979
Copy link
Member

Yes, you are right, it is an existing issue. I can see every 10 mins fresh all WF refresh. We need to come up with some implementation like

  1. Keep requeue the WF if it suspended with duration or WF has activedeadlinseconds
  2. Implement scheduler to requeue the WF with a specified time duration (suspend->duration or activedeadlineseconds).

if we have the above implementation then the controller will behave the expected behavior.

@sarabala1979
Copy link
Member

Workflow code is already using c.workqueue.AddAfter(key, addAfter) Can you use this to requeue the suspended wf after activedeadlineseconds?

@markterm
Copy link
Contributor Author

markterm commented Nov 7, 2019

@sarabala1979 I have applied this to requeue suspended steps.

@sarabala1979 sarabala1979 merged commit fae7382 into argoproj:master Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants