flyteorg / flyte

Scalable and flexible workflow orchestration platform that seamlessly unifies data, ML and analytics stacks.
https://flyte.org
Apache License 2.0
5.72k stars 641 forks source link

[BUG] Flyteadmin shouldn't allow launching an execution in archived state #1354

Open pmahindrakar-oss opened 3 years ago

pmahindrakar-oss commented 3 years ago

Describe the bug Each launch plan can be in one of two states.

Admin exposes an api to toggle between these two states.

But in case a user launches an execution for a launchplan then this state flag is ignored.

Expected behavior Flyteadmin create execution should throw an error in case the launch plan state is archived.

[Optional] Additional context To Reproduce Steps to reproduce the behavior:

  1. update the launch plan state to archived
  2. create an execution for the same launchplan

Screenshots If applicable, add screenshots to help explain your problem.

pmahindrakar-oss commented 3 years ago

cc : @katrogan @kumare3

EngHabu commented 3 years ago

I think we should not fail all archived requests... It's ok to fail user-initiated launches (like from flytectl/UI) but if the origin of the launch is system (e.g. propeller), it should let it be...

kumare3 commented 3 years ago

@EngHabu I guess you are saying so for the cases when we have a nested launchplan within another workflow. But, IMO the solution is that deactivate / deprecate should

  1. Not allow new executions from UI, CLI, Flytecl etc
  2. Not allow new schedules to go through. this can happen because there is potentially a delay between the deactivation and the scheduler actually stopping the schedules. There may be in-progress schedules in the buffer, the sync interval may be too high. The expectation of the user is that on deactivation, immediately no more schedules show up for this workflow.

This can be achieved by allowing the user-agent / origin type of semantics for the execution. For all Flyte components, the origin is always known.

For example we can use ExecutionMode to only allow executions of type SYSTEM maybe with a parent-node-id?

katrogan commented 3 years ago

maybe it's worth decoupling the schedule state (active|inactive) from the launch plan state and then focusing around disallowing archived launch plans from being executed if we want to be strict with semantics

for 2) this should already be the case, upon dequeuing a pending execution we should still validate whether an active version of the reference launch plan exists

@kumare3 are you saying that SYSTEM triggered executions should be exempt from the archive status check?

kumare3 commented 2 years ago

@katrogan I think system triggered executions should not be exempt. Only Propeller triggered ones should be. I really think we should handle this to avoid ugainly user experience

katrogan commented 2 years ago

sounds good. we should probably tackle this issue once the UI changes are in to archive entities and therefore archiving is more broadly usable/accessible

github-actions[bot] commented 1 year ago

Hello 👋, This issue has been inactive for over 9 months. To help maintain a clean and focused backlog, we'll be marking this issue as stale and will close the issue if we detect no activity in the next 7 days. Thank you for your contribution and understanding! 🙏

github-actions[bot] commented 1 year ago

Hello 👋, This issue has been inactive for over 9 months and hasn't received any updates since it was marked as stale. We'll be closing this issue for now, but if you believe this issue is still relevant, please feel free to reopen it. Thank you for your contribution and understanding! 🙏

github-actions[bot] commented 3 months ago

Hello 👋, this issue has been inactive for over 9 months. To help maintain a clean and focused backlog, we'll be marking this issue as stale and will engage on it to decide if it is still applicable. Thank you for your contribution and understanding! 🙏