Skip to content
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

Conversation

@mucahitkurt
Copy link
Contributor

commented Apr 1, 2019

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?:

Reduce event spam for AttachVolume storage operation
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@mucahitkurt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

/assign @davidz627

@saad-ali
Copy link
Member

left a comment

/approve

LGTM
/assign @ddebroy @davidz627 @leakingtapan
Will let the CSI migration folks^^^ do final LGTM

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ddebroy

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

/ok-to-test

@mucahitkurt mucahitkurt force-pushed the mucahitkurt:improvement/reduce-event-spam-for-GenerateAttachVolumeFunc branch from db7ccc6 to ab31cd2 Apr 4, 2019

@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Apr 4, 2019

@mucahitkurt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@mucahitkurt mucahitkurt force-pushed the mucahitkurt:improvement/reduce-event-spam-for-GenerateAttachVolumeFunc branch from ab31cd2 to 7c12493 Apr 10, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Apr 10, 2019

@vladimirvivien
Copy link
Member

left a comment

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?

@mucahitkurt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@vladimirvivien I followed the same refactoring pattern that was applied for the GenerateMountVolumeFunc in this PR, so this function(GenerateMountVolumeFunc) also is not returning error.

I also asked about this refactoring on the sig storage mailing list and the answer was The operation generation should just construct the function to execute and not have an option to fail (should not return an error). That way the reconciler can be dumb and continue to execute multiple times per second. Any failure will happen when the operation is actually executed, and will be handled correctly by the exponential back off logic.

I checked the other functions of the interface, the same event spam issue can be valid for the GenerateMapVolumeFunc, other functions just return errors and don't create events.

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 OperationGenerator.

As far as I understand issues were open for the most painful ones.

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

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@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.

@davidz627
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
@davidz627

davidz627 Apr 24, 2019

Contributor

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

This comment has been minimized.

Copy link
@mucahitkurt

mucahitkurt Apr 24, 2019

Author Contributor

Done. Thanks for the comment!

reduce event spam for function GenerateAttachVolumeFunc
Signed-off-by: Mucahit Kurt <mucahitkurt@gmail.com>

@mucahitkurt mucahitkurt force-pushed the mucahitkurt:improvement/reduce-event-spam-for-GenerateAttachVolumeFunc branch from 7c12493 to 1c1da75 Apr 24, 2019

@davidz627

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

/lgtm
Thanks for the fix!

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 24, 2019

@k8s-ci-robot k8s-ci-robot merged commit 88992ba into kubernetes:master Apr 25, 2019

20 checks passed

cla/linuxfoundation mucahitkurt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@mucahitkurt mucahitkurt referenced this pull request Sep 7, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.