Damans227 commented on code in PR #13562:
URL: https://github.com/apache/cloudstack/pull/13562#discussion_r3624820971
##########
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java:
##########
@@ -176,6 +176,7 @@
import com.cloud.storage.dao.DiskOfferingDao;
import com.cloud.storage.dao.GuestOSDao;
import com.cloud.storage.dao.SnapshotDao;
+import com.cloud.storage.dao.StoragePoolAndAccessGroupMapDao;
Review Comment:
Nit: breaks alphabetical import order, this sits between `SnapshotDao` and
`SnapshotPolicyDao`.
##########
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java:
##########
@@ -3196,95 +3199,198 @@ public void
executeStepsToChangeOwnershipOfVmTestResourceCountRunningVmsOnlyEnab
}
@Test
- public void validateStorageAccessGroupsOnHostsMatchingSAGsNoException() {
- Host srcHost = Mockito.mock(Host.class);
- Host destHost = Mockito.mock(Host.class);
+ public void
testValidateStorageAccessGroupsOnHostsVolumeBasedSuccessWithMatchingGroups() {
+ VMInstanceVO vm = Mockito.mock(VMInstanceVO.class);
+ Host destinationHost = Mockito.mock(Host.class);
- Mockito.when(srcHost.getId()).thenReturn(1L);
- Mockito.when(destHost.getId()).thenReturn(2L);
- when(storageManager.getStorageAccessGroups(null, null, null,
srcHost.getId())).thenReturn(new String[]{"sag1", "sag2"});
- when(storageManager.getStorageAccessGroups(null, null, null,
destHost.getId())).thenReturn(new String[]{"sag1", "sag2", "sag3"});
+ List<VolumeVO> vmVolumes = new ArrayList<>();
+ VolumeVO volume1 = Mockito.mock(VolumeVO.class);
+ VolumeVO volume2 = Mockito.mock(VolumeVO.class);
+ vmVolumes.add(volume1);
+ vmVolumes.add(volume2);
- userVmManagerImpl.validateStorageAccessGroupsOnHosts(srcHost,
destHost);
+ StoragePoolVO storagePool1 = Mockito.mock(StoragePoolVO.class);
Review Comment:
Nit: `storagePool1` (and `storagePool2` in a couple of the other new tests
here) is declared but never used, looks like leftover from before this test was
reworked to be volume-based. Same pattern repeats in a few sibling test methods
below.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -592,6 +593,8 @@ public class UserVmManagerImpl extends ManagerBase
implements UserVmManager, Vir
@Inject
private StorageManager storageManager;
@Inject
+ private StoragePoolAndAccessGroupMapDao _storagePoolAndAccessGroupMapDao;
Review Comment:
Nit: this new field uses the legacy underscore-prefix style, but the
`@Inject` fields right around it (`storageManager`, `serviceOfferingJoinDao`)
are plain camelCase.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -413,6 +413,7 @@
import com.google.gson.Gson;
import com.google.gson.reflect.TypeToken;
+import com.cloud.storage.dao.StoragePoolAndAccessGroupMapDao;
Review Comment:
Nit: this import is tacked on after the `com.google.gson` imports rather
than grouped with the other `com.cloud.storage.dao.*` imports further up.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -7311,23 +7314,44 @@ private boolean checkEnforceStrictHostTagCheck(HostVO
host, ServiceOffering serv
return host.checkHostServiceOfferingAndTemplateTags(serviceOffering,
template, strictHostTags);
}
- protected void validateStorageAccessGroupsOnHosts(Host srcHost, Host
destinationHost) {
- String[] storageAccessGroupsOnSrcHost =
storageManager.getStorageAccessGroups(null, null, null, srcHost.getId());
- String[] storageAccessGroupsOnDestHost =
storageManager.getStorageAccessGroups(null, null, null,
destinationHost.getId());
-
- List<String> srcHostStorageAccessGroupsList =
storageAccessGroupsOnSrcHost != null ?
Arrays.asList(storageAccessGroupsOnSrcHost) : Collections.emptyList();
- List<String> destHostStorageAccessGroupsList =
storageAccessGroupsOnDestHost != null ?
Arrays.asList(storageAccessGroupsOnDestHost) : Collections.emptyList();
-
- if (CollectionUtils.isEmpty(srcHostStorageAccessGroupsList)) {
+ protected void validateStorageAccessGroupsOnHosts(VMInstanceVO vm, Host
destinationHost) {
+ List<VolumeVO> vmVolumes = _volsDao.findByInstance(vm.getId());
+ if (CollectionUtils.isEmpty(vmVolumes)) {
+ logger.debug("No volumes found for VM {}, skipping storage access
group validation", vm);
return;
}
+ String[] storageAccessGroupsOnDestHost =
storageManager.getStorageAccessGroups(null, null, null,
destinationHost.getId());
+ List<String> destHostStorageAccessGroupsList =
Arrays.asList(storageAccessGroupsOnDestHost);
if (CollectionUtils.isEmpty(destHostStorageAccessGroupsList)) {
- throw new CloudRuntimeException("Source host has storage access
groups, but destination host has none.");
+ logger.debug("Destination host {} has no storage access groups",
destinationHost.getName());
}
- if
(!destHostStorageAccessGroupsList.containsAll(srcHostStorageAccessGroupsList)) {
- throw new CloudRuntimeException("Storage access groups on the
source and destination hosts did not match.");
+ for (VolumeVO volume : vmVolumes) {
+ if (volume.getPoolId() == null) {
+ logger.debug("Volume {} has no storage pool assigned, skipping
storage access group validation", volume.getName());
+ continue;
+ }
+ List<String> poolStorageAccessGroupsList =
_storagePoolAndAccessGroupMapDao.getStorageAccessGroups(volume.getPoolId());
+ if (CollectionUtils.isEmpty(poolStorageAccessGroupsList)) {
+ logger.debug("Storage pool {} has no storage access groups,
skipping validation for volume {}",
+ volume.getPoolId(), volume.getName());
+ continue;
+ }
+ if (Collections.disjoint(poolStorageAccessGroupsList,
destHostStorageAccessGroupsList)) {
+ throw new CloudRuntimeException(String.format(
+ "Destination host %s does not have any storage access
groups that match the requirements for volume %s (Resource Class: %s) on
storage pool %d. " +
+ "Pool requires access groups: %s, but destination host
only has: %s",
+ destinationHost.getName(),
+ volume.getName(),
+ volume.getVolumeType(), volume.getPoolId(),
+ String.join(", ", poolStorageAccessGroupsList),
+ String.join(", ", destHostStorageAccessGroupsList)
+ ));
+ }
+ logger.debug(String.format("Storage access group validation passed
for volume %s (Resource Class: %s) on storage pool %d. " +
Review Comment:
Nit: prefer SLF4J `{}` placeholders over `String.format` inside a debug log
call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
