Skip to content

Commit

Permalink
fix: tensorboard inherits imagePullSecrets from experiment [DET-8458] (
Browse files Browse the repository at this point in the history
…#5123)

The secrets are inherited only when the image from that experiment is
also inherited.
  • Loading branch information
rb-determined-ai authored Sep 26, 2022
1 parent 5fc93fd commit 47dd822
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 2 deletions.
7 changes: 7 additions & 0 deletions docs/release-notes/5123-tb-secrets.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
:orphan:

**Fixes**

- Tensorboards in Kubernetes now inherit the environment.pod_spec.spec.imagePullSecrets value from
the relevant experiment configuration any time the tensorboard task inherits its image from that
experiment configuration.
36 changes: 34 additions & 2 deletions e2e_tests/tests/command/test_tensorboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,7 @@ def test_start_tensorboard_for_multi_experiment(tmp_path: Path, secrets: Dict[st
def test_start_tensorboard_with_custom_image(tmp_path: Path) -> None:
"""
Start a random experiment, start a TensorBoard instance pointed
to the experiment with custom image, verify the image has been set,
and kill the TensorBoard instance.
to the experiment with custom image, verify the image has been set.
"""
experiment_id = exp.run_basic_test(
conf.fixtures_path("no_op/single-one-short-step.yaml"),
Expand Down Expand Up @@ -196,3 +195,36 @@ def test_start_tensorboard_with_custom_image(tmp_path: Path) -> None:
and config["environment"]["image"]["cuda"] == "alpine"
and config["environment"]["image"]["rocm"] == "alpine"
), config


@pytest.mark.e2e_cpu
def test_tensorboard_inherit_image_pull_secrets(tmp_path: Path) -> None:
"""
Start a random experiment with image_pull_secrets, start a TensorBoard
instance pointed to the experiment, verify the secrets are inherited.
"""
exp_secrets = [{"name": "ips"}]
config_obj = conf.load_config(conf.fixtures_path("no_op/single-one-short-step.yaml"))
pod = config_obj.setdefault("environment", {}).setdefault("pod_spec", {})
pod.setdefault("spec", {})["imagePullSecrets"] = [{"name": "ips"}]
experiment_id = exp.run_basic_test_with_temp_config(config_obj, conf.fixtures_path("no_op"), 1)

command = [
"det",
"-m",
conf.make_master_url(),
"tensorboard",
"start",
str(experiment_id),
"--no-browser",
"--detach",
]
res = subprocess.run(command, universal_newlines=True, stdout=subprocess.PIPE, check=True)
t_id = res.stdout.strip("\n")
command = ["det", "-m", conf.make_master_url(), "tensorboard", "config", t_id]
res = subprocess.run(command, universal_newlines=True, stdout=subprocess.PIPE, check=True)
config = yaml.safe_load(res.stdout)

ips = config["environment"]["pod_spec"]["spec"]["imagePullSecrets"]

assert ips == exp_secrets, (ips, exp_secrets)
21 changes: 21 additions & 0 deletions master/internal/api_tensorboard.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (

"github.com/pkg/errors"

k8sV1 "k8s.io/api/core/v1"

"github.com/determined-ai/determined/master/internal/api"
"github.com/determined-ai/determined/master/internal/db"
"github.com/determined-ai/determined/master/pkg/actor"
Expand Down Expand Up @@ -274,6 +276,25 @@ func (a *apiServer) LaunchTensorboard(
CUDA: expConf.Environment().Image().CUDA(),
ROCM: expConf.Environment().Image().ROCM(),
}

// Inherit ImagePullSecrets too, if we inherit the image.
presentPod := spec.Config.Environment.PodSpec
experimentPod := expConf.Environment().PodSpec()
if experimentPod != nil && len(experimentPod.Spec.ImagePullSecrets) > 0 {
if presentPod != nil {
// Update the k8sV1.Pod with the experiment's pod's ImagePullSecrets.
presentPod.Spec.ImagePullSecrets = append(
presentPod.Spec.ImagePullSecrets, experimentPod.Spec.ImagePullSecrets...,
)
} else {
// Construct a new k8sV1.Pod with just ImagePullSecrets set.
spec.Config.Environment.PodSpec = &k8sV1.Pod{
Spec: k8sV1.PodSpec{
ImagePullSecrets: experimentPod.Spec.ImagePullSecrets,
},
}
}
}
}
// Prefer RegistryAuth already present over the one from inferred from the experiment.
if spec.Config.Environment.RegistryAuth == nil {
Expand Down

0 comments on commit 47dd822

Please sign in to comment.