Use type parameter to dispatch custom logic #135

harveylowndes · 2018-06-12T14:43:27Z

Resolves #131

Changelog


- Use the 'type' parameter in a storage class to dispatch custom logic. By default the type is 'blockStorage'. If the type is 'blockStorage' then the 'fsType' parameter can also be set. By default this is ext4 but there is also support for ext3. The follow will create a block storage with file system type ext3:

kind: StorageClass
apiVersion: storage.k8s.io/v1beta1
metadata:
  name: oci
provisioner: oracle.com/oci
parameters:
  type: blockStorage
  fsType: ext3

prydie · 2018-06-12T16:53:05Z

pkg/provisioner/core/provisioner.go

+
+	fsType := getFsTypeWithDefault(storageClass.Parameters["fsType"])
+
+	provisioner, ok := p.storageClassProvisioners[fsType]


As @garthy highlighted offline this breaks down when the following happens:

You Provision a volume with a given fsType

The volume provisioner is restarted

Delete is called before a Provision with the same fsType

prydie · 2018-06-12T16:54:54Z

pkg/provisioner/core/provisioner.go

+	return getFsTypeWithDefault(options.Parameters["fsType"])
+}
+
+func getFsTypeWithDefault(fsType string) string {


I would fold this into resolveFSType() and add a const along the lines of defaultFSType rather than hardcoding the constant here.

prydie · 2018-06-12T16:55:12Z

pkg/provisioner/block/block_test.go

@@ -48,7 +48,9 @@ func TestResolveFSTypeWhenNotConfigured(t *testing.T) {

 func TestResolveFSTypeWhenConfigured(t *testing.T) {
 	// test default fsType of 'ext3' is always returned when configured.
-	options := controller.VolumeOptions{Parameters: map[string]string{fsType: "ext3"}}


This is preferable IMO

Yeah it was breaking for some reason on vsCode but worked out why and I will change back.

prydie · 2018-06-12T16:57:18Z

pkg/provisioner/core/provisioner.go

@@ -109,16 +109,29 @@ func mapAvailabilityDomainToFailureDomain(AD string) string {
 	return parts[len(parts)-1]
 }

+func resolveFSType(options controller.VolumeOptions) string {


Does it make sense to deduplicate this (pkg/provisioner/block/block.go)?

prydie · 2018-06-12T16:58:06Z

pkg/provisioner/core/provisioner.go

-	provisioner, ok := p.storageClassProvisioners[volume.Spec.StorageClassName]
+	storageClass, err := p.kubeClient.Storage().StorageClasses().Get(volume.Spec.StorageClassName, metav1.GetOptions{})
+	if err != nil {
+		return fmt.Errorf("Could not find storage class with the associated name '%s'", volume.Spec.StorageClassName)


return fmt.Errorf("could not find storage class %q", volume.Spec.StorageClassName)

prydie · 2018-06-13T08:09:43Z

pkg/provisioner/core/provisioner.go

 // Provision creates a storage asset and returns a PV object representing it.
 func (p *OCIProvisioner) Provision(options controller.VolumeOptions) (*v1.PersistentVolume, error) {
 	availabilityDomainName, availabilityDomain, err := p.chooseAvailabilityDomain(options.PVC)
 	if err != nil {
 		return nil, err
 	}

-	provisioner, ok := p.storageClassProvisioners[*options.PVC.Spec.StorageClassName]
+	fsType := resolveFSType(options)
+	provisioner, ok := p.storageClassProvisioners[fsType]


I'm still not overly happy with the overloading of fsType here. For one thing, it means creating multiple copies of the block storage provisioner in p.storageClassProvisioners unnecessarily. More importantly, it's not really what I'd expect as a user as throughout Kubernetes FSType is used to denote the filesystem type rather than the storage backend:

// Filesystem type to mount. // Must be a filesystem type supported by the host operating system. // Ex. "ext4", "xfs", "ntfs". The default filesystem depends on FlexVolume script. // +optional FSType string

Source

I suggest we do something akin to what hyperkube does with symlinks in the flexvolume driver DaemonSet and then dispatch on provisioner in the StorageClass (i.e. oracle/oci-block-storage / oracle/oci-filesystem-storage).

owainlewis · 2018-06-13T08:16:36Z

pkg/provisioner/core/provisioner.go

@@ -87,8 +87,8 @@ func NewOCIProvisioner(kubeClient kubernetes.Interface, nodeInformer informersv1
 		nodeLister:       nodeInformer.Lister(),
 		nodeListerSynced: nodeInformer.Informer().HasSynced,
 		storageClassProvisioners: map[string]plugin.ProvisionerPlugin{
-			"oci":      blockProvisioner,
-			"oci-ext3": blockProvisioner,


Dispatching here should be done on some additional parameter e.g. type: block, type: nfs. This allows us to support FSS and Block. The block provisioner gets the volume options passed in so it can lookup the fsType directly without it being passed in.

We discussed this as an option yesterday. Personally I prefer the symlink approach but this to overloading fsType also.

* Add OCI proxy support

harveylowndes requested review from prydie and garthy June 12, 2018 14:43

owainlewis added the enhancement label Jun 12, 2018

harveylowndes changed the title ~~Storage class fsType parameter used to dispatch custom logic~~ Use storage class fsType parameter to dispatch custom logic Jun 12, 2018

prydie suggested changes Jun 13, 2018

View reviewed changes

owainlewis reviewed Jun 13, 2018

View reviewed changes

harveylowndes force-pushed the use-storageclass-fstype-to-dispatch-custom-logic branch from cf364ff to c392a06 Compare June 13, 2018 09:25

harveylowndes changed the title ~~Use storage class fsType parameter to dispatch custom logic~~ Use type parameter to dispatch custom logic Jun 13, 2018

harveylowndes force-pushed the use-storageclass-fstype-to-dispatch-custom-logic branch from c392a06 to 1fa59fa Compare June 13, 2018 09:35

Storage type used to dispatch custom logic

18ecaa5

harveylowndes force-pushed the use-storageclass-fstype-to-dispatch-custom-logic branch from 1fa59fa to 18ecaa5 Compare June 13, 2018 12:44

prydie closed this Jun 13, 2018

prydie deleted the use-storageclass-fstype-to-dispatch-custom-logic branch September 17, 2018 13:48

rjtsdl pushed a commit to rjtsdl/oci-volume-provisioner that referenced this pull request Dec 20, 2018

Add OCI proxy support (oracle#135)

c71e92f

* Add OCI proxy support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use type parameter to dispatch custom logic #135

Use type parameter to dispatch custom logic #135

harveylowndes commented Jun 12, 2018 •

edited

Loading

prydie Jun 12, 2018

prydie Jun 12, 2018

prydie Jun 12, 2018

harveylowndes Jun 13, 2018

prydie Jun 12, 2018

prydie Jun 12, 2018

prydie Jun 13, 2018

owainlewis Jun 13, 2018

prydie Jun 13, 2018


		fsType := getFsTypeWithDefault(storageClass.Parameters["fsType"])

		provisioner, ok := p.storageClassProvisioners[fsType]

Use type parameter to dispatch custom logic #135

Use type parameter to dispatch custom logic #135

Conversation

harveylowndes commented Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harveylowndes commented Jun 12, 2018 •

edited

Loading