MORPH-1029: Gen2 hot disk resize support#168
Conversation
updated minVersion from 8.0.3 to 9.0.2
|
|
||
| if (masterItem.Generation != null) { | ||
| def generation = ScvmmGenerationUtil.toGenerationConfig(masterItem.Generation) | ||
| def hotResize = ScvmmGenerationUtil.isGeneration2(generation) |
There was a problem hiding this comment.
potential improvement here. The logic to determine hotResize is defined here, but also in the ScvvmGenerationUtil. It could be kept in one place.
Address PR review: move generation/hotResize sync out of VirtualMachineSync into syncGenerationFromCloudItem so hotResize is determined in one place.
|
|
||
| class ScvmmGenerationUtil { | ||
|
|
||
| static String toGenerationConfig(Object generation) { |
There was a problem hiding this comment.
rename teh function to getGenerationConfig() ?
Again config also does not sound the right word.
But the point is the function should be a get function.
There was a problem hiding this comment.
I agree that this is a beneficial minor improvement.
| return generationConfig == 'generation2' | ||
| } | ||
|
|
||
| static boolean hotResizeFromCloudItem(Map cloudItem) { |
There was a problem hiding this comment.
rename to hotResizeEnabled ?
| if (!server) { | ||
| return false | ||
| } | ||
| if (server.hotResize == true) { |
There was a problem hiding this comment.
why are we checking if (server.hotResize == true) {
We should only do
return isGeneration2(server.getConfigProperty('generation'))
| applyGenerationConfig(server, scvmmGeneration) | ||
| } | ||
|
|
||
| private static void applyGenerationConfig(ComputeServer server, String generation) { |
There was a problem hiding this comment.
rename to setGenerationConfig ?
There was a problem hiding this comment.
i guess all functions starting with apply should be renamed to set ?
| rtn.hotResize = false | ||
| } else if (opts.volumes && hasDiskResizeOrAdd(resizeRequest)) { | ||
| rtn.hotResize = supportsHotDisk | ||
| } else { |
There was a problem hiding this comment.
which cases will land in this else case ?
|
@dgaharwar12 We generally follow the practice of attaching screenshots / screen recoding video/ logs to prove that the fix works and that gives more confidence to the reviewers. |
|
Sure, I can share them tomorrow. |
|
|
||
| class ScvmmGenerationUtil { | ||
|
|
||
| static String toGenerationConfig(Object generation) { |
There was a problem hiding this comment.
I agree that this is a beneficial minor improvement.
| * @param opts additional options | ||
| * @return Response from API | ||
| */ | ||
| @Override |
There was a problem hiding this comment.
The function header above line 2196 used to be the function header for the resizeWorkload function below it. I don't think that was intentional. Correct and add a function header for the new method?
There was a problem hiding this comment.
this change is looking similar to embedded fix merged here: https://github.com/HPE-EMU/morpheus-ui/pull/4387/changes
There was a problem hiding this comment.
The embedded PR decides on hot resize based on generation and Dynamic Memory status
And the current PR decides on hot resize based on only generation.
I feel the table given in the embedded PR makes more sense to me.
Hot-Add Capability Matrix
| Operation | Gen 2 | Gen 1 |
|---|---|---|
| Memory (Dynamic) | Yes | Yes |
| Memory (Static) | No | No |
| CPU add | Yes | No |
| CPU remove | No | No |
| Disk (SCSI) | Yes | Yes |
There was a problem hiding this comment.
@dgaharwar12 was the hot resize problem occured after the embedded PR merge too ?
There was a problem hiding this comment.
But looks like the embedded PR was well tested for all the scenarios listed in the table.
Issue description (MORPH-1029 / Case 5393638946)
Summary
When adding or expanding disks on SCVMM VMs through Morpheus, the VM was stopped and started even though the same operations in the SCVMM console left the VM running. The disk change succeeded, but customers saw an unnecessary restart (downtime).
Root cause
Morpheus SCVMM resize logic always treated disk changes as requiring a powered-off VM.
getResizeConfigalways sethotResize = falseFor every resize, including disk-only changes with no memory or CPU change, Morpheus decided a stop was required.
resizeWorkloadAndServerstops the VM whenhotResizeis falseIt calls
stopWorkload/stopServerbefore disk work, then starts the VM again afterward. Customer logs showed shutdown → resize → startup (~40s downtime) even for disk-only reconfigure.This was not required by SCVMM
The plugin already uses
Expand-SCVirtualDiskDriveandNew-SCVirtualDiskDrive, which SCVMM can run on running Generation 2 VMs. The restart was imposed by Morpheus, not by SCVMM.VM generation was not used
Gen 1 VMs often need a power-off for disk changes; Gen 2 VMs support online disk add/expand. The plugin did not distinguish them:
hotResizewas hard-coded tofalsein resize configGenerationfrom SCVMM or sethotResizeon discovered VMsWhat was fixed
Plugin-only (
morpheus-scvmm-plugin):Expected behavior after fix
Note: Run a cloud sync after upgrading the plugin so existing Gen 2 VMs get
hotResizeandgenerationfrom SCVMM.