Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ public class PrimaryDataStoreTO implements DataStoreTO {
private Boolean diskProvisioningStrictnessFlag;
private final boolean isManaged;

private final StoragePoolType parentPoolType;

public PrimaryDataStoreTO(PrimaryDataStore dataStore) {
this.uuid = dataStore.getUuid();
this.name = dataStore.getName();
Expand All @@ -66,6 +68,7 @@ public PrimaryDataStoreTO(PrimaryDataStore dataStore) {
this.url = dataStore.getUri();
this.details = dataStore.getDetails();
this.isManaged = dataStore.isManaged();
this.parentPoolType = dataStore.getParentPoolType();
}

public long getId() {
Expand Down Expand Up @@ -172,4 +175,8 @@ public Boolean getDiskProvisioningStrictnessFlag() {
public void setDiskProvisioningStrictnessFlag(Boolean diskProvisioningStrictnessFlag) {
this.diskProvisioningStrictnessFlag = diskProvisioningStrictnessFlag;
}

public StoragePoolType getParentPoolType() {
return parentPoolType;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

import org.apache.cloudstack.engine.subsystem.api.storage.disktype.DiskFormat;

import com.cloud.storage.Storage;

public interface PrimaryDataStore extends DataStore, PrimaryDataStoreInfo {
DataObject create(DataObject dataObject, String configuration);

Expand All @@ -38,4 +40,6 @@ public interface PrimaryDataStore extends DataStore, PrimaryDataStoreInfo {
SnapshotInfo getSnapshot(long snapshotId);

DiskFormat getDefaultDiskType();

Storage.StoragePoolType getParentPoolType();
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@
import javax.inject.Inject;
import javax.naming.ConfigurationException;

import com.cloud.event.UsageEventVO;
import org.apache.log4j.Logger;

import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotOptions;
import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotStrategy;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.to.VolumeObjectTO;
import org.apache.commons.lang3.StringUtils;
import org.apache.log4j.Logger;

import com.cloud.agent.AgentManager;
import com.cloud.agent.api.Answer;
Expand All @@ -45,6 +45,7 @@
import com.cloud.agent.api.VMSnapshotTO;
import com.cloud.event.EventTypes;
import com.cloud.event.UsageEventUtils;
import com.cloud.event.UsageEventVO;
import com.cloud.exception.AgentUnavailableException;
import com.cloud.exception.OperationTimedoutException;
import com.cloud.host.HostVO;
Expand All @@ -53,6 +54,7 @@
import com.cloud.storage.GuestOSHypervisorVO;
import com.cloud.storage.GuestOSVO;
import com.cloud.storage.Storage.ImageFormat;
import com.cloud.storage.StoragePool;
import com.cloud.storage.VolumeVO;
import com.cloud.storage.dao.DiskOfferingDao;
import com.cloud.storage.dao.GuestOSDao;
Expand Down Expand Up @@ -97,6 +99,8 @@ public class DefaultVMSnapshotStrategy extends ManagerBase implements VMSnapshot
DiskOfferingDao diskOfferingDao;
@Inject
HostDao hostDao;
@Inject
PrimaryDataStoreDao primaryDataStoreDao;

@Override
public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
Expand Down Expand Up @@ -323,14 +327,26 @@ protected void finalizeRevert(VMSnapshotVO vmSnapshot, List<VolumeObjectTO> volu
vmSnapshotDao.persist(vmSnapshot);
}

private void updateVolumePath(List<VolumeObjectTO> volumeTOs) {
protected void updateVolumePath(List<VolumeObjectTO> volumeTOs) {
for (VolumeObjectTO volume : volumeTOs) {
if (volume.getPath() != null) {
VolumeVO volumeVO = volumeDao.findById(volume.getId());
if (StringUtils.isAllEmpty(volume.getDataStoreUuid(), volume.getPath(), volume.getChainInfo())) {
continue;
}
VolumeVO volumeVO = volumeDao.findById(volume.getId());
if (StringUtils.isNotEmpty(volume.getDataStoreUuid())) {
StoragePool pool = primaryDataStoreDao.findPoolByUUID(volume.getDataStoreUuid());
if (pool != null && pool.getId() != volumeVO.getPoolId()) {
volumeVO.setPoolId(pool.getId());
}
}
if (StringUtils.isNotEmpty(volume.getPath())) {
volumeVO.setPath(volume.getPath());
volumeVO.setVmSnapshotChainSize(volume.getSize());
volumeDao.persist(volumeVO);
}
if (StringUtils.isNotEmpty(volume.getChainInfo())) {
volumeVO.setChainInfo(volume.getChainInfo());
}
volumeVO.setVmSnapshotChainSize(volume.getSize());
volumeDao.persist(volumeVO);
Comment on lines +332 to +349

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More of a general remark than about this piece of code; can these lines be extracted and tested in unit tests. (extracted is maybe not needed in this case, but unit tests would be nice)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland added unit test

}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.cloudstack.storage.vmsnapshot;

import java.util.ArrayList;
import java.util.List;
import java.util.UUID;

import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
import org.apache.cloudstack.storage.to.VolumeObjectTO;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.Spy;
import org.mockito.junit.MockitoJUnitRunner;
import org.mockito.stubbing.Answer;

import com.cloud.storage.Storage;
import com.cloud.storage.Volume;
import com.cloud.storage.VolumeVO;
import com.cloud.storage.dao.VolumeDao;

@RunWith(MockitoJUnitRunner.class)
public class DefaultVMSnapshotStrategyTest {
@Mock
VolumeDao volumeDao;
@Mock
PrimaryDataStoreDao primaryDataStoreDao;

@Spy
@InjectMocks
private final DefaultVMSnapshotStrategy defaultVMSnapshotStrategy = new DefaultVMSnapshotStrategy();

protected List<VolumeVO> persistedVolumes = new ArrayList<>();


private void setupVolumeDaoPersistMock() {
persistedVolumes.clear();
Mockito.when(volumeDao.persist(Mockito.any())).thenAnswer((Answer<VolumeVO>) invocation -> {
VolumeVO volume = (VolumeVO)invocation.getArguments()[0];
persistedVolumes.add(volume);
return volume;
});
}

@Test
public void testUpdateVolumePath() {
setupVolumeDaoPersistMock();
VolumeObjectTO vol1 = Mockito.mock(VolumeObjectTO.class);
Mockito.when(vol1.getDataStoreUuid()).thenReturn(null);
Mockito.when(vol1.getPath()).thenReturn(null);
Mockito.when(vol1.getChainInfo()).thenReturn(null);
VolumeObjectTO vol2 = Mockito.mock(VolumeObjectTO.class);
Long volumeId = 1L;
String newDSUuid = UUID.randomUUID().toString();
String oldVolPath = "old";
String newVolPath = "new";
String oldVolChain = "old-chain";
String newVolChain = "new-chain";
Long vmSnapshotChainSize = 1000L;
Long oldPoolId = 1L;
Long newPoolId = 2L;
Mockito.when(vol2.getDataStoreUuid()).thenReturn(newDSUuid);
Mockito.when(vol2.getPath()).thenReturn(newVolPath);
Mockito.when(vol2.getChainInfo()).thenReturn(newVolChain);
Mockito.when(vol2.getSize()).thenReturn(vmSnapshotChainSize);
Mockito.when(vol2.getId()).thenReturn(volumeId);
VolumeVO volumeVO = new VolumeVO("name", 0l, 0l, 0l, 0l, 0l, "folder", "path", Storage.ProvisioningType.THIN, 0l, Volume.Type.ROOT);
volumeVO.setPoolId(oldPoolId);
volumeVO.setChainInfo(oldVolChain);
volumeVO.setPath(oldVolPath);
Mockito.when(volumeDao.findById(volumeId)).thenReturn(volumeVO);
StoragePoolVO storagePoolVO = Mockito.mock(StoragePoolVO.class);
Mockito.when(storagePoolVO.getId()).thenReturn(newPoolId);
Mockito.when(primaryDataStoreDao.findPoolByUUID(newDSUuid)).thenReturn(storagePoolVO);
Mockito.when(volumeDao.findById(volumeId)).thenReturn(volumeVO);
defaultVMSnapshotStrategy.updateVolumePath(List.of(vol1, vol2));
Assert.assertEquals(1, persistedVolumes.size());
VolumeVO persistedVolume = persistedVolumes.get(0);
Assert.assertNotNull(persistedVolume);
Assert.assertEquals(newPoolId, persistedVolume.getPoolId());
Assert.assertEquals(newVolPath, persistedVolume.getPath());
Assert.assertEquals(vmSnapshotChainSize, persistedVolume.getVmSnapshotChainSize());
Assert.assertEquals(newVolChain, persistedVolume.getChainInfo());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import org.apache.cloudstack.engine.subsystem.api.storage.VMSnapshotStrategy;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.to.VolumeObjectTO;
import org.apache.cloudstack.test.utils.SpringUtils;
import org.junit.Before;
Expand Down Expand Up @@ -92,6 +93,8 @@ public class VMSnapshotStrategyTest extends TestCase {
VMSnapshotDao vmSnapshotDao;
@Inject
HostDao hostDao;
@Inject
PrimaryDataStoreDao primaryDataStoreDao;

@Override
@Before
Expand Down Expand Up @@ -304,5 +307,10 @@ public DiskOfferingDao diskOfferingDao() {
public HostDao hostDao() {
return Mockito.mock(HostDao.class);
}

@Bean
public PrimaryDataStoreDao primaryDataStoreDao() {
return Mockito.mock(PrimaryDataStoreDao.class);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public class PrimaryDataStoreImpl implements PrimaryDataStore {

protected PrimaryDataStoreDriver driver;
protected StoragePoolVO pdsv;
protected StoragePoolVO parentStoragePool;
@Inject
protected PrimaryDataStoreDao dataStoreDao;
protected PrimaryDataStoreLifeCycle lifeCycle;
Expand Down Expand Up @@ -99,6 +100,9 @@ public void configure(StoragePoolVO pdsv, PrimaryDataStoreDriver driver, DataSto
this.pdsv = pdsv;
this.driver = driver;
this.provider = provider;
if (pdsv.getParent() != null && pdsv.getParent() > 0L) {
this.parentStoragePool = dataStoreDao.findById(pdsv.getParent());
}
}

public static PrimaryDataStoreImpl createDataStore(StoragePoolVO pdsv, PrimaryDataStoreDriver driver, DataStoreProvider provider) {
Expand Down Expand Up @@ -447,4 +451,12 @@ public DataStoreTO getTO() {
}
return to;
}

@Override
public StoragePoolType getParentPoolType() {
if (this.parentStoragePool != null) {
return this.parentStoragePool.getPoolType();
}
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.cloud.hypervisor.vmware.mo.DatastoreMO;
import com.cloud.hypervisor.vmware.mo.VmwareHypervisorHost;
import com.cloud.hypervisor.vmware.util.VmwareContext;
import com.cloud.storage.resource.VmwareStorageProcessor;

public interface VmwareHostService {
VmwareContext getServiceContext(Command cmd);
Expand All @@ -31,4 +32,6 @@ public interface VmwareHostService {
String getWorkerName(VmwareContext context, Command cmd, int workerSequence, DatastoreMO dsMo) throws Exception;

String createLogMessageException(Throwable e, Command command);

VmwareStorageProcessor getStorageProcessor();
}
Loading