cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Burwell <jburw...@basho.com>
Subject Re: Review Request: S3-backed Secondary Storage
Date Fri, 21 Dec 2012 01:42:48 GMT
Edison,

The ConfigurationManagerImpl import change was result of a merge conflict resolution.  During
that process, I noticed that the imports didn't conform to the projects guidelines, so I organized
them as part of the resolution process.

I have resolved the issue with the pom.xml change.  The change submitted was the result of
an upstream merge conflict resolution.  However, as I reviewed the changes in my branch and
master, the content was identical.  Therefore, I reverted pom.xml in my branch to master,
regenerated the patch, and submitted it via review board [1].

Thanks again for your help,
-John

[1] https://reviews.apache.org/r/8123/

On Dec 19, 2012, at 4:58 PM, Edison Su <Edison.su@citrix.com> wrote:

> If I click https://reviews.apache.org/r/8123/diff/5/,  I get:
> The patch to 'pom.xml' didn't apply cleanly. The temporary files have been left in '/tmp/reviewboard.gkgGgx'
for debugging purposes. `patch` returned: patching file /tmp/reviewboard.gkgGgx/tmpvi_JO8
Reversed (or previously applied) patch detected! Assume -R? [n] Apply anyway? [n] Skipping
patch. 1 out of 1 hunk ignored -- saving rejects to file /tmp/reviewboard.gkgGgx/tmpvi_JO8-new.rej
> 
> In diff5, there is lot of changes on java import in ConfigurationManagerImpl, while there
is no code change in the class itself, 
> 
> Something like:
> +++ b/server/src/com/cloud/configuration/ConfigurationManagerImpl.java
> @@ -16,35 +16,110 @@
> // under the License.
> package com.cloud.configuration;
> 
> +import java.net.URI;
> +import java.sql.PreparedStatement;
> +import java.sql.ResultSet;
> +import java.sql.SQLException;
> +import java.util.ArrayList;
> +import java.util.Collection;
> +import java.util.HashMap;
> +import java.util.HashSet;
> +import java.util.Hashtable;
> +import java.util.Iterator;
> +import java.util.List;
> +import java.util.Map;
> +import java.util.Set;
> +import java.util.UUID;
> +
> +import javax.ejb.Local;
> +import javax.naming.ConfigurationException;
> +import javax.naming.Context;
> +import javax.naming.NamingException;
> +import javax.naming.directory.DirContext;
> +import javax.naming.directory.InitialDirContext;
> +
> +import org.apache.log4j.Logger;
> +
> import com.cloud.acl.SecurityChecker;
> import com.cloud.alert.AlertManager;
> import com.cloud.api.ApiConstants.LDAPParams;
> import com.cloud.api.ApiDBUtils;
> -import com.cloud.api.commands.*;
> +import com.cloud.api.commands.CreateDiskOfferingCmd;
> +import com.cloud.api.commands.CreateNetworkOfferingCmd;
> +import com.cloud.api.commands.CreateServiceOfferingCmd;
> +import com.cloud.api.commands.CreateVlanIpRangeCmd;
> +import com.cloud.api.commands.CreateZoneCmd;
> +import com.cloud.api.commands.DeleteDiskOfferingCmd;
> +import com.cloud.api.commands.DeleteNetworkOfferingCmd;
> +import com.cloud.api.commands.DeletePodCmd;
> +import com.cloud.api.commands.DeleteServiceOfferingCmd;
> +import com.cloud.api.commands.DeleteVlanIpRangeCmd;
> +import com.cloud.api.commands.DeleteZoneCmd;
> +import com.cloud.api.commands.LDAPConfigCmd;
> +import com.cloud.api.commands.LDAPRemoveCmd;
> +import com.cloud.api.commands.ListNetworkOfferingsCmd;
> +import com.cloud.api.commands.UpdateCfgCmd;
> +import com.cloud.api.commands.UpdateDiskOfferingCmd;
> +import com.cloud.api.commands.UpdateNetworkOfferingCmd;
> +import com.cloud.api.commands.UpdatePodCmd;
> +import com.cloud.api.commands.UpdateServiceOfferingCmd;
> +import com.cloud.api.commands.UpdateZoneCmd;
> import com.cloud.capacity.dao.CapacityDao;
> import com.cloud.configuration.Resource.ResourceType;
> import com.cloud.configuration.dao.ConfigurationDao;
> -import com.cloud.dc.*;
> +import com.cloud.dc.AccountVlanMapVO;
> +import com.cloud.dc.ClusterVO;
> +import com.cloud.dc.DataCenter;
> import com.cloud.dc.DataCenter.NetworkType;
> +import com.cloud.dc.DataCenterIpAddressVO;
> +import com.cloud.dc.DataCenterLinkLocalIpAddressVO;
> +import com.cloud.dc.DataCenterVO;
> +import com.cloud.dc.HostPodVO;
> +import com.cloud.dc.Pod;
> +import com.cloud.dc.PodVlanMapVO;
> +import com.cloud.dc.Vlan;
> import com.cloud.dc.Vlan.VlanType;
> -import com.cloud.dc.dao.*;
> +import com.cloud.dc.VlanVO;
> +import com.cloud.dc.dao.AccountVlanMapDao;
> +import com.cloud.dc.dao.ClusterDao;
> +import com.cloud.dc.dao.DataCenterDao;
> +import com.cloud.dc.dao.DataCenterIpAddressDao;
> +import com.cloud.dc.dao.DataCenterLinkLocalIpAddressDaoImpl;
> +import com.cloud.dc.dao.HostPodDao;
> +import com.cloud.dc.dao.PodVlanMapDao;
> +import com.cloud.dc.dao.VlanDao;
> import com.cloud.deploy.DataCenterDeployment;
> import com.cloud.domain.Domain;
> import com.cloud.domain.DomainVO;
> import com.cloud.domain.dao.DomainDao;
> import com.cloud.event.ActionEvent;
> import com.cloud.event.EventTypes;
> -import com.cloud.exception.*;
> +import com.cloud.exception.ConcurrentOperationException;
> +import com.cloud.exception.InsufficientCapacityException;
> +import com.cloud.exception.InvalidParameterValueException;
> +import com.cloud.exception.PermissionDeniedException;
> +import com.cloud.exception.ResourceAllocationException;
> +import com.cloud.exception.ResourceUnavailableException;
> import com.cloud.host.HostVO;
> import com.cloud.hypervisor.Hypervisor.HypervisorType;
> -import com.cloud.network.*;
> +import com.cloud.network.IPAddressVO;
> +import com.cloud.network.Network;
> import com.cloud.network.Network.Capability;
> import com.cloud.network.Network.GuestType;
> import com.cloud.network.Network.Provider;
> import com.cloud.network.Network.Service;
> +import com.cloud.network.NetworkManager;
> +import com.cloud.network.NetworkVO;
> import com.cloud.network.Networks.BroadcastDomainType;
> import com.cloud.network.Networks.TrafficType;
> -import com.cloud.network.dao.*;
> +import com.cloud.network.PhysicalNetwork;
> +import com.cloud.network.PhysicalNetworkVO;
> +import com.cloud.network.dao.FirewallRulesDao;
> +import com.cloud.network.dao.IPAddressDao;
> +import com.cloud.network.dao.NetworkDao;
> +import com.cloud.network.dao.PhysicalNetworkDao;
> +import com.cloud.network.dao.PhysicalNetworkTrafficTypeDao;
> +import com.cloud.network.dao.PhysicalNetworkTrafficTypeVO;
> import com.cloud.network.vpc.VpcManager;
> import com.cloud.offering.DiskOffering;
> import com.cloud.offering.NetworkOffering;
> 
> 
> Are all these imports are necessary?
> 
> 
> 
>> -----Original Message-----
>> From: John Burwell [mailto:noreply@reviews.apache.org] On Behalf Of John
>> Burwell
>> Sent: Wednesday, December 19, 2012 1:40 PM
>> To: Edison Su
>> Cc: cloudstack; John Burwell
>> Subject: Re: Review Request: S3-backed Secondary Storage
>> 
>> 
>> 
>>> On Dec. 17, 2012, 5:51 p.m., edison su wrote:
>>>> Hi John, thanks for the patch. Few comments: 1. I can't apply the patch
>> against master top, there is conflict in pom.xml. 2. Could you remove the
>> unnecessary change in ConfigurationManagerImpl file?
>> 
>> Edison,
>> 
>> Which change in ConfigurationManagerImpl is unnecessary?  Looking
>> through the diff, I cant find any unnecessary changes.  Also, can you specify
>> the merge issue you are encountering with pom.xml?  I just merged my
>> feature branch with cloudstack/master and had no conflicts.
>> 
>> Thanks,
>> -John
>> 
>> 
>> - John
>> 
>> 
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/8123/#review14588
>> -----------------------------------------------------------
>> 
>> 
>> On Dec. 14, 2012, 11:54 p.m., John Burwell wrote:
>>> 
>>> -----------------------------------------------------------
>>> This is an automatically generated e-mail. To reply, visit:
>>> https://reviews.apache.org/r/8123/
>>> -----------------------------------------------------------
>>> 
>>> (Updated Dec. 14, 2012, 11:54 p.m.)
>>> 
>>> 
>>> Review request for cloudstack and edison su.
>>> 
>>> 
>>> Description
>>> -------
>>> 
>>> Backs NFS-based secondary storage with an S3-compatible object store.
>> Periodically, a reaper thread synchronizes templates and ISOs stored on a
>> NFS secondary storage mount with a configured S3 object store. It also
>> pushes snapshots to the object store when they are created and downloads
>> them in other zones on-demand. In addition to permitting the use of
>> commodity or IaaS storage solutions for static assets, it provides a means of
>> automatically synchronizing template and ISO assets across multiple zones.
>>> 
>>> For more information about the design of the patch, please see the design
>> document (https://cwiki.apache.org/confluence/display/CLOUDSTACK/S3-
>> backed+Secondary+Storage).
>>> 
>>> 
>>> This addresses bug CLOUDSTACK-509.
>>> 
>>> 
>>> Diffs
>>> -----
>>> 
>>>  api/src/com/cloud/api/commands/ListS3sCmd.java PRE-CREATION
>>>  build/package.xml 09ed939
>>>  pom.xml 4a4276e
>>>  server/src/com/cloud/configuration/ConfigurationManagerImpl.java
>> ef940e8
>>>  server/src/com/cloud/server/ManagementServerImpl.java 117be57
>>>  server/src/com/cloud/storage/dao/VMTemplateDao.java f5b6913
>>>  server/src/com/cloud/storage/dao/VMTemplateDaoImpl.java 2a0dfc8
>>>  server/src/com/cloud/storage/s3/S3ManagerImpl.java PRE-CREATION
>>>  tools/marvin/marvin/cloudstackConnection.py c805213
>>>  tools/marvin/marvin/deployDataCenter.py bdf08cc
>>>  utils/src/com/cloud/utils/db/DbUtil.java feef7b3
>>> 
>>> Diff: https://reviews.apache.org/r/8123/diff/
>>> 
>>> 
>>> Testing
>>> -------
>>> 
>>> I am submitting patch to begin the feedback process while we complete
>> integration testing.  I have verified that it does not interfere with normal
>> CloudStack operations when S3-backed Secondary Storage is disabled (the
>> default setting) .  I have successfully tested operation of single zone
>> template and ISO scenarios on devcloud described in the design document.  I
>> am currently working through some issues in our multi-zone test
>> environment to complete all scenarios described.  The following are the
>> known deficiencies of the current implementation which I plan to correct in a
>> subsequent patch:
>>> 
>>>   * Cross zone garbage collection: When a global asset is deleted from one
>> zone's secondary storage, it is not deleted from the secondary storage of
>> other zones which have downloaded it from the object store
>>>   * S3 Configuration Update: The API only supports adding an object store
>> configuration.  Users should be able to edit the access key, secret key,
>> connection timeout. max error retries, and socket timeout.
>>>   * Multi-threaded Uploads: Permit the upload of multiple assets to the
>> object store simultaneously to decrease the propagation latency across all
>> zones.
>>> 
>>> 
>>> Screenshots
>>> -----------
>>> 
>>> S3 Configuration Form
>>>  https://reviews.apache.org/r/8123/s/13/
>>> S3 Enable Menu on the Zone Tab
>>>  https://reviews.apache.org/r/8123/s/14/
>>> 
>>> 
>>> Thanks,
>>> 
>>> John Burwell
>>> 
>>> 
> 


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message