Skip to content

Commit 7be7ef6

Browse files
BryanMLimaGutoVeroneziDaanHoogland
authored
Improve error message on storage tags update (#6269)
Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com> Co-authored-by: dahn <daan.hoogland@gmail.com>
1 parent 9c2a462 commit 7be7ef6

File tree

5 files changed

+96
-5
lines changed

5 files changed

+96
-5
lines changed

‎engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,4 +131,12 @@ public interface VolumeDao extends GenericDao<VolumeVO, Long>, StateDao<Volume.S
131131
* Updates the disk offering for the given volume.
132132
*/
133133
void updateDiskOffering(long volumeId, long diskOfferingId);
134+
135+
/**
136+
* Retrieves volumes that use the disk offering passed as parameter.
137+
*
138+
* @param diskOfferingId the disk offering ID.
139+
* @return the list of volumes that uses that disk offering.
140+
*/
141+
List<VolumeVO> findByDiskOfferingId(long diskOfferingId);
134142
}

‎engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ public class VolumeDaoImpl extends GenericDaoBase<VolumeVO, Long> implements Vol
6161
protected final GenericSearchBuilder<VolumeVO, Long> ActiveTemplateSearch;
6262
protected final SearchBuilder<VolumeVO> InstanceStatesSearch;
6363
protected final SearchBuilder<VolumeVO> AllFieldsSearch;
64+
protected final SearchBuilder<VolumeVO> diskOfferingSearch;
6465
protected final SearchBuilder<VolumeVO> RootDiskStateSearch;
6566
protected GenericSearchBuilder<VolumeVO, Long> CountByAccount;
6667
protected GenericSearchBuilder<VolumeVO, SumCount> primaryStorageSearch;
@@ -262,6 +263,14 @@ public List<VolumeVO> findByTemplateAndZone(long templateId, long zoneId) {
262263
return listIncludingRemovedBy(sc);
263264
}
264265

266+
@Override
267+
public List<VolumeVO> findByDiskOfferingId(long diskOfferingId) {
268+
SearchCriteria<VolumeVO> sc = diskOfferingSearch.create();
269+
sc.setParameters("diskOfferingId", diskOfferingId);
270+
271+
return listBy(sc);
272+
}
273+
265274
@Override
266275
public boolean isAnyVolumeActivelyUsingTemplateOnPool(long templateId, long poolId) {
267276
SearchCriteria<Long> sc = ActiveTemplateSearch.create();
@@ -392,6 +401,10 @@ public VolumeDaoImpl() {
392401
TemplateZoneSearch.and("zone", TemplateZoneSearch.entity().getDataCenterId(), Op.EQ);
393402
TemplateZoneSearch.done();
394403

404+
diskOfferingSearch = createSearchBuilder();
405+
diskOfferingSearch.and("diskOfferingId", diskOfferingSearch.entity().getDiskOfferingId(), Op.EQ);
406+
diskOfferingSearch.done();
407+
395408
TotalSizeByPoolSearch = createSearchBuilder(SumCount.class);
396409
TotalSizeByPoolSearch.select("sum", Func.SUM, TotalSizeByPoolSearch.entity().getSize());
397410
TotalSizeByPoolSearch.select("count", Func.COUNT, (Object[])null);

‎engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,10 @@ public class PrimaryDataStoreDaoImpl extends GenericDaoBase<StoragePoolVO, Long>
7474
protected final String TagsSqlPrefix = "SELECT storage_pool.* from storage_pool LEFT JOIN storage_pool_tags ON storage_pool.id = storage_pool_tags.pool_id WHERE storage_pool.removed is null and storage_pool.status = 'Up' and storage_pool.data_center_id = ? and (storage_pool.pod_id = ? or storage_pool.pod_id is null) and storage_pool.scope = ? and (";
7575
protected final String TagsSqlSuffix = ") GROUP BY storage_pool_tags.pool_id HAVING COUNT(storage_pool_tags.tag) >= ?";
7676

77-
private static final String GET_STORAGE_POOLS_OF_VOLUMES_WITHOUT_OR_NOT_HAVING_TAGS = "select s.id " +
78-
"from volumes vol " +
79-
"join storage_pool s on vol.pool_id=s.id " +
80-
"where vol.disk_offering_id= ? and vol.state not in (\"Destroy\", \"Error\", \"Expunging\") group by s.id";
77+
private static final String GET_STORAGE_POOLS_OF_VOLUMES_WITHOUT_OR_NOT_HAVING_TAGS = "SELECT s.* " +
78+
"FROM volumes vol " +
79+
"JOIN storage_pool s ON vol.pool_id = s.id " +
80+
"WHERE vol.disk_offering_id = ? AND vol.state NOT IN (\"Destroy\", \"Error\", \"Expunging\", \"Expunged\") GROUP BY s.id";
8181

8282
/**
8383
* Used in method findPoolsByDetailsOrTagsInternal

‎server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import javax.inject.Inject;
4545
import javax.naming.ConfigurationException;
4646

47+
4748
import org.apache.cloudstack.acl.SecurityChecker;
4849
import org.apache.cloudstack.affinity.AffinityGroup;
4950
import org.apache.cloudstack.affinity.AffinityGroupService;
@@ -118,6 +119,7 @@
118119
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
119120
import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao;
120121
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
122+
import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
121123
import org.apache.commons.collections.CollectionUtils;
122124
import org.apache.commons.collections.MapUtils;
123125
import org.apache.commons.lang3.ObjectUtils;
@@ -239,6 +241,7 @@
239241
import com.cloud.storage.Storage.ProvisioningType;
240242
import com.cloud.storage.StorageManager;
241243
import com.cloud.storage.Volume;
244+
import com.cloud.storage.VolumeVO;
242245
import com.cloud.storage.dao.DiskOfferingDao;
243246
import com.cloud.storage.dao.StoragePoolTagsDao;
244247
import com.cloud.storage.dao.VMTemplateZoneDao;
@@ -3900,7 +3903,14 @@ protected void updateOfferingTagsIfIsNotNull(String tags, DiskOfferingVO diskOff
39003903
for (StoragePoolVO storagePoolVO : pools) {
39013904
List<String> tagsOnPool = storagePoolTagDao.getStoragePoolTags(storagePoolVO.getId());
39023905
if (CollectionUtils.isEmpty(tagsOnPool) || !tagsOnPool.containsAll(listOfTags)) {
3903-
throw new InvalidParameterValueException(String.format("There are active volumes using offering [%s], and the pools [%s] don't have the new tags", diskOffering.getId(), pools));
3906+
DiskOfferingVO offeringToRetrieveInfo = _diskOfferingDao.findById(diskOffering.getId());
3907+
List<VolumeVO> volumes = _volumeDao.findByDiskOfferingId(diskOffering.getId());
3908+
String listOfVolumesNamesAndUuid = ReflectionToStringBuilderUtils.reflectOnlySelectedFields(volumes, "name", "uuid");
3909+
String diskOfferingInfo = ReflectionToStringBuilderUtils.reflectOnlySelectedFields(offeringToRetrieveInfo, "name", "uuid");
3910+
String poolInfo = ReflectionToStringBuilderUtils.reflectOnlySelectedFields(storagePoolVO, "name", "uuid");
3911+
throw new InvalidParameterValueException(String.format("There are active volumes using the disk offering %s, and the pool %s doesn't have the new tags. " +
3912+
"The following volumes are using the mentioned disk offering %s. Please first add the new tags to the mentioned storage pools before adding them" +
3913+
" to the disk offering.", diskOfferingInfo, poolInfo, listOfVolumesNamesAndUuid));
39043914
}
39053915
}
39063916
}

‎server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@
5353
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
5454
import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
5555
import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
56+
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
57+
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
5658
import org.apache.log4j.Logger;
5759
import org.junit.After;
5860
import org.junit.Assert;
@@ -104,6 +106,8 @@
104106
import com.cloud.network.dao.PhysicalNetworkDao;
105107
import com.cloud.network.dao.PhysicalNetworkVO;
106108
import com.cloud.projects.ProjectManager;
109+
import com.cloud.storage.dao.DiskOfferingDao;
110+
import com.cloud.storage.dao.StoragePoolTagsDao;
107111
import com.cloud.storage.DiskOfferingVO;
108112
import com.cloud.storage.VolumeVO;
109113
import com.cloud.storage.dao.VolumeDao;
@@ -186,6 +190,16 @@ public class ConfigurationManagerTest {
186190
@Mock
187191
DiskOfferingVO diskOfferingVOMock;
188192
@Mock
193+
PrimaryDataStoreDao primaryDataStoreDao;
194+
@Mock
195+
StoragePoolTagsDao storagePoolTagsDao;
196+
@Mock
197+
DiskOfferingDao diskOfferingDao;
198+
@Mock
199+
VolumeVO volumeVO;
200+
@Mock
201+
StoragePoolVO storagePoolVO;
202+
@Mock
189203
DataCenterGuestIpv6PrefixDao dataCenterGuestIpv6PrefixDao;
190204
@Mock
191205
Ipv6GuestPrefixSubnetNetworkMapDao ipv6GuestPrefixSubnetNetworkMapDao;
@@ -1023,6 +1037,52 @@ public void updateDiskOfferingTagsIfIsNotNullTestWhenTagsIsNotNull(){
10231037
Mockito.verify(configurationMgr, Mockito.times(1)).updateOfferingTagsIfIsNotNull(tags, diskOfferingVOMock);
10241038
}
10251039

1040+
@Test (expected = InvalidParameterValueException.class)
1041+
public void updateDiskOfferingTagsWithPrimaryStorageTagsEqualNullTestThrowException(){
1042+
String tags = "tags";
1043+
List<String> storageTagsNull = new ArrayList<>();
1044+
List<StoragePoolVO> pools = new ArrayList<>(Arrays.asList(storagePoolVO));
1045+
List<VolumeVO> volumes = new ArrayList<>(Arrays.asList(volumeVO));
1046+
1047+
Mockito.when(primaryDataStoreDao.listStoragePoolsWithActiveVolumesByOfferingId(anyLong())).thenReturn(pools);
1048+
Mockito.when(storagePoolTagsDao.getStoragePoolTags(anyLong())).thenReturn(storageTagsNull);
1049+
Mockito.when(diskOfferingDao.findById(anyLong())).thenReturn(diskOfferingVOMock);
1050+
Mockito.when(_volumeDao.findByDiskOfferingId(anyLong())).thenReturn(volumes);
1051+
1052+
this.configurationMgr.updateOfferingTagsIfIsNotNull(tags, diskOfferingVOMock);
1053+
}
1054+
1055+
@Test (expected = InvalidParameterValueException.class)
1056+
public void updateDiskOfferingTagsWithPrimaryStorageMissingTagsTestThrowException(){
1057+
String tags = "tag1,tag2";
1058+
List<String> storageTagsWithMissingTag = new ArrayList<>(Arrays.asList("tag1"));
1059+
List<StoragePoolVO> pools = new ArrayList<>(Arrays.asList(storagePoolVO));
1060+
List<VolumeVO> volumes = new ArrayList<>(Arrays.asList(volumeVO));
1061+
1062+
Mockito.when(primaryDataStoreDao.listStoragePoolsWithActiveVolumesByOfferingId(anyLong())).thenReturn(pools);
1063+
Mockito.when(storagePoolTagsDao.getStoragePoolTags(anyLong())).thenReturn(storageTagsWithMissingTag);
1064+
Mockito.when(diskOfferingDao.findById(anyLong())).thenReturn(diskOfferingVOMock);
1065+
Mockito.when(_volumeDao.findByDiskOfferingId(anyLong())).thenReturn(volumes);
1066+
1067+
this.configurationMgr.updateOfferingTagsIfIsNotNull(tags, diskOfferingVOMock);
1068+
}
1069+
1070+
@Test
1071+
public void updateDiskOfferingTagsWithPrimaryStorageWithCorrectTagsTestSuccess(){
1072+
String tags = "tag1,tag2";
1073+
List<String> storageTagsWithCorrectTags = new ArrayList<>(Arrays.asList("tag1","tag2"));
1074+
List<StoragePoolVO> pools = new ArrayList<>(Arrays.asList(storagePoolVO));
1075+
List<VolumeVO> volumes = new ArrayList<>(Arrays.asList(volumeVO));
1076+
1077+
Mockito.when(primaryDataStoreDao.listStoragePoolsWithActiveVolumesByOfferingId(anyLong())).thenReturn(pools);
1078+
Mockito.when(storagePoolTagsDao.getStoragePoolTags(anyLong())).thenReturn(storageTagsWithCorrectTags);
1079+
Mockito.when(diskOfferingDao.findById(anyLong())).thenReturn(diskOfferingVOMock);
1080+
Mockito.when(_volumeDao.findByDiskOfferingId(anyLong())).thenReturn(volumes);
1081+
1082+
this.configurationMgr.updateOfferingTagsIfIsNotNull(tags, diskOfferingVOMock);
1083+
Mockito.verify(diskOfferingVOMock, Mockito.times(1)).setTags(tags);
1084+
}
1085+
10261086
@Test(expected = IllegalArgumentException.class)
10271087
public void testInvalidCreateDataCenterGuestIpv6Prefix() {
10281088
CreateGuestNetworkIpv6PrefixCmd cmd = Mockito.mock(CreateGuestNetworkIpv6PrefixCmd.class);

0 commit comments

Comments
 (0)