-
Notifications
You must be signed in to change notification settings - Fork 602
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
[Windows] refactor stats package to fetch EBS Volume stats for Windows #4217
base: feature/ebs-windows
Are you sure you want to change the base?
Conversation
Presently, the code path in stats package of the agent, which invokes the CSI Client to fetch volume metrics is limited to Linux platform only. The same code path can be used for Windows as well. This change refactors the existing code to ensure that the EBS volume stats are collected for Windows as well in addition to Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We just need to ensure that the downstream method within csi client is implemented and tested for Windows.
derivedCtx, cancel := context.WithTimeout(engine.ctx, time.Second*1) | ||
// releases resources if GetVolumeMetrics finishes before timeout | ||
defer cancel() | ||
return engine.csiClient.GetVolumeMetrics(derivedCtx, volumeId, hostPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method implemented for Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the code path is common for both Linux and Windows-
func (cc *csiClient) GetVolumeMetrics(ctx context.Context, volumeId string, hostMountPath string) (*Metrics, error) { |
GetVolumeMetrics
depends upon few methods-
NodeGetVolumeStats
fromCSI Driver
which is dependent upon the following-PathExists
inmounter
packageIsBlockDevice
inutils
packagegetBlockSizeBytes
fromCSI Driver
GetMetrics
fromCSI Driver
getFsInfo
Info
method fromfs
package
From the list above, the following methods are implemented as part of the PR you raised- #4214
PathExists
getBlockSizeBytes
Info
IsBlockDevice
method is part of #4224.
func (engine *DockerStatsEngine) getEBSVolumeMetrics(taskArn string) []*ecstcs.VolumeMetric { | ||
task, err := engine.resolver.ResolveTaskByARN(taskArn) | ||
if err != nil { | ||
logger.Error(fmt.Sprintf("Unable to get corresponding task from dd with task arn: %s", taskArn)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we ignoring the error intentionally? If so, can we log the error as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from the existing code which has been refactored into platform agnostic file.
Also, the logger.Error
method does log the error via logrus. Reference-
func Error(message string, fields ...Fields) { |
Summary
Presently, the code path in
stats package
of the agent, which invokes the CSI Client to fetch volume metrics, is limited to Linux platform only. The same code path can be used for Windows as well.This change refactors the existing code to ensure that the EBS volume stats are collected for Windows as well in addition to Linux.
Implementation details
Refactor the code from Linux specific files to common platform files.
Testing
Using unit and integration tests for Windows and Linux.
New tests cover the changes: N/A
Description for the changelog
refactor stats package to fetch EBS Volume stats for Windows
Does this PR include breaking model changes? If so, Have you added transformation functions?
NoLicensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.