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

feat(zfspv): pvc should be bound only if volume has been created. #121

Merged
merged 1 commit into from
May 21, 2020

Conversation

pawanpraka1
Copy link
Contributor

@pawanpraka1 pawanpraka1 commented May 19, 2020

fixes: #111

Signed-off-by: Pawan pawan@mayadata.io

Why is this PR required? What issue does it fix?:

The controller does not check whether the volume has been created or not
and return successful. Which in turn binds the pvc to the pv.

What this PR does?:

The PVC should not bound until corresponding zfs volume has been created.
Now controller will check the ZFSVolume CR state to be "Ready" before returning
successful. The CSI will retry the CreateVolume request when it will get
a error reply and when the ZFS node agent creates the ZFS volume and sets the
ZFSVolume CR state to be "Ready", the controller will return success for the
CreateVolume Request and then PVC will be bound.

Checklist:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on

@pawanpraka1 pawanpraka1 added enhancement Add new functionality to existing feature pr/hold-review hold the review. labels May 19, 2020
@pawanpraka1 pawanpraka1 added this to the v0.8.0 milestone May 19, 2020
@pawanpraka1 pawanpraka1 requested a review from kmova May 19, 2020 15:00
@pawanpraka1 pawanpraka1 force-pushed the pvcBound branch 2 times, most recently from bfdc6df to f29006a Compare May 19, 2020 20:20
@pawanpraka1 pawanpraka1 removed the pr/hold-review hold the review. label May 19, 2020
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2020

Codecov Report

Merging #121 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #121   +/-   ##
=======================================
  Coverage   22.90%   22.90%           
=======================================
  Files          14       14           
  Lines         489      489           
=======================================
  Hits          112      112           
  Misses        376      376           
  Partials        1        1           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9118f56...d36694e. Read the comment docs.

@@ -0,0 +1 @@
enhancement to bound the pvc only if volume has been created
Copy link
Member

Choose a reason for hiding this comment

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

n_ : bound -> bind

Copy link
Member

Choose a reason for hiding this comment

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

Fixes an issue where ZFS PV with an invalid pool was bound to PVC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the description.

pkg/zfs/volume.go Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
Fixes an issue where PVC was bound to PV because of invalid/wrong parameters in PVC/Storageclass.
Copy link
Member

Choose a reason for hiding this comment

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

n_ : Fixes an issue where PVC was bound to invalid PV created using incorrect values provided in PVC/Storageclass.

Need to find a better word for "invalid" to qualify this PV.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made it unusable.
Fixes an issue where PVC was bound to unusable PV created using incorrect values provided in PVC/Storageclass.

The controller does not check whether the volume has been created or not
and return successful. Which in turn binds the pvc to the pv.

The PVC should not bound until corresponding zfs volume has been created.
Now controller will check the ZFSVolume CR state to be "Ready" before returning
successful. The CSI will retry the CreateVolume request when it will get
a error reply and when the ZFS node agent creates the ZFS volume and sets the
ZFSVolume CR state to be "Ready", the controller will return success for the
CreateVolume Request and then PVC will be bound.

Signed-off-by: Pawan <pawan@mayadata.io>
@kmova kmova merged commit 25d1f1a into openebs:master May 21, 2020
@pawanpraka1 pawanpraka1 deleted the pvcBound branch May 21, 2020 09:07
pawanpraka1 added a commit to pawanpraka1/zfs-localpv that referenced this pull request Jun 4, 2020
When we are deleting a PVC which is not bound because of the wrong
parameters/poolname in the storageclass. Reverting the behavior
introduced in openebs#121, Now
PVC will be bound but still zv will be showing state a Pending
until the volume is created.

Signed-off-by: Pawan <pawan@mayadata.io>
pawanpraka1 added a commit to pawanpraka1/zfs-localpv that referenced this pull request Jun 4, 2020
PVC will not bound if there is wrong parameters/poolname in the storageclass,
the ZFSVolume CR will be still created and will remain in Pending State,
deletion of the PVC will delete PVC and since PVC is not bound, ZFS-LocalPV
driver will not get the delete call and will leave the ZFSVolume CR hanging there.
Reverting the behavior introduced in openebs#121,
Now PVC will be bound but still ZFSVolume will be in Pending state until the volume is created.

Signed-off-by: Pawan <pawan@mayadata.io>
pawanpraka1 added a commit to pawanpraka1/zfs-localpv that referenced this pull request Jun 4, 2020
PVC will not bound if there is wrong parameters/poolname in the storageclass,
the ZFSVolume CR will be still created and will remain in Pending State,
deletion of the PVC will delete PVC and since PVC is not bound, ZFS-LocalPV
driver will not get the delete call and will leave the ZFSVolume CR hanging there.
Reverting the behavior introduced in openebs#121,
Now PVC will be bound but still ZFSVolume will be in Pending state until the volume is created.

Signed-off-by: Pawan <pawan@mayadata.io>
pawanpraka1 added a commit to pawanpraka1/zfs-localpv that referenced this pull request Jun 4, 2020
PVC will not bound if there is wrong parameters/poolname in the storageclass,
the ZFSVolume CR will be still created and will remain in Pending State,
deletion of the PVC will delete PVC and since PVC is not bound, ZFS-LocalPV
driver will not get the delete call and will leave the ZFSVolume CR hanging there.
Reverting the behavior introduced in openebs#121,
Now PVC will be bound but still ZFSVolume will be in Pending state until the volume is created.

Signed-off-by: Pawan <pawan@mayadata.io>
pawanpraka1 added a commit to pawanpraka1/zfs-localpv that referenced this pull request Jun 5, 2020
PVC will not bound if there are wrong parameters/poolname in the storageclass,
the ZFSVolume CR will be still created and will remain in Pending State,
deletion of the PVC will delete PVC and since PVC is not bound, ZFS-LocalPV
driver will not get the delete call and will leave the ZFSVolume CR hanging there.
Reverting the behavior introduced in openebs#121,
Now PVC will be bound but still ZFSVolume will be in Pending state until the volume is created.

Signed-off-by: Pawan <pawan@mayadata.io>
pawanpraka1 added a commit to pawanpraka1/zfs-localpv that referenced this pull request Jun 5, 2020
PVC will not bound if there are wrong parameters/poolname in the storageclass,
the ZFSVolume CR will be still created and will remain in Pending State,
deletion of the PVC will delete PVC and since PVC is not bound, ZFS-LocalPV
driver will not get the delete call and will leave the ZFSVolume CR hanging there.
Reverting the behavior introduced in openebs#121,
Now PVC will be bound but still ZFSVolume will be in Pending state until the volume is created.

Signed-off-by: Pawan <pawan@mayadata.io>
kmova pushed a commit that referenced this pull request Jun 8, 2020
PVC will not bound if there are wrong parameters/poolname in the storageclass,
the ZFSVolume CR will be still created and will remain in Pending State,
deletion of the PVC will delete PVC and since PVC is not bound, ZFS-LocalPV
driver will not get the delete call and will leave the ZFSVolume CR hanging there.
Reverting the behavior introduced in #121,
Now PVC will be bound but still ZFSVolume will be in Pending state until the volume is created.

Signed-off-by: Pawan <pawan@mayadata.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Add new functionality to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pvc is getting bound even if the zpool name is given wrong
3 participants