Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add PV.Name into names of generated GCE/AWS/OpenStack volumes. #20900

Merged
merged 1 commit into from
Feb 14, 2016
Merged

Add PV.Name into names of generated GCE/AWS/OpenStack volumes. #20900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/kube-controller-manager/app/controllermanager.go
View file
Open in desktop
Expand Up @@ -312,7 +312,7 @@ func StartControllers(s *options.CMServer, kubeClient *client.Client, kubeconfig
pvRecycler.Run()

if provisioner != nil {
pvController, err := persistentvolumecontroller.NewPersistentVolumeProvisionerController(persistentvolumecontroller.NewControllerClient(clientset.NewForConfigOrDie(client.AddUserAgent(kubeconfig, "persistent-volume-provisioner"))), s.PVClaimBinderSyncPeriod.Duration, volumePlugins, provisioner, cloud)
pvController, err := persistentvolumecontroller.NewPersistentVolumeProvisionerController(persistentvolumecontroller.NewControllerClient(clientset.NewForConfigOrDie(client.AddUserAgent(kubeconfig, "persistent-volume-provisioner"))), s.PVClaimBinderSyncPeriod.Duration, s.ClusterName, volumePlugins, provisioner, cloud)
if err != nil {
glog.Fatalf("Failed to start persistent volume provisioner controller: %+v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion contrib/mesos/pkg/controllermanager/controllermanager.go
View file
Open in desktop
Expand Up @@ -268,7 +268,7 @@ func (s *CMServer) Run(_ []string) error {
pvRecycler.Run()

if provisioner != nil {
pvController, err := persistentvolumecontroller.NewPersistentVolumeProvisionerController(persistentvolumecontroller.NewControllerClient(clientset.NewForConfigOrDie(client.AddUserAgent(kubeconfig, "persistent-volume-controller"))), s.PVClaimBinderSyncPeriod.Duration, volumePlugins, provisioner, cloud)
pvController, err := persistentvolumecontroller.NewPersistentVolumeProvisionerController(persistentvolumecontroller.NewControllerClient(clientset.NewForConfigOrDie(client.AddUserAgent(kubeconfig, "persistent-volume-controller"))), s.PVClaimBinderSyncPeriod.Duration, s.ClusterName, volumePlugins, provisioner, cloud)
if err != nil {
glog.Fatalf("Failed to start persistent volume provisioner controller: %+v", err)
}
Expand Down
7 changes: 5 additions & 2 deletions pkg/cloudprovider/providers/openstack/openstack.go
View file
Open in desktop
Expand Up @@ -1038,7 +1038,7 @@ func (os *OpenStack) getVolume(diskName string) (volumes.Volume, error) {
}

// Create a volume of given size (in GiB)
func (os *OpenStack) CreateVolume(size int, tags *map[string]string) (volumeName string, err error) {
func (os *OpenStack) CreateVolume(name string, size int, tags *map[string]string) (volumeName string, err error) {

sClient, err := openstack.NewBlockStorageV1(os.provider, gophercloud.EndpointOpts{
Region: os.region,
Expand All @@ -1049,7 +1049,10 @@ func (os *OpenStack) CreateVolume(size int, tags *map[string]string) (volumeName
return "", err
}

opts := volumes.CreateOpts{Size: size}
opts := volumes.CreateOpts{
Name: name,
Size: size,
}
if tags != nil {
opts.Metadata = *tags
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/cloudprovider/providers/openstack/openstack_test.go
View file
Open in desktop
Expand Up @@ -22,6 +22,8 @@ import (
"testing"
"time"

"k8s.io/kubernetes/pkg/util/rand"

"github.com/rackspace/gophercloud"
)

Expand Down Expand Up @@ -213,7 +215,7 @@ func TestVolumes(t *testing.T) {
tags := map[string]string{
"test": "value",
}
vol, err := os.CreateVolume(1, &tags)
vol, err := os.CreateVolume("kubernetes-test-volume-"+rand.String(10), 1, &tags)
if err != nil {
t.Fatalf("Cannot create a new Cinder volume: %v", err)
}
Expand Down
15 changes: 11 additions & 4 deletions pkg/controller/persistentvolume/persistentvolume_provisioner_controller.go
View file
Open in desktop
Expand Up @@ -49,6 +49,7 @@ type PersistentVolumeProvisionerController struct {
pluginMgr volume.VolumePluginMgr
stopChannels map[string]chan struct{}
mutex sync.RWMutex
clusterName string
}

// constant name values for the controllers stopChannels map.
Expand All @@ -57,11 +58,12 @@ const volumesStopChannel = "volumes"
const claimsStopChannel = "claims"

// NewPersistentVolumeProvisionerController creates a new PersistentVolumeProvisionerController
func NewPersistentVolumeProvisionerController(client controllerClient, syncPeriod time.Duration, plugins []volume.VolumePlugin, provisioner volume.ProvisionableVolumePlugin, cloud cloudprovider.Interface) (*PersistentVolumeProvisionerController, error) {
func NewPersistentVolumeProvisionerController(client controllerClient, syncPeriod time.Duration, clusterName string, plugins []volume.VolumePlugin, provisioner volume.ProvisionableVolumePlugin, cloud cloudprovider.Interface) (*PersistentVolumeProvisionerController, error) {
controller := &PersistentVolumeProvisionerController{
client: client,
cloud: cloud,
provisioner: provisioner,
clusterName: clusterName,
}

if err := controller.pluginMgr.InitPlugins(plugins, controller); err != nil {
Expand Down Expand Up @@ -172,7 +174,7 @@ func (controller *PersistentVolumeProvisionerController) reconcileClaim(claim *a
}

glog.V(5).Infof("PersistentVolumeClaim[%s] provisioning", claim.Name)
provisioner, err := newProvisioner(controller.provisioner, claim, nil)
provisioner, err := controller.newProvisioner(controller.provisioner, claim, nil)
if err != nil {
return fmt.Errorf("Unexpected error getting new provisioner for claim %s: %v\n", claim.Name, err)
}
Expand Down Expand Up @@ -274,7 +276,7 @@ func provisionVolume(pv *api.PersistentVolume, controller *PersistentVolumeProvi
}
claim := obj.(*api.PersistentVolumeClaim)

provisioner, _ := newProvisioner(controller.provisioner, claim, pv)
provisioner, _ := controller.newProvisioner(controller.provisioner, claim, pv)
err := provisioner.Provision(pv)
if err != nil {
glog.Errorf("Could not provision %s", pv.Name)
Expand Down Expand Up @@ -330,7 +332,7 @@ func (controller *PersistentVolumeProvisionerController) Stop() {
}
}

func newProvisioner(plugin volume.ProvisionableVolumePlugin, claim *api.PersistentVolumeClaim, pv *api.PersistentVolume) (volume.Provisioner, error) {
func (controller *PersistentVolumeProvisionerController) newProvisioner(plugin volume.ProvisionableVolumePlugin, claim *api.PersistentVolumeClaim, pv *api.PersistentVolume) (volume.Provisioner, error) {
tags := make(map[string]string)
tags[cloudVolumeCreatedForClaimNamespaceTag] = claim.Namespace
tags[cloudVolumeCreatedForClaimNameTag] = claim.Name
Expand All @@ -345,6 +347,11 @@ func newProvisioner(plugin volume.ProvisionableVolumePlugin, claim *api.Persiste
AccessModes: claim.Spec.AccessModes,
PersistentVolumeReclaimPolicy: api.PersistentVolumeReclaimDelete,
CloudTags: &tags,
ClusterName: controller.clusterName,
}

if pv != nil {
volumeOptions.PVName = pv.Name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is pv.Name set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Provisioners of individual volume plugins, currently as PV.GenerateName = "pv-cinder-". (pv-aws-, pv-gce-)

E.g. https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/cinder/cinder.go#L434

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pv-name is guaranteed to be unique in Kubernetes cluster

I don't understand how the name is guaranteed to be unique then. For example, won't this make the volume name someCluster-dynamic-pv-cinder-?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment would help.

It is guaranteed unique because we first persist the PV with, for example, GenerateName: "pv-aws-", objectMeta.GenerateName

We then want to use that PV name as the resource name to tie them together.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks Mark! Yes, a comment would help.

So one more question: why set the final name in such a roundabout fashion? i.e. NewPersistentVolumeTemplate() will set it to pv-aws-UID then CreateVolume(...) will set it to clusterName-dynamic-pv-aws-UID. Why not just set it to the final name directly in NewPersistentVolumeTemplate()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of name is guaranteed to be unique in the cluster besides a new UUID? objMeta.GenerateName allows a human-readable prefix with a suffix that makes a unique name.

Also, once the PV is persisted, we get more than a name. We get a marker (with a future Provisioning Condition) that makes the provisioning process async. We use annotations today but that is better expressed as a Condition (can put meaningful status on the CLI).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, so we want/expect the name of the PV in the provider to be different then (contain more info then) the name of the API PV object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a note describing where pv.Name comes from.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saad-ali strictly speaking, we are assigning PV.Name from Kube to GCEPD.Name in the provider. The name field exists on both, so we think it would be beneficial for them to match.

}

provisioner, err := plugin.NewProvisioner(volumeOptions)
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/persistentvolume/persistentvolume_provisioner_controller_test.go
View file
Open in desktop
Expand Up @@ -95,7 +95,7 @@ func makeTestClaim() *api.PersistentVolumeClaim {
func makeTestController() (*PersistentVolumeProvisionerController, *mockControllerClient, *volume.FakeVolumePlugin) {
mockClient := &mockControllerClient{}
mockVolumePlugin := &volume.FakeVolumePlugin{}
controller, _ := NewPersistentVolumeProvisionerController(mockClient, 1*time.Second, nil, mockVolumePlugin, &fake_cloud.FakeCloud{})
controller, _ := NewPersistentVolumeProvisionerController(mockClient, 1*time.Second, "fake-kubernetes", nil, mockVolumePlugin, &fake_cloud.FakeCloud{})
return controller, mockClient, mockVolumePlugin
}

Expand Down
20 changes: 15 additions & 5 deletions pkg/volume/aws_ebs/aws_util.go
View file
Open in desktop
Expand Up @@ -132,12 +132,22 @@ func (util *AWSDiskUtil) CreateVolume(c *awsElasticBlockStoreProvisioner) (volum
return "", 0, err
}

requestBytes := c.options.Capacity.Value()
// The cloud provider works with gigabytes, convert to GiB with rounding up
requestGB := volume.RoundUpSize(requestBytes, 1024*1024*1024)
// AWS volumes don't have Name field, store the name in Name tag
var tags map[string]string
if c.options.CloudTags == nil {
tags = make(map[string]string)
} else {
tags = *c.options.CloudTags
}
tags["Name"] = volume.GenerateVolumeName(c.options.ClusterName, c.options.PVName, 255) // AWS tags can have 255 characters

volumeOptions := &aws.VolumeOptions{}
volumeOptions.CapacityGB = int(requestGB)
requestBytes := c.options.Capacity.Value()
// AWS works with gigabytes, convert to GiB with rounding up
requestGB := int(volume.RoundUpSize(requestBytes, 1024*1024*1024))
volumeOptions := &aws.VolumeOptions{
CapacityGB: requestGB,
Tags: &tags,
}

name, err := cloud.CreateDisk(volumeOptions)
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion pkg/volume/cinder/cinder_util.go
View file
Open in desktop
Expand Up @@ -150,7 +150,8 @@ func (util *CinderDiskUtil) CreateVolume(c *cinderVolumeProvisioner) (volumeID s
volSizeBytes := c.options.Capacity.Value()
// Cinder works with gigabytes, convert to GiB with rounding up
volSizeGB := int(volume.RoundUpSize(volSizeBytes, 1024*1024*1024))
name, err := cloud.CreateVolume(volSizeGB, c.options.CloudTags)
name := volume.GenerateVolumeName(c.options.ClusterName, c.options.PVName, 255) // Cinder volume name can have up to 255 characters
name, err = cloud.CreateVolume(name, volSizeGB, c.options.CloudTags)
if err != nil {
glog.V(2).Infof("Error creating cinder volume: %v", err)
return "", 0, err
Expand Down
3 changes: 1 addition & 2 deletions pkg/volume/gce_pd/gce_util.go
View file
Open in desktop
Expand Up @@ -27,7 +27,6 @@ import (
"github.com/golang/glog"
"k8s.io/kubernetes/pkg/cloudprovider"
gcecloud "k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
"k8s.io/kubernetes/pkg/util"
"k8s.io/kubernetes/pkg/util/exec"
"k8s.io/kubernetes/pkg/util/keymutex"
"k8s.io/kubernetes/pkg/util/runtime"
Expand Down Expand Up @@ -134,7 +133,7 @@ func (gceutil *GCEDiskUtil) CreateVolume(c *gcePersistentDiskProvisioner) (volum
return "", 0, err
}

name := fmt.Sprintf("kube-dynamic-%s", util.NewUUID())
name := volume.GenerateVolumeName(c.options.ClusterName, c.options.PVName, 63) // GCE PD name can have up to 63 characters
requestBytes := c.options.Capacity.Value()
// GCE works with gigabytes, convert to GiB with rounding up
requestGB := volume.RoundUpSize(requestBytes, 1024*1024*1024)
Expand Down
4 changes: 4 additions & 0 deletions pkg/volume/plugins.go
View file
Open in desktop
Expand Up @@ -49,6 +49,10 @@ type VolumeOptions struct {
AccessModes []api.PersistentVolumeAccessMode
// Reclamation policy for a persistent volume
PersistentVolumeReclaimPolicy api.PersistentVolumeReclaimPolicy
// PV.Name of the appropriate PersistentVolume. Used to generate cloud volume name.
PVName string
// Unique name of Kubernetes cluster.
ClusterName string
// Tags to attach to the real volume in the cloud provider - e.g. AWS EBS
CloudTags *map[string]string
}
Expand Down
17 changes: 17 additions & 0 deletions pkg/volume/util.go
View file
Open in desktop
Expand Up @@ -148,3 +148,20 @@ func CalculateTimeoutForVolume(minimumTimeout, timeoutIncrement int, pv *api.Per
func RoundUpSize(volumeSizeBytes int64, allocationUnitBytes int64) int64 {
return (volumeSizeBytes + allocationUnitBytes - 1) / allocationUnitBytes
}

// GenerateVolumeName returns a PV name with clusterName prefix.
// The function should be used to generate a name of GCE PD or Cinder volume.
// It basically adds "<clusterName>-dynamic-" before the PV name,
// making sure the resulting string fits given length and cuts "dynamic"
// if not.
func GenerateVolumeName(clusterName, pvName string, maxLength int) string {
prefix := clusterName + "-dynamic"
pvLen := len(pvName)

// cut the "<clusterName>-dynamic" to fit full pvName into maxLength
// +1 for the '-' dash
if pvLen+1+len(prefix) > maxLength {
prefix = prefix[:maxLength-pvLen-1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if pvLen + 1 > maxLength?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Currently, it cannot happen - GCE has limit of 63 characters and pv.Name there has 12 characters. All the other clouds have 255 characters limit.

Of course, it's not robust, but cutting pvName (guaranteed to be unique) may generate non-unique volume names. And error is not an option. Suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-ideal, but it should be ok, because it shouldn't happen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe add a comment explaining this, in case someone encounters it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added, I hope it's not too confusing.

}
return prefix + "-" + pvName
}
25 changes: 25 additions & 0 deletions pkg/volume/util_test.go
View file
Open in desktop
Expand Up @@ -128,3 +128,28 @@ func TestCalculateTimeoutForVolume(t *testing.T) {
t.Errorf("Expected 4500 for timeout but got %v", timeout)
}
}

func TestGenerateVolumeName(t *testing.T) {

// Normal operation, no truncate
v1 := GenerateVolumeName("kubernetes", "pv-cinder-abcde", 255)
if v1 != "kubernetes-dynamic-pv-cinder-abcde" {
t.Errorf("Expected kubernetes-dynamic-pv-cinder-abcde, got %s", v1)
}

// Truncate trailing "6789-dynamic"
prefix := strings.Repeat("0123456789", 9) // 90 characters prefix + 8 chars. of "-dynamic"
v2 := GenerateVolumeName(prefix, "pv-cinder-abcde", 100)
expect := prefix[:84] + "-pv-cinder-abcde"
if v2 != expect {
t.Errorf("Expected %s, got %s", expect, v2)
}

// Truncate really long cluster name
prefix = strings.Repeat("0123456789", 1000) // 10000 characters prefix
v3 := GenerateVolumeName(prefix, "pv-cinder-abcde", 100)
if v3 != expect {
t.Errorf("Expected %s, got %s", expect, v3)
}

}