feat(growpart): add LVM resize support#6586
feat(growpart): add LVM resize support#6586xiachen-rh wants to merge 1 commit intocanonical:mainfrom
Conversation
xiachen-rh
force-pushed
the
growpart-lvm-resize
branch
from
edda542 to
6f2082b
Compare
|
I think there should be a configuration switch to control whether or not the LV is resized (the PV resize seems fine) - often when LVM is used there will be multiple LVs (i.e. for /home, for /var/log, etc) and so having the LV for rootfs ("/") be grown to use up the whole of the VG is likely to be undesirable. Also the growpart script (from cloud-utils) has some pvresize functionality (I forget any issues with it) and so it doesn't make sense to have LVM functionality in both cloud-util's growpart and in cloud-init's growpart module. |
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging blackboxsw, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag blackboxsw to reopen it.) |
github-actions
bot
added
the
stale-pr
|
@dermotbradley thanks a lot for your comment, let me check how to update the code. |
|
Hello @blackboxsw could you help me re-open it please? I will continue working on it. |
github-actions
bot
removed
the
stale-pr
|
@dermotbradley So the solution for cloud-init could be, Or making a fix solution for cloud-utils, What's your opinion?
|
If we can have a more complete solution within cloud-init without depending on other external utilities, I think that might be more preferable. |
|
@holmanb @TheRealFalcon can you provide your inputs as well please? thanks. |
|
Another point is, with LVM we may not even need to grow an existing partition, we can add a new partition and use pvcreate+vgextend to add it to the existing volume group without any disruption. It should in theory be easier/safer than doing this without LVM. Why the current approach uses resize With LVM, we may not need to resize partitions because What do you think of it? Which solution is better? |
xiachen-rh
force-pushed
the
growpart-lvm-resize
branch
from
6f2082b to
4c3f8d1
Compare
github-actions
bot
added
the
documentation
xiachen-rh
force-pushed
the
growpart-lvm-resize
branch
4 times, most recently
from
59ee3b0 to
0c716f0
Compare
|
I updated the code, the main change is, Enhance the LVM resize functionality with:
The Update schema, documentation, and add comprehensive tests. |
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging blackboxsw, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag blackboxsw to reopen it.) |
github-actions
bot
added
the
stale-pr
|
Could anyone help review the code please? |
|
Thanks for putting together this proposal. I'll try to find some time soon to give it a proper review. Have you ran the integration test? |
xiachen-rh
force-pushed
the
growpart-lvm-resize
branch
from
0c716f0 to
5300b30
Compare
|
Yes, I ran the integration test on the ubuntu instance, PASS I also test it manually on the Azure with RHEL instance, it resized the rootvg-rootlv by default.
|
ani-sinha
left a comment
ani-sinha
left a comment
There was a problem hiding this comment.
Overall LGTM except those two comments.
cloudinit/config/cc_growpart.py
Outdated
| e, | ||
| ) | ||
| # On error, don't skip (safer to resize all) | ||
| skip_pvresize = False |
There was a problem hiding this comment.
This part I do not like. On error, we should just bail out without trying to resize all forcefully. I would just call
raise ResizeFailedException(e) from e
There was a problem hiding this comment.
Yes, this warning message is misleading and I will overwrite this part and let resize_lvm() handle all the logic. thanks again
cloudinit/config/cc_growpart.py
Outdated
| the VG. Default: False. | ||
| """ | ||
| LOG.info("starting LVM resize flow for %s", blockdev) | ||
| vg = _get_vg_for_lv(blockdev) |
There was a problem hiding this comment.
No need to call these again. Just pass vg and pvs you got from lines 654 and 655 into this function as arguments.
There was a problem hiding this comment.
Yes, there are redundant code, let me update it and thanks for your comments!
Fixes canonicalGH-3265 This change integrates LVM logical volume resizing into the existing growpart flow. When the target block device is an LVM LV, cloud-init now: - Performs lvs lookup to determine the volume group - Enumerates all PVs in the VG using new helper functions - Intelligently handles pvresize: * Skips pvresize for single-PV VGs when using growpart (growpart already handles it via maybe_lvm_resize) * Resizes all PVs for multi-PV VGs (growpart only resizes the partition's PV) - Conditionally extends LV based on `resize_lv` config option * When `resize_lv: true` (default): lvextend +100%FREE on the LV * When `resize_lv: false`: Only resize PVs, leaving LV unchanged (useful for multi-LV setups where free space should be preserved) The `resize_lv` option is particularly useful when multiple LVs exist in the same VG (e.g., separate LVs for /home, /var/log, etc.), allowing users to preserve free space for other LVs. New helper functions: - `_get_vg_for_lv()`: Returns VG name for a given LV device - `_get_pvs_for_vg()`: Returns list of PV device paths for a VG Filesystem resizing is intentionally omitted because resizefs is handled by a separate module. Unit tests have been added under test_cc_growpart.py to validate: - successful pvresize and lvextend calls - error propagation for failed operations - resize_lv=False behavior - Single-PV VG with growpart (skip pvresize) - Multi-PV VG with growpart (resize all PVs) This change does not affect existing non-LVM growpart behaviour. Signed-off-by: Amy Chen <xiachen@redhat.com>
xiachen-rh
force-pushed
the
growpart-lvm-resize
branch
from
5300b30 to
06b03d2
Compare
ani-sinha
left a comment
ani-sinha
left a comment
There was a problem hiding this comment.
Much better. Except for one comment, LGTM.
| LOG.debug("growpart: empty device list") | ||
| return | ||
|
|
||
| resize_lv = util.get_cfg_option_bool(mycfg, "resize_lv", True) |
There was a problem hiding this comment.
nit: I think the code will look cleaner if resize_lv is made global so that it does not need to be passed around various functions. This comes from the config so global seems reasonable.
There was a problem hiding this comment.
Thanks for the suggestion and I see the appeal of making resize_lv global and I agree that could slightly reduce parameter passing.
That said, I’d prefer to keep the parameter-passing approach here. It keeps the pattern consistent with other modules, makes the dependencies explicit, and helps with testability since the functions can be exercised independently without relying on global state. It also keeps the functions reusable if we ever need to call them with different values in the future.
In this case the passing depth is fairly small (handle() → resize_devices() → resize_lvm()), so the added clarity and flexibility feel worth the small amount of extra wiring.
Happy to revisit this if we see the parameter being threaded through many more layers later on.
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging blackboxsw, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag blackboxsw to reopen it.) |
github-actions
bot
added
the
stale-pr
|
Hello @blackboxsw, this pull request will automatically close in 7 days, what can I do for it? |
github-actions
bot
removed
the
stale-pr
4 participants
Fixes GH-3265
This change integrates LVM logical volume resizing into the existing
growpart flow. When the target block device is an LVM LV, cloud-init
now:
- Performs lvs lookup to determine the volume group
- Enumerates all PVs in the VG using new helper functions
- Intelligently handles pvresize:
* Skips pvresize for single-PV VGs when using growpart (growpart
already handles it via maybe_lvm_resize)
* Resizes all PVs for multi-PV VGs (growpart only resizes the
partition's PV)
- Conditionally extends LV based on
resize_lvconfig option* When
resize_lv: true(default): lvextend +100%FREE on the LV* When
resize_lv: false: Only resize PVs, leaving LV unchanged(useful for multi-LV setups where free space should be preserved)
The
resize_lvoption is particularly useful when multiple LVs existin the same VG (e.g., separate LVs for /home, /var/log, etc.), allowing
users to preserve free space for other LVs.
New helper functions:
-
_get_vg_for_lv(): Returns VG name for a given LV device-
_get_pvs_for_vg(): Returns list of PV device paths for a VGFilesystem resizing is intentionally omitted because resizefs is handled
by a separate module.
Unit tests have been added under test_cc_growpart.py to validate:
- successful pvresize and lvextend calls
- error propagation for failed operations
- resize_lv=False behavior
- Single-PV VG with growpart (skip pvresize)
- Multi-PV VG with growpart (resize all PVs)
This change does not affect existing non-LVM growpart behaviour.
Redhat Jira ticket: [RFE][Azure]growpart fails to resize a root partition on LVM
https://issues.redhat.com/browse/RHEL-107485
Test Steps
I tested it on Azure and AWS with rhel image, for example,
Boot up a VM on Azure and change the os disk to bigger than 64G
Check the rootlv partition size, the root partition size is extended after VM boot up.
[Before changes]
The root partition size is not extended.
[After changes]
The root partition size is extended.
I’ve added three unit tests. Please let me know if further coverage is needed.
The integration test case is copied from #887, however I did not test it because that I failed to set up the local integration test environment, I will run it later.