DaanHoogland commented on a change in pull request #4248: URL: https://github.com/apache/cloudstack/pull/4248#discussion_r467768177
########## File path: api/src/main/java/com/cloud/dc/VsphereStoragePolicy.java ########## @@ -0,0 +1,31 @@ +// 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 com.cloud.dc; Review comment: new classes should go in base-package org.apache.cloudstack ########## File path: core/src/main/java/org/apache/cloudstack/storage/command/CheckDataStoreStoragePolicyComplainceCommand.java ########## @@ -0,0 +1,61 @@ +// +// 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.command; + +import com.cloud.agent.api.to.StorageFilerTO; + +public class CheckDataStoreStoragePolicyComplainceCommand extends StorageSubSystemCommand { + + String storagePolicyId; + private StorageFilerTO storagePool; + + public CheckDataStoreStoragePolicyComplainceCommand(String storagePolicyId, StorageFilerTO storagePool) { Review comment: can you add a javadoc explaining what this command/answer pattern implememtation is for? ########## File path: engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/VolumeOrchestrationService.java ########## @@ -84,6 +84,8 @@ VolumeInfo moveVolume(VolumeInfo volume, long destPoolDcId, Long destPoolPodId, String getVmNameOnVolume(Volume volume); + StoragePool findChildDataStoreInDataStoreCluster(DataCenter dc, Pod pod, Long clusterId, Long hostId, VirtualMachine vm, Long datastoreClusterId); Review comment: `findChildDataStoreInDataStoreCluster(..)` is long isn't `findDataStoreInCluster(..)` enough? or maybe `findDataStoreInDataStoreCluster(..)` ########## File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java ########## @@ -295,6 +295,34 @@ public StoragePool findStoragePool(DiskProfile dskCh, DataCenter dc, Pod pod, Lo return null; } + @Override + public StoragePool findChildDataStoreInDataStoreCluster(DataCenter dc, Pod pod, Long clusterId, Long hostId, VirtualMachine vm, Long datastoreClusterId) { + Long podId = null; + if (pod != null) { + podId = pod.getId(); + } else if (clusterId != null) { + Cluster cluster = _entityMgr.findById(Cluster.class, clusterId); + if (cluster != null) { + podId = cluster.getPodId(); + } + } Review comment: `determinePodId(..)` ########## File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java ########## @@ -295,6 +295,34 @@ public StoragePool findStoragePool(DiskProfile dskCh, DataCenter dc, Pod pod, Lo return null; } + @Override + public StoragePool findChildDataStoreInDataStoreCluster(DataCenter dc, Pod pod, Long clusterId, Long hostId, VirtualMachine vm, Long datastoreClusterId) { + Long podId = null; + if (pod != null) { + podId = pod.getId(); + } else if (clusterId != null) { + Cluster cluster = _entityMgr.findById(Cluster.class, clusterId); + if (cluster != null) { + podId = cluster.getPodId(); + } + } + List<StoragePoolVO> childDatastores = _storagePoolDao.listChildStoragePoolsInDatastoreCluster(datastoreClusterId); + List<StoragePool> suitablePools = new ArrayList<StoragePool>(); + + for (StoragePoolVO childDatastore: childDatastores) + suitablePools.add((StoragePool)dataStoreMgr.getDataStore(childDatastore.getId(), DataStoreRole.Primary)); + + VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm); + for (StoragePoolAllocator allocator : _storagePoolAllocators) { + DataCenterDeployment plan = new DataCenterDeployment(dc.getId(), podId, clusterId, hostId, null, null); + final List<StoragePool> poolList = allocator.reorderPools(suitablePools, profile, plan); + + if (poolList != null && !poolList.isEmpty()) { + return (StoragePool)dataStoreMgr.getDataStore(poolList.get(0).getId(), DataStoreRole.Primary); + } + } Review comment: `pickFormPoolList(..)` ########## File path: plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/api/command/admin/zone/ListVsphereStoragePoliciesCmd.java ########## @@ -0,0 +1,109 @@ +// 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.api.command.admin.zone; + +import com.cloud.dc.DataCenter; +import com.cloud.dc.VsphereStoragePolicy; +import com.cloud.exception.ConcurrentOperationException; +import com.cloud.exception.InsufficientCapacityException; +import com.cloud.exception.NetworkRuleConflictException; +import com.cloud.exception.ResourceAllocationException; +import com.cloud.exception.ResourceUnavailableException; +import com.cloud.hypervisor.vmware.VmwareDatacenterService; +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.ListResponse; +import org.apache.cloudstack.api.response.VsphereStoragePoliciesResponse; +import org.apache.cloudstack.api.response.ZoneResponse; +import org.apache.cloudstack.context.CallContext; +import org.apache.log4j.Logger; + +import javax.inject.Inject; +import java.util.ArrayList; +import java.util.List; + +@APICommand(name = ListVsphereStoragePoliciesCmd.APINAME, description = "List vSphere storage policies", + responseObject = VsphereStoragePoliciesResponse.class, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, + authorized = {RoleType.Admin}) Review comment: since = "4.15" ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java ########## @@ -742,6 +743,24 @@ private Answer execute(ResizeVolumeCommand cmd) { } else if (newSize == oldSize) { return new ResizeVolumeAnswer(cmd, true, "success", newSize * ResourceType.bytesToKiB); } + /* + // FR41 this is yet to fix + ManagedObjectReference morDS1 = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, cmd.getPoolUuid()); + DatastoreMO dsMo1 = new DatastoreMO(hyperHost.getContext(), morDS1); + vmdkDataStorePath = VmwareStorageLayoutHelper.getLegacyDatastorePathFromVmdkFileName(dsMo1, path + VMDK_EXTENSION); + DatastoreFile dsFile1 = new DatastoreFile(vmdkDataStorePath); + + s_logger.debug("vDiskid does not exist for volume " + vmdkDataStorePath + " registering the disk now"); + VirtualStorageObjectManagerMO vStorageObjectManagerMO = new VirtualStorageObjectManagerMO(getServiceContext()); + try { + VStorageObject vStorageObject = vStorageObjectManagerMO.registerVirtualDisk(dsFile1, null, dsMo1.getOwnerDatacenter().second()); + VStorageObjectConfigInfo diskConfigInfo = vStorageObject.getConfig(); + ID vdiskId = diskConfigInfo.getId(); + } catch (Throwable e) { + if (e instanceof AlreadyExistsFaultMsg) { + + } + }*/ Review comment: `removeOrFix(..)` ;) ########## File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java ########## @@ -1496,6 +1608,14 @@ public PrimaryDataStoreInfo cancelPrimaryStorageForMaintenance(CancelPrimaryStor DataStoreProvider provider = _dataStoreProviderMgr.getDataStoreProvider(primaryStorage.getStorageProviderName()); DataStoreLifeCycle lifeCycle = provider.getDataStoreLifeCycle(); DataStore store = _dataStoreMgr.getDataStore(primaryStorage.getId(), DataStoreRole.Primary); + if (primaryStorage.getPoolType() == StoragePoolType.DatastoreCluster) { + //FR41 need to handle when one of the primary stores is unable to cancel the maintenance mode Review comment: internal project code in comment, method extract needed. ########## File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java ########## @@ -3333,7 +3362,14 @@ protected SecondaryStorageVmVO destroySecondaryStorageVm(final long instanceId) } if (storageId != null) { - sc.setJoinParameters("volumeSearch", "poolId", storageId); + StoragePoolVO storagePool = _primaryDataStoreDao.findById(storageId); + if (storagePool.getPoolType() == Storage.StoragePoolType.DatastoreCluster) { + List<StoragePoolVO> childDataStores = _primaryDataStoreDao.listChildStoragePoolsInDatastoreCluster(storageId); + List<Long> childDatastoreIds = childDataStores.stream().map(mo -> mo.getId()).collect(Collectors.toList()); + sc.setJoinParameters("volumeSearch", "poolId", childDatastoreIds.toArray()); + } else { + sc.setJoinParameters("volumeSearch", "poolId", storageId); + } Review comment: please extract ########## File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java ########## @@ -3008,6 +3034,12 @@ public DiskOffering createDiskOffering(final CreateDiskOfferingCmd cmd) { } } + if (storagePolicyId != null) { + if (vsphereStoragePolicyDao.findById(storagePolicyId) == null) { + throw new InvalidParameterValueException("Please specify a valid vSphere storage policy id"); + } + } Review comment: why double nesting? this is actually only one error condition is it? ########## File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/PbmPlacementSolverMO.java ########## @@ -0,0 +1,68 @@ +// 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 com.cloud.hypervisor.vmware.mo; Review comment: package name `org.apache.cloudstack.hypervisor.vmware.mo` ########## File path: engine/schema/src/main/java/com/cloud/dc/dao/VsphereStoragePolicyDaoImpl.java ########## @@ -0,0 +1,64 @@ +// 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 com.cloud.dc.dao; Review comment: base package ########## File path: engine/schema/src/main/java/com/cloud/dc/dao/VsphereStoragePolicyDao.java ########## @@ -0,0 +1,30 @@ +// 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 com.cloud.dc.dao; Review comment: base package ########## File path: engine/schema/src/main/java/com/cloud/dc/VsphereStoragePolicyVO.java ########## @@ -0,0 +1,126 @@ +// 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 com.cloud.dc; Review comment: new packages should be in org.apache.cloudstack ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java ########## @@ -1383,6 +1396,79 @@ private void doesZoneExist(Long zoneId) throws InvalidParameterValueException { } } + @Override + public List<? extends VsphereStoragePolicy> importVsphereStoragePolicies(ImportVsphereStoragePoliciesCmd cmd) { + Long zoneId = cmd.getZoneId(); + // Validate Id of zone + doesZoneExist(zoneId); + + final VmwareDatacenterZoneMapVO vmwareDcZoneMap = vmwareDatacenterZoneMapDao.findByZoneId(zoneId); + // Check if zone is associated with VMware DC + if (vmwareDcZoneMap == null) { + throw new CloudRuntimeException("Zone " + zoneId + " is not associated with any VMware datacenter."); + } + + final long vmwareDcId = vmwareDcZoneMap.getVmwareDcId(); + return importVsphereStoragePoliciesInternal(zoneId, vmwareDcId); + } + + public List<? extends VsphereStoragePolicy> importVsphereStoragePoliciesInternal(Long zoneId, Long vmwareDcId) { + + // Get DC associated with this zone + VmwareDatacenterVO vmwareDatacenter = vmwareDcDao.findById(vmwareDcId); + String vmwareDcName = vmwareDatacenter.getVmwareDatacenterName(); + String vCenterHost = vmwareDatacenter.getVcenterHost(); + String userName = vmwareDatacenter.getUser(); + String password = vmwareDatacenter.getPassword(); + List<PbmProfile> storageProfiles = null; + try { + s_logger.debug(String.format("Importing vSphere Storage Policies for the vmware DC %d in zone %d", vmwareDcId, zoneId)); + VmwareContext context = VmwareContextFactory.getContext(vCenterHost, userName, password); + PbmProfileManagerMO profileManagerMO = new PbmProfileManagerMO(context); + storageProfiles = profileManagerMO.getStorageProfiles(); + s_logger.debug(String.format("Import vSphere Storage Policies for the vmware DC %d in zone %d is successful", vmwareDcId, zoneId)); + } catch (Exception e) { + String msg = String.format("Unable to list storage profiles from DC %s due to : %s", vmwareDcName, VmwareHelper.getExceptionMessage(e)); + s_logger.error(msg); + throw new CloudRuntimeException(msg); + } + + for (PbmProfile storageProfile : storageProfiles) { + VsphereStoragePolicyVO storagePolicyVO = vsphereStoragePolicyDao.findByPolicyId(zoneId, storageProfile.getProfileId().getUniqueId()); + if (storagePolicyVO == null) { + storagePolicyVO = new VsphereStoragePolicyVO(zoneId, storageProfile.getProfileId().getUniqueId(), storageProfile.getName(), storageProfile.getDescription()); + vsphereStoragePolicyDao.persist(storagePolicyVO); + } else { + storagePolicyVO.setDescription(storageProfile.getDescription()); + storagePolicyVO.setName(storageProfile.getName()); + vsphereStoragePolicyDao.update(storagePolicyVO.getId(), storagePolicyVO); + } + } + + List<VsphereStoragePolicyVO> allStoragePolicies = vsphereStoragePolicyDao.listAll(); + List<PbmProfile> finalStorageProfiles = storageProfiles; + List<VsphereStoragePolicyVO> needToMarkRemoved = allStoragePolicies.stream() + .filter(existingPolicy -> !finalStorageProfiles.stream() + .anyMatch(storageProfile -> storageProfile.getProfileId().getUniqueId().equals(existingPolicy.getPolicyId()))) + .collect(Collectors.toList()); + + for (VsphereStoragePolicyVO storagePolicy : needToMarkRemoved) { + vsphereStoragePolicyDao.remove(storagePolicy.getId()); + } + + List<VsphereStoragePolicyVO> storagePolicies = vsphereStoragePolicyDao.listAll(); Review comment: `getfilteredStoragePolicies(..)` ########## File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java ########## @@ -295,6 +295,34 @@ public StoragePool findStoragePool(DiskProfile dskCh, DataCenter dc, Pod pod, Lo return null; } + @Override + public StoragePool findChildDataStoreInDataStoreCluster(DataCenter dc, Pod pod, Long clusterId, Long hostId, VirtualMachine vm, Long datastoreClusterId) { + Long podId = null; + if (pod != null) { + podId = pod.getId(); + } else if (clusterId != null) { + Cluster cluster = _entityMgr.findById(Cluster.class, clusterId); + if (cluster != null) { + podId = cluster.getPodId(); + } + } + List<StoragePoolVO> childDatastores = _storagePoolDao.listChildStoragePoolsInDatastoreCluster(datastoreClusterId); + List<StoragePool> suitablePools = new ArrayList<StoragePool>(); + + for (StoragePoolVO childDatastore: childDatastores) + suitablePools.add((StoragePool)dataStoreMgr.getDataStore(childDatastore.getId(), DataStoreRole.Primary)); + Review comment: `getSuitablePools(..)` also please add block braces ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java ########## @@ -3555,6 +3577,38 @@ public Answer handleDownloadTemplateToPrimaryStorage(DirectDownloadCommand cmd) return null; } + @Override + public Answer CheckDataStoreStoragePolicyComplaince(CheckDataStoreStoragePolicyComplainceCommand cmd) { + String primaryStorageNameLabel = cmd.getStoragePool().getUuid(); + String storagePolicyId = cmd.getStoragePolicyId(); + VmwareContext context = hostService.getServiceContext(cmd); + try { + VmwareHypervisorHost hyperHost = hostService.getHyperHost(context, cmd); + ManagedObjectReference morPrimaryDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, primaryStorageNameLabel); + if (morPrimaryDs == null) { + String msg = "Unable to find datastore: " + primaryStorageNameLabel; + s_logger.error(msg); + throw new Exception(msg); + } + + DatastoreMO primaryDsMo = new DatastoreMO(hyperHost.getContext(), morPrimaryDs); + boolean isDatastoreStoragePolicyComplaint = primaryDsMo.isDatastoreStoragePolicyComplaint(storagePolicyId); Review comment: again `Complaint` instead of `Compliant` ########## File path: engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java ########## @@ -90,7 +100,54 @@ public boolean hostConnect(long hostId, long poolId) throws StorageConflictExcep } } } + StoragePoolVO poolVO = this.primaryStoreDao.findById(poolId); + updateStoragePoolHostVOAndDetails(poolVO, hostId, mspAnswer); + + if (pool.getPoolType() == Storage.StoragePoolType.DatastoreCluster) { + for (ModifyStoragePoolAnswer childDataStoreAnswer : ((ModifyStoragePoolAnswer) answer).getDatastoreClusterChildren()) { + StoragePoolInfo childStoragePoolInfo = childDataStoreAnswer.getPoolInfo(); + StoragePoolVO dataStoreVO = primaryStoreDao.findPoolByUUID(childStoragePoolInfo.getUuid()); + if (dataStoreVO != null) { + continue; + } + dataStoreVO = new StoragePoolVO(); + dataStoreVO.setStorageProviderName(poolVO.getStorageProviderName()); + dataStoreVO.setHostAddress(childStoragePoolInfo.getHost()); + dataStoreVO.setPoolType(Storage.StoragePoolType.PreSetup); + dataStoreVO.setPath(childStoragePoolInfo.getHostPath()); + dataStoreVO.setPort(poolVO.getPort()); + dataStoreVO.setName(childStoragePoolInfo.getName()); + dataStoreVO.setUuid(childStoragePoolInfo.getUuid()); + dataStoreVO.setDataCenterId(poolVO.getDataCenterId()); + dataStoreVO.setPodId(poolVO.getPodId()); + dataStoreVO.setClusterId(poolVO.getClusterId()); + dataStoreVO.setStatus(StoragePoolStatus.Up); + dataStoreVO.setUserInfo(poolVO.getUserInfo()); + dataStoreVO.setManaged(poolVO.isManaged()); + dataStoreVO.setCapacityIops(poolVO.getCapacityIops()); + dataStoreVO.setCapacityBytes(childDataStoreAnswer.getPoolInfo().getCapacityBytes()); + dataStoreVO.setUsedBytes(childDataStoreAnswer.getPoolInfo().getCapacityBytes() - childDataStoreAnswer.getPoolInfo().getAvailableBytes()); + dataStoreVO.setHypervisor(poolVO.getHypervisor()); + dataStoreVO.setScope(poolVO.getScope()); + dataStoreVO.setParent(poolVO.getId()); + + Map<String, String> details = new HashMap<>(); + if(StringUtils.isNotEmpty(childDataStoreAnswer.getPoolType())) { + details.put("pool_type", childDataStoreAnswer.getPoolType()); + } + + List<String> storageTags = storagePoolTagsDao.getStoragePoolTags(poolId); + primaryStoreDao.persist(dataStoreVO, details, storageTags); + + updateStoragePoolHostVOAndDetails(dataStoreVO, hostId, childDataStoreAnswer); + } + } + s_logger.info("Connection established between storage pool " + pool + " and host " + hostId); + return true; + } Review comment: `doStuffToDataStoreAnswers(..)` (better name should be thought off) ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java ########## @@ -5298,20 +5363,28 @@ protected Answer execute(GetStorageStatsCommand cmd) { ManagedObjectReference morDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, cmd.getStorageId()); if (morDs != null) { - DatastoreMO datastoreMo = new DatastoreMO(context, morDs); - DatastoreSummary summary = datastoreMo.getSummary(); - assert (summary != null); + long capacity = 0; + long free = 0; + if (cmd.getPooltype() == StoragePoolType.DatastoreCluster) { + StoragepodMO datastoreClusterMo = new StoragepodMO(getServiceContext(), morDs); + StoragePodSummary summary = datastoreClusterMo.getDatastoreClusterSummary(); + capacity = summary.getCapacity(); + free = summary.getFreeSpace(); + } else { + DatastoreMO datastoreMo = new DatastoreMO(context, morDs); + DatastoreSummary summary = datastoreMo.getDatastoreSummary(); + capacity = summary.getCapacity(); + free = summary.getFreeSpace(); + } Review comment: summary null check has disappeared. should it ? also please put in a separate method. ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java ########## @@ -1383,6 +1396,79 @@ private void doesZoneExist(Long zoneId) throws InvalidParameterValueException { } } + @Override + public List<? extends VsphereStoragePolicy> importVsphereStoragePolicies(ImportVsphereStoragePoliciesCmd cmd) { + Long zoneId = cmd.getZoneId(); + // Validate Id of zone + doesZoneExist(zoneId); + + final VmwareDatacenterZoneMapVO vmwareDcZoneMap = vmwareDatacenterZoneMapDao.findByZoneId(zoneId); + // Check if zone is associated with VMware DC + if (vmwareDcZoneMap == null) { + throw new CloudRuntimeException("Zone " + zoneId + " is not associated with any VMware datacenter."); + } + + final long vmwareDcId = vmwareDcZoneMap.getVmwareDcId(); + return importVsphereStoragePoliciesInternal(zoneId, vmwareDcId); + } + + public List<? extends VsphereStoragePolicy> importVsphereStoragePoliciesInternal(Long zoneId, Long vmwareDcId) { Review comment: `public bla<? extends blob> _commitSomeCrime_Internal(..)` , so it is `public` but named `*Internal*`, that is confusing. Also it is doing complex things, can you simplify/disect? ########## File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java ########## @@ -295,6 +295,34 @@ public StoragePool findStoragePool(DiskProfile dskCh, DataCenter dc, Pod pod, Lo return null; } + @Override + public StoragePool findChildDataStoreInDataStoreCluster(DataCenter dc, Pod pod, Long clusterId, Long hostId, VirtualMachine vm, Long datastoreClusterId) { Review comment: even though the method is not too long its complexity is high with several sequential and nested conditions. Can we further disect this method into logical parts? ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java ########## @@ -3555,6 +3577,38 @@ public Answer handleDownloadTemplateToPrimaryStorage(DirectDownloadCommand cmd) return null; } + @Override + public Answer CheckDataStoreStoragePolicyComplaince(CheckDataStoreStoragePolicyComplainceCommand cmd) { + String primaryStorageNameLabel = cmd.getStoragePool().getUuid(); + String storagePolicyId = cmd.getStoragePolicyId(); + VmwareContext context = hostService.getServiceContext(cmd); + try { + VmwareHypervisorHost hyperHost = hostService.getHyperHost(context, cmd); + ManagedObjectReference morPrimaryDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, primaryStorageNameLabel); + if (morPrimaryDs == null) { + String msg = "Unable to find datastore: " + primaryStorageNameLabel; + s_logger.error(msg); + throw new Exception(msg); + } + + DatastoreMO primaryDsMo = new DatastoreMO(hyperHost.getContext(), morPrimaryDs); + boolean isDatastoreStoragePolicyComplaint = primaryDsMo.isDatastoreStoragePolicyComplaint(storagePolicyId); + + String failedMessage = String.format("DataStore %s is not complaince with storage policy id %s", primaryStorageNameLabel, storagePolicyId); + if (!isDatastoreStoragePolicyComplaint) + return new Answer(cmd, isDatastoreStoragePolicyComplaint, failedMessage); + else + return new Answer(cmd, isDatastoreStoragePolicyComplaint, null); + } catch (Throwable e) { + if (e instanceof RemoteException) { + hostService.invalidateServiceContext(context); + } + String details = String.format("Exception while checking if datastore %s is storage policy %s complaince : %s", primaryStorageNameLabel, storagePolicyId, VmwareHelper.getExceptionMessage(e)); + s_logger.error(details, e); + return new Answer(cmd, false, details); + } Review comment: `handleComplianceCheckFailure(..)` ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java ########## @@ -1383,6 +1396,79 @@ private void doesZoneExist(Long zoneId) throws InvalidParameterValueException { } } + @Override + public List<? extends VsphereStoragePolicy> importVsphereStoragePolicies(ImportVsphereStoragePoliciesCmd cmd) { + Long zoneId = cmd.getZoneId(); + // Validate Id of zone + doesZoneExist(zoneId); + + final VmwareDatacenterZoneMapVO vmwareDcZoneMap = vmwareDatacenterZoneMapDao.findByZoneId(zoneId); + // Check if zone is associated with VMware DC + if (vmwareDcZoneMap == null) { + throw new CloudRuntimeException("Zone " + zoneId + " is not associated with any VMware datacenter."); + } + + final long vmwareDcId = vmwareDcZoneMap.getVmwareDcId(); + return importVsphereStoragePoliciesInternal(zoneId, vmwareDcId); + } + + public List<? extends VsphereStoragePolicy> importVsphereStoragePoliciesInternal(Long zoneId, Long vmwareDcId) { + + // Get DC associated with this zone + VmwareDatacenterVO vmwareDatacenter = vmwareDcDao.findById(vmwareDcId); + String vmwareDcName = vmwareDatacenter.getVmwareDatacenterName(); + String vCenterHost = vmwareDatacenter.getVcenterHost(); + String userName = vmwareDatacenter.getUser(); + String password = vmwareDatacenter.getPassword(); + List<PbmProfile> storageProfiles = null; + try { + s_logger.debug(String.format("Importing vSphere Storage Policies for the vmware DC %d in zone %d", vmwareDcId, zoneId)); + VmwareContext context = VmwareContextFactory.getContext(vCenterHost, userName, password); + PbmProfileManagerMO profileManagerMO = new PbmProfileManagerMO(context); + storageProfiles = profileManagerMO.getStorageProfiles(); + s_logger.debug(String.format("Import vSphere Storage Policies for the vmware DC %d in zone %d is successful", vmwareDcId, zoneId)); Review comment: ```suggestion s_logger.info(String.format("Import vSphere Storage Policies for the vmware DC %d in zone %d is successful", vmwareDcId, zoneId)); ``` ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java ########## @@ -1383,6 +1396,79 @@ private void doesZoneExist(Long zoneId) throws InvalidParameterValueException { } } + @Override + public List<? extends VsphereStoragePolicy> importVsphereStoragePolicies(ImportVsphereStoragePoliciesCmd cmd) { + Long zoneId = cmd.getZoneId(); + // Validate Id of zone + doesZoneExist(zoneId); + + final VmwareDatacenterZoneMapVO vmwareDcZoneMap = vmwareDatacenterZoneMapDao.findByZoneId(zoneId); + // Check if zone is associated with VMware DC + if (vmwareDcZoneMap == null) { + throw new CloudRuntimeException("Zone " + zoneId + " is not associated with any VMware datacenter."); + } + + final long vmwareDcId = vmwareDcZoneMap.getVmwareDcId(); + return importVsphereStoragePoliciesInternal(zoneId, vmwareDcId); + } + + public List<? extends VsphereStoragePolicy> importVsphereStoragePoliciesInternal(Long zoneId, Long vmwareDcId) { + + // Get DC associated with this zone + VmwareDatacenterVO vmwareDatacenter = vmwareDcDao.findById(vmwareDcId); + String vmwareDcName = vmwareDatacenter.getVmwareDatacenterName(); + String vCenterHost = vmwareDatacenter.getVcenterHost(); + String userName = vmwareDatacenter.getUser(); + String password = vmwareDatacenter.getPassword(); + List<PbmProfile> storageProfiles = null; + try { + s_logger.debug(String.format("Importing vSphere Storage Policies for the vmware DC %d in zone %d", vmwareDcId, zoneId)); Review comment: ```suggestion if (s_logger.isDebuugEnabled()) { s_logger.debug(String.format("Importing vSphere Storage Policies for the vmware DC %d in zone %d", vmwareDcId, zoneId)); } ``` ########## File path: engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java ########## @@ -90,7 +100,54 @@ public boolean hostConnect(long hostId, long poolId) throws StorageConflictExcep } } } + StoragePoolVO poolVO = this.primaryStoreDao.findById(poolId); + updateStoragePoolHostVOAndDetails(poolVO, hostId, mspAnswer); + + if (pool.getPoolType() == Storage.StoragePoolType.DatastoreCluster) { + for (ModifyStoragePoolAnswer childDataStoreAnswer : ((ModifyStoragePoolAnswer) answer).getDatastoreClusterChildren()) { + StoragePoolInfo childStoragePoolInfo = childDataStoreAnswer.getPoolInfo(); + StoragePoolVO dataStoreVO = primaryStoreDao.findPoolByUUID(childStoragePoolInfo.getUuid()); + if (dataStoreVO != null) { + continue; + } + dataStoreVO = new StoragePoolVO(); + dataStoreVO.setStorageProviderName(poolVO.getStorageProviderName()); + dataStoreVO.setHostAddress(childStoragePoolInfo.getHost()); + dataStoreVO.setPoolType(Storage.StoragePoolType.PreSetup); + dataStoreVO.setPath(childStoragePoolInfo.getHostPath()); + dataStoreVO.setPort(poolVO.getPort()); + dataStoreVO.setName(childStoragePoolInfo.getName()); + dataStoreVO.setUuid(childStoragePoolInfo.getUuid()); + dataStoreVO.setDataCenterId(poolVO.getDataCenterId()); + dataStoreVO.setPodId(poolVO.getPodId()); + dataStoreVO.setClusterId(poolVO.getClusterId()); + dataStoreVO.setStatus(StoragePoolStatus.Up); + dataStoreVO.setUserInfo(poolVO.getUserInfo()); + dataStoreVO.setManaged(poolVO.isManaged()); + dataStoreVO.setCapacityIops(poolVO.getCapacityIops()); + dataStoreVO.setCapacityBytes(childDataStoreAnswer.getPoolInfo().getCapacityBytes()); + dataStoreVO.setUsedBytes(childDataStoreAnswer.getPoolInfo().getCapacityBytes() - childDataStoreAnswer.getPoolInfo().getAvailableBytes()); + dataStoreVO.setHypervisor(poolVO.getHypervisor()); + dataStoreVO.setScope(poolVO.getScope()); + dataStoreVO.setParent(poolVO.getId()); + + Map<String, String> details = new HashMap<>(); + if(StringUtils.isNotEmpty(childDataStoreAnswer.getPoolType())) { + details.put("pool_type", childDataStoreAnswer.getPoolType()); + } + + List<String> storageTags = storagePoolTagsDao.getStoragePoolTags(poolId); + primaryStoreDao.persist(dataStoreVO, details, storageTags); + + updateStoragePoolHostVOAndDetails(dataStoreVO, hostId, childDataStoreAnswer); Review comment: `doStuffToSingleChildDataStoreAnswer(..)` (a better name can be thought off) ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java ########## @@ -1383,6 +1396,79 @@ private void doesZoneExist(Long zoneId) throws InvalidParameterValueException { } } + @Override + public List<? extends VsphereStoragePolicy> importVsphereStoragePolicies(ImportVsphereStoragePoliciesCmd cmd) { + Long zoneId = cmd.getZoneId(); + // Validate Id of zone + doesZoneExist(zoneId); + + final VmwareDatacenterZoneMapVO vmwareDcZoneMap = vmwareDatacenterZoneMapDao.findByZoneId(zoneId); + // Check if zone is associated with VMware DC + if (vmwareDcZoneMap == null) { + throw new CloudRuntimeException("Zone " + zoneId + " is not associated with any VMware datacenter."); + } + + final long vmwareDcId = vmwareDcZoneMap.getVmwareDcId(); + return importVsphereStoragePoliciesInternal(zoneId, vmwareDcId); + } + + public List<? extends VsphereStoragePolicy> importVsphereStoragePoliciesInternal(Long zoneId, Long vmwareDcId) { + + // Get DC associated with this zone + VmwareDatacenterVO vmwareDatacenter = vmwareDcDao.findById(vmwareDcId); + String vmwareDcName = vmwareDatacenter.getVmwareDatacenterName(); + String vCenterHost = vmwareDatacenter.getVcenterHost(); + String userName = vmwareDatacenter.getUser(); + String password = vmwareDatacenter.getPassword(); + List<PbmProfile> storageProfiles = null; + try { + s_logger.debug(String.format("Importing vSphere Storage Policies for the vmware DC %d in zone %d", vmwareDcId, zoneId)); + VmwareContext context = VmwareContextFactory.getContext(vCenterHost, userName, password); + PbmProfileManagerMO profileManagerMO = new PbmProfileManagerMO(context); + storageProfiles = profileManagerMO.getStorageProfiles(); + s_logger.debug(String.format("Import vSphere Storage Policies for the vmware DC %d in zone %d is successful", vmwareDcId, zoneId)); + } catch (Exception e) { + String msg = String.format("Unable to list storage profiles from DC %s due to : %s", vmwareDcName, VmwareHelper.getExceptionMessage(e)); + s_logger.error(msg); + throw new CloudRuntimeException(msg); + } + + for (PbmProfile storageProfile : storageProfiles) { + VsphereStoragePolicyVO storagePolicyVO = vsphereStoragePolicyDao.findByPolicyId(zoneId, storageProfile.getProfileId().getUniqueId()); + if (storagePolicyVO == null) { + storagePolicyVO = new VsphereStoragePolicyVO(zoneId, storageProfile.getProfileId().getUniqueId(), storageProfile.getName(), storageProfile.getDescription()); + vsphereStoragePolicyDao.persist(storagePolicyVO); + } else { + storagePolicyVO.setDescription(storageProfile.getDescription()); + storagePolicyVO.setName(storageProfile.getName()); + vsphereStoragePolicyDao.update(storagePolicyVO.getId(), storagePolicyVO); + } + } Review comment: `somePrivateMethod(..)` ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java ########## @@ -4898,27 +4919,71 @@ protected Answer execute(ModifyStoragePoolCommand cmd) { VmwareHypervisorHost hyperHost = getHyperHost(getServiceContext()); StorageFilerTO pool = cmd.getPool(); - if (pool.getType() != StoragePoolType.NetworkFilesystem && pool.getType() != StoragePoolType.VMFS) { + if (pool.getType() != StoragePoolType.NetworkFilesystem && pool.getType() != StoragePoolType.VMFS && pool.getType() != StoragePoolType.PreSetup && pool.getType() != StoragePoolType.DatastoreCluster) { throw new Exception("Unsupported storage pool type " + pool.getType()); } ManagedObjectReference morDatastore = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, pool.getUuid()); if (morDatastore == null) { - morDatastore = hyperHost.mountDatastore(pool.getType() == StoragePoolType.VMFS, pool.getHost(), pool.getPort(), pool.getPath(), pool.getUuid().replace("-", "")); + morDatastore = hyperHost.mountDatastore((pool.getType() == StoragePoolType.VMFS || pool.getType() == StoragePoolType.PreSetup), pool.getHost(), pool.getPort(), pool.getPath(), pool.getUuid().replace("-", ""), true); } assert (morDatastore != null); - DatastoreSummary summary = new DatastoreMO(getServiceContext(), morDatastore).getSummary(); + DatastoreMO dsMo = new DatastoreMO(getServiceContext(), morDatastore); + HypervisorHostHelper.createBaseFolder(dsMo, hyperHost, pool.getType()); + + long capacity = 0; + long available = 0; + List<ModifyStoragePoolAnswer> childDatastoresModifyStoragePoolAnswers = new ArrayList<>(); + if (pool.getType() == StoragePoolType.DatastoreCluster) { + StoragepodMO datastoreClusterMo = new StoragepodMO(getServiceContext(), morDatastore); + StoragePodSummary dsClusterSummary = datastoreClusterMo.getDatastoreClusterSummary(); + capacity = dsClusterSummary.getCapacity(); + available = dsClusterSummary.getFreeSpace(); + + List<ManagedObjectReference> childDatastoreMors = datastoreClusterMo.getDatastoresInDatastoreCluster(); + for (ManagedObjectReference childDsMor : childDatastoreMors) { + DatastoreMO childDsMo = new DatastoreMO(getServiceContext(), childDsMor); + + Map<String, TemplateProp> tInfo = new HashMap<>(); + DatastoreSummary summary = childDsMo.getDatastoreSummary();; + ModifyStoragePoolAnswer answer = new ModifyStoragePoolAnswer(cmd, summary.getCapacity(), summary.getFreeSpace(), tInfo); + StoragePoolInfo poolInfo = answer.getPoolInfo(); + poolInfo.setName(summary.getName()); + String datastoreClusterPath = pool.getPath(); + int pathstartPosition = datastoreClusterPath.lastIndexOf('/'); + String datacenterName = datastoreClusterPath.substring(0, pathstartPosition+1); + String childPath = datacenterName + summary.getName(); + poolInfo.setHostPath(childPath); + String uuid = UUID.nameUUIDFromBytes(((pool.getHost() + childPath)).getBytes()).toString(); + poolInfo.setUuid(uuid); + poolInfo.setLocalPath(cmd.LOCAL_PATH_PREFIX + File.separator + uuid); + + answer.setPoolInfo(poolInfo); + answer.setPoolType(summary.getType()); + answer.setLocalDatastoreName(morDatastore.getValue()); + + childDsMo.setCustomFieldValue(CustomFieldConstants.CLOUD_UUID, uuid); + HypervisorHostHelper.createBaseFolderInDatastore(childDsMo, hyperHost); + + childDatastoresModifyStoragePoolAnswers.add(answer); + } + } else { + HypervisorHostHelper.createBaseFolderInDatastore(dsMo, hyperHost); - long capacity = summary.getCapacity(); - long available = summary.getFreeSpace(); + DatastoreSummary summary = dsMo.getDatastoreSummary(); + capacity = summary.getCapacity(); + available = summary.getFreeSpace(); + } Review comment: this method is already closing in on 700 lines without these changes please do not add to it. this should be in a separate method (as in fact all additions here. It is not maintainable in this way. ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java ########## @@ -4898,27 +4919,71 @@ protected Answer execute(ModifyStoragePoolCommand cmd) { VmwareHypervisorHost hyperHost = getHyperHost(getServiceContext()); StorageFilerTO pool = cmd.getPool(); - if (pool.getType() != StoragePoolType.NetworkFilesystem && pool.getType() != StoragePoolType.VMFS) { + if (pool.getType() != StoragePoolType.NetworkFilesystem && pool.getType() != StoragePoolType.VMFS && pool.getType() != StoragePoolType.PreSetup && pool.getType() != StoragePoolType.DatastoreCluster) { Review comment: the condition is getting complicated and warrants a `private boolean isUnsupportedSorragePoolType(..)` ########## File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java ########## @@ -2815,6 +2829,23 @@ public String getDeviceBusName(List<VirtualDevice> allDevices, VirtualDevice the if (devices != null && devices.size() > 0) { for (VirtualDevice device : devices) { if (device instanceof VirtualDisk) { + if (((VirtualDisk) device).getVDiskId() == null) { + try { + // Register as first class disk + VirtualDeviceBackingInfo backingInfo = device.getBacking(); + if (backingInfo instanceof VirtualDiskFlatVer2BackingInfo) { + VirtualDiskFlatVer2BackingInfo diskBackingInfo = (VirtualDiskFlatVer2BackingInfo) backingInfo; + DatastoreFile dsBackingFile = new DatastoreFile(diskBackingInfo.getFileName()); + s_logger.debug("vDiskid does not exist for volume " + diskBackingInfo.getFileName() + " registering the disk now"); + VirtualStorageObjectManagerMO vStorageObjectManagerMO = new VirtualStorageObjectManagerMO(getOwnerDatacenter().first().getContext()); + VStorageObject vStorageObject = vStorageObjectManagerMO.registerVirtualDisk(dsBackingFile, null, getOwnerDatacenter().first().getName()); + VStorageObjectConfigInfo diskConfigInfo = vStorageObject.getConfig(); + ((VirtualDisk) device).setVDiskId(diskConfigInfo.getId()); + } + } catch (Exception e) { + s_logger.warn("Exception while trying to register a disk as first class disk to get the unique identifier, main operation still continues: " + e.getMessage()); + } + } Review comment: `tryToRegisterAsFirstClassDisk(..) {...}` , in fact shouldn't this just be a call to `registerVirtualDisk(..)` above (maybe with extra switch in it)? ########## File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostMO.java ########## @@ -865,22 +865,30 @@ public ManagedObjectReference mountDatastore(boolean vmfsDatastore, String poolH s_logger.trace("vCenter API trace - mountDatastore() done(failed)"); throw new Exception(msg); } + dsMo = new DatastoreMO(_context, morDatastore); } else { morDatastore = _context.getDatastoreMorByPath(poolPath); if (morDatastore == null) { - String msg = "Unable to create VMFS datastore. host: " + poolHostAddress + ", port: " + poolHostPort + ", path: " + poolPath + ", uuid: " + poolUuid; - s_logger.error(msg); + morDatastore = findDatastore(_context.getDatastoreNameFromPath(poolPath)); + if (morDatastore == null) { + String msg = "Unable to create VMFS datastore. host: " + poolHostAddress + ", port: " + poolHostPort + ", path: " + poolPath + ", uuid: " + poolUuid; + s_logger.error(msg); - if (s_logger.isTraceEnabled()) - s_logger.trace("vCenter API trace - mountDatastore() done(failed)"); - throw new Exception(msg); + if (s_logger.isTraceEnabled()) + s_logger.trace("vCenter API trace - mountDatastore() done(failed)"); + throw new Exception(msg); + } } - DatastoreMO dsMo = new DatastoreMO(_context, morDatastore); + dsMo = new DatastoreMO(_context, morDatastore); dsMo.setCustomFieldValue(CustomFieldConstants.CLOUD_UUID, poolUuid); } } + if (dsMo != null && !"StoragePod".equals(morDatastore.getType()) && createBaseFolder) { + HypervisorHostHelper.createBaseFolderInDatastore(dsMo, this); + } + Review comment: new bits in a large method, please extract ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java ########## @@ -3555,6 +3577,38 @@ public Answer handleDownloadTemplateToPrimaryStorage(DirectDownloadCommand cmd) return null; } + @Override + public Answer CheckDataStoreStoragePolicyComplaince(CheckDataStoreStoragePolicyComplainceCommand cmd) { + String primaryStorageNameLabel = cmd.getStoragePool().getUuid(); + String storagePolicyId = cmd.getStoragePolicyId(); + VmwareContext context = hostService.getServiceContext(cmd); + try { + VmwareHypervisorHost hyperHost = hostService.getHyperHost(context, cmd); + ManagedObjectReference morPrimaryDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, primaryStorageNameLabel); + if (morPrimaryDs == null) { + String msg = "Unable to find datastore: " + primaryStorageNameLabel; + s_logger.error(msg); + throw new Exception(msg); + } Review comment: `getAvailablePrimaryStoreObjectRef(..)` ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java ########## @@ -1124,6 +1129,12 @@ private void postCreatePrivateTemplate(String installFullPath, long templateId, throw new Exception(msg); } + DatacenterMO dcMo = new DatacenterMO(context, hyperHost.getHyperHostDatacenter()); + ManagedObjectReference morPool = hyperHost.getHyperHostOwnerResourcePool(); + vmMo.createFullCloneWithSpecificDisk(templateUniqueName, dcMo.getVmFolder(), morPool, VmwareHelper.getDiskDeviceDatastore(volumeDeviceInfo.first()), volumeDeviceInfo); + clonedVm = dcMo.findVm(templateUniqueName); + + /* FR41 THIS IS OLD way of creating template using snapshot Review comment: please remove code in comment ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java ########## @@ -4898,27 +4919,71 @@ protected Answer execute(ModifyStoragePoolCommand cmd) { VmwareHypervisorHost hyperHost = getHyperHost(getServiceContext()); StorageFilerTO pool = cmd.getPool(); - if (pool.getType() != StoragePoolType.NetworkFilesystem && pool.getType() != StoragePoolType.VMFS) { + if (pool.getType() != StoragePoolType.NetworkFilesystem && pool.getType() != StoragePoolType.VMFS && pool.getType() != StoragePoolType.PreSetup && pool.getType() != StoragePoolType.DatastoreCluster) { throw new Exception("Unsupported storage pool type " + pool.getType()); } ManagedObjectReference morDatastore = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, pool.getUuid()); if (morDatastore == null) { - morDatastore = hyperHost.mountDatastore(pool.getType() == StoragePoolType.VMFS, pool.getHost(), pool.getPort(), pool.getPath(), pool.getUuid().replace("-", "")); + morDatastore = hyperHost.mountDatastore((pool.getType() == StoragePoolType.VMFS || pool.getType() == StoragePoolType.PreSetup), pool.getHost(), pool.getPort(), pool.getPath(), pool.getUuid().replace("-", ""), true); Review comment: `morDatastore` is what? I suppose that first parameter is a boolean but this is not prozaic code that is clear at first sight. I'm sure this is highly improved by little effort. ########## File path: plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/api/command/admin/zone/ImportVsphereStoragePoliciesCmd.java ########## @@ -0,0 +1,111 @@ +// 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.api.command.admin.zone; + +import com.cloud.dc.DataCenter; +import com.cloud.dc.VsphereStoragePolicy; +import com.cloud.exception.ConcurrentOperationException; +import com.cloud.exception.InsufficientCapacityException; +import com.cloud.exception.NetworkRuleConflictException; +import com.cloud.exception.ResourceAllocationException; +import com.cloud.exception.ResourceUnavailableException; +import com.cloud.hypervisor.vmware.VmwareDatacenterService; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.VsphereStoragePoliciesResponse; +import org.apache.cloudstack.api.response.ListResponse; +import org.apache.cloudstack.api.response.ZoneResponse; +import org.apache.cloudstack.context.CallContext; +import org.apache.log4j.Logger; + +import javax.inject.Inject; +import java.util.ArrayList; +import java.util.List; + +@APICommand(name = ImportVsphereStoragePoliciesCmd.APINAME, description = "Import vSphere storage policies", + responseObject = VsphereStoragePoliciesResponse.class, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, + authorized = {RoleType.Admin}) Review comment: since = "4.15" ########## File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualStorageObjectManagerMO.java ########## @@ -0,0 +1,94 @@ +// 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 com.cloud.hypervisor.vmware.mo; + +import com.vmware.vim25.ID; +import com.vmware.vim25.TaskInfo; +import com.vmware.vim25.VStorageObject; +import com.vmware.vim25.VirtualDiskType; +import com.vmware.vim25.VslmCreateSpec; +import com.vmware.vim25.VslmCreateSpecDiskFileBackingSpec; +import org.apache.log4j.Logger; + +import com.vmware.vim25.ManagedObjectReference; + +import com.cloud.hypervisor.vmware.util.VmwareContext; + +public class VirtualStorageObjectManagerMO extends BaseMO { + @SuppressWarnings("unused") + private static final Logger LOGGER = Logger.getLogger(VirtualStorageObjectManagerMO.class); + + public VirtualStorageObjectManagerMO(VmwareContext context) { + super(context, context.getServiceContent().getVStorageObjectManager()); + } + + public VirtualStorageObjectManagerMO(VmwareContext context, ManagedObjectReference morDiskMgr) { + super(context, morDiskMgr); + } + + public VirtualStorageObjectManagerMO(VmwareContext context, String morType, String morValue) { + super(context, morType, morValue); + } + + public VStorageObject registerVirtualDisk(DatastoreFile datastoreFile, String name, String dcName) throws Exception { + StringBuilder sb = new StringBuilder(); + //https://10.2.2.254/folder/i-2-4-VM/89e3756d9b7444dc92388eb36ddd026b.vmdk?dcPath=datacenter-21&dsName=c84e4af9b6ac33e887a25d9242650091 Review comment: I understand the comment here, but would it really explain to a novice scrolling into this code? ########## File path: server/src/main/java/com/cloud/api/query/dao/StoragePoolJoinDaoImpl.java ########## @@ -94,6 +99,10 @@ public StoragePoolResponse newStoragePoolResponse(StoragePoolJoinVO pool) { poolResponse.setHypervisor(pool.getHypervisor().toString()); } + StoragePoolDetailVO poolType = storagePoolDetailsDao.findDetail(pool.getId(), "pool_type"); + if (poolType != null) { + poolResponse.setType(poolType.getValue()); + } Review comment: ok, won't make _the_ remark this time ;) ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java ########## @@ -3555,6 +3577,38 @@ public Answer handleDownloadTemplateToPrimaryStorage(DirectDownloadCommand cmd) return null; } + @Override + public Answer CheckDataStoreStoragePolicyComplaince(CheckDataStoreStoragePolicyComplainceCommand cmd) { + String primaryStorageNameLabel = cmd.getStoragePool().getUuid(); + String storagePolicyId = cmd.getStoragePolicyId(); + VmwareContext context = hostService.getServiceContext(cmd); + try { + VmwareHypervisorHost hyperHost = hostService.getHyperHost(context, cmd); + ManagedObjectReference morPrimaryDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, primaryStorageNameLabel); + if (morPrimaryDs == null) { + String msg = "Unable to find datastore: " + primaryStorageNameLabel; + s_logger.error(msg); + throw new Exception(msg); + } + + DatastoreMO primaryDsMo = new DatastoreMO(hyperHost.getContext(), morPrimaryDs); + boolean isDatastoreStoragePolicyComplaint = primaryDsMo.isDatastoreStoragePolicyComplaint(storagePolicyId); + + String failedMessage = String.format("DataStore %s is not complaince with storage policy id %s", primaryStorageNameLabel, storagePolicyId); + if (!isDatastoreStoragePolicyComplaint) + return new Answer(cmd, isDatastoreStoragePolicyComplaint, failedMessage); + else + return new Answer(cmd, isDatastoreStoragePolicyComplaint, null); Review comment: `produceCompliancyAnswer(..)` ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java ########## @@ -3555,6 +3577,38 @@ public Answer handleDownloadTemplateToPrimaryStorage(DirectDownloadCommand cmd) return null; } + @Override + public Answer CheckDataStoreStoragePolicyComplaince(CheckDataStoreStoragePolicyComplainceCommand cmd) { Review comment: spello in method name: ```suggestion public Answer CheckDataStoreStoragePolicyCompliance(CheckDataStoreStoragePolicyComplianceCommand cmd) { ``` note that `CheckDataStoreStoragePolicyComplainceCommand` needs renaming as well! ########## File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java ########## @@ -593,6 +598,12 @@ public String updateConfiguration(final long userId, final String name, final St } _storagePoolDetailsDao.addDetail(resourceId, name, value, true); + if (pool.getPoolType() == Storage.StoragePoolType.DatastoreCluster) { + List<StoragePoolVO> childDataStores = _storagePoolDao.listChildStoragePoolsInDatastoreCluster(resourceId); + for (StoragePoolVO childDataStore: childDataStores) { + _storagePoolDetailsDao.addDetail(childDataStore.getId(), name, value, true); + } + } Review comment: I think both the entire `case StoragePool`-block and this part from it should be extracted. ########## File path: server/src/main/java/com/cloud/api/query/QueryManagerImpl.java ########## @@ -952,7 +957,12 @@ } if (storageId != null) { - sb.and("poolId", sb.entity().getPoolId(), SearchCriteria.Op.EQ); + StoragePoolVO poolVO = _storagePoolDao.findById((Long) storageId); + if (poolVO.getPoolType() == Storage.StoragePoolType.DatastoreCluster) { + sb.and("poolId", sb.entity().getPoolId(), SearchCriteria.Op.IN); + } else { + sb.and("poolId", sb.entity().getPoolId(), SearchCriteria.Op.EQ); + } Review comment: this method was already closing on 300 lines, please do not add to that and extract a `private method(..)`. ########## File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java ########## @@ -3291,9 +3313,16 @@ protected SecondaryStorageVmVO destroySecondaryStorageVm(final long instanceId) sb.and("nulltype", sb.entity().getType(), SearchCriteria.Op.IN); if (storageId != null) { - final SearchBuilder<VolumeVO> volumeSearch = _volumeDao.createSearchBuilder(); - volumeSearch.and("poolId", volumeSearch.entity().getPoolId(), SearchCriteria.Op.EQ); - sb.join("volumeSearch", volumeSearch, sb.entity().getId(), volumeSearch.entity().getInstanceId(), JoinBuilder.JoinType.INNER); + StoragePoolVO storagePool = _primaryDataStoreDao.findById(storageId); + if (storagePool.getPoolType() == Storage.StoragePoolType.DatastoreCluster) { + final SearchBuilder<VolumeVO> volumeSearch = _volumeDao.createSearchBuilder(); + volumeSearch.and("poolId", volumeSearch.entity().getPoolId(), SearchCriteria.Op.IN); + sb.join("volumeSearch", volumeSearch, sb.entity().getId(), volumeSearch.entity().getInstanceId(), JoinBuilder.JoinType.INNER); + } else { + final SearchBuilder<VolumeVO> volumeSearch = _volumeDao.createSearchBuilder(); + volumeSearch.and("poolId", volumeSearch.entity().getPoolId(), SearchCriteria.Op.EQ); + sb.join("volumeSearch", volumeSearch, sb.entity().getId(), volumeSearch.entity().getInstanceId(), JoinBuilder.JoinType.INNER); + } Review comment: being pedantic I see three methods here. ########## File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualStorageObjectManagerMO.java ########## @@ -0,0 +1,94 @@ +// 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 com.cloud.hypervisor.vmware.mo; Review comment: arguably, this should go into `org.apache.cloudstack`. (we have to start somewhen) ########## File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java ########## @@ -2444,14 +2455,21 @@ public ServiceOffering createServiceOffering(final CreateServiceOfferingCmd cmd) } } + final Long storagePolicyId = cmd.getStoragePolicy(); + if (storagePolicyId != null) { + if (vsphereStoragePolicyDao.findById(storagePolicyId) == null) { + throw new InvalidParameterValueException("Please specify a valid vSphere storage policy id"); + } + } Review comment: I have to scroll 150 line up just to find what method this is part of. In an ide i would have tooling for that of course but still let's not add to the readability defect of the code ########## File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java ########## @@ -1870,6 +1990,57 @@ public boolean storagePoolHasEnoughSpaceForResize(StoragePool pool, long current } } + @Override + public boolean isStoragePoolComplaintWithStoragePolicy(List<Volume> volumes, StoragePool pool) throws StorageUnavailableException { Review comment: 50 line, two for loops and quadruple nested blocks. please dissect. ########## File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualStorageObjectManagerMO.java ########## @@ -0,0 +1,94 @@ +// 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 com.cloud.hypervisor.vmware.mo; + +import com.vmware.vim25.ID; +import com.vmware.vim25.TaskInfo; +import com.vmware.vim25.VStorageObject; +import com.vmware.vim25.VirtualDiskType; +import com.vmware.vim25.VslmCreateSpec; +import com.vmware.vim25.VslmCreateSpecDiskFileBackingSpec; +import org.apache.log4j.Logger; + +import com.vmware.vim25.ManagedObjectReference; + +import com.cloud.hypervisor.vmware.util.VmwareContext; + +public class VirtualStorageObjectManagerMO extends BaseMO { + @SuppressWarnings("unused") + private static final Logger LOGGER = Logger.getLogger(VirtualStorageObjectManagerMO.class); + + public VirtualStorageObjectManagerMO(VmwareContext context) { + super(context, context.getServiceContent().getVStorageObjectManager()); + } + + public VirtualStorageObjectManagerMO(VmwareContext context, ManagedObjectReference morDiskMgr) { + super(context, morDiskMgr); + } + + public VirtualStorageObjectManagerMO(VmwareContext context, String morType, String morValue) { + super(context, morType, morValue); + } + + public VStorageObject registerVirtualDisk(DatastoreFile datastoreFile, String name, String dcName) throws Exception { + StringBuilder sb = new StringBuilder(); + //https://10.2.2.254/folder/i-2-4-VM/89e3756d9b7444dc92388eb36ddd026b.vmdk?dcPath=datacenter-21&dsName=c84e4af9b6ac33e887a25d9242650091 + sb.append("https://";).append(_context.getServerAddress()).append("/folder/"); + sb.append(datastoreFile.getRelativePath()); + sb.append("?dcPath="); + sb.append(dcName); + sb.append("&dsName="); + sb.append(datastoreFile.getDatastoreName()); + return _context.getService().registerDisk(_mor, sb.toString(), name); + } + + public VStorageObject retrieveVirtualDisk (ID id, ManagedObjectReference morDS) throws Exception { + return _context.getService().retrieveVStorageObject(_mor, id, morDS); + } + + public VStorageObject createDisk(ManagedObjectReference morDS, VirtualDiskType diskType, long currentSizeInBytes, String datastoreFilepath, String filename) throws Exception { + long currentSizeInMB = currentSizeInBytes/(1024*1024); + + VslmCreateSpecDiskFileBackingSpec diskFileBackingSpec = new VslmCreateSpecDiskFileBackingSpec(); + diskFileBackingSpec.setDatastore(morDS); + diskFileBackingSpec.setProvisioningType(diskType.value()); + // path should be just the folder name. For example, instead of '[datastore1] folder1/filename.vmdk' you would just do 'folder1'. + // path is introduced from 6.7. In 6.5 disk will be created in the default folder "fcd" + diskFileBackingSpec.setPath(null); + + VslmCreateSpec vslmCreateSpec = new VslmCreateSpec(); + vslmCreateSpec.setBackingSpec(diskFileBackingSpec); + vslmCreateSpec.setCapacityInMB(currentSizeInMB); + vslmCreateSpec.setName(filename); + + ManagedObjectReference morTask = _context.getService().createDiskTask(_mor, vslmCreateSpec); + boolean result = _context.getVimClient().waitForTask(morTask); + + VStorageObject vStorageObject = null; + if (result) { + _context.waitForTaskProgressDone(morTask); + //_context.getService().reconcileDatastoreInventoryTask(_mor, morDS); Review comment: please remove code in comment ########## File path: server/src/main/java/com/cloud/api/query/QueryManagerImpl.java ########## @@ -1078,7 +1088,14 @@ } if (storageId != null) { - sc.setParameters("poolId", storageId); + StoragePoolVO poolVO = _storagePoolDao.findById((Long) storageId); + if (poolVO.getPoolType() == Storage.StoragePoolType.DatastoreCluster) { + List<StoragePoolVO> childDatastores = _storagePoolDao.listChildStoragePoolsInDatastoreCluster((Long) storageId); + List<Long> childDatastoreIds = childDatastores.stream().map(mo -> mo.getId()).collect(Collectors.toList()); + sc.setParameters("poolId", childDatastoreIds.toArray()); + } else { + sc.setParameters("poolId", storageId); + } Review comment: this method was already closing on 300 lines, please do not add to that and extract a `private method(..)`. ########## File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java ########## @@ -2192,6 +2193,13 @@ public Volume migrateVolume(MigrateVolumeCmd cmd) { throw new InvalidParameterValueException("Cannot migrate volume " + vol + "to the destination storage pool " + destPool.getName() + " as the storage pool is in maintenance mode."); } + if (destPool.getPoolType() == Storage.StoragePoolType.DatastoreCluster) { + DataCenter dc = _entityMgr.findById(DataCenter.class, vol.getDataCenterId()); + Pod destPoolPod = _entityMgr.findById(Pod.class, destPool.getPodId()); + + destPool = _volumeMgr.findChildDataStoreInDataStoreCluster(dc, destPoolPod, destPool.getClusterId(), null, null, destPool.getId()); + } + Review comment: `conditionallyTraverseDatastoreClusterToReachBasicDatastore(..)` ########## File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java ########## @@ -2227,37 +2236,50 @@ public Answer createVolume(CreateObjectCommand cmd) { String volumeUuid = UUID.randomUUID().toString().replace("-", ""); String volumeDatastorePath = dsMo.getDatastorePath(volumeUuid + ".vmdk"); - String dummyVmName = hostService.getWorkerName(context, cmd, 0); - try { - s_logger.info("Create worker VM " + dummyVmName); - vmMo = HypervisorHostHelper.createWorkerVM(hyperHost, dsMo, dummyVmName); - if (vmMo == null) { - throw new Exception("Unable to create a dummy VM for volume creation"); - } - synchronized (this) { - try { - vmMo.createDisk(volumeDatastorePath, (int)(volume.getSize() / (1024L * 1024L)), morDatastore, vmMo.getScsiDeviceControllerKey()); - vmMo.detachDisk(volumeDatastorePath, false); + VirtualStorageObjectManagerMO vStorageObjectManagerMO = new VirtualStorageObjectManagerMO(context); + VStorageObject virtualDisk = vStorageObjectManagerMO.createDisk(morDatastore, VirtualDiskType.THIN, volume.getSize(), volumeDatastorePath, volumeUuid); + VolumeObjectTO newVol = new VolumeObjectTO(); + DatastoreFile file = new DatastoreFile(((BaseConfigInfoDiskFileBackingInfo)virtualDisk.getConfig().getBacking()).getFilePath()); + newVol.setPath(file.getFileBaseName()); + newVol.setSize(volume.getSize()); + return new CreateObjectAnswer(newVol); + + /* Review comment: please remove ########## File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/PbmProfileManagerMO.java ########## @@ -0,0 +1,94 @@ +// 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 com.cloud.hypervisor.vmware.mo; Review comment: package `org.apache.cloudstack` ########## File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/StoragepodMO.java ########## @@ -0,0 +1,48 @@ +// 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 com.cloud.hypervisor.vmware.mo; Review comment: package should be `org.apache.cloudstack` ########## File path: server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java ########## @@ -1268,6 +1269,18 @@ public int compare(Volume v1, Volume v2) { requestVolumes = new ArrayList<Volume>(); requestVolumes.add(vol); + if (potentialHost.getHypervisorType() == HypervisorType.VMware) { + try { + boolean isStoragePoolStoragepolicyComplaince = _storageMgr.isStoragePoolComplaintWithStoragePolicy(requestVolumes, potentialSPool); + if (!isStoragePoolStoragepolicyComplaince) { + continue; + } + } catch (StorageUnavailableException e) { + s_logger.warn(String.format("Could not verify storage policy complaince against storage pool %s due to exception %s", potentialSPool.getUuid(), e.getMessage())); + continue; + } + } Review comment: `handleVmwareStoragePoolCompliance(..)` ########## File path: server/src/main/java/com/cloud/template/TemplateManagerImpl.java ########## @@ -616,6 +616,18 @@ public void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestinat private void prepareTemplateInOneStoragePool(final VMTemplateVO template, final StoragePoolVO pool) { s_logger.info("Schedule to preload template " + template.getId() + " into primary storage " + pool.getId()); + if (pool.getPoolType() == Storage.StoragePoolType.DatastoreCluster) { + List<StoragePoolVO> childDataStores = _poolDao.listChildStoragePoolsInDatastoreCluster(pool.getId()); + s_logger.debug("Schedule to preload template " + template.getId() + " into child datastores of DataStore cluster: " + pool.getId()); + for (StoragePoolVO childDataStore : childDataStores) { + prepareTemplateInOneStoragePoolInternal(template, childDataStore); + } + } else { + prepareTemplateInOneStoragePoolInternal(template, pool); + } + } + + private void prepareTemplateInOneStoragePoolInternal(final VMTemplateVO template, final StoragePoolVO pool) { Review comment: naming is kind of off; `private prepareTemplateInOneStoragePool(..)` is already internal as the fact that it is `private` suggests. I would think this name would point to what it actually does, like `startAThreadToPreloadTemplate(..)` or such a thing. ########## File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java ########## @@ -1469,11 +1537,55 @@ public PrimaryDataStoreInfo preparePrimaryStorageForMaintenance(Long primaryStor DataStoreProvider provider = _dataStoreProviderMgr.getDataStoreProvider(primaryStorage.getStorageProviderName()); DataStoreLifeCycle lifeCycle = provider.getDataStoreLifeCycle(); DataStore store = _dataStoreMgr.getDataStore(primaryStorage.getId(), DataStoreRole.Primary); + + if (primaryStorage.getPoolType() == StoragePoolType.DatastoreCluster) { + handlePrepareDatastoreCluserMaintenance(lifeCycle, primaryStorageId); + } lifeCycle.maintain(store); return (PrimaryDataStoreInfo)_dataStoreMgr.getDataStore(primaryStorage.getId(), DataStoreRole.Primary); } + private void handlePrepareDatastoreCluserMaintenance(DataStoreLifeCycle lifeCycle, Long primaryStorageId) { Review comment: rather large, but it be further split? ########## File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java ########## @@ -901,10 +925,54 @@ public boolean deletePool(DeletePoolCmd cmd) { s_logger.warn("Unable to delete storage id: " + id + " due to it is not in Maintenance state"); throw new InvalidParameterValueException("Unable to delete storage due to it is not in Maintenance state, id: " + id); } - Pair<Long, Long> vlms = _volsDao.getCountAndTotalByPool(id); + + if (sPool.getPoolType() == StoragePoolType.DatastoreCluster) { + // FR41 yet to handle on failure of deletion of any of the child storage pool + if (checkIfDataStoreClusterCanbeDeleted(sPool, forced)) { Review comment: are we sure the check outside the transaction is secure/safe? even if it seems to be just the return, still in the check and during the stack drop context switches may have occurred. please reconsider. ########## File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java ########## @@ -1469,11 +1537,55 @@ public PrimaryDataStoreInfo preparePrimaryStorageForMaintenance(Long primaryStor DataStoreProvider provider = _dataStoreProviderMgr.getDataStoreProvider(primaryStorage.getStorageProviderName()); DataStoreLifeCycle lifeCycle = provider.getDataStoreLifeCycle(); DataStore store = _dataStoreMgr.getDataStore(primaryStorage.getId(), DataStoreRole.Primary); + + if (primaryStorage.getPoolType() == StoragePoolType.DatastoreCluster) { + handlePrepareDatastoreCluserMaintenance(lifeCycle, primaryStorageId); + } lifeCycle.maintain(store); return (PrimaryDataStoreInfo)_dataStoreMgr.getDataStore(primaryStorage.getId(), DataStoreRole.Primary); } + private void handlePrepareDatastoreCluserMaintenance(DataStoreLifeCycle lifeCycle, Long primaryStorageId) { + // Before preparing the datastorecluster to maintenance mode, the storagepools in the datastore cluster needs to put in maintenance + List<StoragePoolVO> childDatastores = _storagePoolDao.listChildStoragePoolsInDatastoreCluster(primaryStorageId); + Transaction.execute(new TransactionCallbackNoReturn() { + @Override + public void doInTransactionWithoutResult(TransactionStatus status) { + for (StoragePoolVO childDatastore : childDatastores) { + // set the pool state to prepare for maintenance, so that VMs will not migrate to the storagepools in the same cluster + childDatastore.setStatus(StoragePoolStatus.PrepareForMaintenance); + _storagePoolDao.update(childDatastore.getId(), childDatastore); + } + } + }); + List<DataStore> maintenanceSuccessfulStoragePools = new ArrayList<>(); + for (Iterator<StoragePoolVO> iteratorChildDatastore = childDatastores.listIterator(); iteratorChildDatastore.hasNext(); ) { + DataStore childStore = _dataStoreMgr.getDataStore(iteratorChildDatastore.next().getId(), DataStoreRole.Primary); + try { + lifeCycle.maintain(childStore); + } catch (Exception e) { + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Exception on maintenance preparation of one of the child datastores in datastore cluster %d with error %s", primaryStorageId, e)); + s_logger.debug(String.format("Cancelling the maintenance mode of child datastores in datastore cluster %d", primaryStorageId)); + } + // Cancel maintenance mode of already prepared child storage pools Review comment: comments lie this are a tell that a method could be factored out. ########## File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java ########## @@ -1469,11 +1537,55 @@ public PrimaryDataStoreInfo preparePrimaryStorageForMaintenance(Long primaryStor DataStoreProvider provider = _dataStoreProviderMgr.getDataStoreProvider(primaryStorage.getStorageProviderName()); DataStoreLifeCycle lifeCycle = provider.getDataStoreLifeCycle(); DataStore store = _dataStoreMgr.getDataStore(primaryStorage.getId(), DataStoreRole.Primary); + + if (primaryStorage.getPoolType() == StoragePoolType.DatastoreCluster) { + handlePrepareDatastoreCluserMaintenance(lifeCycle, primaryStorageId); + } lifeCycle.maintain(store); return (PrimaryDataStoreInfo)_dataStoreMgr.getDataStore(primaryStorage.getId(), DataStoreRole.Primary); } + private void handlePrepareDatastoreCluserMaintenance(DataStoreLifeCycle lifeCycle, Long primaryStorageId) { + // Before preparing the datastorecluster to maintenance mode, the storagepools in the datastore cluster needs to put in maintenance + List<StoragePoolVO> childDatastores = _storagePoolDao.listChildStoragePoolsInDatastoreCluster(primaryStorageId); + Transaction.execute(new TransactionCallbackNoReturn() { + @Override + public void doInTransactionWithoutResult(TransactionStatus status) { + for (StoragePoolVO childDatastore : childDatastores) { + // set the pool state to prepare for maintenance, so that VMs will not migrate to the storagepools in the same cluster + childDatastore.setStatus(StoragePoolStatus.PrepareForMaintenance); + _storagePoolDao.update(childDatastore.getId(), childDatastore); + } + } + }); + List<DataStore> maintenanceSuccessfulStoragePools = new ArrayList<>(); + for (Iterator<StoragePoolVO> iteratorChildDatastore = childDatastores.listIterator(); iteratorChildDatastore.hasNext(); ) { + DataStore childStore = _dataStoreMgr.getDataStore(iteratorChildDatastore.next().getId(), DataStoreRole.Primary); + try { + lifeCycle.maintain(childStore); + } catch (Exception e) { + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Exception on maintenance preparation of one of the child datastores in datastore cluster %d with error %s", primaryStorageId, e)); + s_logger.debug(String.format("Cancelling the maintenance mode of child datastores in datastore cluster %d", primaryStorageId)); + } + // Cancel maintenance mode of already prepared child storage pools + maintenanceSuccessfulStoragePools.add(childStore); + for (DataStore dataStore: maintenanceSuccessfulStoragePools) { + lifeCycle.cancelMaintain(dataStore); + } + // Set back to Up state of remaining child storage pools Review comment: another method name in comment ;) ########## File path: server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java ########## @@ -117,9 +117,11 @@ public boolean maintain(DataStore store) { spes = primaryDataStoreDao.listBy(pool.getDataCenterId(), pool.getPodId(), pool.getClusterId(), ScopeType.CLUSTER); } for (StoragePoolVO sp : spes) { - if (sp.getStatus() == StoragePoolStatus.PrepareForMaintenance) { - throw new CloudRuntimeException("Only one storage pool in a cluster can be in PrepareForMaintenance mode, " + sp.getId() + - " is already in PrepareForMaintenance mode "); + if (sp.getParent() != pool.getParent()) { // If Datastore cluster is tried to prepare for maintenance then child storage pools are also kept in PrepareForMaintenance mode + if (sp.getStatus() == StoragePoolStatus.PrepareForMaintenance) { + throw new CloudRuntimeException("Only one storage pool in a cluster can be in PrepareForMaintenance mode, " + sp.getId() + + " is already in PrepareForMaintenance mode "); + } Review comment: good comment but it should be reflected in a method name, modularising the code further. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
