provisioning: Refactor volume plugins.

NewPersistentVolumeTemplate() and Provision() are merged into one call.

pkg/controller/persistentvolume/persistentvolume_framework_test.go

@@ -41,6 +41,7 @@ import (
 	"k8s.io/kubernetes/pkg/conversion"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/types"
+	"k8s.io/kubernetes/pkg/util"
 	"k8s.io/kubernetes/pkg/util/diff"
 	vol "k8s.io/kubernetes/pkg/volume"
 )
@@ -783,6 +784,7 @@ type mockVolumePlugin struct {
 	deleteCallCounter    int
 	recycleCalls         []error
 	recycleCallCounter   int
+	provisionOptions     vol.VolumeOptions
 }
 
 var _ vol.VolumePlugin = &mockVolumePlugin{}
@@ -813,30 +815,49 @@ func (plugin *mockVolumePlugin) NewProvisioner(options vol.VolumeOptions) (vol.P
 	if len(plugin.provisionCalls) > 0 {
 		// mockVolumePlugin directly implements Provisioner interface
 		glog.V(4).Infof("mock plugin NewProvisioner called, returning mock provisioner")
+		plugin.provisionOptions = options
 		return plugin, nil
 	} else {
 		return nil, fmt.Errorf("Mock plugin error: no provisionCalls configured")
 	}
 }
 
