Merge pull request #1358 from jamiehannaford/storage-class-spec

Implement StorageClass spec field

CHANGELOG.md

@@ -8,6 +8,8 @@ Finally, it is safe to upgrade operator. It's highly recommended to save a backu
 
 ### Added
 
+- A new `StorageClass` spec field, allowing more granular control over how etcd clusters are backed up to PVs.
+
 ### Changed
 
 - Default timeout for snapshots done by backup sidecar increased from 5 seconds to 1 minute

pkg/cluster/backup_manager.go

@@ -20,6 +20,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"path"
 	"time"
 
 	"github.com/coreos/etcd-operator/client/experimentalclient"
@@ -77,10 +78,14 @@ func (bm *backupManager) setupStorage() (s backupstorage.Storage, err error) {
 	b := cl.Spec.Backup
 	switch b.StorageType {
 	case spec.BackupStorageTypePersistentVolume, spec.BackupStorageTypeDefault:
-		if c.PVProvisioner == constants.PVProvisionerNone {
-			return nil, errNoPVForBackup
+		storageClass := b.PV.StorageClass
+		if len(storageClass) == 0 {
+			if c.PVProvisioner == constants.PVProvisionerNone {
+				return nil, errNoPVForBackup
+			}
+			storageClass = k8sutil.StorageClassPrefix + "-" + path.Base(c.PVProvisioner)
 		}
-		s, err = backupstorage.NewPVStorage(c.KubeCli, cl.Name, cl.Namespace, c.PVProvisioner, *b)
+		s, err = backupstorage.NewPVStorage(c.KubeCli, cl.Name, cl.Namespace, storageClass, *b)
 	case spec.BackupStorageTypeS3:
 		if len(c.S3Context.AWSConfig) == 0 && b.S3 == nil {
 			return nil, errNoS3ConfigForBackup

pkg/cluster/backupstorage/pv.go

@@ -8,26 +8,26 @@ import (
 )
 
 type pv struct {
-	clusterName   string
-	namespace     string
-	pvProvisioner string
-	backupPolicy  spec.BackupPolicy
-	kubecli       kubernetes.Interface
+	clusterName  string
+	namespace    string
+	storageClass string
+	backupPolicy spec.BackupPolicy
+	kubecli      kubernetes.Interface
 }
 
-func NewPVStorage(kubecli kubernetes.Interface, cn, ns, pvp string, backupPolicy spec.BackupPolicy) (Storage, error) {
+func NewPVStorage(kubecli kubernetes.Interface, cn, ns, sc string, backupPolicy spec.BackupPolicy) (Storage, error) {
 	s := &pv{
-		clusterName:   cn,
-		namespace:     ns,
-		pvProvisioner: pvp,
-		backupPolicy:  backupPolicy,
-		kubecli:       kubecli,
+		clusterName:  cn,
+		namespace:    ns,
+		storageClass: sc,
+		backupPolicy: backupPolicy,
+		kubecli:      kubecli,
 	}
 	return s, nil
 }
 
 func (s *pv) Create() error {
-	return k8sutil.CreateAndWaitPVC(s.kubecli, s.clusterName, s.namespace, s.pvProvisioner, s.backupPolicy.PV.VolumeSizeInMB)
+	return k8sutil.CreateAndWaitPVC(s.kubecli, s.clusterName, s.namespace, s.storageClass, s.backupPolicy.PV.VolumeSizeInMB)
 }
 
 func (s *pv) Clone(from string) error {

pkg/util/k8sutil/backup.go

@@ -36,7 +36,9 @@ import (
 )
 
 const (
-	storageClassPrefix        = "etcd-backup"
+	// StorageClassPrefix is the prefix used when creating custom storage classes
+	// for backups through a given provisioner.
+	StorageClassPrefix        = "etcd-backup"
 	BackupPodSelectorAppField = "etcd_backup_tool"
 	backupPVVolName           = "etcd-backup-storage"
 	awsCredentialDir          = "/root/.aws/"
@@ -50,7 +52,7 @@ const (
 
 func CreateStorageClass(kubecli kubernetes.Interface, pvProvisioner string) error {
 	// We need to get rid of prefix because naming doesn't support "/".
-	name := storageClassPrefix + "-" + path.Base(pvProvisioner)
+	name := StorageClassPrefix + "-" + path.Base(pvProvisioner)
 	class := &v1beta1storage.StorageClass{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,
@@ -61,9 +63,8 @@ func CreateStorageClass(kubecli kubernetes.Interface, pvProvisioner string) erro
 	return err
 }
 
-func CreateAndWaitPVC(kubecli kubernetes.Interface, clusterName, ns, pvProvisioner string, volumeSizeInMB int) error {
+func CreateAndWaitPVC(kubecli kubernetes.Interface, clusterName, ns, storageClass string, volumeSizeInMB int) error {
 	name := makePVCName(clusterName)
-	storageClassName := storageClassPrefix + "-" + path.Base(pvProvisioner)
 	claim := &v1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,
@@ -72,7 +73,7 @@ func CreateAndWaitPVC(kubecli kubernetes.Interface, clusterName, ns, pvProvision
 				"app":          "etcd",
 			},
 			Annotations: map[string]string{
-				"volume.beta.kubernetes.io/storage-class": storageClassName,
+				"volume.beta.kubernetes.io/storage-class": storageClass,
 			},
 		},
 		Spec: v1.PersistentVolumeClaimSpec{

test/e2e/cluster_status_test.go

@@ -71,7 +71,7 @@ func TestBackupStatus(t *testing.T) {
 	}
 	f := framework.Global
 
-	bp := e2eutil.NewPVBackupPolicy(true)
+	bp := e2eutil.NewPVBackupPolicy(true, "")
 	testEtcd, err := e2eutil.CreateCluster(t, f.CRClient, f.Namespace, e2eutil.ClusterWithBackup(e2eutil.NewCluster("test-etcd-", 1), bp))
 	if err != nil {
 		t.Fatal(err)

test/e2e/e2eutil/spec_util.go

@@ -61,14 +61,15 @@ func NewOperatorS3BackupPolicy(cleanup bool) *spec.BackupPolicy {
 	}
 }
 
-func NewPVBackupPolicy(cleanup bool) *spec.BackupPolicy {
+func NewPVBackupPolicy(cleanup bool, storageClass string) *spec.BackupPolicy {
 	return &spec.BackupPolicy{
 		BackupIntervalInSecond: 60 * 60,
 		MaxBackups:             5,
 		StorageType:            spec.BackupStorageTypePersistentVolume,
 		StorageSource: spec.StorageSource{
 			PV: &spec.PVSource{
 				VolumeSizeInMB: 512,
+				StorageClass:   storageClass,
 			},
 		},
 		AutoDelete: cleanup,

test/e2e/recovery_test.go

@@ -74,13 +74,23 @@ func TestOneMemberRecovery(t *testing.T) {
 	}
 }
 
-// TestDisasterRecoveryMaj tests disaster recovery that
-// ooperator will make a backup from the left one pod.
+// TestDisasterRecoveryMaj ensures the operator will make a backup from the
+// one remaining pod.
 func TestDisasterRecoveryMaj(t *testing.T) {
 	if os.Getenv(envParallelTest) == envParallelTestTrue {
 		t.Parallel()
 	}
-	testDisasterRecovery(t, 2)
+	testDisasterRecovery(t, 2, "")
+}
+
+// TestDisasterRecoveryMajWithCustomStorageClass ensures that the operator
+// will make a backup using a custom storage class with the state from the
+// one remaining pod.
+func TestDisasterRecoveryMajWithCustomStorageClass(t *testing.T) {
+	if os.Getenv(envParallelTest) == envParallelTestTrue {
+		t.Parallel()
+	}
+	testDisasterRecovery(t, 2, "standard")
 }
 
 // testDisasterRecoveryAll tests disaster recovery that
@@ -89,11 +99,11 @@ func TestDisasterRecoveryAll(t *testing.T) {
 	if os.Getenv(envParallelTest) == envParallelTestTrue {
 		t.Parallel()
 	}
-	testDisasterRecovery(t, 3)
+	testDisasterRecovery(t, 3, "")
 }
 
-func testDisasterRecovery(t *testing.T, numToKill int) {
-	testDisasterRecoveryWithBackupPolicy(t, numToKill, e2eutil.NewPVBackupPolicy(true))
+func testDisasterRecovery(t *testing.T, numToKill int, storageClass string) {
+	testDisasterRecoveryWithBackupPolicy(t, numToKill, e2eutil.NewPVBackupPolicy(true, storageClass))
 }
 
 func testDisasterRecoveryWithBackupPolicy(t *testing.T, numToKill int, backupPolicy *spec.BackupPolicy) {

test/e2e/restore_test.go

@@ -64,7 +64,7 @@ func TestClusterRestoreDifferentName(t *testing.T) {
 }
 
 func testClusterRestore(t *testing.T, needDataClone bool) {
-	testClusterRestoreWithBackupPolicy(t, needDataClone, e2eutil.NewPVBackupPolicy(false))
+	testClusterRestoreWithBackupPolicy(t, needDataClone, e2eutil.NewPVBackupPolicy(false, ""))
 }
 
 func testClusterRestoreWithBackupPolicy(t *testing.T, needDataClone bool, backupPolicy *spec.BackupPolicy) {

test/e2e/upgradetest/upgrade_test.go

@@ -140,7 +140,7 @@ func TestHealOneMemberForOldCluster(t *testing.T) {
 // TestRestoreFromBackup tests that new operator could recover a new cluster from a backup of the old cluster.
 func TestRestoreFromBackup(t *testing.T) {
 	t.Run("Restore from PV backup of old cluster", func(t *testing.T) {
-		testRestoreWithBackupPolicy(t, e2eutil.NewPVBackupPolicy(false))
+		testRestoreWithBackupPolicy(t, e2eutil.NewPVBackupPolicy(false, ""))
 	})
 	t.Run("Restore from S3 backup of old cluster", func(t *testing.T) {
 		t.Run("per cluster s3 policy", func(t *testing.T) {
@@ -257,7 +257,7 @@ func testRestoreWithBackupPolicy(t *testing.T, bp *spec.BackupPolicy) {
 // TestBackupForOldCluster tests that new backup sidecar could make backup from old cluster.
 func TestBackupForOldCluster(t *testing.T) {
 	t.Run("PV backup for old cluster", func(t *testing.T) {
-		testBackupForOldClusterWithBackupPolicy(t, e2eutil.NewPVBackupPolicy(true))
+		testBackupForOldClusterWithBackupPolicy(t, e2eutil.NewPVBackupPolicy(true, ""))
 	})
 	t.Run("S3 backup for old cluster", func(t *testing.T) {
 		t.Run("per cluster s3 policy", func(t *testing.T) {
@@ -340,7 +340,7 @@ func testBackupForOldClusterWithBackupPolicy(t *testing.T, bp *spec.BackupPolicy
 // TestDisasterRecovery tests if the new operator could do disaster recovery from backup of the old cluster.
 func TestDisasterRecovery(t *testing.T) {
 	t.Run("Recover from PV backup", func(t *testing.T) {
-		testDisasterRecoveryWithBackupPolicy(t, e2eutil.NewPVBackupPolicy(true))
+		testDisasterRecoveryWithBackupPolicy(t, e2eutil.NewPVBackupPolicy(true, ""))
 	})
 	t.Run("Recover from S3 backup", func(t *testing.T) {
 		t.Run("per cluster s3 policy", func(t *testing.T) {