Skip to content
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

protect against stray snapshot-details without snapshot #70

Draft
wants to merge 3 commits into
base: 4.15
Choose a base branch
from
Draft

protect against stray snapshot-details without snapshot #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d775163
protect against stray snapshot-details without snapshot
Apr 2, 2021
120dd41
extra null checks
Apr 18, 2021
bd3f639
remove unused
Apr 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ...ration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,9 @@ protected List<DataObject> getAllReadySnapshotsAndChains(DataStore srcDataStore,
snapshotVO != null && snapshotVO.getHypervisorType() != Hypervisor.HypervisorType.Simulator
&& snapshot.getParentSnapshotId() == 0 ) {
SnapshotInfo snap = snapshotFactory.getSnapshot(snapshotVO.getSnapshotId(), DataStoreRole.Image);
files.add(snap);
if (snap != null) {
files.add(snap);
}
}
}

Expand Down
3 changes: 3 additions & 0 deletions ...napshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ public List<SnapshotInfo> getSnapshots(long volumeId, DataStoreRole role) {
@Override
public SnapshotInfo getSnapshot(long snapshotId, DataStoreRole role) {
Copy link
Member

Choose a reason for hiding this comment

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

@DaanHoogland make sure 'null' returned here is checked, wherever this method is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that at line 94/97 null is returned as well. It is called 20 times, not including tests. if the snapshot is null and an SnapshotInfo object is returned with a null snapshot field it will result in runtime exceptions if it is used. I agree that errors may occur in different placec, but not more errors will occur. I'll spend some time researching thos 20 callers.

Copy link
Member Author

Choose a reason for hiding this comment

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

added two null checks. I think these are superfluent, but they wont hurt.

SnapshotVO snapshot = snapshotDao.findById(snapshotId);
if (snapshot == null) {
return null;
}
SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findBySnapshot(snapshotId, role);
if (snapshotStore == null) {
snapshotStore = snapshotStoreDao.findByVolume(snapshot.getVolumeId(), role);
Expand Down
5 changes: 4 additions & 1 deletion ...storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,10 @@ public List<SnapshotInfo> getChildren() {
List<SnapshotInfo> children = new ArrayList<>();
if (vos != null) {
for (SnapshotDataStoreVO vo : vos) {
children.add(snapshotFactory.getSnapshot(vo.getSnapshotId(), DataStoreRole.Image));
SnapshotInfo info = snapshotFactory.getSnapshot(vo.getSnapshotId(), DataStoreRole.Image);
if (info != null) {
children.add(info);
}
}
}
return children;
Expand Down
4 changes: 4 additions & 0 deletions ...ork/jobs/src/main/java/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1097,6 +1097,10 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
final List<SnapshotDetailsVO> snapshotList = _snapshotDetailsDao.findDetails(AsyncJob.Constants.MS_ID, Long.toString(msid), false);
for (final SnapshotDetailsVO snapshotDetailsVO : snapshotList) {
SnapshotInfo snapshot = snapshotFactory.getSnapshot(snapshotDetailsVO.getResourceId(), DataStoreRole.Primary);
if (snapshot == null) {
_snapshotDetailsDao.remove(snapshotDetailsVO.getId());
continue;
Copy link
Member

@sureshanaparti sureshanaparti Apr 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@DaanHoogland here the snapshot details is scanned for 'MS_ID' only, and the associated snapshot object (if exists) holding this detail is transitioned to failed/error state. So, I think it is safe to remove detail 'MS_ID' only (not sure if any other detail is being process elsewhere).

Copy link
Member

Choose a reason for hiding this comment

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

Ignore my last comment. Above ^^ remove stmt is correct as the record is being removed by details record id, and it is safe.

}
snapshotSrv.processEventOnSnapshotObject(snapshot, Snapshot.Event.OperationFailed);
_snapshotDetailsDao.removeDetail(snapshotDetailsVO.getResourceId(), AsyncJob.Constants.MS_ID);
}
Expand Down
3 changes: 0 additions & 3 deletions server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import com.cloud.agent.api.Answer;
import com.cloud.agent.api.Command;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.storage.Snapshot;
import com.cloud.storage.SnapshotVO;
import com.cloud.storage.Volume;

Expand Down Expand Up @@ -83,7 +82,5 @@ public interface SnapshotManager extends Configurable {

SnapshotVO getParentSnapshot(VolumeInfo volume);

Snapshot backupSnapshot(Long snapshotId);

SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationException;
}
10 changes: 0 additions & 10 deletions server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,16 +412,6 @@ public Snapshot archiveSnapshot(Long snapshotId) {
return snapshotOnSecondary;
}

@Override
public Snapshot backupSnapshot(Long snapshotId) {
SnapshotInfo snapshot = snapshotFactory.getSnapshot(snapshotId, DataStoreRole.Image);
if (snapshot != null) {
throw new CloudRuntimeException("Already in the backup snapshot:" + snapshotId);
}

return snapshotSrv.backupSnapshot(snapshot);
}

@Override
public Snapshot backupSnapshotFromVmSnapshot(Long snapshotId, Long vmId, Long volumeId, Long vmSnapshotId) {
VMInstanceVO vm = _vmDao.findById(vmId);
Expand Down