Added the ability to terminate a run #528

Ark-kun · 2018-12-12T23:05:38Z

This change is

yebrahim · 2018-12-12T23:49:34Z

backend/src/apiserver/server/run_server.go

@@ -122,6 +122,14 @@ func (s *RunServer) validateCreateRunRequest(request *api.CreateRunRequest) erro
 	return nil
 }

+func (s *RunServer) TerminateRun(ctx context.Context, request *api.TerminateRunRequest) (*empty.Empty, error) {


Why return the empty object? Why not just the error?

I copied the style from func (s *JobServer) DisableJob(ctx context.Context, request *api.DisableJobRequest) (*empty.Empty, error) {

AFAIK, there was some compilation problem if the result was just an error. I'll try it again in the new code base.

The code generated from .proto has this signature:

pipelines/backend/api/go_client/run.pb.go

Line 1197 in a374585

TerminateRun(context.Context, *TerminateRunRequest) (*empty.Empty, error)

All of the methods in run.proto have similar interface:

pipelines/backend/api/run.proto

Line 81 in a374585

rpc ArchiveRun(ArchiveRunRequest) returns (google.protobuf.Empty) {

Do you think this should be changed? Maybe we should change this everywhere.

Yes but I'll let @IronPan weigh in on the style, there might be a good reason behind it.

backend/src/apiserver/storage/run_store.go

yebrahim · 2018-12-13T00:01:39Z

backend/src/common/client/api_server/run_client_fake.go

@@ -74,3 +74,14 @@ func (c *RunClientFake) ListAll(params *runparams.ListRunsParams, maxResultSize
 	[]*runmodel.APIRun, error) {
 	return listAllForRun(c, params, maxResultSize)
 }
+
+func (c *RunClientFake) Terminate(params *runparams.TerminateRunParams) error {


We'll need an integration test for this, can you add a TODO and open an issue to track it?

vicaire · 2018-12-13T01:36:19Z

(I am assuming this is WIP because of the title. Let me know when ready).

…

On Wed, Dec 12, 2018 at 5:00 PM Alexey Volkov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In backend/src/apiserver/server/run_server.go <#528 (comment)>: > @@ -122,6 +122,14 @@ func (s *RunServer) validateCreateRunRequest(request *api.CreateRunRequest) erro return nil } +func (s *RunServer) TerminateRun(ctx context.Context, request *api.TerminateRunRequest) (*empty.Empty, error) { I copied the style from func (s *JobServer) DisableJob(ctx context.Context, request *api.DisableJobRequest) (*empty.Empty, error) { AFAIK, there was some compilation problem if the result was just an error. I'll try it again in the new code base. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#528 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYkFTcri-wJtKYCeIVDQ5ASF231krgfCks5u4aaogaJpZM4ZQnbv> .

vicaire · 2018-12-14T07:04:40Z

backend/api/go_http_client/run_client/run_service/run_service_client.go

+/*
+TerminateRun terminate run API
+*/
+func (a *Client) TerminateRun(params *TerminateRunParams, authInfo runtime.ClientAuthInfoWriter) (*TerminateRunOK, error) {


For each API that you call, please have an abstract client with a fake implementation (for tests) and a real implementation (for production).

See this example for the persistence agent:
https://github.com/kubeflow/pipelines/tree/master/backend/src/agent/persistence/client

workflow_fake.go has been updated to fake this behavior.
see: https://github.com/kubeflow/pipelines/pull/528/files#diff-b43f69f9e25073a72d6ebec688536c24

Thank you Riley. Looks like this PR is still WIP according to the title. Let me know when it is ready for review. Thanks!

backend/src/apiserver/server/run_server.go

yebrahim · 2019-01-26T00:59:26Z

backend/src/cmd/ml/cmd/run.go

+		// Validation
+		Args: func(cmd *cobra.Command, args []string) error {
+			runID, err = ValidateSingleString(args, "ID")
+			if err != nil {


Similar to above, replace these four lines with return err?

No generated files for now.

Now the call chain is run_client->run_server->resource_manager->run_store

…g the workflow.

Added successful test in resource_manager_test.go and completed, barring nits and conventions, the implementation of Patch() and isTerimated() within workflow_fake.go Additional tests and lots of clean-up are still necessary

…store_tests

vicaire · 2019-03-22T00:54:54Z

/lgtm
/approve

k8s-ci-robot · 2019-03-22T00:54:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vicaire

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:

~~backend/OWNERS~~ [vicaire]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-03-22T00:54:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vicaire

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:

~~backend/OWNERS~~ [vicaire]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ne params (kubeflow#528) * fix regex parsing for custom task * fix lint

k8s-ci-robot added the do-not-merge/work-in-progress label Dec 12, 2018

k8s-ci-robot requested review from IronPan and vicaire December 12, 2018 23:05

k8s-ci-robot added the size/XL label Dec 12, 2018

yebrahim reviewed Dec 13, 2018

View reviewed changes

vicaire suggested changes Dec 14, 2018

View reviewed changes

Ark-kun force-pushed the terminate_run_new branch from 8664caf to a374585 Compare January 25, 2019 01:55

yebrahim reviewed Jan 26, 2019

View reviewed changes

backend/src/apiserver/server/run_server.go Show resolved Hide resolved

yebrahim reviewed Jan 26, 2019

View reviewed changes

rileyjbauer force-pushed the terminate_run_new branch from a374585 to 2ad53fe Compare February 27, 2019 21:06

k8s-ci-robot added size/XXL and removed size/XL labels Mar 20, 2019

rileyjbauer force-pushed the terminate_run_new branch from 3728787 to 6053e4b Compare March 20, 2019 18:39

Ark-kun and others added 11 commits March 20, 2019 16:52

Added the terminate run command to backend and CLI.

06dc66a

No generated files for now.

Added the generated files.

18c64db

Moved the code to run_store.go

dda28be

Now the call chain is run_client->run_server->resource_manager->run_store

Using the backoff package for retries.

0c18c5a

Trying to update run status in the DB to "Terminating" before patchin…

123af88

…g the workflow.

Stopped using the Argo errors module.

78e39d2

Fixed the compilation errors due to recent backend changes.

a9dea94

Adds a few more tests to resource_manager_test and cleans up a bit

b84cdce

Further clean up. Stopped using squirrel for UPDATE query. Added run_…

285e174

…store_tests

Adds terminate run integration test

dfb3524

rileyjbauer force-pushed the terminate_run_new branch from ce2616a to dfb3524 Compare March 20, 2019 23:56

rileyjbauer added 2 commits March 20, 2019 17:38

Undo changes to go.sum

ab5b5d6

Fixes path to long-running.yaml in integration test

a6a52ac

Allow runs with no Conditions to be terminated

eb4e3ac

vicaire approved these changes Mar 22, 2019

View reviewed changes

k8s-ci-robot assigned vicaire Mar 22, 2019

k8s-ci-robot added lgtm labels Mar 22, 2019

k8s-ci-robot added the approved label Mar 22, 2019

rileyjbauer removed the do-not-merge/work-in-progress label Mar 22, 2019

rileyjbauer changed the title ~~[WIP]Added the ability to terminate a run~~ Added the ability to terminate a run Mar 22, 2019

k8s-ci-robot merged commit f98ec68 into kubeflow:master Mar 22, 2019

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023

Remove redundant code which is confusing (kubeflow#528)

553b7a1

HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024

fix(sdk): Fix regex parsing for custom task to handle multiple pipeli…

b74d4a5

…ne params (kubeflow#528) * fix regex parsing for custom task * fix lint

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the ability to terminate a run #528

Added the ability to terminate a run #528

Ark-kun commented Dec 12, 2018 •

edited

Loading

yebrahim Dec 12, 2018

Ark-kun Dec 13, 2018

Ark-kun Jan 25, 2019

yebrahim Jan 26, 2019

yebrahim Dec 13, 2018

vicaire commented Dec 13, 2018 via email

vicaire Dec 14, 2018

rileyjbauer Feb 27, 2019

vicaire Feb 28, 2019

yebrahim Jan 26, 2019

rileyjbauer Feb 27, 2019

vicaire commented Mar 22, 2019

k8s-ci-robot commented Mar 22, 2019

k8s-ci-robot commented Mar 22, 2019

Added the ability to terminate a run #528

Added the ability to terminate a run #528

Conversation

Ark-kun commented Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicaire commented Dec 13, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicaire commented Mar 22, 2019

k8s-ci-robot commented Mar 22, 2019

k8s-ci-robot commented Mar 22, 2019

Ark-kun commented Dec 12, 2018 •

edited

Loading