Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Backend][Multi-user] Add namespace to Experiment API implementation #3243

Merged
merged 6 commits into from
Mar 17, 2020
Merged

[Backend][Multi-user] Add namespace to Experiment API implementation #3243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b70e7ff
Experiment API implementation
chensun Feb 29, 2020
b8a8c86
Address review comments and add additional unit tests
chensun Mar 14, 2020
0ff252f
Update build file
chensun Mar 14, 2020
0b4b38d
fix unit tests
chensun Mar 14, 2020
d06f4d6
Reject empty namespace in multi user mode
chensun Mar 16, 2020
0dcd2a1
Address review comments
chensun Mar 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backend/src/apiserver/resource/BUILD.bazel
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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.",
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no namespace reference here, so it will always fail in multi user mode.

Shall we add a check earlier? We just skip trying to create a default namespace when in multi user mode?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! Apparently this is one place I missed unit test coverage, because this default experiment creation doesn't hit experiment_server.
To allow creating default experiment in multi-user mode, I moved the namespace requirement out of ToModelExperiment -- it is enforced in experimentServer.CreateExperiment which is sufficient to capture invalid input. Also added unit tests for CreateDefaultExperiment and ToModelExperiment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully remember your design about default experiment. If default experiment doesn't contain a namespace, it will be useless in multi user mode.

Maybe we should just throw an error message here for multi user mode like Please specify an experiment. No default experiment has been set up.?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is:

  1. for short term, we provide a SQL script to help users backfill existing experiments including the default one.
  2. long term, we will automate the backfill in api-server using the namespace specified by a config map/environment variable.
    So allowing the default experiment without namespace is okay for now.

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
Open in desktop
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
Open in desktop
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 {
Bobgy marked this conversation as resolved.
Show resolved Hide resolved
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