Federico Simoncelli has posted comments on this change.

Change subject: [wip] providers: add the openstack image support
......................................................................


Patch Set 4: (10 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/IsoDomainListSyncronizer.java
Line 214:             List<RepoImage> problematicRepoFileList, final 
ImageFileType imageType) {
Line 215:         final RepoFileMetaDataDAO repoFileMetaDataDao = 
repoStorageDom;
Line 216: 
Line 217:         Provider provider = providerDao.get(new 
Guid(storageDomain.getStorage()));
Line 218:         final OpenstackImageProviderProxy client = 
ProviderProxyFactory.getInstance().create(provider);
Postponed. To be discussed.
Line 219: 
Line 220:         Lock syncObject = getSyncObject(storageDomain.getId(), 
imageType);
Line 221:         try {
Line 222:             syncObject.lock();


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/OpenstackImageProviderProxy.java
Line 24: 
Line 25: 
Line 26: public class OpenstackImageProviderProxy implements ProviderProxy {
Line 27: 
Line 28:     enum GlanceImageFormat {
Because these are Glance specific formats that happen to have the same name. 
See:

http://docs.openstack.org/developer/glance/formats.html

I implemented only the ones I currently need/support.
Line 29:         RAW("raw"),
Line 30:         ISO("iso"),
Line 31:         COW("qcow2");
Line 32: 


Line 41:         }
Line 42:     }
Line 43: 
Line 44:     enum GlanceImageContainer {
Line 45:         BARE("bare");
Same concept, this comes from:

http://docs.openstack.org/developer/glance/formats.html

I implemented only the one I'm able to deal with at the moment.
Line 46: 
Line 47:         private String value;
Line 48: 
Line 49:         GlanceImageContainer(String value) {


Line 68:     }
Line 69: 
Line 70:     @Override
Line 71:     public void testConnection() {
Line 72:     }
Done
Line 73: 
Line 74:     @Override
Line 75:     public List<? extends Certificate> getCertificateChain() {
Line 76:         return null;


Line 80:         return DbFacade.getInstance();
Line 81:     }
Line 82: 
Line 83:     @Override
Line 84:     public void onAddition() {
1. ui that I know of, rest too probably, 2. yes
Line 85:         // Storage domain static
Line 86:         StorageDomainStatic domainStaticEntry = new 
StorageDomainStatic();
Line 87:         domainStaticEntry.setId(Guid.newGuid());
Line 88:         domainStaticEntry.setStorage(provider.getId().toString());


Line 95:         // Storage domain dynamic
Line 96:         StorageDomainDynamic domainDynamicEntry = new 
StorageDomainDynamic();
Line 97:         domainDynamicEntry.setId(domainStaticEntry.getId());
Line 98:         domainDynamicEntry.setAvailableDiskSize(0);
Line 99:         domainDynamicEntry.setUsedDiskSize(0);
Because glance doesn't let you discover the available space, and we don't want 
to monitor it anyway. What do you suggest? keeping it null? I remember I had 
problems leaving it null (failing code around), so I set it to 0 and in the UI 
it is displayed as "NA".
Line 100:         
getDbFacade().getStorageDomainDynamicDao().save(domainDynamicEntry);
Line 101:     }
Line 102: 
Line 103:     @Override


Line 108:         // removing the static and dynamic storage domain entries
Line 109:         for (StorageDomainStatic domainStaticEntry : 
domainStaticEntries) {
Line 110:             
getDbFacade().getStorageDomainDynamicDao().remove(domainStaticEntry.getId());
Line 111:             
getDbFacade().getStorageDomainStaticDao().remove(domainStaticEntry.getId());
Line 112:         }
It is, the transaction is in the onAddition/onRemoval caller. The scope we have 
here is too limited, the transaction involves other actions.
Line 113:     }
Line 114: 
Line 115:     private Provider getProvider() {
Line 116:         return provider;


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/OpenstackImageProviderProperties.java
Line 1: package org.ovirt.engine.core.common.businessentities;
Line 2: 
Line 3: public class OpenstackImageProviderProperties extends 
TenantProviderProperties {
Because we might need additional properties along the way. It is also confusing 
seeing generics using TenantProviderProperties when dealing with 
OpenstackImageProvider. Introducing this in the future would be slightly harder 
(we'd need to go over the TenantProviderProperties usages and understand where 
to change it to OpenstackImageProviderProperties).
Line 4: 
Line 5:     private static final long serialVersionUID = -3887979451360188295L;
Line 6: 
Line 7:     @Override


....................................................
File 
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/provider/ProviderDaoDbFacadeImpl.java
Line 39:                 tenantName = networkProperties.getTenantName();
Line 40:                 pluginType = networkProperties.getPluginType().name();
Line 41:                 break;
Line 42:             case OPENSTACK_IMAGE:
Line 43:                 OpenstackImageProviderProperties imageProperties =
The base class exists and they both inherit from that (in this patch). Please 
elaborate what you are suggesting here.
Line 44:                         (OpenstackImageProviderProperties) 
entity.getAdditionalProperties();
Line 45:                 tenantName = imageProperties.getTenantName();
Line 46:                 break;
Line 47:             default:


....................................................
File 
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageDomainStaticDAO.java
Line 24:      * @param storage
Line 25:      *            the storage value
Line 26:      * @return the domain
Line 27:      */
Line 28:     List<StorageDomainStatic> getAllByStorage(String storage);
Please check the sql query (that is provided in this patch as well).
Line 29: 
Line 30:     /**
Line 31:      * Retrieves all domains of the specified type for the specified 
pool.
Line 32:      *


-- 
To view, visit http://gerrit.ovirt.org/15899
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I424998de6f6514d65938484fffa5405e35857e43
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to