-
Notifications
You must be signed in to change notification settings - Fork 228
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 provisioning request #1351
Retry provisioning request #1351
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
70b345a
to
90dae37
Compare
/kind feature |
/hold |
c602a0a
to
b3a3eff
Compare
/hold cancel |
8c033e5
to
d25d221
Compare
if matches(req, wl.Name, checkName) { | ||
prc, err := c.helper.ProvReqConfigForAdmissionCheck(ctx, checkName) | ||
if err == nil && c.reqIsNeeded(ctx, wl, prc) && requestHasParamaters(req, prc) { | ||
if currPr, exists := checkNameToPr[checkName]; !exists || getAttempt(ctx, currPr, wl.Name, checkName) < getAttempt(ctx, req, wl.Name, checkName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should only be one ProvReq that is not Failed at a time. Furthermore, the most recent one is the one that we should care about. There shouldn't be a need to parse the number of attempts from the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTOH, the counter as part of the name has a nice implication: you can't create two ProvReq with the same name, so we don't have to worry about having more than one active ProvReq when the informer cache is slightly out of date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I would add two arguments:
- we need to keep the number of attempts somewhere, we could use annotation, but this is API change and the intention is to backport to 0.5
- it is possible that the request failed and the cloud provider needs some time to clean up nodes / resources corresponding to the old one. Starting new avoids the problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
return reconcile.Result{}, nil | ||
} | ||
|
||
func (c *Controller) getCheckNameToPr(ctx context.Context, wl *kueue.Workload, relevantChecks []string, ownedPrs []autoscaling.ProvisioningRequest) map[string]*autoscaling.ProvisioningRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (c *Controller) getCheckNameToPr(ctx context.Context, wl *kueue.Workload, relevantChecks []string, ownedPrs []autoscaling.ProvisioningRequest) map[string]*autoscaling.ProvisioningRequest { | |
func (c *Controller) activeOrLastPRNameForChecks(ctx context.Context, wl *kueue.Workload, relevantChecks []string, ownedPrs []autoscaling.ProvisioningRequest) map[string]*autoscaling.ProvisioningRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is more specific, rename done, but I use activeOrLastPRForChecks
, to avoid a Get
inside syncCheckStates
@@ -40,7 +42,7 @@ const ( | |||
customResourceTwo = "example.org/res2" | |||
) | |||
|
|||
var _ = ginkgo.Describe("Provisioning", func() { | |||
var _ = ginkgo.Describe("Provisioning", ginkgo.Ordered, ginkgo.ContinueOnFailure, func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modify the vars
, and different tests use different value, so I don't want the tests to run in parallel, but maybe this is not needed?
test/integration/controller/admissionchecks/provisioning/provisioning_test.go
Outdated
Show resolved
Hide resolved
}, util.Timeout, util.Interval).Should(gomega.Succeed()) | ||
}) | ||
|
||
ginkgo.By("Checking the admission check is pending", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to check this. We only need to check that a new ProvReq appears and the check is still Pending by then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well. I have added the check, because this was a source of a bug in my initial implementation. I would transition to Rejected, and then back to Failed when the replacement provisioning request is added. So, I wanted to test that there is no flip to Rejected
test/integration/controller/admissionchecks/provisioning/provisioning_test.go
Outdated
Show resolved
Hide resolved
[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 |
/remove-kind api-change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: eaa1ae065806872292fd04bc9ba2ee7cbf9ce652
|
/hold |
4013407
to
dcde2d8
Compare
/hold cancel |
LGTM label has been added. Git tree hash: ef4a44ddb8782acf7cf00a21007f3391bc91e011
|
func GetProvisioningRequestName(workloadName, checkName string, attempt int32) string { | ||
fullName := fmt.Sprintf("%s-%s-%d", workloadName, checkName, int(attempt)) | ||
return limitObjectName(fullName) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mimowo @alculquicondor What happens if admins upgrade kueue v0.4 to the latest version?
I guess that ProvisioningRequests (PR) using the old name form will be recreated, and I'm wondering if this name form change will affect the PRs if CA is provisioning nodes triggered by the PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding PRs with CapacityAvailable
, Provisioned
, or Failed
, I don't think that the above concerns exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be considered attempt 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be considered attempt 1.
You mean that PRs with old name forms wouldn't recreated, right? If so, that makes sense.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the old PRs need to be recreated as they would not pass the matches
function.
It is easy to match old PRs (without the suffix) as attempt "1", but one thing to consider is that provisioning request wl-ac-1
, would be matching to both ac
and ac-1
.
I think this is ok to leave as is, because there is no support for provisioning requests in Kueue 0.4, it starts in 0.5.0 and the ProvisioningACC
is Alpha. See feature-stages.
@mwielgus @alculquicondor @trasc WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a release note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is always good to have additional safety and not loosing any in-flight requests, but additional work to perfect upgrade story for just released non-default, alpha-like feature seem to have relatively little impact and return on investment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is always good to have additional safety and not loosing any in-flight requests, but additional work to perfect upgrade story for just released non-default, alpha-like feature seem to have relatively little impact and return on investment.
That makes sense. I'm ok with leaving on recreation for ProvisioningRequest since the ProvisioningACC is the Alpha stage.
I believe that users don't use the featureGate yet in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mimowo Thanks for updating the release note. The notification would be helpful :)
…ts (#1595) * Support retry for provisioning requests * Fix creation of the second provisioning request
/release-note-edit
|
What type of PR is this?
/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?