-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Reduce event spam for function GenerateAttachVolumeFunc #75986
Reduce event spam for function GenerateAttachVolumeFunc #75986
Conversation
Hi @mucahitkurt. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/assign @davidz627 |
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.
/approve
LGTM
/assign @ddebroy @davidz627 @leakingtapan
Will let the CSI migration folks^^^ do final LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mucahitkurt, saad-ali The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/ok-to-test |
db7ccc6
to
ab31cd2
Compare
/test pull-kubernetes-e2e-gce-device-plugin-gpu |
ab31cd2
to
7c12493
Compare
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.
My major concern with this PR is that it is changing the return signature of GenerateAttachVolumeFunc
to be different from all other methods in the interface, creating a one-of situation. Are the other func generator methods not suffering from the same issue solved here?
@vladimirvivien I followed the same refactoring pattern that was applied for the I also asked about this refactoring on the sig storage mailing list and the answer was I checked the other functions of the interface, the same event spam issue can be valid for the I'm not sure about the above answer about the returning error from those generator functions is valid for all of the methods of the interface As far as I understand issues were open for the most painful ones.
|
@mucahitkurt sounds like this refactoring has been gradually applied and eventually other methods will be changed to follow the same pattern. I am good with that. Thanks for clarifying. |
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.
Thanks for the change! Mostly LGTM
attachableVolumePluginName := unknownAttachableVolumePlugin | ||
attachableVolumePlugin, err := | ||
og.volumePluginMgr.FindAttachablePluginBySpec(volumeToAttach.VolumeSpec) | ||
// It's ok to ignore the error, returning error is not expected from this function |
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.
document this idea in the comment too: https://github.com/kubernetes/kubernetes/pull/75986/files#r277510187
Also are we only using this for the attachableVolumePluginName
now, which we only use for the CompleteFunc
? If so, can we move this code closer to the place where we use the attachableVolumePluginName
- after the attachVolumeFunc
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.
Done. Thanks for the comment!
Signed-off-by: Mucahit Kurt <mucahitkurt@gmail.com>
7c12493
to
1c1da75
Compare
/lgtm |
What type of PR is this?
/kind cleanup
/sig storage
What this PR does / why we need it:
Delete return type error of the function, expand the scope of generated function, delete generated events inside the function(these events will be produced when the generated function is run with rate limiting) to reduce the event spam caused by GenerateAttachVolumFunc.
Which issue(s) this PR fixes:
Fixes #74988
Special notes for your reviewer:
Does this PR introduce a user-facing change?: