Merge lp:~julian-edwards/gwacl/vhd-define into lp:gwacl

[Julian Edwards]

Add a convenience function CreateVHD to create a VHD based on a filesystem image.

Add a convenience function to create a VHD based on a filesystem image. It works by making a page blob of a fixed 20Mb size and uploading a fixed VHD footer of 512 bytes to the last page. The supplied filesystem image is then uploaded from the start of the blob. The resulting blob is then able to be mounted as an Azure disk on an instance.

The point of all this is to enable a callsite to make arbitrary data available to the new instance, since Azure does not provide any other way.

This branch probably (read: definitely) needs more error testing, but quite frankly I find writing test code in Go to be utter drudgery and have lost the will to live. If you have any specific and useful ideas to help me out, go for it.

[Gavin Panella (allenap)]

Looks good, but I'm concerned about [3]. I'm not 100% sure that's a
bug, so Needs Information.

[1]

+    Size           int       // How many bytes from the Filesystem data to
+                             // upload.  *Must* be a modulo of 512.

I'm not sure modulo is the right word here (and I don't think it's a
noun either). How about s/modulo/multiple/?

[2]

+    if err != nil {
+        // This really shouldn't ever happen since there's a test to make sure
+        // it can be decoded.
+        panic("Unable to base64 decode VHD_FOOTER")
+    }

We lose the original error here. How about panic(err) instead?

[3]

+    err = context.PutPage(&PutPageRequest{
+        Container:  req.Container,
+        Filename:   req.Filename,
+        StartRange: VHD_SIZE-513,
+        EndRange:   VHD_SIZE-1,
+        Data:       dataReader,
+    })

This should probably use len(data) instead of literals.

Also, this means that the maximum size of the filesystem data is
VHD_SIZE - len(data), not VHD_SIZE.

[4]

+func (context *StorageContext) CreateVHD(req *CreateVHDRequest) error {

This isn't a general purpose function for creating a VHD, so I think
it ought to be renamed to, say, CreateVHDForUserData or something like
that.

review: Needs Information

[lp:~julian-edwards/gwacl/vhd-define]

126. By Julian Edwards on 2013-06-24

tweaks and review comments

[Julian Edwards (julian-edwards)]

On 21/06/13 18:58, Gavin Panella wrote:
> Review: Needs Information
>
> Looks good, but I'm concerned about [3]. I'm not 100% sure that's a
> bug, so Needs Information.
>
>
> [1]
>
> + Size int // How many bytes from the Filesystem data to
> + // upload. *Must* be a modulo of 512.
>
> I'm not sure modulo is the right word here (and I don't think it's a
> noun either). How about s/modulo/multiple/?

It seems perfectly cromulent to me. But I'll change it since I don't
want to trip people up deliberately.

> [2]
>
> + if err != nil {
> + // This really shouldn't ever happen since there's a test to make sure
> + // it can be decoded.
> + panic("Unable to base64 decode VHD_FOOTER")
> + }
>
> We lose the original error here. How about panic(err) instead?

Yeah, probably. But this should never happen, right :)

>
>
> [3]
>
> + err = context.PutPage(&PutPageRequest{
> + Container: req.Container,
> + Filename: req.Filename,
> + StartRange: VHD_SIZE-513,
> + EndRange: VHD_SIZE-1,
> + Data: dataReader,
> + })
>
> This should probably use len(data) instead of literals.

No - Start/EndRange is a target position in the blob and the data has to
be uploaded to this exact page at the end of the blob. These are not
sizes, they're positions.

> Also, this means that the maximum size of the filesystem data is
> VHD_SIZE - len(data), not VHD_SIZE.

That's fine. While I did mean to add 512 bytes to the size it's not a
problem in practice. You still get a big disk.

