Skip to content

Commit 2258621

Browse files
black-dragon74black-dragon74
committed
Refactor schedule-precedence to allow overrides
This patch refactors the schedule-precedence logic to allow the value to be overridden by a ConfigMap. The default mode of operation still prefers PVCs first as source of key rotation and reclaimspace annotations. The values for `schedule-precedence` keys are now concretely defined as either `pvc` or `storageclass`. Signed-off-by: Niraj Yadav <niryadav@redhat.com>
1 parent 1bd5a01 commit 2258621

File tree

4 files changed

+83
-30
lines changed

4 files changed

+83
-30
lines changed

‎cmd/manager/main.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"context"
2121
"crypto/tls"
2222
"flag"
23+
"fmt"
2324
"os"
2425
"time"
2526

@@ -100,7 +101,7 @@ func main() {
100101
flag.BoolVar(&enableAdmissionWebhooks, "enable-admission-webhooks", false, "[DEPRECATED] Enable the admission webhooks")
101102
flag.BoolVar(&secureMetrics, "metrics-secure", true, "If set, the metrics endpoint is served securely via HTTPS. Use --metrics-secure=false to use HTTP instead.")
102103
flag.BoolVar(&showVersion, "version", false, "Print Version details")
103-
flag.StringVar(&cfg.SchedulePrecedence, "schedule-precedence", "", "The order of precedence in which schedule of reclaimspace and keyrotation is considered. Possible values are sc-only")
104+
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))
104105
flag.BoolVar(&enableAuth, "enable-auth", true, "Enables TLS and adds bearer token to the headers (enabled by default)")
105106
flag.IntVar(&cfg.MaxGroupPVC, "max-group-pvc", cfg.MaxGroupPVC, "Maximum number of PVCs allowed in a volume group")
106107
opts := zap.Options{
@@ -111,11 +112,6 @@ func main() {
111112
standardflags.AddAutomaxprocs(setupLog.Info)
112113
flag.Parse()
113114

114-
if cfg.SchedulePrecedence != "" && cfg.SchedulePrecedence != util.ScheduleSCOnly {
115-
setupLog.Error(nil, "invalid value for schedule-precedence", "schedule-precedence", cfg.SchedulePrecedence)
116-
os.Exit(1)
117-
}
118-
119115
if showVersion {
120116
version.PrintVersion()
121117
return

‎internal/controller/csiaddons/persistentvolumeclaim_controller.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ func (r *PersistentVolumeClaimReconciler) determineScheduleAndRequeue(
235235

236236
logger.Info("Determining schedule using precedence", "SchedulePrecedence", r.SchedulePrecedence)
237237

238-
if r.SchedulePrecedence == util.ScheduleSCOnly {
238+
if r.SchedulePrecedence == util.ScheduleSC {
239239
if schedule, err = r.getScheduleFromSC(ctx, pvc, logger, annotationKey); schedule != "" {
240240
return schedule, nil
241241
}
@@ -1046,7 +1046,7 @@ func (r *PersistentVolumeClaimReconciler) checkDisabledByAnnotation(
10461046
return checkAnnotationForValue(storageClass, annotationKey, "false"), nil
10471047
}
10481048

1049-
if r.SchedulePrecedence == util.ScheduleSCOnly {
1049+
if r.SchedulePrecedence == util.ScheduleSC {
10501050
disabled, err := isDisabledOnSC()
10511051
if err != nil {
10521052
return false, err

‎internal/util/config.go

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package util
1919
import (
2020
"context"
2121
"fmt"
22+
"slices"
2223
"strconv"
2324
"time"
2425

@@ -45,7 +46,8 @@ const (
4546
defaultMaxConcurrentReconciles = 100
4647
defaultReclaimSpaceTimeout = time.Minute * 3
4748
SchedulePrecedenceKey = "schedule-precedence"
48-
ScheduleSCOnly = "sc-only"
49+
ScheduleSC = "storageclass"
50+
SchedulePVC = "pvc"
4951
MaxGroupPVCKey = "max-group-pvcs"
5052
defaultMaxGroupPVC = 100 // based on ceph's support/testing
5153
)
@@ -56,7 +58,7 @@ func NewConfig() Config {
5658
Namespace: defaultNamespace,
5759
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
5860
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
59-
SchedulePrecedence: "",
61+
SchedulePrecedence: SchedulePVC,
6062
MaxGroupPVC: defaultMaxGroupPVC,
6163
}
6264
}
@@ -98,10 +100,9 @@ func (cfg *Config) readConfig(dataMap map[string]string) error {
98100
cfg.MaxConcurrentReconciles = maxConcurrentReconciles
99101

100102
case SchedulePrecedenceKey:
101-
if val != ScheduleSCOnly {
102-
return fmt.Errorf("invalid value %q for key %q", val, SchedulePrecedenceKey)
103+
if err := cfg.validateAndSetSchedulePrecedence(val); err != nil {
104+
return err
103105
}
104-
cfg.SchedulePrecedence = val
105106

106107
case MaxGroupPVCKey:
107108
maxGroupPVCs, err := strconv.Atoi(val)
@@ -121,3 +122,31 @@ func (cfg *Config) readConfig(dataMap map[string]string) error {
121122

122123
return nil
123124
}
125+
126+
// validateAndSetSchedulePrecedence is a helper function to check for
127+
// valid values for schedule-precedence ConfigMap key.
128+
// It also preserves backwards compatibility in cases where users might be using
129+
// once valid values for it i.e. "sc-only" and "".
130+
func (cfg *Config) validateAndSetSchedulePrecedence(val string) error {
131+
validVals := []string{
132+
SchedulePVC,
133+
ScheduleSC,
134+
"sc-only", // This is kept to avoid breaking changes and should be removed in later releases
135+
"", // Same as above
136+
}
137+
138+
if !slices.Contains(validVals, val) {
139+
return fmt.Errorf("invalid value %q for key %q", val, SchedulePrecedenceKey)
140+
}
141+
cfg.SchedulePrecedence = val
142+
143+
// FIXME: Remove in later releases
144+
switch val {
145+
case "sc-only":
146+
cfg.SchedulePrecedence = ScheduleSC
147+
case "":
148+
cfg.SchedulePrecedence = SchedulePVC
149+
}
150+
151+
return nil
152+
}

‎internal/util/config_test.go

Lines changed: 45 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ func TestConfigReadConfigFile(t *testing.T) {
3737
Namespace: defaultNamespace,
3838
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
3939
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
40-
SchedulePrecedence: "",
40+
SchedulePrecedence: SchedulePVC,
4141
MaxGroupPVC: 100,
4242
},
4343
wantErr: false,
@@ -49,7 +49,7 @@ func TestConfigReadConfigFile(t *testing.T) {
4949
Namespace: defaultNamespace,
5050
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
5151
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
52-
SchedulePrecedence: "",
52+
SchedulePrecedence: SchedulePVC,
5353
MaxGroupPVC: 100,
5454
},
5555
wantErr: false,
@@ -63,7 +63,7 @@ func TestConfigReadConfigFile(t *testing.T) {
6363
Namespace: defaultNamespace,
6464
ReclaimSpaceTimeout: time.Minute * 10,
6565
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
66-
SchedulePrecedence: "",
66+
SchedulePrecedence: SchedulePVC,
6767
MaxGroupPVC: 100,
6868
},
6969
wantErr: false,
@@ -77,7 +77,7 @@ func TestConfigReadConfigFile(t *testing.T) {
7777
Namespace: defaultNamespace,
7878
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
7979
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
80-
SchedulePrecedence: "",
80+
SchedulePrecedence: SchedulePVC,
8181
MaxGroupPVC: 100,
8282
},
8383
wantErr: true,
@@ -91,7 +91,7 @@ func TestConfigReadConfigFile(t *testing.T) {
9191
Namespace: defaultNamespace,
9292
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
9393
MaxConcurrentReconciles: 1,
94-
SchedulePrecedence: "",
94+
SchedulePrecedence: SchedulePVC,
9595
MaxGroupPVC: 100,
9696
},
9797
wantErr: false,
@@ -105,7 +105,7 @@ func TestConfigReadConfigFile(t *testing.T) {
105105
Namespace: defaultNamespace,
106106
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
107107
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
108-
SchedulePrecedence: "",
108+
SchedulePrecedence: SchedulePVC,
109109
MaxGroupPVC: 100,
110110
},
111111
wantErr: true,
@@ -120,7 +120,7 @@ func TestConfigReadConfigFile(t *testing.T) {
120120
Namespace: defaultNamespace,
121121
ReclaimSpaceTimeout: time.Minute * 10,
122122
MaxConcurrentReconciles: 5,
123-
SchedulePrecedence: "",
123+
SchedulePrecedence: SchedulePVC,
124124
MaxGroupPVC: 100,
125125
},
126126
wantErr: false,
@@ -134,21 +134,49 @@ func TestConfigReadConfigFile(t *testing.T) {
134134
Namespace: defaultNamespace,
135135
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
136136
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
137-
SchedulePrecedence: "",
137+
SchedulePrecedence: SchedulePVC,
138138
MaxGroupPVC: 100,
139139
},
140140
wantErr: true,
141141
},
142142
{
143-
name: "config file modifies schedule-precedence",
143+
name: "config file modifies schedule-precedence to pvc",
144+
dataMap: map[string]string{
145+
"schedule-precedence": "pvc",
146+
},
147+
newConfig: Config{
148+
Namespace: defaultNamespace,
149+
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
150+
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
151+
SchedulePrecedence: SchedulePVC,
152+
MaxGroupPVC: 100,
153+
},
154+
wantErr: false,
155+
},
156+
{
157+
name: "config file modifies schedule-precedence to storageclass",
158+
dataMap: map[string]string{
159+
"schedule-precedence": "storageclass",
160+
},
161+
newConfig: Config{
162+
Namespace: defaultNamespace,
163+
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
164+
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
165+
SchedulePrecedence: ScheduleSC,
166+
MaxGroupPVC: 100,
167+
},
168+
wantErr: false,
169+
},
170+
{
171+
name: "config file modifies schedule-precedence to a once valid value of sc-only",
144172
dataMap: map[string]string{
145173
"schedule-precedence": "sc-only",
146174
},
147175
newConfig: Config{
148176
Namespace: defaultNamespace,
149177
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
150178
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
151-
SchedulePrecedence: ScheduleSCOnly,
179+
SchedulePrecedence: ScheduleSC,
152180
MaxGroupPVC: 100,
153181
},
154182
wantErr: false,
@@ -162,24 +190,24 @@ func TestConfigReadConfigFile(t *testing.T) {
162190
Namespace: defaultNamespace,
163191
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
164192
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
165-
SchedulePrecedence: "",
193+
SchedulePrecedence: SchedulePVC,
166194
MaxGroupPVC: 100,
167195
},
168196
wantErr: true,
169197
},
170198
{
171-
name: "config file has empty schedule-precedence",
199+
name: "config file has empty schedule-precedence which was once valid and inferred as pvc",
172200
dataMap: map[string]string{
173201
"schedule-precedence": "",
174202
},
175203
newConfig: Config{
176204
Namespace: defaultNamespace,
177205
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
178206
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
179-
SchedulePrecedence: "",
207+
SchedulePrecedence: SchedulePVC,
180208
MaxGroupPVC: 100,
181209
},
182-
wantErr: true,
210+
wantErr: false,
183211
},
184212
{
185213
name: "config file has empty max-group-pvcs",
@@ -190,7 +218,7 @@ func TestConfigReadConfigFile(t *testing.T) {
190218
Namespace: defaultNamespace,
191219
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
192220
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
193-
SchedulePrecedence: "",
221+
SchedulePrecedence: SchedulePVC,
194222
MaxGroupPVC: 100,
195223
},
196224
wantErr: true,
@@ -204,7 +232,7 @@ func TestConfigReadConfigFile(t *testing.T) {
204232
Namespace: defaultNamespace,
205233
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
206234
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
207-
SchedulePrecedence: "",
235+
SchedulePrecedence: SchedulePVC,
208236
MaxGroupPVC: 25,
209237
},
210238
wantErr: false,
@@ -218,7 +246,7 @@ func TestConfigReadConfigFile(t *testing.T) {
218246
Namespace: defaultNamespace,
219247
ReclaimSpaceTimeout: defaultReclaimSpaceTimeout,
220248
MaxConcurrentReconciles: defaultMaxConcurrentReconciles,
221-
SchedulePrecedence: "",
249+
SchedulePrecedence: SchedulePVC,
222250
MaxGroupPVC: 100,
223251
},
224252
wantErr: true,

0 commit comments

Comments
 (0)