Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Added support for an Openstack provider. #1931
Conversation
wallyworld
reviewed
Mar 25, 2015
| + LoopProviderType: &loopProvider{logAndExec}, | ||
| + RootfsProviderType: &rootfsProvider{logAndExec}, | ||
| + TmpfsProviderType: &tmpfsProvider{logAndExec}, | ||
| + OpenstackProviderType: &openstackProvider{newGooseAdapter}, |
wallyworld
Mar 25, 2015
Owner
This doesn't belong here - common providers are those that work with any environment. The correct place to register this provider in the init() in providers/openstack. See how it's done for providers/ec2 and the ebs provider
wallyworld
reviewed
Mar 25, 2015
| +) | ||
| + | ||
| +const ( | ||
| + OpenstackProviderType = storage.ProviderType("openstack") |
wallyworld
Mar 25, 2015
Owner
This should be named CinderProviderType
Just as for AWS, we have a EBS_Provider type.
The storage provider type is named after the type of volume it manages, not the cloud on which it provisions the storage.
wallyworld
reviewed
Mar 25, 2015
| + | ||
| +// Scope implements storage.Provider. | ||
| +func (s *openstackProvider) Scope() storage.Scope { | ||
| + panic("not implemented") |
kat-co
Mar 30, 2015
Contributor
Are you sure? I just updated from trunk and the interface specifies Scope. Should I be rebasing from somewhere else?
axw
Mar 31, 2015
Member
Ian meant the method should return storage.EnvironScope. It needs to be implemented.
wallyworld
reviewed
Mar 25, 2015
| +var _ storage.VolumeSource = (*openstackVolumeSource)(nil) | ||
| + | ||
| +// CreateVolumes implements storage.VolumeSource. | ||
| +func (s *openstackVolumeSource) CreateVolumes(args []storage.VolumeParams) ([]storage.Volume, []storage.VolumeAttachment, error) { |
wallyworld
Mar 25, 2015
Owner
The ebs provider uses a deferred function to delete any created volumes in that operation when the method exists with an error. We should do the same here too to prevent resource leakage.
axw
Mar 25, 2015
Member
In case you're wondering, CreateVolumes does not need to be idempotent as it is not operating on existing resources (excluding instances). If a volume is created but never recorded in state, then later the storage provisioner will harvest them (when the code is written).
kat-co
Mar 30, 2015
Contributor
If this is the case, would it be OK to leave the deferred removal out of this function? It clutter's the purpose a bit. Not a huge deal either way.
axw
Mar 31, 2015
Member
Seeing as harvesting is not yet implemented, I'd prefer if we do it here for now at least. I think it'd be better to destroy them sooner-rather-than-later anyway. Also bear in mind that (AFAIK) tagging and creating are independent steps, so we'll need to make an effort to destroy created-but-not-tagged volumes.
wallyworld
reviewed
Mar 25, 2015
| + continue | ||
| + } | ||
| + | ||
| + if a.Attachment != nil { |
wallyworld
Mar 25, 2015
Owner
Don't do the attachment here. Restrict CreateVolumes to just creating. The storage provisioner will call Attach(). We were going to remove the Attachment attribute. It may still need to be there though because with ebs for example, we need to know the AZ at creation time. Need to check the latest on this though.
axw
Mar 25, 2015
Member
I'd suggest leaving it as-is for now. It's necessary to create the attachment with the volume iff the volume is non-persistent. For ec2 (branch forthcoming), the attachment is only created by CreateVolumes for that condition, otherwise it's deferred.
kat-co
Mar 30, 2015
Contributor
Also, if there is a case where we wouldn't want to attach, we need to update the docstring on VolumeParams. It specifies that if Attachment != nil, you should attach.
wallyworld
reviewed
Mar 25, 2015
| + | ||
| +// ValidateVolumeParams implements storage.VolumeSource. | ||
| +func (s *openstackVolumeSource) ValidateVolumeParams(params storage.VolumeParams) error { | ||
| + panic("not implemented") |
axw
Mar 25, 2015
Member
Definitely. When my ec2/ebs branch lands, it'll also need to complain if the params doesn't have an attachment or an instance-ID (for non-persistent volumes).
wallyworld
reviewed
Mar 25, 2015
| + | ||
| +// DetachVolumes implements storage.VolumeSource. | ||
| +func (s *openstackVolumeSource) DetachVolumes(params []storage.VolumeAttachmentParams) error { | ||
| + panic("not implemented") |
wallyworld
Mar 25, 2015
Owner
Can we implement this before landing? Or if not, let's do it straight away afterwards.
kat-co
Mar 25, 2015
Contributor
I need to add support for this in goose. I'll land as a separate branch.
wallyworld
reviewed
Mar 25, 2015
| + panic("not implemented") | ||
| +} | ||
| + | ||
| +type OpenstackAdapter interface { |
wallyworld
Mar 25, 2015
Owner
Does this need to be exported? Maybe it does, so needs a comment, as do the methods.
wallyworld
reviewed
Mar 25, 2015
| + AttachVolume(serverId, volumeId, mountPoint string) (storage.VolumeAttachment, error) | ||
| +} | ||
| + | ||
| +func newGooseAdapter(cfg *storage.Config) (OpenstackAdapter, error) { |
wallyworld
Mar 25, 2015
Owner
As with ebs, the Juju cloud provider itself creates a client with with to connect to the cloud. Can we look at reusing that existing code, probably via extracting to a helper method as was done for ec2/ebs.
wallyworld
reviewed
Mar 25, 2015
| + resp, err := ga.cinder.CreateVolume(cinder.CreateVolumeVolumeParams{ | ||
| + // NOTE: The Cinder documentation incorrectly states the | ||
| + // size parameter is in GB. It is actually GiB. | ||
| + Size: int(math.Ceil(float64(size / 1024))), |
wallyworld
Mar 25, 2015
Owner
This needs to be rounded up ie (size+1023)/1024
There are helper functions in providers/ec2 - mibToGib() and gibToMib()
Let's re-home those and use everywhere.
wallyworld
reviewed
Mar 25, 2015
| +func (s *openstackSuite) TestAttachVolumes(c *gc.C) { | ||
| + | ||
| + numCalls := 0 | ||
| + mockAdapter := &mockAdapter{ |
wallyworld
Mar 25, 2015
Owner
Just as the AWS test server was extended to listen and respond to EBS requests, I'm in 2 minds as to whether we should be doing that here too. All the other openstack tests use the openstack test server. I'd like to discuss this further.
axw
reviewed
Mar 25, 2015
| + return nil, nil, err | ||
| + } | ||
| + | ||
| + volAtt = append(volAtt, attachment) |
axw
Mar 25, 2015
Member
you'll still need to add createdVol to vols. I suggest doing what's in the else-block unconditionally, and just creating the attachment if Attachment!=nil
axw
reviewed
Mar 25, 2015
| + attachment, err := s.openstackClient.AttachVolume( | ||
| + string(a.Attachment.InstanceId), | ||
| + createdVol.VolumeId, | ||
| + "", // TODO(katco): Fill this in when attachment support is added to parameters. |
axw
Mar 25, 2015
Member
This presumably must be a TODO for this branch, unless OpenStack chooses a device name for you if you don't specify?
kat-co
Mar 25, 2015
Contributor
Sorry, I meant to ask in tonight's stand-up. This is the mount-point in OpenStack I didn't see a place to get this from the parameters passed in, and assumed it was work that needed to be done on the API. Where can I retrieve the mount point in this function?
axw
Mar 25, 2015
Member
Knowing what device name to specify requires fairly intimate knowledge of the provider. In EC2/EBS, we start from the first device name reserved/suggested for EBS volumes, and iterate until we find one that is not in use.
See: https://github.com/juju/juju/blob/master/provider/ec2/disks.go#L207
(the code that uses it at the moment is a bit broken; I'll have fixes up for review today)
kat-co
Mar 30, 2015
Contributor
It looks like for Cinder I can just pass in nothing and it will auto-assign. Nice!
axw
Mar 31, 2015
Member
Lovely. It is slightly concerning that the docs say "if supported", but let's run with that for now.
axw
reviewed
Mar 25, 2015
| + return storage.Volume{ | ||
| + VolumeId: volume.ID, | ||
| + Size: size, | ||
| + Tag: names.NewVolumeTag(volume.ID), |
axw
Mar 25, 2015
Member
Tag should be exactly what was in the storage.VolumeParams, not based off the provider ID.
kat-co
Apr 6, 2015
Contributor
Thanks, corrected. There is a bit of an oddity when listing Volumes as there are no storage.VolumeParams coming in. I'm passing in an empty tag. Any suggestions on how to handle that better?
axw
Apr 6, 2015
Member
Yeah, the DescribeVolumes signature is broken. It should be returning a different structure, one that does not include a tag; just info that the provider can supply. Passing in an empty tag is a reasonable thing to do for now, as long as there's a TODO to fix it once the API is updated.
axw
reviewed
Mar 25, 2015
| + return storage.VolumeAttachment{ | ||
| + Volume: names.NewVolumeTag(attachment.VolumeId), | ||
| + Machine: names.NewMachineTag(attachment.ServerId), | ||
| + DeviceName: attachment.Device, |
axw
Mar 25, 2015
Member
One thing to bear in mind: Juju's DeviceName is the bare device name, not the path. e.g. "sda" and not "/dev/sda". I'm not sure what it is in Nova/Cinder, but EC2/EBS returns the path. What's more, in EC2 you specify, say, "/dev/sda" but that actually gets translated to "/dev/xvda" on the machine that's created (in recent Linux kernels). So there's some translation required for EC2 - not sure what the case is here.
axw
reviewed
Mar 25, 2015
| + continue | ||
| + } | ||
| + | ||
| + attachment, err := s.openstackClient.AttachVolume( |
axw
Mar 25, 2015
Member
Please make sure AttachVolumes and DetachVolumes are idempotent. This is primarily to cover partial failures. In the storage provisioner, we'll issue Attach calls for machine-scoped storage whenever the machine agent starts up; I don't think we'll do this for environ-scoped (shouldn't be necessary), but we do need to cover partial failures (i.e. where an attachment is created, but not recorded in state).
axw
Mar 25, 2015
Member
P.S. I could live with that being done in a follow-up, but it is a must-have.
axw
reviewed
Mar 25, 2015
| + | ||
| + errors := make([]error, len(volIds)) | ||
| + for volIdx, volId := range volIds { | ||
| + if err := s.openstackClient.DeleteVolume(volId); err != nil { |
axw
Mar 25, 2015
Member
Like Attach/Detach, DestroyVolumes needs to be idempotent and not fail if the volume doesn't exist.
|
Thanks, looking pretty good overall. |
axw
reviewed
Mar 31, 2015
| + | ||
| + // In most cases, it is quicker to get all volumes and loop | ||
| + // locally than to make several round-trips to the provider. | ||
| + cinderVols, err := s.openstackClient.GetVolumesSimple() |
axw
Mar 31, 2015
Member
Is there no way to do server-side filtering on volume IDs? If there is (which I can't see looking at the API), please add a TODO to use it.
kat-co
Apr 6, 2015
Contributor
I don't think there is. You can get detailed volume information via /v2/{tenant_id}/volumes/{volume_id}, but I figured 1 batch request would be faster than several API calls.
axw
reviewed
Mar 31, 2015
| + if err := <-s.openstackClient.VolumeStatusNotifier( | ||
| + attachArg.VolumeId, | ||
| + "available", | ||
| + 10, |
axw
reviewed
Mar 31, 2015
| + attachArg.VolumeId, | ||
| + "available", | ||
| + 10, | ||
| + 5*time.Second, |
axw
reviewed
Mar 31, 2015
| + continue | ||
| + } | ||
| + | ||
| + // Check to see if the volume is already attached. |
axw
Mar 31, 2015
Member
I think it'd be better to just call AttachVolume, and then check the error result. Fewer API calls that way. Did you consider this and dismiss the option?
axw
reviewed
Mar 31, 2015
| + continue | ||
| + } | ||
| + | ||
| + err = s.openstackClient.DetachVolume(string(attachArg.InstanceId), attachment.Volume.String()) |
kat-co
Apr 6, 2015
Contributor
Here at least, we need the the actual attachment ID to detach the volume. This is only provided by finding the correct attachment on the server.
axw
Apr 6, 2015
Member
Forgive me if I'm being dense, but I don't see an attachment ID being used in the DetachVolume call; only an instance ID and a volume ID.
axw
reviewed
Mar 31, 2015
| + continue | ||
| + } | ||
| + | ||
| + if err := <-s.openstackClient.VolumeStatusNotifier( |
axw
Mar 31, 2015
Member
Perhaps a comment just above here, noting that volumes must be "available" before they can be attached to a server.
axw
reviewed
Apr 6, 2015
| + | ||
| +// Scope implements storage.Provider. | ||
| +func (s *OpenstackProvider) Scope() storage.Scope { | ||
| + panic("not implemented") |
axw
reviewed
Apr 6, 2015
| + | ||
| +// Dynamic implements storage.Provider. | ||
| +func (p *OpenstackProvider) Dynamic() bool { | ||
| + panic("not implemented") |
axw
Apr 6, 2015
Member
return true
there will be very few static storage providers; only the ones that can't allocate storage separately from machines
axw
reviewed
Apr 7, 2015
| + volAtt = make([]storage.VolumeAttachment, 0, len(args)) | ||
| + | ||
| + for _, a := range args { | ||
| + if a.Provider != CinderProviderType { |
axw
Apr 7, 2015
Member
I don't think this is necessary, it'll never happen. I may have said at an earlier time to do this because StartInstance would be passed params for different providers, but this is dynamic now and will never be passed anything other than Cinder-type params.
axw
reviewed
Apr 7, 2015
| + // clousures instead of one to take advantage of the loop | ||
| + // parameter. | ||
| + defer func(arg storage.VolumeParams, createdVol storage.Volume) { | ||
| + if returnErr == nil || createdVol == (storage.Volume{}) { |
axw
reviewed
Apr 7, 2015
| + if arg.Attachment != nil { | ||
| + volAttachments, err := s.openstackClient.ListVolumeAttachments(string(arg.Attachment.InstanceId)) | ||
| + if err != nil { | ||
| + panic(err) |
axw
Apr 7, 2015
Member
Whoa! {anicking is a bit drastic, and suggests that the caller did something wrong.
Really there's nothing we can do at this point, so panicking isn't helpful. Just log the error and continue. Once harvesting is implemented we'll retry.
kat-co
Apr 7, 2015
Contributor
Yeah I was going to ask what we'd like to do here. Switched to logging.
axw
reviewed
Apr 7, 2015
| @@ -13,4 +13,5 @@ func init() { | ||
| RegisterProvider(providerType, p) | ||
| } | ||
| + RegisterProvider(provider.CinderProviderType, &provider.OpenstackProvider{provider.NewGooseAdapter}) |
axw
reviewed
Apr 7, 2015
| + &provider.OpenstackProvider{provider.NewGooseAdapter}, | ||
| + ) | ||
| + | ||
| + // Inform the storage provider registry about the Openstack provider. |
axw
Apr 7, 2015
Member
Comment is a little weird. It's recording in the registry which storage providers are supported by OpenStack.
axw
reviewed
Apr 7, 2015
| + | ||
| + "github.com/juju/errors" | ||
| + "github.com/juju/names" | ||
| + |
axw
reviewed
Apr 7, 2015
| + "net/url" | ||
| + "time" | ||
| + | ||
| + "github.com/juju/juju/environs/config" |
axw
reviewed
Apr 7, 2015
| + if arg.Attachment != nil { | ||
| + volAttachments, err := s.openstackClient.ListVolumeAttachments(string(arg.Attachment.InstanceId)) | ||
| + if err != nil { | ||
| + logger.Warningf("could not list volumes while cleaning up: %v", err) |
axw
reviewed
Apr 7, 2015
| + | ||
| + "github.com/juju/names" | ||
| + jc "github.com/juju/testing/checkers" | ||
| + |
axw
reviewed
Apr 7, 2015
| + "io/ioutil" | ||
| + "net/http" | ||
| + | ||
| + "github.com/juju/juju/instance" |
|
LGTM with a few more fixes. |
axw
reviewed
Apr 8, 2015
| + ListVolumeAttachments(serverId string) ([]storage.VolumeAttachment, error) | ||
| +} | ||
| + | ||
| +func NewGooseAdapter(environConfig *config.Config) (OpenstackAdapter, error) { |
axw
Apr 8, 2015
Member
Either this should be newOpenstackAdapter, s/newOpenstackAdapter/NewGooseAdapter/g
Not sure this should be exported anyway...
axw
reviewed
Apr 9, 2015
| @@ -48,6 +48,6 @@ gopkg.in/yaml.v1 git 9f9df34309c04878acc86042b16630b0f696e1de 2014-09-24T16:16:0 | ||
| launchpad.net/gnuflag bzr roger.peppe@canonical.com-20140716064605-pk32dnmfust02yab 13 | ||
| launchpad.net/golxc bzr ian.booth@canonical.com-20141121040613-ztm1q0iy9rune3zt 13 | ||
| launchpad.net/gomaasapi bzr ian.booth@canonical.com-20150113032002-n7hj4l5a9j9dzaa0 61 | ||
| -launchpad.net/goose bzr tarmac-20140908075634-5iinsru19k3d8w55 128 | ||
| +gopkg.in/goose.v1 git ff89212517d2f87ea24d0cb451e7c3a03244627e 2015-03-24T13:17:47Z |
axw
reviewed
Apr 9, 2015
| + } | ||
| + }(a, createdVol) | ||
| + | ||
| + if a.Attachment != nil { |
axw
Apr 9, 2015
Member
&& a.Attachment.InstanceId != ""
Because this is a dynamic provider, the instance may not be provisioned yet. This should change in the future (i.e. we should wait until the instance is provisioned before calling CreateVolumes with an attachment), but until then we need to check.
axw
reviewed
Apr 9, 2015
| + | ||
| +func novaToJujuVolumeAttachment(volAttach *nova.VolumeAttachment) storage.VolumeAttachment { | ||
| + return storage.VolumeAttachment{ | ||
| + Volume: names.NewVolumeTag(volAttach.VolumeId), |
axw
reviewed
Apr 9, 2015
| +func novaToJujuVolumeAttachment(volAttach *nova.VolumeAttachment) storage.VolumeAttachment { | ||
| + return storage.VolumeAttachment{ | ||
| + Volume: names.NewVolumeTag(volAttach.VolumeId), | ||
| + Machine: names.NewMachineTag(volAttach.ServerId), |
axw
Apr 9, 2015
Member
likewise.
tags correspond to Juju's internal names for entities ("0", "1", etc.), not whatever OpenStack or AWS says.
axw
reviewed
Apr 9, 2015
| + return storage.VolumeAttachment{ | ||
| + Volume: names.NewVolumeTag(volAttach.VolumeId), | ||
| + Machine: names.NewMachineTag(volAttach.ServerId), | ||
| + DeviceName: volAttach.Device, |
axw
Apr 9, 2015
Member
I thought this was resolved previously, so sorry for not bringing it up before LGTM: OpenStack returns "/dev/sda", but in storage.VolumeAttachment we need to record "sda".
axw
reviewed
Apr 9, 2015
| + return nil | ||
| + } | ||
| + | ||
| + return openstackClient.DetachVolume(instanceId, attachment.Volume.String()) |
axw
Apr 9, 2015
Member
attachment.Volume is the wrong thing to be using here. You should be using the attachment ID from OpenStack, which is not recorded in storage.VolumeAttachment.
I think you should change the adapter so that it only deals with nova structures. I guess it wouldn't be an adapter at all, just an interface that implements the volume-related bits of Nova and Cinder.
kat-co commentedMar 25, 2015
No description provided.