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