Skip to content

Commit

Permalink
[Backend][Multi-user] Add namespace to Experiment API implementation (#…
Browse files Browse the repository at this point in the history 
…3243)

* Experiment API implementation

* Address review comments and add additional unit tests

* Update build file

* fix unit tests

* Reject empty namespace in multi user mode

* Address review comments
  • Loading branch information
@chensun
chensun authored Mar 17, 2020
1 parent 908be8a commit 809f97a
Show file tree
Hide file tree
Showing 13 changed files with 654 additions and 53 deletions.
1 change: 1 addition & 0 deletions backend/src/apiserver/resource/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ go_test(
"//backend/src/crd/pkg/apis/scheduledworkflow/v1beta1:go_default_library",
"@com_github_argoproj_argo//pkg/apis/workflow/v1alpha1:go_default_library",
"@com_github_ghodss_yaml//:go_default_library",
"@com_github_google_go_cmp//cmp:go_default_library",
"@com_github_pkg_errors//:go_default_library",
"@com_github_spf13_viper//:go_default_library",
"@com_github_stretchr_testify//assert:go_default_library",
Expand Down
19 changes: 19 additions & 0 deletions backend/src/apiserver/resource/model_converter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,27 @@ import (
"github.com/kubeflow/pipelines/backend/src/apiserver/common"
"github.com/kubeflow/pipelines/backend/src/apiserver/model"
"github.com/kubeflow/pipelines/backend/src/common/util"
"github.com/pkg/errors"
)

func (r *ResourceManager) ToModelExperiment(apiExperiment *api.Experiment) (*model.Experiment, error) {
namespace := ""
resourceReferences := apiExperiment.GetResourceReferences()
if resourceReferences != nil {
if len(resourceReferences) != 1 ||
resourceReferences[0].Key.Type != api.ResourceType_NAMESPACE ||
resourceReferences[0].Relationship != api.Relationship_OWNER {
return nil, util.NewInternalServerError(errors.New("Invalid resource references for experiment"), "Unable to convert to model experiment.")
}
namespace = resourceReferences[0].Key.Id
}
return &model.Experiment{
Name: apiExperiment.Name,
Description: apiExperiment.Description,
Namespace: namespace,
}, nil
}

func (r *ResourceManager) ToModelRunMetric(metric *api.RunMetric, runUUID string) *model.RunMetric {
return &model.RunMetric{
RunUUID: runUUID,
Expand Down
89 changes: 89 additions & 0 deletions backend/src/apiserver/resource/model_converter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
package resource

import (
"strings"
"testing"

"github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
"github.com/golang/protobuf/ptypes/timestamp"
"github.com/google/go-cmp/cmp"
api "github.com/kubeflow/pipelines/backend/api/go_client"
"github.com/kubeflow/pipelines/backend/src/apiserver/common"
"github.com/kubeflow/pipelines/backend/src/apiserver/model"
Expand All @@ -33,6 +35,93 @@ func initResourceManager() (*FakeClientManager, *ResourceManager) {
return store, NewResourceManager(store)
}

func TestToModelExperiment(t *testing.T) {
store, manager := initResourceManager()
defer store.Close()

tests := []struct {
name string
experiment *api.Experiment
wantError bool
errorMessage string
expectedModelExperiment *model.Experiment
}{
{
"No resource references",
&api.Experiment{
Name: "exp1",
Description: "This is an experiment",
},
false,
"",
&model.Experiment{
Name: "exp1",
Description: "This is an experiment",
Namespace: "",
},
},
{
"Valid resource references",
&api.Experiment{
Name: "exp1",
Description: "This is an experiment",
ResourceReferences: []*api.ResourceReference{
&api.ResourceReference{
Key: &api.ResourceKey{
Type: api.ResourceType_NAMESPACE,
Id: "ns1",
},
Relationship: api.Relationship_OWNER,
},
},
},
false,
"",
&model.Experiment{
Name: "exp1",
Description: "This is an experiment",
Namespace: "ns1",
},
},
{
"Invalid resource references",
&api.Experiment{
Name: "exp1",
Description: "This is an experiment",
ResourceReferences: []*api.ResourceReference{
&api.ResourceReference{
Key: &api.ResourceKey{
Type: api.ResourceType_EXPERIMENT,
Id: "invalid",
},
Relationship: api.Relationship_OWNER,
},
},
},
true,
"Invalid resource references for experiment",
nil,
},
}

for _, tc := range tests {
modelExperiment, err := manager.ToModelExperiment(tc.experiment)
if tc.wantError {
if err == nil {
t.Errorf("TestToModelExperiment(%v) expect error but got nil", tc.name)
} else if !strings.Contains(err.Error(), tc.errorMessage) {
t.Errorf("TestToModelExperiment(%v) expect error containing: %v, but got: %v", tc.name, tc.errorMessage, err)
}
} else {
if err != nil {
t.Errorf("TestToModelExperiment(%v) expect no error but got %v", tc.name, err)
} else if !cmp.Equal(tc.expectedModelExperiment, modelExperiment) {
t.Errorf("TestToModelExperiment(%v) expect (%+v) but got (%+v)", tc.name, tc.expectedModelExperiment, modelExperiment)
}
}
}
}

func TestToModelRunMetric(t *testing.T) {
store, manager := initResourceManager()
defer store.Close()
Expand Down
20 changes: 16 additions & 4 deletions backend/src/apiserver/resource/resource_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,17 +108,21 @@ func (r *ResourceManager) GetTime() util.TimeInterface {
return r.time
}

func (r *ResourceManager) CreateExperiment(experiment *model.Experiment) (*model.Experiment, error) {
func (r *ResourceManager) CreateExperiment(apiExperiment *api.Experiment) (*model.Experiment, error) {
experiment, err := r.ToModelExperiment(apiExperiment)
if err != nil {
return nil, util.Wrap(err, "Failed to convert experiment model")
}
return r.experimentStore.CreateExperiment(experiment)
}

func (r *ResourceManager) GetExperiment(experimentId string) (*model.Experiment, error) {
return r.experimentStore.GetExperiment(experimentId)
}

func (r *ResourceManager) ListExperiments(opts *list.Options) (
func (r *ResourceManager) ListExperiments(filterContext *common.FilterContext, opts *list.Options) (
experiments []*model.Experiment, total_size int, nextPageToken string, err error) {
return r.experimentStore.ListExperiments(opts)
return r.experimentStore.ListExperiments(filterContext, opts)
}

func (r *ResourceManager) DeleteExperiment(experimentID string) error {
Expand Down Expand Up @@ -817,7 +821,7 @@ func (r *ResourceManager) CreateDefaultExperiment() (string, error) {
}

// Create default experiment
defaultExperiment := &model.Experiment{
defaultExperiment := &api.Experiment{
Name: "Default",
Description: "All runs created without specifying an experiment will be grouped here.",
}
Expand Down Expand Up @@ -1017,6 +1021,14 @@ func (r *ResourceManager) IsRequestAuthorized(userIdentity string, namespace str
return r.kfamClient.IsAuthorized(userIdentity, namespace)
}

func (r *ResourceManager) GetNamespaceFromExperimentID(experimentID string) (string, error) {
experiment, err := r.GetExperiment(experimentID)
if err != nil {
return "", util.Wrap(err, "Failed to get namespace from experiment ID.")
}
return experiment.Namespace, nil
}

func (r *ResourceManager) GetNamespaceFromRunID(runId string) (string, error) {
runDetail, err := r.GetRun(runId)
if err != nil {
Expand Down
59 changes: 51 additions & 8 deletions backend/src/apiserver/resource/resource_manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ func initWithExperiment(t *testing.T) (*FakeClientManager, *ResourceManager, *mo
initEnvVars()
store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
manager := NewResourceManager(store)
experiment := &model.Experiment{Name: "e1"}
experiment, err := manager.CreateExperiment(experiment)
apiExperiment := &api.Experiment{Name: "e1"}
experiment, err := manager.CreateExperiment(apiExperiment)
assert.Nil(t, err)
return store, manager, experiment
}
Expand All @@ -98,8 +98,8 @@ func initWithExperimentAndPipeline(t *testing.T) (*FakeClientManager, *ResourceM
initEnvVars()
store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
manager := NewResourceManager(store)
experiment := &model.Experiment{Name: "e1"}
experiment, err := manager.CreateExperiment(experiment)
apiExperiment := &api.Experiment{Name: "e1"}
experiment, err := manager.CreateExperiment(apiExperiment)
assert.Nil(t, err)
pipeline, err := manager.CreatePipeline("p1", "", []byte(testWorkflow.ToStringForStore()))
assert.Nil(t, err)
Expand Down Expand Up @@ -307,8 +307,8 @@ func TestGetPipelineTemplate_PipelineFileNotFound(t *testing.T) {
func TestCreateRun_ThroughPipelineID(t *testing.T) {
store, manager, p := initWithPipeline(t)
defer store.Close()
experiment := &model.Experiment{Name: "e1"}
experiment, err := manager.CreateExperiment(experiment)
apiExperiment := &api.Experiment{Name: "e1"}
experiment, err := manager.CreateExperiment(apiExperiment)
assert.Nil(t, err)
apiRun := &api.Run{
Name: "run1",
Expand Down Expand Up @@ -898,8 +898,8 @@ func TestCreateJob_ThroughWorkflowSpec(t *testing.T) {
func TestCreateJob_ThroughPipelineID(t *testing.T) {
store, manager, pipeline := initWithPipeline(t)
defer store.Close()
experiment := &model.Experiment{Name: "e1"}
experiment, err := manager.CreateExperiment(experiment)
apiExperiment := &api.Experiment{Name: "e1"}
experiment, err := manager.CreateExperiment(apiExperiment)
job := &api.Job{
Name: "j1",
Enabled: true,
Expand Down Expand Up @@ -2290,3 +2290,46 @@ func TestDeletePipelineVersion_FileError(t *testing.T) {
assert.Nil(t, err)
assert.NotNil(t, version)
}

func TestCreateDefaultExperiment(t *testing.T) {
store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
defer store.Close()
manager := NewResourceManager(store)

experimentID, err := manager.CreateDefaultExperiment()
assert.Nil(t, err)
experiment, err := manager.GetExperiment(experimentID)
assert.Nil(t, err)

expectedExperiment := &model.Experiment{
UUID: DefaultFakeUUID,
CreatedAtInSec: 1,
Name: "Default",
Description: "All runs created without specifying an experiment will be grouped here.",
Namespace: "",
}
assert.Equal(t, expectedExperiment, experiment)
}

func TestCreateDefaultExperiment_MultiUser(t *testing.T) {
viper.Set(common.MultiUserMode, "true")
defer viper.Set(common.MultiUserMode, "false")

store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch())
defer store.Close()
manager := NewResourceManager(store)

experimentID, err := manager.CreateDefaultExperiment()
assert.Nil(t, err)
experiment, err := manager.GetExperiment(experimentID)
assert.Nil(t, err)

expectedExperiment := &model.Experiment{
UUID: DefaultFakeUUID,
CreatedAtInSec: 1,
Name: "Default",
Description: "All runs created without specifying an experiment will be grouped here.",
Namespace: "",
}
assert.Equal(t, expectedExperiment, experiment)
}
28 changes: 17 additions & 11 deletions backend/src/apiserver/server/api_converter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,24 @@ import (
)

func ToApiExperiment(experiment *model.Experiment) *api.Experiment {
resourceReferences := []*api.ResourceReference(nil)
if common.IsMultiUserMode() {
resourceReferences = []*api.ResourceReference{
&api.ResourceReference{
Key: &api.ResourceKey{
Type: api.ResourceType_NAMESPACE,
Id: experiment.Namespace,
},
Relationship: api.Relationship_OWNER,
},
}
}
return &api.Experiment{
Id: experiment.UUID,
Name: experiment.Name,
Description: experiment.Description,
CreatedAt: &timestamp.Timestamp{Seconds: experiment.CreatedAtInSec},
Id: experiment.UUID,
Name: experiment.Name,
Description: experiment.Description,
CreatedAt: &timestamp.Timestamp{Seconds: experiment.CreatedAtInSec},
ResourceReferences: resourceReferences,
}
}

Expand All @@ -42,13 +55,6 @@ func ToApiExperiments(experiments []*model.Experiment) []*api.Experiment {
return apiExperiments
}

func ToModelExperiment(experiment *api.Experiment) *model.Experiment {
return &model.Experiment{
Name: experiment.Name,
Description: experiment.Description,
}
}

func ToApiPipeline(pipeline *model.Pipeline) *api.Pipeline {
params, err := toApiParameters(pipeline.Parameters)
if err != nil {
Expand Down
Loading

0 comments on commit 809f97a

Please sign in to comment.