[nikhil3495]

What this PR does / why we need it:

The Flux plugin mounts its memory-backed emptyDir volume
(FluxMemoryVolumeName) at /dev/shm with ReadOnly: true. That volume was
added in #3325 so MPI jobs get the host's shared memory instead of the
container runtime's 64M default. A read-only mount prevents any process from
writing to /dev/shm, which defeats that purpose.

This PR removes .WithReadOnly(true) from the FluxMemoryVolumeName
VolumeMount in EnforceMLPolicy, so the mount is writable. The config-map
and curve-secret mounts remain read-only.

The expected mount in TestFlux is updated to match. The test fails on the
unfixed code, where the diff shows ReadOnly: &true on shared-memory, and
passes with the fix.

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

Checklist:

• Docs included if any changes are user facing

Not applicable: no user-facing docs change.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andreyvelich for approval. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[abhishekKokadwar]

Thanks @nikhil3495. I think this is right, and I ran it to check rather than go on reading alone.

Opinion

The volume is a memory-backed emptyDir mounted at /dev/shm so MPI jobs get more shared memory than the container runtime default. Mounted read-only it cannot do that job, so dropping WithReadOnly(true) is the correct fix. Leaving the config map and curve secret mounts read-only is also right.

What I ran

Unit tests, at 92f2d7d:

	Result
PR code with PR test, go test ./pkg/runtime/framework/plugins/flux/	ok
flux.go from master with the PR test	FAIL: TestFlux, diff shows ReadOnly: &true where nil is expected

So the updated expectation does pin the change.

Behaviour in a cluster. The issue says MPI workloads "will likely fail", so I checked what the mount actually does. In kind (Kubernetes v1.36.1), one Pod with a medium: Memory emptyDir named shared-memory, mounted at /dev/shm the way the plugin does it, plus a container with no volume for comparison:

Container	/proc/mounts	Size	echo x > /dev/shm/probe
readOnly: true (master today)	tmpfs /dev/shm tmpfs ro,...	1.9G	Read-only file system
no readOnly (this PR)	tmpfs /dev/shm tmpfs rw,...	1.9G	succeeds
no volume at all	shm	64M	succeeds

Two things follow. The read-only mount is confirmed to reject writes. And the current behaviour is worse than not having the volume: without it a container still gets a writable 64M /dev/shm, with it the container gets none it can write to. That is a stronger case than the issue makes, and it might be worth adding to the description.

What I did not test

I did not run a Flux TrainJob or an MPI workload, so I have not seen a real job fail on master or pass with this change. The check above covers the mount semantics only. The branch is 21 commits behind master, but GitHub reports it as mergeable and the Flux plugin tests pass on it as is.

One question

The read-only flag came in with #3325. I could not find a reason for it in that PR. If someone who knows the Flux plugin can confirm it was not deliberate, I see nothing else to hold this.

/lgtm

[nikhil3495]

Thanks for testing this in a cluster — that confirms the fix is
correct. You are right that the behaviour without this PR is worse
than having no volume at all, since the container loses a writable
/dev/shm entirely. I checked #3325 as well and could not find a
reason for the read-only flag; it appears to have been added by
mistake.

[google-oss-prow]

@andreyvelich: GitHub didn't allow me to assign the following users: vsoch.

Note that only kubeflow members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

We should keep ReedOnly for Flux.
/assign @vsoch
Could you take a look please?
/hold

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.

[nikhil3495]

Thanks for the context. Could you share the reason why ReadOnly should
be kept for the shared-memory volume? The volume was added in #3325 to
give MPI jobs more shared memory than the 64M container runtime default,
but a read-only mount prevents any writes to /dev/shm entirely — which
is worse than not having the volume at all, as tested above by
abhishekKokadwar. Happy to revert if there is a specific Flux requirement
for read-only, but I could not find one in #3325.

[nikhil3495]

Done — removed the ReadOnly field from the FluxMemoryVolumeName assertion at line 2460 in trainjob_controller_test.go to match the fix in flux.go.

[google-oss-prow]

New changes are detected. LGTM label has been removed.

[abhishekKokadwar]

Thanks @vsoch for the context, and for catching the integration test expectation. I only ran the Flux plugin unit tests in my earlier check and missed that assertion. I checked c8a70b54 and every reference to FluxMemoryVolumeName is now consistent. The Go unit and integration tests have not run on this PR yet, so I am starting them below.

[abhishekKokadwar]

/ok-to-test

[nikhil3495]

Thank you for confirming the fix and for catching the integration
test assertion — I've updated it in c8a70b5 so all references to
FluxMemoryVolumeName are now consistent.

[abhishekKokadwar]

@nikhil3495 the Go unit and integration tests pass now, including the updated assertion. The E2E jobs failed before any test ran: master moved to Go 1.27 in #4217, and this branch is still on 1.26 (go.mod requires go >= 1.27.0 (running go 1.26.0)). A rebase onto master should clear it; the branch rebases cleanly. I will take another look once E2E has run.