Support archiving/unarchiving runs on the backend #552

yebrahim · 2018-12-17T22:41:25Z

Part of #247.

This PR adds two new endpoints to archive and unarchive runs. The proto field is an enum to make this easier to extend in the future.

/assign @IronPan
/assign @neuromage

This change is

neuromage · 2018-12-17T23:43:39Z

backend/src/apiserver/resource/resource_manager.go

@@ -31,7 +31,7 @@ import (
 	scheduledworkflow "github.com/kubeflow/pipelines/backend/src/crd/pkg/apis/scheduledworkflow/v1alpha1"
 	scheduledworkflowclient "github.com/kubeflow/pipelines/backend/src/crd/pkg/client/clientset/versioned/typed/scheduledworkflow/v1alpha1"
 	"github.com/pkg/errors"
-	"k8s.io/apimachinery/pkg/apis/meta/v1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"


Nit: can remove explicit name since it's the same.

vscode keeps adding it back with autoformat. I can remove it, but no guarantees it won't pop back up in the future haha.

Haha, no feel free to keep it as is, not a big deal

neuromage · 2018-12-17T23:52:00Z

backend/src/apiserver/model/run.go

@@ -18,6 +18,7 @@ type Run struct {
 	UUID               string `gorm:"column:UUID; not null; primary_key"`
 	DisplayName        string `gorm:"column:DisplayName; not null;"` /* The name that user provides. Can contain special characters*/
 	Name               string `gorm:"column:Name; not null;"`        /* The name of the K8s resource. Follow regex '[a-z0-9]([-a-z0-9]*[a-z0-9])?'*/
+	StorageState       string `gorm:"column:StorageState;"`


Should StorageState have a default value? Maybe we can also add not null, and enforce that all runs are in a valid state (archived or available).

Can we add a test to the CreateRun call to ensure that the state is valid?

Makes sense, done.
Didn't add a new test, just a field to the expected object in old tests, since it's required now.

Btw, this was done in the insertion code path here. Let me know if you think there's a better way. @IronPan @neuromage.

neuromage · 2018-12-18T17:29:06Z

backend/src/apiserver/storage/run_store.go

@@ -238,12 +260,16 @@ func parseResourceReferences(resourceRefString sql.NullString) ([]*model.Resourc
 }

 func (s *RunStore) CreateRun(r *model.RunDetail) (*model.RunDetail, error) {
+	if r.StorageState == "" {


I would default to AVAILABLE if r.StorageState was empty or in state UNKNOWN. If it's specified as one of AVAILABLE or ARCHIVED, I would keep it as is, and if it's neither one of those options, I would return an error.

Something like:

switch r.StorageState {
case "", api.Run_STORAGE_STATE_UNKNOWN.String():
r.StorageState = ...AVAILABLE
case api.Run_STORAGESTATE_ARCHIVED.String(), api.Run_STORAGESTATE_AVAILABLE:
break;
default:
return nil, fmt.Errorf("unknown storage state specified: %q", r.StorageState)
}

But this would put the burden on clients to fill in a value for that field. I think this isn't something clients should be forced to do, this is why I also removed the UNSPECIFIED enum option.
wdyt?

Sync'ed offline. Added some validation around this, PTAL.

neuromage

/lgtm

neuromage · 2018-12-18T20:11:24Z

backend/src/apiserver/storage/run_store.go

@@ -262,7 +262,11 @@ func parseResourceReferences(resourceRefString sql.NullString) ([]*model.Resourc
 func (s *RunStore) CreateRun(r *model.RunDetail) (*model.RunDetail, error) {
 	if r.StorageState == "" {
 		r.StorageState = api.Run_STORAGESTATE_AVAILABLE.String()
+	} else if r.StorageState != api.Run_STORAGESTATE_AVAILABLE.String() &&
+		r.StorageState != api.Run_STORAGESTATE_ARCHIVED.String() {
+		return nil, util.NewInvalidInputError("Invalid value for StorageState field: %s.", r.StorageState)


nit: %q will quote the invalid storage set.

Good tip, done.

neuromage · 2018-12-18T23:28:23Z

/lgtm

yebrahim · 2018-12-19T06:23:21Z

/test kubeflow-pipeline-e2e-test

neuromage

/lgtm

Reviewed 18 of 18 files at r1, 9 of 9 files at r2, 2 of 2 files at r3, 2 of 2 files at r4, 11 of 11 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yebrahim, @IronPan, and @vicaire)

backend/src/common/client/api_server/run_client.go, line 155 at r5 (raw file):

		return util.NewUserError(err,
			fmt.Sprintf("Failed to archive runs. Params: '%+v'", parameters),

nit: failed to 'unarchive runs'. Do we need the second error too? Seems to be saying the same thing.

yebrahim · 2018-12-19T18:31:22Z

backend/src/common/client/api_server/run_client.go, line 155 at r5 (raw file):

Previously, neuromage (Ajay Gopinathan) wrote…

nit: failed to 'unarchive runs'. Do we need the second error too? Seems to be saying the same thing.

One is internal and the other external, so they can have the same message.
Fixed the typo.

yebrahim · 2018-12-19T18:54:59Z

/test kubeflow-pipeline-e2e-test

neuromage · 2018-12-19T20:01:04Z

/lgtm

IronPan

/lgtm

IronPan · 2018-12-19T21:56:17Z

/approve

k8s-ci-robot · 2018-12-19T21:56:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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~~ [IronPan]

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 · 2018-12-19T21:56:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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~~ [IronPan]

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

Yasser Elsayed added 10 commits December 17, 2018 13:54

skip integration tests when unit test flag is set to true

f7e32fa

wip

e3a4137

add StorageState enum to proto

35c238b

add StorageState to model

28b8d04

archive proto/model changes

dec5737

wip archive endpoint

8e7fb1d

wip adding tests

0f032d0

archive test

320f91f

unarchive proto and implementation

a7d2a9e

cleanup

5314182

k8s-ci-robot assigned IronPan and neuromage Dec 17, 2018

k8s-ci-robot added size/XXL labels Dec 17, 2018

k8s-ci-robot requested review from IronPan and vicaire December 17, 2018 22:41

neuromage reviewed Dec 17, 2018

View reviewed changes

Yasser Elsayed added 2 commits December 17, 2018 16:39

make storage state required, with a default value

0d750e4

remove unspecified value from storage state enum

ebe6437

neuromage reviewed Dec 18, 2018

View reviewed changes

pr comments

4953afb

k8s-ci-robot added the lgtm label Dec 18, 2018

neuromage reviewed Dec 18, 2018

View reviewed changes

pr comments

83fb021

k8s-ci-robot removed the lgtm label Dec 18, 2018

k8s-ci-robot added lgtm labels Dec 18, 2018

k8s-ci-robot removed the lgtm label Dec 19, 2018

yebrahim force-pushed the be-archive-bit branch from cc7ef20 to 78ce694 Compare December 19, 2018 06:47

fix archive/unarchive endpoints, add api integration test

ec339c9

yebrahim force-pushed the be-archive-bit branch from 78ce694 to ec339c9 Compare December 19, 2018 17:46

neuromage reviewed Dec 19, 2018

View reviewed changes

k8s-ci-robot added lgtm labels Dec 19, 2018

typo

561aa75

k8s-ci-robot removed the lgtm label Dec 19, 2018

k8s-ci-robot added lgtm labels Dec 19, 2018

IronPan reviewed Dec 19, 2018

View reviewed changes

k8s-ci-robot added the approved label Dec 19, 2018

k8s-ci-robot merged commit 549a366 into kubeflow:master Dec 19, 2018

yebrahim deleted the be-archive-bit branch December 19, 2018 22:09

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

kubeflow#552 Fixes a bad config for 0.2.1 (kubeflow#553)

1bb2d12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support archiving/unarchiving runs on the backend #552

Support archiving/unarchiving runs on the backend #552

yebrahim commented Dec 17, 2018 •

edited by jlewi

Loading

neuromage Dec 17, 2018

yebrahim Dec 18, 2018

neuromage Dec 18, 2018

neuromage Dec 17, 2018

yebrahim Dec 18, 2018

yebrahim Dec 18, 2018

neuromage Dec 18, 2018

yebrahim Dec 18, 2018

yebrahim Dec 18, 2018

neuromage left a comment

neuromage Dec 18, 2018

yebrahim Dec 18, 2018

neuromage commented Dec 18, 2018

yebrahim commented Dec 19, 2018

neuromage left a comment

yebrahim commented Dec 19, 2018

yebrahim commented Dec 19, 2018

neuromage commented Dec 19, 2018

IronPan left a comment

IronPan commented Dec 19, 2018

k8s-ci-robot commented Dec 19, 2018

k8s-ci-robot commented Dec 19, 2018

Support archiving/unarchiving runs on the backend #552

Support archiving/unarchiving runs on the backend #552

Conversation

yebrahim commented Dec 17, 2018 • edited by jlewi 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

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

neuromage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neuromage commented Dec 18, 2018

yebrahim commented Dec 19, 2018

neuromage left a comment

Choose a reason for hiding this comment

yebrahim commented Dec 19, 2018

yebrahim commented Dec 19, 2018

neuromage commented Dec 19, 2018

IronPan left a comment

Choose a reason for hiding this comment

IronPan commented Dec 19, 2018

k8s-ci-robot commented Dec 19, 2018

k8s-ci-robot commented Dec 19, 2018

yebrahim commented Dec 17, 2018 •

edited by jlewi

Loading