Skip to content

Commit 939e488

Browse files
GaOrtigaGaOrtiga
committed
address sureshanaparti's review
1 parent d46b59e commit 939e488

6 files changed

Lines changed: 42 additions & 20 deletions

File tree

‎engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreManager.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,4 +60,6 @@ public interface DataStoreManager {
6060
DataStore getImageStoreByUuid(String uuid);
6161

6262
Long getStoreZoneId(long storeId, DataStoreRole role);
63+
64+
boolean isRemovedOrReadonly(DataStore store);
6365
}

‎engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ public class TemplateServiceImpl implements TemplateService {
165165
@Inject
166166
StorageOrchestrationService storageOrchestrator;
167167
@Inject
168-
ImageStoreDao dataStoreDao;
168+
ImageStoreDao imageStoreDao;
169169

170170
class TemplateOpContext<T> extends AsyncRpcContext<T> {
171171
final TemplateObject template;
@@ -298,9 +298,7 @@ public void handleSysTemplateDownload(HypervisorType hostHyper, Long dcId) {
298298
}
299299

300300
protected boolean shouldDownloadTemplateToStore(VMTemplateVO template, DataStore store) {
301-
if (dataStoreDao.findById(store.getId()).isReadonly()) {
302-
logger.debug("Template [{}] will not be downloaded to image store [{}] because this store is marked as read-only.", template.getUniqueName(),
303-
store.getName());
301+
if (_storeMgr.isRemovedOrReadonly(store)) {
304302
return false;
305303
}
306304

‎engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -107,10 +107,10 @@ public class TemplateServiceImplTest {
107107
DataCenterDao _dcDao;
108108

109109
@Mock
110-
ImageStoreDao imageStore;
110+
ImageStoreDao imageStoreDao;
111111

112112
@Mock
113-
ImageStoreVO imageMock;
113+
ImageStoreVO imageStoreMock;
114114

115115
Map<String, TemplateProp> templatesInSourceStore = new HashMap<>();
116116

@@ -127,55 +127,55 @@ public void setUp() {
127127
Mockito.doReturn(templateInfoMock).when(templateDataFactoryMock).getTemplate(2L, sourceStoreMock);
128128
Mockito.doReturn(3L).when(dataStoreMock).getId();
129129
Mockito.doReturn(zoneScopeMock).when(dataStoreMock).getScope();
130-
Mockito.when(imageStore.findById(3L)).thenReturn(imageMock);
130+
Mockito.when(imageStoreDao.findById(3L)).thenReturn(imageStoreMock);
131131
}
132132

133133
@Test
134134
public void shouldDownloadTemplateToStoreTestSkipsTemplateDirectedToAnotherStorage() {
135135
DataStore destinedStore = Mockito.mock(DataStore.class);
136-
Mockito.when(imageMock.isReadonly()).thenReturn(false);
136+
Mockito.when(imageStoreMock.isReadonly()).thenReturn(false);
137137
Mockito.doReturn(dataStoreMock.getId() + 1L).when(destinedStore).getId();
138138
Mockito.when(templateManagerMock.verifyHeuristicRulesForZone(tmpltMock, zoneScopeMock.getScopeId())).thenReturn(destinedStore);
139139
Assert.assertFalse(templateService.shouldDownloadTemplateToStore(tmpltMock, dataStoreMock));
140140
}
141141

142142
@Test
143143
public void shouldDownloadTemplateToStoreTestDownloadsPublicTemplate() {
144-
Mockito.when(imageMock.isReadonly()).thenReturn(false);
144+
Mockito.when(imageStoreMock.isReadonly()).thenReturn(false);
145145
Mockito.when(tmpltMock.isPublicTemplate()).thenReturn(true);
146146
Assert.assertTrue(templateService.shouldDownloadTemplateToStore(tmpltMock, dataStoreMock));
147147
}
148148

149149
@Test
150150
public void shouldDownloadTemplateToStoreTestDownloadsFeaturedTemplate() {
151-
Mockito.when(imageMock.isReadonly()).thenReturn(false);
151+
Mockito.when(imageStoreMock.isReadonly()).thenReturn(false);
152152
Mockito.when(tmpltMock.isFeatured()).thenReturn(true);
153153
Assert.assertTrue(templateService.shouldDownloadTemplateToStore(tmpltMock, dataStoreMock));
154154
}
155155

156156
@Test
157157
public void shouldDownloadTemplateToStoreTestDownloadsSystemTemplate() {
158-
Mockito.when(imageMock.isReadonly()).thenReturn(false);
158+
Mockito.when(imageStoreMock.isReadonly()).thenReturn(false);
159159
Mockito.when(tmpltMock.getTemplateType()).thenReturn(Storage.TemplateType.SYSTEM);
160160
Assert.assertTrue(templateService.shouldDownloadTemplateToStore(tmpltMock, dataStoreMock));
161161
}
162162

163163
@Test
164164
public void shouldDownloadTemplateToStoreTestDownloadsPrivateNoRefTemplate() {
165-
Mockito.when(imageMock.isReadonly()).thenReturn(false);
165+
Mockito.when(imageStoreMock.isReadonly()).thenReturn(false);
166166
Assert.assertTrue(templateService.shouldDownloadTemplateToStore(tmpltMock, dataStoreMock));
167167
}
168168

169169
@Test
170170
public void shouldDownloadTemplateToStoreTestSkipsPrivateExistingTemplate() {
171-
Mockito.when(imageMock.isReadonly()).thenReturn(false);
171+
Mockito.when(imageStoreMock.isReadonly()).thenReturn(false);
172172
Mockito.when(templateDataStoreDao.findByTemplateZone(tmpltMock.getId(), zoneScopeMock.getScopeId(), DataStoreRole.Image)).thenReturn(Mockito.mock(TemplateDataStoreVO.class));
173173
Assert.assertFalse(templateService.shouldDownloadTemplateToStore(tmpltMock, dataStoreMock));
174174
}
175175

176176
@Test
177177
public void shouldDownloadTemplateToStoreTestSkipsWhenStorageIsReadOnly() {
178-
Mockito.when(imageMock.isReadonly()).thenReturn(true);
178+
Mockito.when(imageStoreMock.isReadonly()).thenReturn(true);
179179
Assert.assertFalse(templateService.shouldDownloadTemplateToStore(tmpltMock, dataStoreMock));
180180

181181
}

‎engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/DataStoreManagerImpl.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@
2626
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
2727
import org.apache.cloudstack.engine.subsystem.api.storage.ZoneScope;
2828
import org.apache.cloudstack.engine.subsystem.api.storage.Scope;
29+
import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
30+
import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
2931
import org.apache.cloudstack.storage.object.datastore.ObjectStoreProviderManager;
32+
import org.apache.logging.log4j.LogManager;
33+
import org.apache.logging.log4j.Logger;
3034
import org.springframework.stereotype.Component;
3135

3236
import org.apache.cloudstack.storage.image.datastore.ImageStoreProviderManager;
@@ -37,12 +41,16 @@
3741

3842
@Component
3943
public class DataStoreManagerImpl implements DataStoreManager {
44+
protected Logger logger = LogManager.getLogger(DataStoreManagerImpl.class);
45+
4046
@Inject
4147
PrimaryDataStoreProviderManager primaryStoreMgr;
4248
@Inject
4349
ImageStoreProviderManager imageDataStoreMgr;
4450
@Inject
4551
ObjectStoreProviderManager objectStoreProviderMgr;
52+
@Inject
53+
ImageStoreDao imageStoreDao;
4654

4755
@Override
4856
public DataStore getDataStore(long storeId, DataStoreRole role) {
@@ -199,4 +207,18 @@ public Long getStoreZoneId(long storeId, DataStoreRole role) {
199207
} catch (CloudRuntimeException ignored) {}
200208
return null;
201209
}
210+
211+
@Override
212+
public boolean isRemovedOrReadonly(DataStore store) {
213+
ImageStoreVO storeVO = imageStoreDao.findById(store.getId());
214+
if (storeVO == null) {
215+
logger.debug("Could not find image store with id [{}], skipping it.", store.getId());
216+
return true;
217+
}
218+
if (storeVO.isReadonly()) {
219+
logger.debug("Image store [{}] is read-only, skipping it.", storeVO);
220+
return true;
221+
}
222+
return false;
223+
}
202224
}

‎engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ public abstract class BaseImageStoreDriverImpl implements ImageStoreDriver {
128128
@Inject
129129
DataStoreManager dataStoreManager;
130130
@Inject
131-
ImageStoreDao dataStoreDao;
131+
ImageStoreDao imageStoreDao;
132132

133133
protected String _proxy = null;
134134

@@ -178,9 +178,9 @@ public void createAsync(DataStore dataStore, DataObject data, AsyncCompletionCal
178178
AsyncCallbackDispatcher<BaseImageStoreDriverImpl, DownloadAnswer> caller = AsyncCallbackDispatcher.create(this);
179179
caller.setContext(context);
180180
if (data.getType() == DataObjectType.TEMPLATE) {
181-
if (dataStoreDao.findById(dataStore.getId()).isReadonly()) {
182-
logger.debug("Template [{}] will not be downloaded to image store [{}] because this store is marked as read-only.", data.getName(),
183-
dataStore.getName());
181+
if (dataStoreManager.isRemovedOrReadonly(dataStore)) {
182+
DownloadAnswer ans = new DownloadAnswer("Data store is removed or in read-only mode", VMTemplateStorageResourceAssoc.Status.UNKNOWN);
183+
caller.complete(ans);
184184
return;
185185
}
186186
caller.setCallback(caller.getTarget().createTemplateAsyncCallback(null, null));

‎server/src/test/java/com/cloud/template/HypervisorTemplateAdapterTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -526,13 +526,13 @@ public void isZoneAndImageStoreAvailableTestTemplateIsPrivateAndItIsAlreadyAlloc
526526
@Test
527527
public void isZoneAndImageStoreAvailableTestTemplateIsPrivateAndItIsNotAlreadyAllocatedToTheSameZoneShouldReturnTrue() {
528528
DataStore dataStoreMock = Mockito.mock(DataStore.class);
529-
ImageStoreVO ImageStoreVOMock = Mockito.mock(ImageStoreVO.class);
529+
ImageStoreVO imageStoreVoMock = Mockito.mock(ImageStoreVO.class);
530530
Long zoneId = 1L;
531531
Set<Long> zoneSet = new HashSet<>();
532532
boolean isTemplatePrivate = true;
533533
DataCenterVO dataCenterVOMock = Mockito.mock(DataCenterVO.class);
534534

535-
Mockito.when(_imgStoreDao.findById(Mockito.anyLong())).thenReturn(ImageStoreVOMock);
535+
Mockito.when(_imgStoreDao.findById(Mockito.anyLong())).thenReturn(imageStoreVoMock);
536536
Mockito.when(_dcDao.findById(Mockito.anyLong())).thenReturn(dataCenterVOMock);
537537
Mockito.when(dataCenterVOMock.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled);
538538
Mockito.when(statsCollectorMock.imageStoreHasEnoughCapacity(any(DataStore.class))).thenReturn(true);

0 commit comments

Comments
 (0)