[GitHub] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252558821
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -2046,6 +2207,51 @@ 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 (srcStoragePoolVO.getPoolType() !=
Storage.StoragePoolType.NetworkFilesystem ||
+destStoragePoolVO.getPoolType() !=
Storage.StoragePoolType.NetworkFilesystem) {
Review comment:
@GabrielBrascher quick question does #2997 also work with live VM + local
data disk migration?
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] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252558595
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -2046,6 +2207,51 @@ 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 (srcStoragePoolVO.getPoolType() !=
Storage.StoragePoolType.NetworkFilesystem ||
+destStoragePoolVO.getPoolType() !=
Storage.StoragePoolType.NetworkFilesystem) {
Review comment:
Fixed.
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] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252213990
##
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:
There is no *behaviour* in this class, the purpose of the class is to hold
data/config (or state as you will).
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] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252203448
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -2046,6 +2208,51 @@ 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 (srcStoragePoolVO.getPoolType() !=
Storage.StoragePoolType.NetworkFilesystem ||
+destStoragePoolVO.getPoolType() !=
Storage.StoragePoolType.NetworkFilesystem) {
+throw new CloudRuntimeException("Currently only NFS source
and destination storage pools are supported " +
+"when destination is not managed on KVM live
storage migrations");
+}
+if (destStoragePoolVO.getScope() != ScopeType.CLUSTER) {
+throw new CloudRuntimeException("KVM live storage
migrations currently support cluster-wide " +
+"not managed destination storage");
+}
+if (!sourcePools.containsKey(srcStoragePoolVO.getUuid())) {
+sourcePools.put(srcStoragePoolVO.getUuid(),
srcStoragePoolVO.getPoolType());
+}
+}
+}
+verifyDestinationStorage(sourcePools, destHost);
+}
+
+/**
+ * Perform storage validation on destination host for KVM live storage
migrations.
+ * Validate that volume source storage pools are mounted on the
destination host prior the migration
+ * @throws CloudRuntimeException if any source storage pool is not mounted
on the destination host
+ */
+private void verifyDestinationStorage(Map
sourcePools, Host destHost) {
+if (MapUtils.isNotEmpty(sourcePools)) {
+LOGGER.debug("Verifying NFS source pools are already mounted on
destination host " + destHost.getUuid());
Review comment:
Fixed.
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] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252202366
##
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:
Well the idea of the class is to capture migration options, I don't see why
two constructors cannot be used.
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] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252200696
##
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)
{
Review comment:
Well depends how you'd want to look at it. It's not unused if you're using
the collection.
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] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252167338
##
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:
I think the `KvmNonManagedStorageDataMotionStrategy` and
`StorageSystemDataMotionStrategy` are both instantiated and consumed the same
way, we'll have to check if by simply moving the changes will not break the
API. I'll discuss with @nvazquez and get back on this, we can submit a
refactoring PR in future.
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] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252168256
##
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) {
Review comment:
The consumer of the method will have to do the Map null check then.
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] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252168568
##
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)
{
Review comment:
Same as before, the consumer will have to check the map for nullity, unbox
the map and pass the list.
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] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252168305
##
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:
Yes.
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] rhtyd commented on a change in pull request #2983: KVM live storage migration intra cluster from NFS source and destination
rhtyd 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_r252167424
##
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:
Yes.
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
