FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

CLOUDSTACK-8415. SSVM shutdown during snapshot operation leaves behin… by likitha · Pull Request #540 · apache/cloudstack · GitHub

  1. apache
  2. cloudstack

Repository navigation

CLOUDSTACK-8415. SSVM shutdown during snapshot operation leaves behin… #540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
likitha wants to merge 1 commit into from
Conversation 18 Commits 1 Checks 0 Files changed
Closed

CLOUDSTACK-8415. SSVM shutdown during snapshot operation leaves behin… #540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension .java  (6) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
2 changes: 2 additions & 0 deletions engine/schema/src/com/cloud/storage/dao/SnapshotDao.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ public interface SnapshotDao extends GenericDao<SnapshotVO, Long>, StateDao<Snap

List<SnapshotVO> listAllByStatus(Snapshot.State... status);

List<SnapshotVO> listAllByStatusIncludingRemoved(Snapshot.State... status);

void updateVolumeIds(long oldVolId, long newVolId);

}
7 changes: 7 additions & 0 deletions engine/schema/src/com/cloud/storage/dao/SnapshotDaoImpl.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,13 @@ public List<SnapshotVO> listAllByStatus(Snapshot.State... status) {
return listBy(sc, null);
}

@Override
public List<SnapshotVO> listAllByStatusIncludingRemoved(Snapshot.State... status) {
SearchCriteria<SnapshotVO> sc = this.StatusSearch.create();
sc.setParameters("status", (Object[])status);
return listIncludingRemovedBy(sc, null);
}

