Skip to content

Commit

Permalink
Merge pull request #1233 from otorreno/reduce-efs-calls-reuse-ap
Browse files Browse the repository at this point in the history 
Refactor re-use Access Point
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Jul 25, 2024
2 parents df6e2ce + 58c8272 commit 79cb3a5
Show file tree
Hide file tree
Showing 6 changed files with 355 additions and 274 deletions.
35 changes: 9 additions & 26 deletions pkg/cloud/cloud.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,10 @@ type Efs interface {

type Cloud interface {
GetMetadata() MetadataService
CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions, reuseAccessPoint bool) (accessPoint *AccessPoint, err error)
CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions) (accessPoint *AccessPoint, err error)
DeleteAccessPoint(ctx context.Context, accessPointId string) (err error)
DescribeAccessPoint(ctx context.Context, accessPointId string) (accessPoint *AccessPoint, err error)
FindAccessPointByClientToken(ctx context.Context, clientToken, fileSystemId string) (accessPoint *AccessPoint, err error)
ListAccessPoints(ctx context.Context, fileSystemId string) (accessPoints []*AccessPoint, err error)
DescribeFileSystem(ctx context.Context, fileSystemId string) (fs *FileSystem, err error)
DescribeMountTargets(ctx context.Context, fileSystemId, az string) (fs *MountTarget, err error)
Expand Down Expand Up @@ -164,26 +165,8 @@ func (c *cloud) GetMetadata() MetadataService {
return c.metadata
}

