Merge lp:~wallyworld/goose/RunServer-userData-fix into lp:~gophers/goose/trunk

[Ian Booth]

Fix RunServerOpts Userdata

The UserData attribute of RunServerOpts was []byte, but it needs to be string to make juju bootstrap work.

All the current nova tests simply ignore this attribute - they pass in nil. I tried writing a test to set up
a cloud init config with a simple script command and introduced a deliberate error but the nova create server
operation succeeded nonetheless. Yet the huge cloud init script used by juju bootstrap fails. So I'm not sure
how to set up a small, self contained test in goose for the userData attribute. The code in this merge proposal
makes juju happy though, apart from that fact that Canonistack sometimes gives a "Resource limit exeeded" error
when bootstrapping a juju instance.

https://codereview.appspot.com/7221070/

[Ian Booth (wallyworld)]

Reviewers: mp+145539_code.launchpad.net,

Message:
Please take a look.

Description:
Fix RunServerOpts Userdata

The UserData attribute of RunServerOpts was []byte, but it needs to be
string to make juju bootstrap work.

All the current nova tests simply ignore this attribute - they pass in
nil. I tried writing a test to set up
a cloud init config with a simple script command and introduced a
deliberate error but the nova create server
operation succeeded nonetheless. Yet the huge cloud init script used by
juju bootstrap fails. So I'm not sure
how to set up a small, self contained test in goose for the userData
attribute. The code in this merge proposal
makes juju happy though, apart from that fact that Canonistack sometimes
gives a "Resource limit exeeded" error
when bootstrapping a juju instance.

https://code.launchpad.net/~wallyworld/goose/RunServer-userData-fix/+merge/145539

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7221070/

Affected files:
   A [revision details]
   M nova/live_test.go
   M nova/nova.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision: <email address hidden>

Index: nova/live_test.go
=== modified file 'nova/live_test.go'
--- nova/live_test.go 2013-01-29 16:04:31 +0000
+++ nova/live_test.go 2013-01-30 04:40:58 +0000
@@ -67,9 +67,8 @@
    Name: name,
    FlavorId: testFlavourId,
    ImageId: testImageId,
- UserData: nil,
   }
- instance, err = s.nova.RunServer(opts)
+ instance, err = s.nova.RunServer(opts, nil)
   if err != nil {
    return nil, err
   }

Index: nova/nova.go
=== modified file 'nova/nova.go'
--- nova/nova.go 2013-01-29 16:32:50 +0000
+++ nova/nova.go 2013-01-30 04:40:58 +0000
@@ -242,19 +242,18 @@
   Name string `json:"name"` //
Required
   FlavorId string `json:"flavorRef"` //
Required
   ImageId string `json:"imageRef"` //
Required
- UserData []byte `json:"user_data"` //
Optional
+ UserData string `json:"user_data"` //
Optional
   SecurityGroupNames []SecurityGroupName `json:"security_groups"` //
Optional
  }

  // RunServer creates a new server, based on the given RunServerOpts.
-func (c *Client) RunServer(opts RunServerOpts) (*Entity, error) {
+func (c *Client) RunServer(opts RunServerOpts, userData []byte) (*Entity,
error) {
   var req struct {
    Server RunServerOpts `json:"server"`
   }
   req.Server = opts
- if opts.UserData != nil {
- encoded := base64.StdEncoding.EncodeToString(opts.UserData)
- req.Server.UserData = []byte(encoded)
+ if userData != nil {
+ req.Server.UserData = base64.StdEncoding.EncodeToString(userData)
   }
   var resp struct {
    Server Entity `json:"server"`

[John A Meinel (jameinel)]

I think this is the right fix. ISTR that if you use []byte in a data
structure, JSON.Marshal(struct) will encode "foo" as [102, 111, 111].

So I like the change, but I'd really like to see a test for it, since it
is one of those "everything seemed fine but failed in reality" issues.

https://codereview.appspot.com/7221070/

[Dimiter Naydenov (dimitern)]

On 2013/01/30 04:50:43, wallyworld wrote:
> Please take a look.

LGTM, but why did you move UserData out of the RunServerOpts struct?

https://codereview.appspot.com/7221070/

[John A Meinel (jameinel)]

On 2013/01/30 06:57:14, dimitern wrote:
> On 2013/01/30 04:50:43, wallyworld wrote:
> > Please take a look.

> LGTM, but why did you move UserData out of the RunServerOpts struct?

I don't think he removed it, he just changed its type from a []byte to a
string.
However, if you still want to pass in a []byte, you can't just shove it
in the struct, because that slot now holds a string.

Then again, we shouldn't really be using the struct for
bytes-on-the-wire as the same struct for
passing-parameters-to-the-function. Because of differences like this.

Maybe it means the conversion to string should be done at a higher level
(before passing it into this function). But it does look like the other
conversions are being done at this level.

https://codereview.appspot.com/7221070/

[Dimiter Naydenov (dimitern)]

On 2013/01/30 04:50:43, wallyworld wrote:
> Please take a look.

NOT LGTM - as discussed, this change is not needed, because the problem
is we shouldn't encode userData in base64, as being []byte this is done
automatically by go (see the docs). I have a CL which just removes the
encoding and will propose it shortly.

https://codereview.appspot.com/7221070/

Unmerged revisions

58. By Ian Booth on 2013-01-30

Change RunServerOpts.UserData from []byte to string to make server creation work for openstack provider

=== modified file 'nova/live_test.go'
--- nova/live_test.go	2013-01-29 16:04:31 +0000
+++ nova/live_test.go	2013-01-30 04:51:21 +0000
@@ -67,9 +67,8 @@
 		Name:     name,
 		FlavorId: testFlavourId,
 		ImageId:  testImageId,
-		UserData: nil,
 	}
-	instance, err = s.nova.RunServer(opts)
+	instance, err = s.nova.RunServer(opts, nil)
 	if err != nil {
 		return nil, err
 	}

=== modified file 'nova/nova.go'
--- nova/nova.go	2013-01-29 16:32:50 +0000
+++ nova/nova.go	2013-01-30 04:51:21 +0000
@@ -242,19 +242,18 @@
 	Name               string              `json:"name"`            // Required
 	FlavorId           string              `json:"flavorRef"`       // Required
 	ImageId            string              `json:"imageRef"`        // Required
-	UserData           []byte              `json:"user_data"`       // Optional
+	UserData           string              `json:"user_data"`       // Optional
 	SecurityGroupNames []SecurityGroupName `json:"security_groups"` // Optional
 }
 
 // RunServer creates a new server, based on the given RunServerOpts.
-func (c *Client) RunServer(opts RunServerOpts) (*Entity, error) {
+func (c *Client) RunServer(opts RunServerOpts, userData []byte) (*Entity, error) {
 	var req struct {
 		Server RunServerOpts `json:"server"`
 	}
 	req.Server = opts
-	if opts.UserData != nil {
-		encoded := base64.StdEncoding.EncodeToString(opts.UserData)
-		req.Server.UserData = []byte(encoded)
+	if userData != nil {
+		req.Server.UserData = base64.StdEncoding.EncodeToString(userData)
 	}
 	var resp struct {
 		Server Entity `json:"server"`