bhouse-nexthop opened a new pull request, #14647: URL: https://github.com/apache/cloudstack/pull/14647
### Description A size-only `resizeVolume` of a running system VM's volume, for example a virtual router's ROOT, was accepted. The storage and the database got the new size, but the running VM was never told, so the guest kept the old size until it next started. This PR refuses that resize unless the system VM is Stopped. Fixes #14645 #### Why it happens `orchestrateResizeVolume` looks the VM up only with `_userVmDao.findById()`. For a system VM that is null, so: - the resize is sent with the instance name `none` and no host. On KVM the image is grown, but `virsh blockresize` is never run for the running domain; - the checks in the same method are skipped: XenServer resizing only a stopped VM's disk, and on KVM the VM's host being Up and Enabled. A ROOT resize through a new disk offering, in `resizeVolume` or `changeOfferingForVolume`, is already refused unless the VM is Stopped (`checkIfVolumeIsRootAndVmIsRunning`). The size-only `resizeVolume` path had no such check. #### Fix `validateVolumeResizeWithSize`, which both `resizeVolume` and `changeOfferingForVolume` call before any size change, now refuses one more case. If the volume is attached to a system VM (`VirtualMachine.Type.isUsedBySystem()`) that is not Stopped, the call fails with: ``` Cannot resize volume [ROOT-N] of system VM r-N-VM while it is in state Running. Stop the system VM before resizing its volume. ``` - A stopped system VM's volume can still be resized, and the VM sees the new size when it starts. - User VMs are unaffected, and so are IOPS-only changes, since the check is in the size-change branch. I chose to refuse rather than do the resize live. A live resize would need `orchestrateResizeVolume` to handle system VMs on every hypervisor, through the VM work queue and the host checks, and that needs testing on each one. Refusing matches how other volume operations treat system VMs: snapshots of their volumes are refused, and attach and detach accept only user VMs. This touches a different part of `VolumeApiServiceImpl` from #14602, and the two merge cleanly. ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [x] Bug fix (non-breaking change which fixes an issue) - [ ] Enhancement (improves an existing feature and functionality) - [ ] Cleanup (Code refactoring and cleanup, that may add test cases) - [ ] Build/CI - [ ] Test (unit or integration test code) ### Feature/Enhancement Scale or Bug Severity #### Bug Severity - [ ] BLOCKER - [ ] Critical - [ ] Major - [x] Minor - [ ] Trivial ### How Has This Been Tested? The bug was seen on a KVM + Ceph RBD lab while testing #14602. A size-only `resizeVolume size=20` on a running router's ROOT succeeded. `rbd info` and the API showed 20 GiB, but `virsh domblkinfo` still showed the old size until the router restarted. The fix itself is covered by unit tests; it has not been run on the lab yet. New tests in `VolumeApiServiceImplTest`: - `validateVolumeIsNotOnRunningSystemVmTestNotStoppedSystemVms`: a router, console proxy, secondary storage VM and internal LB VM, each in Running, Starting, Stopping and Migrating, are all refused. - `validateVolumeIsNotOnRunningSystemVmTestStoppedRouter`, `...TestRunningUserVm` and `...TestDetachedVolume`: allowed. A detached volume needs no VM lookup. - `resizeVolumeSizeOnlyOnRunningRouterIsRefused`: a size-only grow through `resizeVolume` on a running router's volume is refused, and `VolumeService.resize` is never called. Without the new check, this request gets past validation. `VolumeApiServiceImplTest` passes (150 tests). Checkstyle is clean for `server`. #### How did you try to break this feature and the system with this change? - I looked for internal callers of `resizeVolume` / `changeDiskOfferingForVolumeInternal` that could resize a system VM's volume. There are two, Shared FileSystem and CKS, and both work on user VMs. - The check uses `isUsedBySystem()`, so user, CKS and Shared FileSystem VMs (all `Type.User`) are unaffected. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