func (c *cloud) CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions, reuseAccessPoint bool) (accessPoint *AccessPoint, err error) {
func (c *cloud) CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions) (accessPoint *AccessPoint, err error) {
efsTags := parseEfsTags(accessPointOpts.Tags)

//if reuseAccessPoint is true, check for AP with same Root Directory exists in efs
// if found reuse that AP
if reuseAccessPoint {
existingAP, err := c.findAccessPointByClientToken(ctx, clientToken, accessPointOpts)
if err != nil {
return nil, fmt.Errorf("failed to find access point: %v", err)
}
if existingAP != nil {
//AP path already exists
klog.V(2).Infof("Existing AccessPoint found : %+v", existingAP)
return &AccessPoint{
AccessPointId: existingAP.AccessPointId,
FileSystemId: existingAP.FileSystemId,
CapacityGiB: accessPointOpts.CapacityGiB,
}, nil
}
}
createAPInput := &efs.CreateAccessPointInput{
ClientToken: &clientToken,
FileSystemId: &accessPointOpts.FileSystemId,
Expand Down Expand Up @@ -262,22 +245,22 @@ func (c *cloud) DescribeAccessPoint(ctx context.Context, accessPointId string) (
}, nil
}

func (c *cloud) findAccessPointByClientToken(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions) (accessPoint *AccessPoint, err error) {
klog.V(5).Infof("AccessPointOptions to find AP : %+v", accessPointOpts)
func (c *cloud) FindAccessPointByClientToken(ctx context.Context, clientToken, fileSystemId string) (accessPoint *AccessPoint, err error) {
klog.V(5).Infof("Filesystem ID to find AP : %+v", fileSystemId)
klog.V(2).Infof("ClientToken to find AP : %s", clientToken)
describeAPInput := &efs.DescribeAccessPointsInput{
FileSystemId: &accessPointOpts.FileSystemId,
FileSystemId: &fileSystemId,
MaxResults: aws.Int64(AccessPointPerFsLimit),
}
res, err := c.efs.DescribeAccessPointsWithContext(ctx, describeAPInput)
if err != nil {
if isAccessDenied(err) {
return
return nil, ErrAccessDenied
}
if isFileSystemNotFound(err) {
return
return nil, ErrNotFound
}
err = fmt.Errorf("failed to list Access Points of efs = %s : %v", accessPointOpts.FileSystemId, err)
err = fmt.Errorf("failed to list Access Points of efs = %s : %v", fileSystemId, err)
return
}
for _, ap := range res.AccessPoints {
Expand Down
181 changes: 118 additions & 63 deletions pkg/cloud/cloud_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func TestCreateAccessPoint(t *testing.T) {
testFunc func(t *testing.T)
}{
{
name: "Success - AP does not exist",
name: "Success",
testFunc: func(t *testing.T) {
mockCtl := gomock.NewController(t)
mockEfs := mocks.NewMockEfs(mockCtl)
Expand Down Expand Up @@ -74,63 +74,9 @@ func TestCreateAccessPoint(t *testing.T) {
},
}

describeAPOutput := &efs.DescribeAccessPointsOutput{
AccessPoints: nil,
}

ctx := context.Background()
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(describeAPOutput, nil)
mockEfs.EXPECT().CreateAccessPointWithContext(gomock.Eq(ctx), gomock.Any()).Return(output, nil)
res, err := c.CreateAccessPoint(ctx, clientToken, req, true)

if err != nil {
t.Fatalf("CreateAccessPointFailed is failed: %v", err)
}

if res == nil {
t.Fatal("Result is nil")
}

if accessPointId != res.AccessPointId {
t.Fatalf("AccessPointId mismatched. Expected: %v, Actual: %v", accessPointId, res.AccessPointId)
}

if fsId != res.FileSystemId {
t.Fatalf("FileSystemId mismatched. Expected: %v, Actual: %v", fsId, res.FileSystemId)
}
mockCtl.Finish()
},
},
{
name: "Success - AP already exists",
testFunc: func(t *testing.T) {
mockCtl := gomock.NewController(t)
mockEfs := mocks.NewMockEfs(mockCtl)
c := &cloud{
efs: mockEfs,
}

tags := make(map[string]string)
tags["cluster"] = "efs"

req := &AccessPointOptions{
FileSystemId: fsId,
Uid: uid,
Gid: gid,
DirectoryPerms: directoryPerms,
DirectoryPath: directoryPath,
Tags: tags,
}

describeAPOutput := &efs.DescribeAccessPointsOutput{
AccessPoints: []*efs.AccessPointDescription{
{AccessPointId: aws.String(accessPointId), FileSystemId: aws.String(fsId), ClientToken: aws.String(clientToken), RootDirectory: &efs.RootDirectory{Path: aws.String(directoryPath)}, Tags: []*efs.Tag{{Key: aws.String(PvcNameTagKey), Value: aws.String(volName)}}},
},
}

ctx := context.Background()
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(describeAPOutput, nil)
res, err := c.CreateAccessPoint(ctx, clientToken, req, true)
res, err := c.CreateAccessPoint(ctx, clientToken, req)

if err != nil {
t.Fatalf("CreateAccessPointFailed is failed: %v", err)
Expand Down Expand Up @@ -164,14 +110,10 @@ func TestCreateAccessPoint(t *testing.T) {
DirectoryPerms: directoryPerms,
DirectoryPath: directoryPath,
}
describeAPOutput := &efs.DescribeAccessPointsOutput{
AccessPoints: nil,
}

ctx := context.Background()
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(describeAPOutput, nil)
mockEfs.EXPECT().CreateAccessPointWithContext(gomock.Eq(ctx), gomock.Any()).Return(nil, errors.New("CreateAccessPointWithContext failed"))
_, err := c.CreateAccessPoint(ctx, clientToken, req, true)
_, err := c.CreateAccessPoint(ctx, clientToken, req)
if err == nil {
t.Fatalf("CreateAccessPoint did not fail")
}
Expand All @@ -195,7 +137,7 @@ func TestCreateAccessPoint(t *testing.T) {

ctx := context.Background()
mockEfs.EXPECT().CreateAccessPointWithContext(gomock.Eq(ctx), gomock.Any()).Return(nil, awserr.New(AccessDeniedException, "Access Denied", errors.New("Access Denied")))
_, err := c.CreateAccessPoint(ctx, clientToken, req, false)
_, err := c.CreateAccessPoint(ctx, clientToken, req)
if err == nil {
t.Fatalf("CreateAccessPoint did not fail")
}
Expand Down Expand Up @@ -551,6 +493,119 @@ func TestDescribeAccessPoint(t *testing.T) {
}
}

func TestFindAccessPointByClientToken(t *testing.T) {
var (
fsId = "fs-abcd1234"
accessPointId = "ap-abc123"
clientToken = "token"
path = "/myDir"
Gid int64 = 1000
Uid int64 = 1000
)
testCases := []struct {
name string
testFunc func(t *testing.T)
}{
{
name: "Success - clientToken found",
testFunc: func(t *testing.T) {
mockctl := gomock.NewController(t)
mockEfs := mocks.NewMockEfs(mockctl)
c := &cloud{efs: mockEfs}

output := &efs.DescribeAccessPointsOutput{
AccessPoints: []*efs.AccessPointDescription{
{
AccessPointId: aws.String(accessPointId),
FileSystemId: aws.String(fsId),
ClientToken: aws.String(clientToken),
RootDirectory: &efs.RootDirectory{
Path: aws.String(path),
},
PosixUser: &efs.PosixUser{
Gid: aws.Int64(Gid),
Uid: aws.Int64(Uid),
},
},
},
NextToken: nil,
}

ctx := context.Background()
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(output, nil)
res, err := c.FindAccessPointByClientToken(ctx, clientToken, fsId)
if err != nil {
t.Fatalf("Find Access Point by Client Token failed: %v", err)
}

if res == nil {
t.Fatal("Result is nil")
}

mockctl.Finish()
},
},
{
name: "Success - nil result if clientToken is not found",
testFunc: func(t *testing.T) {
mockctl := gomock.NewController(t)
mockEfs := mocks.NewMockEfs(mockctl)
c := &cloud{efs: mockEfs}

output := &efs.DescribeAccessPointsOutput{
AccessPoints: []*efs.AccessPointDescription{
{
AccessPointId: aws.String(accessPointId),
FileSystemId: aws.String(fsId),
ClientToken: aws.String("differentToken"),
RootDirectory: &efs.RootDirectory{
Path: aws.String(path),
},
PosixUser: &efs.PosixUser{
Gid: aws.Int64(Gid),
Uid: aws.Int64(Uid),
},
},
},
NextToken: nil,
}

ctx := context.Background()
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(output, nil)
res, err := c.FindAccessPointByClientToken(ctx, clientToken, fsId)
if err != nil {
t.Fatalf("Find Access Point by Client Token failed: %v", err)
}

if res != nil {
t.Fatal("Result should be nil. No access point with the specified token")
}

mockctl.Finish()
},
},
{
name: "Fail - Access Denied",
testFunc: func(t *testing.T) {
mockctl := gomock.NewController(t)
mockEfs := mocks.NewMockEfs(mockctl)
c := &cloud{efs: mockEfs}
ctx := context.Background()
mockEfs.EXPECT().DescribeAccessPointsWithContext(gomock.Eq(ctx), gomock.Any()).Return(nil, awserr.New(AccessDeniedException, "Access Denied", errors.New("Access Denied")))
_, err := c.FindAccessPointByClientToken(ctx, clientToken, fsId)
if err == nil {
t.Fatalf("Find Access Point by Client Token should have failed: %v", err)
}

mockctl.Finish()
},
},
}
for _, tc := range testCases {
t.Run(tc.name, tc.testFunc)
}
}

func TestListAccessPoints(t *testing.T) {
var (
fsId = "fs-abcd1234"
Expand Down Expand Up @@ -1024,7 +1079,7 @@ func Test_findAccessPointByPath(t *testing.T) {
tt.prepare(mockEfs)
}

gotAccessPoint, err := c.findAccessPointByClientToken(ctx, tt.args.clientToken, tt.args.accessPointOpts)
gotAccessPoint, err := c.FindAccessPointByClientToken(ctx, tt.args.clientToken, tt.args.accessPointOpts.FileSystemId)
if (err != nil) != tt.wantErr {
t.Errorf("findAccessPointByClientToken() error = %v, wantErr %v", err, tt.wantErr)
return
Expand Down
10 changes: 9 additions & 1 deletion pkg/cloud/fakes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func (c *FakeCloudProvider) GetMetadata() MetadataService {
return c.m
}

func (c *FakeCloudProvider) CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions, usePvcName bool) (accessPoint *AccessPoint, err error) {
func (c *FakeCloudProvider) CreateAccessPoint(ctx context.Context, clientToken string, accessPointOpts *AccessPointOptions) (accessPoint *AccessPoint, err error) {
ap, exists := c.accessPoints[clientToken]
if exists {
if accessPointOpts.CapacityGiB == ap.CapacityGiB {
Expand Down Expand Up @@ -98,6 +98,14 @@ func (c *FakeCloudProvider) DescribeMountTargets(ctx context.Context, fileSystem
return nil, ErrNotFound
}

func (c *FakeCloudProvider) FindAccessPointByClientToken(ctx context.Context, clientToken, fileSystemId string) (accessPoint *AccessPoint, err error) {
if ap, exists := c.accessPoints[clientToken]; exists {
return ap, nil
} else {
return nil, nil
}
}

func (c *FakeCloudProvider) ListAccessPoints(ctx context.Context, fileSystemId string) ([]*AccessPoint, error) {
accessPoints := []*AccessPoint{
c.accessPoints[fileSystemId],
Expand Down
Loading

0 comments on commit 79cb3a5

Please sign in to comment.