storage-operator: use corev1.PersistentVolumeMode instead of a boolean

This makes Volume closer to the underlying PersistentVolume (which simplify the code and also helps the user already familiar with PersistentVolume). Refs: #2421 Signed-off-by: Sylvain Laperche <[email protected]>
scality · Jul 6, 2020 · 57bc9fb · 57bc9fb
1 parent 07dfa18
commit 57bc9fb
Show file tree

Hide file tree

Showing 8 changed files with 49 additions and 48 deletions.
diff --git a/docs/operation/volume_management/volume_creation_deletion_cli.rst b/docs/operation/volume_management/volume_creation_deletion_cli.rst
@@ -24,18 +24,18 @@ This section describes how to create a **Volume** from the **CLI**.
        spec:
          nodeName: <node_name>
          storageClassName: <storageclass_name>
+         mode: "Filesystem"
          rawBlockDevice:
            devicePath: <device_path>
-           noFormat: false
 
    Set the following fields:
 
    - **name**: the name of your volume, must be unique
    - **nodeName**: the name of the node where the volume will be located.
    - **storageClassName**: the **StorageClass** to use
+   - **mode**: describes how the volume is intended to be consumed, either
+     Block or Filesystem (default to Filesystem if not specified).
    - **devicePath**: path to the block device (for example, `/dev/sda1`).
-   - **noFormat**: skip the volume formatting if **true**, optional (default to
-     **false**, i.e. apply a formatting according to the **StorageClass**).
 
 #. Create the **Volume**
 

diff --git a/salt/_modules/metalk8s_volumes.py b/salt/_modules/metalk8s_volumes.py
@@ -382,7 +382,7 @@ def clean_up(self):
             log.warning('{} already removed'.format(exn.filename))
 
 
-class SparseLoopDeviceNoFormat(SparseLoopDevice):
+class SparseLoopDeviceBlock(SparseLoopDevice):
     # Recent losetup support `--nooverlap` but not the one shipped with
     # CentOS 7.
     PROVISIONING_COMMAND = ('losetup', '--find', '--partscan')
@@ -400,7 +400,7 @@ def is_prepared(self):
         return re.search(pattern, result['stdout']) is not None
 
     def prepare(self, force=False):
-        prepare_noformat(self.path, self.get('metadata.uid'))
+        prepare_block(self.path, self.get('metadata.uid'))
 
 
 # }}}
@@ -441,9 +441,9 @@ def is_cleaned_up(self):
     def clean_up(self):
         return  # Nothing to do
 
-class RawBlockDeviceNoFormat(RawBlockDevice):
+class RawBlockDeviceBlock(RawBlockDevice):
     def __init__(self, volume):
-        super(RawBlockDeviceNoFormat, self).__init__(volume)
+        super(RawBlockDeviceBlock, self).__init__(volume)
         # Detect which kind of device we have: a real disk, only a partition or
         # an LVM volume.
         name = device_name(self.path)
@@ -499,7 +499,7 @@ def prepare(self, force=False):
             ]))
         # Otherwise, create a GPT table and a unique partition.
         else:
-            prepare_noformat(self.path, uuid)
+            prepare_block(self.path, uuid)
 
 class DeviceType:
     DISK      = 1
@@ -515,16 +515,17 @@ def _get_volume(name):
     volume = __pillar__['metalk8s']['volumes'].get(name)
     if volume is None:
         raise ValueError('volume {} not found in pillar'.format(name))
+    mode = volume['spec'].get('mode', 'Filesystem')
     if 'rawBlockDevice' in volume['spec']:
-        if volume['spec']['rawBlockDevice'].get('noFormat', False):
-            return RawBlockDeviceNoFormat(volume)
-        else:
+        if mode == 'Filesystem':
             return RawBlockDevice(volume)
-    elif 'sparseLoopDevice' in volume['spec']:
-        if volume['spec']['sparseLoopDevice'].get('noFormat', False):
-            return SparseLoopDeviceNoFormat(volume)
         else:
