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

clock-expire: prevent active tasks from being clock-expired #6025

Closed
oliver-sanders opened this issue Mar 15, 2024 · 9 comments · Fixed by #6046
Closed

clock-expire: prevent active tasks from being clock-expired #6025

oliver-sanders opened this issue Mar 15, 2024 · 9 comments · Fixed by #6046
Assignees
Labels
Milestone

Comments

@oliver-sanders
Copy link
Member

oliver-sanders commented Mar 15, 2024

From discussion on #6020

Clock expiry can complete a task.

If the task is already active then it should not be completed in this way.

Note, I think we would expect a clock-expired task to be able to expire between retries.

@oliver-sanders oliver-sanders added the question Flag this as a question for the next Cylc project meeting. label Mar 15, 2024
@oliver-sanders oliver-sanders changed the title should we be able to set the expired output on an active task clock-expire: prevent active tasks from being clock-expired Mar 15, 2024
@oliver-sanders oliver-sanders added small and removed question Flag this as a question for the next Cylc project meeting. labels Mar 15, 2024
@oliver-sanders oliver-sanders added this to the cylc-8.3.0 milestone Mar 19, 2024
@hjoliver
Copy link
Member

Relatedly, we should probably not allow any final output (i.e., expired, submit-failed, failed, succeeded) to be manually set on an active task.

Setting these outputs also changes the task state to a final state, which is inconsistent with having a submitted or running job. And we have previously decided that only cylc kill can kill jobs.

We could decide not to change the state of an active task via cylc set, so you could (e.g.) trigger a downstream success branch while the task is still running, but it seems more reasonable (and consistent with setting final outputs in non-active tasks) to disallow it. The user can always kill the job, then set the final output.

@oliver-sanders
Copy link
Member Author

Relatedly, we should probably not allow any final output (i.e., expired, submit-failed, failed, succeeded) to be manually set on an active task.

No, the proposal explicitly permits this.

https://github.com/cylc/cylc-admin/blob/219740c7bfb49e49049ebee503749e9029d72404/docs/proposal-interventions.md?plain=1#L158-L162

@hjoliver
Copy link
Member

hjoliver commented Mar 20, 2024

No it doesn't 😁 - it only explicitly states that we should be able to cancel a retry chain by setting a final output.

Typically you would do that after a job fails and is waiting for a retry, which is fine.

By active, I meant literally active, i.e. "submitted" or "running", not just "present in the task pool".

Setting an active task to failed leaves an orphaned job out in the world, which can be confusing (I see a bit of this from users, for other reasons), or even dangerous (if the user triggers the task again they'll get duplicate jobs fighting it out on the filesystem).

So, if the task is active, I think we should have to kill it first, then set it to failed (which is always possible, even for short retry delays, because a killed task gets held).

If the job can't be killed, then we have no choice but to orphan it, but that's an edge case (it could be handled automatically on failure to kill, or with --force option).

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Mar 21, 2024

No it doesn't 😁 - it only explicitly states that we should be able to cancel a retry chain by setting a final output.

Yes it does, see also:

https://github.com/cylc/cylc-admin/blob/219740c7bfb49e49049ebee503749e9029d72404/docs/proposal-interventions.md?plain=1#L121-L140

@hjoliver
Copy link
Member

Ah OK, I stand corrected - you linked to the wrong bit of the proposal.

However, my argument above still stands!

The proposal neglects to address the problem of orphaned jobs, which can be avoided simply by requiring a job kill before a state reset (where possible - note I did also state above that we need the ability to force it if the job can't be killed).

@oliver-sanders
Copy link
Member Author

This issue covers the mechanism of clock-expiry which is simple and uncontroversial, more significant change will require a new proposal.

@hjoliver
Copy link
Member

hjoliver commented Mar 22, 2024

This is closely related, hence I raised it here for discussion. If it wasn't controversial (I didn't expect it to be) we could simply change the issue title slightly. But fine - I'll shift it to another issue. We don't need a new proposal for every small detail... #6033

@oliver-sanders
Copy link
Member Author

We don't need a new proposal for every small detail

Full proposal, no, but we should put up an amendment if a change directly contradicts an existing proposal, especially a fresh one that is still being implemented.

@oliver-sanders
Copy link
Member Author

The proposal doesn't make mention of the interaction between clock-expire and active tasks, possibly oversight, but likely because we assumed continuation of Cylc 7 behaviour here?

One related part of the proposal is point 10:

Manually triggering a task should not cause an expiry event.

-- https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal

Suggest tackling that along with this issue rather than shoehorning it into #5640 as the solution will likely overlap.

@oliver-sanders oliver-sanders self-assigned this Mar 25, 2024
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Mar 25, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Mar 25, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Apr 3, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Apr 3, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Apr 3, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Apr 3, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Apr 8, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Apr 10, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Apr 12, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Apr 15, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Apr 19, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this issue Apr 19, 2024
* Active tasks should not be considered for clock-expiry.
  Closes cylc#6025
* Manually triggered tasks should not be considered for clock-expiry.
  Implements proposal point 10
  https://cylc.github.io/cylc-admin/proposal-optional-output-extension.html#proposal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants