wire csi-lib GateBackoffConfig as --gate-backoff-* flags and Helm values#682
wire csi-lib GateBackoffConfig as --gate-backoff-* flags and Helm values#682patyogesh20 wants to merge 8 commits into
Conversation
Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
cert-manager-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
cert-manager-prow
Bot
added
the
needs-rebase
|
Hi @patyogesh20. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
cert-manager-prow
Bot
added
needs-ok-to-test
Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
cert-manager-prow
Bot
removed
the
needs-rebase
|
@patyogesh20: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
cert-manager-prow
Bot
added
the
needs-rebase
Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
cert-manager-prow
Bot
removed
the
needs-rebase
Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
SgtCoDFish
left a comment
SgtCoDFish
left a comment
There was a problem hiding this comment.
Few minor things, hopefully shouldn't take long. Thank you for this!
| # Backoff applied between gate-pending retries (i.e. when ReadyToRequest | ||
| # reports a readiness gate is not yet met). Distinct from csi-lib's | ||
| # renewal backoff, which protects against signer failures and is left at | ||
| # csi-lib's defaults. Values below mirror csi-lib's own defaults for | ||
| # GateBackoffConfig; override to tune polling cadence for slower or | ||
| # faster gate-resolution profiles. | ||
| gateBackoff: | ||
| duration: 1s | ||
| factor: 2.0 | ||
| jitter: 0.5 | ||
| cap: 60s |
There was a problem hiding this comment.
suggestion: The comment on gateBackoff suggests that the defaults mirror csi-lib, but they don't. The csi-lib cap is 10s, not 60s - we should either fix the comment or fix the default.
There was a problem hiding this comment.
Changed cap: 60s to cap: 10s to match csi-lib default
| duration: 1s | ||
| factor: 2.0 | ||
| jitter: 0.5 | ||
| cap: 60s |
There was a problem hiding this comment.
suggestion: It'dbe nice to add comments to each of the fields explaining their purpose. That'll help the docs!
There was a problem hiding this comment.
I agree. Added comment, they show up in re-rendered README now.
| Duration: opts.GateBackoffDuration, | ||
| Factor: opts.GateBackoffFactor, | ||
| Jitter: opts.GateBackoffJitter, | ||
| Cap: opts.GateBackoffCap, | ||
| Steps: math.MaxInt32, |
There was a problem hiding this comment.
suggestion (non-blocking): We don't validate any of the values from the options at all, which could maybe bite users in the future. I don't think we need to go super crazy validating them (these are fairly advanced options) but maybe we could do some basic checks? Your call.
There was a problem hiding this comment.
I see your point and agree that, the implications of having these values mis-configured are not simple like failed Vs succeeded. Added validateGateBackoff(opts) helper. Validation checks are simple with the failure mode is clear startup error. Error message is to signal how the values should be chosen rather than what.
- Duration > 0
- Factor >= 1
- Jitter in [0,1]
- Cap >= Duration.
There was a problem hiding this comment.
Hmm, I'm not sure about Cap >= Duration - we force Duration > 0 which implies that Cap must be > 0.
But setting Cap to zero would disable the cap, which is a valid thing to want, right?
Put another way, I think we should check Cap == 0 || Cap >= Duration, no?
…validate flags Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
thank you for the review and spotting the incosistency. I have addressed your comments. |
|
/ok-to-test |
cert-manager-prow
Bot
added
ok-to-test
and removed
needs-ok-to-test
SgtCoDFish
left a comment
SgtCoDFish
left a comment
There was a problem hiding this comment.
Couple more bits - please feel free to ignore the AI suggestion as it's a small improvement but not functionally likely to matter
| // misconfigured operator gets a clear startup error rather than a runtime | ||
| // retry storm (e.g. Duration=0 → infinite no-wait spin, Jitter>1 → undefined | ||
| // behaviour in wait.Backoff). | ||
| func validateGateBackoff(opts *options.Options) error { |
There was a problem hiding this comment.
suggestion: I think this'd benefit from some unit tests, e.g.
func TestValidateGateBackoff(t *testing.T) {
cases := map[string]struct {
opts options.Options
wantErr string
}{
"valid defaults": {opts: options.Options{GateBackoffDuration: time.Second, GateBackoffFactor: 2, GateBackoffJitter: 0.5, GateBackoffCap: 10 * time.Second}},
"zero duration": {opts: options.Options{GateBackoffCap: time.Second}, wantErr: "--gate-backoff-duration must be > 0"},
"factor below 1": {opts: options.Options{GateBackoffDuration: time.Second, GateBackoffFactor: 0.5, GateBackoffCap: time.Second}, wantErr: "--gate-backoff-factor must be >= 1"},
"jitter negative": {...},
"jitter above 1": {...},
"cap below duration": {opts: options.Options{GateBackoffDuration: 30 * time.Second, GateBackoffFactor: 1, GateBackoffCap: 10 * time.Second}, wantErr: "--gate-backoff-cap"},
}
...
}| // Gate-pending backoff: applied between readiness-gate checks while the gate | ||
| // is not yet met. Distinct from the renewal backoff used for issuance errors, | ||
| // which is configured by csi-lib's defaults. Defaults below mirror csi-lib's | ||
| // own defaults for GateBackoffConfig so leaving them unset is a no-op. | ||
| fs.DurationVar(&o.GateBackoffDuration, "gate-backoff-duration", time.Second, | ||
| "Base duration between gate-pending retries when --pod-readiness-gate is set.") | ||
| fs.Float64Var(&o.GateBackoffFactor, "gate-backoff-factor", 2.0, | ||
| "Multiplier applied to gate-backoff-duration after each failed gate check.") | ||
| fs.Float64Var(&o.GateBackoffJitter, "gate-backoff-jitter", 0.5, | ||
| "Random jitter (+/- fraction of duration) applied to each gate-pending wait.") | ||
| fs.DurationVar(&o.GateBackoffCap, "gate-backoff-cap", 10*time.Second, | ||
| "Maximum duration between gate-pending retries.") |
There was a problem hiding this comment.
suggestion(AI): Copy pasting an AI review comment here since it explains it well. I think using fs.Changed might be a good shout - but this is gold-plating stuff, and I don't think this is required.
Defaults duplicated in three places → silent-drift risk
csi-lib defines the defaults in manager.go:141-152, options.go redefines them at cmd/app/options/options.go:231-238, and the Helm chart re-states them again in values.yaml and values.schema.json. If csi-lib bumps its defaults in a future release, the csi-driver silently pins to the old values because it always sets GateBackoffConfig when gates are configured (cmd/app/app.go:147-153).
Two possible mitigations, either is fine:
- Preferred: only build GateBackoffConfig when at least one gate-backoff flag was changed by the user; otherwise leave mgrOpts.GateBackoffConfig nil so csi-lib applies its own defaults. pflag.FlagSet.Changed() gives you this per-flag.
if fs.Changed("gate-backoff-duration") || fs.Changed("gate-backoff-factor") ||
fs.Changed("gate-backoff-jitter") || fs.Changed("gate-backoff-cap") {
mgrOpts.GateBackoffConfig = &wait.Backoff{ ... }
}
- This does complicate wiring (needs access to the FlagSet at RunE time), but it preserves the "leaving them unset is a no-op" promise the comment makes.
- Cheaper alternative: add a comment pointing at the exact csi-lib version whose defaults are mirrored (e.g. // Keep in sync with csi-lib@v0.11.0 GateBackoffConfig defaults; see manager.go:141-152.) and treat any csi-lib bump as a prompt to re-audit.
There was a problem hiding this comment.
Agreed on the drift risk. TL;DR - I went with proposed preferred option (FlagSet.Changed()) rather than the comment-only mitigation.
I was checking if there are any precedents/established idiom for "don't duplicate a library's internal default", since I didn't want to introduce a one-off pattern. Here is what I found:
-
RenewalBackoffConfig(csi-lib's other backoff) has no driver-level flags at all. We just never set it, so csi-lib's default always wins. No duplication, but also no operator override, which we do want for gate-backoff. -
--kube-api-qps/--kube-api-burstin this same file use a zero-value sentinel. Clean, but doesn't transfer here as0isn't a safe sentinel for validateGateBackoff duration. -
controller-runtime'smanager.Options.LeaseDuration/RenewDeadline/RetryPeriodare*time.Duration, nil-checked internally. This is the cleanest idiom, but it requires the library (csi-lib) to expose nil-able defaults or exported constants like client-go'sDefaultQPS/DefaultBurst. -
Changed()- based flag precedence shows up in kubeadm/kube-proxy ComponentConfig loading for exactly this situation: a default that can't be expressed as a safe zero value.
One thing worth calling out explicitly: this is a slightly special case relative to the other config knobs in this driver, because --gate-backoff-* exists specifically to give operators optionality to override csi-lib's tuning, unlike RenewalBackoffConfig , which we deliberately don't expose at all. So the fix needs to preserve "operator can override" while still deferring to csi-lib when they don't, which is exactly what Changed() gives us.
Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
…t as invalid Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
|
@patyogesh20: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Why
csi-lib's renewal loop used a single backoff (
RenewalBackoffConfig, default 30s base / 5min cap) for both genuine issuance failures (signer down, apiserver errors, denials) andReadyToRequestFuncreturning false. With--pod-readiness-gateconfigured, that cadence is wrong for the gate-pending case: the underlying condition(CNI assigning a pod IP, a controller flipping a pod condition) typically resolves in seconds, but the driver was sleeping for 30+ seconds between checks. csi-lib#235 separated the two backoffs by adding
GateBackoffConfig. This PR exposes that option to csi-driver operators.What this PR changes
cert-manager-csi-driver, mirroringwait.Backofffields:--gate-backoff-duration(default1s)--gate-backoff-factor(default2.0)--gate-backoff-jitter(default0.5)--gate-backoff-cap(default10s)cmd/app/app.go:mgrOpts.GateBackoffConfigis set from these flagsBackwards compatibility
--pod-readiness-gate: no behaviour change.mgrOpts.GateBackoffConfigis left nil, csi-lib uses its own defaults if--pod-readiness-gate: the gate-pending wait window drops from csi-lib's renewal-backoff cadence (~30s) to the gate-backoff cadence (~1s typical), which is the entire point of the change.Dependencies