Allow for one to install jobset in a different namespace #719
Conversation
k8s-ci-robot
added
kind/bug
k8s-ci-robot
added
the
size/S
✅ Deploy Preview for kubernetes-sigs-jobset canceled.
|
There was a problem hiding this comment.
Thanks @kannon92!
It will help us to unblock this TODO: https://github.com/kubeflow/training-operator/blob/master/manifests/v2/overlays/standalone/kustomization.yaml#L10
| return cert.AddRotator(mgr, &cert.CertRotator{ | ||
| SecretKey: types.NamespacedName{ | ||
| Namespace: secretNamespace, | ||
| Name: secretName, | ||
| Namespace: *cfg.Namespace, |
There was a problem hiding this comment.
@tenzen-y @kannon92 I am wondering is there any specific reason that we want to configure namespace in the Config rather than taking it from the Pod's /var/run/secrets/kubernetes.io/serviceaccount/namespace ?
Our internal cert manager should always create certificates for the service in the same namespace where manager is deployed.
There was a problem hiding this comment.
We are technically doing that.
https://github.com/kubernetes-sigs/jobset/blob/main/api/config/v1alpha1/defaults.go#L58
This is setting the namespace based on the serviceAccount. And then we use that value for certRotation.
There was a problem hiding this comment.
Yeah, but why do we allow users to change the namespace in the Config ?
There was a problem hiding this comment.
Ah I see. yea that does seem to be concerning. The most likely reason for this is that was what we did in Kueue. The component config work was mostly a copy of Kueue and it looks like Kueue has the same config for namespace. By default, it will pick namespace based on serviceAccount.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah, Kueue has similar config. I think, we should remove it, if user should not set other namespace there.
For instance, in Katib we don't allow to set that value for CertGenerator config: https://github.com/kubeflow/katib/blob/master/pkg/apis/config/v1beta1/types.go#L86-L98.
Thoughts @tenzen-y @ahg-g @danielvegamyhre ?
There was a problem hiding this comment.
IMO this is not really related to this issue/PR.
We have a real problem where folks can't run jobset if it is installed outside of jobset-system. I'm happy to discuss this but I think it belongs as a separate issue.
There was a problem hiding this comment.
Yeah, sure! I am happy to discuss it as a followup.
|
/lgtm |
|
@andreyvelich: changing LGTM is restricted to collaborators In response to this:
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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/cherry-pick release-0.7 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Allow for one to change the namespace of jobset.
Which issue(s) this PR fixes:
Fixes #713
Special notes for your reviewer:
Does this PR introduce a user-facing change?