fix: Address comments

Signed-off-by: Ce Gao <gaoce@caicloud.io>
kubeflow · May 15, 2019 · 452f928 · 452f928
1 parent f0f4a6a
commit 452f928
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 61 deletions.
diff --git a/pkg/controller/v1alpha2/experiment/experiment_controller.go b/pkg/controller/v1alpha2/experiment/experiment_controller.go
@@ -70,11 +70,11 @@ func newReconciler(mgr manager.Manager) reconcile.Reconciler {
 	imp := viper.GetString(consts.ConfigExperimentSuggestionName)
 	r.Suggestion = newSuggestion(imp)
 
-	producer, err := manifest.New()
+	generator, err := manifest.New()
 	if err != nil {
 		panic(err)
 	}
-	r.Producer = producer
+	r.Generator = generator
 	return r
 }
 
@@ -195,7 +195,7 @@ type ReconcileExperiment struct {
 	scheme *runtime.Scheme
 
 	suggestion.Suggestion
-	manifest.Producer
+	manifest.Generator
 }
 
 // Reconcile reads that state of the cluster for a Experiment object and makes changes based on the state read

diff --git a/.../v1alpha2/experiment/manifest/producer.go → ...v1alpha2/experiment/manifest/generator.go b/.../v1alpha2/experiment/manifest/producer.go → ...v1alpha2/experiment/manifest/generator.go
@@ -20,30 +20,30 @@ const (
 	defaultMetricsCollectorTemplateName = "defaultMetricsCollectorTemplate.yaml"
 )
 
