Repository: kubernetes-sigs/aws-ebs-csi-driver

Issue:
CSI support attach a volume to multiple instances with amazon EBS Multi-Attach
**Describe the solution you'd like in detail**
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volumes-multi.html



Discussion:
@leakingtapan: /kind feature@varunkumar: Is there any WIP on this?@leakingtapan: Hey, there is no progress on this feature yet as we still need the design for this@fejta-bot: Issues go stale after 90d of inactivity.
Mark the issue as fresh with `/remove-lifecycle stale`.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with `/close`.

Send feedback to sig-testing, kubernetes/test-infra and/or [fejta](https://github.com/fejta).
/lifecycle stale@frittentheke: /remove-lifecycle stale@fejta-bot: Issues go stale after 90d of inactivity.
Mark the issue as fresh with `/remove-lifecycle stale`.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with `/close`.

Send feedback to sig-testing, kubernetes/test-infra and/or [fejta](https://github.com/fejta).
/lifecycle stale@azhurbilo: /remove-lifecycle stale@gzur: > Hey, there is no progress on this feature yet as we still need the design for this

Are there any design discussions happening somewhere else?  @leakingtapan @rubenstolk: Would love to understand if there will be any progress on this?@fejta-bot: Issues go stale after 90d of inactivity.
Mark the issue as fresh with `/remove-lifecycle stale`.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with `/close`.

Send feedback to sig-testing, kubernetes/test-infra and/or [fejta](https://github.com/fejta).
/lifecycle stale@azhurbilo: /remove-lifecycle stale@fejta-bot: Issues go stale after 90d of inactivity.
Mark the issue as fresh with `/remove-lifecycle stale`.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with `/close`.

Send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).
/lifecycle stale@ayberk: /lifecycle frozen

cc @AndyXiangLi @coder0028: Is there any progress? There is an open issue on stackoverflow [https://stackoverflow.com/a/67210073/7755174](https://stackoverflow.com/a/67210073/7755174)@AndyXiangLi: @coder0028 We haven't start on this feature but any help are welcome!@yrsurya: Any update on this? pls let us know@andreazorzetto: Hi everyone, I'd like to learn about any progress on this too@jsravn: Multi-attached EBSs have several limitations. I'm wondering how people are expecting to actually use this? It won't be as simple as attaching a PVC to multiple pods:

> Standard file systems, such as XFS and EXT4, are not designed to be accessed simultaneously by multiple servers, such as EC2 instances. Using Multi-Attach with a standard file system can result in data corruption or loss, so this is not safe for production workloads. You can use a clustered file system to ensure data resiliency and reliability for production workloads.

> Multi-Attach enabled volumes do not support I/O fencing. I/O fencing protocols control write access in a shared storage environment to maintain data consistency. Your applications must provide write ordering for the attached instances to maintain data consistency.

From https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volumes-multi.html.

Seems like it would only be useful for specialized purposes - if people tried using multi-attached EBSs as shared PVCs with typical filesystems they are going to run into a world of pain.
@jbg: Those caveats are common to multiple writers on any shared block storage aren't they?

Our application is to write once, detach the writer and then attach read only to many. So a clustered FS is not needed.@ksliu58: +1 to @jsravn above. 

What use cases is everyone trying to solve? How you plan to use it?@KeenanLawrence: We use [Tekton](https://tekton.dev/) as part of our CI/CD system.
Tekton allows you to create [workspaces](https://tekton.dev/vault/pipelines-v0.14.3/workspaces/#overview) using PVCs. When a pipeline executes, Tekton creates an affinity assistant if the pipeline requires a workspace. This affinity assistant is used to run all pods that the tasks in the pipeline create _on the same instance_ that the EBS volume is bound to, for obvious reasons. 
During scheduling, the affinity assistant resource requests are extremely small and it is unaware of the resource requests of the pods that the pipeline will create. So when our build nodes are close to capacity, and the affinity assistant is scheduled to one of these nodes, the subsequent tasks take a long time to provision as it needs to wait for resources to free up.

If we had multi-attach EBS, we would have no need for the affinity assistant (our pod templates would restrict it to running in the same zone, of course) and this would improve our build times and even out our EC2 utilisation.@jsravn: > Those caveats are common to multiple writers on any shared block storage aren't they?
> 
> Our application is to write once, detach the writer and then attach read only to many. So a clustered FS is not needed.

Even ext4 writes back to the journal on readonly mounts. So you're playing with fire and risking your data in a major way. Not a good idea.

> If we had multi-attach EBS, we would have no need for the affinity assistant (our pod templates would restrict it to running in the same zone, of course) and this would improve our build times and even out our EC2 utilisation.

No, multi-attach doesn't give you this. It's shared block storage, not a clustered file system that supports multiple writers. Maybe you want K8s to manage/setup something like Redhat GFS for you?@jbg: > Even ext4 writes back to the journal on readonly mounts. So you're playing with fire and risking your data in a major way. Not a good idea.

"playing with fire and risking your data in a major way" is FUD. It's *completely fine* to attach multiple readers (with no writers) in a way that is entirely safe:

* Ensure the filesystem was unmounted cleanly
* Make the underlying block device read-only (not strictly needed, but helps avoid mistakes)
* Mount with `ro,noload` (`noload` is safe because there is no journal to replay — the filesystem was unmounted cleanly)

If you can't ensure these constraints then don't do it. But it's perfectly safe if done properly.@jbg: > I personally wouldn't risk disabling journalling, even read-only, in the general case (due to nodes crashing/EBS's hanging/etc.)

There are no writes to the journal on a read-only mount even if nodes crash, the block storage hangs, or anything else. The only writes that will ever occur on a read-only mount are on startup to replay the journal, *if and only if the filesystem was not unmounted cleanly by the most recent read/write mount*.

With extfs there are enormous risks of having multiple readers *if any writer exists*, but (assuming the most recent read/write mount was unmounted cleanly) there are zero risks of multiple readers *if no writer exists*. It's not that niche a case, honestly.

I would think that this driver should focus on the block storage part? As far as I understand it, `fsType` can be anything supported by the node, so for those that *do* need read/write, quite a few Linux clustered filesystems should immediately be possible if this driver can supply the multi-attached block storage.@jsravn: Sorry, deleted my comment because I felt I was going way off topic.

I agree the driver should just focus on the block storage part.

AFAIK there is no "fsType" that would support it right now - they usually all require supporting infrastructure like Redhat DLM for GFS. There is also the question of I/O fencing which multi-attach doesn't support. It's likely more work will have to be done on the K8s storage side to provide the functionality people are expecting.

I'd think right now this would just be used in block storage mode and users have to manage it themselves by setting up the infra needed. Or carefully manage the mount lifecycle like you suggest.@jeevanragula: Any updates on this issue? 
Are there plans to add this feature of supporting multiAttach in the CSI driver?
I see there is a PR raised but in a closed state now.
https://github.com/kubernetes-sigs/aws-ebs-csi-driver/pull/1177
If needed, I can take up and raise a PR.
@daecabhir: Is there a particular reason why, for pods requiring a standard filesystem, that EFS / NFS is not a viable solution to the problem. It seems to be way less complicated, have far fewer pre-conditions (e.g. pre-building volumes, ensuring clean unmouts, etc.) and is fully supported by EKS, EKS Fargate and standalone Kube. Multi-attach is provided largely to meet very specific use cases where software needs concurrent access to manage a block device / filesystem, whereas EFS is really intended to address the broader generic shared filesystem functionality.@sebastianlzy: > Is there a particular reason why, for pods requiring a standard filesystem, that EFS / NFS is not a viable solution to the problem. It seems to be way less complicated, have far fewer pre-conditions (e.g. pre-building volumes, ensuring clean unmouts, etc.) and is fully supported by EKS, EKS Fargate and standalone Kube. Multi-attach is provided largely to meet very specific use cases where software needs concurrent access to manage a block device / filesystem, whereas EFS is really intended to address the broader generic shared filesystem functionality.

One common reason I had in using EBS multi-attach instead of EFS is cost (On a cost perspective only, EFS is ~2-3x more expensive than EBS - io2).  In addition, I will also like to perform a lift and shift migration to EKS from VM, replicating what the workload have been using in VMs (multiple EC2 to 1 EBS volume). These workloads are mainly read-only @bruno-chavez: Another use case for this feature is enabling persistent volumes for Windows worloads, FSx for Windows File Server is not always a choice since you need an Active Directory Domain to even deploy a FSx file system.@tom10271: https://docs.aws.amazon.com/ebs/latest/userguide/ebs-volumes-multi.html#considerations

Latest io1 EBS supports I/O Fencing


> Multi-Attach enabled io2 volumes support I/O fencing. I/O fencing protocols control write access in a shared storage environment to maintain data consistency. Your applications must provide write ordering for the attached instances to maintain data consistency. For more information, see [Use NVMe reservations with Multi-Attach enabled Amazon EBS volumes](https://docs.aws.amazon.com/ebs/latest/userguide/nvme-reservations.html).

Solution patch:
diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go
index 97a1888b..1e37dc00 100644
--- a/pkg/driver/controller.go
+++ b/pkg/driver/controller.go
@@ -107,7 +107,10 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 	}
 
 	if !isValidVolumeCapabilities(volCaps) {
-		return nil, status.Error(codes.InvalidArgument, "Volume capabilities not supported")
+		modes := util.GetAccessModes(volCaps)
+		stringModes := strings.Join(*modes, ", ")
+		errString := "Volume capabilities " + stringModes + " not supported. Only AccessModes[ReadWriteOnce] supported."
+		return nil, status.Error(codes.InvalidArgument, errString)
 	}
 
 	disk, err := d.cloud.GetDiskByName(ctx, volName, volSizeBytes)
@@ -256,7 +259,10 @@ func (d *controllerService) ControllerPublishVolume(ctx context.Context, req *cs
 
 	caps := []*csi.VolumeCapability{volCap}
 	if !isValidVolumeCapabilities(caps) {
-		return nil, status.Error(codes.InvalidArgument, "Volume capability not supported")
+		modes := util.GetAccessModes(caps)
+		stringModes := strings.Join(*modes, ", ")
+		errString := "Volume capabilities " + stringModes + " not supported. Only AccessModes[ReadWriteOnce] supported."
+		return nil, status.Error(codes.InvalidArgument, errString)
 	}
 
 	if !d.cloud.IsExistInstance(ctx, nodeID) {
diff --git a/pkg/util/util.go b/pkg/util/util.go
index 7a51a90b..7e5b79a3 100644
--- a/pkg/util/util.go
+++ b/pkg/util/util.go
@@ -23,6 +23,8 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+
+	csi "github.com/container-storage-interface/spec/lib/go/csi"
 )
 
 const (
@@ -78,3 +80,13 @@ func ParseEndpoint(endpoint string) (string, string, error) {
 func roundUpSize(volumeSizeBytes int64, allocationUnitBytes int64) int64 {
 	return (volumeSizeBytes + allocationUnitBytes - 1) / allocationUnitBytes
 }
+
+// GetAccessModes returns a slice containing all of the access modes defined
+// in the passed in VolumeCapabilities.
+func GetAccessModes(caps []*csi.VolumeCapability) *[]string {
+	modes := []string{}
+	for _, c := range caps {
+		modes = append(modes, c.AccessMode.GetMode().String())
+	}
+	return &modes
+}



Test patch:
diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go
index 7904c9e1..9a460173 100644
--- a/pkg/driver/controller_test.go
+++ b/pkg/driver/controller_test.go
@@ -152,6 +152,13 @@ func TestCreateVolume(t *testing.T) {
 			},
 		},
 	}
+	invalidVolCap := []*csi.VolumeCapability{
+		{
+			AccessMode: &csi.VolumeCapability_AccessMode{
+				Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_SINGLE_WRITER,
+			},
+		},
+	}
 	stdVolSize := int64(5 * 1024 * 1024 * 1024)
 	stdCapRange := &csi.CapacityRange{RequiredBytes: stdVolSize}
 	stdParams := map[string]string{}
@@ -1223,6 +1230,45 @@ func TestCreateVolume(t *testing.T) {
 				}
 			},
 		},
+		{
+			name: "fail with invalid volume access modes",
+			testFunc: func(t *testing.T) {
+				req := &csi.CreateVolumeRequest{
+					Name:               "vol-test",
+					CapacityRange:      stdCapRange,
+					VolumeCapabilities: invalidVolCap,
+					Parameters: map[string]string{
+						VolumeTypeKey: cloud.VolumeTypeIO1,
+						"unknownKey":  "unknownValue",
+					},
+				}
+
+				ctx := context.Background()
+
+				mockCtl := gomock.NewController(t)
+				defer mockCtl.Finish()
+
+				mockCloud := mocks.NewMockCloud(mockCtl)
+
+				awsDriver := controllerService{
+					cloud:         mockCloud,
+					driverOptions: &DriverOptions{},
+				}
+
+				_, err := awsDriver.CreateVolume(ctx, req)
+				if err == nil {
+					t.Fatalf("Expected CreateVolume to fail but got no error")
+				}
+
+				srvErr, ok := status.FromError(err)
+				if !ok {
+					t.Fatalf("Could not get error status code from error: %v", srvErr)
+				}
+				if srvErr.Code() != codes.InvalidArgument {
+					t.Fatalf("Expect InvalidArgument but got: %s", srvErr.Code())
+				}
+			},
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go
index e560e5f8..fd285d64 100644
--- a/pkg/util/util_test.go
+++ b/pkg/util/util_test.go
@@ -18,7 +18,10 @@ package util
 
 import (
 	"fmt"
+	"reflect"
 	"testing"
+
+	csi "github.com/container-storage-interface/spec/lib/go/csi"
 )
 
 func TestRoundUpBytes(t *testing.T) {
@@ -125,3 +128,26 @@ func TestParseEndpoint(t *testing.T) {
 	}
 
 }
+
+func TestGetAccessModes(t *testing.T) {
+	testVolCap := []*csi.VolumeCapability{
+		{
+			AccessMode: &csi.VolumeCapability_AccessMode{
+				Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+			},
+		},
+		{
+			AccessMode: &csi.VolumeCapability_AccessMode{
+				Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_READER_ONLY,
+			},
+		},
+	}
+	expectedModes := []string{
+		"SINGLE_NODE_WRITER",
+		"SINGLE_NODE_READER_ONLY",
+	}
+	actualModes := GetAccessModes(testVolCap)
+	if !reflect.DeepEqual(expectedModes, *actualModes) {
+		t.Fatalf("Wrong values returned for volume capabilities. Expected %v, got %v", expectedModes, actualModes)
+	}
+}