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.

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

jupyter-web-app's PodDefault configurations are keyed by their label selector's key, not their name #7552

Open
ca-scribner opened this issue Apr 16, 2024 · 5 comments
Labels
Milestone

Comments

@ca-scribner
Copy link
Contributor

/kind bug

What steps did you take and what happened:
The Notebook spawner page is configured using the spawner_ui_config.yaml input file.
The configurations section lets an admin define which PodDefaults should be selected by default. spawner_ui_config.yaml's comments state:

the list of PodDefault names that are selected by default

If I create a PodDefault named example:

apiVersion: kubeflow.org/v1alpha1
kind: PodDefault
metadata:
 name: example
spec:
 desc: Example PodDefault
 env:
 - name: SOMETHING
   value: stuff
 selector:
   matchLabels:
     example-match-selector: "true"

then set the spawner_ui_config.yaml's configurations to:

  configurations:
    readOnly: false

    # the list of PodDefault names that are selected by default
    # (take care to ensure these PodDefaults exist in Profile Namespaces)
    value:
    - example

I do not see it selected in the spawner UI by default:

image

What did you expect to happen:

When specifying PodDefaults by name in the spawner_ui_config.yaml, I expect them to be selected by default on the spawner page:

image

Anything else you would like to add:

I think this is a bug in the jwa backend's get_poddefaults(). get_poddefaults() returns a result where the label field is set as the key of the 0th matchLabels selector:

@bp.route("/api/namespaces/<namespace>/poddefaults")
def get_poddefaults(namespace):
pod_defaults = api.list_poddefaults(namespace)
# Return a list of pod defaults adding custom fields (label, desc) for
# forms
contents = []
for pd in pod_defaults["items"]:
label = list(pd["spec"]["selector"]["matchLabels"].keys())[0]
if "desc" in pd["spec"]:
desc = pd["spec"]["desc"]
else:
desc = pd["metadata"]["name"]
pd["label"] = label
pd["desc"] = desc
contents.append(pd)
log.info("Found poddefaults: %s", contents)
return api.success_response("poddefaults", contents)

This can be confirmed by setting

  configurations:
    readOnly: false

    # the list of PodDefault names that are selected by default
    # (take care to ensure these PodDefaults exist in Profile Namespaces)
    value:
    - example-match-selector

And reloading/refreshing the spawner page:

image

I think this should probably instead be:

        label = pd["metadata"]["name"]  # haven't compiled this - might have typo'd something

Environment:

  • Kubeflow version: (version number can be found at the bottom left corner of the Kubeflow dashboard): 1.8
  • kfctl version: (use kfctl version):
  • Kubernetes platform: (e.g. minikube)
  • Kubernetes version: (use kubectl version):
  • OS (e.g. from /etc/os-release):
@ca-scribner
Copy link
Contributor Author

ca-scribner commented Apr 16, 2024

This bug is also the cause of #6812, which identified how a the jwa fails to handle PodDefaults that have no matchLabels selector

@ca-scribner
Copy link
Contributor Author

ca-scribner commented Apr 16, 2024

This bug appears to also affect the tensorboard web app backend, which has a copy of the same get_poddefaults()

@ca-scribner
Copy link
Contributor Author

I don't understand typescript enough to be certain that the label is intended to be the name of the PodDefault, but given the comments in the spawner_ui_config.yaml I think that's probably true

@boarder7395
Copy link
Contributor

@ca-scribner I'm looking at the code in a little more detail and I see what you're saying. I think both our solutions are not optimal though.

This logic uses the matchLabel key to set a the label on the notebook to true.

def set_notebook_configurations(notebook, body, defaults):
notebook_labels = notebook["metadata"]["labels"]
labels = get_form_value(body, defaults, "configurations")
if not isinstance(labels, list):
raise BadRequest("Labels for PodDefaults are not list: %s" % labels)
for label in labels:
notebook_labels[label] = "true"

For this reason I think we need to use the label of the poddefault instead of the name of the poddefault.

BUT I'm not sure this should be the actual logic. Lets take a look at the issue with my solution . Consider the pod default

metadata:
  name: example
spec:
  annotations:
    karpenter.sh/do-not-disrupt: "true"
  selector:
    matchExpressions:
    - key: some-key
      operator: NotIn
      values: ['true']

In this case the configuration should be "some-key" and selecting that configuration in the dropdown would add the "some-key" label to the notebook. Although due to the matchExpressions that is doing the exact opposite of what we want.

I'm currently at a wall with matchExpressions because what we really want to do use this selector to create a label that conforms to the matchExpressions requirement.

@boarder7395
Copy link
Contributor

@thesuperzapper Do you have any input on the above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

3 participants