Add Helm chart for kubeflow trainer

[ChenYi015]

What this PR does / why we need it:

Close #1197

Checklist:

• Docs included if any changes are user facing

[andreyvelich]

Thank you for doing this @ChenYi015!
Did you get a chance to explore Kueue script to generate Helm Charts from Kustomize manifests ?
I think, that should significantly help us to keep Kustomize and Charts in sync.

https://github.com/kubernetes-sigs/kueue/blob/main/hack/update-helm.sh

[ChenYi015]

I will take a look at the shell script, it is a bit complicated. I think it may be more easier to sync manifests templated by Helm charts to Kustomize manifests, WDYT?

[ChenYi015]

I have added a shell script hack/sync-manifests. Now one can execute make sync-manifests to sync the Kustomize manifests from the manifests templated by the Helm chart.

[andreyvelich]

Thanks for this effort @ChenYi015!
@tenzen-y @kannon92 @ahg-g @astefanutti @Electronic-Waste @kubeflow/wg-training-leads What do you think about it ?
Should we go other way around to sync Helm Charts to Kustomize Manifests ?
We can use the same approach for JobSet/Kueue if that is easier.

[andreyvelich]

cc @kubeflow/wg-manifests-leads @kubeflow/release-team @varodrig to review script of sync Kustomize + Helm automatically.

[andreyvelich]

cc @franciscojavierarceo

[andreyvelich]

@ChenYi015 Given that we created Helm Charts for JobSet, are we ready to finish this PR?
@kannon92 did we push charts to the OCI registry after this PR: kubernetes-sigs/jobset#792 ?

[kannon92]

@ChenYi015 Given that we created Helm Charts for JobSet, are we ready to finish this PR? @kannon92 did we push charts to the OCI registry after this PR: kubernetes-sigs/jobset#792 ?

Not yet. We are still working on that change. Once we have a release it should push to the registry.

[ChenYi015]

/hold for waiting jobset helm chart to be published

[varodrig]

This is great work! Thank for working on this.

I left a couple of comments and recommendations.

[ChenYi015]

/hold cancel for jobset chart v0.8.0 has been released. I have updated the PR, now one can test the Helm chart installation with the following comand:

helm upgrade kubeflow-trainer charts/kubeflow-trainer \
      --install \
      --dependency-update \
      --namespace kubeflow-system \
      --create-namespace \
      --wait \
      --timeout 5m0s \
      --set jobset.install=true

[ChenYi015]

/unhold

[andreyvelich]

/lgtm
/cc @tenzen-y @johnugeorge @saileshd1402 @Electronic-Waste @juliusvonkohout @deepanker13 @franciscojavierarceo @thesuperzapper @astefanutti

[google-oss-prow]

@andreyvelich: GitHub didn't allow me to request PR reviews from the following users: saileshd1402.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/lgtm
/cc @tenzen-y @johnugeorge @saileshd1402 @Electronic-Waste @juliusvonkohout @deepanker13 @franciscojavierarceo @thesuperzapper @astefanutti

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[andreyvelich]

@ChenYi015 I was testing your Helm Charts. A few questions:

1. Can we remove the kubeflow-trainer- prefix from the JobSet controller or this is a requirements that all resources created from this chart must start with kubeflow-trainer-?

k get pods -n $NS
NAME                                                  READY   STATUS              RESTARTS     AGE
kubeflow-trainer-controller-manager-9d8bb45ff-gm82p   0/1     ContainerCreating   0            4s
kubeflow-trainer-jobset-controller-744494458d-6hmgw   0/1     Running             1 (3s ago)   4s

2. The ClusterTrainingRuntime are not installed as part of the Charts.

3. For JobSet why do we create RoleBinding and ClusterRoleBinding ?

k get rolebinding -n kubeflow-system        
NAME                                 ROLE                                      AGE
kubeflow-trainer-jobset-controller   Role/kubeflow-trainer-jobset-controller   5m23s

k get clusterrolebinding -n kubeflow-system | grep jobset
kubeflow-trainer-jobset-controller                              ClusterRole/kubeflow-trainer-jobset-controller                                     5m26s

cc @kannon92 @kubeflow/wg-training-leads @Electronic-Waste @astefanutti

[andreyvelich]

Otherwise, the default installation fails.

Suggested change

	# -- Image tag.
	# @default -- If not set, the chart version will be used.
	tag: ""
	# -- Image tag.
	tag: latest

[ChenYi015]

Yes, but it will work as expected if we have released v2.0.0 and push associated image kubeflow/trainer/trainer-controller-manager:2.0.0 to GHCR.

[andreyvelich]

Historically, we try to keep latest tag in the master branch and release tag in the release-xx branch.
Like in Katib:

• master branch: https://github.com/kubeflow/katib/blob/master/manifests/v1beta1/installs/katib-standalone/kustomization.yaml#L23
• release branch: https://github.com/kubeflow/katib/blob/release-0.18/manifests/v1beta1/installs/katib-standalone/kustomization.yaml#L23

Shall we do the same for Helm Charts @ChenYi015 @Electronic-Waste @astefanutti @kubeflow/wg-training-leads?

[ChenYi015]

Have updated to use latest as image tag.

[ChenYi015]

Can we remove the kubeflow-trainer- prefix from the JobSet controller or this is a requirements that all resources created from this chart must start with kubeflow-trainer-?

I thinks this is the default behavior of installing jobset as a sub chart of trainer. I will try to find a way to remove the prefix.

For JobSet why do we create RoleBinding and ClusterRoleBinding ?

The ClusterRole/ClusterRoleBinding comes from role.yaml and role_binding.yaml and are used to grant cluster-scoped permissions.

The Role/RoleBinding comes from leader_election_role.yaml and leader_election_role_binding.yaml and are used to grant namespace-scoped permissions (e.g. leader election).

[andreyvelich]

The Role/RoleBinding comes from leader_election_role.yaml and leader_election_role_binding.yaml and are used to grant namespace-scoped permissions (e.g. leader election).

Oh, I see. Maybe we should rename the postfix for this role as: -leader-election. To be consistent with manifests: https://github.com/kubernetes-sigs/jobset/blob/main/config/components/rbac/role_binding.yaml

cc @kannon92

[ChenYi015]

The ClusterTrainingRuntime are not installed as part of the Charts.

We may have problems when do helm upgrading, when the webhook server are restarted and not ready to validate/mutate ClusterTrainingRuntime, it will fail the upgrading. I will try to find a better way to manage ClusterTrainingRuntime in the follow-up PRs.

[andreyvelich]

Thank you for the update!
Feel free to un-hold the PR if you want to address the JobSet naming changes in the followup PRs.
/lgtm
/approve
/hold

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [andreyvelich]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ChenYi015]

Feel free to un-hold the PR if you want to address the JobSet naming changes in the followup PRs.

@andreyvelich Thank you for your review and comments. Let us improve the Helm chart step by step.

/unhold