-
Notifications
You must be signed in to change notification settings - Fork 239
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
feat: organize node expand logics #1008
feat: organize node expand logics #1008
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mowangdk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
0623c19
to
8c047de
Compare
8c047de
to
f59692a
Compare
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.
This is a good step forward. But I think it is even better just pass the required size to resizefs, and let it check and report error.
In case user manually resize the disk to larger size, and set PVC capacity to a smaller one, should we occupy the whole disk? (I think no, we should only occupy what is declared)
volExpandBytes := int64(req.GetCapacityRange().GetRequiredBytes()) | ||
requestGB := float64((volExpandBytes + 1024*1024*1024 - 1) / (1024 * 1024 * 1024)) | ||
volExpandBytes := float64(req.GetCapacityRange().GetRequiredBytes()) | ||
requestGB := math.Floor(volExpandBytes / float64(1024*1024*1024)) |
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.
Do all calculation with int64 and bytes? to avoid any potential issue with floating point precision lost. Just be consistent with ControllerExpandVolume
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.
volExpandBytes variable here is only use to generate requestGB, I think it would be fine.
// After calling openapi to expansion cloud disk, the size of the underlying block device may not change. need to retry | ||
log.Errorf("NodeExpandVolume:: Actual block size: %s is smaller than expected block size: %s, need to retry waiting", deviceCapacity, requestGB) | ||
return nil, status.Errorf(codes.Aborted, "deviceCapacity: %v, requestGB: %v not in range", deviceCapacity, requestGB) | ||
} | ||
log.Infof( | ||
"NodeExpandVolume:: filesystem resize context device capacity: %v, before resize fs capacity: %v resize fs capacity: %v, requestGB: %v. file system lose percent: %v", | ||
deviceCapacity, beforeResizeDiskCapacity, diskCapacity, requestGB, GlobalConfigVar.FilesystemLosePercent) |
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.
Remove all codes about FilesystemLosePercent
?
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, I don't think It serves its purpose.
Please also update the PR title and commit message. |
f59692a
to
a292e2b
Compare
a292e2b
to
809f5ed
Compare
809f5ed
to
377c9e9
Compare
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes # None
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: