Skip to content

Commit 70583a4

Browse files
Pearl1594Pearl1594
committed
prevent snapshot backup for incremental clvm_ng snaps, fix build failure, add unit tests
1 parent 8ed2a3a commit 70583a4

File tree

5 files changed

+677
-12
lines changed

5 files changed

+677
-12
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ boolean copyPoliciesBetweenVolumesAndDestroySourceVolumeAfterMigration(ObjectInD
177177
* @param vmHostId VM's current host ID (or last host ID if stopped)
178178
* @return true if CLVM lock transfer is needed
179179
*/
180-
boolean isLockTransferRequired(VolumeInfo volumeToAttach, StoragePoolType volumePoolType, StoragePoolType vmPoolType,
180+
boolean isLockTransferRequired(VolumeInfo volumeToAttach, StoragePoolType volumePoolType, StoragePoolType vmPoolType,
181181
Long volumePoolId, Long vmPoolId, Long vmHostId);
182182

183183
/**
@@ -189,6 +189,6 @@ boolean isLockTransferRequired(VolumeInfo volumeToAttach, StoragePoolType volume
189189
* @param vmPoolPath Storage pool path for the VM
190190
* @return true if lightweight migration should be used
191191
*/
192-
boolean isLightweightMigrationNeeded(StoragePoolType volumePoolType, StoragePoolType vmPoolType,
192+
boolean isLightweightMigrationNeeded(StoragePoolType volumePoolType, StoragePoolType vmPoolType,
193193
String volumePoolPath, String vmPoolPath);
194194
}

‎engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2976,7 +2976,7 @@ public boolean transferVolumeLock(VolumeInfo volume, Long sourceHostId, Long des
29762976
return false;
29772977
}
29782978

2979-
logger.info("Transferring CLVM lock for volume {} (pool: {}) from host {} to host {}",
2979+
logger.info("Transferring CLVM lock for volume {} (pool: {}) from host {} to host {}",
29802980
volume.getUuid(), pool.getName(), sourceHostId, destHostId);
29812981

29822982
return clvmLockManager.transferClvmVolumeLock(volume.getUuid(), volume.getId(), volume.getPath(),
@@ -3000,7 +3000,7 @@ public Long findVolumeLockHost(VolumeInfo volume) {
30003000
if (instanceId != null) {
30013001
VMInstanceVO vmInstance = vmDao.findById(instanceId);
30023002
if (vmInstance != null && vmInstance.getHostId() != null) {
3003-
logger.debug("Volume {} is attached to VM {} on host {}",
3003+
logger.debug("Volume {} is attached to VM {} on host {}",
30043004
volume.getUuid(), vmInstance.getUuid(), vmInstance.getHostId());
30053005
return vmInstance.getHostId();
30063006
}
@@ -3012,7 +3012,7 @@ public Long findVolumeLockHost(VolumeInfo volume) {
30123012
if (hosts != null && !hosts.isEmpty()) {
30133013
for (HostVO host : hosts) {
30143014
if (host.getStatus() == com.cloud.host.Status.Up) {
3015-
logger.debug("Using fallback: first UP host {} in cluster {} for volume {}",
3015+
logger.debug("Using fallback: first UP host {} in cluster {} for volume {}",
30163016
host.getId(), pool.getClusterId(), volume.getUuid());
30173017
return host.getId();
30183018
}
@@ -3031,33 +3031,33 @@ public VolumeInfo performLockMigration(VolumeInfo volume, Long destHostId) {
30313031
}
30323032

30333033
String volumeUuid = volume.getUuid();
3034-
logger.info("Starting CLVM lock migration for volume {} (id: {}) to host {}",
3034+
logger.info("Starting CLVM lock migration for volume {} (id: {}) to host {}",
30353035
volumeUuid, volume.getUuid(), destHostId);
30363036

30373037
Long sourceHostId = findVolumeLockHost(volume);
30383038
if (sourceHostId == null) {
3039-
logger.warn("Could not determine source host for CLVM volume {} lock, assuming volume is not exclusively locked",
3039+
logger.warn("Could not determine source host for CLVM volume {} lock, assuming volume is not exclusively locked",
30403040
volumeUuid);
30413041
sourceHostId = destHostId;
30423042
}
30433043

30443044
if (sourceHostId.equals(destHostId)) {
3045-
logger.info("CLVM volume {} already has lock on destination host {}, no migration needed",
3045+
logger.info("CLVM volume {} already has lock on destination host {}, no migration needed",
30463046
volumeUuid, destHostId);
30473047
return volume;
30483048
}
30493049

3050-
logger.info("Migrating CLVM volume {} lock from host {} to host {}",
3050+
logger.info("Migrating CLVM volume {} lock from host {} to host {}",
30513051
volumeUuid, sourceHostId, destHostId);
30523052

30533053
boolean success = transferVolumeLock(volume, sourceHostId, destHostId);
30543054
if (!success) {
30553055
throw new CloudRuntimeException(
3056-
String.format("Failed to transfer CLVM lock for volume %s from host %s to host %s",
3056+
String.format("Failed to transfer CLVM lock for volume %s from host %s to host %s",
30573057
volumeUuid, sourceHostId, destHostId));
30583058
}
30593059

3060-
logger.info("Successfully migrated CLVM volume {} lock from host {} to host {}",
3060+
logger.info("Successfully migrated CLVM volume {} lock from host {} to host {}",
30613061
volumeUuid, sourceHostId, destHostId);
30623062

30633063
return volFactory.getVolume(volume.getId());

‎engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceImplClvmTest.java‎

Lines changed: 311 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,311 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
package org.apache.cloudstack.storage.volume;
18+
19+
import static org.junit.Assert.assertFalse;
20+
import static org.junit.Assert.assertTrue;
21+
import static org.mockito.Mockito.when;
22+
23+
import com.cloud.storage.ClvmLockManager;
24+
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
25+
import org.junit.Before;
26+
import org.junit.Test;
27+
import org.junit.runner.RunWith;
28+
import org.mockito.InjectMocks;
29+
import org.mockito.Mock;
30+
import org.mockito.Spy;
31+
import org.mockito.junit.MockitoJUnitRunner;
32+
33+
import com.cloud.storage.Storage.StoragePoolType;
34+
import com.cloud.storage.Volume;
35+
import com.cloud.storage.VolumeVO;
36+
import com.cloud.storage.dao.VolumeDao;
37+
38+
/**
39+
* Tests for CLVM lock management methods in VolumeServiceImpl.
40+
*/
41+
@RunWith(MockitoJUnitRunner.class)
42+
public class VolumeServiceImplClvmTest {
43+
44+
@Spy
45+
@InjectMocks
46+
private VolumeServiceImpl volumeService;
47+
48+
@Mock
49+
private VolumeDao volumeDao;
50+
51+
@Mock
52+
private VolumeInfo volumeInfoMock;
53+
54+
@Mock
55+
private VolumeVO volumeVOMock;
56+
57+
@Mock
58+
ClvmLockManager clvmLockManager;
59+
60+
private static final Long VOLUME_ID = 1L;
61+
private static final Long POOL_ID_1 = 100L;
62+
private static final Long POOL_ID_2 = 200L;
63+
private static final Long HOST_ID_1 = 10L;
64+
private static final Long HOST_ID_2 = 20L;
65+
private static final String POOL_PATH_VG1 = "/vg1";
66+
private static final String POOL_PATH_VG2 = "/vg2";
67+
68+
@Before
69+
public void setup() {
70+
when(volumeInfoMock.getId()).thenReturn(VOLUME_ID);
71+
when(volumeInfoMock.getUuid()).thenReturn("test-volume-uuid");
72+
}
73+
74+
@Test
75+
public void testAreBothPoolsClvmType_BothCLVM() {
76+
assertTrue(volumeService.areBothPoolsClvmType(StoragePoolType.CLVM, StoragePoolType.CLVM));
77+
}
78+
79+
@Test
80+
public void testAreBothPoolsClvmType_BothCLVM_NG() {
81+
assertTrue(volumeService.areBothPoolsClvmType(StoragePoolType.CLVM_NG, StoragePoolType.CLVM_NG));
82+
}
83+
84+
@Test
85+
public void testAreBothPoolsClvmType_MixedCLVMAndCLVM_NG() {
86+
assertTrue(volumeService.areBothPoolsClvmType(StoragePoolType.CLVM, StoragePoolType.CLVM_NG));
87+
assertTrue(volumeService.areBothPoolsClvmType(StoragePoolType.CLVM_NG, StoragePoolType.CLVM));
88+
}
89+
90+
@Test
91+
public void testAreBothPoolsClvmType_OneCLVMOneNFS() {
92+
assertFalse(volumeService.areBothPoolsClvmType(StoragePoolType.CLVM, StoragePoolType.NetworkFilesystem));
93+
assertFalse(volumeService.areBothPoolsClvmType(StoragePoolType.NetworkFilesystem, StoragePoolType.CLVM));
94+
}
95+
96+
@Test
97+
public void testAreBothPoolsClvmType_OneCLVM_NGOneNFS() {
98+
assertFalse(volumeService.areBothPoolsClvmType(StoragePoolType.CLVM_NG, StoragePoolType.NetworkFilesystem));
99+
assertFalse(volumeService.areBothPoolsClvmType(StoragePoolType.NetworkFilesystem, StoragePoolType.CLVM_NG));
100+
}
101+
102+
@Test
103+
public void testAreBothPoolsClvmType_BothNFS() {
104+
assertFalse(volumeService.areBothPoolsClvmType(StoragePoolType.NetworkFilesystem, StoragePoolType.NetworkFilesystem));
105+
}
106+
107+
@Test
108+
public void testAreBothPoolsClvmType_NullVolumePoolType() {
109+
assertFalse(volumeService.areBothPoolsClvmType(null, StoragePoolType.CLVM));
110+
}
111+
112+
@Test
113+
public void testAreBothPoolsClvmType_NullVmPoolType() {
114+
assertFalse(volumeService.areBothPoolsClvmType(StoragePoolType.CLVM, null));
115+
}
116+
117+
@Test
118+
public void testAreBothPoolsClvmType_BothNull() {
119+
assertFalse(volumeService.areBothPoolsClvmType(null, null));
120+
}
121+
122+
123+
@Test
124+
public void testIsLockTransferRequired_NonCLVMPool() {
125+
assertFalse(volumeService.isLockTransferRequired(
126+
volumeInfoMock, StoragePoolType.NetworkFilesystem, StoragePoolType.CLVM,
127+
POOL_ID_1, POOL_ID_1, HOST_ID_1));
128+
}
129+
130+
@Test
131+
public void testIsLockTransferRequired_DifferentPools() {
132+
assertFalse(volumeService.isLockTransferRequired(
133+
volumeInfoMock, StoragePoolType.CLVM, StoragePoolType.CLVM,
134+
POOL_ID_1, POOL_ID_2, HOST_ID_1));
135+
}
136+
137+
@Test
138+
public void testIsLockTransferRequired_NullPoolIds() {
139+
assertFalse(volumeService.isLockTransferRequired(
140+
volumeInfoMock, StoragePoolType.CLVM, StoragePoolType.CLVM,
141+
null, POOL_ID_1, HOST_ID_1));
142+
143+
assertFalse(volumeService.isLockTransferRequired(
144+
volumeInfoMock, StoragePoolType.CLVM, StoragePoolType.CLVM,
145+
POOL_ID_1, null, HOST_ID_1));
146+
}
147+
148+
@Test
149+
public void testIsLockTransferRequired_DetachedVolumeReady() {
150+
when(volumeDao.findById(VOLUME_ID)).thenReturn(volumeVOMock);
151+
when(volumeVOMock.getState()).thenReturn(Volume.State.Ready);
152+
when(volumeVOMock.getInstanceId()).thenReturn(null); // Detached
153+
154+
when(volumeService.findVolumeLockHost(volumeInfoMock)).thenReturn(null);
155+
156+
assertTrue(volumeService.isLockTransferRequired(
157+
volumeInfoMock, StoragePoolType.CLVM, StoragePoolType.CLVM,
158+
POOL_ID_1, POOL_ID_1, HOST_ID_1));
159+
}
160+
161+
@Test
162+
public void testIsLockTransferRequired_DetachedVolumeNotReady() {
163+
when(volumeDao.findById(VOLUME_ID)).thenReturn(volumeVOMock);
164+
when(volumeVOMock.getState()).thenReturn(Volume.State.Allocated);
165+
166+
when(volumeService.findVolumeLockHost(volumeInfoMock)).thenReturn(null);
167+
168+
assertFalse(volumeService.isLockTransferRequired(
169+
volumeInfoMock, StoragePoolType.CLVM, StoragePoolType.CLVM,
170+
POOL_ID_1, POOL_ID_1, HOST_ID_1));
171+
}
172+
173+
@Test
174+
public void testIsLockTransferRequired_DifferentHosts() {
175+
when(volumeService.findVolumeLockHost(volumeInfoMock)).thenReturn(HOST_ID_1);
176+
177+
assertTrue(volumeService.isLockTransferRequired(
178+
volumeInfoMock, StoragePoolType.CLVM, StoragePoolType.CLVM,
179+
POOL_ID_1, POOL_ID_1, HOST_ID_2));
180+
}
181+
182+
@Test
183+
public void testIsLockTransferRequired_SameHost() {
184+
when(volumeService.findVolumeLockHost(volumeInfoMock)).thenReturn(HOST_ID_1);
185+
186+
assertFalse(volumeService.isLockTransferRequired(
187+
volumeInfoMock, StoragePoolType.CLVM, StoragePoolType.CLVM,
188+
POOL_ID_1, POOL_ID_1, HOST_ID_1));
189+
}
190+
191+
@Test
192+
public void testIsLockTransferRequired_NullVmHostId() {
193+
when(volumeService.findVolumeLockHost(volumeInfoMock)).thenReturn(HOST_ID_1);
194+
195+
assertFalse(volumeService.isLockTransferRequired(
196+
volumeInfoMock, StoragePoolType.CLVM, StoragePoolType.CLVM,
197+
POOL_ID_1, POOL_ID_1, null));
198+
}
199+
200+
@Test
201+
public void testIsLockTransferRequired_CLVM_NG_DifferentHosts() {
202+
when(volumeService.findVolumeLockHost(volumeInfoMock)).thenReturn(HOST_ID_1);
203+
204+
assertTrue(volumeService.isLockTransferRequired(
205+
volumeInfoMock, StoragePoolType.CLVM_NG, StoragePoolType.CLVM_NG,
206+
POOL_ID_1, POOL_ID_1, HOST_ID_2));
207+
}
208+
209+
@Test
210+
public void testIsLightweightMigrationNeeded_NonCLVMPools() {
211+
assertFalse(volumeService.isLightweightMigrationNeeded(
212+
StoragePoolType.NetworkFilesystem, StoragePoolType.NetworkFilesystem,
213+
POOL_PATH_VG1, POOL_PATH_VG1));
214+
}
215+
216+
@Test
217+
public void testIsLightweightMigrationNeeded_OneCLVMOneNFS() {
218+
assertFalse(volumeService.isLightweightMigrationNeeded(
219+
StoragePoolType.CLVM, StoragePoolType.NetworkFilesystem,
220+
POOL_PATH_VG1, POOL_PATH_VG1));
221+
}
222+
223+
@Test
224+
public void testIsLightweightMigrationNeeded_SameVG() {
225+
assertTrue(volumeService.isLightweightMigrationNeeded(
226+
StoragePoolType.CLVM, StoragePoolType.CLVM,
227+
"/vg1", "/vg1"));
228+
}
229+
230+
@Test
231+
public void testIsLightweightMigrationNeeded_SameVG_NoSlash() {
232+
assertTrue(volumeService.isLightweightMigrationNeeded(
233+
StoragePoolType.CLVM, StoragePoolType.CLVM,
234+
"vg1", "vg1"));
235+
}
236+
237+
@Test
238+
public void testIsLightweightMigrationNeeded_SameVG_MixedSlash() {
239+
assertTrue(volumeService.isLightweightMigrationNeeded(
240+
StoragePoolType.CLVM, StoragePoolType.CLVM,
241+
"/vg1", "vg1"));
242+
243+
assertTrue(volumeService.isLightweightMigrationNeeded(
244+
StoragePoolType.CLVM, StoragePoolType.CLVM,
245+
"vg1", "/vg1"));
246+
}
247+
248+
@Test
249+
public void testIsLightweightMigrationNeeded_DifferentVG() {
250+
assertFalse(volumeService.isLightweightMigrationNeeded(
251+
StoragePoolType.CLVM, StoragePoolType.CLVM,
252+
"/vg1", "/vg2"));
253+
}
254+
255+
@Test
256+
public void testIsLightweightMigrationNeeded_CLVM_NG_SameVG() {
257+
assertTrue(volumeService.isLightweightMigrationNeeded(
258+
StoragePoolType.CLVM_NG, StoragePoolType.CLVM_NG,
259+
"/vg1", "/vg1"));
260+
}
261+
262+
@Test
263+
public void testIsLightweightMigrationNeeded_CLVM_NG_DifferentVG() {
264+
assertFalse(volumeService.isLightweightMigrationNeeded(
265+
StoragePoolType.CLVM_NG, StoragePoolType.CLVM_NG,
266+
"/vg1", "/vg2"));
267+
}
268+
269+
@Test
270+
public void testIsLightweightMigrationNeeded_MixedCLVM_CLVM_NG_SameVG() {
271+
assertTrue(volumeService.isLightweightMigrationNeeded(
272+
StoragePoolType.CLVM, StoragePoolType.CLVM_NG,
273+
"/vg1", "/vg1"));
274+
275+
assertTrue(volumeService.isLightweightMigrationNeeded(
276+
StoragePoolType.CLVM_NG, StoragePoolType.CLVM,
277+
"/vg1", "/vg1"));
278+
}
279+
280+
@Test
281+
public void testIsLightweightMigrationNeeded_NullVolumePath() {
282+
assertFalse(volumeService.isLightweightMigrationNeeded(
283+
StoragePoolType.CLVM, StoragePoolType.CLVM,
284+
null, "/vg1"));
285+
}
286+
287+
@Test
288+
public void testIsLightweightMigrationNeeded_NullVmPath() {
289+
assertFalse(volumeService.isLightweightMigrationNeeded(
290+
StoragePoolType.CLVM, StoragePoolType.CLVM,
291+
"/vg1", null));
292+
}
293+
294+
@Test
295+
public void testIsLightweightMigrationNeeded_BothPathsNull() {
296+
assertFalse(volumeService.isLightweightMigrationNeeded(
297+
StoragePoolType.CLVM, StoragePoolType.CLVM,
298+
null, null));
299+
}
300+
301+
@Test
302+
public void testIsLightweightMigrationNeeded_ComplexVGNames() {
303+
assertTrue(volumeService.isLightweightMigrationNeeded(
304+
StoragePoolType.CLVM, StoragePoolType.CLVM,
305+
"/cloudstack-vg-01", "/cloudstack-vg-01"));
306+
307+
assertFalse(volumeService.isLightweightMigrationNeeded(
308+
StoragePoolType.CLVM, StoragePoolType.CLVM,
309+
"/cloudstack-vg-01", "/cloudstack-vg-02"));
310+
}
311+
}

0 commit comments

Comments
 (0)