PodMounter: Add Mountpoint Pod Sharing Support by yerzhan7 · Pull Request #439 · awslabs/mountpoint-s3-csi-driver

yerzhan7 · 2025-04-23T08:28:16Z

Issue: #353

Description of changes: This PR adds support for sharing Mountpoint Pods between multiple workload pods when they have compatible configurations. This optimization reduces resource usage by reusing existing Mountpoint Pods instead of creating a new one for each workload pod.

Key Changes:

Custom Resource Definition
- Added MountpointS3PodAttachment CRD to track workload-to-Mountpoint Pod attachments
Controller Changes
- Added reconciler logic to create/manage MountpointS3PodAttachment resources
- Added stale attachment cleaner to handle orphaned attachments
- Implemented expectations pattern to handle eventual consistency
- Added cleanup of resources when pods are terminated
Node Changes
- Modified PodMounter to use shared source mount points and perform bind mounts on target path
- Added dedicated PodUnmounter to handle clean unmounting
- Added needs-unmount annotation handling for graceful pod cleanup
- Added locking mechanisms to prevent race conditions
Testing
- Added e2e and controller tests covering various pod sharing scenarios
- Manual testing performed on personal EKS cluster

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

anurag4DSB · 2025-04-30T09:11:20Z

General question: What is the motivation for transitioning from a systemd-based mounter to a pod-based mounter?
Is the primary reason scalability, or are there other factors driving this change?

unexge · 2025-04-30T09:58:55Z

Hey @anurag4DSB, see #279 and the linked issues for more details on the motivation why we're moving away from systemd mounter. This PR specifically makes progress towards #353.

anurag4DSB · 2025-04-30T10:03:30Z

Hey @anurag4DSB, see #279 and the linked issues for more details on the motivation why we're moving away from systemd mounter. This PR specifically makes progress towards #353.

Thank you so much. Appreciate the prompt response.

unexge

This is a great work @yerzhan7! Left some comments, but feel free to address them as follow-up PRs. I think we can merge this PR as-is.

unexge · 2025-05-02T10:26:48Z

