Refactor schedule-precedence to allow user overrides

PendingReleaseNotes.md

@@ -0,0 +1,15 @@
+# v0.13.0 Pending Release Notes
+
+## Breaking changes
+
+## Features
+
+- Allow override of precedence of key rotation and reclaim space related annotations
+  using a ConfigMap key `schedule-precedence`. The default is `pvc` which reads the
+  annotations in order of PVC > NS > SC. It can be set to `storageclass` to respect only
+  the annotations found on the Storage Classes.
+
+## NOTE
+
+- `sc-only`, a once valid value for `schedule-precedence` key is being deprecated in favor of
+  `storageclass` and will be removed in a later release.

cmd/manager/main.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"crypto/tls"
 	"flag"
+	"fmt"
 	"os"
 	"time"
 
@@ -100,7 +101,7 @@ func main() {
 	flag.BoolVar(&enableAdmissionWebhooks, "enable-admission-webhooks", false, "[DEPRECATED] Enable the admission webhooks")
 	flag.BoolVar(&secureMetrics, "metrics-secure", true, "If set, the metrics endpoint is served securely via HTTPS. Use --metrics-secure=false to use HTTP instead.")
 	flag.BoolVar(&showVersion, "version", false, "Print Version details")
-	flag.StringVar(&cfg.SchedulePrecedence, "schedule-precedence", "", "The order of precedence in which schedule of reclaimspace and keyrotation is considered. Possible values are sc-only")
+	flag.StringVar(&cfg.SchedulePrecedence, "schedule-precedence", util.SchedulePVC, fmt.Sprintf("The order of precedence in which schedule of reclaimspace and keyrotation is considered. Possible values are %q and %q. Defaults to %q", util.SchedulePVC, util.ScheduleSC, util.SchedulePVC))
 	flag.BoolVar(&enableAuth, "enable-auth", true, "Enables TLS and adds bearer token to the headers (enabled by default)")
 	flag.IntVar(&cfg.MaxGroupPVC, "max-group-pvc", cfg.MaxGroupPVC, "Maximum number of PVCs allowed in a volume group")
 	opts := zap.Options{
@@ -111,11 +112,6 @@ func main() {
 	standardflags.AddAutomaxprocs(setupLog.Info)
 	flag.Parse()
 
-	if cfg.SchedulePrecedence != "" && cfg.SchedulePrecedence != util.ScheduleSCOnly {
-		setupLog.Error(nil, "invalid value for schedule-precedence", "schedule-precedence", cfg.SchedulePrecedence)
-		os.Exit(1)
-	}
-
 	if showVersion {
 		version.PrintVersion()
 		return

docs/csi-addons-config.md

@@ -4,14 +4,17 @@ CSI-Addons Operator can consume configuration from a ConfigMap named `csi-addons
 in the same namespace as the operator. This enables configuration of the operator to persist across
 upgrades. The ConfigMap can support the following configuration options:
 
-| Option                      | Default value | Description                                      |
-| --------------------------- | ------------- | ------------------------------------------------ |
-| `reclaim-space-timeout`     | `"3m"`        | Timeout for reclaimspace operation               |
-| `max-concurrent-reconciles` | `"100"`       | Maximum number of concurrent reconciles          |
-| `max-group-pvcs`            | `"100"`       | Maximum number of PVCs allowed in a volume group |
+| Option                      | Default value | Description                                               |
+| --------------------------- | ------------- | --------------------------------------------------------- |
+| `reclaim-space-timeout`     | `"3m"`        | Timeout for reclaimspace operation                        |
+| `max-concurrent-reconciles` | `"100"`       | Maximum number of concurrent reconciles                   |
+| `max-group-pvcs`            | `"100"`       | Maximum number of PVCs allowed in a volume group          |
+| `schedule-precedence`       | `"pvc"`       | The order in which the schedule annotation should be read |
 
 [`csi-addons-config` ConfigMap](../deploy/controller/csi-addons-config.yaml) is provided as an example.
 
 > Note: The operator pod needs to be restarted for any change in configuration to take effect.
 >
 > Note: `max-group-pvcs` default value is set based on ceph's support/testing. User can tweak this value based on the supported count for their storage vendor.
+>
+> Note: The valid values for `schedule-precedence` are `storageclass` and `pvc`. If set to `storageclass` only the annotations from StorageClasses are considered for schedule of reclaim space and key rotation. `pvc` reads annotations in the order of: PVC > NS > StorageClasses.

internal/controller/csiaddons/persistentvolumeclaim_controller.go

@@ -235,7 +235,7 @@ func (r *PersistentVolumeClaimReconciler) determineScheduleAndRequeue(
 
 	logger.Info("Determining schedule using precedence", "SchedulePrecedence", r.SchedulePrecedence)
 
-	if r.SchedulePrecedence == util.ScheduleSCOnly {
+	if r.SchedulePrecedence == util.ScheduleSC {
 		if schedule, err = r.getScheduleFromSC(ctx, pvc, logger, annotationKey); schedule != "" {
 			return schedule, nil
 		}
@@ -1046,7 +1046,7 @@ func (r *PersistentVolumeClaimReconciler) checkDisabledByAnnotation(
 		return checkAnnotationForValue(storageClass, annotationKey, "false"), nil
 	}
 
-	if r.SchedulePrecedence == util.ScheduleSCOnly {
+	if r.SchedulePrecedence == util.ScheduleSC {
 		disabled, err := isDisabledOnSC()
 		if err != nil {
 			return false, err

internal/util/config.go

@@ -19,6 +19,7 @@ package util
 import (
 	"context"
 	"fmt"
+	"slices"
 	"strconv"
 	"time"
 
@@ -45,7 +46,8 @@ const (
 	defaultMaxConcurrentReconciles = 100
 	defaultReclaimSpaceTimeout     = time.Minute * 3
 	SchedulePrecedenceKey          = "schedule-precedence"
-	ScheduleSCOnly                 = "sc-only"
+	ScheduleSC                     = "storageclass"
+	SchedulePVC                    = "pvc"
 	MaxGroupPVCKey                 = "max-group-pvcs"
 	defaultMaxGroupPVC             = 100 // based on ceph's support/testing
 )
@@ -56,7 +58,7 @@ func NewConfig() Config {
 		Namespace:               defaultNamespace,
 		ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 		MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-		SchedulePrecedence:      "",
+		SchedulePrecedence:      SchedulePVC,
 		MaxGroupPVC:             defaultMaxGroupPVC,
 	}
 }
@@ -98,10 +100,9 @@ func (cfg *Config) readConfig(dataMap map[string]string) error {
 			cfg.MaxConcurrentReconciles = maxConcurrentReconciles
 
 		case SchedulePrecedenceKey:
-			if val != ScheduleSCOnly {
-				return fmt.Errorf("invalid value %q for key %q", val, SchedulePrecedenceKey)
+			if err := cfg.validateAndSetSchedulePrecedence(val); err != nil {
+				return err
 			}
-			cfg.SchedulePrecedence = val
 
 		case MaxGroupPVCKey:
 			maxGroupPVCs, err := strconv.Atoi(val)
@@ -121,3 +122,31 @@ func (cfg *Config) readConfig(dataMap map[string]string) error {
 
 	return nil
 }
+
+// validateAndSetSchedulePrecedence is a helper function to check for
+// valid values for schedule-precedence ConfigMap key.
+// It also preserves backwards compatibility in cases where users might be using
+// once valid values for it i.e. "sc-only" and "".
+func (cfg *Config) validateAndSetSchedulePrecedence(val string) error {
+	validVals := []string{
+		SchedulePVC,
+		ScheduleSC,
+		"sc-only", // This is kept to avoid breaking changes and should be removed in later releases
+		"",        // Same as above
+	}
+
+	if !slices.Contains(validVals, val) {
+		return fmt.Errorf("invalid value %q for key %q", val, SchedulePrecedenceKey)
+	}
+	cfg.SchedulePrecedence = val
+
+	// FIXME: Remove in later releases
+	switch val {
+	case "sc-only":
+		cfg.SchedulePrecedence = ScheduleSC
+	case "":
+		cfg.SchedulePrecedence = SchedulePVC
+	}
+
+	return nil
+}

internal/util/config_test.go

@@ -37,7 +37,7 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: false,
@@ -49,7 +49,7 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: false,
@@ -63,7 +63,7 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     time.Minute * 10,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: false,
@@ -77,7 +77,7 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: true,
@@ -91,7 +91,7 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: 1,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: false,
@@ -105,7 +105,7 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: true,
@@ -120,7 +120,7 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     time.Minute * 10,
 				MaxConcurrentReconciles: 5,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: false,
@@ -134,21 +134,49 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: true,
 		},
 		{
-			name: "config file modifies schedule-precedence",
+			name: "config file modifies schedule-precedence to pvc",
+			dataMap: map[string]string{
+				"schedule-precedence": "pvc",
+			},
+			newConfig: Config{
+				Namespace:               defaultNamespace,
+				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
+				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
+				SchedulePrecedence:      SchedulePVC,
+				MaxGroupPVC:             100,
+			},
+			wantErr: false,
+		},
+		{
+			name: "config file modifies schedule-precedence to storageclass",
+			dataMap: map[string]string{
+				"schedule-precedence": "storageclass",
+			},
+			newConfig: Config{
+				Namespace:               defaultNamespace,
+				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
+				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
+				SchedulePrecedence:      ScheduleSC,
+				MaxGroupPVC:             100,
+			},
+			wantErr: false,
+		},
+		{
+			name: "config file modifies schedule-precedence to a once valid value of sc-only",
 			dataMap: map[string]string{
 				"schedule-precedence": "sc-only",
 			},
 			newConfig: Config{
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      ScheduleSCOnly,
+				SchedulePrecedence:      ScheduleSC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: false,
@@ -162,24 +190,24 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: true,
 		},
 		{
-			name: "config file has empty schedule-precedence",
+			name: "config file has empty schedule-precedence which was once valid and inferred as pvc",
 			dataMap: map[string]string{
 				"schedule-precedence": "",
 			},
 			newConfig: Config{
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
-			wantErr: true,
+			wantErr: false,
 		},
 		{
 			name: "config file has empty max-group-pvcs",
@@ -190,7 +218,7 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: true,
@@ -204,7 +232,7 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             25,
 			},
 			wantErr: false,
@@ -218,7 +246,7 @@ func TestConfigReadConfigFile(t *testing.T) {
 				Namespace:               defaultNamespace,
 				ReclaimSpaceTimeout:     defaultReclaimSpaceTimeout,
 				MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
-				SchedulePrecedence:      "",
+				SchedulePrecedence:      SchedulePVC,
 				MaxGroupPVC:             100,
 			},
 			wantErr: true,