>
>
> [4]
>
> +func (context *StorageContext) CreateVHD(req *CreateVHDRequest) error {
>
> This isn't a general purpose function for creating a VHD, so I think
> it ought to be renamed to, say, CreateVHDForUserData or something like
> that.

I see what you mean. I didn't want to mention user data in gwacl though
as that's a cloud-init term and gwacl is agnostic. Mmmmm.

Perhaps CreateInstanceDataVHD? I'll embellish the [doc] comment as well.

[Gavin Panella (allenap)]

> > [3]
> >
> > +    err = context.PutPage(&PutPageRequest{
> > +        Container:  req.Container,
> > +        Filename:   req.Filename,
> > +        StartRange: VHD_SIZE-513,
> > +        EndRange:   VHD_SIZE-1,
> > +        Data:       dataReader,
> > +    })
> >
> > This should probably use len(data) instead of literals.
>
> No - Start/EndRange is a target position in the blob and the data has to
> be uploaded to this exact page at the end of the blob.  These are not
> sizes, they're positions.

Agreed. However, StartRange needs to be len(data) before EndRange.

>
> > Also, this means that the maximum size of the filesystem data is
> > VHD_SIZE - len(data), not VHD_SIZE.
>
> That's fine.  While I did mean to add 512 bytes to the size it's not a
> problem in practice.  You still get a big disk.

It's pertinent in documentation and in validation. For example,
req.Size must not be greater than (VHD_SIZE - len(data)) or you'll
overwrite the footer.

review: Approve

[lp:~julian-edwards/gwacl/vhd-define]

127. By Julian Edwards on 2013-06-25

Add comments explaining the exact VHD size and fix doc comment to reflect what size data can be attached

128. By Julian Edwards on 2013-06-25

Make sure data cannot overwrite the vhd footer

[Julian Edwards (julian-edwards)]

On 24/06/13 20:22, Gavin Panella wrote:
> Review: Approve
>
>>> [3]
>>>
>>> + err = context.PutPage(&PutPageRequest{
>>> + Container: req.Container,
>>> + Filename: req.Filename,
>>> + StartRange: VHD_SIZE-513,
>>> + EndRange: VHD_SIZE-1,
>>> + Data: dataReader,
>>> + })
>>>
>>> This should probably use len(data) instead of literals.
>>
>> No - Start/EndRange is a target position in the blob and the data has to
>> be uploaded to this exact page at the end of the blob. These are not
>> sizes, they're positions.
>
> Agreed. However, StartRange needs to be len(data) before EndRange.

len(data) is always 512. It has to be, or the footer is broken. I
don't want to make StartRange len(data) because if the data is ever
broken this stuff won't fail in an obvious way. If I do use len(data)
then in the case of the footer being >512 it will either fail on an
invalid page range, or upload to more than one page which will succeed
here but later incomprehensibly fail when adding the disk to the VM.

I can understand where you are coming from with this, but I feel that
len(data) is not as declarative of intent as the way I've done it, as to
a casual reader it could look like we're pushing to more than one page
of data with some arbitrary length footer data.

>>> Also, this means that the maximum size of the filesystem data is
>>> VHD_SIZE - len(data), not VHD_SIZE.
>>
>> That's fine. While I did mean to add 512 bytes to the size it's not a
>> problem in practice. You still get a big disk.
>
> It's pertinent in documentation and in validation. For example,
> req.Size must not be greater than (VHD_SIZE - len(data)) or you'll
> overwrite the footer.

Ah! You're right about that, I'll fix comments/docs and add an extra
fix/test to make sure it can't happen.

Cheers.

=== modified file 'Makefile'
--- Makefile	2013-04-30 21:50:12 +0000
+++ Makefile	2013-06-25 01:21:23 +0000
@@ -1,6 +1,6 @@
 # Build, and run tests.
 check: examples
-	go test ./...
+	go test -gocheck.v=true ./...
 
 all_source := $(shell find . -name '*.go' ! -name '*_test.go')
 

=== modified file 'storage.go'
--- storage.go	2013-06-11 16:33:25 +0000
+++ storage.go	2013-06-25 01:21:23 +0000
@@ -8,6 +8,8 @@
 
 import (
     "bytes"
+    "encoding/base64"
+    "fmt"
     "io"
     . "launchpad.net/gwacl/logging"
     "strconv"
@@ -114,3 +116,81 @@
     batch.Containers = containers
     return batch, nil
 }
+
+type CreateVHDRequest struct {
+    Container      string    // Container name in the storage account
+    Filename       string    // Specify the filename in which to store the VHD
+    FilesystemData io.Reader // A formatted filesystem, e.g. iso9660.
+    Size           int       // How many bytes from the Filesystem data to
+                             // upload.  *Must* be a multiple of 512.
+}
+
+// CreateInstanceDataVHD will take the supplied filesystem data and create an
+// Azure VHD in a page blob out of it.  The data cannot be bigger than
+// gwacl.VHD_SIZE-512.  This is intended to be used as a way of passing
+// arbitrary data to a new instance - create a disk here and then attach it to
+// the new instance.
+func (context *StorageContext) CreateInstanceDataVHD(req *CreateVHDRequest) error {
+    // We need several steps:
+    // 1. Create an empty page blob of exactly VHD_SIZE bytes (see
+    // vhd_footer.go)
+    // 2. Upload VHD_FOOTER to the last page of the blob.
+    // 3. Upload the supplied FilesystemData from the start of the blob.
+
+    var err error
+
+    if req.Size % 512 != 0 {
+        return fmt.Errorf("Size must be a multiple of 512")
+    }
+    if req.Size > VHD_SIZE-512 {
+        // Protect against writing over the VHD footer.
+        return fmt.Errorf("Size cannot be bigger than %d", VHD_SIZE-512)
+    }
+
+    // Step 1.
+    err = context.PutBlob(&PutBlobRequest{
+        Container: req.Container,
+        BlobType:  "page",
+        Filename:  req.Filename,
+        Size:      VHD_SIZE,
+    })
+
+    if err != nil {
+        return err
+    }
+
+    // Step 2.
+    data, err := base64.StdEncoding.DecodeString(VHD_FOOTER)
+    if err != nil {
+        // This really shouldn't ever happen since there's a test to make sure
+        // it can be decoded.
+        panic(err)
+    }
+    dataReader := bytes.NewReader(data)
+    err = context.PutPage(&PutPageRequest{
+        Container:  req.Container,
+        Filename:   req.Filename,
+        StartRange: VHD_SIZE-513,  // last page of the blob
+        EndRange:   VHD_SIZE-1,
+        Data:       dataReader,
+    })
+
+    if err != nil {
+        return err
+    }
+
+    // Step 3.
+    err = context.PutPage(&PutPageRequest{
+        Container:  req.Container,
+        Filename:   req.Filename,
+        StartRange: 0,
+        EndRange:   req.Size-1,
+        Data:       req.FilesystemData,
+    })
+
+    if err != nil {
+        return err
+    }
+
+    return nil
+}

=== modified file 'storage_test.go'
--- storage_test.go	2013-06-20 07:31:25 +0000
+++ storage_test.go	2013-06-25 01:21:23 +0000
@@ -5,6 +5,7 @@
 
 import (
     "bytes"
+    "encoding/base64"
     "fmt"
     "io/ioutil"
     . "launchpad.net/gocheck"
@@ -333,3 +334,71 @@
     c.Check(containers.Containers[0].Name, Equals, firstContainer)
     c.Check(containers.Containers[1].Name, Equals, lastContainer)
 }
+
+type testCreateInstanceDataVHD struct{}
+
+var _ = Suite(&testCreateInstanceDataVHD{})
+
+func (s *testCreateInstanceDataVHD) TestHappyPath(c *C) {
+    response := http.Response{
+        Status:     fmt.Sprintf("%d", http.StatusOK),
+        StatusCode: http.StatusCreated,
+    }
+    transport := &TestTransport2{}
+    transport.AddExchange(&response, nil) // putblob response
+    transport.AddExchange(&response, nil) // first putpage
+    transport.AddExchange(&response, nil) // second putpage
+    context := makeStorageContext(transport)
+
+    randomData := MakeRandomByteSlice(512)
+    dataReader := bytes.NewReader(randomData)
+
+    var err error
+
+    err = context.CreateInstanceDataVHD(&CreateVHDRequest{
+        Container: "container", Filename: "filename",
+        FilesystemData: dataReader, Size: 512})
+    c.Assert(err, IsNil)
+
+    // Check the PutBlob exchange.
+    c.Check(
+        transport.Exchanges[0].Request.Header.Get("x-ms-blob-type"),
+        Equals, "PageBlob")
+    expected_size := fmt.Sprintf("%d", VHD_SIZE)
+    c.Check(
+        transport.Exchanges[0].Request.Header.Get("x-ms-blob-content-length"),
+        Equals, expected_size)
+
+    // Check the PutPage for the footer exchange.
+    data, err := ioutil.ReadAll(transport.Exchanges[1].Request.Body)
+    c.Assert(err, IsNil)
+    expectedData, err := base64.StdEncoding.DecodeString(VHD_FOOTER)
+    c.Assert(err, IsNil)
+    c.Check(data, DeepEquals, expectedData)
+    expected_range := fmt.Sprintf("bytes=%d-%d", VHD_SIZE-513, VHD_SIZE-1)
+    c.Check(
+        transport.Exchanges[1].Request.Header.Get("x-ms-range"),
+        Equals, expected_range)
+
+    // Check the PutPage for the data exchange.
+    data, err = ioutil.ReadAll(transport.Exchanges[2].Request.Body)
+    c.Assert(err, IsNil)
+    c.Check(data, DeepEquals, randomData)
+
+    c.Check(
+        transport.Exchanges[2].Request.Header.Get("x-ms-range"),
+        Equals, "bytes=0-511")
+}
+
+func (s *testCreateInstanceDataVHD) TestSizeConstraints(c *C) {
+    var err error
+    context := makeStorageContext(&TestTransport{})
+
+    err = context.CreateInstanceDataVHD(&CreateVHDRequest{Size: 10})
+    c.Check(err, ErrorMatches, "Size must be a multiple of 512")
+
+    err = context.CreateInstanceDataVHD(&CreateVHDRequest{
+        Size: VHD_SIZE})
+    errString := fmt.Sprintf("Size cannot be bigger than %d", VHD_SIZE-512)
+    c.Check(err, ErrorMatches, errString)
+}

=== modified file 'vhd_footer.go'
--- vhd_footer.go	2013-06-21 03:39:08 +0000
+++ vhd_footer.go	2013-06-25 01:21:23 +0000
@@ -20,6 +20,7 @@
 // example), the easiest way is to use VirtualBox to define a new one, and
 // then do 'tail -c 512 | base64' on that file.
 
+const VHD_SIZE = 20972032  // This is 20Mib + 512 bytes
 const VHD_FOOTER = `
 Y29uZWN0aXgAAAACAAEAAP//////////GVKuuHZib3gABAACV2kyawAAAAABQAAAAAAAAAFAAAAC
 WgQRAAAAAv//5y4OEjVapHY7QpuodZNf77j6AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA