Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Enrich TerminateExecution error to tell propeller the execution already terminated #551

Merged
merged 2 commits into from
Apr 16, 2023

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented Apr 11, 2023

TL;DR

TerminateExecution error when a workflow is already in terminal state is PermissionDenied. This changes it to return PreconditionFailed w/ specific grpc status message.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Tracking Issue

fixes flyteorg/flyte#3582

EngHabu added 2 commits April 11, 2023 12:59
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #551 (1f98916) into master (a89b359) will increase coverage by 0.28%.
The diff coverage is 48.27%.

❗ Current head 1f98916 differs from pull request most recent head bd45b17. Consider uploading reports for the commit bd45b17 to get more accurate results

@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
+ Coverage   60.08%   60.37%   +0.28%     
==========================================
  Files         168      170       +2     
  Lines       15010    13155    -1855     
==========================================
- Hits         9019     7942    -1077     
+ Misses       5197     4379     -818     
- Partials      794      834      +40     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/repositories/config/migrations.go 1.01% <0.00%> (-1.97%) ⬇️
pkg/rpc/adminservice/base.go 3.88% <0.00%> (+0.34%) ⬆️
pkg/rpc/adminservice/execution.go 0.00% <0.00%> (ø)
pkg/manager/impl/metrics_manager.go 70.83% <70.83%> (ø)
pkg/repositories/transformers/task_execution.go 76.03% <83.60%> (+3.25%) ⬆️
pkg/repositories/transformers/node_execution.go 75.20% <86.66%> (+5.20%) ⬆️
pkg/manager/impl/util/resources.go 91.52% <91.52%> (ø)
pkg/manager/impl/execution_manager.go 72.59% <92.30%> (+1.21%) ⬆️
pkg/manager/impl/node_execution_manager.go 72.62% <100.00%> (+2.69%) ⬆️
pkg/manager/impl/resources/resource_manager.go 70.03% <100.00%> (+5.88%) ⬆️
... and 7 more

... and 139 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@EngHabu EngHabu marked this pull request as ready for review April 14, 2023 02:01
@EngHabu EngHabu merged commit 1026231 into master Apr 16, 2023
@EngHabu EngHabu deleted the terminate-wf-err branch April 16, 2023 16:48
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
…dy terminated (#551)

* Enrich TerminateWorkflow error to tell propeller the execution already terminated

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Fix unit test

Signed-off-by: Haytham Abuelfutuh <[email protected]>

---------

Signed-off-by: Haytham Abuelfutuh <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Potential race condition in Flyte Propeller
2 participants