Skip to content

fix: use actual volume size for CSI and snapshot creation #1052

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
Merged
ushitora-anqou merged 5 commits into from
Jun 19, 2025
Merged

fix: use actual volume size for CSI and snapshot creation #1052

ushitora-anqou merged 5 commits into from
Jun 19, 2025

Conversation

@molpako
Copy link
Contributor

@molpako molpako commented May 28, 2025 •
edited
Loading

@molpako molpako force-pushed the fix/1039-correct-lvsize branch 4 times, most recently from adc8f0d to d142231 Compare June 2, 2025 09:08
@molpako molpako changed the title set lv size to LogicalVolume status field fix: use actual volume size for CSI and snapshot creation Jun 2, 2025
@molpako molpako requested a review from llamerada-jp June 2, 2025 09:40
@molpako molpako marked this pull request as ready for review June 2, 2025 09:40
@molpako molpako requested a review from a team as a code owner June 2, 2025 09:40
@molpako molpako requested review from daichimukai and ushitora-anqou and removed request for daichimukai June 2, 2025 09:40
llamerada-jp
llamerada-jp requested changes Jun 3, 2025
View reviewed changes
@molpako molpako force-pushed the fix/1039-correct-lvsize branch 2 times, most recently from 415e5bd to 46afa2a Compare June 3, 2025 07:05
@molpako molpako requested a review from llamerada-jp June 3, 2025 07:09
llamerada-jp
llamerada-jp requested changes Jun 3, 2025
View reviewed changes
@molpako molpako force-pushed the fix/1039-correct-lvsize branch 4 times, most recently from 0647476 to e519e00 Compare June 10, 2025 02:46
@molpako molpako requested a review from llamerada-jp June 12, 2025 02:14
llamerada-jp
llamerada-jp requested changes Jun 13, 2025
View reviewed changes
@molpako
set actual lv size to LogicalVolume status field
1bf1c0f 
Signed-off-by: Kyori Sakao <sakaok@cybozu.co.jp>
@molpako molpako force-pushed the fix/1039-correct-lvsize branch from e519e00 to 8b5276d Compare June 13, 2025 01:32
@molpako molpako requested a review from llamerada-jp June 13, 2025 02:15
llamerada-jp
llamerada-jp requested changes Jun 13, 2025
View reviewed changes
Copy link
Contributor

@llamerada-jp llamerada-jp left a comment

Choose a reason for hiding this comment

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

Would you update check logic below that should use LogicalVolume::status.currentSize.

topolvm/internal/driver/controller.go

Lines 224 to 225 in 8484158

sourceSizeBytes := sourceVol.Spec.Size.Value()
if requestCapacityBytes < sourceSizeBytes {

molpako added 2 commits June 13, 2025 09:06
@molpako
return actual volume size to csi sidecar
da820bd 
Signed-off-by: Kyori Sakao <sakaok@cybozu.co.jp>
@molpako
create LogicalVolume for snapshot using status.currentSize of source LV
061b806 
Signed-off-by: Kyori Sakao <sakaok@cybozu.co.jp>
@molpako molpako force-pushed the fix/1039-correct-lvsize branch from 8b5276d to b143ecf Compare June 13, 2025 09:06
ushitora-anqou
ushitora-anqou requested changes Jun 13, 2025
View reviewed changes
@molpako molpako force-pushed the fix/1039-correct-lvsize branch from 782338e to da742c3 Compare June 13, 2025 10:01
llamerada-jp
llamerada-jp requested changes Jun 16, 2025
View reviewed changes
Copy link
Contributor

@llamerada-jp llamerada-jp left a comment

Choose a reason for hiding this comment

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

I would like to update the size of other tests as well. For example, the following values.

topolvm/test/e2e/e2e_test.go

Line 71 in 8484158

claimYAML := []byte(fmt.Sprintf(pvcTemplateYAML, "topo-pvc", "Filesystem", 1024, storageClass))

topolvm/test/e2e/thin_provisioning_test.go

Line 42 in 8484158

thinPvcYAML := []byte(fmt.Sprintf(thinPVCTemplateYAML, "thinvol", "1"))

topolvm/test/e2e/snapshot_test.go

Lines 32 to 41 in 8484158

// size of PVC in GBs from the source volume
pvcSizeGB = 1
// size of PVC in GBs from the restored PVCs
restorePVCSizeGB = 2
)
var (
pvcSize = strconv.Itoa(pvcSizeGB)
restorePVCSize = strconv.Itoa(restorePVCSizeGB)
)

llamerada-jp
llamerada-jp reviewed Jun 16, 2025
View reviewed changes
@molpako molpako force-pushed the fix/1039-correct-lvsize branch 3 times, most recently from 9093598 to 1acbd9d Compare June 18, 2025 09:26
molpako added 2 commits June 18, 2025 13:45
@molpako
test: verify actual volume size is set in status field
ce173b7 
Signed-off-by: Kyori Sakao <sakaok@cybozu.co.jp>
@molpako
refactor: LogicalVolumeService returns the LogicalVolume resource tha…
09d8771 
…t was created

Signed-off-by: Kyori Sakao <sakaok@cybozu.co.jp>
@molpako molpako force-pushed the fix/1039-correct-lvsize branch from 1acbd9d to 09d8771 Compare June 18, 2025 13:45
@molpako molpako requested a review from llamerada-jp June 19, 2025 01:48
@ushitora-anqou ushitora-anqou merged commit 1c860ae into main Jun 19, 2025
38 of 44 checks passed
@ushitora-anqou ushitora-anqou deleted the fix/1039-correct-lvsize branch June 19, 2025 09:30
@molpako molpako mentioned this pull request Jun 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@llamerada-jp llamerada-jp llamerada-jp approved these changes

@ushitora-anqou ushitora-anqou ushitora-anqou 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.

4 participants

@molpako @llamerada-jp @ushitora-anqou