Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[KEP] Update KEP #993 - Add retry mechanism to AdmissionChecks #3261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PBundyra
Copy link
Contributor

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Propose changes to introduce retry mechanism for AdmissionChecks

Which issue(s) this PR fixes:

Part of #3258

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 18, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 18, 2024
Copy link

netlify bot commented Oct 18, 2024

Deploy Preview for kubernetes-sigs-kueue ready!

Name Link
🔨 Latest commit 50b9346
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67121f0372a0e900081ac7bb
😎 Deploy Preview https://deploy-preview-3261--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@PBundyra
Copy link
Contributor Author

/assign @mimowo
cc @mwielgus @yaroslava-serdiuk

// backoffMaxSeconds: 600
//
// +listType=map
DefaultRequeuingStrategy *RequeuingStrategy `json:defaultRequeuingStrategy`
Copy link
Contributor

Choose a reason for hiding this comment

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

It this configuration going to be used to configure the mechanism based on this hard-coded values:

defaultMaxRetries = 3
defaultMinBackoffSeconds = 60 // 1 min
defaultMaxBackoffSeconds = 1800 // 30 min
, or is it going to be yet another mechanism?

Please clarify in the KEP, I would prefer to make the existing mechanism configurable and we already have an issue for it #1353. If we reuse the mechanism, can we reuse the same defaults, or state clearly that we are changing the defaults.

type Configuration struct {
...
// admissionCheckRetryStrategy defines strategy for retrying AdmissionChecks
// +optional
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the tags in the global configuration as this is not CRD.

// backoffBaseSeconds: 60
// backoffMaxSeconds: 600
//
// +listType=map
Copy link
Contributor

Choose a reason for hiding this comment

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

you can drop it.

@trasc
Copy link
Contributor

trasc commented Oct 18, 2024

In my opinion we should let individual admission check controllers implement their own retry policy, not all ACCs need this and having another actor managing its ACStates makes room for conflicts or needs additional logic to avoid them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants