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.

Sign up for GitHub

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

Jump to bottom

Set storage size to minimum volume size #441

Merged
merged 5 commits into from
Jul 5, 2022
Merged

Set storage size to minimum volume size #441

merged 5 commits into from
Jul 5, 2022

Conversation

srikiz
Copy link
Contributor

@srikiz srikiz commented Jun 24, 2022

Setting required Bytes to minimum volume size (1Gi) in case it is less than the minimum volume size.

Tested locally and works as expected.

Input pvc & pod for test:

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: csi-pvc
spec:
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 1
  storageClassName: do-block-storage
---
kind: Pod
apiVersion: v1
metadata:
  name: my-csi-app
spec:
  containers:
    - name: my-frontend
      image: busybox
      volumeMounts:
      - mountPath: "/data"
        name: my-do-volume
      command: [ "sleep", "1000000" ]
  volumes:
    - name: my-do-volume
      persistentVolumeClaim:
        claimName: csi-pvc

Logs for reference:

time="2022-06-24T16:23:44Z" level=warning msg="requiredBytes is less than minimum supported volume size, setting requiredBytes default to minimumVolumeSizeBytes" host_id=303920350 minimumVolumeSizeInBytes=1Gi region=blr1 requiredBytes=1 version=v4.1.0

kubectl get pv
NAME                                       CAPACITY   ACCESS MODES   RECLAIM POLICY   STATUS   CLAIM             STORAGECLASS       REASON   AGE
pvc-934a18e6-ab6e-47a9-8e4f-d736e75ca7e6   1Gi        RWO            Delete           Bound    default/csi-pvc   do-block-storage            4m17s

@srikiz
Copy link
Contributor Author

srikiz commented Jun 24, 2022

/fixes #435

@srikiz
Copy link
Contributor Author

srikiz commented Jun 24, 2022

FYI - @timoreimann

timoreimann
timoreimann reviewed Jun 28, 2022
View reviewed changes
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Two small suggestions, otherwise all good. 👍

driver/controller.go Outdated
@@ -955,7 +955,11 @@ func extractStorage(capRange *csi.CapacityRange) (int64, error) {
}

if requiredSet && !limitSet && requiredBytes < minimumVolumeSizeInBytes {
return 0, fmt.Errorf("required (%v) can not be less than minimum supported volume size (%v)", formatBytes(requiredBytes), formatBytes(minimumVolumeSizeInBytes))
d.log.WithFields(logrus.Fields{
"requiredBytes": formatBytes(requiredBytes),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind using camel case here and on the field below in order to be consistent with the existing style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, updated.

driver/controller.go
"requiredBytes": formatBytes(requiredBytes),
"minimumVolumeSizeInBytes": formatBytes(minimumVolumeSizeInBytes),
}).Warn("requiredBytes is less than minimum supported volume size, setting requiredBytes default to minimumVolumeSizeBytes")
return minimumVolumeSizeInBytes, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Let's say "minimum volume size" to not give the impression that minimumVolumeSizeBytes is a known value from the CSI spec (which requiredBytes is only).

@hakman hakman mentioned this pull request Jun 30, 2022
Merged
@srikiz
Copy link
Contributor Author

srikiz commented Jun 30, 2022

@timoreimann - please re-review when you get a chance. Thanks !

timoreimann
timoreimann reviewed Jul 4, 2022
View reviewed changes
driver/controller_test.go Outdated
@@ -181,14 +181,20 @@ func (*fakeTagsDriver) UntagResources(context.Context, string, *godo.UntagResour
}

func TestControllerExpandVolume(t *testing.T) {
default_volume := &godo.Volume{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use snake case here, i.e., defaultVolume.

driver/controller_test.go Outdated
@@ -200,6 +206,10 @@ func TestControllerExpandVolume(t *testing.T) {
},
{
name: "requested size less than minimum supported size",
godo_volume: &godo.Volume{
ID: "volume-id",
SizeGigaBytes: (minimumVolumeSizeInBytes / giB),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we subtract a bit to make sure we're really below the minimum size (and not hitting it exactly)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is size in gb I don't find a way to go less than 1 Gi. I did change that value to 1 to make it simpler. Please suggest if I am missing anything here.

driver/controller_test.go Outdated
}{
{
name: "request exceeds maximum supported size",
name: "request exceeds maximum supported size",
godo_volume: default_volume,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of requiring to define the volume for every current and future test case, could we make it optional and let the test body pick defaultVolume if the volume from the test case struct is nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, updated.

driver/controller_test.go Outdated
req *csi.ControllerExpandVolumeRequest
resp *csi.ControllerExpandVolumeResponse
err error
godo_volume *godo.Volume
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd name this just volume since the type is embedded in the *godo.Volume type. This would also keep the diff in this PR slightly smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

@timoreimann
Copy link
Contributor

Merging since PRs from forks still cannot leverage Github Actions secrets, and the unit tests should suffice here.

@timoreimann timoreimann merged commit e39d38a into digitalocean:master Jul 5, 2022
@srikiz srikiz deleted the support-minimum-size branch July 5, 2022 10:14
@MattIPv4
Copy link

MattIPv4 commented Jul 7, 2022

Hey, @srikiz - Thanks a ton for this sweet PR! 😄

Would you please shoot me an email when you get a chance?
mcowley at digitalocean dot com 🎉

@hakman hakman mentioned this pull request Jul 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timoreimann timoreimann timoreimann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@srikiz @timoreimann @MattIPv4