refactor(suggestion): Use interface (#502)

* refactor(suggestion): Use interface

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* feat(cmd): Add flags to control the fake logic

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* gopkg: Update

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Revert package names

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Use string

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Address comments

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Address comment

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* fix: Fix bug

Signed-off-by: Ce Gao <gaoce@caicloud.io>

cmd/katib-controller/v1alpha2/main.go

@@ -20,49 +20,70 @@ limitations under the License.
 package main
 
 import (
-	"log"
+	"flag"
+	"os"
 
-	"github.com/kubeflow/katib/pkg/api/operators/apis"
-	controller "github.com/kubeflow/katib/pkg/controller/v1alpha2"
+	"github.com/spf13/viper"
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 	"sigs.k8s.io/controller-runtime/pkg/runtime/signals"
+
+	"github.com/kubeflow/katib/pkg/api/operators/apis"
+	controller "github.com/kubeflow/katib/pkg/controller/v1alpha2"
+	"github.com/kubeflow/katib/pkg/controller/v1alpha2/consts"
 )
 
 func main() {
 	logf.SetLogger(logf.ZapLogger(false))
+	log := logf.Log.WithName("entrypoint")
+
+	var experimentSuggestionName string
+
+	flag.StringVar(&experimentSuggestionName, "experiment-suggestion-name",
+		"default", "The implementation of suggestion interface in experiment controller (default|fake)")
+
+	flag.Parse()
+
+	viper.Set(consts.ConfigExperimentSuggestionName, experimentSuggestionName)
+	log.Info("Config:",
+		consts.ConfigExperimentSuggestionName,
+		viper.GetString(consts.ConfigExperimentSuggestionName))
+
 	// Get a config to talk to the apiserver
 	cfg, err := config.GetConfig()
 	if err != nil {
-		log.Printf("config.GetConfig()")
-		log.Fatal(err)
+		log.Error(err, "Fail to get the config")
+		os.Exit(1)
 	}
 
 	// Create a new katib controller to provide shared dependencies and start components
 	mgr, err := manager.New(cfg, manager.Options{})
 	if err != nil {
-		log.Printf("manager.New")
-		log.Fatal(err)
+		log.Error(err, "unable add APIs to scheme")
+		os.Exit(1)
 	}
 
-	log.Printf("Registering Components.")
+	log.Info("Registering Components.")
 
 	// Setup Scheme for all resources
 	if err := apis.AddToScheme(mgr.GetScheme()); err != nil {
-		log.Printf("apis.AddToScheme")
-		log.Fatal(err)
+		log.Error(err, "Fail to create the manager")
+		os.Exit(1)
 	}
 
-	// Setup katib controller
+	// Setup all Controllers
+	log.Info("Setting up controller")
 	if err := controller.AddToManager(mgr); err != nil {
-		log.Printf("controller.AddToManager(mgr)")
-		log.Fatal(err)
+		log.Error(err, "unable to register controllers to the manager")
+		os.Exit(1)
 	}
 
-	log.Printf("Starting the Cmd.")
-
-	// Starting the katib controller
-	log.Fatal(mgr.Start(signals.SetupSignalHandler()))
+	// Start the Cmd
+	log.Info("Starting the Cmd.")
+	if err := mgr.Start(signals.SetupSignalHandler()); err != nil {
+		log.Error(err, "unable to run the manager")
+		os.Exit(1)
+	}
 }

pkg/controller/v1alpha2/consts/const.go

@@ -0,0 +1,5 @@
+package consts
+
+const (
+	ConfigExperimentSuggestionName string = "experiment-suggestion-name"
+)

pkg/controller/v1alpha2/experiment/experiment_controller.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"os"
 
+	"github.com/spf13/viper"
 	admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -38,10 +39,13 @@ import (
 
 	experimentsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/experiment/v1alpha2"
 	trialsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/trial/v1alpha2"
+	"github.com/kubeflow/katib/pkg/controller/v1alpha2/consts"
+	"github.com/kubeflow/katib/pkg/controller/v1alpha2/experiment/suggestion"
+	suggestionfake "github.com/kubeflow/katib/pkg/controller/v1alpha2/experiment/suggestion/fake"
 	"github.com/kubeflow/katib/pkg/controller/v1alpha2/experiment/util"
 )
 
-const KatibController = "katib-controller"
+const katibControllerName = "katib-controller"
 
 var log = logf.Log.WithName("experiment-controller")
 
@@ -58,7 +62,30 @@ func Add(mgr manager.Manager) error {
 
 // newReconciler returns a new reconcile.Reconciler
 func newReconciler(mgr manager.Manager) reconcile.Reconciler {
-	return &ReconcileExperiment{Client: mgr.GetClient(), scheme: mgr.GetScheme()}
+	r := &ReconcileExperiment{
+		Client: mgr.GetClient(),
+		scheme: mgr.GetScheme(),
+	}
+	imp := viper.GetString(consts.ConfigExperimentSuggestionName)
+	r.Suggestion = newSuggestion(imp)
+	return r
+}
+
+// newSuggestion returns the new Suggestion for the given config.
+func newSuggestion(config string) suggestion.Suggestion {
+	// Use different implementation according to the configuration.
+	switch config {
+	case "fake":
+		log.Info("Using the fake suggestion implementation")
+		return suggestionfake.New()
+	case "default":
+		log.Info("Using the default suggestion implementation")
+		return suggestion.New()
+	default:
+		log.Info("No valid name specified, using the default suggestion implementation",
+			"implementation", config)
+		return suggestion.New()
+	}
 }
 
 // add adds a new Controller to mgr with r as the reconcile.Reconciler
@@ -126,13 +153,13 @@ func addWebhook(mgr manager.Manager) error {
 		BootstrapOptions: &webhook.BootstrapOptions{
 			Secret: &types.NamespacedName{
 				Namespace: os.Getenv(experimentsv1alpha2.DefaultKatibNamespaceEnvName),
-				Name:      KatibController,
+				Name:      katibControllerName,
 			},
 			Service: &webhook.Service{
 				Namespace: os.Getenv(experimentsv1alpha2.DefaultKatibNamespaceEnvName),
-				Name:      KatibController,
+				Name:      katibControllerName,
 				Selectors: map[string]string{
-					"app": KatibController,
+					"app": katibControllerName,
 				},
 			},
 			ValidatingWebhookConfigName: "experiment-validating-webhook-config",
@@ -155,6 +182,7 @@ var _ reconcile.Reconciler = &ReconcileExperiment{}
 type ReconcileExperiment struct {
 	client.Client
 	scheme *runtime.Scheme
+	suggestion.Suggestion
 }
 
 // Reconcile reads that state of the cluster for a Experiment object and makes changes based on the state read
@@ -303,14 +331,11 @@ func (r *ReconcileExperiment) ReconcileTrials(instance *experimentsv1alpha2.Expe
 func (r *ReconcileExperiment) createTrials(instance *experimentsv1alpha2.Experiment, addCount int) error {
 
 	logger := log.WithValues("Experiment", types.NamespacedName{Name: instance.GetName(), Namespace: instance.GetNamespace()})
-	trials, err := util.GetSuggestions(instance, addCount)
+	trials, err := r.GetSuggestions(instance, addCount)
 	if err != nil {
 		logger.Error(err, "Get suggestions error")
 		return err
 	}
-	/*trials := []apiv1alpha2.Trial{
-		apiv1alpha2.Trial{Spec: &apiv1alpha2.TrialSpec{}}, apiv1alpha2.Trial{Spec: &apiv1alpha2.TrialSpec{}},
-	}*/
 	for _, trial := range trials {
 		if err = r.createTrialInstance(instance, trial); err != nil {
 			logger.Error(err, "Create trial instance error", "trial", trial)

pkg/controller/v1alpha2/experiment/suggestion/fake/fake.go

@@ -0,0 +1,18 @@
+package fake
+
+import (
+	experimentsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/experiment/v1alpha2"
+	api_pb "github.com/kubeflow/katib/pkg/api/v1alpha2"
+	"github.com/kubeflow/katib/pkg/controller/v1alpha2/experiment/suggestion"
+)
+
+type Fake struct {
+}
+
+func New() suggestion.Suggestion {
+	return &Fake{}
+}
+
+func (k *Fake) GetSuggestions(instance *experimentsv1alpha2.Experiment, addCount int) ([]*api_pb.Trial, error) {
+	return nil, nil
+}

pkg/controller/v1alpha2/experiment/suggestion/suggestion.go

@@ -0,0 +1,23 @@
+package suggestion
+
+import (
+	"fmt"
+
+	experimentsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/experiment/v1alpha2"
+	api_pb "github.com/kubeflow/katib/pkg/api/v1alpha2"
+)
+
+type Suggestion interface {
+	GetSuggestions(instance *experimentsv1alpha2.Experiment, addCount int) ([]*api_pb.Trial, error)
+}
+
+type General struct {
+}
+
+func New() Suggestion {
+	return &General{}
+}
+
+func (g *General) GetSuggestions(instance *experimentsv1alpha2.Experiment, addCount int) ([]*api_pb.Trial, error) {
+	return nil, fmt.Errorf("Not implemented")
+}