winterhazel commented on code in PR #13053:
URL: https://github.com/apache/cloudstack/pull/13053#discussion_r3173793507
##########
.github/workflows/daily-repo-status.lock.yml:
##########
@@ -1,1022 +0,0 @@
-#
Review Comment:
Please remove this change from the PR
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -325,6 +327,11 @@ public StrategyPriority canHandle(Long vmId, Long
rootPoolId, boolean snapshotMe
List<VolumeVO> volumes = volumeDao.findByInstance(vmId);
for (VolumeVO volume : volumes) {
StoragePoolVO storagePoolVO =
storagePool.findById(volume.getPoolId());
+ if (storagePoolVO.isManaged() &&
ONTAP_PROVIDER_NAME.equals(storagePoolVO.getStorageProviderName())) {
+ logger.debug(String.format("%s as the VM has a volume on ONTAP
managed storage pool [%s]. " +
+ "ONTAP managed storage has its own dedicated VM
snapshot strategy.", cantHandleLog, storagePoolVO.getName()));
Review Comment:
```suggestion
logger.debug("{} as the VM has a volume on ONTAP managed
storage pool [{}]. " +
"ONTAP managed storage has its own dedicated VM
snapshot strategy.", cantHandleLog, storagePoolVO.getName());
```
Replace String.format occurrences with parametrized log messages
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/CloudStackVolume.java:
##########
@@ -56,4 +75,14 @@ public DataObject getVolumeInfo() {
public void setVolumeInfo(DataObject volumeInfo) {
this.volumeInfo = volumeInfo;
}
+ public String getFlexVolumeUuid() {
+ return flexVolumeUuid;
+ }
Review Comment:
```suggestion
}
```
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java:
##########
@@ -1340,6 +1340,15 @@ private void
createManagedVolumeCopyTemplateAsync(VolumeInfo volumeInfo, Primary
primaryDataStore.setDetails(details);
grantAccess(volumeInfo, destHost, primaryDataStore);
+ volumeInfo = volFactory.getVolume(volumeInfo.getId(),
primaryDataStore);
+ // For Netapp ONTAP iscsiName or Lun path is available only after
grantAccess
+ String managedStoreTarget = volumeInfo.get_iScsiName() != null ?
volumeInfo.get_iScsiName() : volumeInfo.getUuid();
+ details.put(PrimaryDataStore.MANAGED_STORE_TARGET,
managedStoreTarget);
+ primaryDataStore.setDetails(details);
+ // Update destTemplateInfo with the iSCSI path from volumeInfo
+ if (destTemplateInfo instanceof TemplateObject) {
+
((TemplateObject)destTemplateInfo).setInstallPath(volumeInfo.getPath());
Review Comment:
This `setInstallPath` here may affect other primary storage solutions. Is it
possible to place it inside a method of the ONTAP plugin's driver, or only
execute it if ONTAP is being used?
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -77,6 +77,8 @@ public class KvmFileBasedStorageVmSnapshotStrategy extends
StorageVMSnapshotStra
private static final List<Storage.StoragePoolType>
supportedStoragePoolTypes = List.of(Storage.StoragePoolType.Filesystem,
Storage.StoragePoolType.NetworkFilesystem,
Storage.StoragePoolType.SharedMountPoint);
+ private static final String ONTAP_PROVIDER_NAME = "NetApp ONTAP";
Review Comment:
This constant is declared in 3 places. Perhaps we could create a file in a
single common dependency (maybe cloud-api) to declare the plugin names?
##########
.github/workflows/daily-repo-status.md:
##########
@@ -1,54 +0,0 @@
----
Review Comment:
Remove this change as well
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java:
##########
@@ -1340,6 +1340,15 @@ private void
createManagedVolumeCopyTemplateAsync(VolumeInfo volumeInfo, Primary
primaryDataStore.setDetails(details);
grantAccess(volumeInfo, destHost, primaryDataStore);
+ volumeInfo = volFactory.getVolume(volumeInfo.getId(),
primaryDataStore);
+ // For Netapp ONTAP iscsiName or Lun path is available only after
grantAccess
+ String managedStoreTarget = volumeInfo.get_iScsiName() != null ?
volumeInfo.get_iScsiName() : volumeInfo.getUuid();
Review Comment:
```suggestion
String managedStoreTarget =
ObjectUtils.defaultIfNull(volumeInfo.get_iScsiName(), volumeInfo.getUuid());
```
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java:
##########
@@ -1340,6 +1340,15 @@ private void
createManagedVolumeCopyTemplateAsync(VolumeInfo volumeInfo, Primary
primaryDataStore.setDetails(details);
grantAccess(volumeInfo, destHost, primaryDataStore);
+ volumeInfo = volFactory.getVolume(volumeInfo.getId(),
primaryDataStore);
+ // For Netapp ONTAP iscsiName or Lun path is available only after
grantAccess
+ String managedStoreTarget = volumeInfo.get_iScsiName() != null ?
volumeInfo.get_iScsiName() : volumeInfo.getUuid();
+ details.put(PrimaryDataStore.MANAGED_STORE_TARGET,
managedStoreTarget);
+ primaryDataStore.setDetails(details);
Review Comment:
Can this set be done inside
`org.apache.cloudstack.storage.driver.OntapPrimaryDatastoreDriver#grantAccess`?
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.feign.model;
+
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class VolumeConcise {
+ @JsonProperty("uuid")
+ private String uuid;
+ @JsonProperty("name")
+ private String name;
+ public String getUuid() {
+ return uuid;
+ }
+ public void setUuid(String uuid) {
+ this.uuid = uuid;
+ }
+ public String getName() {
+ return name;
+ }
+ public void setName(String name) { this.name = name; }
Review Comment:
```suggestion
@JsonProperty("uuid")
private String uuid;
@JsonProperty("name")
private String name;
public String getUuid() {
return uuid;
}
public void setUuid(String uuid) {
this.uuid = uuid;
}
public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
```
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/CloudStackVolume.java:
##########
@@ -56,4 +75,14 @@ public DataObject getVolumeInfo() {
public void setVolumeInfo(DataObject volumeInfo) {
this.volumeInfo = volumeInfo;
}
+ public String getFlexVolumeUuid() {
+ return flexVolumeUuid;
+ }
+ public void setFlexVolumeUuid(String flexVolumeUuid) {
+ this.flexVolumeUuid = flexVolumeUuid;
+ }
+
+ public String getDestinationPath() { return this.destinationPath; }
+ public void setDestinationPath(String destinationPath) {
this.destinationPath = destinationPath; }
Review Comment:
Adjust the formatting
##########
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java:
##########
@@ -0,0 +1,933 @@
+/*
+ * 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 static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+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.VolumeDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
+import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
+import org.apache.cloudstack.storage.to.VolumeObjectTO;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.Spy;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.mockito.junit.jupiter.MockitoSettings;
+import org.mockito.quality.Strictness;
+import org.apache.cloudstack.storage.utils.OntapStorageConstants;
+import com.cloud.agent.AgentManager;
+import com.cloud.agent.api.FreezeThawVMAnswer;
+import com.cloud.agent.api.FreezeThawVMCommand;
+import com.cloud.agent.api.VMSnapshotTO;
+import com.cloud.exception.AgentUnavailableException;
+import com.cloud.exception.OperationTimedoutException;
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.storage.GuestOSVO;
+import com.cloud.storage.VolumeDetailVO;
+import com.cloud.storage.VolumeVO;
+import com.cloud.storage.dao.GuestOSDao;
+import com.cloud.storage.dao.VolumeDao;
+import com.cloud.storage.dao.VolumeDetailsDao;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.NoTransitionException;
+import com.cloud.vm.UserVmVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.UserVmDao;
+import com.cloud.vm.snapshot.VMSnapshot;
+import com.cloud.vm.snapshot.VMSnapshotDetailsVO;
+import com.cloud.vm.snapshot.VMSnapshotVO;
+import com.cloud.vm.snapshot.dao.VMSnapshotDao;
+import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao;
+
+/**
+ * Unit tests for {@link OntapVMSnapshotStrategy}.
+ *
+ * <p>Tests cover:
+ * <ul>
+ * <li>canHandle(VMSnapshot) — various conditions for Allocated and
non-Allocated states</li>
+ * <li>canHandle(Long vmId, Long rootPoolId, boolean snapshotMemory) —
allocation-phase checks</li>
+ * <li>takeVMSnapshot — state transition failure scenarios</li>
+ * <li>Freeze/thaw behavior (freeze success/failure, thaw success/failure,
agent errors)</li>
+ * <li>Quiesce behavior (honors user input; freeze/thaw only when
quiesce=true)</li>
+ * </ul>
+ */
+@ExtendWith(MockitoExtension.class)
+@MockitoSettings(strictness = Strictness.LENIENT)
+class OntapVMSnapshotStrategyTest {
+
+ private static final long VM_ID = 100L;
+ private static final long HOST_ID = 10L;
+ private static final long SNAPSHOT_ID = 200L;
+ private static final long VOLUME_ID_1 = 301L;
+ private static final long VOLUME_ID_2 = 302L;
+ private static final long POOL_ID_1 = 401L;
+ private static final long POOL_ID_2 = 402L;
+ private static final long GUEST_OS_ID = 50L;
+ private static final String VM_INSTANCE_NAME = "i-2-100-VM";
+ private static final String VM_UUID = "vm-uuid-123";
+
+ @Spy
+ private OntapVMSnapshotStrategy strategy;
+
+ @Mock
+ private UserVmDao userVmDao;
+ @Mock
+ private VolumeDao volumeDao;
+ @Mock
+ private PrimaryDataStoreDao storagePool;
+ @Mock
+ private StoragePoolDetailsDao storagePoolDetailsDao;
+ @Mock
+ private VMSnapshotDetailsDao vmSnapshotDetailsDao;
+ @Mock
+ private VMSnapshotHelper vmSnapshotHelper;
+ @Mock
+ private VMSnapshotDao vmSnapshotDao;
+ @Mock
+ private AgentManager agentMgr;
+ @Mock
+ private GuestOSDao guestOSDao;
+ @Mock
+ private VolumeDataFactory volumeDataFactory;
+ @Mock
+ private VolumeDetailsDao volumeDetailsDao;
+
+ @BeforeEach
+ void setUp() throws Exception {
+ // Inject mocks into the inherited fields via reflection
+ // DefaultVMSnapshotStrategy fields
+ setField(strategy, DefaultVMSnapshotStrategy.class,
"vmSnapshotHelper", vmSnapshotHelper);
+ setField(strategy, DefaultVMSnapshotStrategy.class, "guestOSDao",
guestOSDao);
+ setField(strategy, DefaultVMSnapshotStrategy.class, "userVmDao",
userVmDao);
+ setField(strategy, DefaultVMSnapshotStrategy.class, "vmSnapshotDao",
vmSnapshotDao);
+ setField(strategy, DefaultVMSnapshotStrategy.class, "agentMgr",
agentMgr);
+ setField(strategy, DefaultVMSnapshotStrategy.class, "volumeDao",
volumeDao);
+
+ // StorageVMSnapshotStrategy fields
+ setField(strategy, StorageVMSnapshotStrategy.class, "storagePool",
storagePool);
+ setField(strategy, StorageVMSnapshotStrategy.class,
"vmSnapshotDetailsDao", vmSnapshotDetailsDao);
+ setField(strategy, StorageVMSnapshotStrategy.class,
"volumeDataFactory", volumeDataFactory);
+
+ // OntapVMSnapshotStrategy fields
+ setField(strategy, OntapVMSnapshotStrategy.class,
"storagePoolDetailsDao", storagePoolDetailsDao);
+ setField(strategy, OntapVMSnapshotStrategy.class, "volumeDetailsDao",
volumeDetailsDao);
Review Comment:
Please use @InjectMocks instead of reflection
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
