This is an automated email from the ASF dual-hosted git repository. harikrishna pushed a commit to branch CheckVolumeAPI in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit b3d4f56f69f07eac92df774aa958f47b88e3e395 Author: Harikrishna Patnala <[email protected]> AuthorDate: Tue Jan 9 16:28:19 2024 +0530 addressed review comments --- .../user/volume/CheckVolumeAndRepairCmd.java | 2 +- .../api/storage/CheckVolumeAndRepairAnswer.java | 33 +++++----------------- .../storage/volume/VolumeServiceImpl.java | 4 +-- .../LibvirtCheckVolumeAndRepairCommandWrapper.java | 2 +- .../org/apache/cloudstack/utils/qemu/QemuImg.java | 4 ++- .../com/cloud/storage/VolumeApiServiceImpl.java | 20 +++++-------- .../cloud/storage/VolumeApiServiceImplTest.java | 10 +++---- .../src/main/java/com/cloud/utils/StringUtils.java | 2 +- 8 files changed, 27 insertions(+), 50 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckVolumeAndRepairCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckVolumeAndRepairCmd.java index 268ccdcc0a4..0aa221dd43d 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckVolumeAndRepairCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckVolumeAndRepairCmd.java @@ -52,7 +52,7 @@ public class CheckVolumeAndRepairCmd extends BaseCmd { @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = VolumeResponse.class, required = true, description = "The ID of the volume") private Long id; - @Parameter(name = ApiConstants.REPAIR, type = CommandType.BOOLEAN, required = false, description = "true if the volume has leaks and repair the volume") + @Parameter(name = ApiConstants.REPAIR, type = CommandType.BOOLEAN, required = false, description = "true to repair the volume, repairs if it has any leaks") private Boolean repair; ///////////////////////////////////////////////////// diff --git a/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairAnswer.java b/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairAnswer.java index 526e8933406..25fc9082787 100644 --- a/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairAnswer.java +++ b/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairAnswer.java @@ -22,55 +22,36 @@ package com.cloud.agent.api.storage; import com.cloud.agent.api.Answer; public class CheckVolumeAndRepairAnswer extends Answer { - private long leaks; - private boolean repaired; - private long leaksFixed; private String volumeCheckExecutionResult; - private String volumeRepairedExecutionResult; + private String volumeRepairExecutionResult; protected CheckVolumeAndRepairAnswer() { super(); } - public CheckVolumeAndRepairAnswer(CheckVolumeAndRepairCommand cmd, boolean result, String details, long leaks, - boolean repaired, long leaksFixed, String volumeCheckExecutionResult, String volumeRepairedExecutionResult) { + public CheckVolumeAndRepairAnswer(CheckVolumeAndRepairCommand cmd, boolean result, String details, String volumeCheckExecutionResult, String volumeRepairedExecutionResult) { super(cmd, result, details); - this.leaks = leaks; - this.repaired = repaired; - this.leaksFixed = leaksFixed; this.volumeCheckExecutionResult = volumeCheckExecutionResult; - this.volumeRepairedExecutionResult = volumeRepairedExecutionResult; + this.volumeRepairExecutionResult = volumeRepairedExecutionResult; } public CheckVolumeAndRepairAnswer(CheckVolumeAndRepairCommand cmd, boolean result, String details) { super(cmd, result, details); } - public long getLeaks() { - return leaks; - } - - public boolean isRepaired() { - return repaired; - } - - public long getLeaksFixed() { - return leaksFixed; - } - public String getVolumeCheckExecutionResult() { return volumeCheckExecutionResult; } - public String getVolumeRepairedExecutionResult() { - return volumeRepairedExecutionResult; + public String getVolumeRepairExecutionResult() { + return volumeRepairExecutionResult; } public void setVolumeCheckExecutionResult(String volumeCheckExecutionResult) { this.volumeCheckExecutionResult = volumeCheckExecutionResult; } - public void setVolumeRepairedExecutionResult(String volumeRepairedExecutionResult) { - this.volumeRepairedExecutionResult = volumeRepairedExecutionResult; + public void setVolumeRepairExecutionResult(String volumeRepairExecutionResult) { + this.volumeRepairExecutionResult = volumeRepairExecutionResult; } } diff --git a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java index e89c0cfb117..bf5ebed87de 100644 --- a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java +++ b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java @@ -2777,9 +2777,9 @@ public class VolumeServiceImpl implements VolumeService { s_logger.debug("Check volume response result: " + answer.getDetails()); payload.setVolumeCheckExecutionResult(answer.getVolumeCheckExecutionResult()); if (payload.isRepair()) { - payload.setVolumeRepairedExecutionResult(answer.getVolumeRepairedExecutionResult()); + payload.setVolumeRepairedExecutionResult(answer.getVolumeRepairExecutionResult()); } - return new Pair<>(answer.getVolumeCheckExecutionResult(), answer.getVolumeRepairedExecutionResult()); + return new Pair<>(answer.getVolumeCheckExecutionResult(), answer.getVolumeRepairExecutionResult()); } else { s_logger.debug("Failed to check and repair the volume with error " + answer.getDetails()); } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapper.java index 42fd020d9bc..b70ce21f378 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapper.java @@ -78,7 +78,7 @@ public class LibvirtCheckVolumeAndRepairCommandWrapper extends CommandWrapper<Ch s_logger.info(String.format("Repair Volume result for the volume %s is %s", vol.getName(), repairVolumeResult)); answer = new CheckVolumeAndRepairAnswer(command, true, finalResult); - answer.setVolumeRepairedExecutionResult(repairVolumeResult); + answer.setVolumeRepairExecutionResult(repairVolumeResult); answer.setVolumeCheckExecutionResult(checkVolumeResult); } return answer; diff --git a/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java b/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java index 9a71a596420..4a888056d63 100644 --- a/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java +++ b/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java @@ -826,7 +826,9 @@ public class QemuImg { public String checkAndRepair(final QemuImgFile file, final QemuImageOptions imageOptions, final List<QemuObject> qemuObjects, final boolean repair) throws QemuImgException { final Script s = new Script(_qemuImgPath); s.add("check"); - s.add(file.getFileName()); + if (imageOptions == null) { + s.add(file.getFileName()); + } for (QemuObject o : qemuObjects) { s.add(o.toCommandFlag()); diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index efef67587d0..44b00f87b64 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1825,23 +1825,18 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic long volumeId = cmd.getId(); boolean repair = cmd.getRepair(); - validationsForCheckVolumeOperation(volumeId); - final VolumeVO volume = _volsDao.findById(volumeId); + validationsForCheckVolumeOperation(volume); + Long vmId = volume.getInstanceId(); - UserVmVO vm = null; if (vmId != null) { - vm = _userVmDao.findById(vmId); - } - - if (vm != null) { // serialize VM operation AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext(); if (jobContext.isJobDispatchedBy(VmWorkConstants.VM_WORK_JOB_DISPATCHER)) { // avoid re-entrance VmWorkJobVO placeHolder = null; - placeHolder = createPlaceHolderWork(vm.getId()); + placeHolder = createPlaceHolderWork(vmId); try { Pair<String, String> result = orchestrateCheckVolumeAndRepair(volumeId, repair); return result; @@ -1849,7 +1844,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic _workJobDao.expunge(placeHolder.getId()); } } else { - Outcome<Pair> outcome = checkVolumeAndRepairThroughJobQueue(vm.getId(), volumeId, repair); + Outcome<Pair> outcome = checkVolumeAndRepairThroughJobQueue(vmId, volumeId, repair); try { outcome.get(); @@ -1887,15 +1882,14 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic } } - protected void validationsForCheckVolumeOperation(long volumeId) { - final VolumeVO volume = _volsDao.findById(volumeId); + protected void validationsForCheckVolumeOperation(VolumeVO volume) { Account caller = CallContext.current().getCallingAccount(); _accountMgr.checkAccess(caller, null, true, volume); + Long volumeId = volume.getId(); Long vmId = volume.getInstanceId(); - UserVmVO vm = null; if (vmId != null) { - vm = _userVmDao.findById(vmId); + UserVmVO vm = _userVmDao.findById(vmId); if (vm == null) { throw new InvalidParameterValueException(String.format("VM not found, please check the VM to which this volume %d is attached", volumeId)); } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 7631ade441f..fb3c842e6c0 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -1682,7 +1682,7 @@ public class VolumeApiServiceImplTest { when(volume.getId()).thenReturn(1L); when(volumeDaoMock.getHypervisorType(1L)).thenReturn(HypervisorType.KVM); - volumeApiServiceImpl.validationsForCheckVolumeOperation(1L); + volumeApiServiceImpl.validationsForCheckVolumeOperation(volume); } @Test(expected = InvalidParameterValueException.class) @@ -1701,7 +1701,7 @@ public class VolumeApiServiceImplTest { when(userVmDaoMock.findById(1L)).thenReturn(vm); when(vm.getState()).thenReturn(State.Running); - volumeApiServiceImpl.validationsForCheckVolumeOperation(1L); + volumeApiServiceImpl.validationsForCheckVolumeOperation(volume); } @Test(expected = InvalidParameterValueException.class) @@ -1718,7 +1718,7 @@ public class VolumeApiServiceImplTest { when(volume.getInstanceId()).thenReturn(1L); when(userVmDaoMock.findById(1L)).thenReturn(null); - volumeApiServiceImpl.validationsForCheckVolumeOperation(1L); + volumeApiServiceImpl.validationsForCheckVolumeOperation(volume); } @Test(expected = InvalidParameterValueException.class) @@ -1738,7 +1738,7 @@ public class VolumeApiServiceImplTest { when(vm.getState()).thenReturn(State.Stopped); when(volume.getState()).thenReturn(Volume.State.Allocated); - volumeApiServiceImpl.validationsForCheckVolumeOperation(1L); + volumeApiServiceImpl.validationsForCheckVolumeOperation(volume); } @Test(expected = InvalidParameterValueException.class) @@ -1760,7 +1760,7 @@ public class VolumeApiServiceImplTest { when(volume.getId()).thenReturn(1L); when(volumeDaoMock.getHypervisorType(1L)).thenReturn(HypervisorType.VMware); - volumeApiServiceImpl.validationsForCheckVolumeOperation(1L); + volumeApiServiceImpl.validationsForCheckVolumeOperation(volume); } @Test diff --git a/utils/src/main/java/com/cloud/utils/StringUtils.java b/utils/src/main/java/com/cloud/utils/StringUtils.java index ac9e6b09329..fd4f7aba698 100644 --- a/utils/src/main/java/com/cloud/utils/StringUtils.java +++ b/utils/src/main/java/com/cloud/utils/StringUtils.java @@ -291,7 +291,7 @@ public class StringUtils { ObjectMapper objectMapper = new ObjectMapper(); Map<String, String> mapResult = new HashMap<>(); - if (org.apache.commons.lang3.StringUtils.isNotEmpty(jsonString)) { + if (org.apache.commons.lang3.StringUtils.isNotBlank(jsonString)) { try { JsonNode jsonNode = objectMapper.readTree(jsonString); jsonNode.fields().forEachRemaining(entry -> {
