-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Support of snapshot copy to StorPool primary storage in different zones #9478
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9478 +/- ##
============================================
- Coverage 15.78% 15.76% -0.02%
- Complexity 12564 12566 +2
============================================
Files 5627 5627
Lines 492250 492863 +613
Branches 61405 62711 +1306
============================================
+ Hits 77710 77712 +2
- Misses 406066 406666 +600
- Partials 8474 8485 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
8cc7b49
to
0df6764
Compare
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
0df6764
to
e0f4283
Compare
@blueorangutan package |
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10756 |
@blueorangutan test |
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
[SF] Trillian test result (tid-11155)
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
e0f4283
to
6f209d8
Compare
api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CopySnapshotCmd.java
Show resolved
Hide resolved
.../src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreCapabilities.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CopySnapshotCmd.java
Outdated
Show resolved
Hide resolved
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
.../src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreCapabilities.java
Outdated
Show resolved
Hide resolved
continue; | ||
} | ||
for (SnapshotDetailsVO snapshot : snapshotDetails) { | ||
String name = snapshot.getValue().split(";")[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure the snapshot detail after .split()
has length >= 2 & then get name and location from it (chance of Index Out of Bound error).
} | ||
} | ||
|
||
private static List<String> snapshotsForRcovery(JsonArray arr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static List<String> snapshotsForRcovery(JsonArray arr) { | |
private static List<String> snapshotsForRecovery(JsonArray arr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, it's fixed in the latest commits
// logger.info(String.format("Cannot copy snapshot to another storage in different zone. It's not in the right state %s", snapshotOnStore.getStatus())); | ||
// return false; | ||
// } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove these comments if not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, it's removed in the latest commits
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
Outdated
Show resolved
Hide resolved
const listStoragePools = json.liststoragepoolsresponse.storagepool | ||
if (listStoragePools) { | ||
this.storagePools = listStoragePools | ||
this.storagePools = this.storagePools.filter(pool => pool.storagecapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES && pool.zoneid !== this.resource.zoneid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.storagePools = this.storagePools.filter(pool => pool.storagecapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES && pool.zoneid !== this.resource.zoneid) | |
this.storagePools = this.storagePools.filter(pool => pool.storagecapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE && pool.zoneid !== this.resource.zoneid) |
524f382
to
457b33f
Compare
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
…n zones Added support to copy a snapshot to another StorPool primary storage in different zones.
Added drop down to choose the primary storage pools to copy a snapshot Small fixes
457b33f
to
88d13d2
Compare
zone_found = True | ||
break | ||
if zone_found == False: | ||
cls.fail("Unable to find snapshot entry for the zone ID: %s" % zone_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slavkap , lint nitpicking on you (EOL before EOF needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm, lots of style comments but no blockers
import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao; | ||
import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; | ||
import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; | ||
import org.apache.commons.collections.CollectionUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.apache.commons.collections.CollectionUtils; | |
import org.apache.commons.collections.CollectionUtils; |
@@ -177,6 +107,74 @@ | |||
import com.cloud.vm.dao.UserVmCloneSettingDao; | |||
import com.cloud.vm.dao.UserVmDao; | |||
import com.cloud.vm.dao.UserVmDetailsDao; | |||
import org.apache.cloudstack.api.ApiCommandResourceType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.apache.cloudstack.api.ApiCommandResourceType; | |
import org.apache.cloudstack.api.ApiCommandResourceType; |
@@ -46,6 +28,22 @@ | |||
import com.cloud.utils.db.SearchCriteria; | |||
import com.cloud.utils.db.TransactionLegacy; | |||
import com.cloud.utils.db.UpdateBuilder; | |||
import org.apache.cloudstack.engine.subsystem.api.storage.DataObjectInStore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.apache.cloudstack.engine.subsystem.api.storage.DataObjectInStore; | |
import org.apache.cloudstack.engine.subsystem.api.storage.DataObjectInStore; |
import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine; | ||
import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.Event; | ||
import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.State; | ||
import org.apache.commons.collections.CollectionUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.apache.commons.collections.CollectionUtils; | |
import org.apache.commons.collections.CollectionUtils; |
import com.cloud.utils.db.TransactionCallbackNoReturn; | ||
import com.cloud.utils.db.TransactionStatus; | ||
import com.cloud.utils.exception.CloudRuntimeException; | ||
import com.cloud.utils.fsm.NoTransitionException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import com.cloud.utils.fsm.NoTransitionException; | |
import com.cloud.utils.fsm.NoTransitionException; | |
import org.apache.cloudstack.utils.volume.VirtualMachineDiskInfo; | ||
import org.apache.commons.collections.CollectionUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.apache.cloudstack.utils.volume.VirtualMachineDiskInfo; | |
import org.apache.commons.collections.CollectionUtils; | |
import org.apache.cloudstack.utils.volume.VirtualMachineDiskInfo; | |
import org.apache.commons.collections.CollectionUtils; |
if (CollectionUtils.isNotEmpty(poolIds)) { | ||
throw new InvalidParameterValueException(String.format("%s can not be specified for snapshots linked with snapshot policy", ApiConstants.STORAGE_ID_LIST)); | ||
} | ||
List<SnapshotPolicyDetailVO> poolDetails = snapshotPolicyDetailsDao.findDetails(policyId, ApiConstants.STORAGE_ID); | ||
poolIds = poolDetails.stream().map(d -> Long.valueOf(d.getValue())).collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new method?
@@ -174,6 +108,73 @@ | |||
import com.cloud.vm.snapshot.VMSnapshotVO; | |||
import com.cloud.vm.snapshot.dao.VMSnapshotDao; | |||
import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao; | |||
import org.apache.cloudstack.acl.SecurityChecker; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.apache.cloudstack.acl.SecurityChecker; | |
import org.apache.cloudstack.acl.SecurityChecker; |
import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; | ||
import org.apache.commons.collections.CollectionUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; | |
import org.apache.commons.collections.CollectionUtils; | |
import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; | |
import org.apache.commons.collections.CollectionUtils; |
private boolean canCopyOnPrimary(List<Long> poolIds, Snapshot snapshot) { | ||
List<Long> poolsToBeRemoved = new ArrayList<>(); | ||
for (Long poolId : poolIds) { | ||
PrimaryDataStore dataStore = (PrimaryDataStore) dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary); | ||
if (dataStore == null) { | ||
poolsToBeRemoved.add(poolId); | ||
continue; | ||
} | ||
SnapshotInfo snapshotInfo = snapshotFactory.getSnapshot(snapshot.getId(), poolId, DataStoreRole.Primary); | ||
if (snapshotInfo != null) { | ||
logger.debug(String.format("Snapshot [%s] already exist on pool [%s]", snapshot.getUuid(), dataStore.getName())); | ||
continue; | ||
} | ||
|
||
VolumeVO volume = _volsDao.findById(snapshot.getVolumeId()); | ||
if (volume == null) { | ||
poolsToBeRemoved.add(poolId); | ||
continue; | ||
} | ||
if (!dataStore.getDriver().getCapabilities() | ||
.containsKey(DataStoreCapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE.toString()) | ||
&& dataStore.getPoolType() != volume.getPoolType()) { | ||
poolsToBeRemoved.add(poolId); | ||
logger.debug(String.format("The %s does not support copy to %s between zones", dataStore.getPoolType(), volume.getPoolType())); | ||
} | ||
} | ||
poolIds.removeAll(poolsToBeRemoved); | ||
if (CollectionUtils.isEmpty(poolIds)) { | ||
return false; | ||
} | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be further distilled into smaller methods. Surely some of those can than be reused for the actual copy method.
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Description
This PR supports copying a snapshot to StorPool primary storage in different zones.
Added additional API param
storageids
in CloudStack API calls:The option
snapshot.backup.to.secondary
does not apply to the copy functionality. The snapshots will be copied only on the required primary storage in a different zone.The user can create volumes/templates from the copied snapshots. The option to copy the snapshots to both primary and secondary storage is available by providing in the create/copy snapshot the destination zone IDs and storage IDs
For other storage plugins that want to adopt this functionality:
CAN_COPY_SNAPSHOT_BETWEEN_ZONES
copySnapshot
method in their SnapshotStrategy and that the driver can handle the COPY operationTypes of changes
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
Manual and smoke tests with StorPool primary storage on multiple zones