[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r252793995
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -2046,6 +2207,46 @@ private void
verifyLiveMigrationMapForKVM(Map volumeDataS
if (destStoragePoolVO == null) {
throw new CloudRuntimeException("Destination storage pool with
ID " + dataStore.getId() + " was not located.");
}
+
+if (storageTypeConsistency == null) {
+storageTypeConsistency = destStoragePoolVO.isManaged();
+} else if (storageTypeConsistency !=
destStoragePoolVO.isManaged()) {
+throw new CloudRuntimeException("Destination storage pools
must be either all managed or all not managed");
+}
+
+if (!destStoragePoolVO.isManaged()) {
+if (destStoragePoolVO.getScope() != ScopeType.CLUSTER) {
Review comment:
Thanks for pointing this out @GabrielBrascher, added NFS check to fix the bug
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r252791057
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -2022,10 +2153,40 @@ private ModifyTargetsCommand
getModifyTargetsCommand(long storagePoolId, String
return modifyTargetsAnswer.getConnectedPaths();
}
+/**
+ * Update reference on template_spool_ref table of copied template to
destination storage
+ */
+protected void updateCopiedTemplateReference(VolumeInfo srcVolumeInfo,
VolumeInfo destVolumeInfo) {
+VMTemplateStoragePoolVO ref =
templatePoolDao.findByPoolTemplate(srcVolumeInfo.getPoolId(),
srcVolumeInfo.getTemplateId());
+VMTemplateStoragePoolVO newRef = new
VMTemplateStoragePoolVO(destVolumeInfo.getPoolId(), ref.getTemplateId());
+newRef.setDownloadPercent(100);
+
newRef.setDownloadState(VMTemplateStorageResourceAssoc.Status.DOWNLOADED);
+newRef.setState(ObjectInDataStoreStateMachine.State.Ready);
+newRef.setTemplateSize(ref.getTemplateSize());
+newRef.setLocalDownloadPath(ref.getLocalDownloadPath());
+newRef.setInstallPath(ref.getInstallPath());
+templatePoolDao.persist(newRef);
+}
+
+/**
+ * Handle post destination volume creation actions depending on the
migrating volume type: full clone or linked clone
+ */
+protected void postVolumeCreationActions(VolumeInfo srcVolumeInfo,
VolumeInfo destVolumeInfo, VirtualMachineTO vmTO, Host srcHost) {
+MigrationOptions migrationOptions =
destVolumeInfo.getMigrationOptions();
+if (migrationOptions != null) {
+if (migrationOptions.getType() ==
MigrationOptions.Type.LinkedClone && migrationOptions.isCopySrcTemplate()) {
+updateCopiedTemplateReference(srcVolumeInfo, destVolumeInfo);
+}
+}
+}
+
/*
-* At a high level: The source storage cannot be managed and the
destination storage must be managed.
+* At a high level: The source storage cannot be managed and
+* the destination storages can be all managed or all not
managed, not mixed.
*/
-private void verifyLiveMigrationMapForKVM(Map
volumeDataStoreMap) {
+protected void verifyLiveMigrationForKVM(Map
volumeDataStoreMap, Host destHost) {
Review comment:
It could be. I think it does not harm to keep a detailed verification and
get a proper error in case the requirements are not met. In the case of
`canHandle` it would be harder to track as in case the requirements are not met
then the strategy will not handle the migration and would need to log the
issue. Both ways are valid but I decided the first one. What do you think?
@GabrielBrascher @rhtyd
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r252785305
##
File path: api/src/main/java/com/cloud/storage/MigrationOptions.java
##
@@ -0,0 +1,86 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+
+public class MigrationOptions implements Serializable {
+
+private String srcPoolUuid;
+private Storage.StoragePoolType srcPoolType;
+private Type type;
+private String srcBackingFilePath;
+private boolean copySrcTemplate;
+private String srcVolumeUuid;
+private int timeout;
+
Review comment:
Yes those are different configs for each type. When consuming the data it
provides, not all fields are relevant, as that would depend on the type (Linked
clone or Full clone)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r252783764
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -253,18 +262,64 @@ private boolean canHandle(DataObject dataObject) {
return false;
}
+/**
+ * True if volumes source storage are NFS
+ */
+protected boolean isSourceNfsPrimaryStorage(Map
volumeMap) {
+if (MapUtils.isNotEmpty(volumeMap)) {
+for (VolumeInfo volumeInfo : volumeMap.keySet()) {
+StoragePoolVO storagePoolVO =
_storagePoolDao.findById(volumeInfo.getPoolId());
+return storagePoolVO != null &&
+storagePoolVO.getPoolType() ==
Storage.StoragePoolType.NetworkFilesystem;
+}
+}
+return false;
+}
+
+/**
+ * True if destination storage is cluster-wide NFS
Review comment:
Same as above
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r252782932
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -253,18 +262,64 @@ private boolean canHandle(DataObject dataObject) {
return false;
}
+/**
+ * True if volumes source storage are NFS
+ */
Review comment:
@sureshanaparti yes, that was the intent as it was planned to be for live
storage migrations from host H1 to host H2 and storage A to storage B, i.e.
multiple volumes on the same storage so no need to check all of them. But it
can be extended to check all the volumes in the map
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r252782932
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -253,18 +262,64 @@ private boolean canHandle(DataObject dataObject) {
return false;
}
+/**
+ * True if volumes source storage are NFS
+ */
Review comment:
@sureshanaparti yes, that was the intent as it was planned to be for live
storage migrations from host H1 to host H2 and storage A to storage B, i.e.
multiple volumes on the same storage so no need to check all of them. But it
can be extended to check all the volumes in the map (supporting multiple
volumes on multiple storage pools)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r252780507
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -253,18 +262,64 @@ private boolean canHandle(DataObject dataObject) {
return false;
}
+/**
+ * True if volumes source storage are NFS
+ */
+protected boolean isSourceNfsPrimaryStorage(Map
volumeMap) {
+if (MapUtils.isNotEmpty(volumeMap)) {
+for (VolumeInfo volumeInfo : volumeMap.keySet()) {
+StoragePoolVO storagePoolVO =
_storagePoolDao.findById(volumeInfo.getPoolId());
+return storagePoolVO != null &&
+storagePoolVO.getPoolType() ==
Storage.StoragePoolType.NetworkFilesystem;
+}
+}
+return false;
+}
+
+/**
+ * True if destination storage is cluster-wide NFS
+ */
+protected boolean
isDestinationNfsPrimaryStorageClusterWide(Map volumeMap)
{
+if (MapUtils.isNotEmpty(volumeMap)) {
+for (DataStore dataStore : volumeMap.values()) {
+StoragePoolVO storagePoolVO =
_storagePoolDao.findById(dataStore.getId());
+return storagePoolVO != null &&
+storagePoolVO.getPoolType() ==
Storage.StoragePoolType.NetworkFilesystem &&
+storagePoolVO.getScope() == ScopeType.CLUSTER;
+}
+}
+return false;
+}
+
+/**
+ * Allow KVM live storage migration for non managed storage when:
+ * - Source host and destination host are different, and are on the same
cluster
+ * - Source and destination storage are NFS
+ * - Destination storage is cluster-wide
+ */
+protected StrategyPriority
canHandleKVMNonManagedLiveStorageMigration(Map volumeMap,
Review comment:
Agree with moving it to `KvmNonManagedStorageDataMotionStrategy` class
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination URL: https://github.com/apache/cloudstack/pull/2983#discussion_r230736042 ## File path: plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java ## @@ -30,8 +31,13 @@ /* The qemu-img binary. We expect this to be in $PATH */ public String _qemuImgPath = "qemu-img"; +private String cloudQemuImgPath = "cloud-qemu-img"; Review comment: Just to make sure it works with snapshot support. If the binary is present on the KVM host it will be used, and if not just the qemu-img. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r230734799
##
File path:
core/src/main/java/com/cloud/agent/api/storage/CheckStorageAvailabilityCommand.java
##
@@ -0,0 +1,43 @@
+//
+// 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.storage.Storage;
+
+import java.util.Map;
+
+public class CheckStorageAvailabilityCommand extends Command {
+
+private Map poolsMap;
+
+public CheckStorageAvailabilityCommand(Map poolsMap) {
+this.poolsMap = poolsMap;
+}
+
+public Map getPoolsMap() {
+return poolsMap;
+}
+
+@Override
+public boolean executeInSequence() {
+return false;
+}
+}
Review comment:
Done, thanks
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r230733469
##
File path:
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapperTest.java
##
@@ -325,4 +442,27 @@ public void testMigrationUriException() {
LibvirtMigrateCommandWrapper lw = new LibvirtMigrateCommandWrapper();
lw.createMigrationURI(null, new LibvirtComputingResource());
}
+
+@Test
+public void testReplaceStorageXmlDiskNotManagedStorage() throws
ParserConfigurationException, TransformerException, SAXException, IOException {
+final LibvirtMigrateCommandWrapper lw = new
LibvirtMigrateCommandWrapper();
+String destDisk1FileName = "XX";
+String destDisk2FileName = "YY";
+String destDisk1Path = String.format("/mnt/%s/%s", destPoolUuid,
destDisk1FileName);
+MigrateCommand.MigrateDiskInfo migrateDisk1Info = new
MigrateCommand.MigrateDiskInfo(disk1SourceFilename,
+MigrateCommand.MigrateDiskInfo.DiskType.FILE,
MigrateCommand.MigrateDiskInfo.DriverType.QCOW2,
+MigrateCommand.MigrateDiskInfo.Source.FILE, destDisk1Path);
+String destDisk2Path = String.format("/mnt/%s/%s", destPoolUuid,
destDisk2FileName);
+MigrateCommand.MigrateDiskInfo migrateDisk2Info = new
MigrateCommand.MigrateDiskInfo(disk2SourceFilename,
+MigrateCommand.MigrateDiskInfo.DiskType.FILE,
MigrateCommand.MigrateDiskInfo.DriverType.QCOW2,
+MigrateCommand.MigrateDiskInfo.Source.FILE, destDisk2Path);
+Map migrateStorage = new
HashMap<>();
+migrateStorage.put(disk1SourceFilename, migrateDisk1Info);
+migrateStorage.put(disk2SourceFilename, migrateDisk2Info);
+String newXml = lw.replaceStorage(sourceMultidiskDomainXml,
migrateStorage, false);
+assertTrue(newXml.contains(destDisk1Path));
Review comment:
It is actually checking the full path as it is being defined as: `String
destDisk1Path = String.format("/mnt/%s/%s", destPoolUuid, destDisk1FileName)`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r229928111
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -1991,8 +2198,49 @@ private void
verifyLiveMigrationMapForKVM(Map volumeDataS
throw new CloudRuntimeException("Destination storage pool with
ID " + dataStore.getId() + " was not located.");
}
+if (storageTypeConsistency == null) {
Review comment:
Actually just for the first item in the map entry set, as the variable is
defined out of the `for` block. The idea is to set it on the first iteration
and compare it on the next ones.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage
migration intra cluster from NFS source and destination
URL: https://github.com/apache/cloudstack/pull/2983#discussion_r229306734
##
File path:
plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java
##
@@ -30,8 +31,13 @@
/* The qemu-img binary. We expect this to be in $PATH */
public String _qemuImgPath = "qemu-img";
+private String cloudQemuImgPath = "cloud-qemu-img";
private int timeout;
+private String getQemuImgPathScript = String.format("which %s >&
/dev/null; " +
+"if [ $? -gt 0 ]; then echo \"%s\"; else echo \"%s\"; fi",
+cloudQemuImgPath, _qemuImgPath, cloudQemuImgPath);
Review comment:
As this feature requires qemu-img command supporting snapshots I have
introduced this check. This cloud-qemu-img can be included on KVM hosts to
support snapshots as per:
http://www.nux.ro/oldblog/archive/2014/01/Taking_KVM_volume_snapshots_with_Cloudstack_4_2_on_CentOS_6_5.html
If cloud-qemu-img is present then we can use it, and if not just qemu-img.
It is also done this way on scripts:
https://github.com/apache/cloudstack/search?q=cloud-qemu-img_q=cloud-qemu-img.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
nvazquez commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination URL: https://github.com/apache/cloudstack/pull/2983#discussion_r229305185 ## File path: engine/schema/src/main/resources/META-INF/db/schema-41120to41200.sql ## @@ -37,4 +37,4 @@ INSERT INTO `cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`, UPDATE `cloud`.`async_job` SET `removed` = now() WHERE `removed` IS NULL; -- PR#1448 update description of 'execute.in.sequence.network.element.commands' parameter to reflect an unused command that has been removed. The removed class command is 'UserDataCommand'. -update `cloud`.`configuration` set description = 'If set to true, DhcpEntryCommand, SavePasswordCommand, VmDataCommand will be synchronized on the agent side. If set to false, these commands become asynchronous. Default value is false.' where name = 'execute.in.sequence.network.element.commands'; \ No newline at end of file +update `cloud`.`configuration` set description = 'If set to true, DhcpEntryCommand, SavePasswordCommand, VmDataCommand will be synchronized on the agent side. If set to false, these commands become asynchronous. Default value is false.' where name = 'execute.in.sequence.network.element.commands'; Review comment: Had previously included an update sql on the `hypervisor_cappabilities` to enable live storage motion on KVM. But due to conflicting versions on qemu just decided to remove it and must have removed an end of line or extra space, will sort it out This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
