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
STOR-1422: Add field for enabling EFS volume metrics #1853
base: master
Are you sure you want to change the base?
Conversation
@bertinatto: This pull request references STOR-1422 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.16.0" version, but no target version was set. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@bertinatto: This pull request references STOR-1422 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Hello @bertinatto! Some important instructions when contributing to openshift/api: |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bertinatto The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
29ad7e3
to
e01c9c8
Compare
// Requires efsVolumeMetricsOptIn to be enabled to take effect. | ||
// The default refresh period in the EFS CSI driver is 240 minutes. | ||
// +optional | ||
EFSVolumeMetricsRefreshPeriod string `json:"efsVolumeMetricsRefreshPeriod,omitempty"` |
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.
I originally used float64
, but it failed the validation invoked by make update
and suggested to use string
instead.
ddf6739
to
c0f376b
Compare
openapi/openapi.json
Outdated
"format": "int32" | ||
}, | ||
"efsVolumeMetricsOptIn": { | ||
"description": "efsVolumeMetricsOptIn enables metrics for EFS CSI Driver volumes. When enabled, the CSI driver traverses the entire volume to calculate and return metrics. This operation can significantly impact system performance due to the extensive volume walk required. Use with caution in performance-sensitive environments.", |
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.
"description": "efsVolumeMetricsOptIn enables metrics for EFS CSI Driver volumes. When enabled, the CSI driver traverses the entire volume to calculate and return metrics. This operation can significantly impact system performance due to the extensive volume walk required. Use with caution in performance-sensitive environments.", | |
"description": "efsVolumeMetricsOptIn enables metrics for EFS CSI Driver volumes. When enabled, the CSI driver traverses the entire volume to calculate and return volume usage metrics. This operation can significantly impact system performance due to the extensive volume walk required. Use with caution in performance-sensitive environments.", |
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.
I would even add names of the metrics.
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.
I am reluctant to include metric names because upstream changes—such as adding, removing, or modifying existing names—could lead to outdated documentation?
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.
Ack.
LGTM from storage point of view
c0f376b
to
7d80272
Compare
7d80272
to
312a6cb
Compare
cbb3e13
to
2ae2bfc
Compare
2ae2bfc
to
41ea9c3
Compare
@bertinatto: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
CC @openshift/storage