Skip to content

wire csi-lib GateBackoffConfig as --gate-backoff-* flags and Helm values#682

Open
patyogesh20 wants to merge 7 commits into
cert-manager:mainfrom
patyogesh20:feat/wire-gate-pending-backoff
Open

wire csi-lib GateBackoffConfig as --gate-backoff-* flags and Helm values#682
patyogesh20 wants to merge 7 commits into
cert-manager:mainfrom
patyogesh20:feat/wire-gate-pending-backoff

Conversation

@patyogesh20

@patyogesh20 patyogesh20 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

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) and ReadyToRequestFunc returning false. With --pod-readiness-gate configured, 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

  • Four new CLI flags on cert-manager-csi-driver, mirroring wait.Backoff fields:
    • --gate-backoff-duration (default 1s)
    • --gate-backoff-factor (default 2.0)
    • --gate-backoff-jitter (default 0.5)
    • --gate-backoff-cap (default 10s)
  • Wiring in cmd/app/app.go: mgrOpts.GateBackoffConfig is set from these flags

Backwards compatibility

  • Deployments without --pod-readiness-gate: no behaviour change. mgrOpts.GateBackoffConfig is left nil, csi-lib uses its own defaults if
  • Deployments with --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.
  • Existing RenewalBackoffConfig behaviour is unchanged. Issuance errors still use the signer-protective backoff.

Dependencies

  • go.mod currently points at a pseudo-version of csi-lib (v0.10.1-0.20260624100922-4eeba3e02d4e) covering the merge of csi-lib#235. Before merging this PR, the dependency should be bumped to a tagged csi-lib release containing [CI] Merge self-upgrade into main #235 — happy to amend once a release is cut.

@cert-manager-prow cert-manager-prow Bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Jun 26, 2026
@cert-manager-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign maelvls for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cert-manager-prow cert-manager-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2026
@cert-manager-prow

Copy link
Copy Markdown
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 cert-manager-prow Bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 26, 2026
@cert-manager-prow

Copy link
Copy Markdown
Contributor

@patyogesh20: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/ok-to-test

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 cert-manager-prow Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 27, 2026

@SgtCoDFish SgtCoDFish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Few minor things, hopefully shouldn't take long. Thank you for this!

Comment thread deploy/charts/csi-driver/values.yaml Outdated
Comment thread deploy/charts/csi-driver/values.yaml Outdated
Comment thread cmd/app/app.go Outdated
Comment on lines +143 to +147
Duration: opts.GateBackoffDuration,
Factor: opts.GateBackoffFactor,
Jitter: opts.GateBackoffJitter,
Cap: opts.GateBackoffCap,
Steps: math.MaxInt32,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

  1. Duration > 0
  2. Factor >= 1
  3. Jitter in [0,1]
  4. Cap >= Duration.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is addressed now with following check in validateGateBackoff() func in cmd/app/app.go

if opts.GateBackoffCap != 0 && opts.GateBackoffCap < opts.GateBackoffDuration {
		return fmt.Errorf("--gate-backoff-cap (%s) must be 0 (uncapped) or >= --gate-backoff-duration (%s)", opts.GateBackoffCap, opts.GateBackoffDuration)
	}

@patyogesh20 patyogesh20 requested a review from SgtCoDFish July 1, 2026 16:37
@patyogesh20

Copy link
Copy Markdown
Contributor Author

Few minor things, hopefully shouldn't take long. Thank you for this!

thank you for the review and spotting the incosistency. I have addressed your comments.

@SgtCoDFish

Copy link
Copy Markdown
Member

/ok-to-test

@cert-manager-prow cert-manager-prow Bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 2, 2026

@SgtCoDFish SgtCoDFish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Couple more bits - please feel free to ignore the AI suggestion as it's a small improvement but not functionally likely to matter

Comment thread cmd/app/app.go
Comment thread cmd/app/options/options.go Outdated
Comment on lines +227 to +238
// 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.")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@patyogesh20 patyogesh20 Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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:

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.

@cert-manager-prow cert-manager-prow Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 9, 2026
Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
…validate flags

Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
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>
Signed-off-by: Yogesh Patil <patyogesh@gmail.com>
@patyogesh20 patyogesh20 force-pushed the feat/wire-gate-pending-backoff branch from 69d25c6 to d7d6523 Compare July 9, 2026 18:42
@patyogesh20

Copy link
Copy Markdown
Contributor Author

@SgtCoDFish I have addressed your latest comments; consider giving it one more pass.

@patyogesh20 patyogesh20 requested a review from SgtCoDFish July 9, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants