Skip to content

Commit bb8f28c

Browse files
author
Pearl Dsilva
committed
address comments
1 parent eaffcd7 commit bb8f28c

2 files changed

Lines changed: 116 additions & 11 deletions

File tree

‎server/src/main/java/com/cloud/storage/StorageManagerImpl.java‎

Lines changed: 46 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1314,17 +1314,17 @@ public PrimaryDataStoreInfo updateStoragePool(UpdateStoragePoolCmd cmd) throws I
13141314
throw new InvalidParameterValueException("Storage pool must be in Maintenance state before its URL can be changed. " +
13151315
"Please put the pool into maintenance first.");
13161316
}
1317-
URI newUri;
1318-
try {
1319-
newUri = new URI(cmd.getUrl());
1320-
} catch (URISyntaxException e) {
1321-
throw new InvalidParameterValueException("Invalid URL format: " + cmd.getUrl());
1322-
}
1323-
storagePool.setHostAddress(newUri.getHost());
1324-
storagePool.setPath(newUri.getPath());
1325-
if (newUri.getPort() != -1) {
1326-
storagePool.setPort(newUri.getPort());
1317+
Map<String, String> newUriParams = extractUriParamsAsMap(cmd.getUrl());
1318+
String newHost = newUriParams.get("host");
1319+
String newPath = newUriParams.get("hostPath");
1320+
if (StringUtils.isAnyBlank(newHost, newPath)) {
1321+
throw new InvalidParameterValueException("Invalid URL: host and path must not be empty. " +
1322+
"Expected format: nfs://hostname/path or gluster://hostname/volume");
13271323
}
1324+
storagePool.setHostAddress(newHost);
1325+
storagePool.setPath(newPath);
1326+
String newPortStr = newUriParams.get("port");
1327+
storagePool.setPort(newPortStr != null ? Integer.parseInt(newPortStr) : 0);
13281328
}
13291329
}
13301330
_storagePoolDao.update(id, storagePool);
@@ -1366,10 +1366,12 @@ private void propagateStoragePoolUrlChangeToHosts(StoragePoolVO updatedPool) {
13661366

13671367
logger.info("Propagating new URL for storage pool [{}] to {} connected host(s).", updatedPool.getName(), connectedHostIds.size());
13681368

1369+
List<String> failedHosts = new ArrayList<>();
13691370
for (Long hostId : connectedHostIds) {
13701371
HostVO host = _hostDao.findById(hostId);
13711372
if (host == null) {
13721373
logger.warn("Host [{}] not found; skipping URL propagation for pool [{}].", hostId, updatedPool.getName());
1374+
failedHosts.add("unknown-host-" + hostId);
13731375
continue;
13741376
}
13751377

@@ -1380,12 +1382,21 @@ private void propagateStoragePoolUrlChangeToHosts(StoragePoolVO updatedPool) {
13801382
String detail = (answer == null) ? "no answer returned" : answer.getDetails();
13811383
logger.warn("Failed to propagate new URL for storage pool [{}] to host [{}]: {}",
13821384
updatedPool.getName(), host.getName(), detail);
1385+
failedHosts.add(host.getName());
13831386
} else {
13841387
logger.info("Successfully propagated new URL for storage pool [{}] to host [{}].",
13851388
updatedPool.getName(), host.getName());
13861389
updateStoragePoolHostVOAndBytes(freshPool, hostId, (ModifyStoragePoolAnswer) answer);
13871390
}
13881391
}
1392+
1393+
if (!failedHosts.isEmpty()) {
1394+
throw new CloudRuntimeException(String.format(
1395+
"Storage pool [%s] URL was updated in the database, but the new mount could not be applied " +
1396+
"on the following host(s): %s. The pool may be in an inconsistent state; please verify " +
1397+
"connectivity and remount manually if necessary.",
1398+
updatedPool.getName(), String.join(", ", failedHosts)));
1399+
}
13891400
}
13901401

