NFS tree quota support #222
Conversation
khareRajshree
requested review from
abhi16394,
bharathsreekanth,
JacobGros,
mjsdell,
jooseppi-luna,
ashleyvjoy,
Deepak-Ghivari,
KshitijaKakde,
Prabhu-Dell,
VamsiSiddu-7,
mbasha-dell and
kumarkgosa
as code owners
service/controller.go
Outdated
| } | ||
|
|
||
| // create quota for the filesystem | ||
| if isQuotaEnabled { |
There was a problem hiding this comment.
Why to check the isQuotaEnabled again here. I think we are checking this above at line no: 379
service/controller.go
Outdated
| // enabling quota on FS | ||
| fs, err := system.GetFileSystemByIDName(fsID, "") | ||
| if err != nil { | ||
| Log.Debugf("Find Volume response: %v Error: %v", fs, err) |
There was a problem hiding this comment.
don't we return error when the filesystem is not found?
service/features/service.feature
Outdated
| @@ -1265,12 +1265,24 @@ Feature: VxFlex OS CSI interface | |||
| When I call Node Probe | |||
| Then the error contains "The given GUID is invalid" | |||
|
|
|||
| Scenario: Controller expand volume for NFS | |||
| Given a VxFlexOS service | |||
| # @wip | |||
There was a problem hiding this comment.
can you removed these commented tests if they are not required.
service/step_handlers_test.go
Outdated
| @@ -791,7 +802,7 @@ func handleFileSystems(w http.ResponseWriter, r *http.Request) { | |||
| replacementMap["__ID__"] = id | |||
| replacementMap["__NAME__"] = name | |||
| replacementMap["__SIZE_IN_Total__"] = fileSystemIDToSizeTotal[id] | |||
| replacementMap["__PARENT_ID__"] = fileSystemIDParentID[id] | |||
| //replacementMap["__IS_QUOTA_ENABLED__"] = fileSystemIDToQuotaEnabled[id] | |||
There was a problem hiding this comment.
why are you replacing the parent id to quota?
| fs, err := system.GetFileSystemByIDName(fsResp.ID, "") | ||
| if err != nil { | ||
| Log.Debugf("Find Volume response error: %v", err) | ||
| return nil, status.Errorf(codes.Unknown, "Find Volume response error: %v", err) |
There was a problem hiding this comment.
I think here codes.notfound or codes.internal will be a great fit rather than codes.unknow thoughts?
There was a problem hiding this comment.
Hi @VamsiSiddu-7, I referred to the existing codes used for such error scenarios, hence went ahead to maintain the same responses.
https://github.com/dell/csi-powerflex/blob/main/service/controller.go#L361
If we plan to change it here, then it should be reflected to other instances as well to maintain the consistency.
There was a problem hiding this comment.
okay let's keep it the same codes.Unknown we will have consistency
| newFs, err := system.GetFileSystemByIDName(fsResp.ID, "") | ||
| if err != nil { | ||
| Log.Debugf("Find Volume response: %v Error: %v", newFs, err) | ||
| Log.Debugf("Find Volume response error: %v", err) | ||
| return nil, status.Errorf(codes.Unknown, "Find Volume response error: %v", err) |
There was a problem hiding this comment.
Keeping it same to maintain consistency.
Description
Add support for tree quota for NFS volume creation and expansion.
GitHub Issues
List the GitHub issues impacted by this PR:
Checklist:
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration
helm test:
executed 1vol-nfs
then expand volume by re-applying the yaml.
k apply -f /csi-powerflex/test/helm/1vol-nfs/templates/pvc0.yaml
cert-csi:
./cert-csi test expansion -v 1 -p 1 -iSize 8Gi -expSize 12Gi --sc vxflexos-nfs
controller logs: