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

Remove resync logic and use workqueue.AddAfter in pv_controller #71761

Open
jingxu97 opened this Issue Dec 5, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@jingxu97
Contributor

jingxu97 commented Dec 5, 2018

What would you like to be added:
Currently pv_controller has its own resync loop which periodically list all PVCs/PVs and put them into the queue. One main reason for this resync is that pv_controller does not add the item back if operation fails, so it relies on resync to put them back to get processed. We should consider to use workqueue_AddAfter method to only add back the object in which the operation fails.

Why is this needed:
Reduce to overhead of resync to add back all PVC/PV objects and process them.

/kind feature

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 5, 2018

@jingxu97: There are no sig labels on this issue. Please add a sig label by either:

  1. mentioning a sig: @kubernetes/sig-<group-name>-<group-suffix>
    e.g., @kubernetes/sig-contributor-experience-<group-suffix> to notify the contributor experience sig, OR

  2. specifying the label manually: /sig <group-name>
    e.g., /sig scalability to apply the sig/scalability label

Note: Method 1 will trigger an email to the group. See the group list.
The <group-suffix> in method 1 has to be replaced with one of these: bugs, feature-requests, pr-reviews, test-failures, proposals.

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.

@jingxu97 jingxu97 added sig/storage and removed needs-sig labels Dec 5, 2018

@wongma7

This comment has been minimized.

Contributor

wongma7 commented Dec 6, 2018

@jsafrane

This comment has been minimized.

Member

jsafrane commented Dec 7, 2018

IMO it would have some impact on PVC binding. If both PVC and PV are not pre-bound to anything, then binding is initiated by an event on PVC. The controller processes an event on PVC (e.g. periodic sync) and looks for a suitable PV and binds them. If there is no PV, it waits for the next periodic sync. Currently, nothing happens if a new PV becomes available so a PVC could be bound now. PVC still waits for periodic sync and gets bound to the new PV only during PVC event processing.

Of course, this can be fixed:

  • Either we add some logic to the controller to bind PVCs when a new (or updated) PV appears.
  • Or we make the workqueue call PVC sync relatively often if a PVC is unbound (~1 minute max?) so it finds newly created PVs quickly, and not so often when the PVC is bound and there is nothing to do. This could be done by smarter workqueue.AddAfter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment