create temp CG for consistent VM snapshot for the VM which is span across multiple flexvolumes#74
create temp CG for consistent VM snapshot for the VM which is span across multiple flexvolumes#74rajiv-jain-netapp wants to merge 8 commits into
Conversation
… multiple volumes at storage
… at multiple flexvolumes
…ective checks by which delete snapshot would be handled from primary only
rajiv-jain-netapp
requested review from
piyush5netapp,
sandeeplocharla and
suryag1201
as code owners
|
testing results and all the executed test cases added under previous PR #67 |
sandeeplocharla
left a comment
sandeeplocharla
left a comment
There was a problem hiding this comment.
Partially reviewed. Still need to go through a good portion of OntapVMSnapshotStrategy.java and SnapshotManagerImpl.java
| return isSnapshotNotFoundError(error.getCause()); | ||
| } | ||
|
|
||
| private long resolveSnapshotPoolId(String poolIdStr, long snapshotId) { |
There was a problem hiding this comment.
All the exceptions in this method look dubious. Please modify them according to the conditional check.
|
|
||
| VolumeVO volumeVO = volumeDao.findById(volumeInfo.getId()); | ||
| if (volumeVO == null) { | ||
| throw new CloudRuntimeException("VolumeVO not found for id: " + volumeInfo.getId()); |
There was a problem hiding this comment.
We can rephrase this as "'volume object'/'volume' not found for id: in the database".
This might be more user friendly.
| if (lunUUID == null) { | ||
| throw new CloudRuntimeException("LUN UUID not found for iSCSI volume " + volumeVO.getId()); | ||
| } | ||
| lunUuid = lunUUID; |
There was a problem hiding this comment.
Why can't we use this in ln 721 directly, instead of creating a new variable lunUUID?
| logger.error("NFS protocol is not enabled on SVM " + svmName); | ||
| throw new CloudRuntimeException("NFS protocol is not enabled on SVM " + svmName); | ||
| } else if (Objects.equals(storage.getProtocol(), ProtocolType.ISCSI) && !svm.getIscsiEnabled()) { | ||
| } else if (Objects.equals(storage.getProtocol(), ProtocolType.ISCSI) && !svm.getIscsiEnabled()) { |
There was a problem hiding this comment.
Please correct the spacing.
| /** | ||
| * Polls an ONTAP async job when present and returns the completed job (for extracting created resource UUIDs). | ||
| */ | ||
| public Job pollJobIfPresentAndGetCompletedJob(JobResponse response, String operationName) { |
There was a problem hiding this comment.
Couldn't find any instance of this being used. Is this needed somewhere?
| JobResponse jobResponse = snapshotFeignClient.deleteSnapshot(getAuthHeader(), flexVolUuid, snapshotUuid); | ||
|
|
||
| if (jobResponse == null || jobResponse.getJob() == null) { | ||
| logger.debug("deleteFlexVolSnapshotForCloudStackVolume: no async job returned for snapshot [{}] " |
There was a problem hiding this comment.
Also, shouldn't we be treating jobResponse 'null' as HTTP failure?
| /** | ||
| * Builds an ONTAP-safe name token from user-provided snapshot text. | ||
| */ | ||
| public static String getOntapCloneName(String cloudStackSnapshotName) { |
There was a problem hiding this comment.
Aren't we doing SFSR? Shouldn't we rename 'getOntapCloneName' method appropriately?
| String cgSnapshotUuid = null; | ||
| long cgFlowStart = System.nanoTime(); | ||
|
|
||
| // All volumes in a VM snapshot belong to ONTAP-managed pools and share the same ONTAP credentials. |
There was a problem hiding this comment.
// All volumes in a VM snapshot belong to ONTAP-managed pools and share the same ONTAP credentials.
If we are assuming this, we need to set this as a pre-requisite to the user. Else, the user might create storagePools from different ONTAPs with different creds.
| if (flexVolGroups.size() > 1) { | ||
| logger.info("takeVMSnapshot: VM [{}] spans {} FlexVol(s); using temporary CG two-phase snapshot flow", | ||
| userVm.getInstanceName(), flexVolGroups.size()); | ||
| createVmSnapshotsViaTemporaryCg(vmSnapshot, userVm, flexVolGroups, snapshotNameBase, createdSnapshots); |
There was a problem hiding this comment.
My understanding from our initial discussion on this approach was that 1CG can be mapped to 1 FV. If this is the case, then why can't we always use createVmSnapshotsViaTemporaryCg? Instead of having createVmSnapshotsViaSingleFlexVol separately for 1FV usecase?
| flexVolUuid, snapshotUuid, snapshotNameBase, volumePath, groupInfo.poolId, protocol)); | ||
| } | ||
|
|
||
| logger.info("takeVMSnapshot: [FlexVol] Snapshot [{}] (uuid={}) on FlexVol [{}] completed in {} ms. Covers volumes: {}", |
There was a problem hiding this comment.
This could be 'debug' log.
2 participants
Description
This PR...
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?