From 7ce74f4b8cff372d4332b65e363885298ee23e9b Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Fri, 1 Oct 2021 23:15:54 +0900 Subject: [PATCH 1/9] resolve conflict --- .../v1beta1/file-metricscollector/main.go | 6 +- .../experiment/manifest/generator_test.go | 2 +- .../suggestion/suggestion_controller_test.go | 6 +- pkg/new-ui/v1beta1/util.go | 2 +- pkg/ui/v1beta1/util.go | 2 +- pkg/util/v1beta1/katibconfig/config.go | 65 ++-- pkg/util/v1beta1/katibconfig/config_test.go | 333 ++++++++++++++++++ .../v1beta1/experiment/validator/validator.go | 2 +- 8 files changed, 375 insertions(+), 43 deletions(-) create mode 100644 pkg/util/v1beta1/katibconfig/config_test.go diff --git a/cmd/metricscollector/v1beta1/file-metricscollector/main.go b/cmd/metricscollector/v1beta1/file-metricscollector/main.go index 9b9f1b481b6..49476bd4db7 100644 --- a/cmd/metricscollector/v1beta1/file-metricscollector/main.go +++ b/cmd/metricscollector/v1beta1/file-metricscollector/main.go @@ -159,7 +159,7 @@ func watchMetricsFile(mFile string, stopRules stopRulesFlag, filters []string) { // Check that metric file exists. checkMetricFile(mFile) - // Get Main proccess. + // Get Main process. _, mainProcPid, err := common.GetMainProcesses(mFile) if err != nil { klog.Fatalf("GetMainProcesses failed: %v", err) @@ -271,7 +271,7 @@ func watchMetricsFile(mFile string, stopRules stopRulesFlag, filters []string) { klog.Fatalf("Write to file %v error: %v", markFile, err) } - // Get child proccess from main PID. + // Get child process from main PID. childProc, err := mainProc.Children() if err != nil { klog.Fatalf("Get children proceses for main PID: %v failed: %v", mainProcPid, err) @@ -291,7 +291,7 @@ func watchMetricsFile(mFile string, stopRules stopRulesFlag, filters []string) { // Report metrics to DB. reportMetrics(filters) - // Wait until main proccess is completed. + // Wait until main process is completed. timeout := 60 * time.Second endTime := time.Now().Add(timeout) isProcRunning := true diff --git a/pkg/controller.v1beta1/experiment/manifest/generator_test.go b/pkg/controller.v1beta1/experiment/manifest/generator_test.go index 17dd2adffea..7bfcff4eb30 100644 --- a/pkg/controller.v1beta1/experiment/manifest/generator_test.go +++ b/pkg/controller.v1beta1/experiment/manifest/generator_test.go @@ -126,7 +126,7 @@ func TestGetRunSpecWithHP(t *testing.T) { Err: true, testDescription: "Number of parameter assignments is not equal to number of Trial parameters", }, - // Parameter from assignments not found in Trial paramters + // Parameter from assignments not found in Trial parameters { Instance: newFakeInstance(), ParameterAssignments: func() []commonapiv1beta1.ParameterAssignment { diff --git a/pkg/controller.v1beta1/suggestion/suggestion_controller_test.go b/pkg/controller.v1beta1/suggestion/suggestion_controller_test.go index 732c2bfc3a5..8a3c888e37a 100644 --- a/pkg/controller.v1beta1/suggestion/suggestion_controller_test.go +++ b/pkg/controller.v1beta1/suggestion/suggestion_controller_test.go @@ -300,8 +300,10 @@ func newFakeInstance() *suggestionsv1beta1.Suggestion { func newKatibConfigMapInstance() *corev1.ConfigMap { // Create suggestion config - suggestionConfig := map[string]map[string]string{ - "random": {"image": suggestionImage}, + suggestionConfig := map[string]katibconfig.SuggestionConfig{ + "random": { + Image: suggestionName, + }, } bSuggestionConfig, _ := json.Marshal(suggestionConfig) diff --git a/pkg/new-ui/v1beta1/util.go b/pkg/new-ui/v1beta1/util.go index 7d49a15cd01..632bb249b8e 100644 --- a/pkg/new-ui/v1beta1/util.go +++ b/pkg/new-ui/v1beta1/util.go @@ -255,7 +255,7 @@ func generateNNImage(architecture string, decoder string) string { Embeding is a map: int to Parameter Parameter has id, type, Option - Beforehand substite all ' to " and wrap the string in ` + Beforehand substitute all ' to " and wrap the string in ` */ replacedDecoder := strings.Replace(decoder, `'`, `"`, -1) diff --git a/pkg/ui/v1beta1/util.go b/pkg/ui/v1beta1/util.go index a0f09b91f54..900660432bf 100644 --- a/pkg/ui/v1beta1/util.go +++ b/pkg/ui/v1beta1/util.go @@ -242,7 +242,7 @@ func generateNNImage(architecture string, decoder string) string { Embeding is a map: int to Parameter Parameter has id, type, Option - Beforehand substite all ' to " and wrap the string in ` + Beforehand substitute all ' to " and wrap the string in ` */ replacedDecoder := strings.Replace(decoder, `'`, `"`, -1) diff --git a/pkg/util/v1beta1/katibconfig/config.go b/pkg/util/v1beta1/katibconfig/config.go index 36602d812fa..8eed895ceee 100644 --- a/pkg/util/v1beta1/katibconfig/config.go +++ b/pkg/util/v1beta1/katibconfig/config.go @@ -19,7 +19,7 @@ package katibconfig import ( "context" "encoding/json" - "errors" + "fmt" "strings" corev1 "k8s.io/api/core/v1" @@ -44,6 +44,12 @@ type SuggestionConfig struct { PersistentVolumeLabels map[string]string `json:"persistentVolumeLabels,omitempty"` } +// EarlyStoppingConfig is the JSON early stopping structure in Katib config. +type EarlyStoppingConfig struct { + Image string `json:"image"` + ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"` +} + // MetricsCollectorConfig is the JSON metrics collector structure in Katib config. type MetricsCollectorConfig struct { Image string `json:"image"` @@ -52,12 +58,6 @@ type MetricsCollectorConfig struct { WaitAllProcesses *bool `json:"waitAllProcesses,omitempty"` } -// EarlyStoppingConfig is the JSON early stopping structure in Katib config. -type EarlyStoppingConfig struct { - Image string `json:"image"` - ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"` -} - // GetSuggestionConfigData gets the config data for the given suggestion algorithm name. func GetSuggestionConfigData(algorithmName string, client client.Client) (SuggestionConfig, error) { configMap := &corev1.ConfigMap{} @@ -73,7 +73,7 @@ func GetSuggestionConfigData(algorithmName string, client client.Client) (Sugges // Try to find suggestion data in config map config, ok := configMap.Data[consts.LabelSuggestionTag] if !ok { - return SuggestionConfig{}, errors.New("failed to find suggestions config in ConfigMap: " + consts.KatibConfigMapName) + return SuggestionConfig{}, fmt.Errorf("failed to find suggestions config in ConfigMap: %s", consts.KatibConfigMapName) } // Parse suggestion data to map where key = algorithm name, value = SuggestionConfig @@ -85,20 +85,17 @@ func GetSuggestionConfigData(algorithmName string, client client.Client) (Sugges // Try to find SuggestionConfig for the algorithm suggestionConfigData, ok = suggestionsConfig[algorithmName] if !ok { - return SuggestionConfig{}, errors.New("failed to find suggestion config for algorithm: " + algorithmName + " in ConfigMap: " + consts.KatibConfigMapName) + return SuggestionConfig{}, fmt.Errorf("failed to find suggestion config for algorithm: %s in ConfigMap: %s", algorithmName, consts.KatibConfigMapName) } // Get image from config image := suggestionConfigData.Image if strings.TrimSpace(image) == "" { - return SuggestionConfig{}, errors.New("required value for image configuration of algorithm name: " + algorithmName) + return SuggestionConfig{}, fmt.Errorf("required value for image configuration of algorithm name: %s", algorithmName) } - // Get Image Pull Policy - imagePullPolicy := suggestionConfigData.ImagePullPolicy - if imagePullPolicy != corev1.PullAlways && imagePullPolicy != corev1.PullIfNotPresent && imagePullPolicy != corev1.PullNever { - suggestionConfigData.ImagePullPolicy = consts.DefaultImagePullPolicy - } + // Set Image Pull Policy + suggestionConfigData.ImagePullPolicy = setImagePullPolicy(suggestionConfigData.ImagePullPolicy) // Set resource requirements for suggestion suggestionConfigData.Resource = setResourceRequirements(suggestionConfigData.Resource) @@ -119,9 +116,8 @@ func GetSuggestionConfigData(algorithmName string, client client.Client) (Sugges } // Set default resources - defaultVolumeStorage, _ := resource.ParseQuantity(consts.DefaultSuggestionVolumeStorage) if len(pvcSpec.Resources.Requests) == 0 { - + defaultVolumeStorage, _ := resource.ParseQuantity(consts.DefaultSuggestionVolumeStorage) pvcSpec.Resources.Requests = make(map[corev1.ResourceName]resource.Quantity) pvcSpec.Resources.Requests[corev1.ResourceStorage] = defaultVolumeStorage } @@ -157,7 +153,7 @@ func GetEarlyStoppingConfigData(algorithmName string, client client.Client) (Ear // Try to find early stopping data in config map. config, ok := configMap.Data[consts.LabelEarlyStoppingTag] if !ok { - return EarlyStoppingConfig{}, errors.New("failed to find early stopping config in ConfigMap: " + consts.KatibConfigMapName) + return EarlyStoppingConfig{}, fmt.Errorf("failed to find early stopping config in ConfigMap: %s", consts.KatibConfigMapName) } // Parse early stopping data to map where key = algorithm name, value = EarlyStoppingConfig. @@ -169,20 +165,17 @@ func GetEarlyStoppingConfigData(algorithmName string, client client.Client) (Ear // Try to find EarlyStoppingConfig for the algorithm. earlyStoppingConfigData, ok = earlyStoppingsConfig[algorithmName] if !ok { - return EarlyStoppingConfig{}, errors.New("failed to find early stopping config for algorithm: " + algorithmName + " in ConfigMap: " + consts.KatibConfigMapName) + return EarlyStoppingConfig{}, fmt.Errorf("failed to find early stopping config for algorithm: %s in ConfigMap: %s", algorithmName, consts.KatibConfigMapName) } // Get image from config. image := earlyStoppingConfigData.Image if strings.TrimSpace(image) == "" { - return EarlyStoppingConfig{}, errors.New("required value for image configuration of algorithm name: " + algorithmName) + return EarlyStoppingConfig{}, fmt.Errorf("required value for image configuration of algorithm name: %s", algorithmName) } - // Get Image Pull Policy. - imagePullPolicy := earlyStoppingConfigData.ImagePullPolicy - if imagePullPolicy != corev1.PullAlways && imagePullPolicy != corev1.PullIfNotPresent && imagePullPolicy != corev1.PullNever { - earlyStoppingConfigData.ImagePullPolicy = consts.DefaultImagePullPolicy - } + // Set Image Pull Policy. + earlyStoppingConfigData.ImagePullPolicy = setImagePullPolicy(earlyStoppingConfigData.ImagePullPolicy) return earlyStoppingConfigData, nil } @@ -202,7 +195,7 @@ func GetMetricsCollectorConfigData(cKind common.CollectorKind, client client.Cli // Try to find metrics collector data in config map config, ok := configMap.Data[consts.LabelMetricsCollectorSidecar] if !ok { - return MetricsCollectorConfig{}, errors.New("failed to find metrics collector config in ConfigMap: " + consts.KatibConfigMapName) + return MetricsCollectorConfig{}, fmt.Errorf("failed to find metrics collector config in ConfigMap: %s", consts.KatibConfigMapName) } // Parse metrics collector data to map where key = collector kind, value = MetricsCollectorConfig kind := string(cKind) @@ -214,20 +207,17 @@ func GetMetricsCollectorConfigData(cKind common.CollectorKind, client client.Cli // Try to find MetricsCollectorConfig for the collector kind metricsCollectorConfigData, ok = mcsConfig[kind] if !ok { - return MetricsCollectorConfig{}, errors.New("failed to find metrics collector config for kind: " + kind + " in ConfigMap: " + consts.KatibConfigMapName) + return MetricsCollectorConfig{}, fmt.Errorf("failed to find metrics collector config for kind: %s in ConfigMap: %s", kind, consts.KatibConfigMapName) } // Get image from config image := metricsCollectorConfigData.Image if strings.TrimSpace(image) == "" { - return MetricsCollectorConfig{}, errors.New("required value for image configuration of metrics collector kind: " + kind) + return MetricsCollectorConfig{}, fmt.Errorf("required value for image configuration of metrics collector kind: %s", kind) } - // Get Image Pull Policy - imagePullPolicy := metricsCollectorConfigData.ImagePullPolicy - if imagePullPolicy != corev1.PullAlways && imagePullPolicy != corev1.PullIfNotPresent && imagePullPolicy != corev1.PullNever { - metricsCollectorConfigData.ImagePullPolicy = consts.DefaultImagePullPolicy - } + // Set Image Pull Policy + metricsCollectorConfigData.ImagePullPolicy = setImagePullPolicy(metricsCollectorConfigData.ImagePullPolicy) // Set resource requirements for metrics collector metricsCollectorConfigData.Resource = setResourceRequirements(metricsCollectorConfigData.Resource) @@ -235,6 +225,13 @@ func GetMetricsCollectorConfigData(cKind common.CollectorKind, client client.Cli return metricsCollectorConfigData, nil } +func setImagePullPolicy(imagePullPolicy corev1.PullPolicy) corev1.PullPolicy { + if imagePullPolicy != corev1.PullAlways && imagePullPolicy != corev1.PullIfNotPresent && imagePullPolicy != corev1.PullNever { + return consts.DefaultImagePullPolicy + } + return imagePullPolicy +} + func setResourceRequirements(configResource corev1.ResourceRequirements) corev1.ResourceRequirements { // If requests are empty create new map @@ -298,7 +295,7 @@ func setResourceRequirements(configResource corev1.ResourceRequirements) corev1. } // If user explicitly sets ephemeral-storage value to something negative, nuke it. - // This enables compability with the GKE nodepool autoscalers, which cannot scale + // This enables compatibility with the GKE nodepool autoscalers, which cannot scale // pods which define ephemeral-storage resource constraints. if diskLimit.Sign() == -1 && diskRequest.Sign() == -1 { delete(configResource.Limits, corev1.ResourceEphemeralStorage) diff --git a/pkg/util/v1beta1/katibconfig/config_test.go b/pkg/util/v1beta1/katibconfig/config_test.go new file mode 100644 index 00000000000..142643648c5 --- /dev/null +++ b/pkg/util/v1beta1/katibconfig/config_test.go @@ -0,0 +1,333 @@ +package katibconfig + +import ( + "encoding/json" + "fmt" + "reflect" + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + commonv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1" + "github.com/kubeflow/katib/pkg/controller.v1beta1/consts" +) + +type katibConfig struct { + suggestion map[string]*SuggestionConfig + earlyStopping map[string]*EarlyStoppingConfig + metricsCollector map[string]*MetricsCollectorConfig +} + +func TestGetSuggestionConfigData(t *testing.T) { + const testAlgorithmName = "test-suggestion" + + tests := []struct { + testDescription string + katibConfigMapSuggestion *SuggestionConfig + expected *SuggestionConfig + inputAlgorithmName string + err bool + nonExistentSuggestion bool + }{ + { + testDescription: "All parameters are specified", + katibConfigMapSuggestion: func() *SuggestionConfig { + s := newFakeSuggestionConfig() + s.ImagePullPolicy = corev1.PullAlways + return s + }(), + expected: func() *SuggestionConfig { + s := newFakeSuggestionConfig() + s.ImagePullPolicy = corev1.PullAlways + return s + }(), + inputAlgorithmName: testAlgorithmName, + err: false, + }, + { + testDescription: "There is not the suggestion field in katib-config configMap", + err: true, + nonExistentSuggestion: true, + }, + { + testDescription: "There is not the AlgorithmName", + katibConfigMapSuggestion: newFakeSuggestionConfig(), + inputAlgorithmName: "invalid-algorithm-name", + err: true, + }, + { + testDescription: "Image filed is empty in katib-config configMap", + katibConfigMapSuggestion: func() *SuggestionConfig { + s := newFakeSuggestionConfig() + s.Image = "" + return s + }(), + inputAlgorithmName: testAlgorithmName, + err: true, + }, + { + testDescription: fmt.Sprintf("GetSuggestionConfigData sets %s to imagePullPolicy", consts.DefaultImagePullPolicy), + katibConfigMapSuggestion: func() *SuggestionConfig { + s := newFakeSuggestionConfig() + s.ImagePullPolicy = "" + return s + }(), + expected: newFakeSuggestionConfig(), + inputAlgorithmName: testAlgorithmName, + err: false, + }, + { + testDescription: "GetSuggestionConfigData sets resource.requests and resource.limits for the suggestion service", + katibConfigMapSuggestion: func() *SuggestionConfig { + s := newFakeSuggestionConfig() + s.Resource = corev1.ResourceRequirements{} + return s + }(), + expected: newFakeSuggestionConfig(), + inputAlgorithmName: testAlgorithmName, + err: false, + }, + { + testDescription: fmt.Sprintf("GetSuggestionConfigData sets %s to volumeMountPath", consts.DefaultContainerSuggestionVolumeMountPath), + katibConfigMapSuggestion: func() *SuggestionConfig { + s := newFakeSuggestionConfig() + s.VolumeMountPath = "" + return s + }(), + expected: newFakeSuggestionConfig(), + inputAlgorithmName: testAlgorithmName, + err: false, + }, + { + testDescription: "GetSuggestionConfigData sets accessMode and resource.requests for PVC", + katibConfigMapSuggestion: func() *SuggestionConfig { + s := newFakeSuggestionConfig() + s.PersistentVolumeClaimSpec = corev1.PersistentVolumeClaimSpec{} + return s + }(), + expected: newFakeSuggestionConfig(), + inputAlgorithmName: testAlgorithmName, + err: false, + }, + { + testDescription: fmt.Sprintf("GetSuggestionConfigData does not set %s to persistentVolumeReclaimPolicy", corev1.PersistentVolumeReclaimDelete), + katibConfigMapSuggestion: func() *SuggestionConfig { + s := newFakeSuggestionConfig() + s.PersistentVolumeSpec = corev1.PersistentVolumeSpec{} + return s + }(), + expected: func() *SuggestionConfig { + s := newFakeSuggestionConfig() + s.PersistentVolumeSpec = corev1.PersistentVolumeSpec{} + return s + }(), + inputAlgorithmName: testAlgorithmName, + err: false, + }, + } + + for _, tt := range tests { + t.Run(tt.testDescription, func(t *testing.T) { + // prepare test + config := &katibConfig{} + if !tt.nonExistentSuggestion { + config.suggestion = map[string]*SuggestionConfig{ + testAlgorithmName: tt.katibConfigMapSuggestion, + } + } + c := newFakeKubeClient(newFakeKatibConfigMap(config)) + + // start test + actual, err := GetSuggestionConfigData(tt.inputAlgorithmName, c) + if (err != nil) != tt.err { + t.Errorf("want error: %v, actual: %v", tt.err, err) + } else if tt.expected != nil { + if !reflect.DeepEqual(actual, *tt.expected) { + t.Errorf("Generated SuggestionConfig is invalid.\n\nactual:\n%v\n\nexpected:\n%v\n\n", actual, *tt.expected) + } + } + }) + } + +} + +func TestGetEarlyStoppingConfigData(t *testing.T) { + const testAlgorithmName = "test-early-stopping" + + tests := []struct { + testDescription string + katibConfigMapEarlyStopping *EarlyStoppingConfig + expected *EarlyStoppingConfig + inputAlgorithmName string + err bool + nonExistentEarlyStopping bool + }{ + {}, + } + + for _, tt := range tests { + t.Run(tt.testDescription, func(t *testing.T) { + // prepare test + config := &katibConfig{} + if !tt.nonExistentEarlyStopping { + config.earlyStopping = map[string]*EarlyStoppingConfig{ + testAlgorithmName: tt.katibConfigMapEarlyStopping, + } + } + c := newFakeKubeClient(newFakeKatibConfigMap(config)) + + // start test + actual, err := GetEarlyStoppingConfigData(tt.inputAlgorithmName, c) + if (err != nil) != tt.err { + t.Errorf("want error: %v, actual: %v", tt.err, err) + } else if tt.expected != nil { + if !reflect.DeepEqual(actual, *tt.expected) { + t.Errorf("Generated EarlyStoppingConfig is invalid.\n\nactual:\n%v\n\nexpected:\n%v\n\n", actual, *tt.expected) + } + } + }) + } +} + +func TestGetMetricsCollectorConfigData(t *testing.T) { + const testMetricsCollector = "test-metrics-collector" + + tests := []struct { + testDescription string + katibConfigMapMetricsCollector *MetricsCollectorConfig + expected *MetricsCollectorConfig + inputCollectorKind commonv1beta1.CollectorKind + err bool + nonExistentMetricsCollector bool + }{ + {}, + } + + for _, tt := range tests { + t.Run(tt.testDescription, func(t *testing.T) { + // prepare test + config := &katibConfig{} + if !tt.nonExistentMetricsCollector { + config.metricsCollector = map[string]*MetricsCollectorConfig{ + testMetricsCollector: tt.katibConfigMapMetricsCollector, + } + } + c := newFakeKubeClient(newFakeKatibConfigMap(config)) + + // start test + actual, err := GetMetricsCollectorConfigData(tt.inputCollectorKind, c) + if (err != nil) != tt.err { + t.Errorf("want error: %v, actual: %v", tt.err, err) + } else if tt.expected != nil { + if !reflect.DeepEqual(actual, *tt.expected) { + t.Errorf("Generated MetricsCollectorConfig is invalid.\n\nactual:\n%v\n\nexpected:\n%v\n\n", actual, *tt.expected) + } + } + }) + } +} + +func newFakeKubeClient(katibConfigMap *corev1.ConfigMap) client.Client { + fakeClientBuilder := fake.NewClientBuilder().WithScheme(scheme.Scheme) + if katibConfigMap != nil { + fakeClientBuilder.WithObjects(katibConfigMap) + } + return fakeClientBuilder.Build() +} + +func newFakeKatibConfigMap(config *katibConfig) *corev1.ConfigMap { + + suggestionConfig := "" + if config.suggestion != nil { + bSuggestionConfig, _ := json.Marshal(config.suggestion) + suggestionConfig = string(bSuggestionConfig) + } + + earlyStoppingConfig := "" + if config.earlyStopping != nil { + bEarlyStoppingConfig, _ := json.Marshal(config.earlyStopping) + earlyStoppingConfig = string(bEarlyStoppingConfig) + } + + metricsCollector := "" + if config.metricsCollector != nil { + bMetricsCollector, _ := json.Marshal(config.metricsCollector) + metricsCollector = string(bMetricsCollector) + } + + return &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + Kind: "ConfigMap", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: consts.KatibConfigMapName, + Namespace: consts.DefaultKatibNamespace, + }, + Data: map[string]string{ + consts.LabelSuggestionTag: suggestionConfig, + consts.LabelEarlyStoppingTag: earlyStoppingConfig, + consts.LabelMetricsCollectorSidecar: metricsCollector, + }, + } +} + +func newFakeSuggestionConfig() *SuggestionConfig { + defaultCPURequest, _ := resource.ParseQuantity(consts.DefaultCPURequest) + defaultMemoryRequest, _ := resource.ParseQuantity(consts.DefaultMemRequest) + defaultEphemeralStorageRequest, _ := resource.ParseQuantity(consts.DefaultDiskRequest) + + defaultCPULimit, _ := resource.ParseQuantity(consts.DefaultCPULimit) + defaultMemoryLimit, _ := resource.ParseQuantity(consts.DefaultMemLimit) + defaultEphemeralStorageLimit, _ := resource.ParseQuantity(consts.DefaultDiskLimit) + + defaultVolumeStorage, _ := resource.ParseQuantity(consts.DefaultSuggestionVolumeStorage) + + return &SuggestionConfig{ + Image: "suggestion-image", + ImagePullPolicy: consts.DefaultImagePullPolicy, + Resource: corev1.ResourceRequirements{ + Requests: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceCPU: defaultCPURequest, + corev1.ResourceMemory: defaultMemoryRequest, + corev1.ResourceEphemeralStorage: defaultEphemeralStorageRequest, + }, + Limits: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceCPU: defaultCPULimit, + corev1.ResourceMemory: defaultMemoryLimit, + corev1.ResourceEphemeralStorage: defaultEphemeralStorageLimit, + }, + }, + VolumeMountPath: consts.DefaultContainerSuggestionVolumeMountPath, + PersistentVolumeClaimSpec: corev1.PersistentVolumeClaimSpec{ + AccessModes: []corev1.PersistentVolumeAccessMode{ + consts.DefaultSuggestionVolumeAccessMode, + }, + Resources: corev1.ResourceRequirements{ + Requests: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceStorage: defaultVolumeStorage, + }, + }, + }, + PersistentVolumeSpec: corev1.PersistentVolumeSpec{ + PersistentVolumeReclaimPolicy: corev1.PersistentVolumeReclaimDelete, + }, + } +} + +func newFakeEarlyStoppingConfig() *EarlyStoppingConfig { + return &EarlyStoppingConfig{ + Image: "early-stopping-image", + } +} + +func newFakeMetricsCollectorConfig() *MetricsCollectorConfig { + return &MetricsCollectorConfig{ + Image: "metrics-collector-image", + } +} diff --git a/pkg/webhook/v1beta1/experiment/validator/validator.go b/pkg/webhook/v1beta1/experiment/validator/validator.go index 09a80d6f5d3..a9e01fa4f34 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator.go @@ -359,7 +359,7 @@ func validatePatchJob(runSpec *unstructured.Unstructured, job interface{}, jobTy // Not necessary to check error job must be valid JSON runSpecAfter, _ := json.Marshal(job) - // Create Patch on tranformed Job (e.g: Job) using unstructured JSON + // Create Patch on transformed Job (e.g: Job) using unstructured JSON runSpecPatchOperations, err := jsonPatch.CreatePatch(runSpecAfter, runSpecBefore) if err != nil { return fmt.Errorf("create patch error: %v", err) From fcf4d79aa76b1316eb9575234f4844e83591fb08 Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Sat, 2 Oct 2021 01:49:27 +0900 Subject: [PATCH 2/9] implement unit tests for GetEarlyStoppingConfigData and GetMetricsCollectorConfigData in katib-config --- pkg/util/v1beta1/katibconfig/config_test.go | 269 ++++++++++++++------ 1 file changed, 196 insertions(+), 73 deletions(-) diff --git a/pkg/util/v1beta1/katibconfig/config_test.go b/pkg/util/v1beta1/katibconfig/config_test.go index 142643648c5..0623a4801da 100644 --- a/pkg/util/v1beta1/katibconfig/config_test.go +++ b/pkg/util/v1beta1/katibconfig/config_test.go @@ -20,7 +20,7 @@ import ( type katibConfig struct { suggestion map[string]*SuggestionConfig earlyStopping map[string]*EarlyStoppingConfig - metricsCollector map[string]*MetricsCollectorConfig + metricsCollector map[commonv1beta1.CollectorKind]*MetricsCollectorConfig } func TestGetSuggestionConfigData(t *testing.T) { @@ -35,22 +35,22 @@ func TestGetSuggestionConfigData(t *testing.T) { nonExistentSuggestion bool }{ { - testDescription: "All parameters are specified", + testDescription: "All parameters correctly are specified", katibConfigMapSuggestion: func() *SuggestionConfig { - s := newFakeSuggestionConfig() - s.ImagePullPolicy = corev1.PullAlways - return s + c := newFakeSuggestionConfig() + c.ImagePullPolicy = corev1.PullAlways + return c }(), expected: func() *SuggestionConfig { - s := newFakeSuggestionConfig() - s.ImagePullPolicy = corev1.PullAlways - return s + c := newFakeSuggestionConfig() + c.ImagePullPolicy = corev1.PullAlways + return c }(), inputAlgorithmName: testAlgorithmName, err: false, }, { - testDescription: "There is not the suggestion field in katib-config configMap", + testDescription: fmt.Sprintf("There is not %s field in katib-config configMap", consts.LabelSuggestionTag), err: true, nonExistentSuggestion: true, }, @@ -63,9 +63,9 @@ func TestGetSuggestionConfigData(t *testing.T) { { testDescription: "Image filed is empty in katib-config configMap", katibConfigMapSuggestion: func() *SuggestionConfig { - s := newFakeSuggestionConfig() - s.Image = "" - return s + c := newFakeSuggestionConfig() + c.Image = "" + return c }(), inputAlgorithmName: testAlgorithmName, err: true, @@ -73,9 +73,9 @@ func TestGetSuggestionConfigData(t *testing.T) { { testDescription: fmt.Sprintf("GetSuggestionConfigData sets %s to imagePullPolicy", consts.DefaultImagePullPolicy), katibConfigMapSuggestion: func() *SuggestionConfig { - s := newFakeSuggestionConfig() - s.ImagePullPolicy = "" - return s + c := newFakeSuggestionConfig() + c.ImagePullPolicy = "" + return c }(), expected: newFakeSuggestionConfig(), inputAlgorithmName: testAlgorithmName, @@ -84,9 +84,9 @@ func TestGetSuggestionConfigData(t *testing.T) { { testDescription: "GetSuggestionConfigData sets resource.requests and resource.limits for the suggestion service", katibConfigMapSuggestion: func() *SuggestionConfig { - s := newFakeSuggestionConfig() - s.Resource = corev1.ResourceRequirements{} - return s + c := newFakeSuggestionConfig() + c.Resource = corev1.ResourceRequirements{} + return c }(), expected: newFakeSuggestionConfig(), inputAlgorithmName: testAlgorithmName, @@ -95,9 +95,9 @@ func TestGetSuggestionConfigData(t *testing.T) { { testDescription: fmt.Sprintf("GetSuggestionConfigData sets %s to volumeMountPath", consts.DefaultContainerSuggestionVolumeMountPath), katibConfigMapSuggestion: func() *SuggestionConfig { - s := newFakeSuggestionConfig() - s.VolumeMountPath = "" - return s + c := newFakeSuggestionConfig() + c.VolumeMountPath = "" + return c }(), expected: newFakeSuggestionConfig(), inputAlgorithmName: testAlgorithmName, @@ -106,9 +106,9 @@ func TestGetSuggestionConfigData(t *testing.T) { { testDescription: "GetSuggestionConfigData sets accessMode and resource.requests for PVC", katibConfigMapSuggestion: func() *SuggestionConfig { - s := newFakeSuggestionConfig() - s.PersistentVolumeClaimSpec = corev1.PersistentVolumeClaimSpec{} - return s + c := newFakeSuggestionConfig() + c.PersistentVolumeClaimSpec = corev1.PersistentVolumeClaimSpec{} + return c }(), expected: newFakeSuggestionConfig(), inputAlgorithmName: testAlgorithmName, @@ -117,14 +117,14 @@ func TestGetSuggestionConfigData(t *testing.T) { { testDescription: fmt.Sprintf("GetSuggestionConfigData does not set %s to persistentVolumeReclaimPolicy", corev1.PersistentVolumeReclaimDelete), katibConfigMapSuggestion: func() *SuggestionConfig { - s := newFakeSuggestionConfig() - s.PersistentVolumeSpec = corev1.PersistentVolumeSpec{} - return s + c := newFakeSuggestionConfig() + c.PersistentVolumeSpec = corev1.PersistentVolumeSpec{} + return c }(), expected: func() *SuggestionConfig { - s := newFakeSuggestionConfig() - s.PersistentVolumeSpec = corev1.PersistentVolumeSpec{} - return s + c := newFakeSuggestionConfig() + c.PersistentVolumeSpec = corev1.PersistentVolumeSpec{} + return c }(), inputAlgorithmName: testAlgorithmName, err: false, @@ -167,7 +167,53 @@ func TestGetEarlyStoppingConfigData(t *testing.T) { err bool nonExistentEarlyStopping bool }{ - {}, + { + testDescription: "All parameters correctly are specified", + katibConfigMapEarlyStopping: func() *EarlyStoppingConfig { + c := newFakeEarlyStoppingConfig() + c.ImagePullPolicy = corev1.PullIfNotPresent + return c + }(), + expected: func() *EarlyStoppingConfig { + c := newFakeEarlyStoppingConfig() + c.ImagePullPolicy = corev1.PullIfNotPresent + return c + }(), + inputAlgorithmName: testAlgorithmName, + err: false, + }, + { + testDescription: fmt.Sprintf("There is not %s field in katib-config configMap", consts.LabelEarlyStoppingTag), + err: true, + nonExistentEarlyStopping: true, + }, + { + testDescription: "There is not the AlgorithmName", + katibConfigMapEarlyStopping: newFakeEarlyStoppingConfig(), + inputAlgorithmName: "invalid-algorithm-name", + err: true, + }, + { + testDescription: "Image filed is empty in katib-config configMap", + katibConfigMapEarlyStopping: func() *EarlyStoppingConfig { + c := newFakeEarlyStoppingConfig() + c.Image = "" + return c + }(), + inputAlgorithmName: testAlgorithmName, + err: true, + }, + { + testDescription: fmt.Sprintf("GetEarlyStoppingConfigData sets %s to imagePullPolicy", consts.DefaultImagePullPolicy), + katibConfigMapEarlyStopping: func() *EarlyStoppingConfig { + c := newFakeEarlyStoppingConfig() + c.ImagePullPolicy = "" + return c + }(), + expected: newFakeEarlyStoppingConfig(), + inputAlgorithmName: testAlgorithmName, + err: false, + }, } for _, tt := range tests { @@ -195,17 +241,94 @@ func TestGetEarlyStoppingConfigData(t *testing.T) { } func TestGetMetricsCollectorConfigData(t *testing.T) { - const testMetricsCollector = "test-metrics-collector" + const ( + invalidCollectorKind commonv1beta1.CollectorKind = "invalid-collector-kind" + testCollectorKind commonv1beta1.CollectorKind = "testCollector" + ) + + nukeResource, _ := resource.ParseQuantity("-1") + nukeResourceRequirements := map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceCPU: nukeResource, + corev1.ResourceMemory: nukeResource, + corev1.ResourceEphemeralStorage: nukeResource, + } tests := []struct { testDescription string katibConfigMapMetricsCollector *MetricsCollectorConfig - expected *MetricsCollectorConfig - inputCollectorKind commonv1beta1.CollectorKind - err bool + expected *MetricsCollectorConfig + inputCollectorKind commonv1beta1.CollectorKind + err bool nonExistentMetricsCollector bool }{ - {}, + { + testDescription: "All parameters correctly are specified", + katibConfigMapMetricsCollector: func() *MetricsCollectorConfig { + c := newFakeMetricsCollectorConfig() + c.ImagePullPolicy = corev1.PullNever + return c + }(), + expected: func() *MetricsCollectorConfig { + c := newFakeMetricsCollectorConfig() + c.ImagePullPolicy = corev1.PullNever + return c + }(), + inputCollectorKind: testCollectorKind, + err: false, + }, + { + testDescription: fmt.Sprintf("There is not %s field in katib-config configMap", consts.LabelMetricsCollectorSidecar), + err: true, + nonExistentMetricsCollector: true, + }, + { + testDescription: "There is not the cKind", + katibConfigMapMetricsCollector: newFakeMetricsCollectorConfig(), + inputCollectorKind: invalidCollectorKind, + err: true, + }, + { + testDescription: "Image filed is empty in katib-config configMap", + katibConfigMapMetricsCollector: func() *MetricsCollectorConfig { + c := newFakeMetricsCollectorConfig() + c.Image = "" + return c + }(), + inputCollectorKind: testCollectorKind, + err: true, + }, + { + testDescription: fmt.Sprintf("GetMetricsConfigData sets %s to imagePullPolicy", consts.DefaultImagePullPolicy), + katibConfigMapMetricsCollector: func() *MetricsCollectorConfig { + c := newFakeMetricsCollectorConfig() + c.ImagePullPolicy = "" + return c + }(), + expected: newFakeMetricsCollectorConfig(), + inputCollectorKind: testCollectorKind, + err: false, + }, + { + testDescription: "GetMetricsConfigData nukes resource.requests and resource.limits for the metrics collector", + katibConfigMapMetricsCollector: func() *MetricsCollectorConfig { + c := newFakeMetricsCollectorConfig() + c.Resource = corev1.ResourceRequirements{ + Requests: nukeResourceRequirements, + Limits: nukeResourceRequirements, + } + return c + }(), + expected: func() *MetricsCollectorConfig { + c := newFakeMetricsCollectorConfig() + c.Resource = corev1.ResourceRequirements{ + Requests: map[corev1.ResourceName]resource.Quantity{}, + Limits: map[corev1.ResourceName]resource.Quantity{}, + } + return c + }(), + inputCollectorKind: testCollectorKind, + err: false, + }, } for _, tt := range tests { @@ -213,8 +336,8 @@ func TestGetMetricsCollectorConfigData(t *testing.T) { // prepare test config := &katibConfig{} if !tt.nonExistentMetricsCollector { - config.metricsCollector = map[string]*MetricsCollectorConfig{ - testMetricsCollector: tt.katibConfigMapMetricsCollector, + config.metricsCollector = map[commonv1beta1.CollectorKind]*MetricsCollectorConfig{ + testCollectorKind: tt.katibConfigMapMetricsCollector, } } c := newFakeKubeClient(newFakeKatibConfigMap(config)) @@ -225,6 +348,7 @@ func TestGetMetricsCollectorConfigData(t *testing.T) { t.Errorf("want error: %v, actual: %v", tt.err, err) } else if tt.expected != nil { if !reflect.DeepEqual(actual, *tt.expected) { + //t.Errorf("%p, %p", &actual, tt.expected) t.Errorf("Generated MetricsCollectorConfig is invalid.\n\nactual:\n%v\n\nexpected:\n%v\n\n", actual, *tt.expected) } } @@ -242,22 +366,18 @@ func newFakeKubeClient(katibConfigMap *corev1.ConfigMap) client.Client { func newFakeKatibConfigMap(config *katibConfig) *corev1.ConfigMap { - suggestionConfig := "" + data := map[string]string{} if config.suggestion != nil { bSuggestionConfig, _ := json.Marshal(config.suggestion) - suggestionConfig = string(bSuggestionConfig) + data[consts.LabelSuggestionTag] = string(bSuggestionConfig) } - - earlyStoppingConfig := "" if config.earlyStopping != nil { bEarlyStoppingConfig, _ := json.Marshal(config.earlyStopping) - earlyStoppingConfig = string(bEarlyStoppingConfig) + data[consts.LabelEarlyStoppingTag] = string(bEarlyStoppingConfig) } - - metricsCollector := "" if config.metricsCollector != nil { bMetricsCollector, _ := json.Marshal(config.metricsCollector) - metricsCollector = string(bMetricsCollector) + data[consts.LabelMetricsCollectorSidecar] = string(bMetricsCollector) } return &corev1.ConfigMap{ @@ -269,40 +389,17 @@ func newFakeKatibConfigMap(config *katibConfig) *corev1.ConfigMap { Name: consts.KatibConfigMapName, Namespace: consts.DefaultKatibNamespace, }, - Data: map[string]string{ - consts.LabelSuggestionTag: suggestionConfig, - consts.LabelEarlyStoppingTag: earlyStoppingConfig, - consts.LabelMetricsCollectorSidecar: metricsCollector, - }, + Data: data, } } func newFakeSuggestionConfig() *SuggestionConfig { - defaultCPURequest, _ := resource.ParseQuantity(consts.DefaultCPURequest) - defaultMemoryRequest, _ := resource.ParseQuantity(consts.DefaultMemRequest) - defaultEphemeralStorageRequest, _ := resource.ParseQuantity(consts.DefaultDiskRequest) - - defaultCPULimit, _ := resource.ParseQuantity(consts.DefaultCPULimit) - defaultMemoryLimit, _ := resource.ParseQuantity(consts.DefaultMemLimit) - defaultEphemeralStorageLimit, _ := resource.ParseQuantity(consts.DefaultDiskLimit) - defaultVolumeStorage, _ := resource.ParseQuantity(consts.DefaultSuggestionVolumeStorage) return &SuggestionConfig{ Image: "suggestion-image", ImagePullPolicy: consts.DefaultImagePullPolicy, - Resource: corev1.ResourceRequirements{ - Requests: map[corev1.ResourceName]resource.Quantity{ - corev1.ResourceCPU: defaultCPURequest, - corev1.ResourceMemory: defaultMemoryRequest, - corev1.ResourceEphemeralStorage: defaultEphemeralStorageRequest, - }, - Limits: map[corev1.ResourceName]resource.Quantity{ - corev1.ResourceCPU: defaultCPULimit, - corev1.ResourceMemory: defaultMemoryLimit, - corev1.ResourceEphemeralStorage: defaultEphemeralStorageLimit, - }, - }, + Resource: *setFakeResourceRequirements(), VolumeMountPath: consts.DefaultContainerSuggestionVolumeMountPath, PersistentVolumeClaimSpec: corev1.PersistentVolumeClaimSpec{ AccessModes: []corev1.PersistentVolumeAccessMode{ @@ -322,12 +419,38 @@ func newFakeSuggestionConfig() *SuggestionConfig { func newFakeEarlyStoppingConfig() *EarlyStoppingConfig { return &EarlyStoppingConfig{ - Image: "early-stopping-image", + Image: "early-stopping-image", + ImagePullPolicy: consts.DefaultImagePullPolicy, } } func newFakeMetricsCollectorConfig() *MetricsCollectorConfig { return &MetricsCollectorConfig{ - Image: "metrics-collector-image", + Image: "metrics-collector-image", + ImagePullPolicy: consts.DefaultImagePullPolicy, + Resource: *setFakeResourceRequirements(), + } +} + +func setFakeResourceRequirements() *corev1.ResourceRequirements { + defaultCPURequest, _ := resource.ParseQuantity(consts.DefaultCPURequest) + defaultMemoryRequest, _ := resource.ParseQuantity(consts.DefaultMemRequest) + defaultEphemeralStorageRequest, _ := resource.ParseQuantity(consts.DefaultDiskRequest) + + defaultCPULimit, _ := resource.ParseQuantity(consts.DefaultCPULimit) + defaultMemoryLimit, _ := resource.ParseQuantity(consts.DefaultMemLimit) + defaultEphemeralStorageLimit, _ := resource.ParseQuantity(consts.DefaultDiskLimit) + + return &corev1.ResourceRequirements{ + Requests: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceCPU: defaultCPURequest, + corev1.ResourceMemory: defaultMemoryRequest, + corev1.ResourceEphemeralStorage: defaultEphemeralStorageRequest, + }, + Limits: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceCPU: defaultCPULimit, + corev1.ResourceMemory: defaultMemoryLimit, + corev1.ResourceEphemeralStorage: defaultEphemeralStorageLimit, + }, } } From c8059cba5bc5e2d8de9ec9d7e43a890d57df2af1 Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Sat, 2 Oct 2021 02:05:58 +0900 Subject: [PATCH 3/9] fix envtest for suggestion-controller --- .../suggestion/suggestion_controller_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/controller.v1beta1/suggestion/suggestion_controller_test.go b/pkg/controller.v1beta1/suggestion/suggestion_controller_test.go index 8a3c888e37a..4e29636e5ef 100644 --- a/pkg/controller.v1beta1/suggestion/suggestion_controller_test.go +++ b/pkg/controller.v1beta1/suggestion/suggestion_controller_test.go @@ -144,13 +144,12 @@ func TestReconcile(t *testing.T) { suggestionDeploy := &appsv1.Deployment{} // Expect that deployment with appropriate name and image is created - g.Eventually(func() bool { + g.Eventually(func() string { if err = c.Get(context.TODO(), types.NamespacedName{Namespace: namespace, Name: resourceName}, suggestionDeploy); err != nil { - return false + return "" } - return len(suggestionDeploy.Spec.Template.Spec.Containers) > 0 && - suggestionDeploy.Spec.Template.Spec.Containers[0].Image == suggestionImage - }, timeout).Should(gomega.BeTrue()) + return suggestionDeploy.Spec.Template.Spec.Containers[0].Image + }, timeout).Should(gomega.Equal(suggestionImage)) // Expect that service with appropriate name is created g.Eventually(func() error { @@ -302,7 +301,7 @@ func newKatibConfigMapInstance() *corev1.ConfigMap { // Create suggestion config suggestionConfig := map[string]katibconfig.SuggestionConfig{ "random": { - Image: suggestionName, + Image: suggestionImage, }, } bSuggestionConfig, _ := json.Marshal(suggestionConfig) From 0e4303a261d501f09a7bf9aff815ffe6f154a3c0 Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Sat, 2 Oct 2021 02:58:53 +0900 Subject: [PATCH 4/9] remove debug code --- pkg/util/v1beta1/katibconfig/config_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/util/v1beta1/katibconfig/config_test.go b/pkg/util/v1beta1/katibconfig/config_test.go index 0623a4801da..62f53838abd 100644 --- a/pkg/util/v1beta1/katibconfig/config_test.go +++ b/pkg/util/v1beta1/katibconfig/config_test.go @@ -348,7 +348,6 @@ func TestGetMetricsCollectorConfigData(t *testing.T) { t.Errorf("want error: %v, actual: %v", tt.err, err) } else if tt.expected != nil { if !reflect.DeepEqual(actual, *tt.expected) { - //t.Errorf("%p, %p", &actual, tt.expected) t.Errorf("Generated MetricsCollectorConfig is invalid.\n\nactual:\n%v\n\nexpected:\n%v\n\n", actual, *tt.expected) } } From 3133462446e4a37cac39e43dfd683dd1e5e01f28 Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Sat, 2 Oct 2021 05:21:51 +0900 Subject: [PATCH 5/9] fix invalidCollectorKind value --- pkg/util/v1beta1/katibconfig/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/util/v1beta1/katibconfig/config_test.go b/pkg/util/v1beta1/katibconfig/config_test.go index 62f53838abd..683009f96d1 100644 --- a/pkg/util/v1beta1/katibconfig/config_test.go +++ b/pkg/util/v1beta1/katibconfig/config_test.go @@ -242,7 +242,7 @@ func TestGetEarlyStoppingConfigData(t *testing.T) { func TestGetMetricsCollectorConfigData(t *testing.T) { const ( - invalidCollectorKind commonv1beta1.CollectorKind = "invalid-collector-kind" + invalidCollectorKind commonv1beta1.CollectorKind = "invalidCollector" testCollectorKind commonv1beta1.CollectorKind = "testCollector" ) From e9b2607e825caa2f88a9d530ef9278b733f7b507 Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Sun, 3 Oct 2021 09:50:17 +0900 Subject: [PATCH 6/9] refactor tests struct --- pkg/util/v1beta1/katibconfig/config_test.go | 245 ++++++++++---------- 1 file changed, 117 insertions(+), 128 deletions(-) diff --git a/pkg/util/v1beta1/katibconfig/config_test.go b/pkg/util/v1beta1/katibconfig/config_test.go index 683009f96d1..525ce250ef0 100644 --- a/pkg/util/v1beta1/katibconfig/config_test.go +++ b/pkg/util/v1beta1/katibconfig/config_test.go @@ -27,19 +27,18 @@ func TestGetSuggestionConfigData(t *testing.T) { const testAlgorithmName = "test-suggestion" tests := []struct { - testDescription string - katibConfigMapSuggestion *SuggestionConfig - expected *SuggestionConfig - inputAlgorithmName string - err bool - nonExistentSuggestion bool + testDescription string + katibConfig *katibConfig + expected *SuggestionConfig + inputAlgorithmName string + err bool }{ { testDescription: "All parameters correctly are specified", - katibConfigMapSuggestion: func() *SuggestionConfig { - c := newFakeSuggestionConfig() - c.ImagePullPolicy = corev1.PullAlways - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}} + kc.suggestion[testAlgorithmName].ImagePullPolicy = corev1.PullAlways + return kc }(), expected: func() *SuggestionConfig { c := newFakeSuggestionConfig() @@ -50,32 +49,37 @@ func TestGetSuggestionConfigData(t *testing.T) { err: false, }, { - testDescription: fmt.Sprintf("There is not %s field in katib-config configMap", consts.LabelSuggestionTag), - err: true, - nonExistentSuggestion: true, + testDescription: "There is not katib-config.", + katibConfig: nil, + err: true, }, { - testDescription: "There is not the AlgorithmName", - katibConfigMapSuggestion: newFakeSuggestionConfig(), - inputAlgorithmName: "invalid-algorithm-name", - err: true, + testDescription: fmt.Sprintf("There is not %s field in katib-config configMap", consts.LabelSuggestionTag), + katibConfig: &katibConfig{}, + err: true, + }, + { + testDescription: "There is not the AlgorithmName", + katibConfig: &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}}, + inputAlgorithmName: "invalid-algorithm-name", + err: true, }, { testDescription: "Image filed is empty in katib-config configMap", - katibConfigMapSuggestion: func() *SuggestionConfig { - c := newFakeSuggestionConfig() - c.Image = "" - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}} + kc.suggestion[testAlgorithmName].Image = "" + return kc }(), inputAlgorithmName: testAlgorithmName, err: true, }, { testDescription: fmt.Sprintf("GetSuggestionConfigData sets %s to imagePullPolicy", consts.DefaultImagePullPolicy), - katibConfigMapSuggestion: func() *SuggestionConfig { - c := newFakeSuggestionConfig() - c.ImagePullPolicy = "" - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}} + kc.suggestion[testAlgorithmName].ImagePullPolicy = "" + return kc }(), expected: newFakeSuggestionConfig(), inputAlgorithmName: testAlgorithmName, @@ -83,10 +87,10 @@ func TestGetSuggestionConfigData(t *testing.T) { }, { testDescription: "GetSuggestionConfigData sets resource.requests and resource.limits for the suggestion service", - katibConfigMapSuggestion: func() *SuggestionConfig { - c := newFakeSuggestionConfig() - c.Resource = corev1.ResourceRequirements{} - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}} + kc.suggestion[testAlgorithmName].Resource = corev1.ResourceRequirements{} + return kc }(), expected: newFakeSuggestionConfig(), inputAlgorithmName: testAlgorithmName, @@ -94,10 +98,10 @@ func TestGetSuggestionConfigData(t *testing.T) { }, { testDescription: fmt.Sprintf("GetSuggestionConfigData sets %s to volumeMountPath", consts.DefaultContainerSuggestionVolumeMountPath), - katibConfigMapSuggestion: func() *SuggestionConfig { - c := newFakeSuggestionConfig() - c.VolumeMountPath = "" - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}} + kc.suggestion[testAlgorithmName].VolumeMountPath = "" + return kc }(), expected: newFakeSuggestionConfig(), inputAlgorithmName: testAlgorithmName, @@ -105,10 +109,10 @@ func TestGetSuggestionConfigData(t *testing.T) { }, { testDescription: "GetSuggestionConfigData sets accessMode and resource.requests for PVC", - katibConfigMapSuggestion: func() *SuggestionConfig { - c := newFakeSuggestionConfig() - c.PersistentVolumeClaimSpec = corev1.PersistentVolumeClaimSpec{} - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}} + kc.suggestion[testAlgorithmName].PersistentVolumeClaimSpec = corev1.PersistentVolumeClaimSpec{} + return kc }(), expected: newFakeSuggestionConfig(), inputAlgorithmName: testAlgorithmName, @@ -116,10 +120,10 @@ func TestGetSuggestionConfigData(t *testing.T) { }, { testDescription: fmt.Sprintf("GetSuggestionConfigData does not set %s to persistentVolumeReclaimPolicy", corev1.PersistentVolumeReclaimDelete), - katibConfigMapSuggestion: func() *SuggestionConfig { - c := newFakeSuggestionConfig() - c.PersistentVolumeSpec = corev1.PersistentVolumeSpec{} - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}} + kc.suggestion[testAlgorithmName].PersistentVolumeSpec = corev1.PersistentVolumeSpec{} + return kc }(), expected: func() *SuggestionConfig { c := newFakeSuggestionConfig() @@ -133,17 +137,8 @@ func TestGetSuggestionConfigData(t *testing.T) { for _, tt := range tests { t.Run(tt.testDescription, func(t *testing.T) { - // prepare test - config := &katibConfig{} - if !tt.nonExistentSuggestion { - config.suggestion = map[string]*SuggestionConfig{ - testAlgorithmName: tt.katibConfigMapSuggestion, - } - } - c := newFakeKubeClient(newFakeKatibConfigMap(config)) - - // start test - actual, err := GetSuggestionConfigData(tt.inputAlgorithmName, c) + fakeKubeClient := newFakeKubeClient(newFakeKatibConfigMap(tt.katibConfig)) + actual, err := GetSuggestionConfigData(tt.inputAlgorithmName, fakeKubeClient) if (err != nil) != tt.err { t.Errorf("want error: %v, actual: %v", tt.err, err) } else if tt.expected != nil { @@ -160,19 +155,18 @@ func TestGetEarlyStoppingConfigData(t *testing.T) { const testAlgorithmName = "test-early-stopping" tests := []struct { - testDescription string - katibConfigMapEarlyStopping *EarlyStoppingConfig - expected *EarlyStoppingConfig - inputAlgorithmName string - err bool - nonExistentEarlyStopping bool + testDescription string + katibConfig *katibConfig + expected *EarlyStoppingConfig + inputAlgorithmName string + err bool }{ { testDescription: "All parameters correctly are specified", - katibConfigMapEarlyStopping: func() *EarlyStoppingConfig { - c := newFakeEarlyStoppingConfig() - c.ImagePullPolicy = corev1.PullIfNotPresent - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testAlgorithmName: newFakeEarlyStoppingConfig()}} + kc.earlyStopping[testAlgorithmName].ImagePullPolicy = corev1.PullIfNotPresent + return kc }(), expected: func() *EarlyStoppingConfig { c := newFakeEarlyStoppingConfig() @@ -183,32 +177,37 @@ func TestGetEarlyStoppingConfigData(t *testing.T) { err: false, }, { - testDescription: fmt.Sprintf("There is not %s field in katib-config configMap", consts.LabelEarlyStoppingTag), - err: true, - nonExistentEarlyStopping: true, + testDescription: "There is not katib-config.", + katibConfig: nil, + err: true, }, { - testDescription: "There is not the AlgorithmName", - katibConfigMapEarlyStopping: newFakeEarlyStoppingConfig(), - inputAlgorithmName: "invalid-algorithm-name", - err: true, + testDescription: fmt.Sprintf("There is not %s field in katib-config configMap", consts.LabelEarlyStoppingTag), + katibConfig: &katibConfig{}, + err: true, + }, + { + testDescription: "There is not the AlgorithmName", + katibConfig: &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testAlgorithmName: newFakeEarlyStoppingConfig()}}, + inputAlgorithmName: "invalid-algorithm-name", + err: true, }, { testDescription: "Image filed is empty in katib-config configMap", - katibConfigMapEarlyStopping: func() *EarlyStoppingConfig { - c := newFakeEarlyStoppingConfig() - c.Image = "" - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testAlgorithmName: newFakeEarlyStoppingConfig()}} + kc.earlyStopping[testAlgorithmName].Image = "" + return kc }(), inputAlgorithmName: testAlgorithmName, err: true, }, { testDescription: fmt.Sprintf("GetEarlyStoppingConfigData sets %s to imagePullPolicy", consts.DefaultImagePullPolicy), - katibConfigMapEarlyStopping: func() *EarlyStoppingConfig { - c := newFakeEarlyStoppingConfig() - c.ImagePullPolicy = "" - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testAlgorithmName: newFakeEarlyStoppingConfig()}} + kc.earlyStopping[testAlgorithmName].ImagePullPolicy = "" + return kc }(), expected: newFakeEarlyStoppingConfig(), inputAlgorithmName: testAlgorithmName, @@ -218,17 +217,8 @@ func TestGetEarlyStoppingConfigData(t *testing.T) { for _, tt := range tests { t.Run(tt.testDescription, func(t *testing.T) { - // prepare test - config := &katibConfig{} - if !tt.nonExistentEarlyStopping { - config.earlyStopping = map[string]*EarlyStoppingConfig{ - testAlgorithmName: tt.katibConfigMapEarlyStopping, - } - } - c := newFakeKubeClient(newFakeKatibConfigMap(config)) - - // start test - actual, err := GetEarlyStoppingConfigData(tt.inputAlgorithmName, c) + fakeKubeClient := newFakeKubeClient(newFakeKatibConfigMap(tt.katibConfig)) + actual, err := GetEarlyStoppingConfigData(tt.inputAlgorithmName, fakeKubeClient) if (err != nil) != tt.err { t.Errorf("want error: %v, actual: %v", tt.err, err) } else if tt.expected != nil { @@ -254,19 +244,18 @@ func TestGetMetricsCollectorConfigData(t *testing.T) { } tests := []struct { - testDescription string - katibConfigMapMetricsCollector *MetricsCollectorConfig - expected *MetricsCollectorConfig - inputCollectorKind commonv1beta1.CollectorKind - err bool - nonExistentMetricsCollector bool + testDescription string + katibConfig *katibConfig + expected *MetricsCollectorConfig + inputCollectorKind commonv1beta1.CollectorKind + err bool }{ { testDescription: "All parameters correctly are specified", - katibConfigMapMetricsCollector: func() *MetricsCollectorConfig { - c := newFakeMetricsCollectorConfig() - c.ImagePullPolicy = corev1.PullNever - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{metricsCollector: map[commonv1beta1.CollectorKind]*MetricsCollectorConfig{testCollectorKind: newFakeMetricsCollectorConfig()}} + kc.metricsCollector[testCollectorKind].ImagePullPolicy = corev1.PullNever + return kc }(), expected: func() *MetricsCollectorConfig { c := newFakeMetricsCollectorConfig() @@ -277,32 +266,37 @@ func TestGetMetricsCollectorConfigData(t *testing.T) { err: false, }, { - testDescription: fmt.Sprintf("There is not %s field in katib-config configMap", consts.LabelMetricsCollectorSidecar), - err: true, - nonExistentMetricsCollector: true, + testDescription: "There is not katib-config.", + katibConfig: nil, + err: true, + }, + { + testDescription: fmt.Sprintf("There is not %s field in katib-config configMap", consts.LabelMetricsCollectorSidecar), + katibConfig: &katibConfig{}, + err: true, }, { - testDescription: "There is not the cKind", - katibConfigMapMetricsCollector: newFakeMetricsCollectorConfig(), - inputCollectorKind: invalidCollectorKind, - err: true, + testDescription: "There is not the cKind", + katibConfig: &katibConfig{metricsCollector: map[commonv1beta1.CollectorKind]*MetricsCollectorConfig{testCollectorKind: newFakeMetricsCollectorConfig()}}, + inputCollectorKind: invalidCollectorKind, + err: true, }, { testDescription: "Image filed is empty in katib-config configMap", - katibConfigMapMetricsCollector: func() *MetricsCollectorConfig { - c := newFakeMetricsCollectorConfig() - c.Image = "" - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{metricsCollector: map[commonv1beta1.CollectorKind]*MetricsCollectorConfig{testCollectorKind: newFakeMetricsCollectorConfig()}} + kc.metricsCollector[testCollectorKind].Image = "" + return kc }(), inputCollectorKind: testCollectorKind, err: true, }, { testDescription: fmt.Sprintf("GetMetricsConfigData sets %s to imagePullPolicy", consts.DefaultImagePullPolicy), - katibConfigMapMetricsCollector: func() *MetricsCollectorConfig { - c := newFakeMetricsCollectorConfig() - c.ImagePullPolicy = "" - return c + katibConfig: func() *katibConfig { + kc := &katibConfig{metricsCollector: map[commonv1beta1.CollectorKind]*MetricsCollectorConfig{testCollectorKind: newFakeMetricsCollectorConfig()}} + kc.metricsCollector[testCollectorKind].ImagePullPolicy = "" + return kc }(), expected: newFakeMetricsCollectorConfig(), inputCollectorKind: testCollectorKind, @@ -310,13 +304,13 @@ func TestGetMetricsCollectorConfigData(t *testing.T) { }, { testDescription: "GetMetricsConfigData nukes resource.requests and resource.limits for the metrics collector", - katibConfigMapMetricsCollector: func() *MetricsCollectorConfig { - c := newFakeMetricsCollectorConfig() - c.Resource = corev1.ResourceRequirements{ + katibConfig: func() *katibConfig { + kc := &katibConfig{metricsCollector: map[commonv1beta1.CollectorKind]*MetricsCollectorConfig{testCollectorKind: newFakeMetricsCollectorConfig()}} + kc.metricsCollector[testCollectorKind].Resource = corev1.ResourceRequirements{ Requests: nukeResourceRequirements, Limits: nukeResourceRequirements, } - return c + return kc }(), expected: func() *MetricsCollectorConfig { c := newFakeMetricsCollectorConfig() @@ -333,17 +327,8 @@ func TestGetMetricsCollectorConfigData(t *testing.T) { for _, tt := range tests { t.Run(tt.testDescription, func(t *testing.T) { - // prepare test - config := &katibConfig{} - if !tt.nonExistentMetricsCollector { - config.metricsCollector = map[commonv1beta1.CollectorKind]*MetricsCollectorConfig{ - testCollectorKind: tt.katibConfigMapMetricsCollector, - } - } - c := newFakeKubeClient(newFakeKatibConfigMap(config)) - - // start test - actual, err := GetMetricsCollectorConfigData(tt.inputCollectorKind, c) + fakeKubeClient := newFakeKubeClient(newFakeKatibConfigMap(tt.katibConfig)) + actual, err := GetMetricsCollectorConfigData(tt.inputCollectorKind, fakeKubeClient) if (err != nil) != tt.err { t.Errorf("want error: %v, actual: %v", tt.err, err) } else if tt.expected != nil { @@ -365,6 +350,10 @@ func newFakeKubeClient(katibConfigMap *corev1.ConfigMap) client.Client { func newFakeKatibConfigMap(config *katibConfig) *corev1.ConfigMap { + if config == nil { + return nil + } + data := map[string]string{} if config.suggestion != nil { bSuggestionConfig, _ := json.Marshal(config.suggestion) From c51a77a375b2fac481e62046a078fcd40b295caa Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Sun, 3 Oct 2021 10:00:59 +0900 Subject: [PATCH 7/9] remove unnecessary empty line --- pkg/util/v1beta1/katibconfig/config_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/util/v1beta1/katibconfig/config_test.go b/pkg/util/v1beta1/katibconfig/config_test.go index 525ce250ef0..9c39cb94bec 100644 --- a/pkg/util/v1beta1/katibconfig/config_test.go +++ b/pkg/util/v1beta1/katibconfig/config_test.go @@ -349,7 +349,6 @@ func newFakeKubeClient(katibConfigMap *corev1.ConfigMap) client.Client { } func newFakeKatibConfigMap(config *katibConfig) *corev1.ConfigMap { - if config == nil { return nil } From 355f47153a9aa6f13473dfb2d20c273366008e00 Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Wed, 6 Oct 2021 22:27:47 +0900 Subject: [PATCH 8/9] add tests for custom resource requirements --- pkg/util/v1beta1/katibconfig/config_test.go | 25 +++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/pkg/util/v1beta1/katibconfig/config_test.go b/pkg/util/v1beta1/katibconfig/config_test.go index 9c39cb94bec..1a8928cd7e7 100644 --- a/pkg/util/v1beta1/katibconfig/config_test.go +++ b/pkg/util/v1beta1/katibconfig/config_test.go @@ -38,11 +38,13 @@ func TestGetSuggestionConfigData(t *testing.T) { katibConfig: func() *katibConfig { kc := &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}} kc.suggestion[testAlgorithmName].ImagePullPolicy = corev1.PullAlways + kc.suggestion[testAlgorithmName].Resource = *newFakeCustomResourceRequirements() return kc }(), expected: func() *SuggestionConfig { c := newFakeSuggestionConfig() c.ImagePullPolicy = corev1.PullAlways + c.Resource = *newFakeCustomResourceRequirements() return c }(), inputAlgorithmName: testAlgorithmName, @@ -441,3 +443,26 @@ func setFakeResourceRequirements() *corev1.ResourceRequirements { }, } } + +func newFakeCustomResourceRequirements() *corev1.ResourceRequirements { + defaultCPURequest, _ := resource.ParseQuantity("25m") + defaultMemoryRequest, _ := resource.ParseQuantity("200Mi") + defaultEphemeralStorageRequest, _ := resource.ParseQuantity("550Mi") + + defaultCPULimit, _ := resource.ParseQuantity("250m") + defaultMemoryLimit, _ := resource.ParseQuantity("2Gi") + defaultEphemeralStorageLimit, _ := resource.ParseQuantity("15Gi") + + return &corev1.ResourceRequirements{ + Requests: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceCPU: defaultCPURequest, + corev1.ResourceMemory: defaultMemoryRequest, + corev1.ResourceEphemeralStorage: defaultEphemeralStorageRequest, + }, + Limits: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceCPU: defaultCPULimit, + corev1.ResourceMemory: defaultMemoryLimit, + corev1.ResourceEphemeralStorage: defaultEphemeralStorageLimit, + }, + } +} From 4c04435a3a27b2c703ca0c12859f9d4084a1a78a Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Wed, 6 Oct 2021 23:12:33 +0900 Subject: [PATCH 9/9] fix variable name --- pkg/util/v1beta1/katibconfig/config_test.go | 24 ++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/util/v1beta1/katibconfig/config_test.go b/pkg/util/v1beta1/katibconfig/config_test.go index 1a8928cd7e7..58707e1c44e 100644 --- a/pkg/util/v1beta1/katibconfig/config_test.go +++ b/pkg/util/v1beta1/katibconfig/config_test.go @@ -445,24 +445,24 @@ func setFakeResourceRequirements() *corev1.ResourceRequirements { } func newFakeCustomResourceRequirements() *corev1.ResourceRequirements { - defaultCPURequest, _ := resource.ParseQuantity("25m") - defaultMemoryRequest, _ := resource.ParseQuantity("200Mi") - defaultEphemeralStorageRequest, _ := resource.ParseQuantity("550Mi") + customCPURequest, _ := resource.ParseQuantity("25m") + customMemoryRequest, _ := resource.ParseQuantity("200Mi") + customEphemeralStorageRequest, _ := resource.ParseQuantity("550Mi") - defaultCPULimit, _ := resource.ParseQuantity("250m") - defaultMemoryLimit, _ := resource.ParseQuantity("2Gi") - defaultEphemeralStorageLimit, _ := resource.ParseQuantity("15Gi") + customCPULimit, _ := resource.ParseQuantity("250m") + customMemoryLimit, _ := resource.ParseQuantity("2Gi") + customEphemeralStorageLimit, _ := resource.ParseQuantity("15Gi") return &corev1.ResourceRequirements{ Requests: map[corev1.ResourceName]resource.Quantity{ - corev1.ResourceCPU: defaultCPURequest, - corev1.ResourceMemory: defaultMemoryRequest, - corev1.ResourceEphemeralStorage: defaultEphemeralStorageRequest, + corev1.ResourceCPU: customCPURequest, + corev1.ResourceMemory: customMemoryRequest, + corev1.ResourceEphemeralStorage: customEphemeralStorageRequest, }, Limits: map[corev1.ResourceName]resource.Quantity{ - corev1.ResourceCPU: defaultCPULimit, - corev1.ResourceMemory: defaultMemoryLimit, - corev1.ResourceEphemeralStorage: defaultEphemeralStorageLimit, + corev1.ResourceCPU: customCPULimit, + corev1.ResourceMemory: customMemoryLimit, + corev1.ResourceEphemeralStorage: customEphemeralStorageLimit, }, } }