Update disk resize to work with lvms-storage-operator/topolvm
#4114
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This comment was marked as outdated.
This comment was marked as outdated.
|
/wip |
|
/hold |
anjannath
force-pushed
the
extradisk
branch
3 times, most recently
from
92fa114
to
efefed3
Compare
anjannath
changed the title
LVMCluster resource during crc startlvms-storage-operator/topolvm
There was a problem hiding this comment.
I'm a bit confused by ' update disk resize to work with new disk layout ', could you detail more in the commit log why we need to move the topolvm partition (ie what fails if we don't do this)? What is the order before, what is the order after? Could this be done proactively in snc? And can we resize both the root partition and the PVs without any issue?
pkg/crc/machine/start.go
Outdated
| } | ||
| part, _, err := sshRunner.RunPrivileged("Get device id", "/usr/sbin/blkid", "-t", fmt.Sprintf("TYPE=%s", diskType), "-o", "device") | ||
| part, _, err := sshRunner.RunPrivileged("Get device id", cmd) |
There was a problem hiding this comment.
WhilesshRunner.RunPrivileged(cmd) will work, the RunPrivileged interface expects split args (ie a []string) as other implementations won't work with a single string containing all the args separated with spaces.
There was a problem hiding this comment.
Might be better to avoid the duplication by using query := "--label root" and query = "-t TYPE=LVM2_member" and then hardcoding /usr/sbin/blkid and -o device in a single place.
There was a problem hiding this comment.
updated the commit to use the above suggestion!
| } | ||
|
|
||
| // with '/dev/[sv]da4' as input, run 'growpart /dev/[sv]da 4' | ||
| if _, _, err := sshRunner.RunPrivileged(fmt.Sprintf("Growing %s partition", rootPart), "/usr/bin/growpart", rootPart[:len("/dev/.da")], rootPart[len("/dev/.da"):]); err != nil { |
There was a problem hiding this comment.
The corresponding code in the new file has become
if err := growPersistentVolume(sshRunner, startConfig.Preset, startConfig.PersistentVolumeSize); err != nil {
return fmt.Errorf("Unable to grow persistent volume partition: %w", err)
}
// with '/dev/[sv]da4' as input, run 'growpart /dev/[sv]da 4'
if err := growPartition(sshRunner, rootPart); err != nil {
return nil
}
While the addition of a helper is a good thing, this should not be hidden in a commit saying it's only moving code.
Fwiw, a helper for move topolvm partition to end of disk could also be useful to make growRootFilesystem easier to read.
I assume this is no longer accurate as you updated the PR? |
after using topolvm as storage provisioner there's two partitions of type xfs and its ambiguos which is the root partition but the root partition also has a label 'root' in case of the ocp bundle
this allows users to also configure the persistent volume's partition size for the ocp preset, in case of ocp preset the default size of the persistent volume partition (topolvm partition) is 3G
to be able to use topolvm we need a separate parition this new partition is added at the end of the disk and used by topolvm so the new disk layout is |efi|boot|root|pv| earlier the root parition could be easily extended after increasing the disk size as there was no patition after it, with the new layout after increasing the disk size we have to move the "pv" partition to create free space after the "root" partition to increase its size this adds code to move topolvm partition and restart the VM which is needed to re-read the changed partition table, after moving the part the root partition can be grown
this makes it a bit easier to navigate the start.go file by moving some of the disk size and partition modification functions into a separate disks.go file in the same machine package
this uses `blkid -t PARTLABEL=topolvm` to get the topolvm partition name and introduces a new helper getDeviceID to run different forms of the `blkid` command
|
@anjannath: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
i've updated the commit message.. but mentioning here as well: after adding a new partition at the end of disk, the partition layout is |
fixes #4117
needs crc-org/snc#877