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.**** >> >> **** >> >> **** >> >> ** ** >> > >