13911402
private void changeStoragePoolScopeToZone(StoragePoolVO primaryStorage) {
@@ -4220,10 +4231,14 @@ public ImageStore updateImageStoreStatus(Long id, String name, Boolean readonly,
42204231
throw new IllegalArgumentException("Unable to find image store with ID: " + id);
42214232
}
42224233
if (url != null) {
4223-
if (!imageStoreVO.isReadonly()) {
4234+
boolean willBeReadonly = (readonly != null && readonly) || imageStoreVO.isReadonly();
4235+
if (!willBeReadonly) {
42244236
throw new InvalidParameterValueException("Image store must be set to read-only (maintenance) state before its URL can be changed. " +
42254237
"Please set readOnly=true on the image store first.");
42264238
}
4239+
4240+
String existingProtocol = imageStoreVO.getProtocol();
4241+
validateUrl(url, existingProtocol);
42274242
imageStoreVO.setUrl(url);
42284243
}
42294244
if (com.cloud.utils.StringUtils.isNotBlank(name)) {
@@ -4239,6 +4254,26 @@ public ImageStore updateImageStoreStatus(Long id, String name, Boolean readonly,
42394254
return imageStoreVO;
42404255
}
42414256

4257+
private void validateUrl(String url, String existingProtocol) {
4258+
try {
4259+
UriUtils.UriInfo newUriInfo = UriUtils.getUriInfo(url);
4260+
String newScheme = newUriInfo.getScheme();
4261+
String newHost = newUriInfo.getStorageHost();
4262+
String newPath = newUriInfo.getStoragePath();
4263+
if (StringUtils.isAnyBlank(newScheme, newHost, newPath)) {
4264+
throw new InvalidParameterValueException("Invalid image store URL: scheme, host and path must not be empty.");
4265+
}
4266+
4267+
if (existingProtocol != null && !existingProtocol.equalsIgnoreCase(newScheme)) {
4268+
throw new InvalidParameterValueException(String.format(
4269+
"URL scheme '%s' does not match the image store's existing protocol '%s'.",
4270+
newScheme, existingProtocol));
4271+
}
4272+
} catch (CloudRuntimeException e) {
4273+
throw new InvalidParameterValueException("Invalid image store URL: " + e.getMessage());
4274+
}
4275+
}
4276+
42424277
@Override
42434278
public ImageStore updateImageStoreStatus(Long id, Boolean readonly) {
42444279
return updateImageStoreStatus(id, null, readonly, null, null);

‎server/src/test/java/com/cloud/storage/StorageManagerImplTest.java‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@
3030
import com.cloud.host.dao.HostDao;
3131
import com.cloud.resource.ResourceManager;
3232
import com.cloud.storage.dao.StoragePoolAndAccessGroupMapDao;
33+
import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
34+
import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
3335
import org.apache.cloudstack.api.ApiConstants;
3436
import org.apache.cloudstack.api.command.admin.storage.ChangeStoragePoolScopeCmd;
3537
import org.apache.cloudstack.api.command.admin.storage.ConfigureStorageAccessCmd;
@@ -170,6 +172,9 @@ public class StorageManagerImplTest {
170172
@Mock
171173
DataStoreManager dataStoreMgr;
172174

175+
@Mock
176+
ImageStoreDao imageStoreDao;
177+
173178
@Test
174179
public void createLocalStoragePoolName() {
175180
String hostMockName = "host1";
@@ -1716,4 +1721,69 @@ public void testDiscoverObjectStoreInitializationFailure() {
17161721

17171722
storageManagerImpl.discoverObjectStore(name, url, size, providerName, details);
17181723
}
1724+
1725+
private ImageStoreVO mockImageStoreVO(String protocol, boolean readonly) {
1726+
ImageStoreVO vo = Mockito.mock(ImageStoreVO.class);
1727+
Mockito.when(vo.getProtocol()).thenReturn(protocol);
1728+
Mockito.when(vo.isReadonly()).thenReturn(readonly);
1729+
return vo;
1730+
}
1731+
1732+
@Test(expected = InvalidParameterValueException.class)
1733+
public void testUpdateImageStoreStatus_rejectsUrlChangeWhenNotReadonly() {
1734+
long storeId = 1L;
1735+
ImageStoreVO vo = mockImageStoreVO("nfs", false);
1736+
Mockito.when(imageStoreDao.findById(storeId)).thenReturn(vo);
1737+
1738+
storageManagerImpl.updateImageStoreStatus(storeId, null, null, null, "nfs://newhost/newpath");
1739+
}
1740+
1741+
@Test
1742+
public void testUpdateImageStoreStatus_allowsUrlChangeWhenAlreadyReadonly() {
1743+
long storeId = 2L;
1744+
ImageStoreVO vo = mockImageStoreVO("nfs", true);
1745+
Mockito.when(imageStoreDao.findById(storeId)).thenReturn(vo);
1746+
Mockito.when(imageStoreDao.update(Mockito.eq(storeId), Mockito.any())).thenReturn(true);
1747+
1748+
ImageStore result = storageManagerImpl.updateImageStoreStatus(storeId, null, null, null, "nfs://newhost/newpath");
1749+
1750+
assertNotNull(result);
1751+
Mockito.verify(imageStoreDao).update(Mockito.eq(storeId), Mockito.any(ImageStoreVO.class));
1752+
}
1753+
1754+
@Test
1755+
public void testUpdateImageStoreStatus_allowsUrlChangeWhenReadonlySetInSameRequest() {
1756+
long storeId = 3L;
1757+
ImageStoreVO vo = mockImageStoreVO("nfs", false);
1758+
Mockito.when(imageStoreDao.findById(storeId)).thenReturn(vo);
1759+
Mockito.when(imageStoreDao.update(Mockito.eq(storeId), Mockito.any())).thenReturn(true);
1760+
1761+
ImageStore result = storageManagerImpl.updateImageStoreStatus(storeId, null, Boolean.TRUE, null, "nfs://newhost/newpath");
1762+
1763+
assertNotNull(result);
1764+
Mockito.verify(imageStoreDao).update(Mockito.eq(storeId), Mockito.any(ImageStoreVO.class));
1765+
}
1766+
1767+
@Test(expected = InvalidParameterValueException.class)
1768+
public void testUpdateImageStoreStatus_rejectsMalformedUrl() {
1769+
long storeId = 4L;
1770+
ImageStoreVO vo = mockImageStoreVO("nfs", true);
1771+
Mockito.when(imageStoreDao.findById(storeId)).thenReturn(vo);
1772+
1773+
storageManagerImpl.updateImageStoreStatus(storeId, null, null, null, "nfs://");
1774+
}
1775+
1776+
@Test(expected = InvalidParameterValueException.class)
1777+
public void testUpdateImageStoreStatus_rejectsMismatchedProtocol() {
1778+
long storeId = 5L;
1779+
ImageStoreVO vo = mockImageStoreVO("nfs", true);
1780+
Mockito.when(imageStoreDao.findById(storeId)).thenReturn(vo);
1781+
1782+
storageManagerImpl.updateImageStoreStatus(storeId, null, null, null, "http://somehost/path");
1783+
}
1784+
1785+
@Test(expected = InvalidParameterValueException.class)
1786+
public void testExtractUriParamsAsMap_rejectsMissingHostForNfs() {
1787+
storageManagerImpl.extractUriParamsAsMap("nfs:///just-a-path");
1788+
}
17191789
}

0 commit comments

Comments
 (0)