[GitHub] DaanHoogland commented on a change in pull request #3154: Copy template to target KVM host if needed when migrating local <> local storage
DaanHoogland commented on a change in pull request #3154: Copy template to
target KVM host if needed when migrating local <> local storage
URL: https://github.com/apache/cloudstack/pull/3154#discussion_r252276888
##
File path:
engine/components-api/src/main/java/com/cloud/storage/StorageManager.java
##
@@ -101,6 +101,9 @@
ConfigKey.Scope.Cluster,
null);
+ConfigKey PRIMARY_STORAGE_DOWNLOAD_WAIT = new
ConfigKey("Storage", Integer.class, "primary.storage.download.wait",
"10800",
Review comment:
I'd say keep your new one and find usages of the old one to unify them. If
need be move the key to a more centralised place.
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] DaanHoogland commented on a change in pull request #3154: Copy template to target KVM host if needed when migrating local <> local storage
DaanHoogland commented on a change in pull request #3154: Copy template to
target KVM host if needed when migrating local <> local storage
URL: https://github.com/apache/cloudstack/pull/3154#discussion_r251756332
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java
##
@@ -140,4 +160,56 @@ protected void setVolumePath(VolumeVO volume) {
protected boolean shouldMigrateVolume(StoragePoolVO sourceStoragePool,
Host destHost, StoragePoolVO destStoragePool) {
return sourceStoragePool.getPoolType() == StoragePoolType.Filesystem;
}
+
+/**
+ * If the template is not on the target primary storage then it migrates
the template.
+ * Otherwise the execution flow fails during migration and creates a null
file created on the target storage pool.
+ */
+@Override
+protected void migrateTemplateToTargetFilesystemStorageIfNeeded(VolumeInfo
srcVolumeInfo, DataStore destDataStore, StoragePool destStoragePool,
+Host destHost) {
+VMTemplateStoragePoolVO vmTemplateStoragePoolVO =
vmTemplatePoolDao.findByPoolTemplate(destStoragePool.getId(),
srcVolumeInfo.getTemplateId());
+if (vmTemplateStoragePoolVO == null && destStoragePool.getPoolType()
== StoragePoolType.Filesystem) {
+DataStore sourceTemplateDataStore =
dataStoreManagerImpl.getImageStore(srcVolumeInfo.getDataCenterId());
+TemplateInfo sourceTemplateInfo =
templateDataFactory.getTemplate(srcVolumeInfo.getTemplateId(),
sourceTemplateDataStore);
+TemplateObjectTO sourceTemplate = new
TemplateObjectTO(sourceTemplateInfo);
+
+LOGGER.debug(String.format("Could not find template [id=%s,
name=%s] on the storage pool [id=%s]; copying the template to the target
storage pool.",
+srcVolumeInfo.getTemplateId(),
sourceTemplateInfo.getName(), destDataStore.getId()));
+
+TemplateInfo destTemplateInfo =
templateDataFactory.getTemplate(srcVolumeInfo.getTemplateId(), destDataStore);
+final TemplateObjectTO destTemplate = new
TemplateObjectTO(destTemplateInfo);
+
+sendCopyCommand(destHost, sourceTemplate, destTemplate,
destDataStore);
+}
+}
+
+/**
+ * Sends the CopyCommand to migrate the template to the dest host.
+ */
+protected void sendCopyCommand(Host destHost, TemplateObjectTO
sourceTemplate, TemplateObjectTO destTemplate, DataStore destDataStore) {
+boolean executeInSequence =
virtualMachineManager.getExecuteInSequence(HypervisorType.KVM);
+CopyCommand copyCommand = new CopyCommand(sourceTemplate,
destTemplate, StorageManager.PRIMARY_STORAGE_DOWNLOAD_WAIT.value(),
executeInSequence);
+try {
+Answer copyCommandAnswer = agentManager.send(destHost.getId(),
copyCommand);
+logInCaseOfTemplateCopyFailure(copyCommandAnswer, sourceTemplate,
destDataStore);
+} catch (AgentUnavailableException | OperationTimedoutException e) {
+throw new CloudRuntimeException(String.format("Failed to copy
template [id=%s, name=%s] to the primary storage pool [id=%s].",
sourceTemplate.getId(),
Review comment:
why is this string constructed twice, here and in below
logInCaseOfTemplateCopyFailure()?
they seem to describe the exact same thing
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] DaanHoogland commented on a change in pull request #3154: Copy template to target KVM host if needed when migrating local <> local storage
DaanHoogland commented on a change in pull request #3154: Copy template to
target KVM host if needed when migrating local <> local storage
URL: https://github.com/apache/cloudstack/pull/3154#discussion_r251754196
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java
##
@@ -140,4 +160,56 @@ protected void setVolumePath(VolumeVO volume) {
protected boolean shouldMigrateVolume(StoragePoolVO sourceStoragePool,
Host destHost, StoragePoolVO destStoragePool) {
return sourceStoragePool.getPoolType() == StoragePoolType.Filesystem;
}
+
+/**
+ * If the template is not on the target primary storage then it migrates
the template.
+ * Otherwise the execution flow fails during migration and creates a null
file created on the target storage pool.
Review comment:
this comments confuses me. you wrote in the description that the template
would be copied and this problem would no longer be there. if that is true this
"Otherwise ..." is not true or badly formulated. Can you explain?
As it is written now it seems that the condition after "Otherwise" can still
occur.
If I understand this correctly you should start with "If we don't do this
...", or "This is needed to prevent ...". Am I correct?
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] DaanHoogland commented on a change in pull request #3154: Copy template to target KVM host if needed when migrating local <> local storage
DaanHoogland commented on a change in pull request #3154: Copy template to
target KVM host if needed when migrating local <> local storage
URL: https://github.com/apache/cloudstack/pull/3154#discussion_r251758952
##
File path:
engine/components-api/src/main/java/com/cloud/storage/StorageManager.java
##
@@ -101,6 +101,9 @@
ConfigKey.Scope.Cluster,
null);
+ConfigKey PRIMARY_STORAGE_DOWNLOAD_WAIT = new
ConfigKey("Storage", Integer.class, "primary.storage.download.wait",
"10800",
Review comment:
isn't such a variable already in place somewhere? there are other places
where it would have use. If not, I think it should be applied to the initial
deploy equally.
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] DaanHoogland commented on a change in pull request #3154: Copy template to target KVM host if needed when migrating local <> local storage
DaanHoogland commented on a change in pull request #3154: Copy template to
target KVM host if needed when migrating local <> local storage
URL: https://github.com/apache/cloudstack/pull/3154#discussion_r251756702
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -1859,14 +1861,21 @@ protected void setVolumePath(VolumeVO volume) {
volume.setPath(volume.get_iScsiName());
}
+/**
+ * For this strategy it is not necessary to check and migrate the
template; however, classes that extend this one may need to check if the
template is on the target storage pool (and if not then migrate) before
migrating the VM.
Review comment:
can you try to wrap this to fit in a github window?
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] DaanHoogland commented on a change in pull request #3154: Copy template to target KVM host if needed when migrating local <> local storage
DaanHoogland commented on a change in pull request #3154: Copy template to
target KVM host if needed when migrating local <> local storage
URL: https://github.com/apache/cloudstack/pull/3154#discussion_r251755714
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java
##
@@ -140,4 +160,56 @@ protected void setVolumePath(VolumeVO volume) {
protected boolean shouldMigrateVolume(StoragePoolVO sourceStoragePool,
Host destHost, StoragePoolVO destStoragePool) {
return sourceStoragePool.getPoolType() == StoragePoolType.Filesystem;
}
+
+/**
+ * If the template is not on the target primary storage then it migrates
the template.
+ * Otherwise the execution flow fails during migration and creates a null
file created on the target storage pool.
+ */
+@Override
+protected void migrateTemplateToTargetFilesystemStorageIfNeeded(VolumeInfo
srcVolumeInfo, DataStore destDataStore, StoragePool destStoragePool,
+Host destHost) {
+VMTemplateStoragePoolVO vmTemplateStoragePoolVO =
vmTemplatePoolDao.findByPoolTemplate(destStoragePool.getId(),
srcVolumeInfo.getTemplateId());
+if (vmTemplateStoragePoolVO == null && destStoragePool.getPoolType()
== StoragePoolType.Filesystem) {
+DataStore sourceTemplateDataStore =
dataStoreManagerImpl.getImageStore(srcVolumeInfo.getDataCenterId());
+TemplateInfo sourceTemplateInfo =
templateDataFactory.getTemplate(srcVolumeInfo.getTemplateId(),
sourceTemplateDataStore);
+TemplateObjectTO sourceTemplate = new
TemplateObjectTO(sourceTemplateInfo);
+
+LOGGER.debug(String.format("Could not find template [id=%s,
name=%s] on the storage pool [id=%s]; copying the template to the target
storage pool.",
+srcVolumeInfo.getTemplateId(),
sourceTemplateInfo.getName(), destDataStore.getId()));
+
+TemplateInfo destTemplateInfo =
templateDataFactory.getTemplate(srcVolumeInfo.getTemplateId(), destDataStore);
+final TemplateObjectTO destTemplate = new
TemplateObjectTO(destTemplateInfo);
+
+sendCopyCommand(destHost, sourceTemplate, destTemplate,
destDataStore);
+}
+}
+
+/**
+ * Sends the CopyCommand to migrate the template to the dest host.
+ */
+protected void sendCopyCommand(Host destHost, TemplateObjectTO
sourceTemplate, TemplateObjectTO destTemplate, DataStore destDataStore) {
+boolean executeInSequence =
virtualMachineManager.getExecuteInSequence(HypervisorType.KVM);
+CopyCommand copyCommand = new CopyCommand(sourceTemplate,
destTemplate, StorageManager.PRIMARY_STORAGE_DOWNLOAD_WAIT.value(),
executeInSequence);
+try {
+Answer copyCommandAnswer = agentManager.send(destHost.getId(),
copyCommand);
+logInCaseOfTemplateCopyFailure(copyCommandAnswer, sourceTemplate,
destDataStore);
+} catch (AgentUnavailableException | OperationTimedoutException e) {
+throw new CloudRuntimeException(String.format("Failed to copy
template [id=%s, name=%s] to the primary storage pool [id=%s].",
sourceTemplate.getId(),
+sourceTemplate.getName(), destDataStore.getId()), e);
+}
+}
+
+/**
+ * Logs in debug mode the copy command failure if the CopyCommand Answer
has result as false.
+ */
+protected void logInCaseOfTemplateCopyFailure(Answer copyCommandAnswer,
TemplateObjectTO sourceTemplate, DataStore destDataStore) {
+if (copyCommandAnswer != null && !copyCommandAnswer.getResult()) {
+String failureDetails = StringUtils.EMPTY;
+if (copyCommandAnswer.getDetails() != null) {
+failureDetails = "; details: " +
copyCommandAnswer.getDetails();
+}
+LOGGER.debug(String.format("Failed to copy template [id=%s,
name=%s] to the primary storage pool [id=%s]%s", sourceTemplate.getId(),
sourceTemplate.getName(),
Review comment:
seems like an 'ERROR' to me
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] DaanHoogland commented on a change in pull request #3154: Copy template to target KVM host if needed when migrating local <> local storage
DaanHoogland commented on a change in pull request #3154: Copy template to
target KVM host if needed when migrating local <> local storage
URL: https://github.com/apache/cloudstack/pull/3154#discussion_r251757894
##
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##
@@ -1859,14 +1861,21 @@ protected void setVolumePath(VolumeVO volume) {
volume.setPath(volume.get_iScsiName());
}
+/**
+ * For this strategy it is not necessary to check and migrate the
template; however, classes that extend this one may need to check if the
template is on the target storage pool (and if not then migrate) before
migrating the VM.
+ */
+protected void migrateTemplateToTargetFilesystemStorageIfNeeded(VolumeInfo
srcVolumeInfo, DataStore destDataStore, StoragePool destStoragePool, Host
destHost) {
+// This method is used by classes that extend this one
Review comment:
I am wary of this pattern. Can you explain why you implemented it this way?
It seems you are implementing a pattern for genericity while already knowing it
is only applicable for localstorage.
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
