Add the ability of Flyteadmin to force terminate an execution #5833
Conversation
RRap0so
force-pushed
the
add-force-termination-admin
branch
from
1f5513a
to
f33059a
Compare
RRap0so
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5833 +/- ##
==========================================
+ Coverage 34.48% 36.33% +1.84%
==========================================
Files 1138 1304 +166
Lines 102742 110152 +7410
==========================================
+ Hits 35434 40026 +4592
- Misses 63634 65956 +2322
- Partials 3674 4170 +496
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
RRap0so
force-pushed
the
add-force-termination-admin
branch
from
65df90f
to
d30b87e
Compare
* Refactor k8s executor abort Signed-off-by: Rafael Raposo <[email protected]> Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
RRap0so
force-pushed
the
add-force-termination-admin
branch
from
75113b5
to
2cc10ea
Compare
|
@wild-endeavor this is a follow up to #5788 that added finalizers to flyteworkflows on creation as opposed to waiting for propeller to pick them up and add finalizers. This was to handle a scenario where flyteworkflow resources were getting deleted prior to getting finalizers set causing issues with subworklow status getting stuck. I'll take a look at this PR this evening. |
| PropagationPolicy: &deletePropagationBackground, | ||
| }) | ||
|
|
||
| w, err := target.FlyteClient.FlyteworkflowV1alpha1().FlyteWorkflows(data.Namespace).Get(ctx, data.ExecutionID.GetName(), v1.GetOptions{}) |
There was a problem hiding this comment.
could you put this fetch under if data.Force so we don't do this fetch if we're just deleting without removing finalizers first
| @@ -94,9 +95,35 @@ func (e K8sWorkflowExecutor) Abort(ctx context.Context, data interfaces.AbortDat | |||
| if err != nil { | |||
| return errors.NewFlyteAdminErrorf(codes.Internal, err.Error()) | |||
| } | |||
| err = target.FlyteClient.FlyteworkflowV1alpha1().FlyteWorkflows(data.Namespace).Delete(ctx, data.ExecutionID.GetName(), v1.DeleteOptions{ | |||
There was a problem hiding this comment.
I think we still want to re-add this after the if data.force{... code block
| @@ -30,9 +32,18 @@ func RemoteDeleteCommand() *cobra.Command { | |||
| Short: deleteCmdShort, | |||
| Long: deleteCmdLong, | |||
| } | |||
| var force bool | |||
| deleteCmd.PersistentFlags().BoolVarP(&force, "force", "f", false, "Force deletion without confirmation") | |||
There was a problem hiding this comment.
I'm unfamiliar if there are any examples, but would be nice if just needed to include the force flag.
Command could be:
flytectl delete execution kxd1i72850 -d development -p flytesnacks --force
Only comment is that we need to ensure that all k8s resources that Flyte starts are cleaned up as part of removing finalizers and deleting the CR. IIUC this is meant to be used on already terminal Flyte workflows, but there are no safeguards here - a user could "force" delete an active execution and that may orphan Pods. Am I understanding this correctly? |
|
I thought about this more, I think it's an ok change because the finalizer isn't what guarantees cleanup, it's the OwnerReference and the propagation delete policy. The finalizer is there to allow propeller to act on the delete request for the workflow CRD. |
Tracking issue
Related to #5832
Why are the changes needed?
Add a force flag on the server side of Flyte to be able to terminate executions if FlytePropeller is not running in the kubernetes cluster.
What changes were proposed in this pull request?
This adds the force bool into
ExecutionTerminateRequest. This in turn will be passed to thek8s_executor. If it's a Forced termination, it will remove the finalizer from the execution CRD and delete.If it's not forced it keeps doing just a deletion (like it was before)
How was this patch tested?
Unit tested.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
TBD
Docs link
TBD