+	var sb strings.Builder
+	for _, k := range keys {
+		sb.WriteString(k)
+		sb.WriteString("=")


nit: You can also use WriteRune for single characters but perf wouldn't matter much here probably. So it's fine to keep it as-is as well

unexge · 2025-05-02T10:32:48Z

+		crdv1beta.FieldAuthenticationSource: authSource,
+	}
+
+	if authSource == "pod" {


Maybe use the constant credentialprovider.AuthenticationSourcePod here? We should probably move these constants to a different package than node/... as now they're also used in controller environment as well but we can do it later

Fixed. Will address moving to different package separately.

unexge · 2025-05-02T10:34:02Z

+	volumeAttributes := mppod.ExtractVolumeAttributes(pv)
+	authSource := volumeAttributes[volumecontext.AuthenticationSource]
+	if authSource == "" {
+		return "driver"


Maybe use credentialprovider.AuthenticationSourceDriver here as well? I feel like defaulting to driver logic should be kept in either credentialprovider or volumecontext package maybe, but we can also address it later

Yeah, this logic is duplicated, will address it separately.

unexge · 2025-05-02T10:39:24Z

+// handleInactivePod handles inactive workload pod.
+func (r *Reconciler) handleInactivePod(ctx context.Context, s3pa *crdv1beta.MountpointS3PodAttachment, workloadUID string, log logr.Logger) (bool, error) {
+	if s3pa == nil {
+		log.Info("Workload pod is not active. Did not find any MountpointS3PodAttachments.")


Should we check if we have a pending expectation here? And if so we should requeue it to ensure it would properly clean up once the expectation is satisfied?

We can add pending expectation check, but most likely if we requeue inactive pod event next time reconciler would just skip it completely because by that time Workload Pod won't be found. I.e. cleanup will be handled by periodic stale attachment cleanup job.

Since handleInactivePod() is "best effort" anyway at the moment I don't think we need this extra check.

Maybe even we don't need handleInactivePod() method at all, and we should rely fully on stale attachment cleaner instead.

unexge · 2025-05-02T12:51:40Z

+	volumeId := mpPod.Labels[mppod.LabelVolumeId]
+
+	if err := u.writeExitFile(podPath); err != nil {
+		return


Would be nice to log errors here, and also maybe move this logic to a separate function as it's probably the same as what unmount does

Added in follow-up #453

unexge · 2025-05-02T12:54:45Z

+		}
+
+		if mpPod == nil {
+			klog.V(4).Infof("Mountpoint Pod not found for UID %s, will unmount and delete folder", mpPodUID)


Wonder if eventual consistency could cause problems here? Maybe we should keep a counter of not founds and delete it only after N times?

I think it should not cause an issue because source Mountpoint is created only during NPV call which waits for for that Pod to be available in local cache. I.e. if the source folder exists (and we do not need to unmount) MP Pod should be in cache already.

Plus, we also wait for full Pod cache sync during driver start up before starting periodic cleanup job.

yerzhan7

Thanks for reviewing! Fixed merge conflicts and resolved small/quick comments in eeb1699

Will address other comments in separate PRs.

yerzhan7 · 2025-05-06T08:13:19Z

+		}
+
+		if mpPod == nil {
+			klog.V(4).Infof("Mountpoint Pod not found for UID %s, will unmount and delete folder", mpPodUID)


I think it should not cause an issue because source Mountpoint is created only during NPV call which waits for for that Pod to be available in local cache. I.e. if the source folder exists (and we do not need to unmount) MP Pod should be in cache already.

Plus, we also wait for full Pod cache sync during driver start up before starting periodic cleanup job.

Add Pod Sharing

e7651b3

yerzhan7 had a problem deploying to approval-gate April 23, 2025 08:28 — with GitHub Actions Failure

unexge reviewed Apr 28, 2025

View reviewed changes

yerzhan7 added 11 commits April 28, 2025 16:44

Add {{- if .Values.experimental.podMounter -}} wrapper to CRD

0ba8d12

Make Expectations private, fix comments

1e1e761

Reconcile fixes

8a53b09

Rename CRD version from v1 to v1beta

75ec928

PodMounter fixes

19ebfa1

Add mppod_lock unit test

161ea2d

Add expectations unit test

3fd56c0

Add PodMounter unit tests

faa33e6

Add PodUnmounter unit tests

5ae555f

Improve PodSharing e2e tests

8fde1c4

Add more e2e tests

87f4edb

yerzhan7 had a problem deploying to approval-gate April 29, 2025 20:30 — with GitHub Actions Failure

yerzhan7 added 2 commits April 30, 2025 15:26

Add attachment time to CRD

75892bd

Add StaleAttachmentCleaner

d192ded

yerzhan7 temporarily deployed to approval-gate April 30, 2025 16:18 — with GitHub Actions Inactive

yerzhan7 temporarily deployed to untrusted April 30, 2025 16:24 — with GitHub Actions Inactive

yerzhan7 had a problem deploying to untrusted April 30, 2025 16:30 — with GitHub Actions Failure

yerzhan7 had a problem deploying to untrusted April 30, 2025 20:35 — with GitHub Actions Failure

yerzhan7 temporarily deployed to untrusted April 30, 2025 20:35 — with GitHub Actions Inactive

yerzhan7 had a problem deploying to untrusted April 30, 2025 20:35 — with GitHub Actions Failure

yerzhan7 temporarily deployed to untrusted April 30, 2025 20:35 — with GitHub Actions Inactive

yerzhan7 had a problem deploying to untrusted April 30, 2025 20:35 — with GitHub Actions Failure

yerzhan7 added 9 commits April 30, 2025 22:17

Fix MountOptions controller test

b6bd5ce

Add needs-unmount annotation logic

183e517

Conditionally support selectable fields

d04d8d1

Add sleep in controller for IRSA role change test

845b508

Merge remote-tracking branch 'upstream/main' into mppodsharing

f224240

Further refactoring, add more doc comments

26dc0d8

go mod tidy

258e0c8

Node: Handle shutdown signal

9dfd816

Merge remote-tracking branch 'upstream/main' into mppodsharing

c52f321

unexge previously approved these changes May 2, 2025

View reviewed changes

yerzhan7 added 3 commits May 2, 2025 14:36

Merge remote-tracking branch 'upstream/main' into mppodsharing

e4bcdb1

Merge remote-tracking branch 'upstream/main' into mppodsharing

ab6e260

Address small comments

eeb1699

yerzhan7 commented May 6, 2025

View reviewed changes

unexge approved these changes May 6, 2025

View reviewed changes

yerzhan7 mentioned this pull request May 6, 2025

Pod sharing fixes and improvements #453

Merged

Conversation

yerzhan7 commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anurag4DSB commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unexge commented Apr 30, 2025

Uh oh!

anurag4DSB commented Apr 30, 2025

Uh oh!

unexge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yerzhan7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yerzhan7 commented Apr 23, 2025 •

edited

Loading

anurag4DSB commented Apr 30, 2025 •

edited

Loading