-// Producer is the type for manifests producer.
-type Producer interface {
+// Generator is the type for manifests Generator.
+type Generator interface {
 	GetRunSpec(e *experimentsv1alpha2.Experiment, experiment, trial, namespace string) (string, error)
 	GetRunSpecWithHyperParameters(e *experimentsv1alpha2.Experiment, experiment, trial, namespace string, hps []*apiv1alpha2.ParameterAssignment) (string, error)
 	GetMetricsCollectorManifest(experimentName string, trialName string, jobKind string, namespace string, metricNames []string, mcs *experimentsv1alpha2.MetricsCollectorSpec) (*bytes.Buffer, error)
 }
 
-// General is the default implementation of Producer.
-type General struct {
+// DefaultGenerator is the default implementation of Generator.
+type DefaultGenerator struct {
 	client katibclient.Client
 }
 
-// New creates a new Producer.
-func New() (Producer, error) {
+// New creates a new Generator.
+func New() (Generator, error) {
 	katibClient, err := katibclient.NewClient(client.Options{})
 	if err != nil {
 		return nil, err
 	}
-	return &General{
+	return &DefaultGenerator{
 		client: katibClient,
 	}, nil
 }
 
-func (g *General) GetMetricsCollectorManifest(experimentName string, trialName string, jobKind string, namespace string, metricNames []string, mcs *experimentsv1alpha2.MetricsCollectorSpec) (*bytes.Buffer, error) {
+func (g *DefaultGenerator) GetMetricsCollectorManifest(experimentName string, trialName string, jobKind string, namespace string, metricNames []string, mcs *experimentsv1alpha2.MetricsCollectorSpec) (*bytes.Buffer, error) {
 	var mtp *template.Template = nil
 	var err error
 	tmpValues := map[string]string{
@@ -83,7 +83,7 @@ func (g *General) GetMetricsCollectorManifest(experimentName string, trialName s
 }
 
 // GetRunSpec get the specification for trial.
-func (g *General) GetRunSpec(e *experimentsv1alpha2.Experiment, experiment, trial, namespace string) (string, error) {
+func (g *DefaultGenerator) GetRunSpec(e *experimentsv1alpha2.Experiment, experiment, trial, namespace string) (string, error) {
 	params := trialTemplateParams{
 		Experiment: experiment,
 		Trial:      trial,
@@ -93,7 +93,7 @@ func (g *General) GetRunSpec(e *experimentsv1alpha2.Experiment, experiment, tria
 }
 
 // GetRunSpecWithHyperParameters get the specification for trial with hyperparameters.
-func (g *General) GetRunSpecWithHyperParameters(e *experimentsv1alpha2.Experiment, experiment, trial, namespace string, hps []*apiv1alpha2.ParameterAssignment) (string, error) {
+func (g *DefaultGenerator) GetRunSpecWithHyperParameters(e *experimentsv1alpha2.Experiment, experiment, trial, namespace string, hps []*apiv1alpha2.ParameterAssignment) (string, error) {
 	params := trialTemplateParams{
 		Experiment:      experiment,
 		Trial:           trial,
@@ -103,7 +103,7 @@ func (g *General) GetRunSpecWithHyperParameters(e *experimentsv1alpha2.Experimen
 	return g.getRunSpec(e, params)
 }
 
-func (g *General) getRunSpec(e *experimentsv1alpha2.Experiment, params trialTemplateParams) (string, error) {
+func (g *DefaultGenerator) getRunSpec(e *experimentsv1alpha2.Experiment, params trialTemplateParams) (string, error) {
 	var buf bytes.Buffer
 	trialTemplate, err := g.getTrialTemplate(e)
 	if err != nil {
@@ -115,7 +115,7 @@ func (g *General) getRunSpec(e *experimentsv1alpha2.Experiment, params trialTemp
 	return buf.String(), nil
 }
 
-func (g *General) getTrialTemplate(instance *experimentsv1alpha2.Experiment) (*template.Template, error) {
+func (g *DefaultGenerator) getTrialTemplate(instance *experimentsv1alpha2.Experiment) (*template.Template, error) {
 	var err error
 	var tpl *template.Template = nil
 

diff --git a/...pha2/experiment/manifest/producer_test.go → ...ha2/experiment/manifest/generator_test.go b/...pha2/experiment/manifest/producer_test.go → ...ha2/experiment/manifest/generator_test.go
@@ -80,7 +80,7 @@ func TestGetMetricsCollectorManifest(t *testing.T) {
 
 	c := katibclientmock.NewMockClient(mockCtrl)
 
-	p := &General{
+	p := &DefaultGenerator{
 		client: c,
 	}
 
@@ -145,7 +145,7 @@ func TestGetTrialTemplateConfigMap(t *testing.T) {
 
 	c := katibclientmock.NewMockClient(mockCtrl)
 
-	p := &General{
+	p := &DefaultGenerator{
 		client: c,
 	}
 
@@ -179,7 +179,7 @@ func TestGetTrialTemplate(t *testing.T) {
 
 	c := katibclientmock.NewMockClient(mockCtrl)
 
-	p := &General{
+	p := &DefaultGenerator{
 		client: c,
 	}
 
@@ -209,7 +209,7 @@ func TestGetRunSpec(t *testing.T) {
 
 	c := katibclientmock.NewMockClient(mockCtrl)
 
-	p := &General{
+	p := &DefaultGenerator{
 		client: c,
 	}
 
@@ -245,7 +245,7 @@ func TestGetRunSpecWithHP(t *testing.T) {
 
 	c := katibclientmock.NewMockClient(mockCtrl)
 
-	p := &General{
+	p := &DefaultGenerator{
 		client: c,
 	}
 

diff --git a/pkg/controller/v1alpha2/experiment/validator/validator.go b/pkg/controller/v1alpha2/experiment/validator/validator.go
@@ -7,13 +7,12 @@ import (
 
 	batchv1beta "k8s.io/api/batch/v1beta1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	k8syaml "k8s.io/apimachinery/pkg/util/yaml"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 
 	commonapiv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/common/v1alpha2"
-	commonv1alpha2 "github.com/kubeflow/katib/pkg/common/v1alpha2"
 	experimentsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/experiment/v1alpha2"
+	commonv1alpha2 "github.com/kubeflow/katib/pkg/common/v1alpha2"
 	"github.com/kubeflow/katib/pkg/controller/v1alpha2/experiment/manifest"
 	"github.com/kubeflow/katib/pkg/controller/v1alpha2/experiment/util"
 )
@@ -24,17 +23,17 @@ type Validator interface {
 	ValidateExperiment(instance *experimentsv1alpha2.Experiment) error
 }
 
-type General struct {
-	manifest.Producer
+type DefaultValidator struct {
+	manifest.Generator
 }
 
-func New(producer manifest.Producer) Validator {
-	return &General{
-		Producer: producer,
+func New(generator manifest.Generator) Validator {
+	return &DefaultValidator{
+		Generator: generator,
 	}
 }
 
-func (g *General) ValidateExperiment(instance *experimentsv1alpha2.Experiment) error {
+func (g *DefaultValidator) ValidateExperiment(instance *experimentsv1alpha2.Experiment) error {
 	if !instance.IsCreated() {
 		if err := g.validateForCreate(instance); err != nil {
 			return err
@@ -70,12 +69,12 @@ func (g *General) ValidateExperiment(instance *experimentsv1alpha2.Experiment) e
 	return nil
 }
 
-func (g *General) validateAlgorithmSettings(inst *experimentsv1alpha2.Experiment) error {
+func (g *DefaultValidator) validateAlgorithmSettings(inst *experimentsv1alpha2.Experiment) error {
 	// TODO: it need call ValidateAlgorithmSettings API of vizier-core manager, implement it when vizier-core done
 	return nil
 }
 
-func (g *General) validateObjective(obj *commonapiv1alpha2.ObjectiveSpec) error {
+func (g *DefaultValidator) validateObjective(obj *commonapiv1alpha2.ObjectiveSpec) error {
 	if obj == nil {
 		return fmt.Errorf("No spec.objective specified.")
 	}
@@ -88,7 +87,7 @@ func (g *General) validateObjective(obj *commonapiv1alpha2.ObjectiveSpec) error
 	return nil
 }
 
-func (g *General) validateAlgorithm(ag *experimentsv1alpha2.AlgorithmSpec) error {
+func (g *DefaultValidator) validateAlgorithm(ag *experimentsv1alpha2.AlgorithmSpec) error {
 	if ag == nil {
 		return fmt.Errorf("No spec.algorithm specified.")
 	}
@@ -99,7 +98,7 @@ func (g *General) validateAlgorithm(ag *experimentsv1alpha2.AlgorithmSpec) error
 	return nil
 }
 
-func (g *General) validateTrialTemplate(instance *experimentsv1alpha2.Experiment) error {
+func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1alpha2.Experiment) error {
 	trialName := fmt.Sprintf("%s-trial", instance.GetName())
 	runSpec, err := g.GetRunSpec(instance, instance.GetName(), trialName, instance.GetNamespace())
 	if err != nil {
@@ -127,7 +126,7 @@ func (g *General) validateTrialTemplate(instance *experimentsv1alpha2.Experiment
 	return nil
 }
 
-func (g *General) validateSupportedJob(job *unstructured.Unstructured) error {
+func (g *DefaultValidator) validateSupportedJob(job *unstructured.Unstructured) error {
 	gvk := job.GroupVersionKind()
 	supportedJobs := commonv1alpha2.GetSupportedJobList()
 	for _, sJob := range supportedJobs {
@@ -138,7 +137,7 @@ func (g *General) validateSupportedJob(job *unstructured.Unstructured) error {
 	return fmt.Errorf("Job type %v not supported", gvk)
 }
 
-func (g *General) validateForCreate(inst *experimentsv1alpha2.Experiment) error {
+func (g *DefaultValidator) validateForCreate(inst *experimentsv1alpha2.Experiment) error {
 	if _, err := util.GetExperimentFromDB(inst); err != nil {
 		if err != sql.ErrNoRows {
 			return fmt.Errorf("Fail to check record for the experiment in DB: %v", err)
@@ -149,7 +148,7 @@ func (g *General) validateForCreate(inst *experimentsv1alpha2.Experiment) error
 	}
 }
 
-func (g *General) validateMetricsCollector(inst *experimentsv1alpha2.Experiment) error {
+func (g *DefaultValidator) validateMetricsCollector(inst *experimentsv1alpha2.Experiment) error {
 	BUFSIZE := 1024
 	experimentName := inst.GetName()
 	trialName := fmt.Sprintf("%s-trial", inst.GetName())
@@ -190,15 +189,3 @@ func (g *General) validateMetricsCollector(inst *experimentsv1alpha2.Experiment)
 	}
 	return nil
 }
-
-func getSupportedJobList() []schema.GroupVersionKind {
-	// TODO: append other supported jobs, such as tfjob, pytorch and so on
-	supportedJobList := []schema.GroupVersionKind{
-		schema.GroupVersionKind{
-			Group:   "batch",
-			Version: "v1",
-			Kind:    "Job",
-		},
-	}
-	return supportedJobList
-}
diff --git a/pkg/controller/v1alpha2/experiment/validator/validator_test.go b/pkg/controller/v1alpha2/experiment/validator/validator_test.go
@@ -40,7 +40,7 @@ spec:
 	p.EXPECT().GetRunSpec(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(trialTFJobTemplate, nil)
 
 	instance := newFakeInstance()
-	if err := g.(*General).validateTrialTemplate(instance); err == nil {
+	if err := g.(*DefaultValidator).validateTrialTemplate(instance); err == nil {
 		t.Errorf("Expected error, got nil")
 	}
 }
@@ -61,7 +61,7 @@ metadata:
 	p.EXPECT().GetRunSpec(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(trialJobTemplate, nil)
 
 	instance := newFakeInstance()
-	if err := g.(*General).validateTrialTemplate(instance); err != nil {
+	if err := g.(*DefaultValidator).validateTrialTemplate(instance); err != nil {
 		t.Errorf("Expected nil, got err %v", err)
 	}
 }

diff --git a/pkg/util/v1alpha2/katibclient/katib_client.go b/pkg/util/v1alpha2/katibclient/katib_client.go
@@ -20,8 +20,6 @@ import (
 	"io/ioutil"
 	"strings"
 
-	"k8s.io/client-go/rest"
-
 	experimentsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/experiment/v1alpha2"
 	apiv1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -43,16 +41,6 @@ type KatibClient struct {
 	client client.Client
 }
 
-func MustNewTestClient(cfg *rest.Config) Client {
-	cli, err := client.New(cfg, client.Options{})
-	if err != nil {
-		panic(err)
-	}
-	return &KatibClient{
-		client: cli,
-	}
-}
-
 func NewClient(options client.Options) (Client, error) {
 	cfg, err := config.GetConfig()
 	if err != nil {