Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add support for managing volumes #41
Conversation
axw
reviewed
Mar 19, 2015
| + srv.mu.Lock() | ||
| + defer srv.mu.Unlock() | ||
| + | ||
| + delete(srv.volumes, v.Id) |
axw
Mar 19, 2015
Member
Should we also delete attachments here? What's the behaviour of EC2 if you try to delete a volume with attachments?
wallyworld
Mar 19, 2015
Member
EC2 requires a volume be in the available state before deletion (ie not attached). Added a check here in the test server.
axw
reviewed
Mar 19, 2015
| + srv.mu.Lock() | ||
| + defer srv.mu.Unlock() | ||
| + va := &volumeAttachment{ec2VolAttachment} | ||
| + srv.volumeAttachments[va.VolumeId] = va |
axw
reviewed
Mar 19, 2015
| + resp.XMLName = xml.Name{defaultXMLName, "AttachVolumeResponse"} | ||
| + resp.RequestId = reqId | ||
| + resp.VolumeId = va.VolumeId | ||
| + resp.InstanceId = va.InstanceId |
wallyworld
Mar 19, 2015
Member
Instance id validation is done in parseVolume attachment. Added availability zone check.
axw
reviewed
Mar 19, 2015
| + XMLName xml.Name | ||
| + ec2.VolumeAttachmentResp | ||
| + } | ||
| + resp.XMLName = xml.Name{defaultXMLName, "AttachVolumeResponse"} |
axw
reviewed
Mar 19, 2015
| +// | ||
| +// See http://goo.gl/uXCf9F for more details. | ||
| +type CreateVolume struct { | ||
| + AvailZone string |
axw
Mar 19, 2015
Member
xml tags on fields?
EDIT: I see other request types don't have it... I guess that's okay, if AWS doesn't care.
axw
reviewed
Mar 19, 2015
| + | ||
| + volumeToCreate := ec2.CreateVolume{ | ||
| + AvailZone: "us-east-1a", | ||
| + VolumeType: "ssd", |
axw
reviewed
Mar 19, 2015
| + resp.Device = va.Device | ||
| + resp.Status = "attaching" | ||
| + resp.AttachTime = time.Now().Format(time.RFC3339) | ||
| + return resp |
axw
Mar 19, 2015
Member
It's probably worth updating the instance-deletion code to handle DeleteOnTermination, as we'll need to test that functionality soon enough.
axw
reviewed
Mar 19, 2015
| + VolumeSize: 20, | ||
| + } | ||
| + resp1, err := s.ec2.CreateVolume(vol1) | ||
| + c.Assert(err, IsNil) |
axw
Mar 19, 2015
Member
I think you ought to add a deferred DeleteVolume right after creating the volume, in case the rest of the test fails.
wallyworld
Mar 19, 2015
Member
I was conforming with how other existing tests were written :-)
Done in this case.
axw
reviewed
Mar 19, 2015
| + VolumeSize: 20, | ||
| + } | ||
| + resp1, err := s.ec2.CreateVolume(vol1) | ||
| + c.Assert(err, IsNil) |
|
LGTM, thanks! |
wallyworld commentedMar 19, 2015
With this branch, it is now possible to:
Tested live.