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

Retry for ProvisioningRequests KEP #1355

Merged

Conversation

mimowo
Copy link
Contributor

@mimowo mimowo commented Nov 21, 2023

What type of PR is this?

/kind documentation
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part of #1353

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. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 21, 2023
Copy link

netlify bot commented Nov 21, 2023

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 2ec6f40
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/655e29a4873d980008824eb0

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 21, 2023
@mimowo
Copy link
Contributor Author

mimowo commented Nov 21, 2023

cc @mwielgus @alculquicondor

@mimowo mimowo force-pushed the retry-provisioning-request-kep branch from e8e7aa4 to cde3c0e Compare November 21, 2023 12:54
// between consecutive attempts are determined by expotential growth up to 30min.
// +optional
// +kubebuilder:default=60
DefaultBackoffSeconds *int32 `json:"minBackoffSeconds,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Default or Min? TBH, minBackoff fits better to exponential backoff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was hesitant, minBackoffSeconds sgtm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the API configuration, just leaving a note about the retries.

@mimowo mimowo force-pushed the retry-provisioning-request-kep branch from b5383a8 to 1afa809 Compare November 21, 2023 15:18
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 21, 2023
@mimowo mimowo force-pushed the retry-provisioning-request-kep branch from 1afa809 to 2db7353 Compare November 21, 2023 16:06
@mimowo
Copy link
Contributor Author

mimowo commented Nov 22, 2023

@alculquicondor should we move forward with the KEP note, or close it?

@alculquicondor
Copy link
Contributor

I think there is value in making it configurable in future versions, but I'm ok deferring this for later, based on user feedback.

@mimowo
Copy link
Contributor Author

mimowo commented Nov 22, 2023

I think there is value in making it configurable in future versions, but I'm ok deferring this for later, based on user feedback.

Sure, for now the PR is just a note about the retry itself. Does not describe the API for configuration. Wondering if the note itself is worth it.

* Retry ProvisioningRequests with respect to the `RetryConfig` configuration in
the `ProvisioningRequestConfig`. For each attempt a new provisioning request is
created with the suffix indicating the attempt number. The corresponding admission
check will remain in the `Pending` state until the retries end.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the note about the number of retries

Copy link
Contributor

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 29329b74b0c6d9ffc8910ac8350d96081ba8d329

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, mimowo

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

The pull request process is described 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2023
@k8s-ci-robot k8s-ci-robot merged commit 4d9ad0c into kubernetes-sigs:main Nov 22, 2023
6 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.6 milestone Nov 22, 2023
B1F030 pushed a commit to B1F030/kueue that referenced this pull request Nov 24, 2023
* Retry for ProvisioningRequests KEP

* review comments

* Added a note about defaults for retry mechanism
@mimowo mimowo deleted the retry-provisioning-request-kep branch November 29, 2023 14:57
kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024
* Retry for ProvisioningRequests KEP

* review comments

* Added a note about defaults for retry mechanism
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants