helm: add Kubeflow Notebooks chart - #3525
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome to the Kubeflow Community Distribution Repository Thanks for opening your first PR. Your contribution means a lot to the Kubeflow community. Before making more PRs: Community Resources:
Thanks again for helping to improve Kubeflow. |
There was a problem hiding this comment.
Pull request overview
Adds a co-located Helm wrapper chart for Kubeflow Notebooks v1 under applications/notebooks-v1/helm, and wires it into the existing Helm/Kustomize render-parity comparison and synchronization workflow so future upstream refreshes also regenerate the chart payload.
Changes:
- Introduce
kubeflow-notebooksHelm wrapper chart (static Kustomize-parity “platform” scenario). - Extend Helm/Kustomize comparison scripts to include the new
kubeflow-notebookscomponent/scenario. - Update the Notebooks v1 synchronization script and CI workflow to keep the chart’s
appVersionand rendered platform template refreshed (and to install Helm v4.2.2 for the comparison job).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
applications/notebooks-v1/helm/Chart.yaml |
New chart metadata for kubeflow-notebooks. |
applications/notebooks-v1/helm/README.md |
Installation and parity-comparison instructions for the new chart. |
applications/notebooks-v1/helm/values.yaml |
Default values, including the scenario switch. |
applications/notebooks-v1/helm/ci/values-platform.yaml |
CI fixture values for the platform scenario. |
applications/notebooks-v1/helm/templates/platform.yaml |
Generated static template containing the platform render payload. |
applications/notebooks-v1/helm/templates/validate.yaml |
Scenario validation guardrail for supported values. |
tests/helm_kustomize_compare.sh |
Adds kubeflow-notebooks wiring (chart path, kustomize overlay, values, namespace). |
tests/helm_kustomize_compare_all.sh |
Adds kubeflow-notebooks to the “all components” scenario matrix and help text. |
tests/helm_kustomize_compare.py |
Allows kubeflow-notebooks as a recognized component in the comparison tool. |
scripts/synchronize-notebooks-v1-manifests.sh |
Extends sync to also update chart appVersion and regenerate templates/platform.yaml. |
.github/workflows/helm-kustomize-comparison.yml |
Triggers on Notebooks changes and installs Helm v4.2.2 for parity checks. |
cea8d26 to
4a0882a
Compare
1447480 to
20f8f1d
Compare
| wait_for_notebooks_deployment() { | ||
| local deployment_name="$1" | ||
|
|
||
| if ! kubectl -n kubeflow rollout status "deployment/${deployment_name}" --timeout=180s; then | ||
| dump_notebooks_state "${deployment_name}" | ||
| exit 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
please revet back to the original 60 seconds
There was a problem hiding this comment.
Keeping 180s for now because the 60s rollout wait was flaky in hosted CI.
8b7e89d to
13ae0cb
Compare
There was a problem hiding this comment.
why is this named platform ?
There was a problem hiding this comment.
It is the consolidated platform scenario output containing all Notebook CRDs and workloads from the Kustomize overlay.
There was a problem hiding this comment.
does it have the same structure as kustomize? will it be automatically kept up to date on each release ? because to me it does not look easy to maintain
There was a problem hiding this comment.
Think 10 releases into the future
There was a problem hiding this comment.
The synchronization is automatic today, and I will apply the Dashboard split-payload generator pattern here so the chart remains reviewable across releases.
2ab0d2c to
7e062a7
Compare
|
/retest |
10cf9a6 to
fb5dbe7
Compare
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
f953b27 to
a8c703c
Compare
The synchronization script wrote the Kustomize build straight into
templates/platform.yaml. Everything under templates/ is evaluated by the Helm
template engine, so any Go template delimiter in an upstream manifest was
interpreted rather than emitted: a ConfigMap value of
"{{ .Values.notebookName }}-workspace" rendered as "-workspace", silently and
with exit code 0. It worked only because upstream currently ships no such
delimiter, and the Notebook Controller exists to template pod specifications.
Generate manifests/platform-crds.yaml and manifests/platform-resources.yaml and
load them with .Files.Get, which Helm does not send through the renderer. The
15850-line template becomes six lines.
Reuse the shared engine rather than copying it. Add the namespace guard, the
custom resource definition toggle, and lint plus parity validation before the
synchronization script commits.
Remove render_kustomize_helm_template from library.sh; it was the function that
wrote into templates/ and nothing else used it.
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
The Notebooks comparison only reached the all-scenarios job, which carries continue-on-error, so a parity regression could not fail the build. Correct the comparison namespace, which named kubeflow-system while every resource declares kubeflow, and add chart behaviour tests including a regression for the upstream delimiter that previously rendered as an empty string. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Record the required namespace, the custom resource definition deviation and why --skip-crds does not apply, that Kustomize-declared values are not exposed yet, and that a clean regeneration proves determinism rather than correctness. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
The Notebooks synchronization script runs helm lint and the Helm/Kustomize parity comparison before committing. The comparison imports PyYAML, which this job did not install, so it failed with ModuleNotFoundError. helm lint had been resolving from the runner image at whatever version it ships, the same class of problem as an inert version pin. Install Helm 4.2.2 explicitly. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Summary of Changes
Adds the Kubeflow Notebooks v1 Helm wrapper chart as a co-located application chart under
applications/notebooks-v1/helm.This PR keeps the first reviewable slice focused on Kustomize parity for the customer-facing Notebooks platform install path. It retains the
platformscenario, which maps toapplications/notebooks-v1/overlays/istio, and does not introduce standalone, split-component, or broad customization modes in this initial chart PR.The PR also updates Helm/Kustomize comparison wiring to the new chart path, uses Helm 4 in the comparison workflow, and extends
scripts/synchronize-notebooks-v1-manifests.shso future Notebooks upstream synchronizations refresh the Helm chartappVersionand regenerate the payloads.Payloads are read, not evaluated
The synchronization script previously wrote the Kustomize build directly into
applications/notebooks-v1/helm/templates/platform.yaml. Everything undertemplates/is evaluated by the Helm template engine, so a Go template delimiter in an upstream manifest was interpreted rather than emitted. A ConfigMap value of"{{ .Values.notebookName }}-workspace"rendered as"-workspace", silently and with exit code 0. That held only because upstream currently ships no such delimiter, and the Notebook Controller exists to template pod specifications.The generator now writes
manifests/platform-crds.yamlandmanifests/platform-resources.yaml, which two small templates load with.Files.Get. Helm does not send that content through the renderer, so upstream delimiters survive verbatim. The 15850-line template becomes six lines.render_kustomize_helm_templateis removed fromscripts/library.sh; it was the function that wrote intotemplates/and nothing else used it.Also in this pull request: a namespace guard, a
customResourceDefinitions.enabledcontract,helm lintand parity validation before the synchronization script commits, a blocking Notebooks comparison job, and a correction to the comparison namespace, which namedkubeflow-systemwhile every resource declareskubeflow.The chart is placed under
applications/notebooks-v1/helmbecause Notebooks v1 is an official application component, and this is the application-side equivalent of placing common wrappers undercommon/<component>/helm. The sync script ownsapplications/notebooks-v1/upstream, while the Helm chart lives as a sibling underapplications/notebooks-v1/helm.CI Note
This PR includes a focused update to
tests/notebooks_install.shbecause the Notebooks runtime jobs triggered by this PR were timing out while waiting for the existing Kustomizejupyter-web-app-deployment. The change only increases the rollout wait and adds diagnostics; it does not change manifests, Helm templates, chart values, or user-facing behavior.Dependencies
Conceptually follows the foundation, wrapper, and Dashboard chart PRs for runtime installation order:
This pull request depends on #3524. Both add
scripts/helm_manifest_generator.py, the shared generation engine, so that Notebooks reuses it rather than carrying a second copy. Whichever merges first, the other will be rebased onto it and the duplicate file resolved. Nothing else is shared.Related Issues
Part of the Project 5 Helm chart work.
Contributor Checklist
Synchronization Flow
The synchronization script refreshes six upstream components and atomically regenerates the complete platform template from the customer-facing Kustomize overlay.
flowchart LR A["Six upstream Notebook<br/>components"] --> B["Kustomize<br/>platform overlay"] B --> C["One complete<br/>Kustomize build"] C --> D["Shared helper writes<br/>platform.yaml atomically"] D --> E["Helm platform scenario"] E --> F["Helm/Kustomize<br/>parity validation"]Relevant File Structure
Installation, supported scenarios, and chart configuration are documented in
applications/notebooks-v1/helm/README.md.Validation
Also verified with temporary Helm
v4.2.2: