Skip to content

protect against stray snapshot-details without snapshot #4924

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

Merged
yadvr merged 3 commits into from
Apr 29, 2021
Merged

protect against stray snapshot-details without snapshot #4924

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) {
SnapshotVO snapshot = snapshotDao.findById(snapshotId);
if (snapshot == null) {
return null;
}

@sureshanaparti sureshanaparti Apr 16, 2021

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.

@DaanHoogland just check whether the 'null' returned here is being verified or not wherever this method is called, to avoid any NPE.

@DaanHoogland DaanHoogland Apr 19, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i ran through the 20 call of getsnapshot(). I added some null checks but think they are superfluent. If we go on with this we should refactor the interface to include a specific exception.

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;
}
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