From 2a5ca85f26b6eeeb104c5bfe6666001e9942d55a Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 20 Feb 2024 15:33:44 +0530 Subject: [PATCH 01/23] Introduced a new API checkVolumeAndRepair that allows users or admins to check and repair if any leaks observed. Currently this is supported only for KVM --- .../main/java/com/cloud/event/EventTypes.java | 1 + .../com/cloud/storage/VolumeApiService.java | 4 + .../apache/cloudstack/api/ApiConstants.java | 4 + .../admin/volume/RecoverVolumeCmdByAdmin.java | 2 +- .../user/volume/CheckVolumeAndRepairCmd.java | 116 +++++++++++++ .../api/response/VolumeResponse.java | 25 +++ .../storage/CheckVolumeAndRepairAnswer.java | 76 +++++++++ .../storage/CheckVolumeAndRepairCommand.java | 77 +++++++++ .../subsystem/api/storage/VolumeService.java | 2 + .../cloud/vm/VmWorkCheckAndRepairVolume.java | 41 +++++ .../storage/volume/VolumeServiceImpl.java | 30 ++++ ...irtCheckVolumeAndRepairCommandWrapper.java | 116 +++++++++++++ .../apache/cloudstack/utils/qemu/QemuImg.java | 39 +++++ .../kvm/storage/LibvirtStoragePoolTest.java | 3 + .../cloud/server/ManagementServerImpl.java | 2 + .../storage/CheckAndRepairVolumePayload.java | 58 +++++++ .../cloud/storage/VolumeApiServiceImpl.java | 161 ++++++++++++++++++ .../java/com/cloud/utils/StringUtils.java | 22 +++ 18 files changed, 778 insertions(+), 1 deletion(-) create mode 100644 api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckVolumeAndRepairCmd.java create mode 100644 core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairAnswer.java create mode 100644 core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java create mode 100644 engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java create mode 100644 plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapper.java create mode 100644 server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java diff --git a/api/src/main/java/com/cloud/event/EventTypes.java b/api/src/main/java/com/cloud/event/EventTypes.java index 5d5252290959..c4833d3433ae 100644 --- a/api/src/main/java/com/cloud/event/EventTypes.java +++ b/api/src/main/java/com/cloud/event/EventTypes.java @@ -303,6 +303,7 @@ public class EventTypes { public static final String EVENT_VOLUME_CREATE = "VOLUME.CREATE"; public static final String EVENT_VOLUME_DELETE = "VOLUME.DELETE"; public static final String EVENT_VOLUME_ATTACH = "VOLUME.ATTACH"; + public static final String EVENT_VOLUME_CHECK = "VOLUME.CHECK"; public static final String EVENT_VOLUME_DETACH = "VOLUME.DETACH"; public static final String EVENT_VOLUME_EXTRACT = "VOLUME.EXTRACT"; public static final String EVENT_VOLUME_UPLOAD = "VOLUME.UPLOAD"; diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java index 8d5f7892f102..652d4ab3cc8a 100644 --- a/api/src/main/java/com/cloud/storage/VolumeApiService.java +++ b/api/src/main/java/com/cloud/storage/VolumeApiService.java @@ -22,9 +22,11 @@ import java.util.List; import java.util.Map; +import com.cloud.utils.Pair; import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd; import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ChangeOfferingForVolumeCmd; +import org.apache.cloudstack.api.command.user.volume.CheckVolumeAndRepairCmd; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; import org.apache.cloudstack.api.command.user.volume.DetachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ExtractVolumeCmd; @@ -178,4 +180,6 @@ Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account acc void publishVolumeCreationUsageEvent(Volume volume); boolean stateTransitTo(Volume vol, Volume.Event event) throws NoTransitionException; + + Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) throws ResourceAllocationException; } diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java index 3ae0f319189c..18d25a0cfc3f 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java @@ -378,6 +378,7 @@ public class ApiConstants { public static final String RECEIVED_BYTES = "receivedbytes"; public static final String RECONNECT = "reconnect"; public static final String RECOVER = "recover"; + public static final String REPAIR = "repair"; public static final String REQUIRES_HVM = "requireshvm"; public static final String RESOURCE_NAME = "resourcename"; public static final String RESOURCE_TYPE = "resourcetype"; @@ -501,6 +502,9 @@ public class ApiConstants { public static final String IS_VOLATILE = "isvolatile"; public static final String VOLUME_ID = "volumeid"; public static final String VOLUMES = "volumes"; + public static final String VOLUME_CHECK_RESULT = "volumecheckresult"; + public static final String VOLUME_REPAIR_RESULT = "volumerepairresult"; + public static final String ZONE = "zone"; public static final String ZONE_ID = "zoneid"; public static final String ZONE_NAME = "zonename"; diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/volume/RecoverVolumeCmdByAdmin.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/volume/RecoverVolumeCmdByAdmin.java index f51aeec97197..92c62fa86629 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/volume/RecoverVolumeCmdByAdmin.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/volume/RecoverVolumeCmdByAdmin.java @@ -30,7 +30,7 @@ import com.cloud.storage.Volume; -@APICommand(name = "recoverVolume", description = "Recovers a Destroy volume.", responseObject = VolumeResponse.class, responseView = ResponseView.Full, entityType = {Volume.class}, +@APICommand(name = "recoverVolume", description = "Recovers a Destroy volume, curr", responseObject = VolumeResponse.class, responseView = ResponseView.Full, entityType = {Volume.class}, since = "4.14.0", authorized = {RoleType.Admin}, requestHasSensitiveInfo = false, 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 new file mode 100644 index 000000000000..268ccdcc0a45 --- /dev/null +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckVolumeAndRepairCmd.java @@ -0,0 +1,116 @@ +// 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 org.apache.cloudstack.api.command.user.volume; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiCommandResourceType; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ResponseObject.ResponseView; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.VolumeResponse; +import org.apache.cloudstack.context.CallContext; +import org.apache.log4j.Logger; + +import com.cloud.exception.ResourceAllocationException; +import com.cloud.storage.Volume; +import com.cloud.user.Account; +import com.cloud.utils.Pair; +import com.cloud.utils.StringUtils; + +@APICommand(name = "checkVolumeAndRepair", description = "Check the volume and repair if needed, this is currently supported for KVM only", responseObject = VolumeResponse.class, entityType = {Volume.class}, + since = "4.18.1", + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}, + requestHasSensitiveInfo = false, + responseHasSensitiveInfo = true) +public class CheckVolumeAndRepairCmd extends BaseCmd { + public static final Logger s_logger = Logger.getLogger(CheckVolumeAndRepairCmd.class.getName()); + + private static final String s_name = "checkvolumeandrepairresponse"; + + ///////////////////////////////////////////////////// + //////////////// API parameters ///////////////////// + ///////////////////////////////////////////////////// + + @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") + private Boolean repair; + + ///////////////////////////////////////////////////// + /////////////////// Accessors /////////////////////// + ///////////////////////////////////////////////////// + + public Long getId() { + return id; + } + + public boolean getRepair() { + return repair == null ? false : repair; + } + + ///////////////////////////////////////////////////// + /////////////// API Implementation/////////////////// + ///////////////////////////////////////////////////// + + @Override + public String getCommandName() { + return s_name; + } + + @Override + public long getEntityOwnerId() { + Volume volume = _entityMgr.findById(Volume.class, getId()); + if (volume != null) { + return volume.getAccountId(); + } + + return Account.ACCOUNT_ID_SYSTEM; // no account info given, parent this command to SYSTEM so ERROR events are tracked + } + + @Override + public Long getApiResourceId() { + return id; + } + + @Override + public ApiCommandResourceType getApiResourceType() { + return ApiCommandResourceType.Volume; + } + + @Override + public void execute() throws ResourceAllocationException { + CallContext.current().setEventDetails("Volume Id: " + getId()); + Pair result = _volumeService.checkAndRepairVolume(this); + Volume volume = _responseGenerator.findVolumeById(getId()); + if (result != null) { + VolumeResponse response = _responseGenerator.createVolumeResponse(ResponseView.Full, volume); + response.setVolumeCheckResult(StringUtils.parseJsonToMap(result.first())); + if (getRepair()) { + response.setVolumeRepairResult(StringUtils.parseJsonToMap(result.second())); + } + response.setResponseName(getCommandName()); + setResponseObject(response); + } else { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to check volume and repair"); + } + } +} diff --git a/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java index 00a1eabc40ba..785284ad46fa 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java @@ -18,6 +18,7 @@ import java.util.Date; import java.util.LinkedHashSet; +import java.util.Map; import java.util.Set; import org.apache.cloudstack.acl.RoleType; @@ -288,6 +289,14 @@ public class VolumeResponse extends BaseResponseWithTagInformation implements Co @Param(description = "volume uuid that is given by virtualisation provider (only for VMware)") private String externalUuid; + @SerializedName(ApiConstants.VOLUME_CHECK_RESULT) + @Param(description = "details for the volume check result") + private Map volumeCheckResult; + + @SerializedName(ApiConstants.VOLUME_REPAIR_RESULT) + @Param(description = "details for the volume repair result") + private Map volumeRepairResult; + public String getPath() { return path; } @@ -817,4 +826,20 @@ public String getExternalUuid() { public void setExternalUuid(String externalUuid) { this.externalUuid = externalUuid; } + + public Map getVolumeCheckResult() { + return volumeCheckResult; + } + + public void setVolumeCheckResult(Map volumeCheckResult) { + this.volumeCheckResult = volumeCheckResult; + } + + public Map getVolumeRepairResult() { + return volumeRepairResult; + } + + public void setVolumeRepairResult(Map volumeRepairResult) { + this.volumeRepairResult = volumeRepairResult; + } } 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 new file mode 100644 index 000000000000..526e89334064 --- /dev/null +++ b/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairAnswer.java @@ -0,0 +1,76 @@ +// +// 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.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; + + protected CheckVolumeAndRepairAnswer() { + super(); + } + + public CheckVolumeAndRepairAnswer(CheckVolumeAndRepairCommand cmd, boolean result, String details, long leaks, + boolean repaired, long leaksFixed, String volumeCheckExecutionResult, String volumeRepairedExecutionResult) { + super(cmd, result, details); + this.leaks = leaks; + this.repaired = repaired; + this.leaksFixed = leaksFixed; + this.volumeCheckExecutionResult = volumeCheckExecutionResult; + this.volumeRepairedExecutionResult = 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 void setVolumeCheckExecutionResult(String volumeCheckExecutionResult) { + this.volumeCheckExecutionResult = volumeCheckExecutionResult; + } + + public void setVolumeRepairedExecutionResult(String volumeRepairedExecutionResult) { + this.volumeRepairedExecutionResult = volumeRepairedExecutionResult; + } +} diff --git a/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java b/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java new file mode 100644 index 000000000000..040e6ca86084 --- /dev/null +++ b/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java @@ -0,0 +1,77 @@ +// +// 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.agent.api.storage; + +import com.cloud.agent.api.Command; +import com.cloud.agent.api.LogLevel; +import com.cloud.agent.api.to.StorageFilerTO; + +import java.util.Arrays; + +public class CheckVolumeAndRepairCommand extends Command { + private String path; + private StorageFilerTO pool; + private boolean repair; + @LogLevel(LogLevel.Log4jLevel.Off) + private byte[] passphrase; + private String encryptFormat; + + public CheckVolumeAndRepairCommand(String path, StorageFilerTO pool, boolean repair, byte[] passphrase, String encryptFormat) { + this.path = path; + this.pool = pool; + this.repair = repair; + this.passphrase = passphrase; + this.encryptFormat = encryptFormat; + } + + public String getPath() { + return path; + } + + public String getPoolUuid() { + return pool.getUuid(); + } + + public StorageFilerTO getPool() { + return pool; + } + + public boolean needRepair() { + return repair; + } + + public String getEncryptFormat() { return encryptFormat; } + + public byte[] getPassphrase() { return passphrase; } + + public void clearPassphrase() { + if (this.passphrase != null) { + Arrays.fill(this.passphrase, (byte) 0); + } + } + + /** + * {@inheritDoc} + */ + @Override + public boolean executeInSequence() { + return false; + } +} diff --git a/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java b/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java index 50aee83f4979..4350e2641250 100644 --- a/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java +++ b/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java @@ -115,4 +115,6 @@ boolean copyPoliciesBetweenVolumesAndDestroySourceVolumeAfterMigration(ObjectInD VolumeInfo sourceVolume, VolumeInfo destinationVolume, boolean retryExpungeVolumeAsync); void moveVolumeOnSecondaryStorageToAnotherAccount(Volume volume, Account sourceAccount, Account destAccount); + + Pair checkAndRepairVolume(VolumeInfo volume); } diff --git a/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java b/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java new file mode 100644 index 000000000000..f4053dd17419 --- /dev/null +++ b/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java @@ -0,0 +1,41 @@ +// 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.vm; + +public class VmWorkCheckAndRepairVolume extends VmWork { + + private static final long serialVersionUID = 341816293003023823L; + + private Long volumeId; + + private boolean repair; + + public VmWorkCheckAndRepairVolume(long userId, long accountId, long vmId, String handlerName, + Long volumeId, boolean repair) { + super(userId, accountId, vmId, handlerName); + this.repair = repair; + this.volumeId = volumeId; + } + + public Long getVolumeId() { + return volumeId; + } + + public boolean needRepair() { + return repair; + } +} 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 8a3cd39ecbf6..8c839a9b4d4d 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 @@ -2760,6 +2760,36 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) { return snapshot; } + @Override + public Pair checkAndRepairVolume(VolumeInfo volume) { + Long poolId = volume.getPoolId(); + StoragePool pool = _storageMgr.getStoragePool(poolId); + CheckAndRepairVolumePayload payload = (CheckAndRepairVolumePayload) volume.getpayload(); + CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.isRepair(), + volume.getPassphrase(), volume.getEncryptFormat()); + + try { + CheckVolumeAndRepairAnswer answer = (CheckVolumeAndRepairAnswer) _storageMgr.sendToPool(pool, null, command); + if (answer != null && answer.getResult()) { + s_logger.debug("Check volume response result: " + answer.getDetails()); + payload.setVolumeCheckExecutionResult(answer.getVolumeCheckExecutionResult()); + if (payload.isRepair()) { + payload.setVolumeRepairedExecutionResult(answer.getVolumeRepairedExecutionResult()); + } + return new Pair<>(answer.getVolumeCheckExecutionResult(), answer.getVolumeRepairedExecutionResult()); + } else { + s_logger.debug("Failed to check and repair the volume with error " + answer.getDetails()); + } + + } catch (Exception e) { + s_logger.debug("sending check and repair volume command failed", e); + } finally { + command.clearPassphrase(); + } + + return null; + } + // For managed storage on Xen and VMware, we need to potentially make space for hypervisor snapshots. // The disk offering can collect this information and pass it on to the volume that's about to be created. // Ex. if you want a 10 GB CloudStack volume to reside on managed storage on Xen, this leads to an SR 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 new file mode 100644 index 000000000000..553de2953c13 --- /dev/null +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapper.java @@ -0,0 +1,116 @@ +// +// 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.kvm.resource.wrapper; + +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.storage.CheckVolumeAndRepairCommand; +import com.cloud.agent.api.storage.CheckVolumeAndRepairAnswer; +import com.cloud.agent.api.to.StorageFilerTO; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; +import com.cloud.hypervisor.kvm.storage.KVMStoragePool; +import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; +import com.cloud.storage.Storage; +import com.cloud.resource.CommandWrapper; +import com.cloud.resource.ResourceWrapper; +import com.cloud.utils.exception.CloudRuntimeException; +import org.apache.cloudstack.utils.cryptsetup.KeyFile; +import org.apache.cloudstack.utils.qemu.QemuImageOptions; +import org.apache.cloudstack.utils.qemu.QemuImg; +import org.apache.cloudstack.utils.qemu.QemuImgException; +import org.apache.cloudstack.utils.qemu.QemuImgFile; +import org.apache.cloudstack.utils.qemu.QemuObject; +import org.apache.commons.lang.ArrayUtils; +import org.apache.log4j.Logger; +import org.libvirt.LibvirtException; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +@ResourceWrapper(handles = CheckVolumeAndRepairCommand.class) +public final class LibvirtCheckVolumeAndRepairCommandWrapper extends CommandWrapper { + + private static final Logger s_logger = Logger.getLogger(LibvirtCheckVolumeAndRepairCommandWrapper.class); + + @Override + public Answer execute(CheckVolumeAndRepairCommand command, LibvirtComputingResource serverResource) { + final String volumeId = command.getPath(); + final boolean repair = command.needRepair(); + final StorageFilerTO spool = command.getPool(); + + final KVMStoragePoolManager storagePoolMgr = serverResource.getStoragePoolMgr(); + KVMStoragePool pool = storagePoolMgr.getStoragePool(spool.getType(), spool.getUuid()); + + if (spool.getType().equals(Storage.StoragePoolType.PowerFlex)) { + pool.connectPhysicalDisk(volumeId, null); + } + + final KVMPhysicalDisk vol = pool.getPhysicalDisk(volumeId); + QemuObject.EncryptFormat encryptFormat = QemuObject.EncryptFormat.enumValue(command.getEncryptFormat()); + try { + String checkVolumeResult = checkVolumeAndRepair(vol, false, encryptFormat, command.getPassphrase(), serverResource); + s_logger.info(String.format("Check Volume result is %s", checkVolumeResult)); + CheckVolumeAndRepairAnswer answer = new CheckVolumeAndRepairAnswer(command, true, checkVolumeResult); + answer.setVolumeCheckExecutionResult(checkVolumeResult); + + if (repair) { + String repairVolumeResult = checkVolumeAndRepair(vol, true, encryptFormat, command.getPassphrase(), serverResource); + String finalResult = (checkVolumeResult != null ? checkVolumeResult.concat(",") : "") + repairVolumeResult; + 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.setVolumeCheckExecutionResult(checkVolumeResult); + } + return answer; + } catch (Exception e) { + return new CheckVolumeAndRepairAnswer(command, false, e.toString()); + } + } + + private String checkVolumeAndRepair(final KVMPhysicalDisk vol, final boolean repair, final QemuObject.EncryptFormat encryptFormat, byte[] passphrase, final LibvirtComputingResource libvirtComputingResource) throws CloudRuntimeException { + List passphraseObjects = new ArrayList<>(); + QemuImageOptions imgOptions = null; + if (ArrayUtils.isEmpty(passphrase)) { + passphrase = null; + } + try (KeyFile keyFile = new KeyFile(passphrase)) { + if (passphrase != null) { + passphraseObjects.add( + QemuObject.prepareSecretForQemuImg(vol.getFormat(), encryptFormat, keyFile.toString(), "sec0", null) + ); + imgOptions = new QemuImageOptions(vol.getFormat(), vol.getPath(),"sec0"); + } + QemuImg q = new QemuImg(libvirtComputingResource.getCmdsTimeout()); + QemuImgFile file = new QemuImgFile(vol.getPath()); + return q.checkAndRepair(file, imgOptions, passphraseObjects, repair); + } catch (QemuImgException | LibvirtException ex) { + throw new CloudRuntimeException("Failed to run qemu-img for check volume", ex); + } catch (IOException ex) { + throw new CloudRuntimeException("Failed to create keyfile for encrypted volume for check volume operation", ex); + } finally { + if (passphrase != null) { + Arrays.fill(passphrase, (byte) 0); + } + } + } +} 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 1cd63b9b5661..9a71a5964208 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 @@ -812,4 +812,43 @@ protected static boolean helpSupportsImageFormat(String text, QemuImg.PhysicalDi Pattern pattern = Pattern.compile("Supported\\sformats:[a-zA-Z0-9-_\\s]*?\\b" + format + "\\b", CASE_INSENSITIVE); return pattern.matcher(text).find(); } + + /** + * check for any leaks for an image and repair. + * + * @param imageOptions + * Qemu style image options to be used in the checking process. + * @param qemuObjects + * Qemu style options (e.g. for passing secrets). + * @param repair + * Boolean option whether to repair any leaks + */ + public String checkAndRepair(final QemuImgFile file, final QemuImageOptions imageOptions, final List qemuObjects, final boolean repair) throws QemuImgException { + final Script s = new Script(_qemuImgPath); + s.add("check"); + s.add(file.getFileName()); + + for (QemuObject o : qemuObjects) { + s.add(o.toCommandFlag()); + } + + if (imageOptions != null) { + s.add(imageOptions.toCommandFlag()); + } + + s.add("--output=json"); + + if (repair) { + s.add("-r"); + s.add("leaks"); + } + + OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); + final String result = s.execute(parser); + if (result != null) { + throw new QemuImgException(result); + } + + return (parser.getLines() != null) ? parser.getLines().trim() : null; + } } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java index b2c58fd9b96a..88d4daa2dabc 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java @@ -87,6 +87,9 @@ public void testExternalSnapshot() { StoragePool storage = Mockito.mock(StoragePool.class); LibvirtStoragePool nfsPool = new LibvirtStoragePool(uuid, name, StoragePoolType.NetworkFilesystem, adapter, storage); + if (nfsPool.getType() != StoragePoolType.NetworkFilesystem) { + System.out.println("tested"); + } assertFalse(nfsPool.isExternalSnapshot()); LibvirtStoragePool rbdPool = new LibvirtStoragePool(uuid, name, StoragePoolType.RBD, adapter, storage); diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 430222a78aca..440f1cb85754 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -552,6 +552,7 @@ import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd; import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ChangeOfferingForVolumeCmd; +import org.apache.cloudstack.api.command.user.volume.CheckVolumeAndRepairCmd; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; import org.apache.cloudstack.api.command.user.volume.DeleteVolumeCmd; import org.apache.cloudstack.api.command.user.volume.DestroyVolumeCmd; @@ -3704,6 +3705,7 @@ public List> getCommands() { cmdList.add(ListVMGroupsCmd.class); cmdList.add(UpdateVMGroupCmd.class); cmdList.add(AttachVolumeCmd.class); + cmdList.add(CheckVolumeAndRepairCmd.class); cmdList.add(CreateVolumeCmd.class); cmdList.add(DeleteVolumeCmd.class); cmdList.add(UpdateVolumeCmd.class); diff --git a/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java b/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java new file mode 100644 index 000000000000..16a1531b1d61 --- /dev/null +++ b/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java @@ -0,0 +1,58 @@ +// 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.storage; + +public class CheckAndRepairVolumePayload { + + public final boolean repair; + public String result; + private String volumeCheckExecutionResult; + private String volumeRepairedExecutionResult; + + public CheckAndRepairVolumePayload(boolean repair) { + this.repair = repair; + } + + public boolean isRepair() { + return repair; + } + + public String getResult() { + return result; + } + + public void setResult(String result) { + this.result = result; + } + + public String getVolumeCheckExecutionResult() { + return volumeCheckExecutionResult; + } + + public String getVolumeRepairedExecutionResult() { + return volumeRepairedExecutionResult; + } + + public void setVolumeCheckExecutionResult(String volumeCheckExecutionResult) { + this.volumeCheckExecutionResult = volumeCheckExecutionResult; + } + + public void setVolumeRepairedExecutionResult(String volumeRepairedExecutionResult) { + this.volumeRepairedExecutionResult = volumeRepairedExecutionResult; + } +} diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 2a0821c5c0a7..017310627578 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -42,6 +42,7 @@ import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd; import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ChangeOfferingForVolumeCmd; +import org.apache.cloudstack.api.command.user.volume.CheckVolumeAndRepairCmd; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; import org.apache.cloudstack.api.command.user.volume.DetachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ExtractVolumeCmd; @@ -217,6 +218,7 @@ import com.cloud.vm.VmDetailConstants; import com.cloud.vm.VmWork; import com.cloud.vm.VmWorkAttachVolume; +import com.cloud.vm.VmWorkCheckAndRepairVolume; import com.cloud.vm.VmWorkConstants; import com.cloud.vm.VmWorkDetachVolume; import com.cloud.vm.VmWorkExtractVolume; @@ -1817,7 +1819,141 @@ public void publishVolumeCreationUsageEvent(Volume volume) { s_logger.debug(String.format("Volume [%s] has been successfully recovered, thus a new usage event %s has been published.", volume.getUuid(), EventTypes.EVENT_VOLUME_CREATE)); } + @Override + @ActionEvent(eventType = EventTypes.EVENT_VOLUME_CHECK, eventDescription = "checking volume and repair if needed", async = true) + public Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) throws ResourceAllocationException { + Account caller = CallContext.current().getCallingAccount(); + + // Verify input parameters + long volumeId = cmd.getId(); + boolean repair = cmd.getRepair(); + final VolumeVO volume = _volsDao.findById(volumeId); + + _accountMgr.checkAccess(caller, null, true, volume); + Long vmId = volume.getInstanceId(); + UserVmVO vm = null; + if (vmId != null) { + 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)); + } + if (vm.getState() != State.Stopped) { + throw new InvalidParameterValueException(String.format("VM to which the volume %d is attached should be in stopped state", volumeId)); + } + } + + if (volume.getState() != Volume.State.Ready) { + throw new InvalidParameterValueException(String.format("VolumeId: %d is not in Ready state", volumeId)); + } + + HypervisorType hypervisorType = _volsDao.getHypervisorType(volume.getId()); + if (!HypervisorType.KVM.equals(hypervisorType)) { + throw new InvalidParameterValueException(String.format("Check and Repair volumes is supported only for KVM hypervisor")); + } + + if (vm != null) { + _accountMgr.checkAccess(caller, null, true, vm); + // 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()); + try { + Pair result = orchestrateCheckVolumeAndRepair(volumeId, repair); + return result; + } finally { + _workJobDao.expunge(placeHolder.getId()); + } + } else { + Outcome outcome = checkVolumeAndRepairThroughJobQueue(vm.getId(), volumeId, repair); + try { + outcome.get(); + } catch (InterruptedException e) { + throw new RuntimeException("Operation is interrupted", e); + } catch (java.util.concurrent.ExecutionException e) { + throw new RuntimeException("Execution exception--", e); + } + + Object jobResult = _jobMgr.unmarshallResultObject(outcome.getJob()); + if (jobResult != null) { + if (jobResult instanceof ConcurrentOperationException) { + throw (ConcurrentOperationException)jobResult; + } else if (jobResult instanceof ResourceAllocationException) { + throw (ResourceAllocationException)jobResult; + } else if (jobResult instanceof Throwable) { + throw new RuntimeException("Unexpected exception", (Throwable)jobResult); + } + } + + // retrieve the entity url from job result + if (jobResult != null && jobResult instanceof Pair) { + return (Pair) jobResult; + } + + return null; + } + } else { + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); + VolumeInfo volumeInfo = volFactory.getVolume(volumeId); + volumeInfo.addPayload(payload); + + Pair result = volService.checkAndRepairVolume(volumeInfo); + return result; + } + } + + private Pair orchestrateCheckVolumeAndRepair(Long volumeId, boolean repair) { + + VolumeInfo volume = volFactory.getVolume(volumeId); + + if (volume == null) { + throw new InvalidParameterValueException("Checking volume and repairing failed due to volume:" + volumeId + " doesn't exist"); + } + + if (volume.getState() != Volume.State.Ready) { + throw new InvalidParameterValueException("VolumeId: " + volumeId + " is not in " + Volume.State.Ready + " state but " + volume.getState() + ". Cannot check and repair the volume."); + } + + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); + volume.addPayload(payload); + + return volService.checkAndRepairVolume(volume); + } + + public Outcome checkVolumeAndRepairThroughJobQueue(final Long vmId, final Long volumeId, boolean repair) { + + final CallContext context = CallContext.current(); + final User callingUser = context.getCallingUser(); + final Account callingAccount = context.getCallingAccount(); + + final VMInstanceVO vm = _vmInstanceDao.findById(vmId); + + VmWorkJobVO workJob = new VmWorkJobVO(context.getContextId()); + + workJob.setDispatcher(VmWorkConstants.VM_WORK_JOB_DISPATCHER); + workJob.setCmd(VmWorkCheckAndRepairVolume.class.getName()); + + workJob.setAccountId(callingAccount.getId()); + workJob.setUserId(callingUser.getId()); + workJob.setStep(VmWorkJobVO.Step.Starting); + workJob.setVmType(VirtualMachine.Type.Instance); + workJob.setVmInstanceId(vm.getId()); + workJob.setRelated(AsyncJobExecutionContext.getOriginJobId()); + + // save work context info (there are some duplications) + VmWorkCheckAndRepairVolume workInfo = new VmWorkCheckAndRepairVolume(callingUser.getId(), callingAccount.getId(), vm.getId(), + VolumeApiServiceImpl.VM_WORK_JOB_HANDLER, volumeId, repair); + workJob.setCmdInfo(VmWorkSerializer.serialize(workInfo)); + + _jobMgr.submitAsyncJob(workJob, VmWorkConstants.VM_WORK_QUEUE, vm.getId()); + + AsyncJobExecutionContext.getCurrentExecutionContext().joinJob(workJob.getId()); + + return new VmJobCheckAndRepairVolumeOutcome(workJob); + } @Override @ActionEvent(eventType = EventTypes.EVENT_VOLUME_CHANGE_DISK_OFFERING, eventDescription = "Changing disk offering of a volume") @@ -4596,6 +4732,24 @@ protected Snapshot retrieve() { } } + public class VmJobCheckAndRepairVolumeOutcome extends OutcomeImpl { + + public VmJobCheckAndRepairVolumeOutcome(final AsyncJob job) { + super(Pair.class, job, VmJobCheckInterval.value(), new Predicate() { + @Override + public boolean checkCondition() { + AsyncJobVO jobVo = _entityMgr.findById(AsyncJobVO.class, job.getId()); + assert (jobVo != null); + if (jobVo == null || jobVo.getStatus() != JobInfo.Status.IN_PROGRESS) { + return true; + } + + return false; + } + }, AsyncJob.Topics.JOB_STATE); + } + } + public Outcome attachVolumeToVmThroughJobQueue(final Long vmId, final Long volumeId, final Long deviceId) { final CallContext context = CallContext.current(); @@ -4833,6 +4987,13 @@ private Pair orchestrateTakeVolumeSnapshot(VmWorkTakeVol return new Pair(JobInfo.Status.SUCCEEDED, _jobMgr.marshallResultObject(work.getSnapshotId())); } + @ReflectionUse + private Pair orchestrateCheckVolumeAndRepair(VmWorkCheckAndRepairVolume work) throws Exception { + Account account = _accountDao.findById(work.getAccountId()); + Pair result = orchestrateCheckVolumeAndRepair(work.getVolumeId(), work.needRepair()); + return new Pair(JobInfo.Status.SUCCEEDED, _jobMgr.marshallResultObject(result)); + } + @Override public Pair handleVmWorkJob(VmWork work) throws Exception { return _jobHandlerProxy.handleVmWorkJob(work); diff --git a/utils/src/main/java/com/cloud/utils/StringUtils.java b/utils/src/main/java/com/cloud/utils/StringUtils.java index 9e197a8a94b6..ac9e6b093292 100644 --- a/utils/src/main/java/com/cloud/utils/StringUtils.java +++ b/utils/src/main/java/com/cloud/utils/StringUtils.java @@ -19,6 +19,10 @@ package com.cloud.utils; +import com.cloud.utils.exception.CloudRuntimeException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; + import java.nio.charset.Charset; import java.util.ArrayList; import java.util.HashMap; @@ -282,4 +286,22 @@ public static Pair getKeyValuePairWithSeparator(String keyValueP final String value = keyValuePair.substring(index + 1); return new Pair<>(key.trim(), value.trim()); } + + public static Map parseJsonToMap(String jsonString) { + ObjectMapper objectMapper = new ObjectMapper(); + Map mapResult = new HashMap<>(); + + if (org.apache.commons.lang3.StringUtils.isNotEmpty(jsonString)) { + try { + JsonNode jsonNode = objectMapper.readTree(jsonString); + jsonNode.fields().forEachRemaining(entry -> { + mapResult.put(entry.getKey(), entry.getValue().asText()); + }); + } catch (Exception e) { + throw new CloudRuntimeException("Error while parsing json to convert it to map " + e.getMessage()); + } + } + + return mapResult; + } } From af4c42295888861650cfac43c56ab2b07f535f2f Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 20 Feb 2024 15:35:21 +0530 Subject: [PATCH 02/23] some fixes --- .../admin/volume/RecoverVolumeCmdByAdmin.java | 2 +- .../cloud/vm/VmWorkCheckAndRepairVolume.java | 1 + .../storage/volume/VolumeServiceImpl.java | 3 ++ .../cloud/storage/VolumeApiServiceImpl.java | 53 +++++++++++-------- 4 files changed, 37 insertions(+), 22 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/volume/RecoverVolumeCmdByAdmin.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/volume/RecoverVolumeCmdByAdmin.java index 92c62fa86629..f51aeec97197 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/volume/RecoverVolumeCmdByAdmin.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/volume/RecoverVolumeCmdByAdmin.java @@ -30,7 +30,7 @@ import com.cloud.storage.Volume; -@APICommand(name = "recoverVolume", description = "Recovers a Destroy volume, curr", responseObject = VolumeResponse.class, responseView = ResponseView.Full, entityType = {Volume.class}, +@APICommand(name = "recoverVolume", description = "Recovers a Destroy volume.", responseObject = VolumeResponse.class, responseView = ResponseView.Full, entityType = {Volume.class}, since = "4.14.0", authorized = {RoleType.Admin}, requestHasSensitiveInfo = false, diff --git a/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java b/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java index f4053dd17419..7e62f08fe618 100644 --- a/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java +++ b/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java @@ -14,6 +14,7 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. + package com.cloud.vm; public class VmWorkCheckAndRepairVolume extends VmWork { 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 8c839a9b4d4d..698a585b987c 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 @@ -87,6 +87,8 @@ import com.cloud.agent.AgentManager; import com.cloud.agent.api.Answer; import com.cloud.agent.api.ModifyTargetsCommand; +import com.cloud.agent.api.storage.CheckVolumeAndRepairAnswer; +import com.cloud.agent.api.storage.CheckVolumeAndRepairCommand; import com.cloud.agent.api.storage.ListVolumeAnswer; import com.cloud.agent.api.storage.ListVolumeCommand; import com.cloud.agent.api.storage.ResizeVolumeCommand; @@ -110,6 +112,7 @@ import com.cloud.org.Grouping.AllocationState; import com.cloud.resource.ResourceState; import com.cloud.server.ManagementService; +import com.cloud.storage.CheckAndRepairVolumePayload; import com.cloud.storage.DataStoreRole; import com.cloud.storage.RegisterVolumePayload; import com.cloud.storage.ScopeType; diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 017310627578..720b7d9a41b8 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1822,38 +1822,19 @@ public void publishVolumeCreationUsageEvent(Volume volume) { @Override @ActionEvent(eventType = EventTypes.EVENT_VOLUME_CHECK, eventDescription = "checking volume and repair if needed", async = true) public Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) throws ResourceAllocationException { - Account caller = CallContext.current().getCallingAccount(); - - // Verify input parameters long volumeId = cmd.getId(); boolean repair = cmd.getRepair(); - final VolumeVO volume = _volsDao.findById(volumeId); - _accountMgr.checkAccess(caller, null, true, volume); + validationsForCheckVolumeOperation(volumeId); + final VolumeVO volume = _volsDao.findById(volumeId); Long vmId = volume.getInstanceId(); UserVmVO vm = null; if (vmId != null) { 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)); - } - if (vm.getState() != State.Stopped) { - throw new InvalidParameterValueException(String.format("VM to which the volume %d is attached should be in stopped state", volumeId)); - } - } - - if (volume.getState() != Volume.State.Ready) { - throw new InvalidParameterValueException(String.format("VolumeId: %d is not in Ready state", volumeId)); - } - - HypervisorType hypervisorType = _volsDao.getHypervisorType(volume.getId()); - if (!HypervisorType.KVM.equals(hypervisorType)) { - throw new InvalidParameterValueException(String.format("Check and Repair volumes is supported only for KVM hypervisor")); } if (vm != null) { - _accountMgr.checkAccess(caller, null, true, vm); // serialize VM operation AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext(); if (jobContext.isJobDispatchedBy(VmWorkConstants.VM_WORK_JOB_DISPATCHER)) { @@ -1905,6 +1886,36 @@ public Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) th } } + private void validationsForCheckVolumeOperation(long volumeId) { + final VolumeVO volume = _volsDao.findById(volumeId); + Account caller = CallContext.current().getCallingAccount(); + _accountMgr.checkAccess(caller, null, true, volume); + + Long vmId = volume.getInstanceId(); + UserVmVO vm = null; + if (vmId != null) { + 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)); + } + + _accountMgr.checkAccess(caller, null, true, vm); + + if (vm.getState() != State.Stopped) { + throw new InvalidParameterValueException(String.format("VM to which the volume %d is attached should be in stopped state", volumeId)); + } + } + + if (volume.getState() != Volume.State.Ready) { + throw new InvalidParameterValueException(String.format("VolumeId: %d is not in Ready state", volumeId)); + } + + HypervisorType hypervisorType = _volsDao.getHypervisorType(volume.getId()); + if (!HypervisorType.KVM.equals(hypervisorType)) { + throw new InvalidParameterValueException(String.format("Check and Repair volumes is supported only for KVM hypervisor")); + } + } + private Pair orchestrateCheckVolumeAndRepair(Long volumeId, boolean repair) { VolumeInfo volume = volFactory.getVolume(volumeId); From a365b4508324b3d34adca71ab3313ca8f32efa08 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 20 Feb 2024 15:37:20 +0530 Subject: [PATCH 03/23] Added unit tests --- .../storage/volume/VolumeServiceImpl.java | 2 +- .../storage/volume/VolumeServiceTest.java | 72 +++++++++ ...irtCheckVolumeAndRepairCommandWrapper.java | 4 +- ...heckVolumeAndRepairCommandWrapperTest.java | 103 ++++++++++++ .../cloudstack/utils/qemu/QemuImgTest.java | 17 ++ .../cloud/storage/VolumeApiServiceImpl.java | 2 +- .../storage/VolumeApiServiceImplTest.java | 146 ++++++++++++++++++ 7 files changed, 342 insertions(+), 4 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java 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 698a585b987c..e89c0cfb117c 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 @@ -201,7 +201,7 @@ public class VolumeServiceImpl implements VolumeService { @Inject private VolumeOrchestrationService _volumeMgr; @Inject - private StorageManager _storageMgr; + protected StorageManager _storageMgr; @Inject private AnnotationDao annotationDao; @Inject diff --git a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java index ee4b77c269cf..8b2b22f0c561 100644 --- a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java +++ b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java @@ -19,6 +19,13 @@ package org.apache.cloudstack.storage.volume; +import com.cloud.agent.api.storage.CheckVolumeAndRepairAnswer; +import com.cloud.agent.api.storage.CheckVolumeAndRepairCommand; +import com.cloud.agent.api.to.StorageFilerTO; +import com.cloud.exception.StorageUnavailableException; +import com.cloud.storage.CheckAndRepairVolumePayload; +import com.cloud.storage.StorageManager; +import com.cloud.storage.StoragePool; import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.VolumeDao; import com.cloud.storage.snapshot.SnapshotManager; @@ -26,6 +33,8 @@ import java.util.Arrays; import java.util.List; import java.util.concurrent.ExecutionException; + +import com.cloud.utils.Pair; import junit.framework.TestCase; import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine; import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory; @@ -65,6 +74,9 @@ public class VolumeServiceTest extends TestCase{ @Mock SnapshotManager snapshotManagerMock; + @Mock + StorageManager storageManagerMock; + @Mock VolumeVO volumeVoMock; @@ -74,6 +86,7 @@ public void setup(){ volumeServiceImplSpy.volFactory = volumeDataFactoryMock; volumeServiceImplSpy.volDao = volumeDaoMock; volumeServiceImplSpy.snapshotMgr = snapshotManagerMock; + volumeServiceImplSpy._storageMgr = storageManagerMock; } @Test(expected = InterruptedException.class) @@ -210,4 +223,63 @@ public void validateDestroySourceVolumeAfterMigrationThrowAnyOtherException() th volumeServiceImplSpy.destroySourceVolumeAfterMigration(ObjectInDataStoreStateMachine.Event.DestroyRequested, null, volumeObject, volumeObject, true); } + + @Test + public void testCheckAndRepairVolume() throws StorageUnavailableException { + VolumeInfo volume = Mockito.mock(VolumeInfo.class); + Mockito.when(volume.getPoolId()).thenReturn(1L); + StoragePool pool = Mockito.mock(StoragePool.class); + Mockito.when(storageManagerMock.getStoragePool(1L)).thenReturn(pool); + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(false); + Mockito.when(volume.getpayload()).thenReturn(payload); + Mockito.when(volume.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); + Mockito.when(volume.getPassphrase()).thenReturn(new byte[] {3, 1, 2, 3}); + Mockito.when(volume.getEncryptFormat()).thenReturn("LUKS"); + + String checkResult = "{\n" + + " \"image-end-offset\": 6442582016,\n" + + " \"total-clusters\": 163840,\n" + + " \"check-errors\": 0,\n" + + " \"leaks\": 124,\n" + + " \"allocated-clusters\": 98154,\n" + + " \"filename\": \"/var/lib/libvirt/images/26be20c7-b9d0-43f6-a76e-16c70737a0e0\",\n" + + " \"format\": \"qcow2\",\n" + + " \"fragmented-clusters\": 96135\n" + + "}"; + + CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.isRepair(), + volume.getPassphrase(), volume.getEncryptFormat()); + + CheckVolumeAndRepairAnswer answer = new CheckVolumeAndRepairAnswer(command, true, checkResult); + answer.setVolumeCheckExecutionResult(checkResult); + Mockito.when(storageManagerMock.sendToPool(pool, null, command)).thenReturn(answer); + + Pair result = volumeServiceImplSpy.checkAndRepairVolume(volume); + + Assert.assertEquals(result.first(), checkResult); + Assert.assertEquals(result.second(), null); + } + + @Test + public void testCheckAndRepairVolumeWhenFailure() throws StorageUnavailableException { + VolumeInfo volume = Mockito.mock(VolumeInfo.class); + Mockito.when(volume.getPoolId()).thenReturn(1L); + StoragePool pool = Mockito.mock(StoragePool.class); + Mockito.when(storageManagerMock.getStoragePool(1L)).thenReturn(pool); + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(false); + Mockito.when(volume.getpayload()).thenReturn(payload); + Mockito.when(volume.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); + Mockito.when(volume.getPassphrase()).thenReturn(new byte[] {3, 1, 2, 3}); + Mockito.when(volume.getEncryptFormat()).thenReturn("LUKS"); + + CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.isRepair(), + volume.getPassphrase(), volume.getEncryptFormat()); + + CheckVolumeAndRepairAnswer answer = new CheckVolumeAndRepairAnswer(command, false, "Unable to execute qemu command"); + Mockito.when(storageManagerMock.sendToPool(pool, null, command)).thenReturn(answer); + + Pair result = volumeServiceImplSpy.checkAndRepairVolume(volume); + + Assert.assertEquals(null, result); + } } 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 553de2953c13..42fd020d9bc1 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 @@ -47,7 +47,7 @@ import java.util.List; @ResourceWrapper(handles = CheckVolumeAndRepairCommand.class) -public final class LibvirtCheckVolumeAndRepairCommandWrapper extends CommandWrapper { +public class LibvirtCheckVolumeAndRepairCommandWrapper extends CommandWrapper { private static final Logger s_logger = Logger.getLogger(LibvirtCheckVolumeAndRepairCommandWrapper.class); @@ -87,7 +87,7 @@ public Answer execute(CheckVolumeAndRepairCommand command, LibvirtComputingResou } } - private String checkVolumeAndRepair(final KVMPhysicalDisk vol, final boolean repair, final QemuObject.EncryptFormat encryptFormat, byte[] passphrase, final LibvirtComputingResource libvirtComputingResource) throws CloudRuntimeException { + protected String checkVolumeAndRepair(final KVMPhysicalDisk vol, final boolean repair, final QemuObject.EncryptFormat encryptFormat, byte[] passphrase, final LibvirtComputingResource libvirtComputingResource) throws CloudRuntimeException { List passphraseObjects = new ArrayList<>(); QemuImageOptions imgOptions = null; if (ArrayUtils.isEmpty(passphrase)) { diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java new file mode 100644 index 000000000000..ae0f75c76ca4 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java @@ -0,0 +1,103 @@ +/* + * Licensed 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.kvm.resource.wrapper; + +import com.cloud.agent.api.storage.CheckVolumeAndRepairAnswer; +import com.cloud.agent.api.storage.CheckVolumeAndRepairCommand; +import com.cloud.agent.api.to.StorageFilerTO; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; +import com.cloud.hypervisor.kvm.storage.KVMStoragePool; +import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; +import com.cloud.storage.Storage; + +import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo; +import org.apache.cloudstack.utils.qemu.QemuImg; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +@RunWith(PowerMockRunner.class) +public class LibvirtCheckVolumeAndRepairCommandWrapperTest { + + @Spy + LibvirtCheckVolumeAndRepairCommandWrapper libvirtCheckVolumeAndRepairCommandWrapperSpy = Mockito.spy(LibvirtCheckVolumeAndRepairCommandWrapper.class); + + @Mock + LibvirtComputingResource libvirtComputingResourceMock; + + @Mock + CheckVolumeAndRepairCommand checkVolumeAndRepairCommand; + + @Mock + QemuImg qemuImgMock; + + @Before + public void init() { + Mockito.when(libvirtComputingResourceMock.getCmdsTimeout()).thenReturn(60); + } + + @Test + @PrepareForTest(LibvirtCheckVolumeAndRepairCommandWrapper.class) + public void testCheckAndRepairVolume() throws Exception { + + CheckVolumeAndRepairCommand cmd = Mockito.mock(CheckVolumeAndRepairCommand.class); + Mockito.when(cmd.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); + Mockito.when(cmd.needRepair()).thenReturn(false); + StorageFilerTO spool = Mockito.mock(StorageFilerTO.class); + Mockito.when(cmd.getPool()).thenReturn(spool); + + KVMStoragePoolManager storagePoolMgr = Mockito.mock(KVMStoragePoolManager.class); + Mockito.when(libvirtComputingResourceMock.getStoragePoolMgr()).thenReturn(storagePoolMgr); + KVMStoragePool pool = Mockito.mock(KVMStoragePool.class); + Mockito.when(spool.getType()).thenReturn(Storage.StoragePoolType.PowerFlex); + Mockito.when(spool.getUuid()).thenReturn("b6be258b-42b8-49a4-ad51-3634ef8ff76a"); + Mockito.when(storagePoolMgr.getStoragePool(Storage.StoragePoolType.PowerFlex, "b6be258b-42b8-49a4-ad51-3634ef8ff76a")).thenReturn(pool); + Mockito.when(pool.connectPhysicalDisk("cbac516a-0f1f-4559-921c-1a7c6c408ccf", null)).thenReturn(true); + + KVMPhysicalDisk vol = Mockito.mock(KVMPhysicalDisk.class); + Mockito.when(pool.getPhysicalDisk("cbac516a-0f1f-4559-921c-1a7c6c408ccf")).thenReturn(vol); + + VolumeInfo volume = Mockito.mock(VolumeInfo.class); + Mockito.when(volume.getPoolId()).thenReturn(1L); + Mockito.when(volume.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); + + String checkResult = "{\n" + + " \"image-end-offset\": 6442582016,\n" + + " \"total-clusters\": 163840,\n" + + " \"check-errors\": 0,\n" + + " \"leaks\": 124,\n" + + " \"allocated-clusters\": 98154,\n" + + " \"filename\": \"/var/lib/libvirt/images/26be20c7-b9d0-43f6-a76e-16c70737a0e0\",\n" + + " \"format\": \"qcow2\",\n" + + " \"fragmented-clusters\": 96135\n" + + "}"; + + PowerMockito.whenNew(QemuImg.class).withArguments(Mockito.anyInt()).thenReturn(qemuImgMock); + Mockito.when(qemuImgMock.checkAndRepair(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.anyBoolean())).thenReturn(checkResult); // Replace with the desired result + + CheckVolumeAndRepairAnswer result = (CheckVolumeAndRepairAnswer) libvirtCheckVolumeAndRepairCommandWrapperSpy.execute(cmd, libvirtComputingResourceMock); + + Assert.assertEquals(checkResult, result.getVolumeCheckExecutionResult()); + } + +} diff --git a/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java b/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java index 8bb762cca852..aa74df57b24c 100644 --- a/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java +++ b/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java @@ -368,4 +368,21 @@ public void testHelpSupportsImageFormat() throws QemuImgException, LibvirtExcept Assert.assertTrue("should support qcow2", QemuImg.helpSupportsImageFormat(partialHelp, PhysicalDiskFormat.QCOW2)); Assert.assertFalse("should not support http", QemuImg.helpSupportsImageFormat(partialHelp, PhysicalDiskFormat.SHEEPDOG)); } + + @Test + public void testCheckAndRepair() throws LibvirtException { + String filename = "/tmp/" + UUID.randomUUID() + ".qcow2"; + + QemuImgFile file = new QemuImgFile(filename); + + try { + QemuImg qemu = new QemuImg(0); + qemu.checkAndRepair(file, null, null, false); + } catch (QemuImgException e) { + fail(e.getMessage()); + } + + File f = new File(filename); + f.delete(); + } } diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 720b7d9a41b8..9eec18cd258b 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1886,7 +1886,7 @@ public Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) th } } - private void validationsForCheckVolumeOperation(long volumeId) { + protected void validationsForCheckVolumeOperation(long volumeId) { final VolumeVO volume = _volsDao.findById(volumeId); Account caller = CallContext.current().getCallingAccount(); _accountMgr.checkAccess(caller, null, true, volume); diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index a41cbb82704e..7631ade441f4 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -25,6 +25,7 @@ import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -39,6 +40,7 @@ import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.SecurityChecker.AccessType; +import org.apache.cloudstack.api.command.user.volume.CheckVolumeAndRepairCmd; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; import org.apache.cloudstack.api.command.user.volume.DetachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.MigrateVolumeCmd; @@ -1660,4 +1662,148 @@ public void testStoragePoolCompatibilityAndAllowEncryptedVolumeMigrationForPower // test passed } } + + @Test + public void testValidationsForCheckVolumeAPI() { + VolumeVO volume = mock(VolumeVO.class); + when(volumeDaoMock.findById(1L)).thenReturn(volume); + + AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); + UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); + CallContext.register(user, account); + + lenient().doNothing().when(accountManagerMock).checkAccess(any(Account.class), any(AccessType.class), any(Boolean.class), any(ControlledEntity.class)); + + when(volume.getInstanceId()).thenReturn(1L); + UserVmVO vm = mock(UserVmVO.class); + when(userVmDaoMock.findById(1L)).thenReturn(vm); + when(vm.getState()).thenReturn(State.Stopped); + when(volume.getState()).thenReturn(Volume.State.Ready); + when(volume.getId()).thenReturn(1L); + when(volumeDaoMock.getHypervisorType(1L)).thenReturn(HypervisorType.KVM); + + volumeApiServiceImpl.validationsForCheckVolumeOperation(1L); + } + + @Test(expected = InvalidParameterValueException.class) + public void testValidationsForCheckVolumeAPIWithRunningVM() { + VolumeVO volume = mock(VolumeVO.class); + when(volumeDaoMock.findById(1L)).thenReturn(volume); + + AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); + UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); + CallContext.register(user, account); + + lenient().doNothing().when(accountManagerMock).checkAccess(any(Account.class), any(AccessType.class), any(Boolean.class), any(ControlledEntity.class)); + + when(volume.getInstanceId()).thenReturn(1L); + UserVmVO vm = mock(UserVmVO.class); + when(userVmDaoMock.findById(1L)).thenReturn(vm); + when(vm.getState()).thenReturn(State.Running); + + volumeApiServiceImpl.validationsForCheckVolumeOperation(1L); + } + + @Test(expected = InvalidParameterValueException.class) + public void testValidationsForCheckVolumeAPIWithNonexistedVM() { + VolumeVO volume = mock(VolumeVO.class); + when(volumeDaoMock.findById(1L)).thenReturn(volume); + + AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); + UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); + CallContext.register(user, account); + + lenient().doNothing().when(accountManagerMock).checkAccess(any(Account.class), any(AccessType.class), any(Boolean.class), any(ControlledEntity.class)); + + when(volume.getInstanceId()).thenReturn(1L); + when(userVmDaoMock.findById(1L)).thenReturn(null); + + volumeApiServiceImpl.validationsForCheckVolumeOperation(1L); + } + + @Test(expected = InvalidParameterValueException.class) + public void testValidationsForCheckVolumeAPIWithAllocatedVolume() { + VolumeVO volume = mock(VolumeVO.class); + when(volumeDaoMock.findById(1L)).thenReturn(volume); + + AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); + UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); + CallContext.register(user, account); + + lenient().doNothing().when(accountManagerMock).checkAccess(any(Account.class), any(AccessType.class), any(Boolean.class), any(ControlledEntity.class)); + + when(volume.getInstanceId()).thenReturn(1L); + UserVmVO vm = mock(UserVmVO.class); + when(userVmDaoMock.findById(1L)).thenReturn(vm); + when(vm.getState()).thenReturn(State.Stopped); + when(volume.getState()).thenReturn(Volume.State.Allocated); + + volumeApiServiceImpl.validationsForCheckVolumeOperation(1L); + } + + @Test(expected = InvalidParameterValueException.class) + public void testValidationsForCheckVolumeAPIWithNonKVMhypervisor() { + VolumeVO volume = mock(VolumeVO.class); + when(volumeDaoMock.findById(1L)).thenReturn(volume); + + AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); + UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); + CallContext.register(user, account); + + lenient().doNothing().when(accountManagerMock).checkAccess(any(Account.class), any(AccessType.class), any(Boolean.class), any(ControlledEntity.class)); + + when(volume.getInstanceId()).thenReturn(1L); + UserVmVO vm = mock(UserVmVO.class); + when(userVmDaoMock.findById(1L)).thenReturn(vm); + when(vm.getState()).thenReturn(State.Stopped); + when(volume.getState()).thenReturn(Volume.State.Ready); + when(volume.getId()).thenReturn(1L); + when(volumeDaoMock.getHypervisorType(1L)).thenReturn(HypervisorType.VMware); + + volumeApiServiceImpl.validationsForCheckVolumeOperation(1L); + } + + @Test + public void testCheckAndRepairVolume() throws ResourceAllocationException { + + CheckVolumeAndRepairCmd cmd = mock(CheckVolumeAndRepairCmd.class); + when(cmd.getId()).thenReturn(1L); + when(cmd.getRepair()).thenReturn(false); + + VolumeVO volume = mock(VolumeVO.class); + when(volumeDaoMock.findById(1L)).thenReturn(volume); + + AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); + UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); + CallContext.register(user, account); + + lenient().doNothing().when(accountManagerMock).checkAccess(any(Account.class), any(AccessType.class), any(Boolean.class), any(ControlledEntity.class)); + + when(volume.getInstanceId()).thenReturn(null); + when(volume.getState()).thenReturn(Volume.State.Ready); + when(volume.getId()).thenReturn(1L); + when(volumeDaoMock.getHypervisorType(1L)).thenReturn(HypervisorType.KVM); + + VolumeInfo volumeInfo = mock(VolumeInfo.class); + when(volumeDataFactoryMock.getVolume(1L)).thenReturn(volumeInfo); + + String checkResult = "{\n" + + " \"image-end-offset\": 6442582016,\n" + + " \"total-clusters\": 163840,\n" + + " \"check-errors\": 0,\n" + + " \"leaks\": 124,\n" + + " \"allocated-clusters\": 98154,\n" + + " \"filename\": \"/var/lib/libvirt/images/26be20c7-b9d0-43f6-a76e-16c70737a0e0\",\n" + + " \"format\": \"qcow2\",\n" + + " \"fragmented-clusters\": 96135\n" + + "}"; + + String repairResult = null; + Pair result = new Pair<>(checkResult, repairResult); + when(volumeServiceMock.checkAndRepairVolume(volumeInfo)).thenReturn(result); + + Pair finalresult = volumeApiServiceImpl.checkAndRepairVolume(cmd); + + Assert.assertEquals(result, finalresult); + } } From f52c8191143e6563a28438e92231b570f83da21a Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 20 Feb 2024 15:38:47 +0530 Subject: [PATCH 04/23] addressed review comments --- .../user/volume/CheckVolumeAndRepairCmd.java | 2 +- .../storage/CheckVolumeAndRepairAnswer.java | 33 ++++--------------- .../storage/volume/VolumeServiceImpl.java | 4 +-- ...irtCheckVolumeAndRepairCommandWrapper.java | 2 +- .../apache/cloudstack/utils/qemu/QemuImg.java | 4 ++- .../cloud/storage/VolumeApiServiceImpl.java | 21 +++++------- .../storage/VolumeApiServiceImplTest.java | 10 +++--- .../java/com/cloud/utils/StringUtils.java | 2 +- 8 files changed, 28 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 268ccdcc0a45..0aa221dd43d2 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 526e89334064..25fc9082787a 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 @@ 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 e89c0cfb117c..bf5ebed87de5 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 Pair checkAndRepairVolume(VolumeInfo volume) { 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 42fd020d9bc1..b70ce21f3789 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 Answer execute(CheckVolumeAndRepairCommand command, LibvirtComputingResou 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 9a71a5964208..4a888056d639 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 @@ protected static boolean helpSupportsImageFormat(String text, QemuImg.PhysicalDi public String checkAndRepair(final QemuImgFile file, final QemuImageOptions imageOptions, final List 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 9eec18cd258b..44b00f87b644 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 Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) th 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 result = orchestrateCheckVolumeAndRepair(volumeId, repair); return result; @@ -1849,7 +1844,8 @@ public Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) th _workJobDao.expunge(placeHolder.getId()); } } else { - Outcome outcome = checkVolumeAndRepairThroughJobQueue(vm.getId(), volumeId, repair); + Outcome outcome = checkVolumeAndRepairThroughJobQueue(vmId, volumeId, repair); + try { outcome.get(); } catch (InterruptedException e) { @@ -1886,15 +1882,14 @@ public Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) th } } - 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 7631ade441f4..fb3c842e6c00 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 void testValidationsForCheckVolumeAPI() { 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 void testValidationsForCheckVolumeAPIWithRunningVM() { 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 void testValidationsForCheckVolumeAPIWithNonexistedVM() { 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 void testValidationsForCheckVolumeAPIWithAllocatedVolume() { 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 void testValidationsForCheckVolumeAPIWithNonKVMhypervisor() { 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 ac9e6b093292..fd4f7aba6987 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 static Map parseJsonToMap(String jsonString) { ObjectMapper objectMapper = new ObjectMapper(); Map 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 -> { From 08d3c063a31547df78c9322642dad682e3aafad8 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 10 Jan 2024 16:50:58 +0530 Subject: [PATCH 05/23] add repair volume while granting access --- .../storage/volume/VolumeServiceImpl.java | 7 +++++++ ...irtCheckVolumeAndRepairCommandWrapper.java | 20 ++++++++----------- .../apache/cloudstack/utils/qemu/QemuImg.java | 2 +- 3 files changed, 16 insertions(+), 13 deletions(-) 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 bf5ebed87de5..e7c9a7c7a421 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 @@ -252,6 +252,13 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore return ((PrimaryDataStoreDriver)dataStoreDriver).grantAccess(dataObject, host, dataStore); } + if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(true); + VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); + volumeInfo.addPayload(payload); + checkAndRepairVolume(volumeInfo); + } + return false; } 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 b70ce21f3789..47c00248f6d7 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 @@ -27,7 +27,6 @@ import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; import com.cloud.hypervisor.kvm.storage.KVMStoragePool; import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; -import com.cloud.storage.Storage; import com.cloud.resource.CommandWrapper; import com.cloud.resource.ResourceWrapper; import com.cloud.utils.exception.CloudRuntimeException; @@ -60,20 +59,17 @@ public Answer execute(CheckVolumeAndRepairCommand command, LibvirtComputingResou final KVMStoragePoolManager storagePoolMgr = serverResource.getStoragePoolMgr(); KVMStoragePool pool = storagePoolMgr.getStoragePool(spool.getType(), spool.getUuid()); - if (spool.getType().equals(Storage.StoragePoolType.PowerFlex)) { - pool.connectPhysicalDisk(volumeId, null); - } - final KVMPhysicalDisk vol = pool.getPhysicalDisk(volumeId); QemuObject.EncryptFormat encryptFormat = QemuObject.EncryptFormat.enumValue(command.getEncryptFormat()); + byte[] passphrase = command.getPassphrase(); try { - String checkVolumeResult = checkVolumeAndRepair(vol, false, encryptFormat, command.getPassphrase(), serverResource); - s_logger.info(String.format("Check Volume result is %s", checkVolumeResult)); + String checkVolumeResult = checkVolumeAndRepair(vol, false, encryptFormat, passphrase, serverResource); + s_logger.info(String.format("Check Volume result for the volume %s is %s", vol.getName(), checkVolumeResult)); CheckVolumeAndRepairAnswer answer = new CheckVolumeAndRepairAnswer(command, true, checkVolumeResult); answer.setVolumeCheckExecutionResult(checkVolumeResult); if (repair) { - String repairVolumeResult = checkVolumeAndRepair(vol, true, encryptFormat, command.getPassphrase(), serverResource); + String repairVolumeResult = checkVolumeAndRepair(vol, true, encryptFormat, passphrase, serverResource); String finalResult = (checkVolumeResult != null ? checkVolumeResult.concat(",") : "") + repairVolumeResult; s_logger.info(String.format("Repair Volume result for the volume %s is %s", vol.getName(), repairVolumeResult)); @@ -84,6 +80,10 @@ public Answer execute(CheckVolumeAndRepairCommand command, LibvirtComputingResou return answer; } catch (Exception e) { return new CheckVolumeAndRepairAnswer(command, false, e.toString()); + } finally { + if (passphrase != null) { + Arrays.fill(passphrase, (byte) 0); + } } } @@ -107,10 +107,6 @@ protected String checkVolumeAndRepair(final KVMPhysicalDisk vol, final boolean r throw new CloudRuntimeException("Failed to run qemu-img for check volume", ex); } catch (IOException ex) { throw new CloudRuntimeException("Failed to create keyfile for encrypted volume for check volume operation", ex); - } finally { - if (passphrase != null) { - Arrays.fill(passphrase, (byte) 0); - } } } } 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 4a888056d639..aff23c85a6d6 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 @@ -842,7 +842,7 @@ public String checkAndRepair(final QemuImgFile file, final QemuImageOptions imag if (repair) { s.add("-r"); - s.add("leaks"); + s.add("all"); } OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); From 3b5514d95546dd6a4fcbb415ea36797659a7a8c3 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 30 Jan 2024 13:10:04 +0530 Subject: [PATCH 06/23] Changed repair parameter to accept both leaks/all --- .../user/volume/CheckVolumeAndRepairCmd.java | 21 ++++++++--- .../api/response/VolumeResponse.java | 4 +-- .../storage/CheckVolumeAndRepairCommand.java | 6 ++-- .../cloud/vm/VmWorkCheckAndRepairVolume.java | 6 ++-- .../storage/volume/VolumeServiceImpl.java | 20 +++++++---- .../storage/volume/VolumeServiceTest.java | 8 ++--- ...irtCheckVolumeAndRepairCommandWrapper.java | 35 ++++++++++++++++--- .../apache/cloudstack/utils/qemu/QemuImg.java | 6 ++-- ...heckVolumeAndRepairCommandWrapperTest.java | 5 ++- .../cloudstack/utils/qemu/QemuImgTest.java | 2 +- .../storage/CheckAndRepairVolumePayload.java | 6 ++-- .../cloud/storage/VolumeApiServiceImpl.java | 14 +++++--- .../storage/VolumeApiServiceImplTest.java | 2 +- 13 files changed, 90 insertions(+), 45 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 0aa221dd43d2..5ac5445fe5f1 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 @@ -16,6 +16,7 @@ // under the License. package org.apache.cloudstack.api.command.user.volume; +import com.cloud.exception.InvalidParameterValueException; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiCommandResourceType; @@ -52,19 +53,29 @@ 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 to repair the volume, repairs if it has any leaks") - private Boolean repair; + @Parameter(name = ApiConstants.REPAIR, type = CommandType.STRING, required = false, description = "parameter to repair the volume, leaks or all are the possible values") + private String repair; ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// + public enum RepairValues { + leaks, all + } + public Long getId() { return id; } - public boolean getRepair() { - return repair == null ? false : repair; + public String getRepair() { + if (org.apache.commons.lang3.StringUtils.isNotEmpty(repair)) { + RepairValues repairType = Enum.valueOf(RepairValues.class, repair); + if (repairType == null) { + throw new InvalidParameterValueException("repair parameter only takes either leaks or all as value"); + } + } + return repair; } ///////////////////////////////////////////////////// @@ -104,7 +115,7 @@ public void execute() throws ResourceAllocationException { if (result != null) { VolumeResponse response = _responseGenerator.createVolumeResponse(ResponseView.Full, volume); response.setVolumeCheckResult(StringUtils.parseJsonToMap(result.first())); - if (getRepair()) { + if (getRepair() != null) { response.setVolumeRepairResult(StringUtils.parseJsonToMap(result.second())); } response.setResponseName(getCommandName()); diff --git a/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java index 785284ad46fa..daf3cf3950fe 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java @@ -290,11 +290,11 @@ public class VolumeResponse extends BaseResponseWithTagInformation implements Co private String externalUuid; @SerializedName(ApiConstants.VOLUME_CHECK_RESULT) - @Param(description = "details for the volume check result") + @Param(description = "details for the volume check result, they may vary for different hypervisors") private Map volumeCheckResult; @SerializedName(ApiConstants.VOLUME_REPAIR_RESULT) - @Param(description = "details for the volume repair result") + @Param(description = "details for the volume repair result, they may vary for different hypervisors") private Map volumeRepairResult; public String getPath() { diff --git a/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java b/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java index 040e6ca86084..e6d12aab513c 100644 --- a/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java +++ b/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java @@ -28,12 +28,12 @@ public class CheckVolumeAndRepairCommand extends Command { private String path; private StorageFilerTO pool; - private boolean repair; + private String repair; @LogLevel(LogLevel.Log4jLevel.Off) private byte[] passphrase; private String encryptFormat; - public CheckVolumeAndRepairCommand(String path, StorageFilerTO pool, boolean repair, byte[] passphrase, String encryptFormat) { + public CheckVolumeAndRepairCommand(String path, StorageFilerTO pool, String repair, byte[] passphrase, String encryptFormat) { this.path = path; this.pool = pool; this.repair = repair; @@ -53,7 +53,7 @@ public StorageFilerTO getPool() { return pool; } - public boolean needRepair() { + public String getRepair() { return repair; } diff --git a/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java b/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java index 7e62f08fe618..b6b73df959f3 100644 --- a/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java +++ b/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java @@ -23,10 +23,10 @@ public class VmWorkCheckAndRepairVolume extends VmWork { private Long volumeId; - private boolean repair; + private String repair; public VmWorkCheckAndRepairVolume(long userId, long accountId, long vmId, String handlerName, - Long volumeId, boolean repair) { + Long volumeId, String repair) { super(userId, accountId, vmId, handlerName); this.repair = repair; this.volumeId = volumeId; @@ -36,7 +36,7 @@ public Long getVolumeId() { return volumeId; } - public boolean needRepair() { + public String getRepair() { return repair; } } 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 e7c9a7c7a421..553417bf2a0c 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 @@ -34,6 +34,7 @@ import org.apache.cloudstack.annotation.AnnotationService; import org.apache.cloudstack.annotation.dao.AnnotationDao; +import org.apache.cloudstack.api.command.user.volume.CheckVolumeAndRepairCmd; import org.apache.cloudstack.engine.cloud.entity.api.VolumeEntity; import org.apache.cloudstack.engine.orchestration.service.VolumeOrchestrationService; import org.apache.cloudstack.engine.subsystem.api.storage.ChapInfo; @@ -92,6 +93,7 @@ import com.cloud.agent.api.storage.ListVolumeAnswer; import com.cloud.agent.api.storage.ListVolumeCommand; import com.cloud.agent.api.storage.ResizeVolumeCommand; +import com.cloud.agent.api.to.DataObjectType; import com.cloud.agent.api.to.StorageFilerTO; import com.cloud.agent.api.to.VirtualMachineTO; import com.cloud.alert.AlertManager; @@ -252,11 +254,15 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore return ((PrimaryDataStoreDriver)dataStoreDriver).grantAccess(dataObject, host, dataStore); } - if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { - CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(true); - VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); - volumeInfo.addPayload(payload); - checkAndRepairVolume(volumeInfo); + if (com.cloud.storage.VolumeApiServiceImpl.AllowVolumeCheckAndRepair.value()) { + if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { + s_logger.info(String.format("Trying to check and repair the volume %d", dataObject.getId())); + String repair = CheckVolumeAndRepairCmd.RepairValues.leaks.name(); + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); + VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); + volumeInfo.addPayload(payload); + checkAndRepairVolume(volumeInfo); + } } return false; @@ -2775,7 +2781,7 @@ public Pair checkAndRepairVolume(VolumeInfo volume) { Long poolId = volume.getPoolId(); StoragePool pool = _storageMgr.getStoragePool(poolId); CheckAndRepairVolumePayload payload = (CheckAndRepairVolumePayload) volume.getpayload(); - CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.isRepair(), + CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.getRepair(), volume.getPassphrase(), volume.getEncryptFormat()); try { @@ -2783,7 +2789,7 @@ public Pair checkAndRepairVolume(VolumeInfo volume) { if (answer != null && answer.getResult()) { s_logger.debug("Check volume response result: " + answer.getDetails()); payload.setVolumeCheckExecutionResult(answer.getVolumeCheckExecutionResult()); - if (payload.isRepair()) { + if (StringUtils.isNotEmpty(payload.getRepair())) { payload.setVolumeRepairedExecutionResult(answer.getVolumeRepairExecutionResult()); } return new Pair<>(answer.getVolumeCheckExecutionResult(), answer.getVolumeRepairExecutionResult()); diff --git a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java index 8b2b22f0c561..46efaa5ae9cf 100644 --- a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java +++ b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java @@ -230,7 +230,7 @@ public void testCheckAndRepairVolume() throws StorageUnavailableException { Mockito.when(volume.getPoolId()).thenReturn(1L); StoragePool pool = Mockito.mock(StoragePool.class); Mockito.when(storageManagerMock.getStoragePool(1L)).thenReturn(pool); - CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(false); + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(null); Mockito.when(volume.getpayload()).thenReturn(payload); Mockito.when(volume.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); Mockito.when(volume.getPassphrase()).thenReturn(new byte[] {3, 1, 2, 3}); @@ -247,7 +247,7 @@ public void testCheckAndRepairVolume() throws StorageUnavailableException { " \"fragmented-clusters\": 96135\n" + "}"; - CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.isRepair(), + CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.getRepair(), volume.getPassphrase(), volume.getEncryptFormat()); CheckVolumeAndRepairAnswer answer = new CheckVolumeAndRepairAnswer(command, true, checkResult); @@ -266,13 +266,13 @@ public void testCheckAndRepairVolumeWhenFailure() throws StorageUnavailableExcep Mockito.when(volume.getPoolId()).thenReturn(1L); StoragePool pool = Mockito.mock(StoragePool.class); Mockito.when(storageManagerMock.getStoragePool(1L)).thenReturn(pool); - CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(false); + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(null); Mockito.when(volume.getpayload()).thenReturn(payload); Mockito.when(volume.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); Mockito.when(volume.getPassphrase()).thenReturn(new byte[] {3, 1, 2, 3}); Mockito.when(volume.getEncryptFormat()).thenReturn("LUKS"); - CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.isRepair(), + CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.getRepair(), volume.getPassphrase(), volume.getEncryptFormat()); CheckVolumeAndRepairAnswer answer = new CheckVolumeAndRepairAnswer(command, false, "Unable to execute qemu command"); 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 47c00248f6d7..280e2aa14d24 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 @@ -30,6 +30,8 @@ import com.cloud.resource.CommandWrapper; import com.cloud.resource.ResourceWrapper; import com.cloud.utils.exception.CloudRuntimeException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.JsonNode; import org.apache.cloudstack.utils.cryptsetup.KeyFile; import org.apache.cloudstack.utils.qemu.QemuImageOptions; import org.apache.cloudstack.utils.qemu.QemuImg; @@ -37,6 +39,7 @@ import org.apache.cloudstack.utils.qemu.QemuImgFile; import org.apache.cloudstack.utils.qemu.QemuObject; import org.apache.commons.lang.ArrayUtils; +import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import org.libvirt.LibvirtException; @@ -53,7 +56,7 @@ public class LibvirtCheckVolumeAndRepairCommandWrapper extends CommandWrapper passphraseObjects = new ArrayList<>(); QemuImageOptions imgOptions = null; if (ArrayUtils.isEmpty(passphrase)) { 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 aff23c85a6d6..b8d94ccaef7e 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 @@ -823,7 +823,7 @@ protected static boolean helpSupportsImageFormat(String text, QemuImg.PhysicalDi * @param repair * Boolean option whether to repair any leaks */ - public String checkAndRepair(final QemuImgFile file, final QemuImageOptions imageOptions, final List qemuObjects, final boolean repair) throws QemuImgException { + public String checkAndRepair(final QemuImgFile file, final QemuImageOptions imageOptions, final List qemuObjects, final String repair) throws QemuImgException { final Script s = new Script(_qemuImgPath); s.add("check"); if (imageOptions == null) { @@ -840,9 +840,9 @@ public String checkAndRepair(final QemuImgFile file, final QemuImageOptions imag s.add("--output=json"); - if (repair) { + if (StringUtils.isNotEmpty(repair)) { s.add("-r"); - s.add("all"); + s.add(repair); } OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java index ae0f75c76ca4..dd7b6e0e2f73 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java @@ -62,7 +62,7 @@ public void testCheckAndRepairVolume() throws Exception { CheckVolumeAndRepairCommand cmd = Mockito.mock(CheckVolumeAndRepairCommand.class); Mockito.when(cmd.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); - Mockito.when(cmd.needRepair()).thenReturn(false); + Mockito.when(cmd.getRepair()).thenReturn(null); StorageFilerTO spool = Mockito.mock(StorageFilerTO.class); Mockito.when(cmd.getPool()).thenReturn(spool); @@ -72,7 +72,6 @@ public void testCheckAndRepairVolume() throws Exception { Mockito.when(spool.getType()).thenReturn(Storage.StoragePoolType.PowerFlex); Mockito.when(spool.getUuid()).thenReturn("b6be258b-42b8-49a4-ad51-3634ef8ff76a"); Mockito.when(storagePoolMgr.getStoragePool(Storage.StoragePoolType.PowerFlex, "b6be258b-42b8-49a4-ad51-3634ef8ff76a")).thenReturn(pool); - Mockito.when(pool.connectPhysicalDisk("cbac516a-0f1f-4559-921c-1a7c6c408ccf", null)).thenReturn(true); KVMPhysicalDisk vol = Mockito.mock(KVMPhysicalDisk.class); Mockito.when(pool.getPhysicalDisk("cbac516a-0f1f-4559-921c-1a7c6c408ccf")).thenReturn(vol); @@ -93,7 +92,7 @@ public void testCheckAndRepairVolume() throws Exception { "}"; PowerMockito.whenNew(QemuImg.class).withArguments(Mockito.anyInt()).thenReturn(qemuImgMock); - Mockito.when(qemuImgMock.checkAndRepair(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.anyBoolean())).thenReturn(checkResult); // Replace with the desired result + Mockito.when(qemuImgMock.checkAndRepair(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(checkResult); CheckVolumeAndRepairAnswer result = (CheckVolumeAndRepairAnswer) libvirtCheckVolumeAndRepairCommandWrapperSpy.execute(cmd, libvirtComputingResourceMock); diff --git a/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java b/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java index aa74df57b24c..b0981dde26e7 100644 --- a/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java +++ b/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java @@ -377,7 +377,7 @@ public void testCheckAndRepair() throws LibvirtException { try { QemuImg qemu = new QemuImg(0); - qemu.checkAndRepair(file, null, null, false); + qemu.checkAndRepair(file, null, null, null); } catch (QemuImgException e) { fail(e.getMessage()); } diff --git a/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java b/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java index 16a1531b1d61..6d06f2a4c36d 100644 --- a/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java +++ b/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java @@ -19,16 +19,16 @@ public class CheckAndRepairVolumePayload { - public final boolean repair; + public final String repair; public String result; private String volumeCheckExecutionResult; private String volumeRepairedExecutionResult; - public CheckAndRepairVolumePayload(boolean repair) { + public CheckAndRepairVolumePayload(String repair) { this.repair = repair; } - public boolean isRepair() { + public String getRepair() { return repair; } diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 44b00f87b644..7edca6b6c9d1 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -381,6 +381,9 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic public static ConfigKey storageTagRuleExecutionTimeout = new ConfigKey<>("Advanced", Long.class, "storage.tag.rule.execution.timeout", "2000", "The maximum runtime," + " in milliseconds, to execute a storage tag rule; if it is reached, a timeout will happen.", true); + public static final ConfigKey AllowVolumeCheckAndRepair = new ConfigKey("Hidden", Boolean.class, "volume.check.and.repair.before.instance.use", "true", + "To check and repair the volume if it has any leaks before performing any volume operation", true); + private final StateMachine2 _volStateMachine; private static final Set STATES_VOLUME_CANNOT_BE_DESTROYED = new HashSet<>(Arrays.asList(Volume.State.Destroy, Volume.State.Expunging, Volume.State.Expunged, Volume.State.Allocated)); @@ -1823,7 +1826,7 @@ public void publishVolumeCreationUsageEvent(Volume volume) { @ActionEvent(eventType = EventTypes.EVENT_VOLUME_CHECK, eventDescription = "checking volume and repair if needed", async = true) public Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) throws ResourceAllocationException { long volumeId = cmd.getId(); - boolean repair = cmd.getRepair(); + String repair = cmd.getRepair(); final VolumeVO volume = _volsDao.findById(volumeId); validationsForCheckVolumeOperation(volume); @@ -1911,7 +1914,7 @@ protected void validationsForCheckVolumeOperation(VolumeVO volume) { } } - private Pair orchestrateCheckVolumeAndRepair(Long volumeId, boolean repair) { + private Pair orchestrateCheckVolumeAndRepair(Long volumeId, String repair) { VolumeInfo volume = volFactory.getVolume(volumeId); @@ -1929,7 +1932,7 @@ private Pair orchestrateCheckVolumeAndRepair(Long volumeId, bool return volService.checkAndRepairVolume(volume); } - public Outcome checkVolumeAndRepairThroughJobQueue(final Long vmId, final Long volumeId, boolean repair) { + public Outcome checkVolumeAndRepairThroughJobQueue(final Long vmId, final Long volumeId, String repair) { final CallContext context = CallContext.current(); final User callingUser = context.getCallingUser(); @@ -4996,7 +4999,7 @@ private Pair orchestrateTakeVolumeSnapshot(VmWorkTakeVol @ReflectionUse private Pair orchestrateCheckVolumeAndRepair(VmWorkCheckAndRepairVolume work) throws Exception { Account account = _accountDao.findById(work.getAccountId()); - Pair result = orchestrateCheckVolumeAndRepair(work.getVolumeId(), work.needRepair()); + Pair result = orchestrateCheckVolumeAndRepair(work.getVolumeId(), work.getRepair()); return new Pair(JobInfo.Status.SUCCEEDED, _jobMgr.marshallResultObject(result)); } @@ -5036,7 +5039,8 @@ public ConfigKey[] getConfigKeys() { AllowUserExpungeRecoverVolume, MatchStoragePoolTagsWithDiskOffering, UseHttpsToUpload, - WaitDetachDevice + WaitDetachDevice, + AllowVolumeCheckAndRepair }; } } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index fb3c842e6c00..bab5bdd20a9d 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -1768,7 +1768,7 @@ public void testCheckAndRepairVolume() throws ResourceAllocationException { CheckVolumeAndRepairCmd cmd = mock(CheckVolumeAndRepairCmd.class); when(cmd.getId()).thenReturn(1L); - when(cmd.getRepair()).thenReturn(false); + when(cmd.getRepair()).thenReturn(null); VolumeVO volume = mock(VolumeVO.class); when(volumeDaoMock.findById(1L)).thenReturn(volume); From ef1bbdd41eb2869163da476df54cce6deeb7cc94 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 30 Jan 2024 13:12:54 +0530 Subject: [PATCH 07/23] Introduced new global setting volume.check.and.repair.before.use to do volume check and repair before VM start or volume attach operations --- .../orchestration/VolumeOrchestrator.java | 9 +++++++++ .../storage/volume/VolumeServiceImpl.java | 20 ++++++++++--------- .../cloud/storage/VolumeApiServiceImpl.java | 4 ++-- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index e95085d53f6e..bf82c468dbe0 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1915,6 +1915,15 @@ public void prepare(VirtualMachineProfile vm, DeployDestination dest) throws Sto } } } + } else { + // For unmanaged storage this is not a mandatory step but this is kept here so that volume can be checked and repaired if needed based on the + // global setting volume.check.and.repair.before.use + Host host = _hostDao.findById(vm.getVirtualMachine().getHostId()); + try { + volService.grantAccess(volFactory.getVolume(vol.getId()), host, (DataStore)pool); + } catch (Exception e) { + s_logger.debug(String.format("Unable to grant access to volume [%s] on host [%s], due to %s.", volToString, host, e.getMessage())); + } } } else if (task.type == VolumeTaskType.MIGRATE) { pool = (StoragePool)dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary); 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 553417bf2a0c..fad413d36356 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 @@ -251,18 +251,20 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore DataStoreDriver dataStoreDriver = dataStore != null ? dataStore.getDriver() : null; if (dataStoreDriver instanceof PrimaryDataStoreDriver) { - return ((PrimaryDataStoreDriver)dataStoreDriver).grantAccess(dataObject, host, dataStore); - } + boolean result = ((PrimaryDataStoreDriver)dataStoreDriver).grantAccess(dataObject, host, dataStore); - if (com.cloud.storage.VolumeApiServiceImpl.AllowVolumeCheckAndRepair.value()) { if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { - s_logger.info(String.format("Trying to check and repair the volume %d", dataObject.getId())); - String repair = CheckVolumeAndRepairCmd.RepairValues.leaks.name(); - CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); - VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); - volumeInfo.addPayload(payload); - checkAndRepairVolume(volumeInfo); + if (com.cloud.storage.VolumeApiServiceImpl.AllowVolumeCheckAndRepair.value()) { + s_logger.info(String.format("Trying to check and repair the volume %d", dataObject.getId())); + String repair = CheckVolumeAndRepairCmd.RepairValues.leaks.name(); + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); + VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); + volumeInfo.addPayload(payload); + checkAndRepairVolume(volumeInfo); + } } + + return result; } return false; diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 7edca6b6c9d1..6df17ca4dda6 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -381,8 +381,8 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic public static ConfigKey storageTagRuleExecutionTimeout = new ConfigKey<>("Advanced", Long.class, "storage.tag.rule.execution.timeout", "2000", "The maximum runtime," + " in milliseconds, to execute a storage tag rule; if it is reached, a timeout will happen.", true); - public static final ConfigKey AllowVolumeCheckAndRepair = new ConfigKey("Hidden", Boolean.class, "volume.check.and.repair.before.instance.use", "true", - "To check and repair the volume if it has any leaks before performing any volume operation", true); + public static final ConfigKey AllowVolumeCheckAndRepair = new ConfigKey("Advanced", Boolean.class, "volume.check.and.repair.before.use", "false", + "To check and repair the volume if it has any leaks before performing volume attach or VM start operations", true); private final StateMachine2 _volStateMachine; From 369e5673276e90eb8656d516718aafaf6cdda034 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Fri, 12 Jan 2024 13:33:41 +0530 Subject: [PATCH 08/23] Added volume check and repair changes only during VM start and volume attach operations --- .../subsystem/api/storage/VolumeService.java | 2 ++ .../orchestration/VolumeOrchestrator.java | 6 ++-- .../storage/volume/VolumeServiceImpl.java | 29 ++++++++++--------- .../cloud/storage/VolumeApiServiceImpl.java | 6 ++++ 4 files changed, 25 insertions(+), 18 deletions(-) diff --git a/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java b/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java index 4350e2641250..be5ca340b6fa 100644 --- a/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java +++ b/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java @@ -117,4 +117,6 @@ boolean copyPoliciesBetweenVolumesAndDestroySourceVolumeAfterMigration(ObjectInD void moveVolumeOnSecondaryStorageToAnotherAccount(Volume volume, Account sourceAccount, Account destAccount); Pair checkAndRepairVolume(VolumeInfo volume); + + void checkAndRepairVolumeBasedOnConfig(DataObject dataObject, Host host); } diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index bf82c468dbe0..4e157303b71e 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1916,13 +1916,11 @@ public void prepare(VirtualMachineProfile vm, DeployDestination dest) throws Sto } } } else { - // For unmanaged storage this is not a mandatory step but this is kept here so that volume can be checked and repaired if needed based on the - // global setting volume.check.and.repair.before.use Host host = _hostDao.findById(vm.getVirtualMachine().getHostId()); try { - volService.grantAccess(volFactory.getVolume(vol.getId()), host, (DataStore)pool); + volService.checkAndRepairVolumeBasedOnConfig(volFactory.getVolume(vol.getId()), host); } catch (Exception e) { - s_logger.debug(String.format("Unable to grant access to volume [%s] on host [%s], due to %s.", volToString, host, e.getMessage())); + s_logger.debug(String.format("Unable to check and repair volume [%s] on host [%s], due to %s.", volToString, host, e.getMessage())); } } } else if (task.type == VolumeTaskType.MIGRATE) { 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 fad413d36356..c0628007326e 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 @@ -251,20 +251,7 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore DataStoreDriver dataStoreDriver = dataStore != null ? dataStore.getDriver() : null; if (dataStoreDriver instanceof PrimaryDataStoreDriver) { - boolean result = ((PrimaryDataStoreDriver)dataStoreDriver).grantAccess(dataObject, host, dataStore); - - if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { - if (com.cloud.storage.VolumeApiServiceImpl.AllowVolumeCheckAndRepair.value()) { - s_logger.info(String.format("Trying to check and repair the volume %d", dataObject.getId())); - String repair = CheckVolumeAndRepairCmd.RepairValues.leaks.name(); - CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); - VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); - volumeInfo.addPayload(payload); - checkAndRepairVolume(volumeInfo); - } - } - - return result; + return ((PrimaryDataStoreDriver)dataStoreDriver).grantAccess(dataObject, host, dataStore); } return false; @@ -2778,6 +2765,20 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) { return snapshot; } + @Override + public void checkAndRepairVolumeBasedOnConfig(DataObject dataObject, Host host) { + if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { + if (com.cloud.storage.VolumeApiServiceImpl.AllowVolumeCheckAndRepair.value()) { + s_logger.info(String.format("Trying to check and repair the volume %d", dataObject.getId())); + String repair = CheckVolumeAndRepairCmd.RepairValues.leaks.name(); + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); + VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); + volumeInfo.addPayload(payload); + checkAndRepairVolume(volumeInfo); + } + } + } + @Override public Pair checkAndRepairVolume(VolumeInfo volume) { Long poolId = volume.getPoolId(); diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 6df17ca4dda6..e58dd110afd1 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -4396,6 +4396,12 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L try { // if we don't have a host, the VM we are attaching the disk to has never been started before if (host != null) { + try { + volService.checkAndRepairVolumeBasedOnConfig(volFactory.getVolume(volumeToAttach.getId()), host); + } catch (Exception e) { + s_logger.debug(String.format("Unable to check and repair volume [%s] on host [%s], due to %s.", volumeToAttach.getName(), host, e.getMessage())); + } + try { volService.grantAccess(volFactory.getVolume(volumeToAttach.getId()), host, dataStore); } catch (Exception e) { From 640288286b510267811923a42a6c69a673ca49ad Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 30 Jan 2024 13:17:35 +0530 Subject: [PATCH 09/23] Refactored the names to look similar across the code --- .../com/cloud/storage/VolumeApiService.java | 4 +-- ...rCmd.java => CheckAndRepairVolumeCmd.java} | 8 +++--- ...r.java => CheckAndRepairVolumeAnswer.java} | 8 +++--- ....java => CheckAndRepairVolumeCommand.java} | 4 +-- .../cloud/vm/VmWorkCheckAndRepairVolume.java | 2 +- .../storage/volume/VolumeServiceImpl.java | 17 ++++++------ .../storage/volume/VolumeServiceTest.java | 12 ++++----- ...rtCheckAndRepairVolumeCommandWrapper.java} | 26 +++++++++---------- ...eckAndRepairVolumeCommandWrapperTest.java} | 16 ++++++------ .../cloud/server/ManagementServerImpl.java | 4 +-- .../cloud/storage/VolumeApiServiceImpl.java | 20 +++++++------- .../storage/VolumeApiServiceImplTest.java | 4 +-- 12 files changed, 63 insertions(+), 62 deletions(-) rename api/src/main/java/org/apache/cloudstack/api/command/user/volume/{CheckVolumeAndRepairCmd.java => CheckAndRepairVolumeCmd.java} (92%) rename core/src/main/java/com/cloud/agent/api/storage/{CheckVolumeAndRepairAnswer.java => CheckAndRepairVolumeAnswer.java} (89%) rename core/src/main/java/com/cloud/agent/api/storage/{CheckVolumeAndRepairCommand.java => CheckAndRepairVolumeCommand.java} (94%) rename plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/{LibvirtCheckVolumeAndRepairCommandWrapper.java => LibvirtCheckAndRepairVolumeCommandWrapper.java} (86%) rename plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/{LibvirtCheckVolumeAndRepairCommandWrapperTest.java => LibvirtCheckAndRepairVolumeCommandWrapperTest.java} (84%) diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java index 652d4ab3cc8a..a673df12d0f4 100644 --- a/api/src/main/java/com/cloud/storage/VolumeApiService.java +++ b/api/src/main/java/com/cloud/storage/VolumeApiService.java @@ -26,7 +26,7 @@ import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd; import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ChangeOfferingForVolumeCmd; -import org.apache.cloudstack.api.command.user.volume.CheckVolumeAndRepairCmd; +import org.apache.cloudstack.api.command.user.volume.CheckAndRepairVolumeCmd; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; import org.apache.cloudstack.api.command.user.volume.DetachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ExtractVolumeCmd; @@ -181,5 +181,5 @@ Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account acc boolean stateTransitTo(Volume vol, Volume.Event event) throws NoTransitionException; - Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) throws ResourceAllocationException; + Pair checkAndRepairVolume(CheckAndRepairVolumeCmd cmd) throws ResourceAllocationException; } 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/CheckAndRepairVolumeCmd.java similarity index 92% rename from api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckVolumeAndRepairCmd.java rename to api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java index 5ac5445fe5f1..01835de8ce17 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/CheckAndRepairVolumeCmd.java @@ -36,15 +36,15 @@ import com.cloud.utils.Pair; import com.cloud.utils.StringUtils; -@APICommand(name = "checkVolumeAndRepair", description = "Check the volume and repair if needed, this is currently supported for KVM only", responseObject = VolumeResponse.class, entityType = {Volume.class}, +@APICommand(name = "checkVolume", description = "Check the volume for any errors or leaks and also repairs when repair parameter is passed, this is currently supported for KVM only", responseObject = VolumeResponse.class, entityType = {Volume.class}, since = "4.18.1", authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}, requestHasSensitiveInfo = false, responseHasSensitiveInfo = true) -public class CheckVolumeAndRepairCmd extends BaseCmd { - public static final Logger s_logger = Logger.getLogger(CheckVolumeAndRepairCmd.class.getName()); +public class CheckAndRepairVolumeCmd extends BaseCmd { + public static final Logger s_logger = Logger.getLogger(CheckAndRepairVolumeCmd.class.getName()); - private static final String s_name = "checkvolumeandrepairresponse"; + private static final String s_name = "checkandrepairvolumeresponse"; ///////////////////////////////////////////////////// //////////////// API parameters ///////////////////// diff --git a/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairAnswer.java b/core/src/main/java/com/cloud/agent/api/storage/CheckAndRepairVolumeAnswer.java similarity index 89% rename from core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairAnswer.java rename to core/src/main/java/com/cloud/agent/api/storage/CheckAndRepairVolumeAnswer.java index 25fc9082787a..3dc7752bfefc 100644 --- a/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairAnswer.java +++ b/core/src/main/java/com/cloud/agent/api/storage/CheckAndRepairVolumeAnswer.java @@ -21,21 +21,21 @@ import com.cloud.agent.api.Answer; -public class CheckVolumeAndRepairAnswer extends Answer { +public class CheckAndRepairVolumeAnswer extends Answer { private String volumeCheckExecutionResult; private String volumeRepairExecutionResult; - protected CheckVolumeAndRepairAnswer() { + protected CheckAndRepairVolumeAnswer() { super(); } - public CheckVolumeAndRepairAnswer(CheckVolumeAndRepairCommand cmd, boolean result, String details, String volumeCheckExecutionResult, String volumeRepairedExecutionResult) { + public CheckAndRepairVolumeAnswer(CheckAndRepairVolumeCommand cmd, boolean result, String details, String volumeCheckExecutionResult, String volumeRepairedExecutionResult) { super(cmd, result, details); this.volumeCheckExecutionResult = volumeCheckExecutionResult; this.volumeRepairExecutionResult = volumeRepairedExecutionResult; } - public CheckVolumeAndRepairAnswer(CheckVolumeAndRepairCommand cmd, boolean result, String details) { + public CheckAndRepairVolumeAnswer(CheckAndRepairVolumeCommand cmd, boolean result, String details) { super(cmd, result, details); } diff --git a/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java b/core/src/main/java/com/cloud/agent/api/storage/CheckAndRepairVolumeCommand.java similarity index 94% rename from core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java rename to core/src/main/java/com/cloud/agent/api/storage/CheckAndRepairVolumeCommand.java index e6d12aab513c..2553fdf477c5 100644 --- a/core/src/main/java/com/cloud/agent/api/storage/CheckVolumeAndRepairCommand.java +++ b/core/src/main/java/com/cloud/agent/api/storage/CheckAndRepairVolumeCommand.java @@ -25,7 +25,7 @@ import java.util.Arrays; -public class CheckVolumeAndRepairCommand extends Command { +public class CheckAndRepairVolumeCommand extends Command { private String path; private StorageFilerTO pool; private String repair; @@ -33,7 +33,7 @@ public class CheckVolumeAndRepairCommand extends Command { private byte[] passphrase; private String encryptFormat; - public CheckVolumeAndRepairCommand(String path, StorageFilerTO pool, String repair, byte[] passphrase, String encryptFormat) { + public CheckAndRepairVolumeCommand(String path, StorageFilerTO pool, String repair, byte[] passphrase, String encryptFormat) { this.path = path; this.pool = pool; this.repair = repair; diff --git a/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java b/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java index b6b73df959f3..eaee4d19eb38 100644 --- a/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java +++ b/engine/components-api/src/main/java/com/cloud/vm/VmWorkCheckAndRepairVolume.java @@ -19,7 +19,7 @@ public class VmWorkCheckAndRepairVolume extends VmWork { - private static final long serialVersionUID = 341816293003023823L; + private static final long serialVersionUID = 341816293003023824L; private Long volumeId; 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 c0628007326e..76b6b7de9c05 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 @@ -34,7 +34,7 @@ import org.apache.cloudstack.annotation.AnnotationService; import org.apache.cloudstack.annotation.dao.AnnotationDao; -import org.apache.cloudstack.api.command.user.volume.CheckVolumeAndRepairCmd; +import org.apache.cloudstack.api.command.user.volume.CheckAndRepairVolumeCmd; import org.apache.cloudstack.engine.cloud.entity.api.VolumeEntity; import org.apache.cloudstack.engine.orchestration.service.VolumeOrchestrationService; import org.apache.cloudstack.engine.subsystem.api.storage.ChapInfo; @@ -88,8 +88,8 @@ import com.cloud.agent.AgentManager; import com.cloud.agent.api.Answer; import com.cloud.agent.api.ModifyTargetsCommand; -import com.cloud.agent.api.storage.CheckVolumeAndRepairAnswer; -import com.cloud.agent.api.storage.CheckVolumeAndRepairCommand; +import com.cloud.agent.api.storage.CheckAndRepairVolumeAnswer; +import com.cloud.agent.api.storage.CheckAndRepairVolumeCommand; import com.cloud.agent.api.storage.ListVolumeAnswer; import com.cloud.agent.api.storage.ListVolumeCommand; import com.cloud.agent.api.storage.ResizeVolumeCommand; @@ -2768,9 +2768,9 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) { @Override public void checkAndRepairVolumeBasedOnConfig(DataObject dataObject, Host host) { if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { - if (com.cloud.storage.VolumeApiServiceImpl.AllowVolumeCheckAndRepair.value()) { + if (com.cloud.storage.VolumeApiServiceImpl.AllowCheckAndRepairVolume.value()) { s_logger.info(String.format("Trying to check and repair the volume %d", dataObject.getId())); - String repair = CheckVolumeAndRepairCmd.RepairValues.leaks.name(); + String repair = CheckAndRepairVolumeCmd.RepairValues.leaks.name(); CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); volumeInfo.addPayload(payload); @@ -2784,11 +2784,11 @@ public Pair checkAndRepairVolume(VolumeInfo volume) { Long poolId = volume.getPoolId(); StoragePool pool = _storageMgr.getStoragePool(poolId); CheckAndRepairVolumePayload payload = (CheckAndRepairVolumePayload) volume.getpayload(); - CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.getRepair(), + CheckAndRepairVolumeCommand command = new CheckAndRepairVolumeCommand(volume.getPath(), new StorageFilerTO(pool), payload.getRepair(), volume.getPassphrase(), volume.getEncryptFormat()); try { - CheckVolumeAndRepairAnswer answer = (CheckVolumeAndRepairAnswer) _storageMgr.sendToPool(pool, null, command); + CheckAndRepairVolumeAnswer answer = (CheckAndRepairVolumeAnswer) _storageMgr.sendToPool(pool, null, command); if (answer != null && answer.getResult()) { s_logger.debug("Check volume response result: " + answer.getDetails()); payload.setVolumeCheckExecutionResult(answer.getVolumeCheckExecutionResult()); @@ -2797,7 +2797,8 @@ public Pair checkAndRepairVolume(VolumeInfo volume) { } return new Pair<>(answer.getVolumeCheckExecutionResult(), answer.getVolumeRepairExecutionResult()); } else { - s_logger.debug("Failed to check and repair the volume with error " + answer.getDetails()); + String errMsg = (answer == null) ? null : answer.getDetails(); + s_logger.debug("Failed to check and repair the volume with error " + errMsg); } } catch (Exception e) { diff --git a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java index 46efaa5ae9cf..240d7f9d12ea 100644 --- a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java +++ b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java @@ -19,8 +19,8 @@ package org.apache.cloudstack.storage.volume; -import com.cloud.agent.api.storage.CheckVolumeAndRepairAnswer; -import com.cloud.agent.api.storage.CheckVolumeAndRepairCommand; +import com.cloud.agent.api.storage.CheckAndRepairVolumeAnswer; +import com.cloud.agent.api.storage.CheckAndRepairVolumeCommand; import com.cloud.agent.api.to.StorageFilerTO; import com.cloud.exception.StorageUnavailableException; import com.cloud.storage.CheckAndRepairVolumePayload; @@ -247,10 +247,10 @@ public void testCheckAndRepairVolume() throws StorageUnavailableException { " \"fragmented-clusters\": 96135\n" + "}"; - CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.getRepair(), + CheckAndRepairVolumeCommand command = new CheckAndRepairVolumeCommand(volume.getPath(), new StorageFilerTO(pool), payload.getRepair(), volume.getPassphrase(), volume.getEncryptFormat()); - CheckVolumeAndRepairAnswer answer = new CheckVolumeAndRepairAnswer(command, true, checkResult); + CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, true, checkResult); answer.setVolumeCheckExecutionResult(checkResult); Mockito.when(storageManagerMock.sendToPool(pool, null, command)).thenReturn(answer); @@ -272,10 +272,10 @@ public void testCheckAndRepairVolumeWhenFailure() throws StorageUnavailableExcep Mockito.when(volume.getPassphrase()).thenReturn(new byte[] {3, 1, 2, 3}); Mockito.when(volume.getEncryptFormat()).thenReturn("LUKS"); - CheckVolumeAndRepairCommand command = new CheckVolumeAndRepairCommand(volume.getPath(), new StorageFilerTO(pool), payload.getRepair(), + CheckAndRepairVolumeCommand command = new CheckAndRepairVolumeCommand(volume.getPath(), new StorageFilerTO(pool), payload.getRepair(), volume.getPassphrase(), volume.getEncryptFormat()); - CheckVolumeAndRepairAnswer answer = new CheckVolumeAndRepairAnswer(command, false, "Unable to execute qemu command"); + CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, false, "Unable to execute qemu command"); Mockito.when(storageManagerMock.sendToPool(pool, null, command)).thenReturn(answer); Pair result = volumeServiceImplSpy.checkAndRepairVolume(volume); 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/LibvirtCheckAndRepairVolumeCommandWrapper.java similarity index 86% rename from plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapper.java rename to plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java index 280e2aa14d24..38eb31875166 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/LibvirtCheckAndRepairVolumeCommandWrapper.java @@ -20,8 +20,8 @@ package com.cloud.hypervisor.kvm.resource.wrapper; import com.cloud.agent.api.Answer; -import com.cloud.agent.api.storage.CheckVolumeAndRepairCommand; -import com.cloud.agent.api.storage.CheckVolumeAndRepairAnswer; +import com.cloud.agent.api.storage.CheckAndRepairVolumeCommand; +import com.cloud.agent.api.storage.CheckAndRepairVolumeAnswer; import com.cloud.agent.api.to.StorageFilerTO; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; @@ -48,13 +48,13 @@ import java.util.Arrays; import java.util.List; -@ResourceWrapper(handles = CheckVolumeAndRepairCommand.class) -public class LibvirtCheckVolumeAndRepairCommandWrapper extends CommandWrapper { +@ResourceWrapper(handles = CheckAndRepairVolumeCommand.class) +public class LibvirtCheckAndRepairVolumeCommandWrapper extends CommandWrapper { - private static final Logger s_logger = Logger.getLogger(LibvirtCheckVolumeAndRepairCommandWrapper.class); + private static final Logger s_logger = Logger.getLogger(LibvirtCheckAndRepairVolumeCommandWrapper.class); @Override - public Answer execute(CheckVolumeAndRepairCommand command, LibvirtComputingResource serverResource) { + public Answer execute(CheckAndRepairVolumeCommand command, LibvirtComputingResource serverResource) { final String volumeId = command.getPath(); final String repair = command.getRepair(); final StorageFilerTO spool = command.getPool(); @@ -66,9 +66,9 @@ public Answer execute(CheckVolumeAndRepairCommand command, LibvirtComputingResou QemuObject.EncryptFormat encryptFormat = QemuObject.EncryptFormat.enumValue(command.getEncryptFormat()); byte[] passphrase = command.getPassphrase(); try { - String checkVolumeResult = checkVolumeAndRepair(vol, repair, encryptFormat, passphrase, serverResource); + String checkVolumeResult = checkAndRepairVolume(vol, repair, encryptFormat, passphrase, serverResource); s_logger.info(String.format("Check Volume result for the volume %s is %s", vol.getName(), checkVolumeResult)); - CheckVolumeAndRepairAnswer answer = new CheckVolumeAndRepairAnswer(command, true, checkVolumeResult); + CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, true, checkVolumeResult); answer.setVolumeCheckExecutionResult(checkVolumeResult); int leaks = 0; @@ -85,7 +85,7 @@ public Answer execute(CheckVolumeAndRepairCommand command, LibvirtComputingResou s_logger.info(msg); String jsonStringFormat = String.format("{ \"message\": \"%s\" }", msg); String finalResult = (checkVolumeResult != null ? checkVolumeResult.concat(",") : "") + jsonStringFormat; - answer = new CheckVolumeAndRepairAnswer(command, true, finalResult); + answer = new CheckAndRepairVolumeAnswer(command, true, finalResult); answer.setVolumeRepairExecutionResult(jsonStringFormat); answer.setVolumeCheckExecutionResult(checkVolumeResult); @@ -94,17 +94,17 @@ public Answer execute(CheckVolumeAndRepairCommand command, LibvirtComputingResou } if (StringUtils.isNotEmpty(repair)) { - String repairVolumeResult = checkVolumeAndRepair(vol, repair, encryptFormat, passphrase, serverResource); + String repairVolumeResult = checkAndRepairVolume(vol, repair, encryptFormat, passphrase, serverResource); String finalResult = (checkVolumeResult != null ? checkVolumeResult.concat(",") : "") + repairVolumeResult; s_logger.info(String.format("Repair Volume result for the volume %s is %s", vol.getName(), repairVolumeResult)); - answer = new CheckVolumeAndRepairAnswer(command, true, finalResult); + answer = new CheckAndRepairVolumeAnswer(command, true, finalResult); answer.setVolumeRepairExecutionResult(repairVolumeResult); answer.setVolumeCheckExecutionResult(checkVolumeResult); } return answer; } catch (Exception e) { - return new CheckVolumeAndRepairAnswer(command, false, e.toString()); + return new CheckAndRepairVolumeAnswer(command, false, e.toString()); } finally { if (passphrase != null) { Arrays.fill(passphrase, (byte) 0); @@ -112,7 +112,7 @@ public Answer execute(CheckVolumeAndRepairCommand command, LibvirtComputingResou } } - protected String checkVolumeAndRepair(final KVMPhysicalDisk vol, final String repair, final QemuObject.EncryptFormat encryptFormat, byte[] passphrase, final LibvirtComputingResource libvirtComputingResource) throws CloudRuntimeException { + protected String checkAndRepairVolume(final KVMPhysicalDisk vol, final String repair, final QemuObject.EncryptFormat encryptFormat, byte[] passphrase, final LibvirtComputingResource libvirtComputingResource) throws CloudRuntimeException { List passphraseObjects = new ArrayList<>(); QemuImageOptions imgOptions = null; if (ArrayUtils.isEmpty(passphrase)) { diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java similarity index 84% rename from plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java rename to plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java index dd7b6e0e2f73..3fa113dadbdd 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeAndRepairCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java @@ -14,8 +14,8 @@ package com.cloud.hypervisor.kvm.resource.wrapper; -import com.cloud.agent.api.storage.CheckVolumeAndRepairAnswer; -import com.cloud.agent.api.storage.CheckVolumeAndRepairCommand; +import com.cloud.agent.api.storage.CheckAndRepairVolumeAnswer; +import com.cloud.agent.api.storage.CheckAndRepairVolumeCommand; import com.cloud.agent.api.to.StorageFilerTO; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; @@ -37,16 +37,16 @@ import org.powermock.modules.junit4.PowerMockRunner; @RunWith(PowerMockRunner.class) -public class LibvirtCheckVolumeAndRepairCommandWrapperTest { +public class LibvirtCheckAndRepairVolumeCommandWrapperTest { @Spy - LibvirtCheckVolumeAndRepairCommandWrapper libvirtCheckVolumeAndRepairCommandWrapperSpy = Mockito.spy(LibvirtCheckVolumeAndRepairCommandWrapper.class); + LibvirtCheckAndRepairVolumeCommandWrapper libvirtCheckAndRepairVolumeCommandWrapperSpy = Mockito.spy(LibvirtCheckAndRepairVolumeCommandWrapper.class); @Mock LibvirtComputingResource libvirtComputingResourceMock; @Mock - CheckVolumeAndRepairCommand checkVolumeAndRepairCommand; + CheckAndRepairVolumeCommand checkAndRepairVolumeCommand; @Mock QemuImg qemuImgMock; @@ -57,10 +57,10 @@ public void init() { } @Test - @PrepareForTest(LibvirtCheckVolumeAndRepairCommandWrapper.class) + @PrepareForTest(LibvirtCheckAndRepairVolumeCommandWrapper.class) public void testCheckAndRepairVolume() throws Exception { - CheckVolumeAndRepairCommand cmd = Mockito.mock(CheckVolumeAndRepairCommand.class); + CheckAndRepairVolumeCommand cmd = Mockito.mock(CheckAndRepairVolumeCommand.class); Mockito.when(cmd.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); Mockito.when(cmd.getRepair()).thenReturn(null); StorageFilerTO spool = Mockito.mock(StorageFilerTO.class); @@ -94,7 +94,7 @@ public void testCheckAndRepairVolume() throws Exception { PowerMockito.whenNew(QemuImg.class).withArguments(Mockito.anyInt()).thenReturn(qemuImgMock); Mockito.when(qemuImgMock.checkAndRepair(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(checkResult); - CheckVolumeAndRepairAnswer result = (CheckVolumeAndRepairAnswer) libvirtCheckVolumeAndRepairCommandWrapperSpy.execute(cmd, libvirtComputingResourceMock); + CheckAndRepairVolumeAnswer result = (CheckAndRepairVolumeAnswer) libvirtCheckAndRepairVolumeCommandWrapperSpy.execute(cmd, libvirtComputingResourceMock); Assert.assertEquals(checkResult, result.getVolumeCheckExecutionResult()); } diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 440f1cb85754..a6935667b8c9 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -552,7 +552,7 @@ import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd; import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ChangeOfferingForVolumeCmd; -import org.apache.cloudstack.api.command.user.volume.CheckVolumeAndRepairCmd; +import org.apache.cloudstack.api.command.user.volume.CheckAndRepairVolumeCmd; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; import org.apache.cloudstack.api.command.user.volume.DeleteVolumeCmd; import org.apache.cloudstack.api.command.user.volume.DestroyVolumeCmd; @@ -3705,7 +3705,7 @@ public List> getCommands() { cmdList.add(ListVMGroupsCmd.class); cmdList.add(UpdateVMGroupCmd.class); cmdList.add(AttachVolumeCmd.class); - cmdList.add(CheckVolumeAndRepairCmd.class); + cmdList.add(CheckAndRepairVolumeCmd.class); cmdList.add(CreateVolumeCmd.class); cmdList.add(DeleteVolumeCmd.class); cmdList.add(UpdateVolumeCmd.class); diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index e58dd110afd1..dcf9fb3f80de 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -42,7 +42,7 @@ import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd; import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ChangeOfferingForVolumeCmd; -import org.apache.cloudstack.api.command.user.volume.CheckVolumeAndRepairCmd; +import org.apache.cloudstack.api.command.user.volume.CheckAndRepairVolumeCmd; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; import org.apache.cloudstack.api.command.user.volume.DetachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ExtractVolumeCmd; @@ -381,7 +381,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic public static ConfigKey storageTagRuleExecutionTimeout = new ConfigKey<>("Advanced", Long.class, "storage.tag.rule.execution.timeout", "2000", "The maximum runtime," + " in milliseconds, to execute a storage tag rule; if it is reached, a timeout will happen.", true); - public static final ConfigKey AllowVolumeCheckAndRepair = new ConfigKey("Advanced", Boolean.class, "volume.check.and.repair.before.use", "false", + public static final ConfigKey AllowCheckAndRepairVolume = new ConfigKey("Advanced", Boolean.class, "volume.check.and.repair.leaks.before.use", "false", "To check and repair the volume if it has any leaks before performing volume attach or VM start operations", true); private final StateMachine2 _volStateMachine; @@ -1824,7 +1824,7 @@ public void publishVolumeCreationUsageEvent(Volume volume) { @Override @ActionEvent(eventType = EventTypes.EVENT_VOLUME_CHECK, eventDescription = "checking volume and repair if needed", async = true) - public Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) throws ResourceAllocationException { + public Pair checkAndRepairVolume(CheckAndRepairVolumeCmd cmd) throws ResourceAllocationException { long volumeId = cmd.getId(); String repair = cmd.getRepair(); @@ -1841,13 +1841,13 @@ public Pair checkAndRepairVolume(CheckVolumeAndRepairCmd cmd) th VmWorkJobVO placeHolder = null; placeHolder = createPlaceHolderWork(vmId); try { - Pair result = orchestrateCheckVolumeAndRepair(volumeId, repair); + Pair result = orchestrateCheckAndRepairVolume(volumeId, repair); return result; } finally { _workJobDao.expunge(placeHolder.getId()); } } else { - Outcome outcome = checkVolumeAndRepairThroughJobQueue(vmId, volumeId, repair); + Outcome outcome = checkAndRepairVolumeThroughJobQueue(vmId, volumeId, repair); try { outcome.get(); @@ -1914,7 +1914,7 @@ protected void validationsForCheckVolumeOperation(VolumeVO volume) { } } - private Pair orchestrateCheckVolumeAndRepair(Long volumeId, String repair) { + private Pair orchestrateCheckAndRepairVolume(Long volumeId, String repair) { VolumeInfo volume = volFactory.getVolume(volumeId); @@ -1932,7 +1932,7 @@ private Pair orchestrateCheckVolumeAndRepair(Long volumeId, Stri return volService.checkAndRepairVolume(volume); } - public Outcome checkVolumeAndRepairThroughJobQueue(final Long vmId, final Long volumeId, String repair) { + public Outcome checkAndRepairVolumeThroughJobQueue(final Long vmId, final Long volumeId, String repair) { final CallContext context = CallContext.current(); final User callingUser = context.getCallingUser(); @@ -5003,9 +5003,9 @@ private Pair orchestrateTakeVolumeSnapshot(VmWorkTakeVol } @ReflectionUse - private Pair orchestrateCheckVolumeAndRepair(VmWorkCheckAndRepairVolume work) throws Exception { + private Pair orchestrateCheckAndRepairVolume(VmWorkCheckAndRepairVolume work) throws Exception { Account account = _accountDao.findById(work.getAccountId()); - Pair result = orchestrateCheckVolumeAndRepair(work.getVolumeId(), work.getRepair()); + Pair result = orchestrateCheckAndRepairVolume(work.getVolumeId(), work.getRepair()); return new Pair(JobInfo.Status.SUCCEEDED, _jobMgr.marshallResultObject(result)); } @@ -5046,7 +5046,7 @@ public ConfigKey[] getConfigKeys() { MatchStoragePoolTagsWithDiskOffering, UseHttpsToUpload, WaitDetachDevice, - AllowVolumeCheckAndRepair + AllowCheckAndRepairVolume }; } } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index bab5bdd20a9d..4957eddecb8c 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -40,7 +40,7 @@ import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.SecurityChecker.AccessType; -import org.apache.cloudstack.api.command.user.volume.CheckVolumeAndRepairCmd; +import org.apache.cloudstack.api.command.user.volume.CheckAndRepairVolumeCmd; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; import org.apache.cloudstack.api.command.user.volume.DetachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.MigrateVolumeCmd; @@ -1766,7 +1766,7 @@ public void testValidationsForCheckVolumeAPIWithNonKVMhypervisor() { @Test public void testCheckAndRepairVolume() throws ResourceAllocationException { - CheckVolumeAndRepairCmd cmd = mock(CheckVolumeAndRepairCmd.class); + CheckAndRepairVolumeCmd cmd = mock(CheckAndRepairVolumeCmd.class); when(cmd.getId()).thenReturn(1L); when(cmd.getRepair()).thenReturn(null); From 8e4017d28a47a5a050fd5f11156b03e203019c38 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Mon, 29 Jan 2024 16:21:34 +0530 Subject: [PATCH 10/23] Some code fixes --- .../command/user/volume/CheckAndRepairVolumeCmd.java | 10 +++++----- .../apache/cloudstack/api/response/VolumeResponse.java | 4 ++-- .../cloudstack/storage/volume/VolumeServiceImpl.java | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java index 01835de8ce17..b47a42e028fb 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java @@ -36,11 +36,11 @@ import com.cloud.utils.Pair; import com.cloud.utils.StringUtils; +import java.util.Arrays; + @APICommand(name = "checkVolume", description = "Check the volume for any errors or leaks and also repairs when repair parameter is passed, this is currently supported for KVM only", responseObject = VolumeResponse.class, entityType = {Volume.class}, since = "4.18.1", - authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}, - requestHasSensitiveInfo = false, - responseHasSensitiveInfo = true) + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) public class CheckAndRepairVolumeCmd extends BaseCmd { public static final Logger s_logger = Logger.getLogger(CheckAndRepairVolumeCmd.class.getName()); @@ -61,7 +61,7 @@ public class CheckAndRepairVolumeCmd extends BaseCmd { ///////////////////////////////////////////////////// public enum RepairValues { - leaks, all + Leaks, All } public Long getId() { @@ -72,7 +72,7 @@ public String getRepair() { if (org.apache.commons.lang3.StringUtils.isNotEmpty(repair)) { RepairValues repairType = Enum.valueOf(RepairValues.class, repair); if (repairType == null) { - throw new InvalidParameterValueException("repair parameter only takes either leaks or all as value"); + throw new InvalidParameterValueException(String.format("Repair parameter can only take the following values: %s" + Arrays.toString(RepairValues.values()))); } } return repair; diff --git a/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java index daf3cf3950fe..6397fce03939 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java @@ -290,11 +290,11 @@ public class VolumeResponse extends BaseResponseWithTagInformation implements Co private String externalUuid; @SerializedName(ApiConstants.VOLUME_CHECK_RESULT) - @Param(description = "details for the volume check result, they may vary for different hypervisors") + @Param(description = "details for the volume check result, they may vary for different hypervisors, since = 4.18.1") private Map volumeCheckResult; @SerializedName(ApiConstants.VOLUME_REPAIR_RESULT) - @Param(description = "details for the volume repair result, they may vary for different hypervisors") + @Param(description = "details for the volume repair result, they may vary for different hypervisors, since = 4.18.1") private Map volumeRepairResult; public String getPath() { 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 76b6b7de9c05..d4475f0da7ba 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 @@ -2770,7 +2770,7 @@ public void checkAndRepairVolumeBasedOnConfig(DataObject dataObject, Host host) if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { if (com.cloud.storage.VolumeApiServiceImpl.AllowCheckAndRepairVolume.value()) { s_logger.info(String.format("Trying to check and repair the volume %d", dataObject.getId())); - String repair = CheckAndRepairVolumeCmd.RepairValues.leaks.name(); + String repair = CheckAndRepairVolumeCmd.RepairValues.Leaks.name(); CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); volumeInfo.addPayload(payload); From a5d3999fcaa9270aef5be4289f880a21aa274f59 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 30 Jan 2024 11:32:26 +0530 Subject: [PATCH 11/23] remove unused code --- .../storage/volume/VolumeServiceImpl.java | 4 ---- .../storage/CheckAndRepairVolumePayload.java | 17 ----------------- 2 files changed, 21 deletions(-) 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 d4475f0da7ba..d765665a0caf 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 @@ -2791,10 +2791,6 @@ public Pair checkAndRepairVolume(VolumeInfo volume) { CheckAndRepairVolumeAnswer answer = (CheckAndRepairVolumeAnswer) _storageMgr.sendToPool(pool, null, command); if (answer != null && answer.getResult()) { s_logger.debug("Check volume response result: " + answer.getDetails()); - payload.setVolumeCheckExecutionResult(answer.getVolumeCheckExecutionResult()); - if (StringUtils.isNotEmpty(payload.getRepair())) { - payload.setVolumeRepairedExecutionResult(answer.getVolumeRepairExecutionResult()); - } return new Pair<>(answer.getVolumeCheckExecutionResult(), answer.getVolumeRepairExecutionResult()); } else { String errMsg = (answer == null) ? null : answer.getDetails(); diff --git a/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java b/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java index 6d06f2a4c36d..eabe1a4c7b81 100644 --- a/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java +++ b/server/src/main/java/com/cloud/storage/CheckAndRepairVolumePayload.java @@ -21,8 +21,6 @@ public class CheckAndRepairVolumePayload { public final String repair; public String result; - private String volumeCheckExecutionResult; - private String volumeRepairedExecutionResult; public CheckAndRepairVolumePayload(String repair) { this.repair = repair; @@ -40,19 +38,4 @@ public void setResult(String result) { this.result = result; } - public String getVolumeCheckExecutionResult() { - return volumeCheckExecutionResult; - } - - public String getVolumeRepairedExecutionResult() { - return volumeRepairedExecutionResult; - } - - public void setVolumeCheckExecutionResult(String volumeCheckExecutionResult) { - this.volumeCheckExecutionResult = volumeCheckExecutionResult; - } - - public void setVolumeRepairedExecutionResult(String volumeRepairedExecutionResult) { - this.volumeRepairedExecutionResult = volumeRepairedExecutionResult; - } } From a109da1e3d2163a044bd12924e8bfc866d9b794d Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 30 Jan 2024 12:44:16 +0530 Subject: [PATCH 12/23] Renamed repair values --- .../api/command/user/volume/CheckAndRepairVolumeCmd.java | 6 +++--- .../apache/cloudstack/storage/volume/VolumeServiceImpl.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java index b47a42e028fb..9e32aac7ae97 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java @@ -61,7 +61,7 @@ public class CheckAndRepairVolumeCmd extends BaseCmd { ///////////////////////////////////////////////////// public enum RepairValues { - Leaks, All + LEAKS, ALL } public Long getId() { @@ -70,12 +70,12 @@ public Long getId() { public String getRepair() { if (org.apache.commons.lang3.StringUtils.isNotEmpty(repair)) { - RepairValues repairType = Enum.valueOf(RepairValues.class, repair); + RepairValues repairType = Enum.valueOf(RepairValues.class, repair.toUpperCase()); if (repairType == null) { throw new InvalidParameterValueException(String.format("Repair parameter can only take the following values: %s" + Arrays.toString(RepairValues.values()))); } } - return repair; + return repair.toLowerCase(); } ///////////////////////////////////////////////////// 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 d765665a0caf..580e7207404f 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 @@ -2770,7 +2770,7 @@ public void checkAndRepairVolumeBasedOnConfig(DataObject dataObject, Host host) if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { if (com.cloud.storage.VolumeApiServiceImpl.AllowCheckAndRepairVolume.value()) { s_logger.info(String.format("Trying to check and repair the volume %d", dataObject.getId())); - String repair = CheckAndRepairVolumeCmd.RepairValues.Leaks.name(); + String repair = CheckAndRepairVolumeCmd.RepairValues.LEAKS.name().toLowerCase(); CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); volumeInfo.addPayload(payload); From d6495c43749f09ba36659de5bc385321c4103853 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 30 Jan 2024 18:37:06 +0530 Subject: [PATCH 13/23] Fixed unit tests --- ...heckAndRepairVolumeCommandWrapperTest.java | 42 +++++++++---------- .../storage/VolumeApiServiceImplTest.java | 5 --- 2 files changed, 20 insertions(+), 27 deletions(-) diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java index 3fa113dadbdd..9eccc7c15b49 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java @@ -30,13 +30,14 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; +import org.mockito.MockedConstruction; import org.mockito.Mockito; import org.mockito.Spy; -import org.powermock.api.mockito.PowerMockito; -import org.powermock.core.classloader.annotations.PrepareForTest; -import org.powermock.modules.junit4.PowerMockRunner; +import org.mockito.junit.MockitoJUnitRunner; -@RunWith(PowerMockRunner.class) +import static org.mockito.Mockito.when; + +@RunWith(MockitoJUnitRunner.class) public class LibvirtCheckAndRepairVolumeCommandWrapperTest { @Spy @@ -53,32 +54,29 @@ public class LibvirtCheckAndRepairVolumeCommandWrapperTest { @Before public void init() { - Mockito.when(libvirtComputingResourceMock.getCmdsTimeout()).thenReturn(60); + when(libvirtComputingResourceMock.getCmdsTimeout()).thenReturn(60); } @Test - @PrepareForTest(LibvirtCheckAndRepairVolumeCommandWrapper.class) public void testCheckAndRepairVolume() throws Exception { CheckAndRepairVolumeCommand cmd = Mockito.mock(CheckAndRepairVolumeCommand.class); - Mockito.when(cmd.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); - Mockito.when(cmd.getRepair()).thenReturn(null); + when(cmd.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); + when(cmd.getRepair()).thenReturn(null); StorageFilerTO spool = Mockito.mock(StorageFilerTO.class); - Mockito.when(cmd.getPool()).thenReturn(spool); + when(cmd.getPool()).thenReturn(spool); KVMStoragePoolManager storagePoolMgr = Mockito.mock(KVMStoragePoolManager.class); - Mockito.when(libvirtComputingResourceMock.getStoragePoolMgr()).thenReturn(storagePoolMgr); + when(libvirtComputingResourceMock.getStoragePoolMgr()).thenReturn(storagePoolMgr); KVMStoragePool pool = Mockito.mock(KVMStoragePool.class); - Mockito.when(spool.getType()).thenReturn(Storage.StoragePoolType.PowerFlex); - Mockito.when(spool.getUuid()).thenReturn("b6be258b-42b8-49a4-ad51-3634ef8ff76a"); - Mockito.when(storagePoolMgr.getStoragePool(Storage.StoragePoolType.PowerFlex, "b6be258b-42b8-49a4-ad51-3634ef8ff76a")).thenReturn(pool); + when(spool.getType()).thenReturn(Storage.StoragePoolType.PowerFlex); + when(spool.getUuid()).thenReturn("b6be258b-42b8-49a4-ad51-3634ef8ff76a"); + when(storagePoolMgr.getStoragePool(Storage.StoragePoolType.PowerFlex, "b6be258b-42b8-49a4-ad51-3634ef8ff76a")).thenReturn(pool); KVMPhysicalDisk vol = Mockito.mock(KVMPhysicalDisk.class); - Mockito.when(pool.getPhysicalDisk("cbac516a-0f1f-4559-921c-1a7c6c408ccf")).thenReturn(vol); + when(pool.getPhysicalDisk("cbac516a-0f1f-4559-921c-1a7c6c408ccf")).thenReturn(vol); VolumeInfo volume = Mockito.mock(VolumeInfo.class); - Mockito.when(volume.getPoolId()).thenReturn(1L); - Mockito.when(volume.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); String checkResult = "{\n" + " \"image-end-offset\": 6442582016,\n" + @@ -91,12 +89,12 @@ public void testCheckAndRepairVolume() throws Exception { " \"fragmented-clusters\": 96135\n" + "}"; - PowerMockito.whenNew(QemuImg.class).withArguments(Mockito.anyInt()).thenReturn(qemuImgMock); - Mockito.when(qemuImgMock.checkAndRepair(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(checkResult); - - CheckAndRepairVolumeAnswer result = (CheckAndRepairVolumeAnswer) libvirtCheckAndRepairVolumeCommandWrapperSpy.execute(cmd, libvirtComputingResourceMock); - - Assert.assertEquals(checkResult, result.getVolumeCheckExecutionResult()); + try (MockedConstruction ignored = Mockito.mockConstruction(QemuImg.class, (mock, context) -> { + when(mock.checkAndRepair(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(checkResult); + })) { + CheckAndRepairVolumeAnswer result = (CheckAndRepairVolumeAnswer) libvirtCheckAndRepairVolumeCommandWrapperSpy.execute(cmd, libvirtComputingResourceMock); + Assert.assertEquals(checkResult, result.getVolumeCheckExecutionResult()); + } } } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 4957eddecb8c..f0b70f36b5c9 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -1666,7 +1666,6 @@ public void testStoragePoolCompatibilityAndAllowEncryptedVolumeMigrationForPower @Test public void testValidationsForCheckVolumeAPI() { VolumeVO volume = mock(VolumeVO.class); - when(volumeDaoMock.findById(1L)).thenReturn(volume); AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); @@ -1688,7 +1687,6 @@ public void testValidationsForCheckVolumeAPI() { @Test(expected = InvalidParameterValueException.class) public void testValidationsForCheckVolumeAPIWithRunningVM() { VolumeVO volume = mock(VolumeVO.class); - when(volumeDaoMock.findById(1L)).thenReturn(volume); AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); @@ -1707,7 +1705,6 @@ public void testValidationsForCheckVolumeAPIWithRunningVM() { @Test(expected = InvalidParameterValueException.class) public void testValidationsForCheckVolumeAPIWithNonexistedVM() { VolumeVO volume = mock(VolumeVO.class); - when(volumeDaoMock.findById(1L)).thenReturn(volume); AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); @@ -1724,7 +1721,6 @@ public void testValidationsForCheckVolumeAPIWithNonexistedVM() { @Test(expected = InvalidParameterValueException.class) public void testValidationsForCheckVolumeAPIWithAllocatedVolume() { VolumeVO volume = mock(VolumeVO.class); - when(volumeDaoMock.findById(1L)).thenReturn(volume); AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); @@ -1744,7 +1740,6 @@ public void testValidationsForCheckVolumeAPIWithAllocatedVolume() { @Test(expected = InvalidParameterValueException.class) public void testValidationsForCheckVolumeAPIWithNonKVMhypervisor() { VolumeVO volume = mock(VolumeVO.class); - when(volumeDaoMock.findById(1L)).thenReturn(volume); AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); From 6ff875f93f5dc1464ca70c6cc075e1b7d0229b81 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 30 Jan 2024 18:50:01 +0530 Subject: [PATCH 14/23] changed version --- .../api/command/user/volume/CheckAndRepairVolumeCmd.java | 2 +- .../org/apache/cloudstack/api/response/VolumeResponse.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java index 9e32aac7ae97..4be4b99c0abc 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java @@ -39,7 +39,7 @@ import java.util.Arrays; @APICommand(name = "checkVolume", description = "Check the volume for any errors or leaks and also repairs when repair parameter is passed, this is currently supported for KVM only", responseObject = VolumeResponse.class, entityType = {Volume.class}, - since = "4.18.1", + since = "4.19.1", authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) public class CheckAndRepairVolumeCmd extends BaseCmd { public static final Logger s_logger = Logger.getLogger(CheckAndRepairVolumeCmd.class.getName()); diff --git a/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java index 6397fce03939..0d502a6d7a73 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/VolumeResponse.java @@ -290,11 +290,11 @@ public class VolumeResponse extends BaseResponseWithTagInformation implements Co private String externalUuid; @SerializedName(ApiConstants.VOLUME_CHECK_RESULT) - @Param(description = "details for the volume check result, they may vary for different hypervisors, since = 4.18.1") + @Param(description = "details for the volume check result, they may vary for different hypervisors, since = 4.19.1") private Map volumeCheckResult; @SerializedName(ApiConstants.VOLUME_REPAIR_RESULT) - @Param(description = "details for the volume repair result, they may vary for different hypervisors, since = 4.18.1") + @Param(description = "details for the volume repair result, they may vary for different hypervisors, since = 4.19.1") private Map volumeRepairResult; public String getPath() { From a143e826eeece75b74bb0314761c345b082a7aad Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 20 Feb 2024 15:41:12 +0530 Subject: [PATCH 15/23] Address review comments --- .../storage/volume/VolumeServiceImpl.java | 3 ++- ...virtCheckAndRepairVolumeCommandWrapper.java | 4 ++-- .../apache/cloudstack/utils/qemu/QemuImg.java | 18 +++++++++--------- .../cloud/storage/VolumeApiServiceImpl.java | 18 +++++++----------- 4 files changed, 20 insertions(+), 23 deletions(-) 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 580e7207404f..65b8b8feaac4 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 @@ -32,6 +32,7 @@ import javax.inject.Inject; +import com.cloud.storage.VolumeApiServiceImpl; import org.apache.cloudstack.annotation.AnnotationService; import org.apache.cloudstack.annotation.dao.AnnotationDao; import org.apache.cloudstack.api.command.user.volume.CheckAndRepairVolumeCmd; @@ -2768,7 +2769,7 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) { @Override public void checkAndRepairVolumeBasedOnConfig(DataObject dataObject, Host host) { if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { - if (com.cloud.storage.VolumeApiServiceImpl.AllowCheckAndRepairVolume.value()) { + if (VolumeApiServiceImpl.AllowCheckAndRepairVolume.value()) { s_logger.info(String.format("Trying to check and repair the volume %d", dataObject.getId())); String repair = CheckAndRepairVolumeCmd.RepairValues.LEAKS.name().toLowerCase(); CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java index 38eb31875166..a9d9cd948e6b 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java @@ -66,7 +66,7 @@ public Answer execute(CheckAndRepairVolumeCommand command, LibvirtComputingResou QemuObject.EncryptFormat encryptFormat = QemuObject.EncryptFormat.enumValue(command.getEncryptFormat()); byte[] passphrase = command.getPassphrase(); try { - String checkVolumeResult = checkAndRepairVolume(vol, repair, encryptFormat, passphrase, serverResource); + String checkVolumeResult = checkAndRepairVolume(vol, null, encryptFormat, passphrase, serverResource); s_logger.info(String.format("Check Volume result for the volume %s is %s", vol.getName(), checkVolumeResult)); CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, true, checkVolumeResult); answer.setVolumeCheckExecutionResult(checkVolumeResult); @@ -77,7 +77,7 @@ public Answer execute(CheckAndRepairVolumeCommand command, LibvirtComputingResou JsonNode jsonNode = objectMapper.readTree(checkVolumeResult); JsonNode leaksNode = jsonNode.get("leaks"); if (leaksNode != null) { - leaks = jsonNode.asInt(); + leaks = leaksNode.asInt(); } if (leaks == 0) { 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 b8d94ccaef7e..0eae283821d2 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 @@ -824,29 +824,29 @@ protected static boolean helpSupportsImageFormat(String text, QemuImg.PhysicalDi * Boolean option whether to repair any leaks */ public String checkAndRepair(final QemuImgFile file, final QemuImageOptions imageOptions, final List qemuObjects, final String repair) throws QemuImgException { - final Script s = new Script(_qemuImgPath); - s.add("check"); + final Script script = new Script(_qemuImgPath); + script.add("check"); if (imageOptions == null) { - s.add(file.getFileName()); + script.add(file.getFileName()); } for (QemuObject o : qemuObjects) { - s.add(o.toCommandFlag()); + script.add(o.toCommandFlag()); } if (imageOptions != null) { - s.add(imageOptions.toCommandFlag()); + script.add(imageOptions.toCommandFlag()); } - s.add("--output=json"); + script.add("--output=json"); if (StringUtils.isNotEmpty(repair)) { - s.add("-r"); - s.add(repair); + script.add("-r"); + script.add(repair); } OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); - final String result = s.execute(parser); + final String result = script.execute(parser); if (result != null) { throw new QemuImgException(result); } diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index dcf9fb3f80de..36b0aee98c5b 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1340,7 +1340,7 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws ResourceAllocationExcep outcome.get(); } catch (InterruptedException e) { throw new RuntimeException("Operation was interrupted", e); - } catch (java.util.concurrent.ExecutionException e) { + } catch (ExecutionException e) { throw new RuntimeException("Execution exception", e); } @@ -1853,7 +1853,7 @@ public Pair checkAndRepairVolume(CheckAndRepairVolumeCmd cmd) th outcome.get(); } catch (InterruptedException e) { throw new RuntimeException("Operation is interrupted", e); - } catch (java.util.concurrent.ExecutionException e) { + } catch (ExecutionException e) { throw new RuntimeException("Execution exception--", e); } @@ -1922,10 +1922,6 @@ private Pair orchestrateCheckAndRepairVolume(Long volumeId, Stri throw new InvalidParameterValueException("Checking volume and repairing failed due to volume:" + volumeId + " doesn't exist"); } - if (volume.getState() != Volume.State.Ready) { - throw new InvalidParameterValueException("VolumeId: " + volumeId + " is not in " + Volume.State.Ready + " state but " + volume.getState() + ". Cannot check and repair the volume."); - } - CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); volume.addPayload(payload); @@ -2132,7 +2128,7 @@ private VolumeVO resizeVolumeInternal(VolumeVO volume, DiskOfferingVO newDiskOff outcome.get(); } catch (InterruptedException e) { throw new RuntimeException("Operation was interrupted", e); - } catch (java.util.concurrent.ExecutionException e) { + } catch (ExecutionException e) { throw new RuntimeException("Execution exception", e); } @@ -2918,7 +2914,7 @@ public Volume detachVolumeFromVM(DetachVolumeCmd cmmd) { outcome.get(); } catch (InterruptedException e) { throw new RuntimeException("Operation is interrupted", e); - } catch (java.util.concurrent.ExecutionException e) { + } catch (ExecutionException e) { throw new RuntimeException("Execution excetion", e); } @@ -3326,7 +3322,7 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) { outcome.get(); } catch (InterruptedException e) { throw new RuntimeException("Operation is interrupted", e); - } catch (java.util.concurrent.ExecutionException e) { + } catch (ExecutionException e) { throw new RuntimeException("Execution excetion", e); } @@ -3655,7 +3651,7 @@ private Snapshot takeSnapshotInternal(Long volumeId, Long policyId, Long snapsho outcome.get(); } catch (InterruptedException e) { throw new RuntimeException("Operation is interrupted", e); - } catch (java.util.concurrent.ExecutionException e) { + } catch (ExecutionException e) { throw new RuntimeException("Execution excetion", e); } @@ -3972,7 +3968,7 @@ public String extractVolume(ExtractVolumeCmd cmd) { outcome.get(); } catch (InterruptedException e) { throw new RuntimeException("Operation is interrupted", e); - } catch (java.util.concurrent.ExecutionException e) { + } catch (ExecutionException e) { throw new RuntimeException("Execution excetion", e); } From 3dc2dd93b73b56358231659b1440a0ca7cbcd5aa Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Thu, 1 Feb 2024 12:28:40 +0530 Subject: [PATCH 16/23] Code refactored --- .../orchestration/VolumeOrchestrator.java | 17 ++- ...irtCheckAndRepairVolumeCommandWrapper.java | 102 ++++++++++------ .../cloud/storage/VolumeApiServiceImpl.java | 111 ++++++++++-------- 3 files changed, 139 insertions(+), 91 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index 4e157303b71e..cde997be62b1 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1916,12 +1916,7 @@ public void prepare(VirtualMachineProfile vm, DeployDestination dest) throws Sto } } } else { - Host host = _hostDao.findById(vm.getVirtualMachine().getHostId()); - try { - volService.checkAndRepairVolumeBasedOnConfig(volFactory.getVolume(vol.getId()), host); - } catch (Exception e) { - s_logger.debug(String.format("Unable to check and repair volume [%s] on host [%s], due to %s.", volToString, host, e.getMessage())); - } + handleCheckAndRepairVolume(vol, vm.getVirtualMachine().getHostId()); } } else if (task.type == VolumeTaskType.MIGRATE) { pool = (StoragePool)dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary); @@ -1964,6 +1959,16 @@ public void prepare(VirtualMachineProfile vm, DeployDestination dest) throws Sto } } + private void handleCheckAndRepairVolume(Volume vol, Long hostId) { + Host host = _hostDao.findById(hostId); + try { + volService.checkAndRepairVolumeBasedOnConfig(volFactory.getVolume(vol.getId()), host); + } catch (Exception e) { + String volumeToString = getReflectOnlySelectedFields(vol); + s_logger.debug(String.format("Unable to check and repair volume [%s] on host [%s], due to %s.", volumeToString, host, e.getMessage())); + } + } + private boolean stateTransitTo(Volume vol, Volume.Event event) throws NoTransitionException { return _volStateMachine.transitTo(vol, event, null, _volsDao); } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java index a9d9cd948e6b..e31a4a0287ac 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java @@ -30,6 +30,7 @@ import com.cloud.resource.CommandWrapper; import com.cloud.resource.ResourceWrapper; import com.cloud.utils.exception.CloudRuntimeException; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.JsonNode; import org.apache.cloudstack.utils.cryptsetup.KeyFile; @@ -38,6 +39,7 @@ import org.apache.cloudstack.utils.qemu.QemuImgException; import org.apache.cloudstack.utils.qemu.QemuImgFile; import org.apache.cloudstack.utils.qemu.QemuObject; +import org.apache.cloudstack.utils.qemu.QemuObject.EncryptFormat; import org.apache.commons.lang.ArrayUtils; import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; @@ -61,47 +63,23 @@ public Answer execute(CheckAndRepairVolumeCommand command, LibvirtComputingResou final KVMStoragePoolManager storagePoolMgr = serverResource.getStoragePoolMgr(); KVMStoragePool pool = storagePoolMgr.getStoragePool(spool.getType(), spool.getUuid()); - final KVMPhysicalDisk vol = pool.getPhysicalDisk(volumeId); - QemuObject.EncryptFormat encryptFormat = QemuObject.EncryptFormat.enumValue(command.getEncryptFormat()); byte[] passphrase = command.getPassphrase(); + try { - String checkVolumeResult = checkAndRepairVolume(vol, null, encryptFormat, passphrase, serverResource); - s_logger.info(String.format("Check Volume result for the volume %s is %s", vol.getName(), checkVolumeResult)); - CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, true, checkVolumeResult); - answer.setVolumeCheckExecutionResult(checkVolumeResult); - - int leaks = 0; - if (StringUtils.isNotEmpty(checkVolumeResult) && StringUtils.isNotEmpty(repair) && repair.equals("leaks")) { - ObjectMapper objectMapper = new ObjectMapper(); - JsonNode jsonNode = objectMapper.readTree(checkVolumeResult); - JsonNode leaksNode = jsonNode.get("leaks"); - if (leaksNode != null) { - leaks = leaksNode.asInt(); - } - - if (leaks == 0) { - String msg = String.format("no leaks found while checking for the volume %s, so skipping repair", vol.getName()); - s_logger.info(msg); - String jsonStringFormat = String.format("{ \"message\": \"%s\" }", msg); - String finalResult = (checkVolumeResult != null ? checkVolumeResult.concat(",") : "") + jsonStringFormat; - answer = new CheckAndRepairVolumeAnswer(command, true, finalResult); - answer.setVolumeRepairExecutionResult(jsonStringFormat); - answer.setVolumeCheckExecutionResult(checkVolumeResult); - - return answer; - } + CheckAndRepairVolumeAnswer answer = checkVolume(vol, command, serverResource); + String checkVolumeResult = answer.getVolumeCheckExecutionResult(); + + CheckAndRepairVolumeAnswer resultAnswer = checkIfRepairLeaksIsRequired(command, checkVolumeResult, vol.getName()); + // resultAnswer is not null when repair is not required, so return from here + if (resultAnswer != null) { + return resultAnswer; } if (StringUtils.isNotEmpty(repair)) { - String repairVolumeResult = checkAndRepairVolume(vol, repair, encryptFormat, passphrase, serverResource); - String finalResult = (checkVolumeResult != null ? checkVolumeResult.concat(",") : "") + repairVolumeResult; - s_logger.info(String.format("Repair Volume result for the volume %s is %s", vol.getName(), repairVolumeResult)); - - answer = new CheckAndRepairVolumeAnswer(command, true, finalResult); - answer.setVolumeRepairExecutionResult(repairVolumeResult); - answer.setVolumeCheckExecutionResult(checkVolumeResult); + answer = repairVolume(vol, command, serverResource, checkVolumeResult); } + return answer; } catch (Exception e) { return new CheckAndRepairVolumeAnswer(command, false, e.toString()); @@ -112,7 +90,61 @@ public Answer execute(CheckAndRepairVolumeCommand command, LibvirtComputingResou } } - protected String checkAndRepairVolume(final KVMPhysicalDisk vol, final String repair, final QemuObject.EncryptFormat encryptFormat, byte[] passphrase, final LibvirtComputingResource libvirtComputingResource) throws CloudRuntimeException { + private CheckAndRepairVolumeAnswer checkVolume(KVMPhysicalDisk vol, CheckAndRepairVolumeCommand command, LibvirtComputingResource serverResource) { + EncryptFormat encryptFormat = EncryptFormat.enumValue(command.getEncryptFormat()); + byte[] passphrase = command.getPassphrase(); + String checkVolumeResult = checkAndRepairVolume(vol, null, encryptFormat, passphrase, serverResource); + s_logger.info(String.format("Check Volume result for the volume %s is %s", vol.getName(), checkVolumeResult)); + CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, true, checkVolumeResult); + answer.setVolumeCheckExecutionResult(checkVolumeResult); + + return answer; + } + + private CheckAndRepairVolumeAnswer repairVolume(KVMPhysicalDisk vol, CheckAndRepairVolumeCommand command, LibvirtComputingResource serverResource, String checkVolumeResult) { + EncryptFormat encryptFormat = EncryptFormat.enumValue(command.getEncryptFormat()); + byte[] passphrase = command.getPassphrase(); + final String repair = command.getRepair(); + + String repairVolumeResult = checkAndRepairVolume(vol, repair, encryptFormat, passphrase, serverResource); + String finalResult = (checkVolumeResult != null ? checkVolumeResult.concat(",") : "") + repairVolumeResult; + s_logger.info(String.format("Repair Volume result for the volume %s is %s", vol.getName(), repairVolumeResult)); + + CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, true, finalResult); + answer.setVolumeRepairExecutionResult(repairVolumeResult); + answer.setVolumeCheckExecutionResult(checkVolumeResult); + + return answer; + } + + private CheckAndRepairVolumeAnswer checkIfRepairLeaksIsRequired(CheckAndRepairVolumeCommand command, String checkVolumeResult, String volumeName) throws JsonProcessingException { + final String repair = command.getRepair(); + int leaks = 0; + if (StringUtils.isNotEmpty(checkVolumeResult) && StringUtils.isNotEmpty(repair) && repair.equals("leaks")) { + ObjectMapper objectMapper = new ObjectMapper(); + JsonNode jsonNode = objectMapper.readTree(checkVolumeResult); + JsonNode leaksNode = jsonNode.get("leaks"); + if (leaksNode != null) { + leaks = leaksNode.asInt(); + } + + if (leaks == 0) { + String msg = String.format("no leaks found while checking for the volume %s, so skipping repair", volumeName); + s_logger.info(msg); + String jsonStringFormat = String.format("{ \"message\": \"%s\" }", msg); + String finalResult = (checkVolumeResult != null ? checkVolumeResult.concat(",") : "") + jsonStringFormat; + CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, true, finalResult); + answer.setVolumeRepairExecutionResult(jsonStringFormat); + answer.setVolumeCheckExecutionResult(checkVolumeResult); + + return answer; + } + } + + return null; + } + + protected String checkAndRepairVolume(final KVMPhysicalDisk vol, final String repair, final EncryptFormat encryptFormat, byte[] passphrase, final LibvirtComputingResource libvirtComputingResource) throws CloudRuntimeException { List passphraseObjects = new ArrayList<>(); QemuImageOptions imgOptions = null; if (ArrayUtils.isEmpty(passphrase)) { diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 36b0aee98c5b..6a89ac4d4d9d 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1834,54 +1834,60 @@ public Pair checkAndRepairVolume(CheckAndRepairVolumeCmd cmd) th Long vmId = volume.getInstanceId(); if (vmId != null) { // serialize VM operation - AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext(); - if (jobContext.isJobDispatchedBy(VmWorkConstants.VM_WORK_JOB_DISPATCHER)) { - // avoid re-entrance + return handleCheckAndRepairVolumeJob(vmId, volumeId, repair); + } else { + return handleCheckAndRepairVolume(volumeId, repair); + } + } - VmWorkJobVO placeHolder = null; - placeHolder = createPlaceHolderWork(vmId); - try { - Pair result = orchestrateCheckAndRepairVolume(volumeId, repair); - return result; - } finally { - _workJobDao.expunge(placeHolder.getId()); - } - } else { - Outcome outcome = checkAndRepairVolumeThroughJobQueue(vmId, volumeId, repair); + private Pair handleCheckAndRepairVolume(Long volumeId, String repair) { + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); + VolumeInfo volumeInfo = volFactory.getVolume(volumeId); + volumeInfo.addPayload(payload); - try { - outcome.get(); - } catch (InterruptedException e) { - throw new RuntimeException("Operation is interrupted", e); - } catch (ExecutionException e) { - throw new RuntimeException("Execution exception--", e); - } + Pair result = volService.checkAndRepairVolume(volumeInfo); + return result; + } - Object jobResult = _jobMgr.unmarshallResultObject(outcome.getJob()); - if (jobResult != null) { - if (jobResult instanceof ConcurrentOperationException) { - throw (ConcurrentOperationException)jobResult; - } else if (jobResult instanceof ResourceAllocationException) { - throw (ResourceAllocationException)jobResult; - } else if (jobResult instanceof Throwable) { - throw new RuntimeException("Unexpected exception", (Throwable)jobResult); - } - } + private Pair handleCheckAndRepairVolumeJob(Long vmId, Long volumeId, String repair) throws ResourceAllocationException { + AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext(); + if (jobContext.isJobDispatchedBy(VmWorkConstants.VM_WORK_JOB_DISPATCHER)) { + // avoid re-entrance + VmWorkJobVO placeHolder = null; + placeHolder = createPlaceHolderWork(vmId); + try { + Pair result = orchestrateCheckAndRepairVolume(volumeId, repair); + return result; + } finally { + _workJobDao.expunge(placeHolder.getId()); + } + } else { + Outcome outcome = checkAndRepairVolumeThroughJobQueue(vmId, volumeId, repair); + try { + outcome.get(); + } catch (InterruptedException e) { + throw new RuntimeException("Operation is interrupted", e); + } catch (ExecutionException e) { + throw new RuntimeException("Execution exception--", e); + } - // retrieve the entity url from job result - if (jobResult != null && jobResult instanceof Pair) { - return (Pair) jobResult; + Object jobResult = _jobMgr.unmarshallResultObject(outcome.getJob()); + if (jobResult != null) { + if (jobResult instanceof ConcurrentOperationException) { + throw (ConcurrentOperationException)jobResult; + } else if (jobResult instanceof ResourceAllocationException) { + throw (ResourceAllocationException)jobResult; + } else if (jobResult instanceof Throwable) { + throw new RuntimeException("Unexpected exception", (Throwable)jobResult); } + } - return null; + // retrieve the entity url from job result + if (jobResult != null && jobResult instanceof Pair) { + return (Pair) jobResult; } - } else { - CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); - VolumeInfo volumeInfo = volFactory.getVolume(volumeId); - volumeInfo.addPayload(payload); - Pair result = volService.checkAndRepairVolume(volumeInfo); - return result; + return null; } } @@ -1892,16 +1898,7 @@ protected void validationsForCheckVolumeOperation(VolumeVO volume) { Long volumeId = volume.getId(); Long vmId = volume.getInstanceId(); if (vmId != null) { - 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)); - } - - _accountMgr.checkAccess(caller, null, true, vm); - - if (vm.getState() != State.Stopped) { - throw new InvalidParameterValueException(String.format("VM to which the volume %d is attached should be in stopped state", volumeId)); - } + validateVMforCheckVolumeOperation(vmId, volumeId); } if (volume.getState() != Volume.State.Ready) { @@ -1914,6 +1911,20 @@ protected void validationsForCheckVolumeOperation(VolumeVO volume) { } } + private void validateVMforCheckVolumeOperation(Long vmId, Long volumeId) { + Account caller = CallContext.current().getCallingAccount(); + 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)); + } + + _accountMgr.checkAccess(caller, null, true, vm); + + if (vm.getState() != State.Stopped) { + throw new InvalidParameterValueException(String.format("VM to which the volume %d is attached should be in stopped state", volumeId)); + } + } + private Pair orchestrateCheckAndRepairVolume(Long volumeId, String repair) { VolumeInfo volume = volFactory.getVolume(volumeId); From c805940cbe0b6906cf91024d22f65d4d574ea820 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Thu, 1 Feb 2024 13:13:23 +0530 Subject: [PATCH 17/23] used volume name in logs --- .../command/user/volume/CheckAndRepairVolumeCmd.java | 3 ++- .../java/com/cloud/storage/VolumeApiServiceImpl.java | 12 ++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java index 4be4b99c0abc..ce1559c43423 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java @@ -74,8 +74,9 @@ public String getRepair() { if (repairType == null) { throw new InvalidParameterValueException(String.format("Repair parameter can only take the following values: %s" + Arrays.toString(RepairValues.values()))); } + return repair.toLowerCase(); } - return repair.toLowerCase(); + return null; } ///////////////////////////////////////////////////// diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 6a89ac4d4d9d..0daa82a4bf23 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1895,14 +1895,14 @@ protected void validationsForCheckVolumeOperation(VolumeVO volume) { Account caller = CallContext.current().getCallingAccount(); _accountMgr.checkAccess(caller, null, true, volume); - Long volumeId = volume.getId(); + String volumeName = volume.getName(); Long vmId = volume.getInstanceId(); if (vmId != null) { - validateVMforCheckVolumeOperation(vmId, volumeId); + validateVMforCheckVolumeOperation(vmId, volumeName); } if (volume.getState() != Volume.State.Ready) { - throw new InvalidParameterValueException(String.format("VolumeId: %d is not in Ready state", volumeId)); + throw new InvalidParameterValueException(String.format("Volume: %s is not in Ready state", volumeName)); } HypervisorType hypervisorType = _volsDao.getHypervisorType(volume.getId()); @@ -1911,17 +1911,17 @@ protected void validationsForCheckVolumeOperation(VolumeVO volume) { } } - private void validateVMforCheckVolumeOperation(Long vmId, Long volumeId) { + private void validateVMforCheckVolumeOperation(Long vmId, String volumeName) { Account caller = CallContext.current().getCallingAccount(); 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)); + throw new InvalidParameterValueException(String.format("VM not found, please check the VM to which this volume %s is attached", volumeName)); } _accountMgr.checkAccess(caller, null, true, vm); if (vm.getState() != State.Stopped) { - throw new InvalidParameterValueException(String.format("VM to which the volume %d is attached should be in stopped state", volumeId)); + throw new InvalidParameterValueException(String.format("VM to which the volume %s is attached should be in stopped state", volumeName)); } } From cac4418cacb9ed355f7c5290927ae50276d28edb Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Mon, 5 Feb 2024 13:54:26 +0530 Subject: [PATCH 18/23] Changed the API to Async and the setting scope to storage pool --- .../user/volume/CheckAndRepairVolumeCmd.java | 15 +++++++++++++-- .../com/cloud/storage/VolumeApiServiceImpl.java | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java index ce1559c43423..9c0d1a1058a8 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/CheckAndRepairVolumeCmd.java @@ -16,13 +16,14 @@ // under the License. package org.apache.cloudstack.api.command.user.volume; +import com.cloud.event.EventTypes; import com.cloud.exception.InvalidParameterValueException; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiCommandResourceType; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ApiErrorCode; -import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.BaseAsyncCmd; import org.apache.cloudstack.api.Parameter; import org.apache.cloudstack.api.ResponseObject.ResponseView; import org.apache.cloudstack.api.ServerApiException; @@ -41,7 +42,7 @@ @APICommand(name = "checkVolume", description = "Check the volume for any errors or leaks and also repairs when repair parameter is passed, this is currently supported for KVM only", responseObject = VolumeResponse.class, entityType = {Volume.class}, since = "4.19.1", authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) -public class CheckAndRepairVolumeCmd extends BaseCmd { +public class CheckAndRepairVolumeCmd extends BaseAsyncCmd { public static final Logger s_logger = Logger.getLogger(CheckAndRepairVolumeCmd.class.getName()); private static final String s_name = "checkandrepairvolumeresponse"; @@ -98,6 +99,16 @@ public long getEntityOwnerId() { return Account.ACCOUNT_ID_SYSTEM; // no account info given, parent this command to SYSTEM so ERROR events are tracked } + @Override + public String getEventType() { + return EventTypes.EVENT_VOLUME_CHECK; + } + + @Override + public String getEventDescription() { + return String.format("check and repair operation on volume: %s", this._uuidMgr.getUuid(Volume.class, getId())); + } + @Override public Long getApiResourceId() { return id; diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 0daa82a4bf23..c8b39d180f21 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -382,7 +382,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic + " in milliseconds, to execute a storage tag rule; if it is reached, a timeout will happen.", true); public static final ConfigKey AllowCheckAndRepairVolume = new ConfigKey("Advanced", Boolean.class, "volume.check.and.repair.leaks.before.use", "false", - "To check and repair the volume if it has any leaks before performing volume attach or VM start operations", true); + "To check and repair the volume if it has any leaks before performing volume attach or VM start operations", true, ConfigKey.Scope.StoragePool); private final StateMachine2 _volStateMachine; From 0b88d52ea0bae8e2ad0bb465a6ce25097dc80ae2 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 7 Feb 2024 15:05:34 +0530 Subject: [PATCH 19/23] Fixed exit value handling with check volume command --- ...irtCheckAndRepairVolumeCommandWrapper.java | 34 +++-- .../apache/cloudstack/utils/qemu/QemuImg.java | 12 +- .../java/com/cloud/utils/script/Script.java | 132 ++++++++++++++++++ 3 files changed, 161 insertions(+), 17 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java index e31a4a0287ac..ff29bdd99b5d 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java @@ -117,33 +117,45 @@ private CheckAndRepairVolumeAnswer repairVolume(KVMPhysicalDisk vol, CheckAndRep return answer; } - private CheckAndRepairVolumeAnswer checkIfRepairLeaksIsRequired(CheckAndRepairVolumeCommand command, String checkVolumeResult, String volumeName) throws JsonProcessingException { + private CheckAndRepairVolumeAnswer checkIfRepairLeaksIsRequired(CheckAndRepairVolumeCommand command, String checkVolumeResult, String volumeName) { final String repair = command.getRepair(); int leaks = 0; if (StringUtils.isNotEmpty(checkVolumeResult) && StringUtils.isNotEmpty(repair) && repair.equals("leaks")) { ObjectMapper objectMapper = new ObjectMapper(); - JsonNode jsonNode = objectMapper.readTree(checkVolumeResult); + JsonNode jsonNode = null; + try { + jsonNode = objectMapper.readTree(checkVolumeResult); + } catch (JsonProcessingException e) { + String msg = String.format("Error processing response %s during check volume %s", checkVolumeResult, e.getMessage()); + s_logger.info(msg); + + return skipRepairVolumeCommand(command, checkVolumeResult, msg); + } JsonNode leaksNode = jsonNode.get("leaks"); if (leaksNode != null) { leaks = leaksNode.asInt(); } if (leaks == 0) { - String msg = String.format("no leaks found while checking for the volume %s, so skipping repair", volumeName); - s_logger.info(msg); - String jsonStringFormat = String.format("{ \"message\": \"%s\" }", msg); - String finalResult = (checkVolumeResult != null ? checkVolumeResult.concat(",") : "") + jsonStringFormat; - CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, true, finalResult); - answer.setVolumeRepairExecutionResult(jsonStringFormat); - answer.setVolumeCheckExecutionResult(checkVolumeResult); - - return answer; + String msg = String.format("No leaks found while checking for the volume %s, so skipping repair", volumeName); + return skipRepairVolumeCommand(command, checkVolumeResult, msg); } } return null; } + private CheckAndRepairVolumeAnswer skipRepairVolumeCommand(CheckAndRepairVolumeCommand command, String checkVolumeResult, String msg) { + s_logger.info(msg); + String jsonStringFormat = String.format("{ \"message\": \"%s\" }", msg); + String finalResult = (checkVolumeResult != null ? checkVolumeResult.concat(",") : "") + jsonStringFormat; + CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, true, finalResult); + answer.setVolumeRepairExecutionResult(jsonStringFormat); + answer.setVolumeCheckExecutionResult(checkVolumeResult); + + return answer; + } + protected String checkAndRepairVolume(final KVMPhysicalDisk vol, final String repair, final EncryptFormat encryptFormat, byte[] passphrase, final LibvirtComputingResource libvirtComputingResource) throws CloudRuntimeException { List passphraseObjects = new ArrayList<>(); QemuImageOptions imgOptions = null; 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 0eae283821d2..566a8dae52f0 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 @@ -838,19 +838,19 @@ public String checkAndRepair(final QemuImgFile file, final QemuImageOptions imag script.add(imageOptions.toCommandFlag()); } - script.add("--output=json"); - if (StringUtils.isNotEmpty(repair)) { script.add("-r"); script.add(repair); } - OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); - final String result = script.execute(parser); + script.add("--output=json"); + script.add("2>/dev/null"); + + final String result = Script.runBashScriptIgnoreExitValue(script.toString(), 3); if (result != null) { - throw new QemuImgException(result); + logger.debug("Check volume execution result " + result); } - return (parser.getLines() != null) ? parser.getLines().trim() : null; + return result; } } diff --git a/utils/src/main/java/com/cloud/utils/script/Script.java b/utils/src/main/java/com/cloud/utils/script/Script.java index 6af08a981d19..a7b25717b1fa 100644 --- a/utils/src/main/java/com/cloud/utils/script/Script.java +++ b/utils/src/main/java/com/cloud/utils/script/Script.java @@ -330,6 +330,118 @@ public String execute(OutputInterpreter interpreter) { } } + public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValue) { + String[] command = _command.toArray(new String[_command.size()]); + + if (_logger.isDebugEnabled()) { + _logger.debug("Executing: " + buildCommandLine(command).split(KeyStoreUtils.KS_FILENAME)[0]); + } + + try { + ProcessBuilder pb = new ProcessBuilder(command); + pb.redirectErrorStream(true); + if (_workDir != null) + pb.directory(new File(_workDir)); + + _process = pb.start(); + if (_process == null) { + _logger.warn("Unable to execute: " + buildCommandLine(command)); + return "Unable to execute the command: " + command[0]; + } + + BufferedReader ir = new BufferedReader(new InputStreamReader(_process.getInputStream())); + + _thread = Thread.currentThread(); + ScheduledFuture future = null; + if (_timeout > 0) { + future = s_executors.schedule(this, _timeout, TimeUnit.MILLISECONDS); + } + + Task task = null; + if (interpreter != null && interpreter.drain()) { + task = new Task(interpreter, ir); + s_executors.execute(task); + } + + while (true) { + _logger.debug("Executing while with timeout : " + _timeout); + try { + //process execution completed within timeout period + if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) { + //process completed successfully + if (_process.exitValue() == 0 || _process.exitValue() == exitValue) { + _logger.debug("Execution is successful."); + if (interpreter != null) { + return interpreter.drain() ? task.getResult() : interpreter.interpret(ir); + } else { + // null return exitValue apparently + return String.valueOf(_process.exitValue()); + } + } else { //process failed + break; + } + } //timeout + } catch (InterruptedException e) { + if (!_isTimeOut) { + /* + * This is not timeout, we are interrupted by others, + * continue + */ + _logger.debug("We are interrupted but it's not a timeout, just continue"); + continue; + } + } finally { + if (future != null) { + future.cancel(false); + } + Thread.interrupted(); + } + + //timeout without completing the process + TimedOutLogger log = new TimedOutLogger(_process); + Task timedoutTask = new Task(log, ir); + + timedoutTask.run(); + if (!_passwordCommand) { + _logger.warn("Timed out: " + buildCommandLine(command) + ". Output is: " + timedoutTask.getResult()); + } else { + _logger.warn("Timed out: " + buildCommandLine(command)); + } + + return ERR_TIMEOUT; + } + + _logger.debug("Exit value is " + _process.exitValue()); + + BufferedReader reader = new BufferedReader(new InputStreamReader(_process.getInputStream()), 128); + + String error; + if (interpreter != null) { + error = interpreter.processError(reader); + } else { + error = String.valueOf(_process.exitValue()); + } + + if (_logger.isDebugEnabled()) { + _logger.debug(error); + } + return error; + } catch (SecurityException ex) { + _logger.warn("Security Exception....not running as root?", ex); + return stackTraceAsString(ex); + } catch (Exception ex) { + _logger.warn("Exception: " + buildCommandLine(command), ex); + return stackTraceAsString(ex); + } finally { + if (_process != null) { + IOUtils.closeQuietly(_process.getErrorStream()); + IOUtils.closeQuietly(_process.getOutputStream()); + IOUtils.closeQuietly(_process.getInputStream()); + _process.destroyForcibly(); + } + } + } + @Override public String call() { try { @@ -563,4 +675,24 @@ public static int runSimpleBashScriptForExitValue(String command, int timeout, b } } + public static String runBashScriptIgnoreExitValue(String command, int exitValue) { + return runBashScriptIgnoreExitValue(command, exitValue, 0); + } + + public static String runBashScriptIgnoreExitValue(String command, int exitValue, int timeout) { + + Script s = new Script("/bin/bash", timeout); + s.add("-c"); + s.add(command); + + OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); + s.executeIgnoreExitValue(parser, exitValue); + + String result = parser.getLines(); + if (result == null || result.trim().isEmpty()) { + return null; + } else { + return result.trim(); + } + } } From 1596d147870d232513b395b39799e8d1fea4fa99 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Thu, 8 Feb 2024 17:10:38 +0530 Subject: [PATCH 20/23] Fixed storage scope to the setting --- .../apache/cloudstack/storage/volume/VolumeServiceImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 65b8b8feaac4..0c296371bf05 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 @@ -2769,11 +2769,11 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) { @Override public void checkAndRepairVolumeBasedOnConfig(DataObject dataObject, Host host) { if (HypervisorType.KVM.equals(host.getHypervisorType()) && DataObjectType.VOLUME.equals(dataObject.getType())) { - if (VolumeApiServiceImpl.AllowCheckAndRepairVolume.value()) { + VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); + if (VolumeApiServiceImpl.AllowCheckAndRepairVolume.valueIn(volumeInfo.getPoolId())) { s_logger.info(String.format("Trying to check and repair the volume %d", dataObject.getId())); String repair = CheckAndRepairVolumeCmd.RepairValues.LEAKS.name().toLowerCase(); CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); - VolumeInfo volumeInfo = volFactory.getVolume(dataObject.getId()); volumeInfo.addPayload(payload); checkAndRepairVolume(volumeInfo); } From fb1026bdde6660d36876e87ab810a7484c774baa Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 20 Feb 2024 15:42:48 +0530 Subject: [PATCH 21/23] Fix volume format issues --- .../storage/volume/VolumeServiceImpl.java | 22 +++++++++++++--- .../storage/volume/VolumeServiceTest.java | 25 +++++++++++++++++-- ...irtCheckAndRepairVolumeCommandWrapper.java | 15 +++++++++-- ...heckAndRepairVolumeCommandWrapperTest.java | 4 +-- .../cloud/storage/VolumeApiServiceImpl.java | 4 +++ .../storage/VolumeApiServiceImplTest.java | 24 +++++++++++++++++- 6 files changed, 83 insertions(+), 11 deletions(-) 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 0c296371bf05..ed602c0a5bd1 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 @@ -82,6 +82,7 @@ import org.apache.cloudstack.storage.image.store.TemplateObject; import org.apache.cloudstack.storage.to.TemplateObjectTO; import org.apache.cloudstack.storage.to.VolumeObjectTO; +import org.apache.commons.collections.CollectionUtils; import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; @@ -2775,21 +2776,35 @@ public void checkAndRepairVolumeBasedOnConfig(DataObject dataObject, Host host) String repair = CheckAndRepairVolumeCmd.RepairValues.LEAKS.name().toLowerCase(); CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(repair); volumeInfo.addPayload(payload); - checkAndRepairVolume(volumeInfo); + checkAndRepairVolumeThroughHost(volumeInfo, host); } } } @Override public Pair checkAndRepairVolume(VolumeInfo volume) { + Long poolId = volume.getPoolId(); + List hostIds = _storageMgr.getUpHostsInPool(poolId); + if (CollectionUtils.isEmpty(hostIds)) { + throw new CloudRuntimeException("Unable to find Up hosts to run the check volume command"); + } + Collections.shuffle(hostIds); + Host host = _hostDao.findById(hostIds.get(0)); + + return checkAndRepairVolumeThroughHost(volume, host); + + } + + private Pair checkAndRepairVolumeThroughHost(VolumeInfo volume, Host host) { Long poolId = volume.getPoolId(); StoragePool pool = _storageMgr.getStoragePool(poolId); CheckAndRepairVolumePayload payload = (CheckAndRepairVolumePayload) volume.getpayload(); CheckAndRepairVolumeCommand command = new CheckAndRepairVolumeCommand(volume.getPath(), new StorageFilerTO(pool), payload.getRepair(), - volume.getPassphrase(), volume.getEncryptFormat()); + volume.getPassphrase(), volume.getEncryptFormat()); try { - CheckAndRepairVolumeAnswer answer = (CheckAndRepairVolumeAnswer) _storageMgr.sendToPool(pool, null, command); + grantAccess(volume, host, volume.getDataStore()); + CheckAndRepairVolumeAnswer answer = (CheckAndRepairVolumeAnswer) _storageMgr.sendToPool(pool, new long[]{host.getId()}, command); if (answer != null && answer.getResult()) { s_logger.debug("Check volume response result: " + answer.getDetails()); return new Pair<>(answer.getVolumeCheckExecutionResult(), answer.getVolumeRepairExecutionResult()); @@ -2801,6 +2816,7 @@ public Pair checkAndRepairVolume(VolumeInfo volume) { } catch (Exception e) { s_logger.debug("sending check and repair volume command failed", e); } finally { + revokeAccess(volume, host, volume.getDataStore()); command.clearPassphrase(); } diff --git a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java index 240d7f9d12ea..55ff2f659af9 100644 --- a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java +++ b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java @@ -23,6 +23,8 @@ import com.cloud.agent.api.storage.CheckAndRepairVolumeCommand; import com.cloud.agent.api.to.StorageFilerTO; import com.cloud.exception.StorageUnavailableException; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; import com.cloud.storage.CheckAndRepairVolumePayload; import com.cloud.storage.StorageManager; import com.cloud.storage.StoragePool; @@ -80,6 +82,12 @@ public class VolumeServiceTest extends TestCase{ @Mock VolumeVO volumeVoMock; + @Mock + HostVO hostMock; + + @Mock + HostDao hostDaoMock; + @Before public void setup(){ volumeServiceImplSpy = Mockito.spy(new VolumeServiceImpl()); @@ -87,6 +95,7 @@ public void setup(){ volumeServiceImplSpy.volDao = volumeDaoMock; volumeServiceImplSpy.snapshotMgr = snapshotManagerMock; volumeServiceImplSpy._storageMgr = storageManagerMock; + volumeServiceImplSpy._hostDao = hostDaoMock; } @Test(expected = InterruptedException.class) @@ -230,6 +239,12 @@ public void testCheckAndRepairVolume() throws StorageUnavailableException { Mockito.when(volume.getPoolId()).thenReturn(1L); StoragePool pool = Mockito.mock(StoragePool.class); Mockito.when(storageManagerMock.getStoragePool(1L)).thenReturn(pool); + List hostIds = new ArrayList<>(); + hostIds.add(1L); + Mockito.when(storageManagerMock.getUpHostsInPool(1L)).thenReturn(hostIds); + Mockito.when(hostMock.getId()).thenReturn(1L); + Mockito.when(hostDaoMock.findById(1L)).thenReturn(hostMock); + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(null); Mockito.when(volume.getpayload()).thenReturn(payload); Mockito.when(volume.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); @@ -252,7 +267,7 @@ public void testCheckAndRepairVolume() throws StorageUnavailableException { CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, true, checkResult); answer.setVolumeCheckExecutionResult(checkResult); - Mockito.when(storageManagerMock.sendToPool(pool, null, command)).thenReturn(answer); + Mockito.when(storageManagerMock.sendToPool(pool, new long[]{1L}, command)).thenReturn(answer); Pair result = volumeServiceImplSpy.checkAndRepairVolume(volume); @@ -266,6 +281,12 @@ public void testCheckAndRepairVolumeWhenFailure() throws StorageUnavailableExcep Mockito.when(volume.getPoolId()).thenReturn(1L); StoragePool pool = Mockito.mock(StoragePool.class); Mockito.when(storageManagerMock.getStoragePool(1L)).thenReturn(pool); + List hostIds = new ArrayList<>(); + hostIds.add(1L); + Mockito.when(storageManagerMock.getUpHostsInPool(1L)).thenReturn(hostIds); + Mockito.when(hostMock.getId()).thenReturn(1L); + Mockito.when(hostDaoMock.findById(1L)).thenReturn(hostMock); + CheckAndRepairVolumePayload payload = new CheckAndRepairVolumePayload(null); Mockito.when(volume.getpayload()).thenReturn(payload); Mockito.when(volume.getPath()).thenReturn("cbac516a-0f1f-4559-921c-1a7c6c408ccf"); @@ -276,7 +297,7 @@ public void testCheckAndRepairVolumeWhenFailure() throws StorageUnavailableExcep volume.getPassphrase(), volume.getEncryptFormat()); CheckAndRepairVolumeAnswer answer = new CheckAndRepairVolumeAnswer(command, false, "Unable to execute qemu command"); - Mockito.when(storageManagerMock.sendToPool(pool, null, command)).thenReturn(answer); + Mockito.when(storageManagerMock.sendToPool(pool, new long[]{1L}, command)).thenReturn(answer); Pair result = volumeServiceImplSpy.checkAndRepairVolume(volume); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java index ff29bdd99b5d..cd81a2fbc232 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapper.java @@ -67,8 +67,19 @@ public Answer execute(CheckAndRepairVolumeCommand command, LibvirtComputingResou byte[] passphrase = command.getPassphrase(); try { - CheckAndRepairVolumeAnswer answer = checkVolume(vol, command, serverResource); - String checkVolumeResult = answer.getVolumeCheckExecutionResult(); + CheckAndRepairVolumeAnswer answer = null; + String checkVolumeResult = null; + if (QemuImg.PhysicalDiskFormat.RAW.equals(vol.getFormat())) { + checkVolumeResult = "Volume format RAW is not supported to check and repair"; + String jsonStringFormat = String.format("{ \"message\": \"%s\" }", checkVolumeResult); + answer = new CheckAndRepairVolumeAnswer(command, true, checkVolumeResult); + answer.setVolumeCheckExecutionResult(jsonStringFormat); + + return answer; + } else { + answer = checkVolume(vol, command, serverResource); + checkVolumeResult = answer.getVolumeCheckExecutionResult(); + } CheckAndRepairVolumeAnswer resultAnswer = checkIfRepairLeaksIsRequired(command, checkVolumeResult, vol.getName()); // resultAnswer is not null when repair is not required, so return from here diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java index 9eccc7c15b49..e2120e46d130 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckAndRepairVolumeCommandWrapperTest.java @@ -23,7 +23,6 @@ import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; import com.cloud.storage.Storage; -import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo; import org.apache.cloudstack.utils.qemu.QemuImg; import org.junit.Assert; import org.junit.Before; @@ -75,8 +74,7 @@ public void testCheckAndRepairVolume() throws Exception { KVMPhysicalDisk vol = Mockito.mock(KVMPhysicalDisk.class); when(pool.getPhysicalDisk("cbac516a-0f1f-4559-921c-1a7c6c408ccf")).thenReturn(vol); - - VolumeInfo volume = Mockito.mock(VolumeInfo.class); + Mockito.when(vol.getFormat()).thenReturn(QemuImg.PhysicalDiskFormat.QCOW2); String checkResult = "{\n" + " \"image-end-offset\": 6442582016,\n" + diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index c8b39d180f21..2e8b36da446a 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1909,6 +1909,10 @@ protected void validationsForCheckVolumeOperation(VolumeVO volume) { if (!HypervisorType.KVM.equals(hypervisorType)) { throw new InvalidParameterValueException(String.format("Check and Repair volumes is supported only for KVM hypervisor")); } + + if (!Arrays.asList(ImageFormat.QCOW2, ImageFormat.VDI).contains(volume.getFormat())) { + throw new InvalidParameterValueException("Volume format is not supported for checking and repair"); + } } private void validateVMforCheckVolumeOperation(Long vmId, String volumeName) { diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index f0b70f36b5c9..b017a2d3371c 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -1647,7 +1647,6 @@ public void testStoragePoolCompatibilityAndAllowEncryptedVolumeMigrationForPower Mockito.when(_diskOfferingDao.findById(1L)).thenReturn(diskOffering); StoragePoolVO srcStoragePoolVOMock = Mockito.mock(StoragePoolVO.class); - StoragePool destPool = Mockito.mock(StoragePool.class); PrimaryDataStore dataStore = Mockito.mock(PrimaryDataStore.class); Mockito.when(vol.getPassphraseId()).thenReturn(1L); @@ -1680,6 +1679,7 @@ public void testValidationsForCheckVolumeAPI() { when(volume.getState()).thenReturn(Volume.State.Ready); when(volume.getId()).thenReturn(1L); when(volumeDaoMock.getHypervisorType(1L)).thenReturn(HypervisorType.KVM); + when(volume.getFormat()).thenReturn(Storage.ImageFormat.QCOW2); volumeApiServiceImpl.validationsForCheckVolumeOperation(volume); } @@ -1796,9 +1796,31 @@ public void testCheckAndRepairVolume() throws ResourceAllocationException { String repairResult = null; Pair result = new Pair<>(checkResult, repairResult); when(volumeServiceMock.checkAndRepairVolume(volumeInfo)).thenReturn(result); + when(volume.getFormat()).thenReturn(Storage.ImageFormat.QCOW2); Pair finalresult = volumeApiServiceImpl.checkAndRepairVolume(cmd); Assert.assertEquals(result, finalresult); } + + @Test(expected = InvalidParameterValueException.class) + public void testValidationsForCheckVolumeAPIWithInvalidVolumeFormat() { + VolumeVO volume = mock(VolumeVO.class); + AccountVO account = new AccountVO("admin", 1L, "networkDomain", Account.Type.NORMAL, "uuid"); + UserVO user = new UserVO(1, "testuser", "password", "firstname", "lastName", "email", "timezone", UUID.randomUUID().toString(), User.Source.UNKNOWN); + CallContext.register(user, account); + + lenient().doNothing().when(accountManagerMock).checkAccess(any(Account.class), any(AccessType.class), any(Boolean.class), any(ControlledEntity.class)); + + when(volume.getInstanceId()).thenReturn(1L); + UserVmVO vm = mock(UserVmVO.class); + when(userVmDaoMock.findById(1L)).thenReturn(vm); + when(vm.getState()).thenReturn(State.Stopped); + when(volume.getState()).thenReturn(Volume.State.Ready); + when(volume.getId()).thenReturn(1L); + when(volumeDaoMock.getHypervisorType(1L)).thenReturn(HypervisorType.KVM); + when(volume.getFormat()).thenReturn(Storage.ImageFormat.RAW); + + volumeApiServiceImpl.validationsForCheckVolumeOperation(volume); + } } From 2dfeecef692d2d295af1f175e22cd042a318ba62 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 20 Feb 2024 15:47:36 +0530 Subject: [PATCH 22/23] Refactored the log messages --- .../storage/volume/VolumeServiceImpl.java | 4 ++-- .../apache/cloudstack/utils/qemu/QemuImg.java | 2 +- .../main/java/com/cloud/utils/script/Script.java | 16 ++++++++-------- 3 files changed, 11 insertions(+), 11 deletions(-) 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 ed602c0a5bd1..57abebfb738d 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 @@ -2806,11 +2806,11 @@ private Pair checkAndRepairVolumeThroughHost(VolumeInfo volume, grantAccess(volume, host, volume.getDataStore()); CheckAndRepairVolumeAnswer answer = (CheckAndRepairVolumeAnswer) _storageMgr.sendToPool(pool, new long[]{host.getId()}, command); if (answer != null && answer.getResult()) { - s_logger.debug("Check volume response result: " + answer.getDetails()); + s_logger.debug(String.format("Check volume response result: %s", answer.getDetails())); return new Pair<>(answer.getVolumeCheckExecutionResult(), answer.getVolumeRepairExecutionResult()); } else { String errMsg = (answer == null) ? null : answer.getDetails(); - s_logger.debug("Failed to check and repair the volume with error " + errMsg); + s_logger.debug(String.format("Failed to check and repair the volume with error %s", errMsg)); } } catch (Exception e) { 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 566a8dae52f0..360c762deb0b 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 @@ -848,7 +848,7 @@ public String checkAndRepair(final QemuImgFile file, final QemuImageOptions imag final String result = Script.runBashScriptIgnoreExitValue(script.toString(), 3); if (result != null) { - logger.debug("Check volume execution result " + result); + logger.debug(String.format("Check volume execution result %s", result)); } return result; diff --git a/utils/src/main/java/com/cloud/utils/script/Script.java b/utils/src/main/java/com/cloud/utils/script/Script.java index a7b25717b1fa..7e90456655bc 100644 --- a/utils/src/main/java/com/cloud/utils/script/Script.java +++ b/utils/src/main/java/com/cloud/utils/script/Script.java @@ -334,7 +334,7 @@ public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValu String[] command = _command.toArray(new String[_command.size()]); if (_logger.isDebugEnabled()) { - _logger.debug("Executing: " + buildCommandLine(command).split(KeyStoreUtils.KS_FILENAME)[0]); + _logger.debug(String.format("Executing: %s", buildCommandLine(command).split(KeyStoreUtils.KS_FILENAME)[0])); } try { @@ -345,8 +345,8 @@ public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValu _process = pb.start(); if (_process == null) { - _logger.warn("Unable to execute: " + buildCommandLine(command)); - return "Unable to execute the command: " + command[0]; + _logger.warn(String.format("Unable to execute: %s", buildCommandLine(command))); + return String.format("Unable to execute the command: %s", command[0]); } BufferedReader ir = new BufferedReader(new InputStreamReader(_process.getInputStream())); @@ -364,7 +364,7 @@ public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValu } while (true) { - _logger.debug("Executing while with timeout : " + _timeout); + _logger.debug(String.format("Executing while with timeout : %d" + _timeout)); try { //process execution completed within timeout period if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) { @@ -403,15 +403,15 @@ public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValu timedoutTask.run(); if (!_passwordCommand) { - _logger.warn("Timed out: " + buildCommandLine(command) + ". Output is: " + timedoutTask.getResult()); + _logger.warn(String.format("Timed out: %s. Output is: %s", buildCommandLine(command), timedoutTask.getResult())); } else { - _logger.warn("Timed out: " + buildCommandLine(command)); + _logger.warn(String.format("Timed out: %s", buildCommandLine(command))); } return ERR_TIMEOUT; } - _logger.debug("Exit value is " + _process.exitValue()); + _logger.debug(String.format("Exit value is %d", _process.exitValue())); BufferedReader reader = new BufferedReader(new InputStreamReader(_process.getInputStream()), 128); @@ -430,7 +430,7 @@ public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValu _logger.warn("Security Exception....not running as root?", ex); return stackTraceAsString(ex); } catch (Exception ex) { - _logger.warn("Exception: " + buildCommandLine(command), ex); + _logger.warn(String.format("Exception: %s", buildCommandLine(command)), ex); return stackTraceAsString(ex); } finally { if (_process != null) { From 30fa6128b3230ac7518fdfbbe30c51c95aac83d0 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Thu, 22 Feb 2024 11:46:18 +0530 Subject: [PATCH 23/23] Fix formatting --- utils/src/main/java/com/cloud/utils/script/Script.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/src/main/java/com/cloud/utils/script/Script.java b/utils/src/main/java/com/cloud/utils/script/Script.java index 7e90456655bc..cdab31f1865d 100644 --- a/utils/src/main/java/com/cloud/utils/script/Script.java +++ b/utils/src/main/java/com/cloud/utils/script/Script.java @@ -364,7 +364,7 @@ public String executeIgnoreExitValue(OutputInterpreter interpreter, int exitValu } while (true) { - _logger.debug(String.format("Executing while with timeout : %d" + _timeout)); + _logger.debug(String.format("Executing while with timeout : %d", _timeout)); try { //process execution completed within timeout period if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {