There are also some commands that don't pass StorageFilerTO, but just a uuid of a storage pool (primaryStoragePoolNameLabel), I'm assuming those will need to be reworked to pass either a type or the whole StorageFilerTO?
On Tue, Dec 11, 2012 at 10:45 AM, Marcus Sorensen <shadow...@gmail.com>wrote: > There are a few snags, there are times when we get the storage pool by > asking LibvirtStorageAdaptor, in which case I don't know the adaptor type > to pass to KVMStoragePoolManager. For example: > > secondaryStoragePool = > _storagePoolMgr.getStoragePoolByURI(secondaryStorageUrl); > > In this case it parses the url and then runs createStoragePool on it, > returning the resulting pool. It seems these need a separate strategy > regarding pulling secondary storage out of Libvirt... do we just hard-code > these to libvirt for now, do we need to push the parsing of the url into > LibvirtComputingResource, or do you have any suggestions? > > > > On Mon, Dec 10, 2012 at 3:27 PM, Marcus Sorensen <shadow...@gmail.com>wrote: > >> Ok, so you do in fact want to change all of the calls in >> LibvirtComputingResource to pass storage adaptor type instead of defaulting >> to libvirt. That's what I was after. >> >> >> On Mon, Dec 10, 2012 at 2:49 PM, Edison Su <edison...@citrix.com> wrote: >> >>> One api: **** >>> >>> public KVMStoragePool getStoragePool(String storageAdaptorType, String >>> uuid)**** >>> >>> should be enough.**** >>> >>> Where the storageAdaptorType coming from? storageAdaptorType should be >>> passed down by mgt server in StorageFilerTO. Whenever you call >>> KVMStoragePoolManager, always pass the storageAdaptorType. In each >>> KVMStoragePoolManager’s api, call getStorageAdaptor at first, find out the >>> corresponding adaptor, then call it.**** >>> >>> ** ** >>> >>> For example, **** >>> >>> >>> public KVMStoragePool createStoragePool(String name, String host, int >>> port, String path, >>> **** >>> >>> >>> 56<https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=blob;f=plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java;h=2c0e0ac9b6f5d4e6862cbcfb5a6fd3fe9b5d29d6;hb=HEAD#l56> >>> String userInfo, >>> StoragePoolType type) { >>> **** >>> >>> ** ** >>> >>> may be implemented like this:**** >>> >>> storageadaptor adaptor = getstorageadaptor(type);**** >>> >>> adaptor.createPool(…)**** >>> >>> ** ** >>> >>> getStorageAadaptor can be implemented like this:**** >>> >>> storageadaptor getStorageAdaptor(StoragePoolType type) {**** >>> >>> storageadaptor adaptor = storageMapper.get(type.toString());*** >>> * >>> >>> if (adaptor == null) {**** >>> >>> adaptor = storageMapper.get(“libvirt”);**** >>> >>> }**** >>> >>> return adaptor;**** >>> >>> }**** >>> >>> *From:* Marcus Sorensen [mailto:shadow...@gmail.com] >>> *Sent:* Monday, December 10, 2012 4:25 PM >>> >>> *To:* Edison Su >>> *Cc:* cloudstack-dev@incubator.apache.org >>> *Subject:* Re: pluggable storage implementation**** >>> >>> ** ** >>> >>> I'm not quite sure what you mean on that last part. I was just thinking >>> as far as not having to go through and change all of the existing code to >>> pass 'null' or 'libvirt' when we mean to use libvirt. Also, some of these >>> items in KVMStoragePoolManager (see createStoragePool, >>> createDiskFromTemplate) have code for libvirt specific things (like 'pass >>> this to LibvirtStorageAdaptor if pool is RBD), so to avoid messing with >>> those maybe we keep things working with libvirt as default. **** >>> >>> ** ** >>> >>> Something like:**** >>> >>> private final Map<String, StorageAdaptor> _storageMapper = new >>> HashMap<String, StorageAdaptor>();**** >>> >>> ** ** >>> >>> _storageMapper.add("new-storage-adaptor-v1", new >>> MyNewV1StorageAdaptor(storagelayer));**** >>> >>> _storageMapper.add("new-storage-adaptor-v2", new >>> MyNewV2StorageAdaptor(storagelayer));**** >>> >>> ** ** >>> >>> //existing code**** >>> >>> public KVMStoragePoolManager(StorageLayer storagelayer, KVMHAMonitor >>> monitor) {**** >>> >>> this._storageAdaptor = new LibvirtStorageAdaptor(storagelayer);* >>> *** >>> >>> this._haMonitor = monitor;**** >>> >>> }**** >>> >>> ** ** >>> >>> public KVMStoragePool getStoragePool(String uuid) {**** >>> >>> return this._storageAdaptor.getStoragePool(uuid);**** >>> >>> }**** >>> >>> ** ** >>> >>> // pluggable getStoragePool**** >>> >>> public KVMStoragePool getStoragePool(String storageAdaptorType, >>> String uuid) {**** >>> >>> StorageAdaptor sa = storageMapper.get(storageAdaptorType);**** >>> >>> ** ** >>> >>> return sa.getStoragePool();**** >>> >>> }**** >>> >>> ** ** >>> >>> ** ** >>> >>> or alternatively... (but the libvirt specific portions will have to be >>> reworked)**** >>> >>> ** ** >>> >>> public KVMStoragePool getStoragePool(String uuid) {**** >>> >>> return getStoragePool("libvirt", uuid);**** >>> >>> }**** >>> >>> ** ** >>> >>> // pluggable getStoragePool**** >>> >>> public KVMStoragePool getStoragePool(String storageAdaptorType, >>> String uuid) {**** >>> >>> StorageAdaptor sa = storageMapper.get(storageAdaptorType);**** >>> >>> ** ** >>> >>> return sa.getStoragePool();**** >>> >>> }**** >>> >>> ** ** >>> >>> >>> >>> >>> **** >>> >>> ** ** >>> >>> On Mon, Dec 10, 2012 at 2:05 PM, Edison Su <edison...@citrix.com> wrote: >>> **** >>> >>> **** >>> >>> **** >>> >>> *From:* Marcus Sorensen [mailto:shadow...@gmail.com] >>> *Sent:* Monday, December 10, 2012 3:37 PM**** >>> >>> >>> *To:* Edison Su >>> *Cc:* cloudstack-dev@incubator.apache.org >>> *Subject:* Re: pluggable storage implementation**** >>> >>> **** >>> >>> Ok, that makes sense. So this will be a part of the storage refactor? If >>> you're not already working on it I'll play with it and see if I can create >>> a working implementation. **** >>> >>> No plan yet, so many stuff on the management server side to refactor. If >>> you will work on it, that’ll be great!**** >>> >>> **** >>> >>> I'm assuming we'd have in your example both a getStoragePool(String >>> uuid) that selects the old/default LibvirtStorageAdaptor and a >>> getStoragePool(String storagePoolType, String uuid), so as not to have to >>> redo any existing code?**** >>> >>> If you are just implementing a new storage for primary storage, then add >>> a new api called getprimarystorage(string pooltype, string uuid).**** >>> >>> On Mon, Dec 10, 2012 at 1:31 PM, Edison Su <edison...@citrix.com> wrote: >>> **** >>> >>> KVMStoragePoolManager should manage the mapping between storage pool >>> type and storageadaptor. **** >>> >>> 1. In KVMStoragePoolManager’s constructor, create a map between >>> storage pool type and storageadaptor:**** >>> >>> sample code:**** >>> >>> map<String, storageadaptor> storageMapper = new HashMap<String, >>> storageadaptor>();**** >>> >>> storageMapper.add(“your-storage-type”, new your-storage-adaptor);**** >>> >>> storageMapper.add(“libvirt-managed-storage”, new LibvirtStorageAdaptor); >>> **** >>> >>> 2. For each api of KVMStoragePoolManager, should pass down a storage >>> pool type, e.g:**** >>> >>> public KVMStoragePool getStoragePool(String storagePoolType, >>> String uuid) {**** >>> >>> storageadaptor adaptor = storageMapper.get(storagepoolType);**** >>> >>> if (adaptor == null) {**** >>> >>> adaptor = storageMapper.get(“libvirt-managed-storage”);*** >>> * >>> >>> }**** >>> >>> return adaptor.getStoragePool(uuid);**** >>> >>> }**** >>> >>> **** >>> >>> **** >>> >>> **** >>> >>> **** >>> >>> *From:* Marcus Sorensen [mailto:shadow...@gmail.com] **** >>> >>> *Sent:* Monday, December 10, 2012 3:06 PM >>> *To:* Edison Su >>> *Cc:* cloudstack-dev@incubator.apache.org**** >>> >>> *Subject:* Re: pluggable storage implementation**** >>> >>> **** >>> >>> Yeah, but I'm not seeing an easy/pluggable way to implement a storage >>> adaptor, where LibvirtComputingResource relies exclusively on >>> KVMStoragePoolManager, which is implementing LibvirtStorageAdaptor. I don't >>> see how to switch between storage adaptors in KVMStoragePoolManager.java >>> based on what primary storage type I happen to be acting upon.**** >>> >>> **** >>> >>> On Mon, Dec 10, 2012 at 12:51 PM, Edison Su <edison...@citrix.com> >>> wrote:**** >>> >>> **** >>> >>> **** >>> >>> *From:* Marcus Sorensen [mailto:shadow...@gmail.com] >>> *Sent:* Monday, December 10, 2012 2:06 PM >>> *To:* Edison Su >>> *Cc:* cloudstack-dev@incubator.apache.org >>> *Subject:* pluggable storage implementation**** >>> >>> **** >>> >>> Just curious, hadn't thought about this before but it seems that at >>> least on KVM (probably similar in Xen and VMware too?), there are two >>> separate issues with storage in the existing code. First, adding a new >>> storage type is a matter of adding in a new 'else if' or something in a >>> bunch of different places, as well as tweaking behavior to match the >>> storage type. Second, everything about the storage is tightly integrated >>> with Libvirt, meaning that if your storage type is not supported by Libvirt >>> it's much, much more difficult to implement.**** >>> >>> **** >>> >>> Are these both being addressed by the storage changes, for example can >>> we write a storage plugin that creates pools/volumes that libvirt doesn't >>> know about and still attach those to instances? Or would we need to patch >>> libvirt to utilize our storage first?**** >>> >>> **** >>> >>> [Edison] That’s the storageadaptor used for: >>> https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=blob;f=plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/StorageAdaptor.java;h=ef1e7c9302ab6ac8f197cf75bbf7235bba0235cf;hb=HEAD >>> **** >>> >>> The LibvirtStorageAdaptor is one of implementation of storageadaptor, >>> which is totally based on libvirt, If you have a new storage, not supported >>> by libvirt, then you can add a new implementation of storageadaptor.**** >>> >>> **** >>> >>> **** >>> >>> ** ** >>> >> >> >
