Skip to content

Commit

Permalink
[Backend][Multi-user] Adjust/implement run api for multiuser support (#…
Browse files Browse the repository at this point in the history 
…3337)

* Adjust/implement run api for multiuser support

* Fix error message

* use consistent run name in test

* add unit test

* ListRuns must specify filter either by namespace or by experiment

* fix comments
  • Loading branch information
@chensun
chensun authored Mar 27, 2020
1 parent 081ee74 commit 3d8077c
Show file tree
Hide file tree
Showing 12 changed files with 589 additions and 83 deletions.
11 changes: 11 additions & 0 deletions backend/src/apiserver/common/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,14 @@ func GetNamespaceFromAPIResourceReferences(resourceRefs []*api.ResourceReference
}
return namespace
}

func GetExperimentIDFromAPIResourceReferences(resourceRefs []*api.ResourceReference) string {
experimentID := ""
for _, resourceRef := range resourceRefs {
if resourceRef.Key.Type == api.ResourceType_EXPERIMENT {
experimentID = resourceRef.Key.Id
break
}
}
return experimentID
}
85 changes: 70 additions & 15 deletions backend/src/apiserver/common/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,28 +22,83 @@ import (
)

func TestGetNamespaceFromResourceReferences(t *testing.T) {
references := []*api.ResourceReference{
tests := []struct {
name string
references []*api.ResourceReference
expectedNamespace string
}{
{
Key: &api.ResourceKey{
Type: api.ResourceType_EXPERIMENT, Id: "123"},
Relationship: api.Relationship_CREATOR,
"resource reference with namespace and experiment",
[]*api.ResourceReference{
{
Key: &api.ResourceKey{
Type: api.ResourceType_EXPERIMENT, Id: "123"},
Relationship: api.Relationship_CREATOR,
},
{
Key: &api.ResourceKey{
Type: api.ResourceType_NAMESPACE, Id: "ns"},
Relationship: api.Relationship_OWNER,
},
},
"ns",
},
{
Key: &api.ResourceKey{
Type: api.ResourceType_NAMESPACE, Id: "ns"},
Relationship: api.Relationship_OWNER,
"resource reference with experiment only",
[]*api.ResourceReference{
{
Key: &api.ResourceKey{
Type: api.ResourceType_EXPERIMENT, Id: "123"},
Relationship: api.Relationship_CREATOR,
},
},
"",
},
}
namespace := GetNamespaceFromAPIResourceReferences(references)
assert.Equal(t, "ns", namespace)
for _, tc := range tests {
namespace := GetNamespaceFromAPIResourceReferences(tc.references)
assert.Equal(t, tc.expectedNamespace, namespace,
"TestGetNamespaceFromResourceReferences(%v) has unexpected result.", tc.name)
}
}

references = []*api.ResourceReference{
func TestGetExperimentIDFromResourceReferences(t *testing.T) {
tests := []struct {
name string
references []*api.ResourceReference
expectedExperimentID string
}{
{
Key: &api.ResourceKey{
Type: api.ResourceType_EXPERIMENT, Id: "123"},
Relationship: api.Relationship_CREATOR,
"resource reference with namespace and experiment",
[]*api.ResourceReference{
{
Key: &api.ResourceKey{
Type: api.ResourceType_EXPERIMENT, Id: "123"},
Relationship: api.Relationship_CREATOR,
},
{
Key: &api.ResourceKey{
Type: api.ResourceType_NAMESPACE, Id: "ns"},
Relationship: api.Relationship_OWNER,
},
},
"123",
},
{
"resource reference with namespace only",
[]*api.ResourceReference{
{
Key: &api.ResourceKey{
Type: api.ResourceType_NAMESPACE, Id: "ns"},
Relationship: api.Relationship_OWNER,
},
},
"",
},
}
for _, tc := range tests {
experimentID := GetExperimentIDFromAPIResourceReferences(tc.references)
assert.Equal(t, tc.expectedExperimentID, experimentID,
"TestGetExperimentIDFromResourceReferences(%v) has unexpected result.", tc.name)
}
namespace = GetNamespaceFromAPIResourceReferences(references)
assert.Equal(t, "", namespace)
}
25 changes: 22 additions & 3 deletions backend/src/apiserver/list/list.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,20 +250,39 @@ func FilterOnResourceReference(tableName string, columns []string, resourceType

// FilterOnExperiment filters the given table by rows based on provided experiment ID,
// and returns the rebuilt SelectBuilder
func FilterRunOnExperiment(
func FilterOnExperiment(
tableName string,
columns []string,
selectCount bool,
experimentID string,
) (sq.SelectBuilder, error) {
return filterByColumnValue(tableName, columns, selectCount, "ExperimentUUID", experimentID), nil
}

func FilterOnNamespace(
tableName string,
columns []string,
selectCount bool,
namespace string,
) (sq.SelectBuilder, error) {
return filterByColumnValue(tableName, columns, selectCount, "Namespace", namespace), nil
}

func filterByColumnValue(
tableName string,
columns []string,
selectCount bool,
columnName string,
filterValue interface{},
) sq.SelectBuilder {
selectBuilder := sq.Select(columns...)
if selectCount {
selectBuilder = sq.Select("count(*)")
}
selectBuilder = selectBuilder.From(tableName).Where(
sq.Eq{"ExperimentUUID": experimentID},
sq.Eq{columnName: filterValue},
)
return selectBuilder, nil
return selectBuilder
}

// Scans the one given row into a number, and returns the number
Expand Down
88 changes: 88 additions & 0 deletions backend/src/apiserver/list/list_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -736,3 +736,91 @@ func TestFilterOnResourceReference(t *testing.T) {
}
}
}

func TestFilterOnExperiment(t *testing.T) {

type testIn struct {
table string
count bool
filter *common.FilterContext
}
tests := []struct {
in *testIn
wantSql string
wantErr error
}{
{
in: &testIn{
table: "testTable",
count: false,
filter: &common.FilterContext{},
},
wantSql: "SELECT * FROM testTable WHERE ExperimentUUID = ?",
wantErr: nil,
},
{
in: &testIn{
table: "testTable",
count: true,
filter: &common.FilterContext{},
},
wantSql: "SELECT count(*) FROM testTable WHERE ExperimentUUID = ?",
wantErr: nil,
},
}

for _, test := range tests {
sqlBuilder, gotErr := FilterOnExperiment(test.in.table, []string{"*"}, test.in.count, "123")
gotSql, _, err := sqlBuilder.ToSql()
assert.Nil(t, err)

if gotSql != test.wantSql || gotErr != test.wantErr {
t.Errorf("FilterOnExperiment(%+v) =\nGot: %q, %v\nWant: %q, %v",
test.in, gotSql, gotErr, test.wantSql, test.wantErr)
}
}
}

func TestFilterOnNamesapce(t *testing.T) {

type testIn struct {
table string
count bool
filter *common.FilterContext
}
tests := []struct {
in *testIn
wantSql string
wantErr error
}{
{
in: &testIn{
table: "testTable",
count: false,
filter: &common.FilterContext{},
},
wantSql: "SELECT * FROM testTable WHERE Namespace = ?",
wantErr: nil,
},
{
in: &testIn{
table: "testTable",
count: true,
filter: &common.FilterContext{},
},
wantSql: "SELECT count(*) FROM testTable WHERE Namespace = ?",
wantErr: nil,
},
}

for _, test := range tests {
sqlBuilder, gotErr := FilterOnNamespace(test.in.table, []string{"*"}, test.in.count, "ns")
gotSql, _, err := sqlBuilder.ToSql()
assert.Nil(t, err)

if gotSql != test.wantSql || gotErr != test.wantErr {
t.Errorf("FilterOnNamespace(%+v) =\nGot: %q, %v\nWant: %q, %v",
test.in, gotSql, gotErr, test.wantSql, test.wantErr)
}
}
}
41 changes: 27 additions & 14 deletions backend/src/apiserver/resource/resource_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,10 +314,30 @@ func (r *ResourceManager) CreateRun(apiRun *api.Run) (*model.RunDetail, error) {
}
}

namespace := common.GetNamespaceFromAPIResourceReferences(apiRun.ResourceReferences)
resourceReferences := apiRun.GetResourceReferences()
experimentID := common.GetExperimentIDFromAPIResourceReferences(resourceReferences)
if len(experimentID) == 0 {
if multiuserMode {
return nil, util.NewInternalServerError(errors.New("Missing experiment"), "Experiment is required for CreateRun/CreateJob.")
} else {
// Add a reference to the default experiment
ref, err := r.getDefaultExperimentResourceReference(resourceReferences)
if err != nil {
return nil, util.Wrap(err, "Failed to create run.")
}
apiRun.ResourceReferences = append(apiRun.ResourceReferences, ref)
experimentID = ref.GetKey().GetId()
}
}
experiment, err := r.GetExperiment(experimentID)
if err != nil {
return nil, util.NewInternalServerError(err, "Failed to get experiment.")
}

namespace := experiment.Namespace
if len(namespace) == 0 {
if multiuserMode {
return nil, util.NewInvalidInputError("Run should specify namespace")
return nil, util.NewInternalServerError(errors.New("Missing namespace"), "Experiment %v doesn't have a namespace.", experiment.Name)
} else {
namespace = common.GetPodNamespace()
}
Expand All @@ -328,15 +348,6 @@ func (r *ResourceManager) CreateRun(apiRun *api.Run) (*model.RunDetail, error) {
return nil, util.NewInternalServerError(err, "Failed to create a workflow for (%s)", workflow.Name)
}

// Add a reference to the default experiment if run does not already have a containing experiment
ref, err := r.getDefaultExperimentIfNoExperiment(apiRun.GetResourceReferences())
if err != nil {
return nil, err
}
if ref != nil {
apiRun.ResourceReferences = append(apiRun.ResourceReferences, ref)
}

// Store run metadata into database
runDetail, err := r.ToModelRunDetail(apiRun, runId, util.NewWorkflow(newWorkflow), string(workflowSpecManifestBytes))
if err != nil {
Expand Down Expand Up @@ -778,7 +789,6 @@ func (r *ResourceManager) getWorkflowSpecBytes(spec *api.PipelineSpec) ([]byte,
if err != nil {
return nil, util.Wrap(err, "Get pipeline YAML failed.")
}

return []byte(workflow.ToStringForStore()), nil
} else if spec.GetWorkflowManifest() != "" {
return []byte(spec.GetWorkflowManifest()), nil
Expand Down Expand Up @@ -847,7 +857,10 @@ func (r *ResourceManager) getDefaultExperimentIfNoExperiment(references []*api.R
return nil, nil
}
}
return r.getDefaultExperimentResourceReference(references)
}

func (r *ResourceManager) getDefaultExperimentResourceReference(references []*api.ResourceReference) (*api.ResourceReference, error) {
// Create reference to the default experiment
defaultExperimentId, err := r.GetDefaultExperimentId()
if err != nil {
Expand Down Expand Up @@ -1032,11 +1045,11 @@ func (r *ResourceManager) GetNamespaceFromRunID(runId string) (string, error) {
if err != nil {
return "", util.Wrap(err, "Failed to get namespace from run id.")
}
namespace := model.GetNamespaceFromModelResourceReferences(runDetail.ResourceReferences)
namespace := runDetail.Namespace
if len(namespace) == 0 {
if common.IsMultiUserMode() {
// All runs should have namespace in multi user mode.
return "", errors.New("Invalid db data: run doesn't have a namespace resource reference")
return "", errors.New("Invalid db data: run_details doesn't have a namespace")
} else {
// When db model doesn't have namespace stored (e.g. legacy runs), use
// pod namespace as default.
Expand Down
Loading

0 comments on commit 3d8077c

Please sign in to comment.