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
Modify CephFs provisioner to use the ceph mgr commands #400
Conversation
Thanks @poornimag . We ( + @Madhu-1 ) are reviewing this change. |
@poornimag please check at the build failure. |
@ajarr PTAL. |
examples/cephfs/storageclass.yaml
Outdated
@@ -22,6 +22,9 @@ parameters: | |||
# Required for provisionVolume: "true" | |||
pool: cephfs_data | |||
|
|||
# Name of the ceph filesystem with in which the volumes will be created |
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.
with in/within ?
2ba9478
to
e1f5b46
Compare
examples/cephfs/storageclass.yaml
Outdated
@@ -22,6 +22,9 @@ parameters: | |||
# Required for provisionVolume: "true" | |||
pool: cephfs_data | |||
|
|||
# Name of the ceph filesystem with in which the volumes will be created | |||
fsName: cephfs |
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.
need to update E2E for this parameter here
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.
need to update readme on how to get this parameter
pkg/cephfs/volume.go
Outdated
return fmt.Errorf("error mounting ceph root: %v", err) | ||
} | ||
func createVolume(volOptions *volumeOptions, volID volumeID, bytesQuota int64) error { | ||
_, _, err := util.ExecCommand( |
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.
@poornimag do we need any changes in Node plugin for this?
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.
Nope, we do not need
Fixes: #328 |
@gman0 PTAL |
@poornimag Please revisit |
181e802
to
f0bda41
Compare
@poornimag do we need any changes in the readme/documentation ? |
Succesfully tested the PVC creation and mounting:
|
No, the fsName parameter is already added in a different path, so this patch has not added any new parameters |
f0bda41
to
7e9f509
Compare
@poornimag Awesome! Can you also please test or write some data in this volume from the pod and check its writable ? |
Yes, have created a file abc in the ngnix test pod, and i see that its created from a different mount point (in the tool box). The log i have pasted also contains this... |
However, during the testing came across an issue:
|
Do we know what is the error logged at time of failed PV creation? that would become handy while raising the issue too. Yeah, lets log it if there is such condition. |
pkg/cephfs/volume.go
Outdated
) | ||
|
||
const ( | ||
cephVolumesRoot = "csi-volumes" | ||
|
||
namespacePrefix = "ns-" | ||
namespacePrefix = "fsvolumens-" |
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.
@poornimag any specific reason for namespace prefix change here ?
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.
Its changed to the default value specified in the ceph subvol create code
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 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.
@poornimag but Isnt it fsvolumens_
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.
Yes. This needs to be 'fsvolumens_'
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.
Ideally, once user creation moves into the ceph fs subvolume create ...
command I assume CSI code will not deal with this namespace name.
@ajarr Supporting user creation (based on some flag) is part of the implementation plan in the future?
The CI is failed and I believe its expected to fail as the ceph cluster in travis is not the version of nautilus we used for testing or rook templates dont have these latest images. The error what I can see there is:
I believe we could list better error to identify the exact reason though. |
Yes, this is the exact error i got without Ramana's fix for quota in subvolumes. Then we wait for his patch to be merged and csi ci to succeed? |
The error was due to, not providing the monitor ip in the storage class. |
@poornimag do we need createBackingVolume() func now ? or in short, do we need getAdminCredentials() and createCephUser() funcs from it? |
I did try to remove that, but createCephUser wasn't necessary even without this patch. Its the same as issue #269 . Will send it out as a separate patch. |
b36e48c
to
c811ed3
Compare
LGTM, from the POV of ceph-mgr subvolume commands |
Thanks @poornimag ! LGTM. Approving. |
@poornimag please update the PR description :) |
c811ed3
to
3d88b2c
Compare
@poornimag E2E is passing locally? |
228cea1
to
5cd7d5b
Compare
Currently CephFs provisioner mounts the ceph filesystem and creates a subdirectory as a part of provisioning the volume. Ceph now supports commands to provision fs subvolumes, hance modify the provisioner to use ceph mgr commands to (de)provision fs subvolumes. Signed-off-by: Poornima G <pgurusid@redhat.com>
5cd7d5b
to
9b552f1
Compare
@Madhu-1 Can you revisit this PR ? urgency due to release . |
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.
please update readme for min ceph version, can be done in next PR
Yeah , we will take care in a follow up PR! |
Signed-off-by: Poornima G <pgurusid@redhat.com>
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.
WIP, testing the same
|
||
if err = m.mount(cephRoot, adminCr, volOptions); err != nil { | ||
return fmt.Errorf("error mounting ceph root: %v", err) | ||
klog.Errorf("failed to purge subvolume %s(%s) in fs %s", string(volID), err, volOptions.FsName) |
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.
avoid volID
to string conversion use %v
for printing
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.
LGTM, nit can be fixed later
Thanks all!! I am removing |
Currently CephFs provisioner mounts the ceph filesystem
and creates a subdirectory as a part of provisioning the
volume. Ceph now supports commands to provision fs subvolumes,
hance modify the provisioner to use ceph mgr commands to
(de)provision fs subvolumes.
Signed-off-by: Poornima G pgurusid@redhat.com