Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow efs volumes to have multiple mount points in a single service #5631

Conversation

CaptainCarpensir
Copy link
Contributor

@CaptainCarpensir CaptainCarpensir commented Jan 22, 2024

This is the easiest possible fix for the issue, we got an internal ticket requesting a fix for this so I wanted to get a basic fix out for review first and iterate if this causes issues.

Related #2921

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3125ff2) 70.00% compared to head (dce8c6e) 70.02%.
Report is 1 commits behind head on mainline.

Additional details and impacted files
@@             Coverage Diff              @@
##           mainline    #5631      +/-   ##
============================================
+ Coverage     70.00%   70.02%   +0.01%     
============================================
  Files           303      303              
  Lines         46592    46592              
  Branches        299      299              
============================================
+ Hits          32617    32625       +8     
+ Misses        12389    12382       -7     
+ Partials       1586     1585       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Jan 22, 2024

🍕 Here are the new binary sizes!

Name New size (kiB) size (kiB) Delta (%)
macOS (amd) 57212 57132 +0.14
macOS (arm) 58232 58156 +0.13
linux (amd) 50172 50096 +0.15
linux (arm) 49472 49348 +0.25
windows (amd) 47268 47200 +0.14

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall! Have you tested applying this new template to an existing deployed workload and see if the update goes through?

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update the integ test to cover the multiple efs mount points scenario?

@iamhopaul123 iamhopaul123 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jan 22, 2024
@CaptainCarpensir CaptainCarpensir removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jan 23, 2024
@mergify mergify bot merged commit c3e4249 into aws:mainline Jan 23, 2024
12 checks passed
Sprint 🏃‍♀️ automation moved this from In review to Pending release Jan 23, 2024
@wub
Copy link

wub commented Feb 11, 2024

LGTM overall! Have you tested applying this new template to an existing deployed workload and see if the update goes through?

@iamhopaul123 @CaptainCarpensir Hey team, how can I manually apply this update to my existing workload? (I still get the error message ✘ validate manifest against environment "staging": validate "storage": cannot specify more than one managed volume per service)

I've tried to redeploy the pipeline, but maybe that's not the right place. Thanks!

@iamhopaul123
Copy link
Contributor

Hello @wub. This should've been fixed in our latest commit. Can you check in your buildspec.yaml and see if you are using the latest Copilot version there for the build stage?

@wub
Copy link

wub commented Feb 12, 2024

Hey @iamhopaul123, this is the head of our buildspec:

version: 0.2
phases:
  install:
    commands:
      - echo "cd into $CODEBUILD_SRC_DIR"
      - cd $CODEBUILD_SRC_DIR
      # Download the copilot linux binary.
      - wget -q https://ecs-cli-v2-release.s3.amazonaws.com/copilot-linux-v1.33.1 -O copilot-linux

@KollaAdithya
Copy link
Contributor

KollaAdithya commented Feb 13, 2024

Hey @wub

Still Copilot does not support mounting multiple volumes created and managed by Copilot in a single service. This is the enhancement that needs to be done on Copilot end.

But you can configure multiple externally-managed EFS volumes in Copilot. Here is a sample manifest configuration for an external EFS volume.

storage:
  volumes:
    externalvolume1:
      path: '/etc/mount1'
      efs:
        id: fs-1234567
    externalvolume2:
      path: '/etc/mount2'
      efs:
        id: fs-4567891

@KollaAdithya
Copy link
Contributor

KollaAdithya commented Feb 13, 2024

Here is a doc that helps on how you can configure external managed EFS volumes in Copilot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Sprint 🏃‍♀️
  
Pending release
Development

Successfully merging this pull request may close these issues.

None yet

6 participants