-func (plugin *mockVolumePlugin) Provision(*api.PersistentVolume) error {
+func (plugin *mockVolumePlugin) Provision() (*api.PersistentVolume, error) {
-	if len(plugin.provisionCalls) <= plugin.provisionCallCounter {
-		return fmt.Errorf("Mock plugin error: unexpected provisioner call %d", plugin.provisionCallCounter)
-	}
-	ret := plugin.provisionCalls[plugin.provisionCallCounter]
-	plugin.provisionCallCounter++
-	glog.V(4).Infof("mock plugin Provision call nr. %d, returning %v", plugin.provisionCallCounter, ret)
-	return ret
-}
-
-func (plugin *mockVolumePlugin) NewPersistentVolumeTemplate() (*api.PersistentVolume, error) {
 	if len(plugin.provisionCalls) <= plugin.provisionCallCounter {
 		return nil, fmt.Errorf("Mock plugin error: unexpected provisioner call %d", plugin.provisionCallCounter)
 	}
-	ret := plugin.provisionCalls[plugin.provisionCallCounter]
+
+	var pv *api.PersistentVolume
+	err := plugin.provisionCalls[plugin.provisionCallCounter]
+	if err == nil {
+		// Create a fake PV
+		fullpath := fmt.Sprintf("/tmp/hostpath_pv/%s", util.NewUUID())
+
+		pv = &api.PersistentVolume{
+			ObjectMeta: api.ObjectMeta{
+				Name: plugin.provisionOptions.PVName,
+				Annotations: map[string]string{
+					"kubernetes.io/createdby": "hostpath-dynamic-provisioner",
+				},
+			},
+			Spec: api.PersistentVolumeSpec{
+				PersistentVolumeReclaimPolicy: plugin.provisionOptions.PersistentVolumeReclaimPolicy,
+				AccessModes:                   plugin.provisionOptions.AccessModes,
+				Capacity: api.ResourceList{
+					api.ResourceName(api.ResourceStorage): plugin.provisionOptions.Capacity,
+				},
+				PersistentVolumeSource: api.PersistentVolumeSource{
+					HostPath: &api.HostPathVolumeSource{
+						Path: fullpath,
+					},
+				},
+			},
+		}
+	}
+
 	plugin.provisionCallCounter++
-	glog.V(4).Infof("mock plugin NewPersistentVolumeTemplate call nr. %d, returning %v", plugin.provisionCallCounter, ret)
+	glog.V(4).Infof("mock plugin Provision call nr. %d, returning %v: %v", plugin.provisionCallCounter, pv, err)
-	return nil, ret
+	return pv, err
 }
 
 // Deleter interfaces

pkg/volume/aws_ebs/aws_ebs.go

@@ -408,14 +408,35 @@ type awsElasticBlockStoreProvisioner struct {
 
 var _ volume.Provisioner = &awsElasticBlockStoreProvisioner{}
 
-func (c *awsElasticBlockStoreProvisioner) Provision(pv *api.PersistentVolume) error {
+func (c *awsElasticBlockStoreProvisioner) Provision() (*api.PersistentVolume, error) {
 	volumeID, sizeGB, labels, err := c.manager.CreateVolume(c)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	pv.Spec.PersistentVolumeSource.AWSElasticBlockStore.VolumeID = volumeID
+
-	pv.Spec.Capacity = api.ResourceList{
+	pv := &api.PersistentVolume{
-		api.ResourceName(api.ResourceStorage): resource.MustParse(fmt.Sprintf("%dGi", sizeGB)),
+		ObjectMeta: api.ObjectMeta{
+			Name:   c.options.PVName,
+			Labels: map[string]string{},
+			Annotations: map[string]string{
+				"kubernetes.io/createdby": "aws-ebs-dynamic-provisioner",
+			},
+		},
+		Spec: api.PersistentVolumeSpec{
+			PersistentVolumeReclaimPolicy: c.options.PersistentVolumeReclaimPolicy,
+			AccessModes:                   c.options.AccessModes,
+			Capacity: api.ResourceList{
+				api.ResourceName(api.ResourceStorage): resource.MustParse(fmt.Sprintf("%dGi", sizeGB)),
+			},
+			PersistentVolumeSource: api.PersistentVolumeSource{
+				AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{
+					VolumeID:  volumeID,
+					FSType:    "ext4",
+					Partition: 0,
+					ReadOnly:  false,
+				},
+			},
+		},
 	}
 
 	if len(labels) != 0 {
@@ -427,34 +448,5 @@ func (c *awsElasticBlockStoreProvisioner) Provision(pv *api.PersistentVolume) er
 		}
 	}
 
-	return nil
+	return pv, nil
-}
-
-func (c *awsElasticBlockStoreProvisioner) NewPersistentVolumeTemplate() (*api.PersistentVolume, error) {
-	// Provide dummy api.PersistentVolume.Spec, it will be filled in
-	// awsElasticBlockStoreProvisioner.Provision()
-	return &api.PersistentVolume{
-		ObjectMeta: api.ObjectMeta{
-			GenerateName: "pv-aws-",
-			Labels:       map[string]string{},
-			Annotations: map[string]string{
-				"kubernetes.io/createdby": "aws-ebs-dynamic-provisioner",
-			},
-		},
-		Spec: api.PersistentVolumeSpec{
-			PersistentVolumeReclaimPolicy: c.options.PersistentVolumeReclaimPolicy,
-			AccessModes:                   c.options.AccessModes,
-			Capacity: api.ResourceList{
-				api.ResourceName(api.ResourceStorage): c.options.Capacity,
-			},
-			PersistentVolumeSource: api.PersistentVolumeSource{
-				AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{
-					VolumeID:  volume.ProvisionedVolumeName,
-					FSType:    "ext4",
-					Partition: 0,
-					ReadOnly:  false,
-				},
-			},
-		},
-	}, nil
 }

pkg/volume/aws_ebs/aws_ebs_test.go

@@ -220,14 +220,7 @@ func TestPlugin(t *testing.T) {
 		PersistentVolumeReclaimPolicy: api.PersistentVolumeReclaimDelete,
 	}
 	provisioner, err := plug.(*awsElasticBlockStorePlugin).newProvisionerInternal(options, &fakePDManager{})
-	persistentSpec, err := provisioner.NewPersistentVolumeTemplate()
+	persistentSpec, err := provisioner.Provision()
-	if err != nil {
-		t.Errorf("NewPersistentVolumeTemplate() failed: %v", err)
-	}
-
-	// get 2nd Provisioner - persistent volume controller will do the same
-	provisioner, err = plug.(*awsElasticBlockStorePlugin).newProvisionerInternal(options, &fakePDManager{})
-	err = provisioner.Provision(persistentSpec)
 	if err != nil {
 		t.Errorf("Provision() failed: %v", err)
 	}

pkg/volume/cinder/cinder.go

@@ -427,25 +427,16 @@ type cinderVolumeProvisioner struct {
 
 var _ volume.Provisioner = &cinderVolumeProvisioner{}
 
-func (c *cinderVolumeProvisioner) Provision(pv *api.PersistentVolume) error {
+func (c *cinderVolumeProvisioner) Provision() (*api.PersistentVolume, error) {
 	volumeID, sizeGB, err := c.manager.CreateVolume(c)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	pv.Spec.PersistentVolumeSource.Cinder.VolumeID = volumeID
-	pv.Spec.Capacity = api.ResourceList{
-		api.ResourceName(api.ResourceStorage): resource.MustParse(fmt.Sprintf("%dGi", sizeGB)),
-	}
-	return nil
-}
 
-func (c *cinderVolumeProvisioner) NewPersistentVolumeTemplate() (*api.PersistentVolume, error) {
+	pv := &api.PersistentVolume{
-	// Provide dummy api.PersistentVolume.Spec, it will be filled in
-	// cinderVolumeProvisioner.Provision()
-	return &api.PersistentVolume{
 		ObjectMeta: api.ObjectMeta{
-			GenerateName: "pv-cinder-",
+			Name:   c.options.PVName,
-			Labels:       map[string]string{},
+			Labels: map[string]string{},
 			Annotations: map[string]string{
 				"kubernetes.io/createdby": "cinder-dynamic-provisioner",
 			},
@@ -454,16 +445,16 @@ func (c *cinderVolumeProvisioner) NewPersistentVolumeTemplate() (*api.Persistent
 			PersistentVolumeReclaimPolicy: c.options.PersistentVolumeReclaimPolicy,
 			AccessModes:                   c.options.AccessModes,
 			Capacity: api.ResourceList{
-				api.ResourceName(api.ResourceStorage): c.options.Capacity,
+				api.ResourceName(api.ResourceStorage): resource.MustParse(fmt.Sprintf("%dGi", sizeGB)),
 			},
 			PersistentVolumeSource: api.PersistentVolumeSource{
 				Cinder: &api.CinderVolumeSource{
-					VolumeID: volume.ProvisionedVolumeName,
+					VolumeID: volumeID,
 					FSType:   "ext4",
 					ReadOnly: false,
 				},
 			},
 		},
-	}, nil
+	}
-
+	return pv, nil
 }

pkg/volume/cinder/cinder_test.go

@@ -212,14 +212,7 @@ func TestPlugin(t *testing.T) {
 		PersistentVolumeReclaimPolicy: api.PersistentVolumeReclaimDelete,
 	}
 	provisioner, err := plug.(*cinderPlugin).newProvisionerInternal(options, &fakePDManager{0})
-	persistentSpec, err := provisioner.NewPersistentVolumeTemplate()
+	persistentSpec, err := provisioner.Provision()
-	if err != nil {
-		t.Errorf("NewPersistentVolumeTemplate() failed: %v", err)
-	}
-
-	// get 2nd Provisioner - persistent volume controller will do the same
-	provisioner, err = plug.(*cinderPlugin).newProvisionerInternal(options, &fakePDManager{0})
-	err = provisioner.Provision(persistentSpec)
 	if err != nil {
 		t.Errorf("Provision() failed: %v", err)
 	}

pkg/volume/gce_pd/gce_pd.go

@@ -370,14 +370,35 @@ type gcePersistentDiskProvisioner struct {
 
 var _ volume.Provisioner = &gcePersistentDiskProvisioner{}
 
-func (c *gcePersistentDiskProvisioner) Provision(pv *api.PersistentVolume) error {
+func (c *gcePersistentDiskProvisioner) Provision() (*api.PersistentVolume, error) {
 	volumeID, sizeGB, labels, err := c.manager.CreateVolume(c)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	pv.Spec.PersistentVolumeSource.GCEPersistentDisk.PDName = volumeID
+
-	pv.Spec.Capacity = api.ResourceList{
+	pv := &api.PersistentVolume{
-		api.ResourceName(api.ResourceStorage): resource.MustParse(fmt.Sprintf("%dGi", sizeGB)),
+		ObjectMeta: api.ObjectMeta{
+			Name:   c.options.PVName,
+			Labels: map[string]string{},
+			Annotations: map[string]string{
+				"kubernetes.io/createdby": "gce-pd-dynamic-provisioner",
+			},
+		},
+		Spec: api.PersistentVolumeSpec{
+			PersistentVolumeReclaimPolicy: c.options.PersistentVolumeReclaimPolicy,
+			AccessModes:                   c.options.AccessModes,
+			Capacity: api.ResourceList{
+				api.ResourceName(api.ResourceStorage): resource.MustParse(fmt.Sprintf("%dGi", sizeGB)),
+			},
+			PersistentVolumeSource: api.PersistentVolumeSource{
+				GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{
+					PDName:    volumeID,
+					FSType:    "ext4",
+					Partition: 0,
+					ReadOnly:  false,
+				},
+			},
+		},
 	}
 
 	if len(labels) != 0 {
@@ -389,34 +410,5 @@ func (c *gcePersistentDiskProvisioner) Provision(pv *api.PersistentVolume) error
 		}
 	}
 
-	return nil
+	return pv, nil
-}
-
-func (c *gcePersistentDiskProvisioner) NewPersistentVolumeTemplate() (*api.PersistentVolume, error) {
-	// Provide dummy api.PersistentVolume.Spec, it will be filled in
-	// gcePersistentDiskProvisioner.Provision()
-	return &api.PersistentVolume{
-		ObjectMeta: api.ObjectMeta{
-			GenerateName: "pv-gce-",
-			Labels:       map[string]string{},
-			Annotations: map[string]string{
-				"kubernetes.io/createdby": "gce-pd-dynamic-provisioner",
-			},
-		},
-		Spec: api.PersistentVolumeSpec{
-			PersistentVolumeReclaimPolicy: c.options.PersistentVolumeReclaimPolicy,
-			AccessModes:                   c.options.AccessModes,
-			Capacity: api.ResourceList{
-				api.ResourceName(api.ResourceStorage): c.options.Capacity,
-			},
-			PersistentVolumeSource: api.PersistentVolumeSource{
-				GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{
-					PDName:    volume.ProvisionedVolumeName,
-					FSType:    "ext4",
-					Partition: 0,
-					ReadOnly:  false,
-				},
-			},
-		},
-	}, nil
 }

pkg/volume/gce_pd/gce_pd_test.go

@@ -216,14 +216,7 @@ func TestPlugin(t *testing.T) {
 		PersistentVolumeReclaimPolicy: api.PersistentVolumeReclaimDelete,
 	}
 	provisioner, err := plug.(*gcePersistentDiskPlugin).newProvisionerInternal(options, &fakePDManager{})
-	persistentSpec, err := provisioner.NewPersistentVolumeTemplate()
+	persistentSpec, err := provisioner.Provision()
-	if err != nil {
-		t.Errorf("NewPersistentVolumeTemplate() failed: %v", err)
-	}
-
-	// get 2nd Provisioner - persistent volume controller will do the same
-	provisioner, err = plug.(*gcePersistentDiskPlugin).newProvisionerInternal(options, &fakePDManager{})
-	err = provisioner.Provision(persistentSpec)
 	if err != nil {
 		t.Errorf("Provision() failed: %v", err)
 	}

pkg/volume/host_path/host_path.go

@@ -252,18 +252,12 @@ type hostPathProvisioner struct {
 
 // Create for hostPath simply creates a local /tmp/hostpath_pv/%s directory as a new PersistentVolume.
 // This Provisioner is meant for development and testing only and WILL NOT WORK in a multi-node cluster.
-func (r *hostPathProvisioner) Provision(pv *api.PersistentVolume) error {
+func (r *hostPathProvisioner) Provision() (*api.PersistentVolume, error) {
-	if pv.Spec.HostPath == nil {
-		return fmt.Errorf("pv.Spec.HostPath cannot be nil")
-	}
-	return os.MkdirAll(pv.Spec.HostPath.Path, 0750)
-}
-
-func (r *hostPathProvisioner) NewPersistentVolumeTemplate() (*api.PersistentVolume, error) {
 	fullpath := fmt.Sprintf("/tmp/hostpath_pv/%s", util.NewUUID())
-	return &api.PersistentVolume{
+
+	pv := &api.PersistentVolume{
 		ObjectMeta: api.ObjectMeta{
-			GenerateName: "pv-hostpath-",
+			Name: r.options.PVName,
 			Annotations: map[string]string{
 				"kubernetes.io/createdby": "hostpath-dynamic-provisioner",
 			},
@@ -280,7 +274,9 @@ func (r *hostPathProvisioner) NewPersistentVolumeTemplate() (*api.PersistentVolu
 				},
 			},
 		},
-	}, nil
+	}
+
+	return pv, os.MkdirAll(pv.Spec.HostPath.Path, 0750)
 }
 
 // hostPathDeleter deletes a hostPath PV from the cluster.

pkg/volume/host_path/host_path_test.go

@@ -163,7 +163,7 @@ func TestProvisioner(t *testing.T) {
 	if err != nil {
 		t.Errorf("Failed to make a new Provisioner: %v", err)
 	}
-	pv, err := creater.NewPersistentVolumeTemplate()
+	pv, err := creater.Provision()
 	if err != nil {
 		t.Errorf("Unexpected error creating volume: %v", err)
 	}

pkg/volume/testing/testing.go

@@ -340,11 +340,12 @@ type FakeProvisioner struct {
 	Host    VolumeHost
 }
 
-func (fc *FakeProvisioner) NewPersistentVolumeTemplate() (*api.PersistentVolume, error) {
+func (fc *FakeProvisioner) Provision() (*api.PersistentVolume, error) {
 	fullpath := fmt.Sprintf("/tmp/hostpath_pv/%s", util.NewUUID())
-	return &api.PersistentVolume{
+
+	pv := &api.PersistentVolume{
 		ObjectMeta: api.ObjectMeta{
-			GenerateName: "pv-fakeplugin-",
+			Name: fc.Options.PVName,
 			Annotations: map[string]string{
 				"kubernetes.io/createdby": "fakeplugin-provisioner",
 			},
@@ -361,11 +362,9 @@ func (fc *FakeProvisioner) NewPersistentVolumeTemplate() (*api.PersistentVolume,
 				},
 			},
 		},
-	}, nil
+	}
-}
 
-func (fc *FakeProvisioner) Provision(pv *api.PersistentVolume) error {
+	return pv, nil
-	return nil
 }
 
 // FindEmptyDirectoryUsageOnTmpfs finds the expected usage of an empty directory existing on

pkg/volume/volume.go

@@ -111,13 +111,10 @@ type Recycler interface {
 // Provisioner is an interface that creates templates for PersistentVolumes and can create the volume
 // as a new resource in the infrastructure provider.
 type Provisioner interface {
-	// Provision creates the resource by allocating the underlying volume in a storage system.
+	// Provision creates the resource by allocating the underlying volume in a
-	// This method should block until completion.
+	// storage system. This method should block until completion and returns
-	Provision(*api.PersistentVolume) error
+	// PersistentVolume representing the created storage resource.
-	// NewPersistentVolumeTemplate creates a new PersistentVolume to be used as a template before saving.
+	Provision() (*api.PersistentVolume, error)
-	// The provisioner will want to tweak its properties, assign correct annotations, etc.
-	// This func should *NOT* persist the PV in the API.  That is left to the caller.
-	NewPersistentVolumeTemplate() (*api.PersistentVolume, error)
 }
 
 // Deleter removes the resource from the underlying storage provider.  Calls to this method should block until