Skip to content
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

Make EphemeralLvm::Helper support all EC2 instance types #11

Merged
merged 2 commits into from Feb 14, 2014

Conversation

Projects
None yet
4 participants
@sonnysideup
Copy link
Contributor

sonnysideup commented Feb 13, 2014

This helper module is broken and raises an exception when executing against the m1.small and c1.medium instance types in AWS. This happens because the block device for those instances is named /dev/sda2 (see here) and the current logic does not allow for a numeric suffix.

This PR adds a test to verify the failure and extends the helper to allow for a single digit at the end of the device name.

@rs-cookbooks-ci

This comment has been minimized.

Copy link

rs-cookbooks-ci commented Feb 13, 2014

Can one of the admins verify this patch?

@arangamani

This comment has been minimized.

Copy link
Member

arangamani commented Feb 13, 2014

@rs-cookbooks-ci test this please

@sonnysideup

This comment has been minimized.

Copy link
Contributor Author

sonnysideup commented Feb 13, 2014

all good?

@@ -87,11 +87,11 @@ def self.get_ephemeral_devices(cloud, node)
#
def self.fix_device_mapping(devices, node_block_devices)
devices.map! do |device|
if node_block_devices.include?(device.match(/\/dev\/([a-z]+)$/)[1])
if node_block_devices.include?(device.match(/\/dev\/([a-z]+[0-9]?)$/)[1])

This comment has been minimized.

Copy link
@arangamani

arangamani Feb 13, 2014

Member

Are we sure that it will always be a single digit number if there is a number?

This comment has been minimized.

Copy link
@sonnysideup

sonnysideup Feb 13, 2014

Author Contributor

According to the AWS' documentation, yes. There's a link in this PR to the specifics.

This comment has been minimized.

Copy link
@douglaswth

douglaswth Feb 13, 2014

Contributor

I think it would be better to support an arbitrary number of digits so we don't have to fix it again if that does come up.

This comment has been minimized.

Copy link
@sonnysideup

sonnysideup Feb 13, 2014

Author Contributor

IMO, it's best to avoid a greedy regex. Given what is currently known, this fixes the bug and satisfies AWS' current ephemeral device naming scheme. I don't see any reason to guard against a situation that may never occur, and that change in the future would be minor and maintain backwards compatibilityl

This comment has been minimized.

Copy link
@douglaswth

douglaswth Feb 13, 2014

Contributor

The whole purpose of this regex is to get the device name without the leading /dev/ so it should be greedy; in fact, I think we should probably just change it to /^\/dev\/(.+)$/. Also, this code is not necessarily just for AWS EC2, but also any other cloud that has the same block device mapping information in its metadata such as OpenStack.

This comment has been minimized.

Copy link
@sonnysideup

sonnysideup Feb 13, 2014

Author Contributor

I don't enough about OpenStack to weigh in regarding their block devices. I'll change the regex: do we want a multi-number capture or a capture all?

This comment has been minimized.

Copy link
@douglaswth

douglaswth Feb 13, 2014

Contributor

Let's just go with a capture all. Thanks!

opens regex capture
remove expected format constraint and simply captures the entire device name
@arangamani

This comment has been minimized.

Copy link
Member

arangamani commented Feb 14, 2014

@rs-cookbooks-ci test this please

douglaswth added a commit that referenced this pull request Feb 14, 2014

Merge pull request #11 from orgsync-cookbooks/fix-ephemeral-helper
Make EphemeralLvm::Helper support all EC2 instance types

@douglaswth douglaswth merged commit 5066046 into rightscale-cookbooks:master Feb 14, 2014

1 check passed

default Merged build finished.
Details
@sonnysideup

This comment has been minimized.

Copy link
Contributor Author

sonnysideup commented Feb 14, 2014

do you all plan to push a bug release to the community site since this was broken?

@douglaswth

This comment has been minimized.

Copy link
Contributor

douglaswth commented Feb 14, 2014

Yes, we do, but it might not be out until Tuesday.

@sonnysideup

This comment has been minimized.

Copy link
Contributor Author

sonnysideup commented Feb 14, 2014

sounds good, thanks for the heads up

@arangamani

This comment has been minimized.

Copy link
Member

arangamani commented Feb 18, 2014

@drywheat Version 1.0.3 is now published to the community.

@sonnysideup

This comment has been minimized.

Copy link
Contributor Author

sonnysideup commented Feb 18, 2014

great to hear, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.