Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

Fix an issue with default VolmeStorage not being accounted for in QuotaRequests #2240

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Fix an issue with default VolmeStorage not being accounted for in QuotaRequests #2240

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 35 additions & 7 deletions pkg/controller/quota/quota.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,16 +146,25 @@ func addStorage(appInstance *v1.AppInstance, quotaRequest *adminv1.QuotaRequestI
size = boundSize
}

// No need to proceed if the size is empty
if size == "" || size == "0" {
// Handle three cases:
// 1. The volume's size is explicitly set to 0. This means there is nothing to count.
// 2. The volume's size is not set. This means we should assume the default size.
// 3. The volume's size is set to a specific value. This means we should use that value.
var sizeQuantity resource.Quantity
switch size {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the size of bound volumes are counted here. Is there any risk that we are double-counting bound volumes? Or are we saying you can have as many volumes as you want as long as none of them are bound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked offline about this, there's some wonky-ness with the boundVolumeSize function that we'll need to address. We came to the conclusion that this should merge as is and a follow-up should be created to fix it.

case "0":
continue
case "":
sizeQuantity = defaultVolumeSize(appInstance, name)
default:
parsedQuantity, err := resource.ParseQuantity(string(size))
if err != nil {
return err
}
sizeQuantity = parsedQuantity
}

parsedSize, err := resource.ParseQuantity(string(size))
if err != nil {
return err
}
quotaRequest.Spec.Resources.VolumeStorage.Add(parsedSize)
quotaRequest.Spec.Resources.VolumeStorage.Add(sizeQuantity)
}

// Add the secrets needed to the quota request. We only parse net new secrets, not
Expand All @@ -169,6 +178,25 @@ func addStorage(appInstance *v1.AppInstance, quotaRequest *adminv1.QuotaRequestI
return nil
}

func defaultVolumeSize(appInstance *v1.AppInstance, name string) resource.Quantity {
result := *v1.DefaultSize // Safe to dereference because it is statically set in the v1 package.

// If the volume has a default size set, use that on status.Defaults, use that. Otherwise, if the
// VolumeSize default is set on status.Defaults, use that.
if defaultVolume, set := appInstance.Status.Defaults.Volumes[name]; set {
// We do not expect this to ever fail because VolumeClasses have their sizes validated. However,
// if it does fail, we'll just return the default size.
parsedQuantity, err := resource.ParseQuantity(string(defaultVolume.Size))
if err == nil {
result = parsedQuantity
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This felt like the right behavior to me if a size failed to parse, but let me know if something else makes more sense.

}
} else if appInstance.Status.Defaults.VolumeSize != nil {
result = *appInstance.Status.Defaults.VolumeSize
}

return result
}

// boundVolumeSize determines if the specified volume will be bound to an existing one. If
// it will not be bound, the size of the new volume is returned.
func boundVolumeSize(name string, bindings []v1.VolumeBinding) (bool, v1.Quantity) {
Expand Down
Loading