+            return RawBlockDeviceBlock(volume)
+    elif 'sparseLoopDevice' in volume['spec']:
+        if mode == 'Filesystem':
             return SparseLoopDevice(volume)
+        else:
+            return SparseLoopDeviceBlock(volume)
     else:
         raise ValueError('unsupported Volume type for Volume {}'.format(name))
 
@@ -656,8 +657,8 @@ def _mkfs_xfs(path, uuid, force=False, options=None):
     command.append(path)
     return command
 
-def prepare_noformat(path, uuid):
-    """Prepare a "noformat" volume.
+def prepare_block(path, uuid):
+    """Prepare a "Block" volume.
 
     We use a GPT table and a single partition to have a link between the volume
     UUID and the paritition label.

diff --git a/storage-operator/deploy/crds/storage_v1alpha1_volume_crd.yaml b/storage-operator/deploy/crds/storage_v1alpha1_volume_crd.yaml
@@ -38,6 +38,13 @@ spec:
           type: object
         spec:
           properties:
+            mode:
+              description: How the volume is intended to be consumed, either Block
+                or Filesystem (default is Filesystem).
+              enum:
+              - Filesystem
+              - Block
+              type: string
             nodeName:
               description: Name of the node on which the volume is available.
               type: string
@@ -46,17 +53,11 @@ spec:
                 devicePath:
                   description: Path of the block device on the node to back the PersistentVolume.
                   type: string
-                noFormat:
-                  description: When true, do not format the device with a filesystem.
-                  type: boolean
               required:
               - devicePath
               type: object
             sparseLoopDevice:
               properties:
-                noFormat:
-                  description: When true, do not format the device with a filesystem.
-                  type: boolean
                 size:
                   description: Size of the generated sparse file backing the PersistentVolume.
                   type: string

diff --git a/storage-operator/pkg/apis/storage/v1alpha1/volume_types.go b/storage-operator/pkg/apis/storage/v1alpha1/volume_types.go
@@ -16,17 +16,11 @@ import (
 type SparseLoopDeviceVolumeSource struct {
 	// Size of the generated sparse file backing the PersistentVolume.
 	Size resource.Quantity `json:"size"`
-	// When true, do not format the device with a filesystem.
-	// +optional
-	NoFormat bool `json:"noFormat,omitempty"`
 }
 
 type RawBlockDeviceVolumeSource struct {
 	// Path of the block device on the node to back the PersistentVolume.
 	DevicePath string `json:"devicePath"`
-	// When true, do not format the device with a filesystem.
-	// +optional
-	NoFormat bool `json:"noFormat,omitempty"`
 }
 
 type VolumeSource struct {
@@ -49,6 +43,12 @@ type VolumeSpec struct {
 	// PersistentVolume if present.
 	StorageClassName string `json:"storageClassName"`
 
+	// How the volume is intended to be consumed, either Block or Filesystem
+	// (default is Filesystem).
+	// +optional
+	// +kubebuilder:validation:Enum=Filesystem,Block
+	Mode corev1.PersistentVolumeMode `json:"mode,omitempty"`
+
 	// Template for the underlying PersistentVolume.
 	// +optional
 	Template PersistentVolumeTemplateSpec `json:"template,omitempty"`
@@ -272,6 +272,11 @@ func (self *Volume) IsValid() error {
 		}
 	}
 
+	// Default to Filesystem when mode is not specified.
+	if self.Spec.Mode == "" {
+		self.Spec.Mode = corev1.PersistentVolumeFilesystem
+	}
+
 	return nil
 }
 
@@ -288,14 +293,7 @@ func (self *Volume) IsInUnrecoverableFailedState() *VolumeCondition {
 }
 
 func (self *Volume) IsFormatted() bool {
-	switch {
-	case self.Spec.RawBlockDevice != nil:
-		return !self.Spec.RawBlockDevice.NoFormat
-	case self.Spec.SparseLoopDevice != nil:
-		return !self.Spec.SparseLoopDevice.NoFormat
-	default:
-		panic("invalid volume: VolumeSource is not defined")
-	}
+	return self.Spec.Mode == corev1.PersistentVolumeFilesystem
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

diff --git a/storage-operator/pkg/apis/storage/v1alpha1/zz_generated.openapi.go b/storage-operator/pkg/apis/storage/v1alpha1/zz_generated.openapi.go
@@ -107,6 +107,13 @@ func schema_pkg_apis_storage_v1alpha1_VolumeSpec(ref common.ReferenceCallback) c
 							Format:      "",
 						},
 					},
