Skip to content
This repository has been archived by the owner on Jul 24, 2020. It is now read-only.

Add -B (boot volume ID) to knife rackspace server create #95

Merged
merged 1 commit into from Sep 8, 2015

Conversation

tdooner
Copy link
Contributor

@tdooner tdooner commented Feb 17, 2015

As the commit message says:

The compute1-* and memory1-* lines of servers currently cannot currently
be created by the knife rackspace server create command because those
servers must be booted from a cloud block storage (CBS) volume.

Conveniently, @smashwilson added a :boot_volume_id option to fog's
create_server call which sets up a CBS volume based on the given image
id and assigns it to the newly created server.

This commit adds a new option, -B, to the server create command that
passes through a value for :boot_volume_id.

Since this option is explicitly for preexisting boot volumes, this
commit also makes the -I flag smart enough to know whether it should
create the server passing the :image_id configuration option or whether
it should create the server passing the :boot_image_id option.

Fixes #91.

I'm particularly curious what you guys think of the flavor.disk == 0 check to determine whether the flavor is CBS-backed only. In an earlier implementation of this, I added a third flag that was --boot-image-id, but it felt like this gem should abstract away the difference if you just want to create a server from a given image.

@chef-supermarket
Copy link

Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog.

GitHub Users Who Are Not Authorized To Contribute

The following GitHub users do not appear to have signed a CLA:

Please sign the CLA here.

@chef-supermarket
Copy link

Hi. Your friendly Curry bot here. Just letting you know that all commit authors have become authorized to contribute. I have added the "Signed CLA" label to this issue so it can easily be found in the future.

@vmihailenco
Copy link

Ping. Any news on this?

@tdooner
Copy link
Contributor Author

tdooner commented May 19, 2015

It should be ready to merge. We've been using it to provision production nodes for some time now.

@martinb3
Copy link
Contributor

👍 I'd love to see this merged.

@phumpal
Copy link

phumpal commented May 27, 2015

Any update here?

@jjasghar
Copy link
Contributor

It would be nice to have some specs around this. I know we don't have a lot, but it's good to start some place :)

@jjasghar
Copy link
Contributor

Also some documentation would be nice also for the README.

martinb3 added a commit to martinb3/knife-rackspace that referenced this pull request Jun 24, 2015
Per chef-boneyard#91, this adds some documentation on top fo the chef-boneyard#95 commits providing the needed functionality. The thread in chef-boneyard#95 asks for some specs, but we don't really have any specs that test each parameter in knife-rackspace, so there isn't really any established pattern. The project appears to have some functional tests ensuring the right FOG behavior (ensuring particular endpoints), but it does not test most parameters as far as I can tell.
@martinb3
Copy link
Contributor

It would be nice to have some specs around this. I know we don't have a lot, but it's good to start some place :)

As far as I can tell, the project doesn't have much in the way of specs that test each parameter. If you would be able to point me at some examples of how you'd like this parameter tested, I'd be happy to add them.

In the mean time though, no one is able to build boot-from-volume servers in our cloud using knife, so unless there's a very straightforward path, I ask that this functionality not be used as an opportunity to start holding up PRs for test refactoring (in other words, we're desperate for this feature!).

Also some documentation would be nice also for the README.

I created PR #104 that includes these commits, and adds some documentation for the README.

Thank you for your consideration!

@jjasghar
Copy link
Contributor

It would be nice to have some specs around this. I know we don't have a lot, but it's good to start some place :)

As far as I can tell, the project doesn't have much in the way of specs that test each parameter. If you would be able to point me at some examples of how you'd like this parameter tested, I'd be happy to add them.

Yep, that's the problem. With something like this we need to create a mock'd out rackspace api endpoint and confirm the changes we make get the responses we want. I don't work for rackspace; so i don't have access to something like that, and i understand reverse engineering it would be an endeavor to say the least.

On the other hand just some simple unit tests for the different methods and portions of the gem you're changing is at least a start. I'd love to enforce the rule no new changes without a coinciding test/change in test. But I understand that's a long way off.

Just my rationality.

@martinb3
Copy link
Contributor

Hi there --

On the other hand just some simple unit tests for the different methods and portions of the gem you're changing is at least a start.

I've looked through the changes, and there really isn't any logic until the first flavor look up, and that requires valid API credentials or a mocked endpoint. I thought I'd test the logic added RE: boot volume IDs, but even that requires API requests to look up which flavors require one. So I'm not really sure what else to do.

