From 574c6570964b4321541cc5d7d333856315e925e3 Mon Sep 17 00:00:00 2001 From: Hougang Liu Date: Fri, 1 Nov 2019 09:37:23 +0800 Subject: [PATCH] Validate algorithm (#904) --- pkg/controller.v1alpha3/consts/const.go | 5 +- .../experiment/manifest/generator.go | 6 ++ .../suggestion/composer/composer.go | 38 +-------- .../v1alpha3/experiment/manifest/generator.go | 15 ++++ .../v1alpha3/util/katibclient/katibclient.go | 14 ++++ pkg/util/v1alpha3/katibclient/katib_client.go | 5 ++ pkg/util/v1alpha3/katibconfig/config.go | 80 +++++++++++++++++++ .../experiment/validator/validator.go | 4 + .../experiment/validator/validator_test.go | 1 + pkg/webhook/v1alpha3/pod/const.go | 7 +- pkg/webhook/v1alpha3/pod/inject_webhook.go | 37 +-------- 11 files changed, 135 insertions(+), 77 deletions(-) create mode 100644 pkg/util/v1alpha3/katibconfig/config.go diff --git a/pkg/controller.v1alpha3/consts/const.go b/pkg/controller.v1alpha3/consts/const.go index 26cbff18f98..9486ae8bb01 100644 --- a/pkg/controller.v1alpha3/consts/const.go +++ b/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" diff --git a/pkg/controller.v1alpha3/experiment/manifest/generator.go b/pkg/controller.v1alpha3/experiment/manifest/generator.go index 01b1010ba53..f4a0d79ef48 100644 --- a/pkg/controller.v1alpha3/experiment/manifest/generator.go +++ b/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{ diff --git a/pkg/controller.v1alpha3/suggestion/composer/composer.go b/pkg/controller.v1alpha3/suggestion/composer/composer.go index 8eb07b306d2..0a66ed90b07 100644 --- a/pkg/controller.v1alpha3/suggestion/composer/composer.go +++ b/pkg/controller.v1alpha3/suggestion/composer/composer.go @@ -1,16 +1,12 @@ 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" @@ -18,6 +14,7 @@ import ( 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) - } -} diff --git a/pkg/mock/v1alpha3/experiment/manifest/generator.go b/pkg/mock/v1alpha3/experiment/manifest/generator.go index 60de2c9620b..9c312b87626 100644 --- a/pkg/mock/v1alpha3/experiment/manifest/generator.go +++ b/pkg/mock/v1alpha3/experiment/manifest/generator.go @@ -65,6 +65,21 @@ func (mr *MockGeneratorMockRecorder) GetRunSpecWithHyperParameters(arg0, arg1, a return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRunSpecWithHyperParameters", reflect.TypeOf((*MockGenerator)(nil).GetRunSpecWithHyperParameters), arg0, arg1, arg2, arg3, arg4) } +// GetSuggestionContainerImage mocks base method +func (m *MockGenerator) GetSuggestionContainerImage(arg0 string) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetSuggestionContainerImage", arg0) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetSuggestionContainerImage indicates an expected call of GetSuggestionContainerImage +func (mr *MockGeneratorMockRecorder) GetSuggestionContainerImage(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetSuggestionContainerImage", reflect.TypeOf((*MockGenerator)(nil).GetSuggestionContainerImage), arg0) +} + // InjectClient mocks base method func (m *MockGenerator) InjectClient(arg0 client.Client) { m.ctrl.T.Helper() diff --git a/pkg/mock/v1alpha3/util/katibclient/katibclient.go b/pkg/mock/v1alpha3/util/katibclient/katibclient.go index c446ee3853c..95d452683d5 100644 --- a/pkg/mock/v1alpha3/util/katibclient/katibclient.go +++ b/pkg/mock/v1alpha3/util/katibclient/katibclient.go @@ -74,6 +74,20 @@ func (mr *MockClientMockRecorder) DeleteExperiment(arg0 interface{}, arg1 ...int return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteExperiment", reflect.TypeOf((*MockClient)(nil).DeleteExperiment), varargs...) } +// GetClient mocks base method +func (m *MockClient) GetClient() client.Client { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetClient") + ret0, _ := ret[0].(client.Client) + return ret0 +} + +// GetClient indicates an expected call of GetClient +func (mr *MockClientMockRecorder) GetClient() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetClient", reflect.TypeOf((*MockClient)(nil).GetClient)) +} + // GetConfigMap mocks base method func (m *MockClient) GetConfigMap(arg0 string, arg1 ...string) (map[string]string, error) { m.ctrl.T.Helper() diff --git a/pkg/util/v1alpha3/katibclient/katib_client.go b/pkg/util/v1alpha3/katibclient/katib_client.go index 05542d7dda3..bee33901cef 100644 --- a/pkg/util/v1alpha3/katibclient/katib_client.go +++ b/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{} diff --git a/pkg/util/v1alpha3/katibconfig/config.go b/pkg/util/v1alpha3/katibconfig/config.go new file mode 100644 index 00000000000..29df9f2855a --- /dev/null +++ b/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) + } +} diff --git a/pkg/webhook/v1alpha3/experiment/validator/validator.go b/pkg/webhook/v1alpha3/experiment/validator/validator.go index 66567d643bc..302e0be3772 100644 --- a/pkg/webhook/v1alpha3/experiment/validator/validator.go +++ b/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 } diff --git a/pkg/webhook/v1alpha3/experiment/validator/validator_test.go b/pkg/webhook/v1alpha3/experiment/validator/validator_test.go index 592d61c251f..dbb2d5ce4fb 100644 --- a/pkg/webhook/v1alpha3/experiment/validator/validator_test.go +++ b/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 diff --git a/pkg/webhook/v1alpha3/pod/const.go b/pkg/webhook/v1alpha3/pod/const.go index 8db93684c70..53104cf9251 100644 --- a/pkg/webhook/v1alpha3/pod/const.go +++ b/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" ) diff --git a/pkg/webhook/v1alpha3/pod/inject_webhook.go b/pkg/webhook/v1alpha3/pod/inject_webhook.go index fbe12992c6f..f8f64233714 100644 --- a/pkg/webhook/v1alpha3/pod/inject_webhook.go +++ b/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 != "" {