-
Notifications
You must be signed in to change notification settings - Fork 619
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
Collect volume metrics and volume clean up for EBS-backed tasks #3929
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
force-pushed
the
ebstacs
branch
15 times, most recently
from
September 27, 2023 21:32
0152e0d
to
071d151
Compare
mye956
changed the title
[WIP] Collect volume metrics for EBS-backed tasks
Collect volume metrics for EBS-backed tasks
Sep 28, 2023
xxx0624
previously approved these changes
Sep 29, 2023
fierlion
requested changes
Oct 2, 2023
fierlion
previously approved these changes
Oct 2, 2023
mye956
force-pushed
the
ebstacs
branch
2 times, most recently
from
October 4, 2023 23:10
896cb0f
to
21af0e0
Compare
mye956
changed the title
Collect volume metrics for EBS-backed tasks
Collect volume metrics and volume clean up for EBS-backed tasks
Oct 4, 2023
Can we please fill out the "Testing" section in the PR description. |
amogh09
reviewed
Oct 6, 2023
mye956
force-pushed
the
ebstacs
branch
3 times, most recently
from
October 7, 2023 00:45
b959f00
to
194e2f0
Compare
amogh09
approved these changes
Oct 9, 2023
fierlion
approved these changes
Oct 9, 2023
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 functionality to collect volume metrics for EBS-backed tasks via the CSI driver as well as task clean up for EBS-backed task.
Implementation details
Changes made to collect Volume metrics in EBS-backed Tasks
Implemented
getEBSVolumeMetrics()
andfetchEBSVolumeMetrics()
where it'll collect the volume metrics viaNodeGetVolumeStats
from the CSI driver for each EBS-backed tasks. ThegetEBSVolumeMetrics()
will be invoked when getting all of the other task and instance metrics inGetInstanceMetrics()
.IsEBSTaskAttachEnabled()
has also been implemented and will scan for any EBS volume configuration within the list of volumes of a task. Although it's being called inAddTask()
within a check, it will temporarily always return false for all tasks (i.e. consider it not EBS-backed task) in order to avoid invoking the CSI driver functionality. This will be fixed and addressed in another PR.List of files changed:
IsEBSTaskAttachEnabled()
NodeGetVolumeStats
on all EBS volumes attached to EBS-backed tasksChanges made to clean up EBS-backed tasks
Implemented
UnstageVolumes
to unmount the host mountpoint for all EBS volumes on a EBS-backed task. This will happen on task clean up.List of files changed:
Testing
Added new unit test.
TestFetchEBSVolumeMetrics
: Test that the newfetchEBSVolumeMetrics
function is working as intended. As a TODO, we'll need to add a unhappy test case as well.TODO: Add unit test for
NodeUnstage
in a follow up PR.Manual testing:
We've manually mounted/staged an EBS volume onto a testing EC2 instance with agent running and tried calling the
getEBSVolumeMetrics
functionality.Agent logs to get volume metrics
CSI driver container NodeGetVolumeStats logs
Agent logs to unstage volume
CSI driver container NodeUnstageVolume logs
New tests cover the changes: Yes
Description for the changelog
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.