Validate algorithm (#904)

pkg/controller.v1alpha3/consts/const.go

@@ -31,9 +31,12 @@ const (
 	KatibConfigMapName = "katib-config"
 	// LabelSuggestionTag is the name of suggestion config in configmap.
 	LabelSuggestionTag = "suggestion"
-
 	// LabelSuggestionImageTag is the name of suggestion image config in configmap.
 	LabelSuggestionImageTag = "image"
+	// LabelMetricsCollectorSidecar is the name of metrics collector config in configmap.
+	LabelMetricsCollectorSidecar = "metrics-collector-sidecar"
+	// LabelMetricsCollectorSidecarImage is the name of metrics collector image config in configmap.
+	LabelMetricsCollectorSidecarImage = "image"
 
 	// ReconcileErrorReason is the reason when there is a reconcile error.
 	ReconcileErrorReason = "ReconcileError"

pkg/controller.v1alpha3/experiment/manifest/generator.go

@@ -11,6 +11,7 @@ import (
 	commonapiv1alpha3 "github.com/kubeflow/katib/pkg/apis/controller/common/v1alpha3"
 	experimentsv1alpha3 "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1alpha3"
 	"github.com/kubeflow/katib/pkg/util/v1alpha3/katibclient"
+	"github.com/kubeflow/katib/pkg/util/v1alpha3/katibconfig"
 )
 
 const (
@@ -22,6 +23,7 @@ type Generator interface {
 	InjectClient(c client.Client)
 	GetRunSpec(e *experimentsv1alpha3.Experiment, experiment, trial, namespace string) (string, error)
 	GetRunSpecWithHyperParameters(e *experimentsv1alpha3.Experiment, experiment, trial, namespace string, hps []commonapiv1alpha3.ParameterAssignment) (string, error)
+	GetSuggestionContainerImage(algorithmName string) (string, error)
 }
 
 // DefaultGenerator is the default implementation of Generator.
@@ -41,6 +43,10 @@ func (g *DefaultGenerator) InjectClient(c client.Client) {
 	g.client.InjectClient(c)
 }
 
+func (g *DefaultGenerator) GetSuggestionContainerImage(algorithmName string) (string, error) {
+	return katibconfig.GetSuggestionContainerImage(algorithmName, g.client.GetClient())
+}
+
 // GetRunSpec get the specification for trial.
 func (g *DefaultGenerator) GetRunSpec(e *experimentsv1alpha3.Experiment, experiment, trial, namespace string) (string, error) {
 	params := trialTemplateParams{

pkg/controller.v1alpha3/suggestion/composer/composer.go

@@ -1,23 +1,20 @@
 package composer
 
 import (
-	"context"
-	"encoding/json"
-	"errors"
 	"fmt"
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	apitypes "k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 
 	suggestionsv1alpha3 "github.com/kubeflow/katib/pkg/apis/controller/suggestions/v1alpha3"
 	"github.com/kubeflow/katib/pkg/controller.v1alpha3/consts"
 	"github.com/kubeflow/katib/pkg/controller.v1alpha3/util"
+	"github.com/kubeflow/katib/pkg/util/v1alpha3/katibconfig"
 )
 
 const (
@@ -111,7 +108,7 @@ func (g *General) DesiredService(s *suggestionsv1alpha3.Suggestion) (*corev1.Ser
 }
 
 func (g *General) desiredContainer(s *suggestionsv1alpha3.Suggestion) (*corev1.Container, error) {
-	suggestionContainerImage, err := g.getSuggestionContainerImage(s.Spec.AlgorithmName)
+	suggestionContainerImage, err := katibconfig.GetSuggestionContainerImage(s.Spec.AlgorithmName, g.Client)
 	if err != nil {
 		return nil, err
 	}
@@ -156,34 +153,3 @@ func (g *General) desiredContainer(s *suggestionsv1alpha3.Suggestion) (*corev1.C
 	}
 	return c, nil
 }
-
-func (g *General) getSuggestionContainerImage(algorithmName string) (string, error) {
-	configMap := &corev1.ConfigMap{}
-	err := g.Client.Get(
-		context.TODO(),
-		apitypes.NamespacedName{Name: consts.KatibConfigMapName, Namespace: consts.DefaultKatibNamespace},
-		configMap)
-	if err != nil {
-		log.Error(err, "Failed to find config map", "name", consts.KatibConfigMapName)
-		// Error reading the object - requeue the request.
-		return "", err
-	}
-	if config, ok := configMap.Data[consts.LabelSuggestionTag]; ok {
-		suggestionConfig := map[string]map[string]string{}
-		if err := json.Unmarshal([]byte(config), &suggestionConfig); err != nil {
-			log.Error(err, "Json Unmarshal error", "Config", config)
-			return "", err
-		}
-		if imageConfig, ok := suggestionConfig[algorithmName]; ok {
-			if image, yes := imageConfig[consts.LabelSuggestionImageTag]; yes {
-				return image, nil
-			} else {
-				return "", errors.New("Failed to find " + consts.LabelSuggestionImageTag + " configuration for algorithm name " + algorithmName)
-			}
-		} else {
-			return "", errors.New("Failed to find algorithm image mapping " + algorithmName)
-		}
-	} else {
-		return "", errors.New("Failed to find algorithm image mapping in configmap " + consts.KatibConfigMapName)
-	}
-}

pkg/util/v1alpha3/katibclient/katib_client.go

@@ -33,6 +33,7 @@ import (
 
 type Client interface {
 	InjectClient(c client.Client)
+	GetClient() client.Client
 	GetExperimentList(namespace ...string) (*experimentsv1alpha3.ExperimentList, error)
 	CreateExperiment(experiment *experimentsv1alpha3.Experiment, namespace ...string) error
 	DeleteExperiment(experiment *experimentsv1alpha3.Experiment, namespace ...string) error
@@ -72,6 +73,10 @@ func (k *KatibClient) InjectClient(c client.Client) {
 	k.client = c
 }
 
+func (k *KatibClient) GetClient() client.Client {
+	return k.client
+}
+
 func (k *KatibClient) GetExperimentList(namespace ...string) (*experimentsv1alpha3.ExperimentList, error) {
 	ns := getNamespace(namespace...)
 	expList := &experimentsv1alpha3.ExperimentList{}

pkg/util/v1alpha3/katibconfig/config.go

@@ -0,0 +1,80 @@
+package katibconfig
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"strings"
+
+	corev1 "k8s.io/api/core/v1"
+	apitypes "k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	common "github.com/kubeflow/katib/pkg/apis/controller/common/v1alpha3"
+	"github.com/kubeflow/katib/pkg/controller.v1alpha3/consts"
+)
+
+func GetSuggestionContainerImage(algorithmName string, client client.Client) (string, error) {
+	configMap := &corev1.ConfigMap{}
+	err := client.Get(
+		context.TODO(),
+		apitypes.NamespacedName{Name: consts.KatibConfigMapName, Namespace: consts.DefaultKatibNamespace},
+		configMap)
+	if err != nil {
+		return "", err
+	}
+	if config, ok := configMap.Data[consts.LabelSuggestionTag]; ok {
+		suggestionConfig := map[string]map[string]string{}
+		if err := json.Unmarshal([]byte(config), &suggestionConfig); err != nil {
+			return "", err
+		}
+		if imageConfig, ok := suggestionConfig[algorithmName]; ok {
+			if image, yes := imageConfig[consts.LabelSuggestionImageTag]; yes {
+				if strings.TrimSpace(image) != "" {
+					return image, nil
+				} else {
+					return "", errors.New("Required value for " + consts.LabelSuggestionImageTag + " configuration of algorithm name " + algorithmName)
+				}
+			} else {
+				return "", errors.New("Failed to find " + consts.LabelSuggestionImageTag + " configuration of algorithm name " + algorithmName)
+			}
+		} else {
+			return "", errors.New("Failed to find algorithm image mapping " + algorithmName)
+		}
+	} else {
+		return "", errors.New("Failed to find algorithm image mapping in configmap " + consts.KatibConfigMapName)
+	}
+}
+
+func GetMetricsCollectorImage(cKind common.CollectorKind, client client.Client) (string, error) {
+	configMap := &corev1.ConfigMap{}
+	err := client.Get(
+		context.TODO(),
+		apitypes.NamespacedName{Name: consts.KatibConfigMapName, Namespace: consts.DefaultKatibNamespace},
+		configMap)
+	if err != nil {
+		return "", err
+	}
+	if mcs, ok := configMap.Data[consts.LabelMetricsCollectorSidecar]; ok {
+		kind := string(cKind)
+		mcsConfig := map[string]map[string]string{}
+		if err := json.Unmarshal([]byte(mcs), &mcsConfig); err != nil {
+			return "", err
+		}
+		if mc, ok := mcsConfig[kind]; ok {
+			if image, yes := mc[consts.LabelMetricsCollectorSidecarImage]; yes {
+				if strings.TrimSpace(image) != "" {
+					return image, nil
+				} else {
+					return "", errors.New("Required value for " + consts.LabelMetricsCollectorSidecarImage + "configuration of metricsCollector kind " + kind)
+				}
+			} else {
+				return "", errors.New("Failed to find " + consts.LabelMetricsCollectorSidecarImage + " configuration of metricsCollector kind " + kind)
+			}
+		} else {
+			return "", errors.New("Cannot support metricsCollector injection for kind " + kind)
+		}
+	} else {
+		return "", errors.New("Failed to find metrics collector configuration in configmap " + consts.KatibConfigMapName)
+	}
+}

pkg/webhook/v1alpha3/experiment/validator/validator.go

@@ -87,6 +87,10 @@ func (g *DefaultValidator) validateAlgorithm(ag *commonapiv1alpha3.AlgorithmSpec
 		return fmt.Errorf("No spec.algorithm.name specified.")
 	}
 
+	if _, err := g.GetSuggestionContainerImage(ag.AlgorithmName); err != nil {
+		return fmt.Errorf("Don't support algorithm %s: %v.", ag.AlgorithmName, err)
+	}
+
 	return nil
 }
 

pkg/webhook/v1alpha3/experiment/validator/validator_test.go

@@ -85,6 +85,7 @@ metadata:
   namespace: fakens`
 
 	p.EXPECT().GetRunSpec(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(trialJobTemplate, nil).AnyTimes()
+	p.EXPECT().GetSuggestionContainerImage(gomock.Any()).Return("algorithmImage", nil).AnyTimes()
 
 	tcs := []struct {
 		Instance *experimentsv1alpha3.Experiment

pkg/webhook/v1alpha3/pod/const.go

@@ -17,9 +17,6 @@ limitations under the License.
 package pod
 
 const (
-	MasterRole                   = "master"
-	MetricsCollectorSidecar      = "metrics-collector-sidecar"
-	MetricsCollectorSidecarImage = "image"
-
-	BatchJob = "Job"
+	MasterRole = "master"
+	BatchJob   = "Job"
 )

pkg/webhook/v1alpha3/pod/inject_webhook.go

@@ -18,8 +18,6 @@ package pod
 
 import (
 	"context"
-	"encoding/json"
-	"errors"
 	"fmt"
 	"net/http"
 	"path/filepath"
@@ -39,9 +37,9 @@ import (
 	common "github.com/kubeflow/katib/pkg/apis/controller/common/v1alpha3"
 	trialsv1alpha3 "github.com/kubeflow/katib/pkg/apis/controller/trials/v1alpha3"
 	katibmanagerv1alpha3 "github.com/kubeflow/katib/pkg/common/v1alpha3"
-	"github.com/kubeflow/katib/pkg/controller.v1alpha3/consts"
 	jobv1alpha3 "github.com/kubeflow/katib/pkg/job/v1alpha3"
 	mccommon "github.com/kubeflow/katib/pkg/metricscollector/v1alpha3/common"
+	"github.com/kubeflow/katib/pkg/util/v1alpha3/katibconfig"
 )
 
 var log = logf.Log.WithName("injector-webhook")
@@ -145,7 +143,7 @@ func (s *sidecarInjector) Mutate(pod *v1.Pod, namespace string) (*v1.Pod, error)
 		metricName += v
 	}
 
-	image, err := s.getMetricsCollectorImage(trial.Spec.MetricsCollector.Collector.Kind)
+	image, err := katibconfig.GetMetricsCollectorImage(trial.Spec.MetricsCollector.Collector.Kind, s.client)
 	if err != nil {
 		return nil, err
 	}
@@ -176,37 +174,6 @@ func (s *sidecarInjector) Mutate(pod *v1.Pod, namespace string) (*v1.Pod, error)
 	return mutatedPod, nil
 }
 
-func (s *sidecarInjector) getMetricsCollectorImage(cKind common.CollectorKind) (string, error) {
-	configMap := &v1.ConfigMap{}
-	err := s.client.Get(
-		context.TODO(),
-		apitypes.NamespacedName{Name: consts.KatibConfigMapName, Namespace: consts.DefaultKatibNamespace},
-		configMap)
-	if err != nil {
-		log.Error(err, "Failed to find config map", "name", consts.KatibConfigMapName)
-		// Error reading the object - requeue the request.
-		return "", err
-	}
-	if mcs, ok := configMap.Data[MetricsCollectorSidecar]; ok {
-		kind := string(cKind)
-		mcsConfig := map[string]map[string]string{}
-		if err := json.Unmarshal([]byte(mcs), &mcsConfig); err != nil {
-			return "", err
-		}
-		if mc, ok := mcsConfig[kind]; ok {
-			if image, yes := mc[MetricsCollectorSidecarImage]; yes {
-				return image, nil
-			} else {
-				return "", errors.New("Failed to find " + MetricsCollectorSidecarImage + " configuration for metricsCollector kind " + kind)
-			}
-		} else {
-			return "", errors.New("Cannot support metricsCollector injection for kind " + kind)
-		}
-	} else {
-		return "", errors.New("Failed to find metrics collector configuration in configmap " + consts.KatibConfigMapName)
-	}
-}
-
 func getMetricsCollectorArgs(trialName, metricName string, mc common.MetricsCollectorSpec) []string {
 	args := []string{"-t", trialName, "-m", metricName, "-s", katibmanagerv1alpha3.GetManagerAddr()}
 	if mountPath, _ := getMountPath(mc); mountPath != "" {