@Override
public boolean updateState(State currentState, Event event, State nextState, SnapshotVO snapshot, Object data) {
TransactionLegacy txn = TransactionLegacy.currentTxn();
Expand Down
12 changes: 10 additions & 2 deletions engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
// under the License.
package org.apache.cloudstack.storage.datastore;

import java.util.UUID;

import javax.inject.Inject;

import org.apache.log4j.Logger;
Expand Down Expand Up @@ -44,6 +46,7 @@
import com.cloud.agent.api.to.DataObjectType;
import com.cloud.agent.api.to.S3TO;
import com.cloud.exception.ConcurrentOperationException;
import com.cloud.hypervisor.Hypervisor.HypervisorType;
import com.cloud.storage.DataStoreRole;
import com.cloud.storage.SnapshotVO;
import com.cloud.storage.VMTemplateStoragePoolVO;
Expand Down Expand Up @@ -172,7 +175,11 @@ public DataObject create(DataObject obj, DataStore dataStore) {
if (snapshotDataStoreVO != null) {
ss.setParentSnapshotId(snapshotDataStoreVO.getSnapshotId());
}
ss.setInstallPath(TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId());
String snapshotInstallPath = TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR + "/" + snapshotDao.findById(obj.getId()).getAccountId() + "/" + snapshot.getVolumeId();
if (snapshot.getHypervisorType().equals(HypervisorType.VMware)) {

DaanHoogland Jul 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this really only true for vmware? It seems that adding a uuid to the path is a rather universal action in the ssvm, no?

wilderrodrigues Jul 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It seems like it should be applied to any hypervisor.

Is that another one we can take in and fix?
snapshotInstallPath = snapshotInstallPath.concat("/" + UUID.randomUUID().toString());

wilderrodrigues Jul 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would also call concat twice: 1 for the "/"; and the 2nd time for the UUID.randomUUID().toString().

DaanHoogland Jul 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

auto spell typo? contact == concat

wilderrodrigues Jul 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yep... I edited. :)
}
ss.setInstallPath(snapshotInstallPath);
ss.setState(ObjectInDataStoreStateMachine.State.Allocated);
ss = snapshotDataStoreDao.persist(ss);
break;
Expand Down Expand Up @@ -265,7 +272,8 @@ public boolean deleteIfNotReady(DataObject dataObj) {
return true;
case SNAPSHOT:
SnapshotDataStoreVO destSnapshotStore = snapshotDataStoreDao.findByStoreSnapshot(dataStore.getRole(), dataStore.getId(), objId);
if (destSnapshotStore != null && destSnapshotStore.getState() != ObjectInDataStoreStateMachine.State.Ready) {
if (destSnapshotStore != null && destSnapshotStore.getState() != ObjectInDataStoreStateMachine.State.Ready &&
destSnapshotStore.getState() != ObjectInDataStoreStateMachine.State.Allocated) {
return snapshotDataStoreDao.remove(destSnapshotStore.getId());
} else {
s_logger.warn("Snapshot " + objId + " is not found on image store " + dataStore.getId() + ", so no need to delete");
Expand Down
8 changes: 4 additions & 4 deletions .../hypervisors/vmware/src/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -1351,10 +1351,10 @@ public DeleteVMSnapshotAnswer execute(VmwareHostService hostService, DeleteVMSna
s_logger.debug("snapshot: " + vmSnapshotName + " is removed");

// after removed snapshot, the volumes' paths have been changed for the VM, needs to report new paths to manager

Map<String, String> mapNewDisk = getNewDiskMap(vmMo);

setVolumeToPathAndSize(listVolumeTo, mapNewDisk, context, hyperHost, cmd.getVmName());
if (listVolumeTo != null) {
Map<String, String> mapNewDisk = getNewDiskMap(vmMo);
setVolumeToPathAndSize(listVolumeTo, mapNewDisk, context, hyperHost, cmd.getVmName());
}

return new DeleteVMSnapshotAnswer(cmd, listVolumeTo);
}
Expand Down
6 changes: 3 additions & 3 deletions plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -1094,8 +1094,8 @@ private Pair<String, String[]> exportVolumeToSecondaryStroage(VirtualMachineMO v
// Ternary<String(backup uuid in secondary storage), String(device bus name), String[](original disk chain in the snapshot)>
private Ternary<String, String, String[]> backupSnapshotToSecondaryStorage(VirtualMachineMO vmMo, String installPath, String volumePath, String snapshotUuid,
String secStorageUrl, String prevSnapshotUuid, String prevBackupUuid, String workerVmName) throws Exception {

String backupUuid = UUID.randomUUID().toString();
String backupUuid = installPath.substring(installPath.lastIndexOf("/") + 1);
installPath = installPath.substring(0, installPath.lastIndexOf("/"));
Pair<String, String[]> snapshotInfo = exportVolumeToSecondaryStroage(vmMo, volumePath, secStorageUrl, installPath, backupUuid, workerVmName);
return new Ternary<String, String, String[]>(backupUuid, snapshotInfo.first(), snapshotInfo.second());
}
Expand Down Expand Up @@ -1184,7 +1184,7 @@ public Answer backupSnapshot(CopyCommand cmd) {
// Get snapshot physical size
long physicalSize = 0l;
String secondaryMountPoint = mountService.getMountPoint(secondaryStorageUrl);
String snapshotDir = destSnapshot.getPath() + "/" + snapshotBackupUuid;
String snapshotDir = destSnapshot.getPath();
File[] files = new File(secondaryMountPoint + "/" + snapshotDir).listFiles();
if(files != null) {
for(File file : files) {
Expand Down
55 changes: 52 additions & 3 deletions server/src/com/cloud/storage/StorageManagerImpl.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,10 @@
import com.cloud.agent.AgentManager;
import com.cloud.agent.api.Answer;
import com.cloud.agent.api.Command;
import com.cloud.agent.api.DeleteSnapshotsDirCommand;
import com.cloud.agent.api.DeleteVMSnapshotCommand;
import com.cloud.agent.api.StoragePoolInfo;
import com.cloud.agent.api.VMSnapshotTO;
import com.cloud.agent.manager.Commands;
import com.cloud.api.ApiDBUtils;
import com.cloud.api.query.dao.TemplateJoinDao;
Expand Down Expand Up @@ -1120,14 +1123,60 @@ public void cleanupStorage(boolean recurring) {
}

// remove snapshots in Error state
List<SnapshotVO> snapshots = _snapshotDao.listAllByStatus(Snapshot.State.Error);
List<SnapshotVO> snapshots = _snapshotDao.listAllByStatusIncludingRemoved(Snapshot.State.Error);
for (SnapshotVO snapshotVO : snapshots) {
try {
List<SnapshotDataStoreVO> storeRefs = _snapshotStoreDao.findBySnapshotId(snapshotVO.getId());
boolean isVMware = snapshotVO.getHypervisorType().equals(HypervisorType.VMware);
boolean removeSnapshot = true;
for (SnapshotDataStoreVO ref : storeRefs) {
_snapshotStoreDao.expunge(ref.getId());
// Cleanup corresponding items (if any) from secondary storage.
if (isVMware) {

DaanHoogland Jul 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

only for vmware???

wilderrodrigues Jul 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good catch!

We would have to get the PR in and test it, without the IF, against Xen and KVM, at least. What do you think?
if(ref.getRole().isImageStore()) { // Delete items from secondary storage
DataStore snapshotDataStore = _dataStoreMgr.getDataStore(ref.getDataStoreId(), DataStoreRole.Image);
DeleteSnapshotsDirCommand cmd = new DeleteSnapshotsDirCommand(snapshotDataStore.getTO(), ref.getInstallPath());
EndPoint ep = _epSelector.select(snapshotDataStore);
if (ep == null) {
s_logger.warn("There is no secondary storage VM for image store: " + snapshotDataStore.getName() + ", cannot cleanup snapshot: " + snapshotVO.getUuid());
removeSnapshot = false;
continue;
}
Answer deleteSnapshotsDirAnswer = ep.sendMessage(cmd);
if ((deleteSnapshotsDirAnswer != null) && deleteSnapshotsDirAnswer.getResult()) {
s_logger.debug("Deleted snapshot: " + snapshotVO.getUuid() + " from secondary storage: " + snapshotDataStore.getName());
_snapshotStoreDao.expunge(ref.getId());
} else {
s_logger.warn("Failed to delete snapshot: " + snapshotVO.getUuid() + " from secondary storage: " + snapshotDataStore.getName());
removeSnapshot = false;
}
} else if (ref.getRole() == DataStoreRole.Primary) { // Delete worker VM snapshot
VolumeVO volume = _volumeDao.findByIdIncludingRemoved(snapshotVO.getVolumeId());
if (volume.getInstanceId() == null)
continue;
VMInstanceVO vm = _vmInstanceDao.findById(volume.getInstanceId());
Long hostId = vm.getHostId() != null ? vm.getHostId() : vm.getLastHostId();
if (hostId != null) {
VMSnapshotTO vmSnapshotTO = new VMSnapshotTO();
vmSnapshotTO.setSnapshotName(ref.getInstallPath());
vmSnapshotTO.setDescription(snapshotVO.getName());
DeleteVMSnapshotCommand deleteSnapshotCommand = new DeleteVMSnapshotCommand(vm.getInstanceName(), vmSnapshotTO, null, null);
Answer deleteSnapshotAnswer = _agentMgr.send(hostId, deleteSnapshotCommand);
if ((deleteSnapshotAnswer != null) && deleteSnapshotAnswer.getResult()) {
s_logger.debug("Deleted worker VM snapshot: " + snapshotVO.getName());
_snapshotStoreDao.expunge(ref.getId());
} else {
s_logger.warn("Failed to delete worker VM snapshot: " + snapshotVO.getName());
removeSnapshot = false;
}
}
}
} else {
_snapshotStoreDao.expunge(ref.getId());
}
}
if (removeSnapshot) {
_snapshotDao.expunge(snapshotVO.getId());
}
_snapshotDao.expunge(snapshotVO.getId());
} catch (Exception e) {
s_logger.warn("Unable to destroy snapshot " + snapshotVO.getUuid(), e);
}
Expand Down
Back | FazBrowse Home | New Git URL