shwstppr commented on code in PR #8875: URL: https://github.com/apache/cloudstack/pull/8875#discussion_r1565422034
########## engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/AbstractPrimaryDataStoreLifeCycleImpl.java: ########## @@ -0,0 +1,125 @@ +// 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.datastore.lifecycle; + +import java.util.ArrayList; +import java.util.List; + +import javax.inject.Inject; + +import org.apache.cloudstack.engine.subsystem.api.storage.ClusterScope; +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +import org.apache.cloudstack.storage.volume.datastore.PrimaryDataStoreHelper; +import org.apache.log4j.Logger; + +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.DeleteStoragePoolCommand; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; +import com.cloud.hypervisor.Hypervisor.HypervisorType; +import com.cloud.resource.ResourceManager; +import com.cloud.storage.StorageManager; +import com.cloud.storage.StoragePool; +import com.cloud.storage.StoragePoolHostVO; +import com.cloud.storage.dao.StoragePoolHostDao; +import com.cloud.utils.Pair; + +public class AbstractPrimaryDataStoreLifeCycleImpl { Review Comment: minor: would it make sense to name it BasePrimaryDataStoreLifeCycleImpl instead? ########## plugins/storage/volume/sample/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/SamplePrimaryDataStoreLifeCycleImpl.java: ########## @@ -146,4 +146,14 @@ public void enableStoragePool(DataStore store) { @Override public void disableStoragePool(DataStore store) { } + + @Override + public void changeStoragePoolScopeToZone(DataStore store, ClusterScope clusterScope, HypervisorType hypervisorType) { + return; Review Comment: Not sure if these return statement add much value ########## engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java: ########## @@ -841,4 +866,13 @@ public List<VolumeVO> listByIds(List<Long> ids) { sc.setParameters("idIN", ids.toArray()); return listBy(sc, null); } + + @Override + public List<VolumeVO> listByPoolIdVMStatesNotInCluster(long clusterId, List<VirtualMachine.State> states, long poolId) { + SearchCriteria<VolumeVO> sc = volumePoolNotInClusterSearch.create(); + sc.setParameters("poolId", poolId); + sc.setParameters("vmStates", states); Review Comment: Should this use `setJoinParameters`? ########## ui/src/config/section/infra/primaryStorages.js: ########## @@ -133,6 +133,24 @@ export default { dataView: true, show: (record) => { return ['Maintenance', 'PrepareForMaintenance', 'ErrorInMaintenance'].includes(record.state) } }, + { + api: 'changeStoragePoolScope', + icon: 'arrow-right-outlined', + label: 'label.action.change.primary.storage.scope', + dataView: true, + popup: true, + show: (record) => { + return (record.state === 'Disabled' && + (record.hypervisor === 'KVM' || + record.hypervisor === 'VMware' || + record.hypervisor === 'HyperV' || + record.hypervisor === 'LXC' || + record.hypervisor === 'Any' || + record.hypervisor === 'Simulator') Review Comment: PR description says feature is supported for KVM, VMware and Simulator only? ########## ui/src/views/infra/changeStoragePoolScope.vue: ########## @@ -0,0 +1,226 @@ +// 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. + +<template> + <a-spin :spinning="loading"> + <div class="form-layout" v-ctrl-enter="handleSubmitForm"> + <div class="form"> + <a-form + :ref="formRef" + :model="form" + :rules="rules" + layout="vertical" + @submit="handleSubmitForm"> + <a-alert type="warning"> + <template #message> + <span v-html="resource.scope=='ZONE' ? $t('message.action.primary.storage.scope.cluster') : $t('message.action.primary.storage.scope.zone')"></span> + </template> + </a-alert> + <a-alert type="warning"> + <template #message> + <span v-html="$t('message.warn.change.primary.storage.scope')"></span> + </template> + </a-alert> Review Comment: Do we need two different alerts? Aesthetically not looking very pleasing. ########## ui/src/views/infra/changeStoragePoolScope.vue: ########## @@ -0,0 +1,226 @@ +// Licensed to the Apache Software Foundation (ASF) under one Review Comment: Should the file be named `ChangeStoragePoolScope.vue` (uppercase `C`) for consistency? ########## engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/AbstractPrimaryDataStoreLifeCycleImpl.java: ########## @@ -0,0 +1,125 @@ +// 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.datastore.lifecycle; + +import java.util.ArrayList; +import java.util.List; + +import javax.inject.Inject; + +import org.apache.cloudstack.engine.subsystem.api.storage.ClusterScope; +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +import org.apache.cloudstack.storage.volume.datastore.PrimaryDataStoreHelper; +import org.apache.log4j.Logger; + +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.DeleteStoragePoolCommand; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; +import com.cloud.hypervisor.Hypervisor.HypervisorType; +import com.cloud.resource.ResourceManager; +import com.cloud.storage.StorageManager; +import com.cloud.storage.StoragePool; +import com.cloud.storage.StoragePoolHostVO; +import com.cloud.storage.dao.StoragePoolHostDao; +import com.cloud.utils.Pair; + +public class AbstractPrimaryDataStoreLifeCycleImpl { + private static final Logger s_logger = Logger.getLogger(AbstractPrimaryDataStoreLifeCycleImpl.class); + @Inject + AgentManager agentMgr; + @Inject + protected ResourceManager resourceMgr; + @Inject + StorageManager storageMgr; + @Inject + PrimaryDataStoreHelper dataStoreHelper; + @Inject + protected HostDao hostDao; + @Inject + protected StoragePoolHostDao storagePoolHostDao; + + private HypervisorType getHypervisorType(long hostId) { + HostVO host = hostDao.findById(hostId); + if (host != null) + return host.getHypervisorType(); + return HypervisorType.None; + } + + private List<HostVO> getPoolHostsList(ClusterScope clusterScope, HypervisorType hypervisorType) { + + List<HostVO> hosts = new ArrayList<HostVO>(); + + if (hypervisorType != null) { + hosts = resourceMgr + .listAllHostsInOneZoneNotInClusterByHypervisor(hypervisorType, clusterScope.getZoneId(), clusterScope.getScopeId()); + } else { + List<HostVO> xenServerHosts = resourceMgr + .listAllHostsInOneZoneNotInClusterByHypervisor(HypervisorType.XenServer, clusterScope.getZoneId(), clusterScope.getScopeId()); + List<HostVO> vmWareServerHosts = resourceMgr + .listAllHostsInOneZoneNotInClusterByHypervisor(HypervisorType.VMware, clusterScope.getZoneId(), clusterScope.getScopeId()); + List<HostVO> kvmHosts = resourceMgr. + listAllHostsInOneZoneNotInClusterByHypervisor(HypervisorType.KVM, clusterScope.getZoneId(), clusterScope.getScopeId()); Review Comment: Will it make sense to add a new method in Dao or ResourceManager to get pool hosts with zone, scopeid and a list of hypervisor types? Not sure if it is querying DB thrice right now ########## test/integration/smoke/test_primary_storage.py: ########## @@ -363,6 +363,151 @@ def test_01_add_primary_storage_disabled_host(self): return +class TestPrimaryStorageScope(cloudstackTestCase): Review Comment: Not sure if this should be in a new file. Others can comment. The problem with adding new classes to the same test file is that marvin may run all test cases in the file on a single failure ########## engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/AbstractPrimaryDataStoreLifeCycleImpl.java: ########## @@ -0,0 +1,125 @@ +// 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.datastore.lifecycle; + +import java.util.ArrayList; +import java.util.List; + +import javax.inject.Inject; + +import org.apache.cloudstack.engine.subsystem.api.storage.ClusterScope; +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +import org.apache.cloudstack.storage.volume.datastore.PrimaryDataStoreHelper; +import org.apache.log4j.Logger; + +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.DeleteStoragePoolCommand; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; +import com.cloud.hypervisor.Hypervisor.HypervisorType; +import com.cloud.resource.ResourceManager; +import com.cloud.storage.StorageManager; +import com.cloud.storage.StoragePool; +import com.cloud.storage.StoragePoolHostVO; +import com.cloud.storage.dao.StoragePoolHostDao; +import com.cloud.utils.Pair; + +public class AbstractPrimaryDataStoreLifeCycleImpl { + private static final Logger s_logger = Logger.getLogger(AbstractPrimaryDataStoreLifeCycleImpl.class); + @Inject + AgentManager agentMgr; + @Inject + protected ResourceManager resourceMgr; + @Inject + StorageManager storageMgr; + @Inject + PrimaryDataStoreHelper dataStoreHelper; + @Inject + protected HostDao hostDao; + @Inject + protected StoragePoolHostDao storagePoolHostDao; + + private HypervisorType getHypervisorType(long hostId) { + HostVO host = hostDao.findById(hostId); + if (host != null) + return host.getHypervisorType(); + return HypervisorType.None; + } + + private List<HostVO> getPoolHostsList(ClusterScope clusterScope, HypervisorType hypervisorType) { + + List<HostVO> hosts = new ArrayList<HostVO>(); + + if (hypervisorType != null) { + hosts = resourceMgr + .listAllHostsInOneZoneNotInClusterByHypervisor(hypervisorType, clusterScope.getZoneId(), clusterScope.getScopeId()); + } else { + List<HostVO> xenServerHosts = resourceMgr Review Comment: Do we support Xenserver for change scope operation, is this needed? ########## ui/src/config/section/infra/primaryStorages.js: ########## @@ -133,6 +133,24 @@ export default { dataView: true, show: (record) => { return ['Maintenance', 'PrepareForMaintenance', 'ErrorInMaintenance'].includes(record.state) } }, + { + api: 'changeStoragePoolScope', + icon: 'arrow-right-outlined', + label: 'label.action.change.primary.storage.scope', + dataView: true, + popup: true, + show: (record) => { + return (record.state === 'Disabled' && + (record.hypervisor === 'KVM' || + record.hypervisor === 'VMware' || + record.hypervisor === 'HyperV' || + record.hypervisor === 'LXC' || + record.hypervisor === 'Any' || + record.hypervisor === 'Simulator') Review Comment: ```suggestion ['KVM', 'VMware', 'HyperV', 'LXC', 'Any', 'Simulator'].includes(record.hypervisor) ``` -- 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]
