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

Conversation

KshitijaKakde
Copy link
Contributor

@KshitijaKakde KshitijaKakde commented Nov 26, 2024

Description

When creating a volume in PowerFlex, it appears to round down instead of up for sizes that are multiples of 8GB, fixing this issue.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#1608

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • Backward compatibility is not broken

How Has This Been Tested?

  1. Deployed the driver using csm-operator, created a PVC with a storage size of 51200000Ki, it correctly rounds up to 56 GiB. Previously, it rounded down to 48 GiB.
  2. Added and executed UT
  3. Executed cert-csi
    pflex-cert-csi.txt

pflex-integration.txt

rishabhatdell
rishabhatdell previously approved these changes Nov 27, 2024
@@ -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

@adarsh-dell
Copy link
Contributor

Please perform end-to-end integration testing to ensure nothing is impacted. Are there any additional scenarios we can consider here, especially related to expanding volume size? I assume a similar check will be reused for those cases as well, correct?

// 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.

@KshitijaKakde
Copy link
Contributor Author

Please perform end-to-end integration testing to ensure nothing is impacted. Are there any additional scenarios we can consider here, especially related to expanding volume size? I assume a similar check will be reused for those cases as well, correct?

I have executed the integration test. The ControllerExpandVolume call also invokes the validateVolSize function to perform similar checks on the expanding volume size.

@KshitijaKakde KshitijaKakde merged commit 22bf0e5 into main Dec 3, 2024
6 checks passed
@KshitijaKakde KshitijaKakde deleted the fix-volsize branch December 3, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants