feature: add Helm chart for jobset

[ChenYi015]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Close #726

Special notes for your reviewer:

As dicussed in kubeflow/trainer#2435 (comment), this PR is based on #760.
One can install the chart as follows:

helm install jobset charts/jobset \
    --namespace jobset-system \
    --create-namespace

For Helm chart development, one can generate the Helm chart README.md by make helm-docs and run the Helm unit tests by make helm-unittest.

Does this PR introduce a user-facing change?

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: slin1237 / name: Simo Lin (e4cf89d)
• ✅ login: ChenYi015 / name: Yi Chen (5d4744d, e626d01, b7f93fd, c6920a5, 0928ac5)

[k8s-ci-robot]

Welcome @ChenYi015!

It looks like this is your first PR to kubernetes-sigs/jobset 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/jobset has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @ChenYi015. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[netlify]

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name	Link
🔨 Latest commit	c6920a5
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/67b5f17c4d25ff0008e6b2ad

[kannon92]

/ok-to-test

minor edits needed but this is looking good. I will test it later today.

[kannon92]

/hold

Good news is that the helm chart installs without issue.

Bad news is that I can't actually run any JobSet with this helm chart.

I get panics for some of the examples and others complain about validation.

Can you confirm on your end that you can run some of the examples?

[andreyvelich]

Thank you for doing this @ChenYi015! I left my initial comments.

Should we also introduce this script to JobSet to sync manifests between
Helm <-> Kustomize ?

[ChenYi015]

Bad news is that I can't actually run any JobSet with this helm chart.

I get panics for some of the examples and others complain about validation.

Can you confirm on your end that you can run some of the examples?

@kannon92 I have updated the PR and it works fine with example JobSet reconciled properly.

[ChenYi015]

Should we also introduce this script to JobSet to sync manifests between
Helm <-> Kustomize ?

I will raise another issue to track it and implement it later.

[ChenYi015]

For the initial version of the jobset Helm chart, I think this PR is ready to be merged. The values is simplified to keep only a minimum set of params. In the future, we will extend the configurations if users request. For the syncing between Helm and Kustomize, the Helm chart related CI workflows and the CRDs upgrading approach, we can implement it in the future.
cc @kannon92 @ahg-g @andreyvelich

[andreyvelich]

Thank you for this effort @ChenYi015!
I left a few small comments. We should remove values from the manifests that we don't use currently. For example:

{{ include "jobset.controller.serviceAccount.name" . }}

I think, after it we should be ready to merge it.

[andreyvelich]

Please can we move the configurations that can be set with Config API to the managerConfig section in the Helm Chart values ? Similar to Kueue: https://github.com/kubernetes-sigs/kueue/blob/main/charts/kueue/values.yaml#L70-L72.

I believe, that makes it clearer for users that these parameters are part of manager config.
WDYT @ChenYi015 @tenzen-y @kannon92 ?

[ChenYi015]

From my experience, I think we do not have to expose the Config API to end users, it will incur more complexity to configure the chart. Besides, for example, if one tries to configure the webhook service name and secret name by leveraging Config API, it will not work since the actual name of webhook service and secret is templated by Helm.

jobset/api/config/v1alpha1/configuration_types.go

Lines 105 to 118 in 943da8b

	type InternalCertManagement struct {
	// Enable controls whether to enable internal cert management or not.
	// Defaults to true. If you want to use a third-party management, e.g. cert-manager,
	// set it to false. See the user guide for more information.
	Enable *bool `json:"enable,omitempty"`
	// WebhookServiceName is the name of the Service used as part of the DNSName.
	// Defaults to jobset-webhook-service.
	WebhookServiceName *string `json:"webhookServiceName,omitempty"`
	// WebhookSecretName is the name of the Secret used to store CA and server certs.
	// Defaults to jobset-webhook-server-cert.
	WebhookSecretName *string `json:"webhookSecretName,omitempty"`
	}

[andreyvelich]

if one tries to configure the webhook service name and secret name by leveraging Config API, it will not work since the actual name of webhook service and secret is templated by Helm.

I guess, in that case we should duplicate information for Config and for actual k8s resources, like here: https://github.com/kubernetes-sigs/kueue/blob/main/charts/kueue/values.yaml#L148-L157
I just feel that if we don't have Config YAML in the Chart values, it would be hard for user to understand that some Values are going to be inserted into Manager Config.

Maybe you know about better solution ?@astefanutti @tenzen-y @dongjiang1989

[ChenYi015]

For cert-manager, these params are kept to allow users to enable/disable cert-manager and use a custom issuer.

[andreyvelich]

Thank you for this great contribution @ChenYi015!
I think, we can address the remaining comments in the followup PRs.
/lgtm
/assign @kannon92 @ahg-g @tenzen-y

[k8s-ci-robot]

@andreyvelich: changing LGTM is restricted to collaborators

In response to this:

Thank you for this great contribution @ChenYi015!
/lgtm
/assign @kannon92 @ahg-g @tenzen-y

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-sigs/prow repository.

[ahg-g]

Thanks a lot @ChenYi015 and for @andreyvelich for the thorough review and I hope we continue this collaboration and you folks become maintainers on this repo as well if you like :)

I will leave it to @kannon92 to approve since he was the primary maintainer looking at this PR.

[kannon92]

Can you disable this by default?

Kustomize doesn't require this.

We can do this as a follow up.

[kannon92]

/lgtm
/approve

I tested this and other than prometheus being required by default, it seemed to work fine!

We can address the prometheus as a follow up but I was able to test by disabling so I think this is good to go.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChenYi015, kannon92

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 [kannon92]

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

[ahg-g]

/hold cancel

[kannon92]

@ChenYi015 @andreyvelich
Are the follow-ups needed before we release this? I’d like to create a release soon.

[ChenYi015]

I am going to raise another PR today to disable prometheus metrics exporting by default in Helm chart. Besides, I think we need to publish the Helm chart so users can install the chart like:

helm repo add jobset https://kuberentes-sigs.github.io/jobset
helm install jobset jobset/jobset \
      --namespace jobset-system \
      --create-namespace

[andreyvelich]

@ChenYi015 How do we want to publish them ? Similar to KServe and Spark Operator ?

• https://github.com/kserve/kserve/pkgs/container/charts%2Fkserve
• https://github.com/kubeflow/spark-operator/pkgs/container/helm-charts%2Fspark-operator

[kannon92]

cc @ahg-g

im happy to wait until we have a site for the helm chart but not sure what your customers want.

[kannon92]

@tenzen-y do we publish the charts in Kueue anywhere?

[ChenYi015]

How do we want to publish them ? Similar to KServe and Spark Operator ?

• https://github.com/kserve/kserve/pkgs/container/charts%2Fkserve
• https://github.com/kubeflow/spark-operator/pkgs/container/helm-charts%2Fspark-operator

Sorry for the late response. I think there is another way to publish the charts, see #790 (comment).