-
Notifications
You must be signed in to change notification settings - Fork 42
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
PV Resizing: Give precedance to usage percentage #1112
Conversation
@@ -270,11 +270,11 @@ func TestPersistentVolumeAdjuster_calculateProposedVolumeSize(t *testing.T) { | |||
actualCapacity: resource.MustParse("200"), | |||
requestedCapacity: resource.MustParse("20"), | |||
}, | |||
wantProposedSize: resource.MustParse("200"), | |||
wantProposedSize: resource.MustParse("106"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that 103% is desired but wantProposedSize: "106"
, is this a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a typo and a logic flaw :) Good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh now that I realized, this is not a logic issue. 106 is the correct value, the reference for usage percentage is the actual volume size which is 200 in this case, which makes the 3% = 6Gi buffer. We cannot use requested size as a reference point as it could differ from actual capacity.
reason = migapi.VolumeAdjustmentCapacityMismatch | ||
} | ||
|
||
if volumeSizeWithThreshold.Cmp(maxSize) == 1 { | ||
maxSize = volumeSizeWithThreshold | ||
reason = migapi.VolumeAdjustmentUsageExceeded | ||
if usagePercentage+int64(pva.getVolumeUsagePercentageThreshold()) > 100 { | ||
reason = migapi.VolumeAdjustmentUsageExceeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: do we let users know that we are planning to downsize their volumes from the provisioned capacity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djwhatle Good point. No we do not.
Nomore being worked on due to conflicting priorities, I will reopen this when I have a full fix ready. |
Fixes #1074