From 8a261276b6055b3acea30b714dac2a56941353f2 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Sun, 23 Jun 2024 13:41:56 +0800
Subject: [PATCH 01/13] chore: add metrics_collector_config in tune function.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
.../v1beta1/kubeflow/katib/api/katib_client.py | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py b/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
index 7988dbaa898..9c1593f31ee 100644
--- a/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
+++ b/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
@@ -186,6 +186,7 @@ def tune(
retain_trials: bool = False,
packages_to_install: List[str] = None,
pip_index_url: str = "https://pypi.org/simple",
+ metrics_collector_config: Dict[str, Any] = {"kind": "StdOut"},
):
"""Create HyperParameter Tuning Katib Experiment from the objective function.
@@ -248,6 +249,8 @@ def tune(
to the base image packages. These packages are installed before
executing the objective function.
pip_index_url: The PyPI url from which to install Python packages.
+ metrics_collector_config: Specify the config of metrics collector,
+ for example, `metrics_collector_config = {"kind": "Push"}`.
Raises:
ValueError: Function arguments have incorrect type or value.
@@ -380,6 +383,19 @@ def tune(
f"Incorrect value for env_per_trial: {env_per_trial}"
)
+ # Add metrics collector to the Katib Experiment.
+ # Up to now, We only support parameter `kind`, of which default value is `StdOut`, to specify the kind of metrics collector.
+ experiment.spec.metrics_collector = models.V1beta1MetricsCollectorSpec(
+ collector=models.V1beta1CollectorSpec(kind=metrics_collector_config["kind"])
+ )
+ if metrics_collector_config["kind"] == "Push":
+ trial_params.append(
+ models.V1beta1TrialParameterSpec(name="trialName", reference="${{trialSpec.Name}}")
+ )
+ env.append(
+ client.V1EnvVar(name="KATIB_TRIAL_NAME", value="${{trialParameters.trialName}}")
+ )
+
# Create Trial specification.
trial_spec = client.V1Job(
api_version="batch/v1",
From 67b4af61457d14aa9326ee3cedee0ff6ecd12599 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Sun, 23 Jun 2024 13:52:09 +0800
Subject: [PATCH 02/13] rebase: rebase feat/new-param-tune to master.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
pkg/apis/controller/common/v1beta1/common_types.go | 4 ++--
pkg/webhook/v1beta1/experiment/validator/validator.go | 4 ++--
pkg/webhook/v1beta1/pod/inject_webhook.go | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/pkg/apis/controller/common/v1beta1/common_types.go b/pkg/apis/controller/common/v1beta1/common_types.go
index 8722e8a474d..251f8887042 100644
--- a/pkg/apis/controller/common/v1beta1/common_types.go
+++ b/pkg/apis/controller/common/v1beta1/common_types.go
@@ -220,8 +220,8 @@ const (
CustomCollector CollectorKind = "Custom"
// When model training source code persists metrics into persistent layer
- // directly, metricsCollector isn't in need, and its kind is "noneCollector"
- NoneCollector CollectorKind = "None"
+ // directly, sidecar container isn't in need, and its kind is "pushCollector"
+ PushCollector CollectorKind = "Push"
MetricsVolume = "metrics-volume"
)
diff --git a/pkg/webhook/v1beta1/experiment/validator/validator.go b/pkg/webhook/v1beta1/experiment/validator/validator.go
index aeabbc0f463..cf865fe529e 100644
--- a/pkg/webhook/v1beta1/experiment/validator/validator.go
+++ b/pkg/webhook/v1beta1/experiment/validator/validator.go
@@ -488,8 +488,8 @@ func (g *DefaultValidator) validateMetricsCollector(inst *experimentsv1beta1.Exp
}
// TODO(hougangliu): log warning message if some field will not be used for the metricsCollector kind
switch mcKind {
- case commonapiv1beta1.NoneCollector, commonapiv1beta1.StdOutCollector:
- return allErrs
+ case commonapiv1beta1.PushCollector, commonapiv1beta1.StdOutCollector:
+ return nil
case commonapiv1beta1.FileCollector:
if mcSpec.Source == nil || mcSpec.Source.FileSystemPath == nil ||
mcSpec.Source.FileSystemPath.Kind != commonapiv1beta1.FileKind || !filepath.IsAbs(mcSpec.Source.FileSystemPath.Path) {
diff --git a/pkg/webhook/v1beta1/pod/inject_webhook.go b/pkg/webhook/v1beta1/pod/inject_webhook.go
index f48b4b96a07..e7ab14029bb 100644
--- a/pkg/webhook/v1beta1/pod/inject_webhook.go
+++ b/pkg/webhook/v1beta1/pod/inject_webhook.go
@@ -147,8 +147,8 @@ func (s *SidecarInjector) Mutate(pod *v1.Pod, namespace string) (*v1.Pod, error)
return mutatedPod, nil
}
- // If Metrics Collector in None, skip the mutation.
- if trial.Spec.MetricsCollector.Collector.Kind == common.NoneCollector {
+ // If Metrics Collector is Push, skip the mutation.
+ if trial.Spec.MetricsCollector.Collector.Kind == common.PushCollector {
return mutatedPod, nil
}
From 2db6fcd56f5fede7d9b40eb5db80649384554821 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Mon, 24 Jun 2024 23:09:18 +0800
Subject: [PATCH 03/13] chore: add metrics collector kind list in comment.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
sdk/python/v1beta1/kubeflow/katib/api/katib_client.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py b/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
index 9c1593f31ee..b153fe3f9a6 100644
--- a/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
+++ b/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
@@ -251,6 +251,7 @@ def tune(
pip_index_url: The PyPI url from which to install Python packages.
metrics_collector_config: Specify the config of metrics collector,
for example, `metrics_collector_config = {"kind": "Push"}`.
+ Currently, we only support `StdOut` and `Push` metrics collector.
Raises:
ValueError: Function arguments have incorrect type or value.
From 02e124b10145e5c67819e7ccf0ebc509254b1a20 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Sat, 6 Jul 2024 05:04:38 +0000
Subject: [PATCH 04/13] fix: always pass Trial name to the training container.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
.../v1beta1/kubeflow/katib/api/katib_client.py | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py b/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
index b153fe3f9a6..55aeca58987 100644
--- a/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
+++ b/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
@@ -190,6 +190,8 @@ def tune(
):
"""Create HyperParameter Tuning Katib Experiment from the objective function.
+ Katib always passes Trial name as env variable `KATIB_TRIAL_NAME` to the training container.
+
Args:
name: Name for the Experiment.
objective: Objective function that Katib uses to train the model.
@@ -389,13 +391,14 @@ def tune(
experiment.spec.metrics_collector = models.V1beta1MetricsCollectorSpec(
collector=models.V1beta1CollectorSpec(kind=metrics_collector_config["kind"])
)
- if metrics_collector_config["kind"] == "Push":
- trial_params.append(
- models.V1beta1TrialParameterSpec(name="trialName", reference="${{trialSpec.Name}}")
- )
- env.append(
- client.V1EnvVar(name="KATIB_TRIAL_NAME", value="${{trialParameters.trialName}}")
- )
+
+ # Pass Trial name as env variable `KATIB_TRIAL_NAME` to the training containers.
+ trial_params.append(
+ models.V1beta1TrialParameterSpec(name="trialName", reference="${{trialSpec.Name}}")
+ )
+ env.append(
+ client.V1EnvVar(name="KATIB_TRIAL_NAME", value="${{trialParameters.trialName}}")
+ )
# Create Trial specification.
trial_spec = client.V1Job(
From b1cd440f5a3d26293f81c67f107709e6f7175ce3 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Wed, 10 Jul 2024 08:24:56 +0000
Subject: [PATCH 05/13] fix: delete passing env variable logics in
katib_client.py
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
sdk/python/v1beta1/kubeflow/katib/api/katib_client.py | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py b/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
index 55aeca58987..398e73ef908 100644
--- a/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
+++ b/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
@@ -190,8 +190,6 @@ def tune(
):
"""Create HyperParameter Tuning Katib Experiment from the objective function.
- Katib always passes Trial name as env variable `KATIB_TRIAL_NAME` to the training container.
-
Args:
name: Name for the Experiment.
objective: Objective function that Katib uses to train the model.
@@ -392,14 +390,6 @@ def tune(
collector=models.V1beta1CollectorSpec(kind=metrics_collector_config["kind"])
)
- # Pass Trial name as env variable `KATIB_TRIAL_NAME` to the training containers.
- trial_params.append(
- models.V1beta1TrialParameterSpec(name="trialName", reference="${{trialSpec.Name}}")
- )
- env.append(
- client.V1EnvVar(name="KATIB_TRIAL_NAME", value="${{trialParameters.trialName}}")
- )
-
# Create Trial specification.
trial_spec = client.V1Job(
api_version="batch/v1",
From 828cbfd43a8adbcbf12d4dc909ba618c06872fc9 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Fri, 12 Jul 2024 03:39:12 +0000
Subject: [PATCH 06/13] fix: passing env variable KATIB_TRIAL_NAME in the
webhook of pod.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
pkg/controller.v1beta1/consts/const.go | 3 +++
pkg/webhook/v1beta1/pod/inject_webhook.go | 20 ++++++++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/pkg/controller.v1beta1/consts/const.go b/pkg/controller.v1beta1/consts/const.go
index b59fb4f4bc6..2cffe30cde3 100644
--- a/pkg/controller.v1beta1/consts/const.go
+++ b/pkg/controller.v1beta1/consts/const.go
@@ -60,6 +60,9 @@ const (
// resources list which can be used as trial template
ConfigTrialResources = "trial-resources"
+ // EnvTrialName is the env variable of Trial name
+ EnvTrialName = "KATIB_TRIAL_NAME"
+
// LabelExperimentName is the label of experiment name.
LabelExperimentName = "katib.kubeflow.org/experiment"
// LabelSuggestionName is the label of suggestion name.
diff --git a/pkg/webhook/v1beta1/pod/inject_webhook.go b/pkg/webhook/v1beta1/pod/inject_webhook.go
index e7ab14029bb..db01fca5d44 100644
--- a/pkg/webhook/v1beta1/pod/inject_webhook.go
+++ b/pkg/webhook/v1beta1/pod/inject_webhook.go
@@ -20,6 +20,7 @@ import (
"context"
"encoding/json"
"errors"
+ "fmt"
"net/http"
"path/filepath"
"strconv"
@@ -140,6 +141,25 @@ func (s *SidecarInjector) Mutate(pod *v1.Pod, namespace string) (*v1.Pod, error)
// Add Katib Trial labels to the Pod metadata.
mutatePodMetadata(mutatedPod, trial)
+
+ // Pass env variable KATIB_TRIAL_NAME to training containers using fieldPath.
+ for idx := range mutatedPod.Spec.Containers {
+ if mutatedPod.Spec.Containers[idx].Env == nil {
+ mutatedPod.Spec.Containers[idx].Env = []v1.EnvVar{}
+ }
+ mutatedPod.Spec.Containers[idx].Env = append(
+ mutatedPod.Spec.Containers[idx].Env,
+ v1.EnvVar{
+ Name: consts.EnvTrialName,
+ ValueFrom: &v1.EnvVarSource{
+ FieldRef: &v1.ObjectFieldSelector{
+ FieldPath: fmt.Sprintf("metadata.labels['%s']", consts.LabelTrialName),
+ },
+ },
+ },
+ )
+ }
+
// Do the following mutation only for the Primary pod.
// If PrimaryPodLabel is not set we mutate all pods which are related to Trial job.
// Otherwise, mutate pod only with the appropriate labels.
From a0c8749057dae231fe308606c208e35b0b317d9a Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Fri, 12 Jul 2024 15:17:21 +0000
Subject: [PATCH 07/13] fix: pass env variable KATIB_TRIAL_NAME only to the
primary container.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
pkg/webhook/v1beta1/pod/inject_webhook.go | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/pkg/webhook/v1beta1/pod/inject_webhook.go b/pkg/webhook/v1beta1/pod/inject_webhook.go
index db01fca5d44..6cba7a2647b 100644
--- a/pkg/webhook/v1beta1/pod/inject_webhook.go
+++ b/pkg/webhook/v1beta1/pod/inject_webhook.go
@@ -141,14 +141,14 @@ func (s *SidecarInjector) Mutate(pod *v1.Pod, namespace string) (*v1.Pod, error)
// Add Katib Trial labels to the Pod metadata.
mutatePodMetadata(mutatedPod, trial)
-
- // Pass env variable KATIB_TRIAL_NAME to training containers using fieldPath.
- for idx := range mutatedPod.Spec.Containers {
- if mutatedPod.Spec.Containers[idx].Env == nil {
- mutatedPod.Spec.Containers[idx].Env = []v1.EnvVar{}
+ // Pass env variable KATIB_TRIAL_NAME to the primary container using fieldPath.
+ index := getPrimaryContainerIndex(mutatedPod.Spec.Containers, trial.Spec.PrimaryContainerName)
+ if index >= 0 {
+ if mutatedPod.Spec.Containers[index].Env == nil {
+ mutatedPod.Spec.Containers[index].Env = []v1.EnvVar{}
}
- mutatedPod.Spec.Containers[idx].Env = append(
- mutatedPod.Spec.Containers[idx].Env,
+ mutatedPod.Spec.Containers[index].Env = append(
+ mutatedPod.Spec.Containers[index].Env,
v1.EnvVar{
Name: consts.EnvTrialName,
ValueFrom: &v1.EnvVarSource{
@@ -158,6 +158,9 @@ func (s *SidecarInjector) Mutate(pod *v1.Pod, namespace string) (*v1.Pod, error)
},
},
)
+ } else {
+ return nil, fmt.Errorf("Unable to find primary container %v in mutated pod containers %v",
+ trial.Spec.PrimaryContainerName, pod.Spec.Containers)
}
// Do the following mutation only for the Primary pod.
From e73826f1d891acbe3cce8bb5d63227e3287329c0 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Fri, 12 Jul 2024 15:20:40 +0000
Subject: [PATCH 08/13] chore: add report_metrics in post_gen.py.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
hack/gen-python-sdk/post_gen.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hack/gen-python-sdk/post_gen.py b/hack/gen-python-sdk/post_gen.py
index b61021edeab..a0d1649d205 100644
--- a/hack/gen-python-sdk/post_gen.py
+++ b/hack/gen-python-sdk/post_gen.py
@@ -41,6 +41,8 @@ def _rewrite_helper(input_file, output_file, rewrite_rules):
if (output_file == "sdk/python/v1beta1/kubeflow/katib/__init__.py"):
lines.append("# Import Katib API client.\n")
lines.append("from kubeflow.katib.api.katib_client import KatibClient\n")
+ lines.append("# Import Katib report metrics functions")
+ lines.append("from kubeflow.katib.api.report_metrics import report_metrics")
lines.append("# Import Katib helper functions.\n")
lines.append("import kubeflow.katib.api.search as search\n")
lines.append("# Import Katib helper constants.\n")
From 93df4a9479ff568d0ba152f4eda24b5be2038c60 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Fri, 12 Jul 2024 15:27:37 +0000
Subject: [PATCH 09/13] fix: change nil error to allErrs(deleted by accident).
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
pkg/webhook/v1beta1/experiment/validator/validator.go | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pkg/webhook/v1beta1/experiment/validator/validator.go b/pkg/webhook/v1beta1/experiment/validator/validator.go
index cf865fe529e..56bde4b0621 100644
--- a/pkg/webhook/v1beta1/experiment/validator/validator.go
+++ b/pkg/webhook/v1beta1/experiment/validator/validator.go
@@ -489,7 +489,7 @@ func (g *DefaultValidator) validateMetricsCollector(inst *experimentsv1beta1.Exp
// TODO(hougangliu): log warning message if some field will not be used for the metricsCollector kind
switch mcKind {
case commonapiv1beta1.PushCollector, commonapiv1beta1.StdOutCollector:
- return nil
+ return allErrs
case commonapiv1beta1.FileCollector:
if mcSpec.Source == nil || mcSpec.Source.FileSystemPath == nil ||
mcSpec.Source.FileSystemPath.Kind != commonapiv1beta1.FileKind || !filepath.IsAbs(mcSpec.Source.FileSystemPath.Path) {
From 3e83699eeabbd3e99bce1fc07d0c78f0bc971701 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Fri, 12 Jul 2024 15:35:51 +0000
Subject: [PATCH 10/13] fix: fix lint error in inject_webhook.go.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
pkg/webhook/v1beta1/pod/inject_webhook.go | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pkg/webhook/v1beta1/pod/inject_webhook.go b/pkg/webhook/v1beta1/pod/inject_webhook.go
index 6cba7a2647b..c96ff16cbcc 100644
--- a/pkg/webhook/v1beta1/pod/inject_webhook.go
+++ b/pkg/webhook/v1beta1/pod/inject_webhook.go
@@ -148,7 +148,7 @@ func (s *SidecarInjector) Mutate(pod *v1.Pod, namespace string) (*v1.Pod, error)
mutatedPod.Spec.Containers[index].Env = []v1.EnvVar{}
}
mutatedPod.Spec.Containers[index].Env = append(
- mutatedPod.Spec.Containers[index].Env,
+ mutatedPod.Spec.Containers[index].Env,
v1.EnvVar{
Name: consts.EnvTrialName,
ValueFrom: &v1.EnvVarSource{
From c6edd0a9a80b23aa4e10fee712a62d1e95d52e10 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Tue, 16 Jul 2024 08:51:47 +0000
Subject: [PATCH 11/13] fix: wrap env variables passing logics into
mutatePodEnv.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
pkg/webhook/v1beta1/pod/inject_webhook.go | 26 +++++-----------------
pkg/webhook/v1beta1/pod/utils.go | 27 +++++++++++++++++++++++
2 files changed, 32 insertions(+), 21 deletions(-)
diff --git a/pkg/webhook/v1beta1/pod/inject_webhook.go b/pkg/webhook/v1beta1/pod/inject_webhook.go
index c96ff16cbcc..3932a6bbfd3 100644
--- a/pkg/webhook/v1beta1/pod/inject_webhook.go
+++ b/pkg/webhook/v1beta1/pod/inject_webhook.go
@@ -20,7 +20,6 @@ import (
"context"
"encoding/json"
"errors"
- "fmt"
"net/http"
"path/filepath"
"strconv"
@@ -141,26 +140,11 @@ func (s *SidecarInjector) Mutate(pod *v1.Pod, namespace string) (*v1.Pod, error)
// Add Katib Trial labels to the Pod metadata.
mutatePodMetadata(mutatedPod, trial)
- // Pass env variable KATIB_TRIAL_NAME to the primary container using fieldPath.
- index := getPrimaryContainerIndex(mutatedPod.Spec.Containers, trial.Spec.PrimaryContainerName)
- if index >= 0 {
- if mutatedPod.Spec.Containers[index].Env == nil {
- mutatedPod.Spec.Containers[index].Env = []v1.EnvVar{}
- }
- mutatedPod.Spec.Containers[index].Env = append(
- mutatedPod.Spec.Containers[index].Env,
- v1.EnvVar{
- Name: consts.EnvTrialName,
- ValueFrom: &v1.EnvVarSource{
- FieldRef: &v1.ObjectFieldSelector{
- FieldPath: fmt.Sprintf("metadata.labels['%s']", consts.LabelTrialName),
- },
- },
- },
- )
- } else {
- return nil, fmt.Errorf("Unable to find primary container %v in mutated pod containers %v",
- trial.Spec.PrimaryContainerName, pod.Spec.Containers)
+ // Add env variables to the Pod's primary container.
+ // We add this function because of push-based metrics collection function `report_metrics` in Python SDK.
+ // Currently, we only pass the Trial name as env variable `KATIB_TRIAL_NAME` to the training container.
+ if err := mutatePodEnv(mutatedPod, trial); err != nil {
+ return nil, err
}
// Do the following mutation only for the Primary pod.
diff --git a/pkg/webhook/v1beta1/pod/utils.go b/pkg/webhook/v1beta1/pod/utils.go
index 6381bf6d895..7dad82553cf 100644
--- a/pkg/webhook/v1beta1/pod/utils.go
+++ b/pkg/webhook/v1beta1/pod/utils.go
@@ -281,6 +281,33 @@ func mutatePodMetadata(pod *v1.Pod, trial *trialsv1beta1.Trial) {
pod.Labels = podLabels
}
+func mutatePodEnv(pod *v1.Pod, trial *trialsv1beta1.Trial) error {
+ // Search for the primary container
+ index := getPrimaryContainerIndex(pod.Spec.Containers, trial.Spec.PrimaryContainerName)
+ if index >= 0 {
+ if pod.Spec.Containers[index].Env == nil {
+ pod.Spec.Containers[index].Env = []v1.EnvVar{}
+ }
+
+ // Pass env variable KATIB_TRIAL_NAME to the primary container using fieldPath
+ pod.Spec.Containers[index].Env = append(
+ pod.Spec.Containers[index].Env,
+ v1.EnvVar{
+ Name: consts.EnvTrialName,
+ ValueFrom: &v1.EnvVarSource{
+ FieldRef: &v1.ObjectFieldSelector{
+ FieldPath: fmt.Sprintf("metadata.labels['%s']", consts.LabelTrialName),
+ },
+ },
+ },
+ )
+ return nil
+ } else {
+ return fmt.Errorf("Unable to find primary container %v in mutated pod containers %v",
+ trial.Spec.PrimaryContainerName, pod.Spec.Containers)
+ }
+}
+
func getSidecarContainerName(cKind common.CollectorKind) string {
if cKind == common.StdOutCollector || cKind == common.FileCollector {
return mccommon.MetricLoggerCollectorContainerName
From 7d6a64d9547c5f759bd1910207bcaf05a4747834 Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Tue, 16 Jul 2024 11:38:18 +0000
Subject: [PATCH 12/13] chore: add unit tests for mutatePodEnv.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
.../v1beta1/pod/inject_webhook_test.go | 103 ++++++++++++++++++
1 file changed, 103 insertions(+)
diff --git a/pkg/webhook/v1beta1/pod/inject_webhook_test.go b/pkg/webhook/v1beta1/pod/inject_webhook_test.go
index ab1646b6769..d55f83172cd 100644
--- a/pkg/webhook/v1beta1/pod/inject_webhook_test.go
+++ b/pkg/webhook/v1beta1/pod/inject_webhook_test.go
@@ -25,7 +25,10 @@ import (
"testing"
"time"
+ "github.com/google/go-cmp/cmp"
+ "github.com/google/go-cmp/cmp/cmpopts"
"github.com/onsi/gomega"
+ "google.golang.org/protobuf/testing/protocmp"
appsv1 "k8s.io/api/apps/v1"
batchv1 "k8s.io/api/batch/v1"
corev1 "k8s.io/api/core/v1"
@@ -1067,3 +1070,103 @@ func TestMutatePodMetadata(t *testing.T) {
}
}
}
+
+func TestMutatePodEnv(t *testing.T) {
+ testcases := map[string]struct {
+ pod *v1.Pod
+ trial *trialsv1beta1.Trial
+ mutatedPod *v1.Pod
+ wantError error
+ }{
+ "Valid case for mutating Pod's env variable": {
+ pod: &v1.Pod{
+ Spec: v1.PodSpec{
+ Containers: []v1.Container{
+ {
+ Name: "training-container",
+ },
+ },
+ },
+ },
+ trial: &trialsv1beta1.Trial{
+ Spec: trialsv1beta1.TrialSpec{
+ PrimaryContainerName: "training-container",
+ },
+ },
+ mutatedPod: &v1.Pod{
+ Spec: v1.PodSpec{
+ Containers: []v1.Container{
+ {
+ Name: "training-container",
+ Env: []v1.EnvVar{
+ {
+ Name: consts.EnvTrialName,
+ ValueFrom: &v1.EnvVarSource{
+ FieldRef: &v1.ObjectFieldSelector{
+ FieldPath: fmt.Sprintf("metadata.labels['%s']", consts.LabelTrialName),
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ "Mismatch for Pod name and primaryContainerName in Trial": {
+ pod: &v1.Pod{
+ Spec: v1.PodSpec{
+ Containers: []v1.Container{
+ {
+ Name: "training-container",
+ },
+ },
+ },
+ },
+ trial: &trialsv1beta1.Trial{
+ Spec: trialsv1beta1.TrialSpec{
+ PrimaryContainerName: "training-containers",
+ },
+ },
+ mutatedPod: &v1.Pod{
+ Spec: v1.PodSpec{
+ Containers: []v1.Container{
+ {
+ Name: "training-container",
+ },
+ },
+ },
+ },
+ wantError: fmt.Errorf(
+ "Unable to find primary container %v in mutated pod containers %v",
+ "training-containers",
+ []v1.Container{
+ {
+ Name: "training-container",
+ },
+ },
+ ),
+ },
+ }
+
+ for name, testcase := range testcases {
+ t.Run(name, func(t *testing.T) {
+ err := mutatePodEnv(testcase.pod, testcase.trial)
+ // Compare error with expected error
+ if testcase.wantError != nil && err != nil {
+ if diff := cmp.Diff(testcase.wantError.Error(), err.Error()); len(diff) != 0 {
+ t.Errorf("Unexpected error (-want,+got):\n%s", diff)
+ }
+ } else if testcase.wantError != nil || err != nil {
+ t.Errorf(
+ "Unexpected error (-want,+got):\n%s",
+ cmp.Diff(testcase.wantError, err, cmpopts.EquateErrors()),
+ )
+ }
+ // Compare Pod with expected pod after mutation
+ if diff := cmp.Diff(testcase.mutatedPod, testcase.pod, protocmp.Transform()); len(diff) != 0 {
+ t.Errorf("Unexpected mutated result (-want,+got):\n%s", diff)
+ }
+ })
+ }
+}
From d2b0c4852bce239c4655e01520540aaee9e221ad Mon Sep 17 00:00:00 2001
From: Electronic-Waste <2690692950@qq.com>
Date: Wed, 17 Jul 2024 05:30:43 +0000
Subject: [PATCH 13/13] fix: delete protocmp.
Signed-off-by: Electronic-Waste <2690692950@qq.com>
---
pkg/webhook/v1beta1/pod/inject_webhook_test.go | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/pkg/webhook/v1beta1/pod/inject_webhook_test.go b/pkg/webhook/v1beta1/pod/inject_webhook_test.go
index d55f83172cd..4436c10e7f1 100644
--- a/pkg/webhook/v1beta1/pod/inject_webhook_test.go
+++ b/pkg/webhook/v1beta1/pod/inject_webhook_test.go
@@ -28,7 +28,6 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/onsi/gomega"
- "google.golang.org/protobuf/testing/protocmp"
appsv1 "k8s.io/api/apps/v1"
batchv1 "k8s.io/api/batch/v1"
corev1 "k8s.io/api/core/v1"
@@ -1164,7 +1163,7 @@ func TestMutatePodEnv(t *testing.T) {
)
}
// Compare Pod with expected pod after mutation
- if diff := cmp.Diff(testcase.mutatedPod, testcase.pod, protocmp.Transform()); len(diff) != 0 {
+ if diff := cmp.Diff(testcase.mutatedPod, testcase.pod); len(diff) != 0 {
t.Errorf("Unexpected mutated result (-want,+got):\n%s", diff)
}
})