Adding EBS watcher implementation #3866
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
mye956
changed the title
mye956
force-pushed
the
ebsWatcher
branch
6 times, most recently
from
5266fda to
2e9d9cd
Compare
amogh09
reviewed
Aug 28, 2023
agent/ebs/watcher.go
Outdated
| wg.Add(1) | ||
| w.mailbox <- func() { | ||
| defer wg.Done() | ||
| empty := len(w.agentState.GetAllPendingEBSAttachments()) == 0 |
There was a problem hiding this comment.
can we rename empty to something more specific?
There was a problem hiding this comment.
Good call out, I renamed it to wasEmpty as well as added some comments to better explain the use case. Thanks!
amogh09
reviewed
Sep 1, 2023
agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/api/resource/ebs_discovery.go
Outdated
Show resolved
Hide resolved
mye956
force-pushed
the
ebsWatcher
branch
2 times, most recently
from
b80b2c6 to
bab80b4
Compare
* Added the implementation for EBS volume discovery on Windows * incorporated the review comments. Also fixed the name of the timeout variable. * incorporated the review comments. Also fixed the name of the timeout variable.
amogh09
reviewed
Sep 6, 2023
amogh09
reviewed
Sep 6, 2023
amogh09
reviewed
Sep 6, 2023
agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/api/resource/ebs_discovery.go
Show resolved
Hide resolved
amogh09
reviewed
Sep 6, 2023
agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/api/resource/ebs_discovery.go
Outdated
Show resolved
Hide resolved
amogh09
reviewed
Sep 6, 2023
amogh09
reviewed
Sep 6, 2023
amogh09
previously approved these changes
Sep 7, 2023
fierlion
reviewed
Sep 7, 2023
|
|
||
| // TestHandleExpiredEBSAttachment tests acknowledging an expired resource attachment of type Elastic Block Stores | ||
| // The resource attachment object should not be saved to the agent state since the expiration date is in the past. | ||
| func TestHandleExpiredEBSAttachment(t *testing.T) { |
There was a problem hiding this comment.
Nice to see you catch this in your tests!
Realmonia
reviewed
Sep 8, 2023
mye956
force-pushed
the
ebsWatcher
branch
2 times, most recently
from
508c67d to
d88e6cb
Compare
Realmonia
previously approved these changes
Sep 11, 2023
fierlion
reviewed
Sep 11, 2023
| ) | ||
|
|
||
| const ( | ||
| ebsVolumeDiscoveryTimeout = 5 * time.Second |
There was a problem hiding this comment.
Where did this timeout come from? Based on our data, 5 seconds is not enough to always give EBS time to handle the attachment.
fierlion
reviewed
Sep 11, 2023
|
|
||
| // parseLsblkOutput will parse the `lsblk` output and search for a EBS volume with a specific device name. | ||
| // Once found we return the volume ID, otherwise we return an empty string along with an error | ||
| // The output of the "lsblk -o NAME,SERIAL -J" command looks like the following: |
There was a problem hiding this comment.
thank you overall for the thorough code comments!!
Realmonia
previously approved these changes
Sep 12, 2023
amogh09
approved these changes
Sep 12, 2023
Realmonia
approved these changes
Sep 12, 2023
Closed
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 will introduce the implementation for the EBS watcher which is responsible for detecting and validating that an EBS volume is attached onto the host.
Implementation details
The general workflow is upon agent start up, the EBS watcher gets initialized and will start to periodically check whether we have any pending EBS attachments to find on the host. If there is then it'll scan for all pending EBS volumes within the agent state and try to find if they're attached via
lsblk. Otherwise it's a noop. The watcher will always be running and will stop whenever agent stops.EBS volumes are added to the agent state whenever we receive and handle an attachment message.
watcher.gointoagent/ebsNewWatcher: Create a new instance of a EBSWatcher structStart: Start the periodic scanning process of the EBSWatcher for all pending EBS attachments saved to the agent stateStop: Stop the EBSWatcherHandleResourceAttachment: Handle a new ResourceAttachment object from the Attachment Message Responder/Handler and save it into the agent state if it does not exists.notifyFoundEBS: Mark the pending ResourceAttachment as no longer pending as well as stops the ACK timerRemoveAttachment: Removes a ResourceAttachment object from the list of attachments within the agent statescanEBSVolumes: Iterates through the list of pending ResourceAttachment objects within the agent state and checks if it's attached within the host viaConfirmEBSVolumeIsAttachedaddEBSAttachmentToState: Starts the ACK timer of a ResourceAttachment object as well as add it to the list of attachments within the agent statehandleEBSAckTimer: Handles the when a ResourceAttachment object once its ACK timer expiresecs-agent/api/resource/resource_attachment.goto safely read ResourceAttachment objects within logsEBSDiscoveryinecs-agent/api/resource/interfaces.goEBSDiscoveryClientinecs-agent/api/resource/ebs_discovery.gowhich implements theEBSDiscoveryinterfaceebs_discovery_linux.goConfirmEBSVolumeIsAttached: Calls uponlsblk -o NAME,SERIAL -Jto obtain all attached volumes on the host and validates whether the given device name and volume ID is within the output.parseLsblkOutput: Parses the output oflsblkand then returns the volume ID of a specific device name from the outputebs_discovery_windows.goConfirmEBSVolumeIsAttached: The windows version to find and validate whether a EBS volume is attached on the hostecs-agent/api/resource/generate_mocks.go: Generate new mocks for theEBSDiscoveryinterface for testingNote: There will be a follow up PR for the ACS Attachment message handler/responder that'll also include additional changes to the EBS watcher.
This PR will also include changes made in the following PR mye956#49.
Testing
New tests cover the changes: yes
Unit tests
Description for the changelog
Adding implementation for EBS watcher and EBS volume discovery.
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.