-
Notifications
You must be signed in to change notification settings - Fork 618
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
Volume plugin changes to fix EFS unmount #4053
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amogh09
force-pushed
the
volume-plugin-redesign
branch
3 times, most recently
from
December 15, 2023 16:21
19d8343
to
534eac3
Compare
amogh09
changed the title
[WIP] Volume plugin changes to fix EFS unmount
Volume plugin changes to fix EFS unmount
Dec 15, 2023
amogh09
force-pushed
the
volume-plugin-redesign
branch
from
December 18, 2023 17:21
dafa62a
to
9f21058
Compare
bit of a nitpick but for the changelog I would word this as: |
sparrc
reviewed
Dec 18, 2023
sparrc
reviewed
Dec 18, 2023
sparrc
previously approved these changes
Dec 18, 2023
yinyic
reviewed
Dec 19, 2023
amogh09
force-pushed
the
volume-plugin-redesign
branch
from
December 21, 2023 19:14
7fa4f8d
to
7b9ebc4
Compare
amogh09
force-pushed
the
volume-plugin-redesign
branch
from
December 21, 2023 21:13
7e4bf35
to
21e84fb
Compare
yinyic
approved these changes
Dec 21, 2023
prateekchaudhry
approved these changes
Dec 21, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR updates Amazon ECS Volume Plugin, that is used for provisioning and deprovisioning EFS volumes on ECS container instances, so that it mounts and unmounts volumes when
Mount
andUnmount
requests are made to it instead ofCreate
andRemove
requests, respectively.For EFS tasks in awsvpc network mode the task network namespace is torn down at task stop which causes EFS volume unmounting to hang when a
Remove
request is made to the plugin for the task's volumes at task cleanup time later. With this change, EFS volumes will be unmounted from the host when the task is being stopped and while task network namespace is still in place and configured.After this change,
Create
,Mount
,Unmount
, andRemove
requests will be handled by the plugin as described below.Implementation details
Create
method toMount
method ofAmazonECSVolumePlugin
. Added relevant tests inTestPluginMount
function. Also added roll-back steps toMount
method to make sure its effects are atomic.Unmount
method ofAmazonECSVolumePlugin
. Added relevant tests inTestPluginUnmount
function. Also added roll-back steps toUnmount
method to make sure its effects are atomic.Remove
method to make it unmount the volume only if it's currently mounted. Added a newIsMounted
method toVolumeDriver
interface. ImplementedIsMounted
method forECSVolumeDriver
that checks if the volume is present in driver's state.Remove
method ofECSVolumeDriver
to swallow unmount errors caused due to the volume not being mounted on the host. The method already had that logic as it swallows errors with "not mounted" in them but during my testing I found out that error "no mount point specified" is also returned byumount
when the volume is not mounted. This is to makeRemove
method not fail for already unmounted volumes.Create
method ofECSVolumeDriver
to not check its internal state for volume's mount state before mounting the requested volume. This is becauseECSVolumeDriver
does not track active mounts on the volume and an unmounted but not yet removed volume will reappear in its state if plugin restarts and with the check in place the driver will not allow theMount
request handler to perform volume mounts. The check does not server any purpose currently as the driver's internal state is completely controlled byAmazonECSVolumePlugin
that has its own checks to prevent duplicate mount attempts. In future we should makeECSVolumeDriver
completely stateless and have all state handling inAmazonECSVolumePlugin
only but that's out of scope of this PR.Testing
Did extensive manual testing with the new plugin. Tested that -
A new functional test has been added that runs an AWSVPC EFS task twice in succession on an instance with a cleanup duration of 1 second. The test fails on instances with current version of the volume plugin but passes on instances with the version of the volume plugin in this PR.
Ran Agent functional tests on instances with a new version of volume plugin installed. All tests passed.
Added many unit tests.
New tests cover the changes: yes
Description for the changelog
Bugfix: Fix EFS unmount hanging issue for awsvpc tasks
Does this PR include breaking model changes? If so, Have you added transformation functions?
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.