feat(backend): Forbid unarchive runs that belog to archived experiment (

kubeflow#7147) * [backend] Forbid unarchive runs that belog to archived experiment Adds validation so that the user cannot activate a Run that belongs to Archived Experiment. Issue: kubeflow#7014 Signed-off-by: Diana Atanasova <[email protected]> * Change the err message to be more action-driven Signed-off-by: Diana Atanasova <[email protected]>
abaland · May 29, 2022 · 4e4cb37 · 4e4cb37
1 parent 33b395a
commit 4e4cb37
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 7 deletions.
diff --git a/backend/src/apiserver/resource/resource_manager.go b/backend/src/apiserver/resource/resource_manager.go
@@ -390,14 +390,14 @@ func (r *ResourceManager) CreateRun(ctx context.Context, apiRun *api.Run) (*mode
 
 	// Patched the default value to apiRun
 	if common.GetBoolConfigWithDefault(common.HasDefaultBucketEnvVar, false) {
-	for _, param := range apiRun.PipelineSpec.Parameters {
-		var err error
-		param.Value, err = common.PatchPipelineDefaultParameter(param.Value)
-		if err != nil {
-			return nil, fmt.Errorf("failed to patch default value to pipeline. Error: %v", err)
+		for _, param := range apiRun.PipelineSpec.Parameters {
+			var err error
+			param.Value, err = common.PatchPipelineDefaultParameter(param.Value)
+			if err != nil {
+				return nil, fmt.Errorf("failed to patch default value to pipeline. Error: %v", err)
+			}
 		}
 	}
-	}
 
 	// Store run metadata into database
 	runDetail, err := r.ToModelRunDetail(apiRun, runId, util.NewWorkflow(newWorkflow), string(manifestBytes), tmpl.GetTemplateType())
@@ -424,6 +424,20 @@ func (r *ResourceManager) ArchiveRun(runId string) error {
 }
 
 func (r *ResourceManager) UnarchiveRun(runId string) error {
+	experimentRef, err := r.resourceReferenceStore.GetResourceReference(runId, common.Run, common.Experiment)
+	if err != nil {
+		return util.Wrap(err, "Failed to retrieve resource reference")
+	}
+
+	experiment, err := r.GetExperiment(experimentRef.ReferenceUUID)
+	if err != nil {
+		return errors.Wrap(err, "Failed to retrieve experiment")
+	}
+
+	if experiment.StorageState == api.Experiment_STORAGESTATE_ARCHIVED.String() {
+		return util.NewFailedPreconditionError(errors.New("Unarchive the experiment first to allow the run to be restored"),
+			fmt.Sprintf("Unarchive experiment with name `%s` first to allow run `%s` to be restored", experimentRef.ReferenceName, runId))
+	}
 	return r.runStore.UnarchiveRun(runId)
 }
 

diff --git a/backend/src/apiserver/resource/resource_manager_test.go b/backend/src/apiserver/resource/resource_manager_test.go
@@ -668,7 +668,6 @@ func TestCreateRun_ThroughWorkflowSpecV2(t *testing.T) {
 	assert.Equal(t, expectedRunDetail, runDetail, "CreateRun stored invalid data in database")
 }
 
-
 func TestCreateRun_ThroughWorkflowSpec(t *testing.T) {
 	store, manager, runDetail := initWithOneTimeRun(t)
 	expectedExperimentUUID := runDetail.ExperimentUUID
@@ -1329,6 +1328,33 @@ func TestRetryRun_UpdateAndCreateFailed(t *testing.T) {
 	assert.Contains(t, err.Error(), "Failed to create or update the run")
 }
 
+func TestUnarchiveRun_OK(t *testing.T) {
+	store, manager, runDetail := initWithOneTimeRun(t)
+	defer store.Close()
+	err := manager.UnarchiveRun(runDetail.UUID)
+	assert.Nil(t, err)
+}
+
+func TestUnarchiveRun_Failed_ExperimentArchived(t *testing.T) {
+	store, manager, runDetail := initWithOneTimeRun(t)
+	defer store.Close()
+	err := manager.ArchiveExperiment(context.Background(), runDetail.ExperimentUUID)
+	assert.Nil(t, err)
+	err = manager.UnarchiveRun(runDetail.UUID)
+	assert.NotNil(t, err)
+	assert.Equal(t, codes.FailedPrecondition, err.(*util.UserError).ExternalStatusCode())
+	assert.Contains(t, err.Error(), "Unarchive the experiment first to allow")
+}
+
+func TestUnarchiveRun_Failed_ResourceNotFound(t *testing.T) {
+	store, manager, _ := initWithExperiment(t)
+	defer store.Close()
+	err := manager.UnarchiveRun(FakeUUIDOne)
+	assert.NotNil(t, err)
+	assert.Equal(t, codes.NotFound, err.(*util.UserError).ExternalStatusCode())
+	assert.Contains(t, err.Error(), "Failed to retrieve resource reference")
+}
+
 // TODO Use table driven to write UT to test CreateJob
 func TestCreateJob_ThroughWorkflowSpec(t *testing.T) {
 	store, _, job := initWithJob(t)

diff --git a/backend/src/common/util/error.go b/backend/src/common/util/error.go
@@ -194,6 +194,14 @@ func NewBadRequestError(err error, externalFormat string, a ...interface{}) *Use
 		codes.Aborted)
 }
 
+func NewFailedPreconditionError(err error, externalFormat string, a ...interface{}) *UserError {
+	externalMessage := fmt.Sprintf(externalFormat, a...)
+	return newUserError(
+		errors.Wrapf(err, fmt.Sprintf("FailedPreconditionError: %v", externalMessage)),
+		externalMessage,
+		codes.FailedPrecondition)
+}
+
 func NewUnauthenticatedError(err error, externalFormat string, a ...interface{}) *UserError {
 	externalMessage := fmt.Sprintf(externalFormat, a...)
 	return newUserError(