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.

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

Make fstype configurable in external provisioner #400

Merged
merged 3 commits into from
Jun 17, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions cmd/csi-provisioner/csi-provisioner.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,28 +26,25 @@ import (
"strings"
"time"

flag "github.com/spf13/pflag"

"github.com/kubernetes-csi/csi-lib-utils/deprecatedflags"
"github.com/kubernetes-csi/csi-lib-utils/leaderelection"
"github.com/kubernetes-csi/csi-lib-utils/metrics"
ctrl "github.com/kubernetes-csi/external-provisioner/pkg/controller"
snapclientset "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned"
"sigs.k8s.io/sig-storage-lib-external-provisioner/v5/controller"

flag "github.com/spf13/pflag"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/util/workqueue"
"k8s.io/klog"

utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/client-go/informers"
"k8s.io/client-go/kubernetes"
v1 "k8s.io/client-go/listers/core/v1"
storagelistersv1 "k8s.io/client-go/listers/storage/v1"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/util/workqueue"
utilflag "k8s.io/component-base/cli/flag"
csitrans "k8s.io/csi-translation-lib"
"k8s.io/klog"
"sigs.k8s.io/sig-storage-lib-external-provisioner/v5/controller"
)

var (
Expand All @@ -74,6 +71,8 @@ var (
metricsAddress = flag.String("metrics-address", "", "The TCP network address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means metrics endpoint is disabled.")
metricsPath = flag.String("metrics-path", "/metrics", "The HTTP path where prometheus metrics will be exposed. Default is `/metrics`.")

defaultFSType = flag.String("default-fstype", "", "Specify the filesystem type of the volume. If not specified, external-provisioner will not set any default filesystem type.")

featureGates map[string]bool
provisionController *controller.ProvisionController
version = "unknown"
Expand Down Expand Up @@ -240,6 +239,7 @@ func main() {
claimLister,
vaLister,
*extraCreateMetadata,
*defaultFSType,
)

provisionController = controller.NewProvisionController(
Expand Down
9 changes: 5 additions & 4 deletions pkg/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,6 @@ const (
backoffFactor = 1.2
backoffSteps = 10

defaultFSType = "ext4"

snapshotKind = "VolumeSnapshot"
snapshotAPIGroup = snapapi.GroupName // "snapshot.storage.k8s.io"
pvcKind = "PersistentVolumeClaim" // Native types don't require an API group
Expand Down Expand Up @@ -211,6 +209,7 @@ type csiProvisioner struct {
timeout time.Duration
identity string
volumeNamePrefix string
defaultFSType string
volumeNameUUIDLength int
config *rest.Config
driverName string
Expand Down Expand Up @@ -292,6 +291,7 @@ func NewCSIProvisioner(client kubernetes.Interface,
claimLister corelisters.PersistentVolumeClaimLister,
vaLister storagelistersv1.VolumeAttachmentLister,
extraCreateMetadata bool,
defaultFSType string,
) controller.Provisioner {
broadcaster := record.NewBroadcaster()
broadcaster.StartLogging(klog.Infof)
Expand All @@ -307,6 +307,7 @@ func NewCSIProvisioner(client kubernetes.Interface,
timeout: connectionTimeout,
identity: identity,
volumeNamePrefix: volumeNamePrefix,
defaultFSType: defaultFSType,
volumeNameUUIDLength: volumeNameUUIDLength,
driverName: driverName,
pluginCapabilities: pluginCapabilities,
Expand Down Expand Up @@ -518,8 +519,8 @@ func (p *csiProvisioner) ProvisionExt(options controller.ProvisionOptions) (*v1.
if fsTypesFound > 1 {
return nil, controller.ProvisioningFinished, fmt.Errorf("fstype specified in parameters with both \"fstype\" and \"%s\" keys", prefixedFsTypeKey)
}
if len(fsType) == 0 {
fsType = defaultFSType
if fsType == "" && p.defaultFSType != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add unit tests that test the various combinations?

  • StorageClass set + default set
  • StorageClass unset + default set
  • StorageClass set + default unset
  • StorageClass unset + default unset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test cases ! ..

fsType = p.defaultFSType
}

capacity := options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]
Expand Down
27 changes: 13 additions & 14 deletions pkg/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ import (
"testing"
"time"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/container-storage-interface/spec/lib/go/csi"
"github.com/golang/mock/gomock"
"github.com/kubernetes-csi/csi-lib-utils/connection"
Expand All @@ -40,6 +37,8 @@ import (
crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1beta1"
"github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/fake"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
v1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
"k8s.io/apimachinery/pkg/api/resource"
Expand Down Expand Up @@ -71,6 +70,7 @@ var (

driverNameAnnotation = map[string]string{annStorageProvisioner: driverName}
translatedKey = "translated"
defaultfsType = "ext4"
)

type csiConnection struct {
Expand Down Expand Up @@ -412,7 +412,7 @@ func TestCreateDriverReturnsInvalidCapacityDuringProvision(t *testing.T) {

pluginCaps, controllerCaps := provisionCapabilities()
csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test",
5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false)
5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false, defaultfsType)

// Requested PVC with requestedBytes storage
deletePolicy := v1.PersistentVolumeReclaimDelete
Expand Down Expand Up @@ -1710,8 +1710,7 @@ func runProvisionTest(t *testing.T, k string, tc provisioningTestcase, requested

pluginCaps, controllerCaps := provisionCapabilities()
csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn,
nil, provisionDriverName, pluginCaps, controllerCaps, supportsMigrationFromInTreePluginName, false, csitrans.New(), nil, nil, nil, nil, nil, tc.withExtraMetadata)

nil, provisionDriverName, pluginCaps, controllerCaps, supportsMigrationFromInTreePluginName, false, csitrans.New(), nil, nil, nil, nil, nil, tc.withExtraMetadata, defaultfsType)
out := &csi.CreateVolumeResponse{
Volume: &csi.Volume{
CapacityBytes: requestedBytes,
Expand Down Expand Up @@ -2446,7 +2445,7 @@ func TestProvisionFromSnapshot(t *testing.T) {

pluginCaps, controllerCaps := provisionFromSnapshotCapabilities()
csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn,
client, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false)
client, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false, defaultfsType)

out := &csi.CreateVolumeResponse{
Volume: &csi.Volume{
Expand Down Expand Up @@ -2620,7 +2619,7 @@ func TestProvisionWithTopologyEnabled(t *testing.T) {
defer close(stopChan)

csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5,
csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, vaLister, false)
csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, vaLister, false, defaultfsType)

pv, err := csiProvisioner.Provision(controller.ProvisionOptions{
StorageClass: &storagev1.StorageClass{},
Expand Down Expand Up @@ -2714,7 +2713,7 @@ func TestProvisionErrorHandling(t *testing.T) {
defer close(stopChan)

csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5,
csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, vaLister, false)
csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, vaLister, false, defaultfsType)
csiProvisionerExt := csiProvisioner.(controller.ProvisionerExt)

options := controller.ProvisionOptions{
Expand Down Expand Up @@ -2788,7 +2787,7 @@ func TestProvisionWithTopologyDisabled(t *testing.T) {
clientSet := fakeclientset.NewSimpleClientset()
pluginCaps, controllerCaps := provisionWithTopologyCapabilities()
csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5,
csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false)
csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false, defaultfsType)

out := &csi.CreateVolumeResponse{
Volume: &csi.Volume{
Expand Down Expand Up @@ -3125,7 +3124,7 @@ func runDeleteTest(t *testing.T, k string, tc deleteTestcase) {
pluginCaps, controllerCaps := provisionCapabilities()
scLister, _, _, _, vaLister, _ := listers(clientSet)
csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5,
csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, nil, nil, nil, vaLister, false)
csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, nil, nil, nil, vaLister, false, defaultfsType)

err = csiProvisioner.Delete(tc.persistentVolume)
if tc.expectErr && err == nil {
Expand Down Expand Up @@ -3546,7 +3545,7 @@ func TestProvisionFromPVC(t *testing.T) {

// Phase: execute the test
csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn,
nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, claimLister, nil, false)
nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, claimLister, nil, false, defaultfsType)

pv, err = csiProvisioner.Provision(tc.volOpts)
if tc.expectErr && err == nil {
Expand Down Expand Up @@ -3664,7 +3663,7 @@ func TestProvisionWithMigration(t *testing.T) {
pluginCaps, controllerCaps := provisionCapabilities()
csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner",
"test", 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps,
inTreePluginName, false, mockTranslator, nil, nil, nil, nil, nil, false)
inTreePluginName, false, mockTranslator, nil, nil, nil, nil, nil, false, defaultfsType)

// Set up return values (AnyTimes to avoid overfitting on implementation)

Expand Down Expand Up @@ -3826,7 +3825,7 @@ func TestDeleteMigration(t *testing.T) {
defer close(stopCh)
csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner",
"test", 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "",
false, mockTranslator, nil, nil, nil, nil, vaLister, false)
false, mockTranslator, nil, nil, nil, nil, vaLister, false, defaultfsType)

// Set mock return values (AnyTimes to avoid overfitting on implementation details)
mockTranslator.EXPECT().IsPVMigratable(gomock.Any()).Return(tc.expectTranslation).AnyTimes()
Expand Down