DaanHoogland commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r461600605
##
File path:
services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java
##
@@ -307,12 +307,15 @@ public Answer
handleDeleteEntityDownloadURLCommand(DeleteEntityDownloadURLComman
//We just need to remove the UUID.vhd
String extractUrl = cmd.getExtractUrl();
-command.add("unlink /var/www/html/userdata/" +
extractUrl.substring(extractUrl.lastIndexOf(File.separator) + 1));
-String result = command.execute();
-if (result != null) {
-// FIXME - Ideally should bail out if you cant delete symlink. Not
doing it right now.
-// This is because the ssvm might already be destroyed and the
symlinks do not exist.
-s_logger.warn("Error in deleting symlink :" + result);
+String result;
+if (extractUrl != null) {
+command.add("unlink /var/www/html/userdata/" +
extractUrl.substring(extractUrl.lastIndexOf(File.separator) + 1));
+result = command.execute();
+if (result != null) {
+// FIXME - Ideally should bail out if you cant delete symlink.
Not doing it right now.
+// This is because the ssvm might already be destroyed and the
symlinks do not exist.
+s_logger.warn("Error in deleting symlink :" + result);
Review comment:
hm, that would be an easy check, wonder if that is all.
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -2698,9 +2701,40 @@ public String extractVolume(ExtractVolumeCmd cmd) {
}
// Check if the url already exists
-VolumeDataStoreVO volumeStoreRef =
_volumeStoreDao.findByVolume(volumeId);
+SearchCriteria sc =
_volumeStoreDao.createSearchCriteria();
+sc.addAnd("state", SearchCriteria.Op.EQ,
ObjectInDataStoreStateMachine.State.Ready.toString());
+sc.addAnd("volumeId", SearchCriteria.Op.EQ, volumeId);
+sc.addAnd("destroyed", SearchCriteria.Op.EQ, false);
+// the volume should not change (attached/detached, vm not updated)
after created
+if (volume.getVolumeType() == Volume.Type.ROOT) { // for ROOT disk
+VMInstanceVO vm = _vmInstanceDao.findById(volume.getInstanceId());
+sc.addAnd("updated", SearchCriteria.Op.GTEQ, vm.getUpdateTime());
+} else if (volume.getVolumeType() == Volume.Type.DATADISK &&
volume.getInstanceId() == null) { // for not attached DATADISK
+sc.addAnd("updated", SearchCriteria.Op.GTEQ, volume.getUpdated());
+} else { // for attached DATA DISK
+VMInstanceVO vm = _vmInstanceDao.findById(volume.getInstanceId());
+sc.addAnd("updated", SearchCriteria.Op.GTEQ, vm.getUpdateTime());
+sc.addAnd("updated", SearchCriteria.Op.GTEQ, volume.getUpdated());
+}
+Filter filter = new Filter(VolumeDataStoreVO.class, "created", false,
0L, 1L);
+List volumeStoreRefs = _volumeStoreDao.search(sc,
filter);
+VolumeDataStoreVO volumeStoreRef = null;
+if (volumeStoreRefs != null && !volumeStoreRefs.isEmpty()) {
+volumeStoreRef = volumeStoreRefs.get(0);
+}
if (volumeStoreRef != null && volumeStoreRef.getExtractUrl() != null) {
return volumeStoreRef.getExtractUrl();
+} else if (volumeStoreRef != null) {
+s_logger.debug("volume " + volumeId + " is already installed on
secondary storage, install path is " +
+volumeStoreRef.getInstallPath());
+ImageStoreEntity secStore = (ImageStoreEntity)
dataStoreMgr.getDataStore(volumeStoreRef.getDataStoreId(), DataStoreRole.Image);
+String extractUrl =
secStore.createEntityExtractUrl(volumeStoreRef.getInstallPath(),
volume.getFormat(), null);
+volumeStoreRef = _volumeStoreDao.findByVolume(volumeId);
+volumeStoreRef.setExtractUrl(extractUrl);
+volumeStoreRef.setExtractUrlCreated(DateUtil.now());
+_volumeStoreDao.update(volumeStoreRef.getId(), volumeStoreRef);
+return extractUrl;
Review comment:
can you extract these two blocks in methods with good descriptive names
please?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
