-
Notifications
You must be signed in to change notification settings - Fork 123
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
Make default terminate API configurable to support graceful termination #631
Conversation
/hold @rafalbigaj Do you think this can satisfy your requirements? Let us know how you would like us to implement this feature into KFP. Thanks. |
@@ -519,7 +519,7 @@ func (r *ResourceManager) ListJobs(filterContext *common.FilterContext, | |||
func TerminateWorkflow(wfClient workflowclient.PipelineRunInterface, name string) error { | |||
patchObj := map[string]interface{}{ | |||
"spec": map[string]interface{}{ | |||
"status": "Cancelled", | |||
"status": common.GetTerminateStatus(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default behavior, do we support user to overwrite it by explicitly specifying the parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR allows the admin to configure this status, not users. If we want to allow users to configure this, we need to have some new annotations/dsl to indicate this new status.
So after the meeting today, we agreed not to expose this feature on the user API level since we don't want users to control the terminate behavior. So this feature should be good to go. |
@ckadner @pugangxa @ScrapCodes maybe one of you can review this PR? thanks. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pugangxa, Tomcli The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Which issue is resolved by this Pull Request:
Resolves #506
Description of your changes:
Make the terminate API status to be configurable for the admin. This way the admin can choose whether to let the finally tasks to run or not.
Environment tested:
python --version
):tkn version
): 0.25.0kubectl version
): 1.18/etc/os-release
):Checklist:
Do you want this pull request (PR) cherry-picked into the current release branch?
Learn more about cherry-picking updates into the release branch.