But we definitely have customers all waiting on this change, and that's why I did #104 to introduce documentation. If you can't provide any specific guidance on tests you'd like to see (even just enumerating a specific example of a test we could write now based on the changes -- I couldn't find any), I'm not sure what else I can do to help. It feels like this is going to just be another one of the PRs on this repo that are stagnant.

@jjasghar
Copy link
Contributor

Hey @martinb3 it seems there's a wire crossed somewhere.

I talked to @mattray and it seems we've lost something in how we merge PRs and push the gem for this project. Maybe we should have a sync up on this. Can ya'll drop me an email at jj@chef.io so we can set up a call?

Please don't hesitate to CC other people that we should probably sync with also.

CC: @mdarby

@mdarby
Copy link

mdarby commented Jul 1, 2015

I'm happy to help any way I can.

@chef-supermarket
Copy link

Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog.

GitHub Users Who Are Not Authorized To Contribute

The following GitHub users do not appear to have signed a CLA:

Please sign the CLA here.

@chef-supermarket
Copy link

Hi. Your friendly Curry bot here. Just letting you know that all commit authors have become authorized to contribute. I have added the "Signed CLA" label to this issue so it can easily be found in the future.

@tdooner
Copy link
Contributor Author

tdooner commented Jul 9, 2015

I just force-pushed a new version of this PR which excludes commits from CLA-requiring authors, so this should be good to go. I'll take a quick stab at tests, but I think we could merge this and test it by trying it.

@catapaicu
Copy link

Hi guys, Is the -B (#95) implemented after all? I really need this feature in knife rackspace.
Thanks.

@martinb3
Copy link
Contributor

martinb3 commented Sep 4, 2015

@catapaicu I think @tdooner and @mdarby were going to combine efforts on this functionality. Not sure what the current status is. I'll let them respond. We should be able to move on this soon.

The compute1-* and memory1-* lines of servers currently cannot currently
be created by the `knife rackspace server create` command because those
servers must be booted from a cloud block storage volume.

Conveniently, @smashwilson [added a :boot_volume_id option][1] to fog's
create_server call which sets up a CBS volume based on the given image
id and assigns it to the newly created server.

This commit adds a new option, -B, to the server create command that
passes through a value for `:boot_volume_id`.

Since this option is explicitly for preexisting boot volumes, this
commit also makes the -I flag smart enough to know whether it should
create the server passing the :image_id configuration option or whether
it should create the server passing the :boot_image_id option.

Fixes chef-boneyard#91.

[1]: fog/fog@07a19df
@tdooner
Copy link
Contributor Author

tdooner commented Sep 4, 2015

I just rebased the commit and resolved the conflicts introduced by the rackconnect v3 changes.

I can't verify the Rackconnect v3 changes but I was able to manually verify that there are no obvious regressions via a test of knife rackspace server create.

@martinb3
Copy link
Contributor

martinb3 commented Sep 8, 2015

@mdarby Are you ready for this to go in? It looks good to me.

@martinb3 martinb3 self-assigned this Sep 8, 2015
@m89an32d7w
Copy link

Please make a release of this. I would like to test it with Rackspace. Will post feedback.

@mdarby
Copy link

mdarby commented Sep 8, 2015

Sorry -- was pulled to another project and lost this thread. IIRC we should be fine; the Rackconnect V3 stuff was pretty isolated.

@martinb3
Copy link
Contributor

martinb3 commented Sep 8, 2015

👍 merging.

martinb3 added a commit that referenced this pull request Sep 8, 2015
Add -B (boot volume ID) to `knife rackspace server create`
@martinb3 martinb3 merged commit f83f9cd into chef-boneyard:master Sep 8, 2015
@martinb3
Copy link
Contributor

martinb3 commented Sep 8, 2015

Will watch CI and if it passes on master after the merge, will bump the version so it releases :)

@m89an32d7w
Copy link

Hi guys, Getting this error when trying to create a compute1-15 server
ERROR: Fog::Compute::RackspaceV2::BadRequest: [HTTP 400 | req-16a3051b-89f2-420b-954d-4e3c93502c43] Invalid imageRef provided. -
I'm using knife-rackspace 0.11.0 ofc.
Not sure what it means

@martinb3
Copy link
Contributor

@m89an32d7w Are you providing a boot-from-volume image ID when you invoke the command? Would you mind opening a separate issue and providing the command you're using, as well as any special knife configuration you've made in the config file? Aside from the API key and username, of course -- don't share those with us :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Enhancement Adds new functionality.
Development

Successfully merging this pull request may close these issues.

Support for new Rackspace instance types and Cloud Block Storage boot volumes
10 participants