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

Cache driver capabilities #241

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

jsafrane
Copy link
Contributor

@jsafrane jsafrane commented Mar 5, 2019

We don't expect that driver capabilities change when the driver is running. In this PR:

  • The provisioner checks capabilities on startup and exits, if the driver does not support dynamic provisioning.
  • It caches all capabilities and check the cached values for snapshot and volume topology support when necessary.

edit: This changes behavior of external-provisioner.

Previously, it was running even if the driver did not support dynamic provisioning and created events for all PVCs that it won't provision them. Users could spot this event easily and talk to cluster admin.

Now users will see just "waiting for a volume to be created, either by external provisioner or manually". The provisioner just dies on startup and it's up to cluster admin to find out what's wrong.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 5, 2019
@k8s-ci-robot k8s-ci-robot requested review from davidz627 and lpabon March 5, 2019 15:48
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 5, 2019
@msau42
Copy link
Collaborator

msau42 commented Mar 9, 2019

If the provisioner just dies on startup, will kubernetes constantly try to restart it? Would that take up more cycles than just having it be idle?

@jsafrane
Copy link
Contributor Author

Yes, Kubernetes will try to restart it (with exponential backoff) and the pod should eventually end up in CrashLoopBackOff state, hoping to get admin's attention. The max backoff is probably 5 minutes, i.e. nothing that I would worry about (for one driver).

Question is if it's the right thing to do. What is the right way how to get admin's attention? Crashing provisioner or event in PVC that this PVC cannot be provisioned?

@msau42
Copy link
Collaborator

msau42 commented Mar 15, 2019

/assign

@msau42
Copy link
Collaborator

msau42 commented Mar 18, 2019

Conclusion:

  • Keep current nice error message (don't exit the sidecar if the driver doesn't support?)
  • for 1.15, we can add capabilities to CSIDriver object so that the PV controller can display the nice error message

@jsafrane jsafrane force-pushed the cache-capabilities branch from 7798ce6 to 9cbcfcf Compare March 19, 2019 11:47
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 19, 2019
@jsafrane
Copy link
Contributor Author

Restored sending of events plugin CONTROLLER_SERVICE capability is not reported instead of exiting.

Copy link
Collaborator

@msau42 msau42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2019
@k8s-ci-robot k8s-ci-robot merged commit d8dfd6a into kubernetes-csi:master Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants