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

Fix Volume Size Rounding Issue in PowerFlex #357

Merged
merged 9 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions service/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -930,17 +930,24 @@ func validateVolSize(cr *csi.CapacityRange) (int64, error) {
}

var (
sizeGiB int64
sizeKiB int64
sizeB int64
sizeGiBFloat float64
sizeGiB int64
sizeKiB int64
sizeB int64
)

// VxFlexOS creates volumes in multiples of 8GiB, rounding up.
// Determine what actual size of volume will be, and check that
// we do not exceed maxSize
sizeGiB = minSize / kiBytesInGiB
// Calculate size in GiB using float for precision
sizeGiBFloat = float64(minSize) / float64(kiBytesInGiB)

// Use math.Ceil to round up to the nearest whole GiB
sizeGiB = int64(math.Ceil(sizeGiBFloat))

// if the requested size was less than 1GB, set the request to 1GB
// so it can be rounded to a 8GiB boundary correctly
if sizeGiB == 0 {
if sizeGiB < 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

As these checks are focused on block storage, I am particularly interested in having similar validations for NFS as well, especially with the same input sizes. That how it behaves, I think should work but better to double check on this as well.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested NFS with filesystem sizes of 48.8 GiB and 3.001 GiB (3073 MiB). In both cases, the filesystems were successfully created. The only requirement for filesystem creation is that the size must not be less than 3 GiB, which is handled in the code.

sizeGiB = 1
}
mod := sizeGiB % VolSizeMultipleGiB
Expand Down
18 changes: 17 additions & 1 deletion service/service_unit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func TestGetVolSize(t *testing.T) {
{
// requesting a size less than 1GiB should result in a minimal size
cr: &csi.CapacityRange{
RequiredBytes: 300 * bytesInKiB,
RequiredBytes: 1,
LimitBytes: 0,
},
sizeKiB: 8 * kiBytesInGiB,
Expand Down Expand Up @@ -105,6 +105,22 @@ func TestGetVolSize(t *testing.T) {
},
sizeKiB: 0,
},
{
// Requesting a size with decimal part that rounds up to next multiple of 8 GiB
cr: &csi.CapacityRange{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add more test cases with higher decimal precision values? Additionally, let's use the same input that helped us identify this issue for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

RequiredBytes: int64(9.5 * float64(bytesInGiB)),
LimitBytes: 0,
},
sizeKiB: 16 * kiBytesInGiB,
},
{
// Requesting a size of 8.5 GiB to test rounding up
cr: &csi.CapacityRange{
RequiredBytes: int64(48.5 * float64(bytesInGiB)),
LimitBytes: 0,
},
sizeKiB: 56 * kiBytesInGiB,
},
}

for _, tt := range tests {
Expand Down
12 changes: 6 additions & 6 deletions service/step_defs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3421,28 +3421,28 @@ func (f *feature) aCorrectNodeGetVolumeStatsResponse() error {
return nil
}

func (f *feature) iCallNodeUnstageVolumeWith(error string) error {
func (f *feature) iCallNodeUnstageVolumeWith(errStr string) error {
// Save the ephemeralStagingMountPath to restore below
ephemeralPath := ephemeralStagingMountPath
header := metadata.New(map[string]string{"csi.requestid": "1"})
if error == "NoRequestID" {
if errStr == "NoRequestID" {
header = metadata.New(map[string]string{"csi.requestid": ""})
}
ctx := metadata.NewIncomingContext(context.Background(), header)
req := new(csi.NodeUnstageVolumeRequest)
req.VolumeId = goodVolumeID
if error == "NoVolumeID" {
if errStr == "NoVolumeID" {
req.VolumeId = ""
}
req.StagingTargetPath = datadir
if error == "NoStagingTarget" {
if errStr == "NoStagingTarget" {
req.StagingTargetPath = ""
}
if error == "UnmountError" {
if errStr == "UnmountError" {
req.StagingTargetPath = "/tmp"
gofsutil.GOFSMock.InduceUnmountError = true
}
if error == "EphemeralVolume" {
if errStr == "EphemeralVolume" {
// Create an ephemeral volume id
ephemeralStagingMountPath = "test/"
err := os.MkdirAll("test"+"/"+goodVolumeID+"/id", 0o777)
Expand Down
9 changes: 9 additions & 0 deletions test/integration/features/integration.feature
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@ Feature: VxFlex OS CSI interface
And when I call DeleteVolume
Then there are no errors

Scenario: Create and delete basic volume with float size
Given a VxFlexOS service
And a basic block volume request "integration1" "48.1"
When I call CreateVolume
When I call ListVolume
Then a valid ListVolumeResponse is returned
And when I call DeleteVolume
Then there are no errors

Scenario: Idempotent create and delete basic volume
Given a VxFlexOS service
And a basic block volume request "integration2" "8"
Expand Down
6 changes: 3 additions & 3 deletions test/integration/step_defs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ func (f *feature) aVxFlexOSService() error {
return nil
}

func (f *feature) aBasicBlockVolumeRequest(name string, size int64) error {
func (f *feature) aBasicBlockVolumeRequest(name string, size float64) error {
req := new(csi.CreateVolumeRequest)
storagePool := os.Getenv("STORAGE_POOL")
params := make(map[string]string)
Expand All @@ -344,7 +344,7 @@ func (f *feature) aBasicBlockVolumeRequest(name string, size int64) error {
makeAUniqueName(&name)
req.Name = name
capacityRange := new(csi.CapacityRange)
capacityRange.RequiredBytes = size * 1024 * 1024 * 1024
capacityRange.RequiredBytes = int64(math.Ceil(size * 1024 * 1024 * 1024))
req.CapacityRange = capacityRange
capability := new(csi.VolumeCapability)
block := new(csi.VolumeCapability_BlockVolume)
Expand Down Expand Up @@ -2737,7 +2737,7 @@ func (f *feature) checkNFS(_ context.Context, systemID string) (bool, error) {
func FeatureContext(s *godog.ScenarioContext) {
f := &feature{}
s.Step(`^a VxFlexOS service$`, f.aVxFlexOSService)
s.Step(`^a basic block volume request "([^"]*)" "(\d+)"$`, f.aBasicBlockVolumeRequest)
s.Step(`^a basic block volume request "([^"]*)" "(\d+(\.\d+)?)"$`, f.aBasicBlockVolumeRequest)
s.Step(`^Set System Name As "([^"]*)"$`, f.iSetSystemName)
s.Step(`^Set Bad AllSystemNames$`, f.iSetBadAllSystemNames)
s.Step(`^I call CreateVolume$`, f.iCallCreateVolume)
Expand Down
Loading