feat(mpi): add --disable-mpi-rbac-management flag

[vicoooo26]

What this PR does / why we need it:

Adds a new --disable-mpi-rbac-management CLI flag (default: false) to the MPIJob controller. When set to true, the controller will:

• Not create ServiceAccount, Role, or RoleBinding for MPIJob launchers
• Not inject ServiceAccountName into the launcher pod template
• Not watch RBAC resources (Role, RoleBinding, ServiceAccount)

This allows operators to run with reduced privileges and supports multi-tenant environments where users pre-provide their own ServiceAccount with the necessary permissions (pods get/list/watch, pods/exec create) via spec.serviceAccountName in the launcher pod template.

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #

Checklist:

- [ ] Docs included if any changes are user facing

[vicoooo26]

@cheyang, could you please review this PR? Thanks in advance.

[cheyang]

Reviewed the feature end to end and ran the MPI envtest suite against this PR head — the added “disabled RBAC management” specs pass. The gating is consistent across the three sites (watch setup, ReconcilePods, launcher SA injection), and since podSpec is a DeepCopy of the launcher template, a user-supplied serviceAccountName is correctly preserved when the flag is on. Two things worth addressing before merge, plus a docs gap (inline).

[Copilot]

Pull request overview

Adds a new --disable-mpi-rbac-management flag to allow the MPIJob controller to run without creating/watching launcher RBAC resources, enabling reduced-privilege / multi-tenant deployments where users provide their own ServiceAccount.

Changes:

• Gate RBAC watches (Role/RoleBinding/ServiceAccount) behind DisableMPIRBACManagement.
• Skip launcher ServiceAccount/Role/RoleBinding reconciliation and ServiceAccountName injection when RBAC management is disabled.
• Add CLI/config plumbing and controller tests covering the disabled-RBAC behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
pkg/controller.v1/mpi/mpijob_controller.go	Conditionally disables RBAC watches, RBAC resource creation, and SA name injection based on the new config flag.
pkg/controller.v1/mpi/mpijob_controller_test.go	Adds envtest coverage verifying no RBAC resources are created and controller still reconciles correctly when disabled.
pkg/config/config.go	Adds DisableMPIRBACManagement to shared controller config.
cmd/training-operator.v1/main.go	Wires up the --disable-mpi-rbac-management CLI flag.

Comments suppressed due to low confidence (1)

pkg/controller.v1/mpi/mpijob_controller_test.go:711

• This test resets DisableMPIRBACManagement to false unconditionally. Restoring the previous value reduces coupling to global state and avoids surprising behavior if the suite default ever changes.

			ctlrconfig.Config.DisableMPIRBACManagement = true
			defer func() {
				ctlrconfig.Config.DisableMPIRBACManagement = false
			}()

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[cheyang]

With the flag on and no serviceAccountName on the launcher, this branch emits a warning but leaves podSpec.Spec.ServiceAccountName empty. The launcher then runs under the namespace default SA, which on a stock cluster cannot do what it needs. I checked on ACK v1.36.1-aliyun.1 with kubectl auth can-i --as=system:serviceaccount:default:default: get, list and watch on pods all return no, and so does create on pods/exec. So the MPIJob is admitted happily and then dies at runtime when the launcher tries to exec into the workers.

Two things make the warning weaker than it first looks.

First, newLauncher is the only caller (line 400, when the launcher Pod is created), so the event fires exactly once. After it ages out of the API server, or if the operator is upgraded while a launcher already exists, nothing points at the misconfiguration any more. There is no status condition, and the Pod spec looks unremarkable.

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

[cheyang]

I removed the if !ctlrconfig.Config.DisableMPIRBACManagement guard in SetupWithManager (line 218), so the Role, RoleBinding and ServiceAccount watches get registered even with the flag on. This spec stayed green.

What it actually asserts is that no RBAC objects are created, and that the ConfigMap and Pod watches still work. Both are worth having. Neither says anything about watch registration. The comment inside the spec is candid that this is structural rather than behavioral; the name is not, and a refactor that quietly drops the guard would sail through CI.

Either rename it to match what it checks, or make it behavioral by asserting on the manager's watch set or informer cache contents.

Credit where it is due: the rest of the added coverage does bite. When I removed the ReconcilePods guard instead, two of the three specs failed, exactly as they should.

[cheyang]

This pin, and the matching ones in integration-tests.yaml, e2e-test-train-api.yaml and test-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.

[cheyang]

Two thirds more memory for an e2e job, in a PR about RBAC. If this is working around OOMKills, a sentence in the commit message would save the next person the archaeology. It probably belongs with the CI changes rather than the feature.

[cheyang]

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.

[cheyang]

Asserting that a warning is recorded locks in today's behavior, which is the behavior I would like to see change. Once there is a status condition or a fail-fast, the test worth keeping is one that pins the launcher never starting silently with an empty ServiceAccountName.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

.github/workflows/test-python.yaml:35

• This workflow pinning (e.g., "kubernetes<36") and the related TODO about SDK deserialization are significant, but they’re not mentioned in the PR description/title (which focus on the MPI RBAC flag). Please either update the PR description to call out these CI/dependency changes (and why they’re needed) or split them into a separate PR to keep review scope clear.

          # TODO: kubernetes 36.0.0 switched to Pydantic models, breaking the
          # FakeResponse deserialization hack in the SDK unit tests. To remove
          # this pin, either adapt FakeResponse in
          # sdk/python/kubeflow/training/utils/utils.py for Pydantic or mock
          # ApiClient.deserialize directly in the test suite.

.github/workflows/test-example-notebooks.yaml:31

• The new fastjsonschema pin is introduced without any rationale. Unexplained dependency pins tend to get removed later and regress CI; please add a short TODO/comment with a link to an issue describing the breakage this pin addresses.

      - name: Install Python Dependencies
        run: |
          pip install papermill==2.6.0 jupyter==1.1.1 ipykernel==6.29.5 "kubernetes<36" "fastjsonschema<2.22"

sdk/python/test/e2e/test_e2e_jaxjob.py:160

• Hard-coding the JAXJob memory limit to 5Gi can make the e2e test brittle across different CI cluster sizes. Consider making the memory limit configurable via an env var (similar to JAX_JOB_IMAGE) so CI can tune it without editing the test.

    return V1Container(
        name=CONTAINER_NAME,
        image=os.getenv("JAX_JOB_IMAGE", "docker.io/kubeflow/jaxjob-dist-spmd-mnist:latest"),
        resources=V1ResourceRequirements(limits={"memory": "5Gi", "cpu": "1.2"}),
    )

[cheyang]

/lgtm
/approve