[virt-tools-list] [virt-convert] ec2 export module

[Cole Robinson]

On 09/22/2009 05:06 PM, Joey Boggs wrote:
> Joey Boggs wrote:
>> I've finally gotten the time to port ec2-converter from 
>> appliance-tools into virt-convert. This module works a little 
>> differently than the normal routine. There is no output configuration 
>> file and no disk conversion, rather the disk being mounted, all 
>> labeled file systems mounted and rsync'ed into a newly created file 
>> system image. The new fs is then injected with all necessary 
>> configurations(fstab,ssh, eth0 config, matching kernel rpm, ami-tools 
>> and api-tools) The kernel url is hardcoded to the latest Fedora EC2 
>> kernel, but can be made to accept any in the future. The same follows 
>> with the ec2-api-tools. The output image can then be used against 
>> Amazon's ami-tools for bundling/upload.
>>
>> Corrensponding EC2 kernel information
>> EC2 AKI:   aki-a71cf9ce
>> EC2 ARI:   ari-a51cf9cc


> rogue ec2config.py file jumped into parsers directory and got added by 
> accident, updated patch has it removed.
> 

Hi Joey,

Thanks for the patch. However, I think it needs a lot of cleanup to even
facilitate a good review. Some overall comments:

- There are very few comments in the code, it is pretty difficult to
decode what exactly is going on. A comment before every step in
parsers/ec2.py would help, as well as comments for each function in the
ec2 helper files. Also, a paragraph explaining how exactly conversion to
ec2 works from start to finish (either in the code or when you repost
the patch), since this is quite different from the other virtconv examples.

- Please run this patch through 'python setup.py check' in the virtinst
repo directory and fix all ec2 related errors (you'll need to install
pylint).

- We cannot call 'fail' from virtconv. Everything in the virtconv
directory is an API: where you use 'fail', we should be raising an
exception and letting virt-convert handle it. Same goes for using
'print', use either logging.info or logging.debug please.

- Please move the virtconv/ec2* files to something like
virtconv/parsers/ec2helpers/*. virtconv/parsers/ec2.py is fine where it is.

- The code needs to be mindful of cleanup. There seems to be a lot of
mounting, downloading, and directory creation going on, and no attempt
at error recovery cleanup.

- On a basic smoketest, this errored immediately with invalid imports
(virtconv.cli and virtconv.rpmcheck). Please test your code before
submitting.

When you repost, an example virt-convert invocation with the command
line output would also be helpful.

Thanks,
Cole