From d88ba380bc8a6e75c4d759d015bb47d84b7202d2 Mon Sep 17 00:00:00 2001 From: Riley Bauer <34456002+rileyjbauer@users.noreply.github.com> Date: Mon, 29 Apr 2019 12:13:35 -0700 Subject: [PATCH] Clear default exp table on delete and create default exp on run create if none exists (#1199) * Clear default exp table on delete and create default exp on run create if no default exists With this change, if the delete experiment API is called on the default experiment, then the ID will also be removed from the default_experiments table. Additionally, if the default experiment doesn't exist and a new run is created without an experiment, a new default experiment will be created, and the run will be placed within this experiment. * Adds integration test for creating a run without an experiment * Fixes failure to close database connection and adds tests for recreating and deleting default experiment * Rename function * Revert some row.Close() calls --- backend/src/apiserver/main.go | 36 +------ .../apiserver/resource/resource_manager.go | 41 +++++++- .../resource/resource_manager_test.go | 93 +++++++++++++++++++ .../src/apiserver/storage/db_status_store.go | 2 + .../storage/default_experiment_store.go | 27 ++++++ .../storage/default_experiment_store_test.go | 35 ++++++- .../src/apiserver/storage/experiment_store.go | 16 +++- backend/src/apiserver/storage/job_store.go | 6 +- backend/src/apiserver/storage/run_store.go | 6 +- .../helloworld.spec.js | 55 ++++++++--- 10 files changed, 253 insertions(+), 64 deletions(-) diff --git a/backend/src/apiserver/main.go b/backend/src/apiserver/main.go index ca5b1a9391e..8d87bb47fc3 100644 --- a/backend/src/apiserver/main.go +++ b/backend/src/apiserver/main.go @@ -30,7 +30,6 @@ import ( "github.com/golang/glog" "github.com/grpc-ecosystem/grpc-gateway/runtime" api "github.com/kubeflow/pipelines/backend/api/go_client" - "github.com/kubeflow/pipelines/backend/src/apiserver/model" "github.com/kubeflow/pipelines/backend/src/apiserver/resource" "github.com/kubeflow/pipelines/backend/src/apiserver/server" "google.golang.org/grpc" @@ -61,7 +60,7 @@ func main() { glog.Fatalf("Failed to load samples. Err: %v", err) } - err = createDefaultExperiment(resourceManager) + _, err = resourceManager.CreateDefaultExperiment() if err != nil { glog.Fatalf("Failed to create default experiment. Err: %v", err) } @@ -135,39 +134,6 @@ func registerHttpHandlerFromEndpoint(handler RegisterHttpHandlerFromEndpoint, se } } -// Used to initialize the Experiment database with a default to be used for runs -func createDefaultExperiment(resourceManager *resource.ResourceManager) error { - // First check that we don't already have a default experiment ID in the DB. - defaultExperimentId, err := resourceManager.GetDefaultExperimentId() - if err != nil { - return fmt.Errorf("Failed to check if default experiment exists. Err: %v", err) - } - // If default experiment ID is already present, don't fail, simply return. - if defaultExperimentId != "" { - glog.Info("Default experiment already exists! ID: %v", defaultExperimentId) - return nil - } - - // Create default experiment - defaultExperiment := &model.Experiment{ - Name: "Default", - Description: "All runs created without specifying an experiment will be grouped here.", - } - experiment, err := resourceManager.CreateExperiment(defaultExperiment) - if err != nil { - return fmt.Errorf("Failed to create default experiment. Err: %v", err) - } - - // Set default experiment ID in the DB - err = resourceManager.SetDefaultExperimentId(experiment.UUID) - if err != nil { - return fmt.Errorf("Failed to set default experiment ID. Err: %v", err) - } - - glog.Info("Default experiment is set. ID is: %v", experiment.UUID) - return nil -} - // Preload a bunch of pipeline samples // Samples are only loaded once when the pipeline system is initially installed. // They won't be loaded when upgrade or pod restart, to prevent them reappear if user explicitly diff --git a/backend/src/apiserver/resource/resource_manager.go b/backend/src/apiserver/resource/resource_manager.go index ec9e4438da0..0521182402f 100644 --- a/backend/src/apiserver/resource/resource_manager.go +++ b/backend/src/apiserver/resource/resource_manager.go @@ -506,7 +506,40 @@ func (r *ResourceManager) getWorkflowSpecBytes(spec *api.PipelineSpec) ([]byte, return nil, util.NewInvalidInputError("Please provide a valid pipeline spec") } -// setDefaultExperimentIfNotPresent If the provided run does not include a reference to a containing +// Used to initialize the Experiment database with a default to be used for runs +func (r *ResourceManager) CreateDefaultExperiment() (string, error) { + // First check that we don't already have a default experiment ID in the DB. + defaultExperimentId, err := r.GetDefaultExperimentId() + if err != nil { + return "", fmt.Errorf("Failed to check if default experiment exists. Err: %v", err) + } + // If default experiment ID is already present, don't fail, simply return. + if defaultExperimentId != "" { + glog.Info("Default experiment already exists! ID: %v", defaultExperimentId) + return "", nil + } + + // Create default experiment + defaultExperiment := &model.Experiment{ + Name: "Default", + Description: "All runs created without specifying an experiment will be grouped here.", + } + experiment, err := r.CreateExperiment(defaultExperiment) + if err != nil { + return "", fmt.Errorf("Failed to create default experiment. Err: %v", err) + } + + // Set default experiment ID in the DB + err = r.SetDefaultExperimentId(experiment.UUID) + if err != nil { + return "", fmt.Errorf("Failed to set default experiment ID. Err: %v", err) + } + + glog.Info("Default experiment is set. ID is: %v", experiment.UUID) + return experiment.UUID, nil +} + +// setExperimentIfNotPresent If the provided run does not include a reference to a containing // experiment, then we fetch the default experiment's ID and create a reference to that. func (r *ResourceManager) setExperimentIfNotPresent(apiRun *api.Run) error { // First check if there is already a referenced experiment @@ -522,7 +555,11 @@ func (r *ResourceManager) setExperimentIfNotPresent(apiRun *api.Run) error { return util.NewInternalServerError(err, "Failed to retrieve default experiment") } if defaultExperimentId == "" { - return util.NewInternalServerError(errors.New("Default experiment ID was empty"), "Default experiment was not set") + glog.Info("No default experiment was found. Creating a new default experiment") + defaultExperimentId, err = r.CreateDefaultExperiment() + if defaultExperimentId == "" || err != nil { + return util.NewInternalServerError(err, "Failed to create new default experiment") + } } defaultExperimentRef := &api.ResourceReference{ Key: &api.ResourceKey{ diff --git a/backend/src/apiserver/resource/resource_manager_test.go b/backend/src/apiserver/resource/resource_manager_test.go index f8d346a2971..f7c5be0511c 100644 --- a/backend/src/apiserver/resource/resource_manager_test.go +++ b/backend/src/apiserver/resource/resource_manager_test.go @@ -315,6 +315,36 @@ func TestCreateRun_ThroughWorkflowSpec(t *testing.T) { assert.Equal(t, expectedRunDetail, runDetail, "CreateRun stored invalid data in database") } +func TestCreateRun_NoExperiment(t *testing.T) { + store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) + manager := NewResourceManager(store) + apiRun := &api.Run{ + Name: "No experiment", + PipelineSpec: &api.PipelineSpec{ + WorkflowManifest: testWorkflow.ToStringForStore(), + Parameters: []*api.Parameter{ + {Name: "param1", Value: "world"}, + }, + }, + // No experiment + ResourceReferences: []*api.ResourceReference{}, + } + runDetail, err := manager.CreateRun(apiRun) + assert.Nil(t, err) + expectedRunDetail := []*model.ResourceReference{{ + ResourceUUID: "workflow1", + ResourceType: common.Run, + // Experiment is now set + ReferenceUUID: DefaultFakeUUID, + ReferenceType: common.Experiment, + Relationship: common.Owner, + }} + assert.Equal(t, expectedRunDetail, runDetail.Run.ResourceReferences, "The CreateRun return has unexpected value.") + runDetail, err = manager.GetRun(runDetail.UUID) + assert.Nil(t, err) + assert.Equal(t, expectedRunDetail, runDetail.Run.ResourceReferences, "CreateRun stored invalid data in database") +} + func TestCreateRun_EmptyPipelineSpec(t *testing.T) { store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) defer store.Close() @@ -448,6 +478,69 @@ func TestDeleteRun_DbFailure(t *testing.T) { assert.Contains(t, err.Error(), "database is closed") } +func TestDeleteExperiment(t *testing.T) { + store, manager, experiment := initWithExperiment(t) + defer store.Close() + err := manager.DeleteExperiment(experiment.UUID) + assert.Nil(t, err) + + _, err = manager.GetExperiment(experiment.UUID) + assert.Equal(t, codes.NotFound, err.(*util.UserError).ExternalStatusCode()) + assert.Contains(t, err.Error(), "not found") +} + +func TestDeleteExperiment_ClearsDefaultExperiment(t *testing.T) { + store, manager, experiment := initWithExperiment(t) + defer store.Close() + // Set default experiment ID. This is not normally done manually + err := manager.SetDefaultExperimentId(experiment.UUID) + assert.Nil(t, err) + // Verify that default experiment ID is set + defaultExperimentId, err := manager.GetDefaultExperimentId() + assert.Nil(t, err) + assert.Equal(t, experiment.UUID, defaultExperimentId) + + err = manager.DeleteExperiment(experiment.UUID) + assert.Nil(t, err) + + _, err = manager.GetExperiment(experiment.UUID) + assert.Equal(t, codes.NotFound, err.(*util.UserError).ExternalStatusCode()) + assert.Contains(t, err.Error(), "not found") + + // Verify that default experiment ID has been cleared + defaultExperimentId, err = manager.GetDefaultExperimentId() + assert.Nil(t, err) + assert.Equal(t, "", defaultExperimentId) +} + +func TestDeleteExperiment_ExperimentNotExist(t *testing.T) { + store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) + defer store.Close() + manager := NewResourceManager(store) + err := manager.DeleteExperiment("1") + assert.Equal(t, codes.NotFound, err.(*util.UserError).ExternalStatusCode()) + assert.Contains(t, err.Error(), "not found") +} + +func TestDeleteExperiment_CrdFailure(t *testing.T) { + store, manager, experiment := initWithExperiment(t) + defer store.Close() + + manager.workflowClient = &FakeBadWorkflowClient{} + err := manager.DeleteExperiment(experiment.UUID) + assert.Nil(t, err) +} + +func TestDeleteExperiment_DbFailure(t *testing.T) { + store, manager, experiment := initWithExperiment(t) + defer store.Close() + + store.DB().Close() + err := manager.DeleteExperiment(experiment.UUID) + assert.Equal(t, codes.Internal, err.(*util.UserError).ExternalStatusCode()) + assert.Contains(t, err.Error(), "database is closed") +} + func TestTerminateRun(t *testing.T) { store, manager, runDetail := initWithOneTimeRun(t) defer store.Close() diff --git a/backend/src/apiserver/storage/db_status_store.go b/backend/src/apiserver/storage/db_status_store.go index 77bd7657231..07798a3c304 100644 --- a/backend/src/apiserver/storage/db_status_store.go +++ b/backend/src/apiserver/storage/db_status_store.go @@ -46,6 +46,7 @@ func (s *DBStatusStore) InitializeDBStatusTable() error { tx.Rollback() return util.NewInternalServerError(err, "Failed to load database status.") } + defer rows.Close() // The table is not initialized if !rows.Next() { @@ -83,6 +84,7 @@ func (s *DBStatusStore) HaveSamplesLoaded() (bool, error) { if err != nil { return false, util.NewInternalServerError(err, "Error when getting load sample status") } + defer rows.Close() if rows.Next() { err = rows.Scan(&haveSamplesLoaded) if err != nil { diff --git a/backend/src/apiserver/storage/default_experiment_store.go b/backend/src/apiserver/storage/default_experiment_store.go index 24683bc0301..6eb5eb5087b 100644 --- a/backend/src/apiserver/storage/default_experiment_store.go +++ b/backend/src/apiserver/storage/default_experiment_store.go @@ -15,6 +15,8 @@ package storage import ( + "database/sql" + sq "github.com/Masterminds/squirrel" "github.com/golang/glog" "github.com/kubeflow/pipelines/backend/src/common/util" @@ -46,6 +48,7 @@ func (s *DefaultExperimentStore) initializeDefaultExperimentTable() error { tx.Rollback() return util.NewInternalServerError(err, "Failed to get default experiment.") } + defer rows.Close() // If the table is not initialized, then set the default value. if !rows.Next() { @@ -77,6 +80,7 @@ func (s *DefaultExperimentStore) SetDefaultExperimentId(id string) error { sql, args, err := sq. Update("default_experiments"). SetMap(sq.Eq{"DefaultExperimentId": id}). + Where(sq.Eq{"DefaultExperimentId": ""}). ToSql() if err != nil { return util.NewInternalServerError(err, "Error creating query to set default experiment ID.") @@ -94,10 +98,13 @@ func (s *DefaultExperimentStore) GetDefaultExperimentId() (string, error) { if err != nil { return "", util.NewInternalServerError(err, "Error creating query to get default experiment ID.") } + rows, err := s.db.Query(sql, args...) if err != nil { return "", util.NewInternalServerError(err, "Error when getting default experiment ID") } + defer rows.Close() + if rows.Next() { err = rows.Scan(&defaultExperimentId) if err != nil { @@ -108,6 +115,26 @@ func (s *DefaultExperimentStore) GetDefaultExperimentId() (string, error) { return "", nil } +// Sets the default experiment ID stored in the DB to the empty string. This needs to happen if the +// experiment is deleted via the normal delete experiment API so that the server knows to create a +// new default. +// This is always done alongside the deletion of the actual experiment itself, so a transaction is +// needed as input. +// Update is used instead of delete so that we don't need to first check that the experiment ID is +// there. +func (s *DefaultExperimentStore) UnsetDefaultExperimentIdIfIdMatches(tx *sql.Tx, id string) error { + sql, args, err := sq. + Update("default_experiments"). + SetMap(sq.Eq{"DefaultExperimentId": ""}). + Where(sq.Eq{"DefaultExperimentId": id}). + ToSql() + _, err = tx.Exec(sql, args...) + if err != nil { + return util.NewInternalServerError(err, "Failed to clear default experiment with ID: %s", id) + } + return nil +} + // factory function for creating default experiment store func NewDefaultExperimentStore(db *DB) *DefaultExperimentStore { s := &DefaultExperimentStore{db: db} diff --git a/backend/src/apiserver/storage/default_experiment_store_test.go b/backend/src/apiserver/storage/default_experiment_store_test.go index cbb35ab857a..415df22ffb6 100644 --- a/backend/src/apiserver/storage/default_experiment_store_test.go +++ b/backend/src/apiserver/storage/default_experiment_store_test.go @@ -33,7 +33,7 @@ func TestInitializeDefaultExperimentTable(t *testing.T) { // Default experiment ID is empty after table initialization defaultExperimentId, err := defaultExperimentStore.GetDefaultExperimentId() assert.Nil(t, err) - assert.Equal(t, defaultExperimentId, "") + assert.Equal(t, "", defaultExperimentId) // Initializing the table with an invalid DB is an error db.Close() @@ -54,10 +54,13 @@ func TestGetAndSetDefaultExperimentId(t *testing.T) { // Get the default experiment ID defaultExperimentId, err := defaultExperimentStore.GetDefaultExperimentId() assert.Nil(t, err) - assert.Equal(t, defaultExperimentId, "test-ID") - // Trying to set the default experiment ID again is an error + assert.Equal(t, "test-ID", defaultExperimentId) + // Trying to set the default experiment ID again is not an error, but the ID is not changed err = defaultExperimentStore.SetDefaultExperimentId("a-different-ID") - assert.NotNil(t, err) + assert.Nil(t, err) + defaultExperimentId, err = defaultExperimentStore.GetDefaultExperimentId() + assert.Nil(t, err) + assert.Equal(t, "test-ID", defaultExperimentId) // Setting or getting the default experiment ID with an invalid DB is an error db.Close() @@ -66,3 +69,27 @@ func TestGetAndSetDefaultExperimentId(t *testing.T) { _, err = defaultExperimentStore.GetDefaultExperimentId() assert.NotNil(t, err) } + +func TestUnsetDefaultExperimentIdIfIdMatches(t *testing.T) { + db := NewFakeDbOrFatal() + defaultExperimentStore := NewDefaultExperimentStore(db) + + // Initialize for the first time + err := defaultExperimentStore.initializeDefaultExperimentTable() + assert.Nil(t, err) + // Set the default experiment ID + err = defaultExperimentStore.SetDefaultExperimentId("test-ID") + assert.Nil(t, err) + // Clear the default experiment ID. This requires a transaction. + tx, _ := db.Begin() + err = defaultExperimentStore.UnsetDefaultExperimentIdIfIdMatches(tx, "test-ID") + assert.Nil(t, err) + err = tx.Commit() + assert.Nil(t, err) + // Get the default experiment ID + defaultExperimentId, err := defaultExperimentStore.GetDefaultExperimentId() + assert.Nil(t, err) + assert.Equal(t, "", defaultExperimentId) + + db.Close() +} diff --git a/backend/src/apiserver/storage/experiment_store.go b/backend/src/apiserver/storage/experiment_store.go index 527266ba8ae..ad906f19597 100644 --- a/backend/src/apiserver/storage/experiment_store.go +++ b/backend/src/apiserver/storage/experiment_store.go @@ -25,6 +25,7 @@ type ExperimentStore struct { time util.TimeInterface uuid util.UUIDGeneratorInterface resourceReferenceStore *ResourceReferenceStore + defaultExperimentStore *DefaultExperimentStore } // Runs two SQL queries in a transaction to return a list of matching experiments, as well as their @@ -178,7 +179,7 @@ func (s *ExperimentStore) DeleteExperiment(id string) error { return util.NewInternalServerError(err, "Failed to create query to delete experiment: %s", id) } - // Use a transaction to make sure both experiment and its resource references are stored. + // Use a transaction to make sure both experiment and its resource references are deleted. tx, err := s.db.Begin() if err != nil { return util.NewInternalServerError(err, "Failed to create a new transaction to delete experiment.") @@ -188,6 +189,11 @@ func (s *ExperimentStore) DeleteExperiment(id string) error { tx.Rollback() return util.NewInternalServerError(err, "Failed to delete experiment %s from table", id) } + err = s.defaultExperimentStore.UnsetDefaultExperimentIdIfIdMatches(tx, id) + if err != nil { + tx.Rollback() + return util.NewInternalServerError(err, "Failed to clear default experiment ID for experiment %v ", id) + } err = s.resourceReferenceStore.DeleteResourceReferences(tx, id, common.Run) if err != nil { tx.Rollback() @@ -202,5 +208,11 @@ func (s *ExperimentStore) DeleteExperiment(id string) error { // factory function for experiment store func NewExperimentStore(db *DB, time util.TimeInterface, uuid util.UUIDGeneratorInterface) *ExperimentStore { - return &ExperimentStore{db: db, time: time, uuid: uuid, resourceReferenceStore: NewResourceReferenceStore(db)} + return &ExperimentStore{ + db: db, + time: time, + uuid: uuid, + resourceReferenceStore: NewResourceReferenceStore(db), + defaultExperimentStore: NewDefaultExperimentStore(db), + } } diff --git a/backend/src/apiserver/storage/job_store.go b/backend/src/apiserver/storage/job_store.go index 97d7be45c3d..f58890c27ed 100644 --- a/backend/src/apiserver/storage/job_store.go +++ b/backend/src/apiserver/storage/job_store.go @@ -204,7 +204,7 @@ func (s *JobStore) scanRows(r *sql.Rows) ([]*model.Job, error) { CronSchedule: model.CronSchedule{ CronScheduleStartTimeInSec: NullInt64ToPointer(cronScheduleStartTimeInSec), CronScheduleEndTimeInSec: NullInt64ToPointer(cronScheduleEndTimeInSec), - Cron: NullStringToPointer(cron), + Cron: NullStringToPointer(cron), }, PeriodicSchedule: model.PeriodicSchedule{ PeriodicScheduleStartTimeInSec: NullInt64ToPointer(periodicScheduleStartTimeInSec), @@ -384,8 +384,8 @@ func (s *JobStore) UpdateJob(swf *util.ScheduledWorkflow) error { // factory function for job store func NewJobStore(db *DB, time util.TimeInterface) *JobStore { return &JobStore{ - db: db, + db: db, resourceReferenceStore: NewResourceReferenceStore(db), - time: time, + time: time, } } diff --git a/backend/src/apiserver/storage/run_store.go b/backend/src/apiserver/storage/run_store.go index 076c6ac76ea..20ad9f74bd8 100644 --- a/backend/src/apiserver/storage/run_store.go +++ b/backend/src/apiserver/storage/run_store.go @@ -562,10 +562,10 @@ func (s *RunStore) toRunMetadatas(models []model.ListableDataModel) []model.Run // used to record artifact metadata. func NewRunStore(db *DB, time util.TimeInterface, metadataStore *metadata.Store) *RunStore { return &RunStore{ - db: db, + db: db, resourceReferenceStore: NewResourceReferenceStore(db), - time: time, - metadataStore: metadataStore, + time: time, + metadataStore: metadataStore, } } diff --git a/test/frontend-integration-test/helloworld.spec.js b/test/frontend-integration-test/helloworld.spec.js index 4e57d57a1f8..128e72b96b7 100644 --- a/test/frontend-integration-test/helloworld.spec.js +++ b/test/frontend-integration-test/helloworld.spec.js @@ -17,11 +17,11 @@ const URL = require('url').URL; const experimentName = 'helloworld-experiment-' + Date.now(); const experimentDescription = 'hello world experiment description'; -const secondExperimentName = 'different-experiment-name-' + Date.now(); -const secondExperimentNameDescription = 'second experiment description'; const pipelineName = 'helloworld-pipeline-' + Date.now(); const runName = 'helloworld-' + Date.now(); const runDescription = 'test run description ' + runName; +const runWithoutExperimentName = 'helloworld-2-' + Date.now(); +const runWithoutExperimentDescription = 'test run without experiment description ' + runWithoutExperimentName; const waitTimeout = 5000; const outputParameterValue = 'Hello world in test' @@ -192,16 +192,46 @@ describe('deploy helloworld sample run', () => { }, waitTimeout); }); - it('creates a new experiment', () => { - $('#newExperimentBtn').click(); - browser.waitUntil(() => { - return new URL(browser.getUrl()).hash.startsWith('#/experiments/new'); - }, waitTimeout); + it('creates a new run without selecting an experiment', () => { + $('#createNewRunBtn').waitForVisible(); + $('#createNewRunBtn').click(); - $('#experimentName').setValue(secondExperimentName); - $('#experimentDescription').setValue(secondExperimentNameDescription); + $('#choosePipelineBtn').waitForVisible(); + $('#choosePipelineBtn').click(); - $('#createExperimentBtn').click(); + $('.tableRow').waitForVisible(); + $('.tableRow').click(); + + $('#usePipelineBtn').click(); + + $('#pipelineSelectorDialog').waitForVisible(waitTimeout, true); + + browser.keys('Tab'); + browser.keys(runWithoutExperimentName); + + browser.keys('Tab'); + browser.keys(runWithoutExperimentDescription); + + // Skip over "choose experiment" button + browser.keys('Tab'); + + browser.keys('Tab'); + browser.keys(outputParameterValue); + + // Deploy + $('#startNewRunBtn').click(); + }); + + it('redirects back to all runs page', () => { + browser.waitUntil(() => { + return (new URL(browser.getUrl())).hash === '#/runs'; + }, waitTimeout, `URL was: ${new URL(browser.getUrl())}`); + }); + + it('displays both runs in all runs page', () => { + $('.tableRow').waitForVisible(); + const rows = $$('.tableRow').length; + assert(rows === 2, 'there should now be two runs in the table, instead there are: ' + rows); }); it('navigates back to the experiment list', () => { @@ -229,11 +259,6 @@ describe('deploy helloworld sample run', () => { assert(rows === 1, 'there should now be one experiment in the table, instead there are: ' + rows); }); - // TODO: Add test for creating a run without an experiment. This will require changing the API - // initialization and integration tests to stop deleting the default experiment at the end of the - // suites. Otherwise, run creation here will fail with: - // 'Failed to store resource references to table for run [ID] : ResourceNotFoundError: [Default Experiment ID]' - //TODO: enable this after we change the pipeline to a unique name such that deleting this // pipeline will not jeopardize the concurrent basic e2e tests. // it('deletes the uploaded pipeline', () => {