feat(mpi): add --disable-mpi-rbac-management flag #1
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,8 +62,9 @@ import ( | |
| ) | ||
|
|
||
| const ( | ||
| FailedDeleteJobReason = "FailedDeleteJob" | ||
| SuccessfulDeleteJobReason = "SuccessfulDeleteJob" | ||
| NoServiceAccountNameReason = "NoServiceAccountName" | ||
|
|
||
| controllerName = "mpijob-controller" | ||
| labelMPIJobName = "mpi-job-name" | ||
|
|
@@ -214,23 +215,25 @@ func (jc *MPIJobReconciler) SetupWithManager(mgr ctrl.Manager, controllerThreads | |
| util.OnDependentFuncs[*corev1.ConfigMap](jc.Scheme, jc.Expectations, &jc.JobController))); err != nil { | ||
| return err | ||
| } | ||
| if !ctlrconfig.Config.DisableMPIRBACManagement { | ||
|
Collaborator
There was a problem hiding this comment. No test fails when this guard is removed, as noted on the watch spec. It is also the one part of the feature whose effect never shows up in the API: a stray RBAC watch just means the operator still needs list and watch on Roles and RoleBindings cluster-wide, which is the thing someone turning this flag on is trying to get rid of. That makes it the piece most likely to regress unnoticed. |
||
| // inject watching for job related Role | ||
| if err = c.Watch(source.Kind[*rbacv1.Role](mgr.GetCache(), &rbacv1.Role{}, | ||
| handler.TypedEnqueueRequestForOwner[*rbacv1.Role](mgr.GetScheme(), mgr.GetRESTMapper(), &kubeflowv1.MPIJob{}, handler.OnlyControllerOwner()), | ||
| util.OnDependentFuncs[*rbacv1.Role](jc.Scheme, jc.Expectations, &jc.JobController))); err != nil { | ||
| return err | ||
| } | ||
| // inject watching for job related RoleBinding | ||
| if err = c.Watch(source.Kind[*rbacv1.RoleBinding](mgr.GetCache(), &rbacv1.RoleBinding{}, | ||
| handler.TypedEnqueueRequestForOwner[*rbacv1.RoleBinding](mgr.GetScheme(), mgr.GetRESTMapper(), &kubeflowv1.MPIJob{}, handler.OnlyControllerOwner()), | ||
| util.OnDependentFuncs[*rbacv1.RoleBinding](jc.Scheme, jc.Expectations, &jc.JobController))); err != nil { | ||
| return err | ||
| } | ||
| // inject watching for job related ServiceAccount | ||
| if err = c.Watch(source.Kind[*corev1.ServiceAccount](mgr.GetCache(), &corev1.ServiceAccount{}, | ||
| handler.TypedEnqueueRequestForOwner[*corev1.ServiceAccount](mgr.GetScheme(), mgr.GetRESTMapper(), &kubeflowv1.MPIJob{}, handler.OnlyControllerOwner()), | ||
| util.OnDependentFuncs[*corev1.ServiceAccount](jc.Scheme, jc.Expectations, &jc.JobController))); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| // skip watching volcano PodGroup if volcano PodGroup is not installed | ||
| if _, err = mgr.GetRESTMapper().RESTMapping(schema.GroupKind{Group: v1beta1.GroupName, Kind: "PodGroup"}, | ||
|
|
@@ -367,24 +370,25 @@ func (jc *MPIJobReconciler) ReconcilePods( | |
| } | ||
| isGPULauncher := isGPULauncher(mpiJob) | ||
|
|
||
| // Get the ConfigMap for this MPIJob. | ||
| if config, err := jc.getOrCreateConfigMap(mpiJob, workerReplicas, isGPULauncher); config == nil || err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if !ctlrconfig.Config.DisableMPIRBACManagement { | ||
| // Get the launcher ServiceAccount for this MPIJob. | ||
| if sa, err := jc.getOrCreateLauncherServiceAccount(mpiJob); sa == nil || err != nil { | ||
| return err | ||
| } | ||
| // Get the launcher Role for this MPIJob. | ||
| if r, err := jc.getOrCreateLauncherRole(mpiJob, workerReplicas); r == nil || err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Get the launcher RoleBinding for this MPIJob. | ||
| if rb, err := jc.getLauncherRoleBinding(mpiJob); rb == nil || err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| worker, err = jc.getOrCreateWorker(mpiJob) | ||
|
|
@@ -1034,8 +1038,13 @@ func (jc *MPIJobReconciler) newLauncher(mpiJob *kubeflowv1.MPIJob, kubectlDelive | |
| jc.PodGroupControl.DecoratePodTemplateSpec(podSpec, mpiJob, rt) | ||
| } | ||
|
|
||
| if !ctlrconfig.Config.DisableMPIRBACManagement { | ||
|
vicoooo26 marked this conversation as resolved.
|
||
| if len(mpiJob.Spec.MPIReplicaSpecs[kubeflowv1.MPIJobReplicaTypeLauncher].Template.Spec.ServiceAccountName) == 0 { | ||
| podSpec.Spec.ServiceAccountName = launcherName | ||
| } | ||
| } else if len(mpiJob.Spec.MPIReplicaSpecs[kubeflowv1.MPIJobReplicaTypeLauncher].Template.Spec.ServiceAccountName) == 0 { | ||
| jc.Recorder.Eventf(mpiJob, corev1.EventTypeWarning, NoServiceAccountNameReason, | ||
|
Collaborator
There was a problem hiding this comment. With the flag on and no Two things make the warning weaker than it first looks. First, Second, and this is the part that surprised me: re-emitting the same warning on every reconcile would not fix it either. client-go's event correlator keys on (involvedObject, reason, message) and patches or suppresses instead of creating a new object. I patched the controller to re-emit on every reconcile to check. With an identical message no new event ever appeared; with a unique message per reconcile it did. So whatever the durable signal ends up being, it has to be a status condition on the MPIJob or a fail-fast (reject in a validating webhook, or return a reconcile error). A repeated event will just get deduped. Either way, it would help to spell out the permissions a user-supplied SA needs: get, list and watch on pods, plus create on pods/exec. Test logs and the mutation matrix behind this: https://github.com/cheyang/trainer/tree/verify/aliyun-trainer-prs/docs/verification/aliyun-trainer-prs |
||
| "Launcher has no ServiceAccountName; ensure the namespace default ServiceAccount has get, list, watch pods and create pods/exec permissions") | ||
| } | ||
|
|
||
| podSpec.Spec.InitContainers = append(podSpec.Spec.InitContainers, corev1.Container{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pin, and the matching ones in
integration-tests.yaml,e2e-test-train-api.yamlandtest-example-notebooks.yaml, fixes a real breakage, and the TODO explaining the Pydantic switch is the kind of comment I wish more pins had. It just has nothing to do with MPI RBAC. Bundled like this, reverting the feature also reverts the CI fix. Worth its own PR so the two can land and be rolled back independently.