From 96ecd83cf3d0dbf11c51eb2619c4f4fdd03be7e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sadi=20J=C3=BAnior?= Date: Mon, 2 May 2022 19:38:29 +0000 Subject: [PATCH 1/4] [VMWare] Limit IOPS in hypervisor Limit compute/disk offerings IOPS in VMWare --- .../vmware/resource/VmwareResource.java | 7 ++- .../resource/VmwareStorageProcessor.java | 6 +- .../cloud/hypervisor/vmware/mo/HostMO.java | 2 + .../vmware/mo/HypervisorHostHelper.java | 4 +- .../vmware/mo/VirtualMachineMO.java | 8 ++- .../hypervisor/vmware/util/VmwareHelper.java | 21 +++++-- .../vmware/util/VmwareHelperTest.java | 56 +++++++++++++++++++ 7 files changed, 93 insertions(+), 11 deletions(-) create mode 100644 vmware-base/src/test/java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java index 3b3ebd88ea89..0a75816f388c 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java @@ -321,6 +321,7 @@ import com.vmware.vim25.PerfMetricSeries; import com.vmware.vim25.PerfQuerySpec; import com.vmware.vim25.RuntimeFaultFaultMsg; +import com.vmware.vim25.StorageIOAllocationInfo; import com.vmware.vim25.StoragePodSummary; import com.vmware.vim25.ToolsUnavailableFaultMsg; import com.vmware.vim25.VAppOvfSectionInfo; @@ -939,6 +940,7 @@ private Answer execute(ResizeVolumeCommand cmd) { boolean volumePathChangeObserved = false; boolean datastoreChangeObserved = false; + StorageIOAllocationInfo limitIops = vdisk.first().getStorageIOAllocation(); Pair pathAndChainInfo = getNewPathAndChainInfoInDatastoreCluster(vmMo, path, chainInfo, managed, cmd.get_iScsiName(), poolUUID, cmd.getContextParam(DiskTO.PROTOCOL_TYPE)); Pair poolUUIDandChainInfo = getNewPoolUUIDAndChainInfoInDatastoreCluster(vmMo, path, chainInfo, managed, cmd.get_iScsiName(), poolUUID, cmd.getContextParam(DiskTO.PROTOCOL_TYPE)); @@ -963,6 +965,7 @@ private Answer execute(ResizeVolumeCommand cmd) { } disk.setCapacityInKB(newSize); + disk.setStorageIOAllocation(limitIops); VirtualDeviceConfigSpec deviceConfigSpec = new VirtualDeviceConfigSpec(); @@ -2365,7 +2368,9 @@ protected StartAnswer execute(StartCommand cmd) { scsiUnitNumber++; } - VirtualDevice device = VmwareHelper.prepareDiskDevice(vmMo, null, controllerKey, diskChain, volumeDsDetails.first(), deviceNumber, i + 1); + Long maxIops = volumeTO.getIopsWriteRate() + volumeTO.getIopsReadRate(); + VirtualDevice device = VmwareHelper.prepareDiskDevice(vmMo, null, controllerKey, diskChain, volumeDsDetails.first(), deviceNumber, i + 1, maxIops); + s_logger.debug(LogUtils.logGsonWithoutException("The following definitions will be used to start the VM: virtual device [%s], volume [%s].", device, volumeTO)); diskStoragePolicyId = volumeTO.getvSphereStoragePolicyId(); if (StringUtils.isNotEmpty(diskStoragePolicyId)) { diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java index d2f7fc88a3b9..8778d8169e3c 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java @@ -50,8 +50,8 @@ import org.apache.cloudstack.storage.command.ResignatureCommand; import org.apache.cloudstack.storage.command.SnapshotAndCopyAnswer; import org.apache.cloudstack.storage.command.SnapshotAndCopyCommand; -import org.apache.cloudstack.storage.command.SyncVolumePathCommand; import org.apache.cloudstack.storage.command.SyncVolumePathAnswer; +import org.apache.cloudstack.storage.command.SyncVolumePathCommand; import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; import org.apache.cloudstack.storage.to.SnapshotObjectTO; import org.apache.cloudstack.storage.to.TemplateObjectTO; @@ -2144,7 +2144,7 @@ private Answer attachVolume(Command cmd, DiskTO disk, boolean isAttach, boolean diskController = vmMo.getRecommendedDiskController(null); } - vmMo.attachDisk(new String[] { datastoreVolumePath }, morDs, diskController, storagePolicyId); + vmMo.attachDisk(new String[] { datastoreVolumePath }, morDs, diskController, storagePolicyId, volumeTO.getIopsReadRate() + volumeTO.getIopsWriteRate()); VirtualMachineDiskInfoBuilder diskInfoBuilder = vmMo.getDiskInfoBuilder(); VirtualMachineDiskInfo diskInfo = diskInfoBuilder.getDiskInfoByBackingFileBaseName(volumePath, dsMo.getName()); chainInfo = _gson.toJson(diskInfo); @@ -2425,7 +2425,7 @@ public Answer dettachVolume(DettachCommand cmd) { @Override public Answer createVolume(CreateObjectCommand cmd) { - + s_logger.debug(LogUtils.logGsonWithoutException("Executing CreateObjectCommand cmd: [%s].", cmd)); VolumeObjectTO volume = (VolumeObjectTO)cmd.getData(); DataStoreTO primaryStore = volume.getDataStore(); String vSphereStoragePolicyId = volume.getvSphereStoragePolicyId(); diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostMO.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostMO.java index 07bead77c1f6..15d80fc79e43 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostMO.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostMO.java @@ -69,6 +69,7 @@ import com.vmware.vim25.VirtualNicManagerNetConfig; import com.cloud.hypervisor.vmware.util.VmwareContext; import com.cloud.hypervisor.vmware.util.VmwareHelper; +import com.cloud.utils.LogUtils; import com.cloud.utils.Pair; public class HostMO extends BaseMO implements VmwareHypervisorHost { @@ -643,6 +644,7 @@ public VirtualMachineMO findVmOnPeerHyperHost(String name) throws Exception { @Override public boolean createVm(VirtualMachineConfigSpec vmSpec) throws Exception { assert (vmSpec != null); + s_logger.debug(LogUtils.logGsonWithoutException("Creating VM with configuration: [%s].", vmSpec)); DatacenterMO dcMo = new DatacenterMO(_context, getHyperHostDatacenter()); ManagedObjectReference morPool = getHyperHostOwnerResourcePool(); diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java index 9f950024f13f..607353c80ced 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java @@ -59,6 +59,7 @@ import com.cloud.offering.NetworkOffering; import com.cloud.storage.Storage.StoragePoolType; import com.cloud.utils.ActionDelegate; +import com.cloud.utils.LogUtils; import com.cloud.utils.NumbersUtil; import com.cloud.utils.Pair; import com.cloud.utils.cisco.n1kv.vsm.NetconfHelper; @@ -1621,6 +1622,7 @@ public static boolean createBlankVm(VmwareHypervisorHost host, String vmName, St DatacenterMO dataCenterMo = new DatacenterMO(host.getContext(), host.getHyperHostDatacenter()); setVMHardwareVersion(vmConfig, clusterMo, dataCenterMo); + s_logger.debug(LogUtils.logGsonWithoutException("Creating blank VM with configuration [%s].", vmConfig)); if (host.createVm(vmConfig)) { // Here, when attempting to find the VM, we need to use the name // with which we created it. This is the only such place where @@ -2127,7 +2129,7 @@ public static void createOvfFile(VmwareHypervisorHost host, String diskFileName, VirtualDeviceConfigSpec deviceConfigSpec = new VirtualDeviceConfigSpec(); // Reconfigure worker VM with datadisk - VirtualDevice device = VmwareHelper.prepareDiskDevice(workerVmMo, null, -1, disks, morDs, -1, 1); + VirtualDevice device = VmwareHelper.prepareDiskDevice(workerVmMo, null, -1, disks, morDs, -1, 1, null); deviceConfigSpec.setDevice(device); deviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.ADD); vmConfigSpec.getDeviceChange().add(deviceConfigSpec); diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java index a25480f4241e..9ceaabf5d92d 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java @@ -1400,7 +1400,10 @@ public void attachDisk(String[] vmdkDatastorePathChain, ManagedObjectReference m } public void attachDisk(String[] vmdkDatastorePathChain, ManagedObjectReference morDs, String diskController, String vSphereStoragePolicyId) throws Exception { + attachDisk(vmdkDatastorePathChain, morDs, diskController, vSphereStoragePolicyId, null); + } + public void attachDisk(String[] vmdkDatastorePathChain, ManagedObjectReference morDs, String diskController, String vSphereStoragePolicyId, Long maxIops) throws Exception { if(s_logger.isTraceEnabled()) s_logger.trace("vCenter API trace - attachDisk(). target MOR: " + _mor.getValue() + ", vmdkDatastorePath: " + GSON.toJson(vmdkDatastorePathChain) + ", datastore: " + morDs.getValue()); @@ -1430,7 +1433,7 @@ public void attachDisk(String[] vmdkDatastorePathChain, ManagedObjectReference m } synchronized (_mor.getValue().intern()) { - VirtualDevice newDisk = VmwareHelper.prepareDiskDevice(this, null, controllerKey, vmdkDatastorePathChain, morDs, unitNumber, 1); + VirtualDevice newDisk = VmwareHelper.prepareDiskDevice(this, null, controllerKey, vmdkDatastorePathChain, morDs, unitNumber, 1, maxIops); if (StringUtils.isNotBlank(diskController)) { String vmdkFileName = vmdkDatastorePathChain[0]; updateVmdkAdapter(vmdkFileName, diskController); @@ -2099,7 +2102,7 @@ public VirtualMachineMO cloneFromDiskChain(String clonedVmName, int cpuSpeedMHz, VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec(); VirtualDeviceConfigSpec deviceConfigSpec = new VirtualDeviceConfigSpec(); - VirtualDevice device = VmwareHelper.prepareDiskDevice(clonedVmMo, null, -1, disks, morDs, -1, 1); + VirtualDevice device = VmwareHelper.prepareDiskDevice(clonedVmMo, null, -1, disks, morDs, -1, 1, null); deviceConfigSpec.setDevice(device); deviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.ADD); @@ -2133,6 +2136,7 @@ public GuestOsDescriptor getGuestOsDescriptor(String guestOsId) throws Exception } public void plugDevice(VirtualDevice device) throws Exception { + s_logger.debug(LogUtils.logGsonWithoutException("Pluging device [%s] to VM [%s].", device, getVmName())); VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec(); VirtualDeviceConfigSpec deviceConfigSpec = new VirtualDeviceConfigSpec(); deviceConfigSpec.setDevice(device); diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java index 14ea3771cd47..28e3ee3161a4 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java @@ -48,9 +48,11 @@ import com.cloud.hypervisor.vmware.mo.VirtualEthernetCardType; import com.cloud.hypervisor.vmware.mo.VirtualMachineMO; import com.cloud.hypervisor.vmware.mo.VmwareHypervisorHost; +import com.cloud.utils.LogUtils; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; import com.cloud.utils.exception.ExceptionUtil; +import com.google.gson.Gson; import com.vmware.vim25.DistributedVirtualSwitchPortConnection; import com.vmware.vim25.DynamicProperty; import com.vmware.vim25.GuestOsDescriptor; @@ -61,6 +63,7 @@ import com.vmware.vim25.PerfCounterInfo; import com.vmware.vim25.PerfMetricId; import com.vmware.vim25.ResourceAllocationInfo; +import com.vmware.vim25.StorageIOAllocationInfo; import com.vmware.vim25.VirtualCdrom; import com.vmware.vim25.VirtualCdromIsoBackingInfo; import com.vmware.vim25.VirtualCdromRemotePassthroughBackingInfo; @@ -141,7 +144,6 @@ private static VirtualEthernetCard createVirtualEthernetCard(VirtualEthernetCard public static VirtualDevice prepareNicOpaque(VirtualMachineMO vmMo, VirtualEthernetCardType deviceType, String portGroupName, String macAddress, int contextNumber, boolean connected, boolean connectOnStart) throws Exception { - assert(vmMo.getRunningHost().hasOpaqueNSXNetwork()); VirtualEthernetCard nic = createVirtualEthernetCard(deviceType); @@ -215,8 +217,10 @@ public static VirtualDevice prepareDvNicDevice(VirtualMachineMO vmMo, ManagedObj // vmdkDatastorePath: [datastore name] vmdkFilePath public static VirtualDevice prepareDiskDevice(VirtualMachineMO vmMo, VirtualDisk device, int controllerKey, String vmdkDatastorePathChain[], - ManagedObjectReference morDs, int deviceNumber, int contextNumber) throws Exception { - + ManagedObjectReference morDs, int deviceNumber, int contextNumber, Long maxIops) throws Exception { + LOGGER.debug(LogUtils.logGsonWithoutException("Trying to prepare disk device to virtual machine [%s], using the following details: Virtual device [%s], " + + "ManagedObjectReference [%s], ControllerKey [%s], VMDK path chain [%s], DeviceNumber [%s], ContextNumber [%s] and max IOPS [%s].", + vmMo, device, morDs, controllerKey, vmdkDatastorePathChain, deviceNumber, contextNumber, maxIops)); assert (vmdkDatastorePathChain != null); assert (vmdkDatastorePathChain.length >= 1); @@ -243,6 +247,13 @@ public static VirtualDevice prepareDiskDevice(VirtualMachineMO vmMo, VirtualDisk disk.setKey(-contextNumber); disk.setUnitNumber(deviceNumber); + if (maxIops != null && maxIops > 0) { + LOGGER.debug(LogUtils.logGsonWithoutException("Adding [%s] as the max IOPS of disk [%s].", maxIops, disk)); + StorageIOAllocationInfo storageIOAllocationInfo = new StorageIOAllocationInfo(); + storageIOAllocationInfo.setLimit(maxIops); + disk.setStorageIOAllocation(storageIOAllocationInfo); + } + VirtualDeviceConnectInfo connectInfo = new VirtualDeviceConnectInfo(); connectInfo.setConnected(true); connectInfo.setStartConnected(true); @@ -255,6 +266,9 @@ public static VirtualDevice prepareDiskDevice(VirtualMachineMO vmMo, VirtualDisk setParentBackingInfo(backingInfo, morDs, parentDisks); } + LOGGER.debug(LogUtils.logGsonWithoutException("Prepared disk device, to attach to virtual machine [%s], has the following details: Virtual device [%s], " + + "ManagedObjectReference [%s], ControllerKey [%s], VMDK path chain [%s], DeviceNumber [%s], ContextNumber [%s] and max IOPS [%s], is: [%s].", + vmMo, device, morDs, controllerKey, vmdkDatastorePathChain, deviceNumber, contextNumber, maxIops, disk)); return disk; } @@ -750,5 +764,4 @@ public static HostMO getHostMOFromHostName(final VmwareContext context, final St } return host; } - } diff --git a/vmware-base/src/test/java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java b/vmware-base/src/test/java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java new file mode 100644 index 000000000000..68e0dec00cac --- /dev/null +++ b/vmware-base/src/test/java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java @@ -0,0 +1,56 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package com.cloud.hypervisor.vmware.util; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.hypervisor.vmware.mo.VirtualMachineMO; +import com.vmware.vim25.VirtualDisk; + +@RunWith(MockitoJUnitRunner.class) +public class VmwareHelperTest { + @Mock + private VirtualMachineMO virtualMachineMO; + + @Test + public void prepareDiskDeviceTestNotLimitingIOPS() throws Exception { + Mockito.when(virtualMachineMO.getIDEDeviceControllerKey()).thenReturn(1); + VirtualDisk virtualDisk = (VirtualDisk) VmwareHelper.prepareDiskDevice(virtualMachineMO, null, -1, new String[1], null, 0, 0, null); + assertEquals(null, virtualDisk.getStorageIOAllocation()); + } + + @Test + public void prepareDiskDeviceTestLimitingIOPS() throws Exception { + Mockito.when(virtualMachineMO.getIDEDeviceControllerKey()).thenReturn(1); + VirtualDisk virtualDisk = (VirtualDisk) VmwareHelper.prepareDiskDevice(virtualMachineMO, null, -1, new String[1], null, 0, 0, Long.valueOf(1000)); + assertEquals(Long.valueOf(1000), virtualDisk.getStorageIOAllocation().getLimit()); + } + + @Test + public void prepareDiskDeviceTestLimitingIOPSToZero() throws Exception { + Mockito.when(virtualMachineMO.getIDEDeviceControllerKey()).thenReturn(1); + VirtualDisk virtualDisk = (VirtualDisk) VmwareHelper.prepareDiskDevice(virtualMachineMO, null, -1, new String[1], null, 0, 0, Long.valueOf(0)); + assertEquals(null, virtualDisk.getStorageIOAllocation()); + } +} \ No newline at end of file From 96c55a2fb2646be94b24762ef7de4be77fbfae16 Mon Sep 17 00:00:00 2001 From: SadiJr Date: Thu, 12 May 2022 11:21:55 -0300 Subject: [PATCH 2/4] Fix imports and checkstyle --- .../vmware/resource/VmwareResource.java | 4 +--- .../resource/VmwareStorageProcessor.java | 1 + .../main/java/com/cloud/utils/LogUtils.java | 24 +++++++++++++++++++ .../vmware/mo/VirtualMachineMO.java | 1 + .../hypervisor/vmware/util/VmwareHelper.java | 8 +++---- 5 files changed, 30 insertions(+), 8 deletions(-) diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java index 0a75816f388c..e39a48ef146c 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java @@ -51,6 +51,7 @@ import com.cloud.agent.api.PatchSystemVmCommand; import com.cloud.resource.ServerResourceBase; import com.cloud.utils.FileUtil; +import com.cloud.utils.LogUtils; import com.cloud.utils.validation.ChecksumUtil; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.storage.command.CopyCommand; @@ -321,7 +322,6 @@ import com.vmware.vim25.PerfMetricSeries; import com.vmware.vim25.PerfQuerySpec; import com.vmware.vim25.RuntimeFaultFaultMsg; -import com.vmware.vim25.StorageIOAllocationInfo; import com.vmware.vim25.StoragePodSummary; import com.vmware.vim25.ToolsUnavailableFaultMsg; import com.vmware.vim25.VAppOvfSectionInfo; @@ -940,7 +940,6 @@ private Answer execute(ResizeVolumeCommand cmd) { boolean volumePathChangeObserved = false; boolean datastoreChangeObserved = false; - StorageIOAllocationInfo limitIops = vdisk.first().getStorageIOAllocation(); Pair pathAndChainInfo = getNewPathAndChainInfoInDatastoreCluster(vmMo, path, chainInfo, managed, cmd.get_iScsiName(), poolUUID, cmd.getContextParam(DiskTO.PROTOCOL_TYPE)); Pair poolUUIDandChainInfo = getNewPoolUUIDAndChainInfoInDatastoreCluster(vmMo, path, chainInfo, managed, cmd.get_iScsiName(), poolUUID, cmd.getContextParam(DiskTO.PROTOCOL_TYPE)); @@ -965,7 +964,6 @@ private Answer execute(ResizeVolumeCommand cmd) { } disk.setCapacityInKB(newSize); - disk.setStorageIOAllocation(limitIops); VirtualDeviceConfigSpec deviceConfigSpec = new VirtualDeviceConfigSpec(); diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java index 8778d8169e3c..b67b3b3b6878 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java @@ -97,6 +97,7 @@ import com.cloud.storage.Volume; import com.cloud.storage.template.OVAProcessor; import com.cloud.template.TemplateManager; +import com.cloud.utils.LogUtils; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; import com.cloud.utils.exception.CloudRuntimeException; diff --git a/utils/src/main/java/com/cloud/utils/LogUtils.java b/utils/src/main/java/com/cloud/utils/LogUtils.java index a7e325794cea..edfb53fabf5b 100644 --- a/utils/src/main/java/com/cloud/utils/LogUtils.java +++ b/utils/src/main/java/com/cloud/utils/LogUtils.java @@ -20,8 +20,10 @@ package com.cloud.utils; import java.io.File; +import java.util.ArrayList; import java.util.Enumeration; import java.util.HashSet; +import java.util.List; import java.util.Set; import org.apache.log4j.Appender; @@ -29,8 +31,11 @@ import org.apache.log4j.Logger; import org.apache.log4j.xml.DOMConfigurator; +import com.google.gson.Gson; + public class LogUtils { public static final Logger LOGGER = Logger.getLogger(LogUtils.class); + private static final Gson GSON = new Gson(); private static String configFileLocation = null; @@ -72,4 +77,23 @@ public static Set getLogFileNames() { } return fileNames; } + + public static String logGsonWithoutException(String formatMessage, Object ... objects) { + List gsons = new ArrayList<>(); + for (Object object : objects) { + try { + gsons.add(GSON.toJson(object)); + } catch (Exception e) { + LOGGER.debug(String.format("Failed to log object [%s] using GSON.", object != null ? object.getClass().getSimpleName() : "null")); + gsons.add("error to decode"); + } + } + try { + return String.format(formatMessage, gsons.toArray()); + } catch (Exception e) { + String errorMsg = String.format("Failed to log objects using GSON due to: [%s].", e.getMessage()); + LOGGER.error(errorMsg, e); + return errorMsg; + } + } } diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java index 9ceaabf5d92d..e33a4d3472a7 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java @@ -51,6 +51,7 @@ import com.cloud.hypervisor.vmware.util.VmwareContext; import com.cloud.hypervisor.vmware.util.VmwareHelper; import com.cloud.utils.ActionDelegate; +import com.cloud.utils.LogUtils; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; import com.cloud.utils.concurrency.NamedThreadFactory; diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java index 28e3ee3161a4..cad6e68ecd9f 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java @@ -52,7 +52,6 @@ import com.cloud.utils.Pair; import com.cloud.utils.Ternary; import com.cloud.utils.exception.ExceptionUtil; -import com.google.gson.Gson; import com.vmware.vim25.DistributedVirtualSwitchPortConnection; import com.vmware.vim25.DynamicProperty; import com.vmware.vim25.GuestOsDescriptor; @@ -90,7 +89,6 @@ import com.vmware.vim25.VirtualVmxnet3; public class VmwareHelper { - @SuppressWarnings("unused") private static final Logger s_logger = Logger.getLogger(VmwareHelper.class); public static final int MAX_SCSI_CONTROLLER_COUNT = 4; @@ -218,7 +216,7 @@ public static VirtualDevice prepareDvNicDevice(VirtualMachineMO vmMo, ManagedObj // vmdkDatastorePath: [datastore name] vmdkFilePath public static VirtualDevice prepareDiskDevice(VirtualMachineMO vmMo, VirtualDisk device, int controllerKey, String vmdkDatastorePathChain[], ManagedObjectReference morDs, int deviceNumber, int contextNumber, Long maxIops) throws Exception { - LOGGER.debug(LogUtils.logGsonWithoutException("Trying to prepare disk device to virtual machine [%s], using the following details: Virtual device [%s], " + s_logger.debug(LogUtils.logGsonWithoutException("Trying to prepare disk device to virtual machine [%s], using the following details: Virtual device [%s], " + "ManagedObjectReference [%s], ControllerKey [%s], VMDK path chain [%s], DeviceNumber [%s], ContextNumber [%s] and max IOPS [%s].", vmMo, device, morDs, controllerKey, vmdkDatastorePathChain, deviceNumber, contextNumber, maxIops)); assert (vmdkDatastorePathChain != null); @@ -248,7 +246,7 @@ public static VirtualDevice prepareDiskDevice(VirtualMachineMO vmMo, VirtualDisk disk.setUnitNumber(deviceNumber); if (maxIops != null && maxIops > 0) { - LOGGER.debug(LogUtils.logGsonWithoutException("Adding [%s] as the max IOPS of disk [%s].", maxIops, disk)); + s_logger.debug(LogUtils.logGsonWithoutException("Adding [%s] as the max IOPS of disk [%s].", maxIops, disk)); StorageIOAllocationInfo storageIOAllocationInfo = new StorageIOAllocationInfo(); storageIOAllocationInfo.setLimit(maxIops); disk.setStorageIOAllocation(storageIOAllocationInfo); @@ -266,7 +264,7 @@ public static VirtualDevice prepareDiskDevice(VirtualMachineMO vmMo, VirtualDisk setParentBackingInfo(backingInfo, morDs, parentDisks); } - LOGGER.debug(LogUtils.logGsonWithoutException("Prepared disk device, to attach to virtual machine [%s], has the following details: Virtual device [%s], " + s_logger.debug(LogUtils.logGsonWithoutException("Prepared disk device, to attach to virtual machine [%s], has the following details: Virtual device [%s], " + "ManagedObjectReference [%s], ControllerKey [%s], VMDK path chain [%s], DeviceNumber [%s], ContextNumber [%s] and max IOPS [%s], is: [%s].", vmMo, device, morDs, controllerKey, vmdkDatastorePathChain, deviceNumber, contextNumber, maxIops, disk)); return disk; From 6b9faf7587589c3eebc49348923510a83860788e Mon Sep 17 00:00:00 2001 From: SadiJr Date: Thu, 12 May 2022 12:23:21 -0300 Subject: [PATCH 3/4] Add more tests --- .../java/com/cloud/utils/LogUtilsTest.java | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 utils/src/test/java/com/cloud/utils/LogUtilsTest.java diff --git a/utils/src/test/java/com/cloud/utils/LogUtilsTest.java b/utils/src/test/java/com/cloud/utils/LogUtilsTest.java new file mode 100644 index 000000000000..350c9e946cd1 --- /dev/null +++ b/utils/src/test/java/com/cloud/utils/LogUtilsTest.java @@ -0,0 +1,39 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.utils; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; + +public class LogUtilsTest { + + @Test + public void logGsonWithoutExceptionTestLogCorrectlyPrimitives() { + String expected = "test primitives: int [1], double [1.11], float [1.2222], boolean [true], null [], char [\"c\"]."; + String log = LogUtils.logGsonWithoutException("test primitives: int [%s], double [%s], float [%s], boolean [%s], null [%s], char [%s].", + 1, 1.11d, 1.2222f, true, null, 'c'); + assertEquals(expected, log); + } + + @Test + public void logGsonWithoutExceptionTestPassWrongNumberOfArgs() { + String expected = "Failed to log objects using GSON due to: [Format specifier '%s']."; + String result = LogUtils.logGsonWithoutException("teste wrong [%s] %s args.", "blablabla"); + assertEquals(expected, result); + } +} From 0cf69aa51e12af98c946b70fdf586d203f21da80 Mon Sep 17 00:00:00 2001 From: SadiJr Date: Mon, 16 Jan 2023 09:48:52 -0300 Subject: [PATCH 4/4] Address reviews --- .../java/com/cloud/hypervisor/vmware/util/VmwareHelper.java | 2 +- .../java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java index cad6e68ecd9f..4a81beeff989 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java @@ -246,7 +246,7 @@ public static VirtualDevice prepareDiskDevice(VirtualMachineMO vmMo, VirtualDisk disk.setUnitNumber(deviceNumber); if (maxIops != null && maxIops > 0) { - s_logger.debug(LogUtils.logGsonWithoutException("Adding [%s] as the max IOPS of disk [%s].", maxIops, disk)); + s_logger.debug(LogUtils.logGsonWithoutException("Defining [%s] as the max IOPS of disk [%s].", maxIops, disk)); StorageIOAllocationInfo storageIOAllocationInfo = new StorageIOAllocationInfo(); storageIOAllocationInfo.setLimit(maxIops); disk.setStorageIOAllocation(storageIOAllocationInfo); diff --git a/vmware-base/src/test/java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java b/vmware-base/src/test/java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java index 68e0dec00cac..4417748efbf0 100644 --- a/vmware-base/src/test/java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java +++ b/vmware-base/src/test/java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java @@ -53,4 +53,4 @@ public void prepareDiskDeviceTestLimitingIOPSToZero() throws Exception { VirtualDisk virtualDisk = (VirtualDisk) VmwareHelper.prepareDiskDevice(virtualMachineMO, null, -1, new String[1], null, 0, 0, Long.valueOf(0)); assertEquals(null, virtualDisk.getStorageIOAllocation()); } -} \ No newline at end of file +}