Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add KATIB_EXPERIMENT_NAME to pod env. #2488

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add KATIB_EXPERIMENT_NAME to pod env. #2488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcd5174
chore(webhook): add KATIB_EXPERIMENT_NAME to pod env.
Electronic-Waste Jan 15, 2025
51c61ac
fix(test): update inject webhook test.
Electronic-Waste Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/controller.v1beta1/consts/const.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ const (

// EnvTrialName is the env variable of Trial name
EnvTrialName = "KATIB_TRIAL_NAME"
// EnvExperimentName is the env variable of Experiment name
EnvExperimentName = "KATIB_EXPERIMENT_NAME"

// LabelExperimentName is the label of experiment name.
LabelExperimentName = "katib.kubeflow.org/experiment"
Expand Down
8 changes: 8 additions & 0 deletions pkg/webhook/v1beta1/pod/inject_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1094,6 +1094,14 @@ func TestMutatePodEnv(t *testing.T) {
},
},
},
{
Name: consts.EnvExperimentName,
ValueFrom: &v1.EnvVarSource{
FieldRef: &v1.ObjectFieldSelector{
FieldPath: fmt.Sprintf("metadata.labels['%s']", consts.LabelExperimentName),
},
},
},
},
},
},
Expand Down
10 changes: 9 additions & 1 deletion pkg/webhook/v1beta1/pod/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ func mutatePodEnv(pod *v1.Pod, trial *trialsv1beta1.Trial) error {
pod.Spec.Containers[index].Env = []v1.EnvVar{}
}

// Pass env variable KATIB_TRIAL_NAME to the primary container using fieldPath
// Pass env variable KATIB_TRIAL_NAME and KATIB_EXPERIMENT_NAME to the primary container using fieldPath
pod.Spec.Containers[index].Env = append(
pod.Spec.Containers[index].Env,
v1.EnvVar{
Expand All @@ -302,6 +302,14 @@ func mutatePodEnv(pod *v1.Pod, trial *trialsv1beta1.Trial) error {
},
},
},
v1.EnvVar{
Name: consts.EnvExperimentName,
ValueFrom: &v1.EnvVarSource{
FieldRef: &v1.ObjectFieldSelector{
FieldPath: fmt.Sprintf("metadata.labels['%s']", consts.LabelExperimentName),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This label exists in Trial Pod. Here is an example:

Name:             tune-experiment-lhs5mrsh-9xqwd
Namespace:        kubeflow
Priority:         0
Service Account:  default
Start Time:       Wed, 15 Jan 2025 06:29:28 +0000
Labels:           batch.kubernetes.io/controller-uid=859791df-d3da-404d-841c-86eca6c9148f
                  batch.kubernetes.io/job-name=tune-experiment-lhs5mrsh
                  controller-uid=859791df-d3da-404d-841c-86eca6c9148f
                  job-name=tune-experiment-lhs5mrsh
                  katib.kubeflow.org/experiment=tune-experiment
                  katib.kubeflow.org/trial=tune-experiment-lhs5mrsh
Annotations:      sidecar.istio.io/inject: false
Status:           Running
...

Copy link

@mickvangelderen mickvangelderen Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh so we can already reference metadata.labels['katib.kubeflow.org/experiment'] in the spec? That means that the issue #2473 can already be closed as completed as we can obtain the namespace from /var/run/secrets/kubernetes.io/serviceaccount/namespace and the experiment name from the metadata.labels['katib.kubeflow.org/experiment'].

Still, I think it would be convenient to have these values exposed as environment variables by default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickvangelderen We could not access metadata.labels['katib.kubeflow.org/experiment'] directly since it's not included in .trialSpec.Labels field. The label lies in .Labels field in Trial and Pod.

And also, I agree with you that it will be more convenient if we could also pass KATIB_NAMESPACE to env of Pod by default. WDTY👀 @kubeflow/wg-automl-leads

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickvangelderen And it will also be okay to pass namespace and trial name as env variables like this:

katib/examples/v1beta1/trial-template/trial-metadata-substitution.yaml

Lines 77 to 81 in fc858d1

env:
- name: TRIAL_NAME
value: ${trialParameters.trialName}
- name: TRIAL_NAMESPACE
value: ${trialParameters.trialNamespace}

},
},
},
)
return nil
} else {
Expand Down