protect against stray snapshot-details without snapshot #70
Conversation
|
@rhtyd @sureshanaparti I would like to submit this for upstream 4.15, small fix but very hard to test. What do you think? |
Adds a custom volume storage migration form This fixes #70 Signed-off-by: Rohit Yadav <[email protected]>
|
@DaanHoogland @rhtyd I was mentioning about this PR: apache#4130 (fixed for service ticket apache#2870). |
| @@ -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; | |||
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
Ignore my last comment. Above ^^ remove stmt is correct as the record is being removed by details record id, and it is safe.
| @@ -87,6 +87,9 @@ public SnapshotInfo getSnapshot(DataObject obj, DataStore store) { | |||
| @Override | |||
| public SnapshotInfo getSnapshot(long snapshotId, DataStoreRole role) { | |||
There was a problem hiding this comment.
@DaanHoogland make sure 'null' returned here is checked, wherever this method is called.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
added two null checks. I think these are superfluent, but they wont hurt.
Description
This PR makes sure no orphaned snapshot details are considered in the cleanup at startup job.
a real solution would be to implement some kind of cascading delete, but as the parent record is "only" marked as removed this would be a bit complicated as a quick fix and also not a clean solution either.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?