-
Notifications
You must be signed in to change notification settings - Fork 6
MORPH-1029: Gen2 hot disk resize support #168
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,9 +20,11 @@ import com.morpheusdata.response.InitializeHypervisorResponse | |
| import com.morpheusdata.response.PrepareWorkloadResponse | ||
| import com.morpheusdata.response.ProvisionResponse | ||
| import com.morpheusdata.response.ServiceResponse | ||
| import com.morpheusdata.response.ValidateResizeWorkloadResponse | ||
| import com.morpheusdata.scvmm.logging.LogInterface | ||
| import com.morpheusdata.scvmm.logging.LogWrapper | ||
| import com.morpheusdata.scvmm.util.MorpheusUtil | ||
| import com.morpheusdata.scvmm.util.ScvmmGenerationUtil | ||
| import groovy.json.JsonSlurper | ||
|
|
||
| class ScvmmProvisionProvider extends AbstractProvisionProvider implements WorkloadProvisionProvider, HostProvisionProvider, ProvisionProvider.HypervisorProvisionFacet, HostProvisionProvider.ResizeFacet, WorkloadProvisionProvider.ResizeFacet, ProvisionProvider.BlockDeviceNameFacet { | ||
|
|
@@ -626,6 +628,7 @@ class ScvmmProvisionProvider extends AbstractProvisionProvider implements Worklo | |
| } | ||
|
|
||
| scvmmOpts.scvmmGeneration = virtualImage?.getConfigProperty('generation') ?: 'generation1' | ||
| ScvmmGenerationUtil.applyGenerationFromScvmmGeneration(server, scvmmOpts.scvmmGeneration) | ||
| scvmmOpts.isSyncdImage = virtualImage?.refType == 'ComputeZone' | ||
| scvmmOpts.isTemplate = !(virtualImage?.remotePath != null) && !virtualImage?.systemImage | ||
| scvmmOpts.templateId = virtualImage?.externalId | ||
|
|
@@ -1945,6 +1948,7 @@ class ScvmmProvisionProvider extends AbstractProvisionProvider implements Worklo | |
| scvmmOpts.secureBoot = virtualImage?.uefi ?: false | ||
| scvmmOpts.imageId = imageId | ||
| scvmmOpts.scvmmGeneration = virtualImage?.getConfigProperty('generation') ?: 'generation1' | ||
| ScvmmGenerationUtil.applyGenerationFromScvmmGeneration(server, scvmmOpts.scvmmGeneration) | ||
| scvmmOpts.diskMap = context.services.virtualImage.getImageDiskMap(virtualImage) | ||
| server = saveAndGetMorpheusServer(server, true) | ||
| scvmmOpts += getScvmmServerOpts(server) | ||
|
|
@@ -2189,6 +2193,16 @@ class ScvmmProvisionProvider extends AbstractProvisionProvider implements Worklo | |
| * @param opts additional options | ||
| * @return Response from API | ||
| */ | ||
| @Override | ||
| ServiceResponse<ValidateResizeWorkloadResponse> validateResizeWorkload(Instance instance, Workload workload, ResizeRequest resizeRequest, Map opts) { | ||
| def resizeConfig = getResizeConfig(workload, null, workload?.instance?.plan ?: resizeRequest?.plan, opts, resizeRequest) | ||
| def response = new ValidateResizeWorkloadResponse() | ||
| response.allowed = resizeConfig.allowed | ||
| response.hotResize = resizeConfig.hotResize | ||
| log.debug("validateResizeWorkload: ${response}") | ||
| return ServiceResponse.success(response) | ||
| } | ||
|
|
||
| @Override | ||
| ServiceResponse resizeWorkload(Instance instance, Workload workload, ResizeRequest resizeRequest, Map opts) { | ||
| log.info("resizeWorkload calling resizeWorkloadAndServer") | ||
|
|
@@ -2366,39 +2380,56 @@ class ScvmmProvisionProvider extends AbstractProvisionProvider implements Worklo | |
| rtn.neededCores = (rtn.requestedCores ?: 1) - (currentCores ?: 1) | ||
| setDynamicMemory(rtn, plan) | ||
|
|
||
| rtn.hotResize = false | ||
| def targetServer = server ?: workload?.server | ||
| def supportsHotDisk = ScvmmGenerationUtil.supportsHotDiskResize(targetServer) | ||
| def memoryOrCpuChange = (rtn.neededMemory != 0 || rtn.neededCores != 0 || rtn.minDynamicMemory || rtn.maxDynamicMemory) | ||
|
|
||
| // Disk changes.. see if stop is required | ||
| if (opts.volumes) { | ||
| resizeRequest.volumesUpdate?.each { volumeUpdate -> | ||
| if (volumeUpdate.existingModel) { | ||
| //existing disk - resize it | ||
| def volumeCode = volumeUpdate.existingModel.type?.code ?: "standard" | ||
| def volumeCode = volumeUpdate.existingModel.type?.code ?: "standard" | ||
| if (volumeUpdate.updateProps.maxStorage > volumeUpdate.existingModel.maxStorage) { | ||
| if (volumeCode.contains("differencing")) { | ||
| log.warn("getResizeConfig - Resize is not supported on Differencing Disks - volume type ${volumeCode}") | ||
| rtn.allowed = false | ||
| } else { | ||
| log.info("getResizeConfig - volumeCode: ${volumeCode}. Volume Resize requested. Current: ${volumeUpdate.existingModel.maxStorage} - requested : ${volumeUpdate.updateProps.maxStorage}") | ||
| rtn.allowed = true | ||
| } | ||
| if (volumeUpdate.existingModel.rootVolume) { | ||
| rtn.hotResize = false | ||
| if (volumeCode.contains("differencing")) { | ||
| log.warn("getResizeConfig - Resize is not supported on Differencing Disks - volume type ${volumeCode}") | ||
| rtn.allowed = false | ||
| } else { | ||
| log.info("getResizeConfig - volumeCode: ${volumeCode}. Volume Resize requested. Current: ${volumeUpdate.existingModel.maxStorage} - requested : ${volumeUpdate.updateProps.maxStorage}") | ||
| rtn.allowed = true | ||
| } | ||
| } | ||
| } else { | ||
| // new disk - add it | ||
| log.info("getResizeConfig - Adding new volume ${volumeUpdate.volume}") | ||
| rtn.allowed = true | ||
| } | ||
| log.info("getResizeConfig - Adding new volume ${volumeUpdate.volume}") | ||
| rtn.allowed = true | ||
| } | ||
| } | ||
| resizeRequest.volumesAdd?.each { volumeAdd -> | ||
| log.info("getResizeConfig - Adding new volume ${volumeAdd?.name}") | ||
| rtn.allowed = true | ||
| } | ||
| } | ||
|
|
||
| if (memoryOrCpuChange || resizeRequest.volumesDelete) { | ||
| rtn.hotResize = false | ||
| } else if (opts.volumes && hasDiskResizeOrAdd(resizeRequest)) { | ||
| rtn.hotResize = supportsHotDisk | ||
| } else { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which cases will land in this else case ? |
||
| rtn.hotResize = supportsHotDisk | ||
| } | ||
| log.debug("getResizeConfig - hotResize: ${rtn.hotResize}, supportsHotDisk: ${supportsHotDisk}, memoryOrCpuChange: ${memoryOrCpuChange}") | ||
| } catch (e) { | ||
| log.error("getResizeConfig error - ${e}", e) | ||
| } | ||
| return rtn | ||
| } | ||
|
|
||
| private static boolean hasDiskResizeOrAdd(ResizeRequest resizeRequest) { | ||
| def hasVolumeUpdates = resizeRequest.volumesUpdate?.any { volumeUpdate -> | ||
| volumeUpdate.existingModel && volumeUpdate.updateProps?.maxStorage > volumeUpdate.existingModel.maxStorage | ||
| } | ||
| def hasVolumeAdds = resizeRequest.volumesAdd?.size() > 0 | ||
| return hasVolumeUpdates || hasVolumeAdds | ||
| } | ||
|
|
||
| def buildStorageVolume(computeServer, volumeAdd, newCounter) { | ||
| def newVolume = new StorageVolume( | ||
| refType: 'ComputeZone', | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this change is looking similar to embedded fix merged here: https://github.com/HPE-EMU/morpheus-ui/pull/4387/changes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dgaharwar12 was the hot resize problem occured after the embedded PR merge too ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But looks like the embedded PR was well tested for all the scenarios listed in the table. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package com.morpheusdata.scvmm.util | ||
|
|
||
| import com.morpheusdata.model.ComputeServer | ||
|
|
||
| class ScvmmGenerationUtil { | ||
|
|
||
| static String toGenerationConfig(Object generation) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename teh function to getGenerationConfig() ? But the point is the function should be a get function.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this is a beneficial minor improvement. |
||
| if (generation == null) { | ||
| return null | ||
| } | ||
| def gen = generation.toString() | ||
| if (gen == '1' || gen.equalsIgnoreCase('generation1')) { | ||
| return 'generation1' | ||
| } | ||
| if (gen == '2' || gen.equalsIgnoreCase('generation2')) { | ||
| return 'generation2' | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| static boolean isGeneration2(String generationConfig) { | ||
| return generationConfig == 'generation2' | ||
| } | ||
|
|
||
| static boolean hotResizeFromCloudItem(Map cloudItem) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename to hotResizeEnabled ? |
||
| return isGeneration2(toGenerationConfig(cloudItem?.Generation)) | ||
| } | ||
|
|
||
| /** | ||
| * Updates generation config and hotResize from SCVMM cloud item when values differ. | ||
| * @return true if the server was modified | ||
| */ | ||
| static boolean syncGenerationFromCloudItem(ComputeServer server, Map cloudItem) { | ||
| def generation = toGenerationConfig(cloudItem?.Generation) | ||
| if (!generation) { | ||
| return false | ||
| } | ||
| def hotResize = isGeneration2(generation) | ||
| def changed = server.getConfigProperty('generation') != generation || server.hotResize != hotResize | ||
| if (changed) { | ||
| server.setConfigProperty('generation', generation) | ||
| server.hotResize = hotResize | ||
| } | ||
| return changed | ||
| } | ||
|
|
||
| static boolean supportsHotDiskResize(ComputeServer server) { | ||
| if (!server) { | ||
| return false | ||
| } | ||
| if (server.hotResize == true) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we checking if (server.hotResize == true) { We should only do |
||
| return true | ||
| } | ||
| return isGeneration2(server.getConfigProperty('generation')) | ||
| } | ||
|
|
||
| static void applyGenerationFromCloudItem(ComputeServer server, Map cloudItem) { | ||
| applyGenerationConfig(server, toGenerationConfig(cloudItem?.Generation)) | ||
| } | ||
|
|
||
| static void applyGenerationFromScvmmGeneration(ComputeServer server, String scvmmGeneration) { | ||
| applyGenerationConfig(server, scvmmGeneration) | ||
| } | ||
|
|
||
| private static void applyGenerationConfig(ComputeServer server, String generation) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename to setGenerationConfig ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i guess all functions starting with apply should be renamed to set ? |
||
| if (!generation) { | ||
| return | ||
| } | ||
| server.setConfigProperty('generation', generation) | ||
| server.hotResize = isGeneration2(generation) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function header above line 2196 used to be the function header for the
resizeWorkloadfunction below it. I don't think that was intentional. Correct and add a function header for the new method?