+					"mode": {
+						SchemaProps: spec.SchemaProps{
+							Description: "How the volume is intended to be consumed, either Block or Filesystem (default is Filesystem).",
+							Type:        []string{"string"},
+							Format:      "",
+						},
+					},
 					"template": {
 						SchemaProps: spec.SchemaProps{
 							Description: "Template for the underlying PersistentVolume.",

diff --git a/storage-operator/pkg/controller/volume/volume_controller.go b/storage-operator/pkg/controller/volume/volume_controller.go
@@ -942,12 +942,6 @@ func newPersistentVolume(
 			"missing field 'parameters.fsType' in StorageClass '%s'", scName,
 		)
 	}
-	var mode corev1.PersistentVolumeMode
-	if volume.IsFormatted() {
-		mode = corev1.PersistentVolumeFilesystem
-	} else {
-		mode = corev1.PersistentVolumeBlock
-	}
 
 	pv := corev1.PersistentVolume{
 		ObjectMeta: volume.Spec.Template.Metadata,
@@ -964,7 +958,7 @@ func newPersistentVolume(
 		corev1.ResourceStorage: volumeSize,
 	}
 	pv.Spec.MountOptions = storageClass.MountOptions
-	pv.Spec.VolumeMode = &mode
+	pv.Spec.VolumeMode = &volume.Spec.Mode
 	pv.Spec.PersistentVolumeSource = corev1.PersistentVolumeSource{
 		Local: &corev1.LocalVolumeSource{
 			Path:   deviceInfo.path,

diff --git a/tests/post/features/volume.feature b/tests/post/features/volume.feature
@@ -167,7 +167,7 @@ Feature: Volume management
         Then the Volume 'test-volume11' does not exist
         And the PersistentVolume 'test-volume11' does not exist
 
-    Scenario: Test volume creation (raw sparseLoopDevice)
+    Scenario: Test volume creation (sparseLoopDevice Block mode)
         Given the Kubernetes API is available
         When I create the following Volume:
             apiVersion: storage.metalk8s.scality.com/v1alpha1
@@ -177,9 +177,9 @@ Feature: Volume management
             spec:
               nodeName: bootstrap
               storageClassName: metalk8s-prometheus
+              mode: Block
               sparseLoopDevice:
                 size: 10Gi
-                noFormat: true
         Then the Volume 'test-volume12' is 'Available'
         And the PersistentVolume 'test-volume12' has size '10Gi'
         And the backing storage for Volume 'test-volume12' is created
diff --git a/tests/post/steps/test_volume.py b/tests/post/steps/test_volume.py
@@ -434,10 +434,10 @@ def check_storage_is_created(context, host, name):
     size = int(host.check_output('stat -c %s {}'.format(path)))
     assert _quantity_to_bytes(capacity) == size
     # Check that the loop device is mounted.
-    if volume['spec']['sparseLoopDevice'].get('noFormat', False):
-        host.run_test('test -b /dev/disk/by-partlabel/{}'.format(uuid))
-    else:
+    if volume['spec'].get('mode', 'Filesystem') == 'Filesystem':
         host.run_test('test -b /dev/disk/by-uuid/{}'.format(uuid))
+    else:
+        host.run_test('test -b /dev/disk/by-partlabel/{}'.format(uuid))
 
 
 @then(parsers.parse("the backing storage for Volume '{name}' is deleted"))