New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LIBCLOUD-537] Added support for generic image management at Rackspace and EC2 #277

Closed
wants to merge 4 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@ghost

ghost commented Apr 17, 2014

I was planning to take care of GCE in this first push as well. However, Google presents an interesting challenge with their persistent disk model and their use of the snapshot and image terms. We could use snapshots as a replacement for images. However, this would then limit our ability to use the default images provided by Google. It seems like we actually need a factory for evaluating both the images and snapshots in order to fold in GCE. I completed the _supported_methods_image_management.rst as well in case we want to include it in the documentation. I did submit an email regarding this effort to the dev mail list but I didn't hear anything back so I just went ahead and made the change.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 17, 2014

Fixed docstring errors

ghost commented Apr 17, 2014

Fixed docstring errors

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Apr 21, 2014

Member

Sorry for the delay, I will look some time in the next couple of days.

Member

Kami commented Apr 21, 2014

Sorry for the delay, I will look some time in the next couple of days.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 22, 2014

No worries I'm just running off of my branch.

ghost commented Apr 22, 2014

No worries I'm just running off of my branch.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 24, 2014

Also noticing we need to add a generic state attribute to NodeImage in compute/base.py and have a statemap for NodeImage in compute/types.py. I'm primarily working to EC2/Rackspace so once we get an initial patch in place I can extend that to support a state on NodeImage for at least those drivers but specify it as None by default so it doesn't interfere with other drivers.

ghost commented Apr 24, 2014

Also noticing we need to add a generic state attribute to NodeImage in compute/base.py and have a statemap for NodeImage in compute/types.py. I'm primarily working to EC2/Rackspace so once we get an initial patch in place I can extend that to support a state on NodeImage for at least those drivers but specify it as None by default so it doesn't interfere with other drivers.

:param description: description for new image.
:type name: ``description``
:param reboot: description for new image.

This comment has been minimized.

@Kami

Kami Apr 24, 2014

Member

What does the reboot argument does?

Also, is this available across all the providers or is it provider specific?

@Kami

Kami Apr 24, 2014

Member

What does the reboot argument does?

Also, is this available across all the providers or is it provider specific?

This comment has been minimized.

@ghost

ghost Apr 24, 2014

For Amazon it makes sure the instance reboots in order to guarantee file system integrity. I looked into Amazon, Rackspace, Google, and even Azure. It does appear that this is an Amazon specific argument and it is not needed as the default should be okay. I will remove it from the base and put back the original implementation in the ec2.py file.

@ghost

ghost Apr 24, 2014

For Amazon it makes sure the instance reboots in order to guarantee file system integrity. I looked into Amazon, Rackspace, Google, and even Azure. It does appear that this is an Amazon specific argument and it is not needed as the default should be okay. I will remove it from the base and put back the original implementation in the ec2.py file.

raise NotImplementedError(
'get_image not implemented for this driver')
def copy_image(self, source_region, node_image, name, description=None):

This comment has been minimized.

@Kami

Kami Apr 24, 2014

Member

Probably a better argument order would be - self, node_image, source_region, name, description.

@Kami

Kami Apr 24, 2014

Member

Probably a better argument order would be - self, node_image, source_region, name, description.

This comment has been minimized.

@ghost

ghost Apr 24, 2014

agreed I will adjust that

@ghost

ghost Apr 24, 2014

agreed I will adjust that

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
:rtype: :class:`NodeImage`
"""
images = self.list_images(ex_image_ids=[image_id, ])

This comment has been minimized.

@Kami

Kami Apr 24, 2014

Member

This is a list and not a tuple so trailing comma is not needed.

@Kami

Kami Apr 24, 2014

Member

This is a list and not a tuple so trailing comma is not needed.

This comment has been minimized.

@ghost

ghost Apr 24, 2014

Okay I will fix these few things and make a commit here in a few minutes.

@ghost

ghost Apr 24, 2014

Okay I will fix these few things and make a commit here in a few minutes.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Apr 24, 2014

Member

This pull request mostly looks good to be, but it would be nice if it can also get reviwed by a couple of other people - /cc @mahendra @Jc2k

Member

Kami commented Apr 24, 2014

This pull request mostly looks good to be, but it would be nice if it can also get reviwed by a couple of other people - /cc @mahendra @Jc2k

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 24, 2014

Is there anything specific I need to do to trigger another Travis CI build?

ghost commented Apr 24, 2014

Is there anything specific I need to do to trigger another Travis CI build?

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Apr 24, 2014

Member

Nope, it should be triggered manually but it might take a while.

Member

Kami commented Apr 24, 2014

Nope, it should be triggered manually but it might take a while.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Apr 28, 2014

Member

Sorry for the delay, I've merged your changes into trunk.

I've noticed you manually created the supported methods rst file. Those files
are generated by ./contrib/generate_provider_feature_matrix_table.py script which I updated (08d7d95) to also support generating this file for image management API.

Member

Kami commented Apr 28, 2014

Sorry for the delay, I've merged your changes into trunk.

I've noticed you manually created the supported methods rst file. Those files
are generated by ./contrib/generate_provider_feature_matrix_table.py script which I updated (08d7d95) to also support generating this file for image management API.

@asfgit asfgit closed this in d2604af Apr 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment