fix: update failure in requeue #245

kerthcet · 2022-05-03T08:52:01Z

Signed-off-by: kerthcet [email protected]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part of #241

Special notes for your reviewer:

kerthcet · 2022-05-03T08:52:38Z

/test pull-kueue-test-integration-main

kerthcet · 2022-05-03T08:55:21Z

/test pull-kueue-test-integration-main

kerthcet · 2022-05-03T08:59:16Z

/test pull-kueue-test-integration-main

kerthcet · 2022-05-03T09:02:05Z

/test pull-kueue-test-integration-main

alculquicondor · 2022-05-03T13:13:47Z

pkg/queue/manager.go

@@ -300,13 +300,14 @@ func (m *Manager) RequeueWorkload(ctx context.Context, info *workload.Info, imme
 		return false
 	}

-	q.AddIfNotPresent(info)
+	newInfo := workload.NewInfo(&w)


Can you explain why you think this solves the problem?

If there was a change in the object, the event would update the object, and AddIfNotPresent would prevent it to be added again.

Refer to the log, we are always failing with update operation, this must be the reason that we stored the old version workload. So we should ensure to store the latest workload. The reason might be that the failed api update with the old version requeue earlier than the newest one, then we stored the old ones. We can also find some clues from the log:

But that screenshot suggests that it wasn't actually added. So I'm not sure if this solves the issue or maybe it obscures a different one.

What's the link for this failure? I want to look at the entire logs.

Here's the link https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kueue/227/pull-kueue-test-integration-main/1520594730851766272/build-log.txt

From the context, we know that the `assume-1` is return from the old version, it requeued successfully, the `assume-2` is from the new version, it requeued failed for old workload has already requeued.

Right, we are actually rejected the newer version. Thanks for the extra explanation.

alculquicondor

Can you add unit tests?

alculquicondor · 2022-05-03T17:56:53Z

pkg/queue/manager.go

@@ -300,13 +300,14 @@ func (m *Manager) RequeueWorkload(ctx context.Context, info *workload.Info, imme
 		return false
 	}

-	q.AddIfNotPresent(info)
+	newInfo := workload.NewInfo(&w)


Right, we are actually rejected the newer version. Thanks for the extra explanation.

pkg/queue/manager.go

alculquicondor · 2022-05-03T18:03:27Z

pkg/queue/manager.go

 	cq := m.clusterQueues[q.ClusterQueue]
 	if cq == nil {
 		return false
 	}

-	added := cq.RequeueIfNotPresent(info, immediate)
+	added := cq.RequeueIfNotPresent(newInfo, immediate)


Should we change this for just Requeue instead of RequeueIfNotPresent?

It might not be necessary, as one of the routines trying to requeue will have the chance to push the newest version.

pkg/queue/manager_test.go

pkg/queue/manager.go

pkg/queue/manager_test.go

kerthcet · 2022-05-05T16:19:09Z

address the comments. @alculquicondor

pkg/queue/manager.go

pkg/queue/manager_test.go

alculquicondor

Please squash.

pkg/queue/manager.go

alculquicondor · 2022-05-06T13:54:01Z

/approve

k8s-ci-robot · 2022-05-06T13:54:14Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [alculquicondor]

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

Signed-off-by: kerthcet <[email protected]>

kerthcet · 2022-05-06T14:06:39Z

The #248 bug occurs, I'll take a spike.

alculquicondor · 2022-05-06T14:48:41Z

/hold

could it be related to this PR? Have you seen the same error for other PRs?

kerthcet · 2022-05-06T15:03:26Z

I think it's not related. I used to met this error for multi times in my local dev env. I think we can ignore the error in the pr.

alculquicondor

/lgtm
/hold cancel

kerthcet · 2022-05-06T16:06:09Z

/retest

…pstream-release-0.1 Automated cherry pick of #245: fix: always requeue with the latest object

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 3, 2022

k8s-ci-robot requested review from ArangoGutierrez and denkensk May 3, 2022 08:52

kerthcet marked this pull request as draft May 3, 2022 08:52

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 3, 2022

kerthcet changed the title ~~[WIP][DO-NOT-REVIEW]fix: update failure in requeue~~ fix: update failure in requeue May 3, 2022

kerthcet marked this pull request as ready for review May 3, 2022 10:57

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 3, 2022

kerthcet mentioned this pull request May 3, 2022

A workload may not be retried if setting the Admission field fails #241

Closed

kerthcet force-pushed the bug/udpate-failure-in-requeue branch from dba9d51 to 542f8fa Compare May 3, 2022 11:12

alculquicondor reviewed May 3, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2022

kerthcet force-pushed the bug/udpate-failure-in-requeue branch from 542f8fa to 16bdd92 Compare May 5, 2022 07:51

k8s-ci-robot added 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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 5, 2022

alculquicondor reviewed May 5, 2022

View reviewed changes

pkg/queue/manager.go Outdated Show resolved Hide resolved

pkg/queue/manager_test.go Outdated Show resolved Hide resolved

alculquicondor reviewed May 5, 2022

View reviewed changes

pkg/queue/manager.go Show resolved Hide resolved

pkg/queue/manager_test.go Outdated Show resolved Hide resolved

alculquicondor reviewed May 6, 2022

View reviewed changes

pkg/queue/manager.go Show resolved Hide resolved

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2022

fix: always requeue with the latest object

2d4812b

Signed-off-by: kerthcet <[email protected]>

kerthcet force-pushed the bug/udpate-failure-in-requeue branch from ea8535b to 2d4812b Compare May 6, 2022 13:59

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2022

alculquicondor reviewed May 6, 2022

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2022

k8s-ci-robot assigned alculquicondor May 6, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2022

k8s-ci-robot merged commit b515c85 into kubernetes-sigs:main May 6, 2022

kerthcet deleted the bug/udpate-failure-in-requeue branch May 6, 2022 16:17

This was referenced Jun 9, 2022

Release v0.1.1 #272

Closed

Automated cherry pick of #245: fix: always requeue with the latest object #274

Merged

k8s-ci-robot added a commit that referenced this pull request Jun 9, 2022

Merge pull request #274 from kerthcet/automated-cherry-pick-of-#245-u…

3b6f8df

…pstream-release-0.1 Automated cherry pick of #245: fix: always requeue with the latest object

alculquicondor mentioned this pull request Nov 23, 2022

☂️ Requirements for release 0.3 #360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update failure in requeue #245

fix: update failure in requeue #245

kerthcet commented May 3, 2022 •

edited

Loading

kerthcet commented May 3, 2022

kerthcet commented May 3, 2022

kerthcet commented May 3, 2022

kerthcet commented May 3, 2022

alculquicondor May 3, 2022

kerthcet May 3, 2022

alculquicondor May 3, 2022

kerthcet May 3, 2022

kerthcet May 3, 2022

alculquicondor May 3, 2022

alculquicondor left a comment

alculquicondor May 3, 2022

alculquicondor May 3, 2022

kerthcet May 5, 2022

kerthcet commented May 5, 2022

alculquicondor left a comment

alculquicondor commented May 6, 2022

k8s-ci-robot commented May 6, 2022

kerthcet commented May 6, 2022

alculquicondor commented May 6, 2022

kerthcet commented May 6, 2022 •

edited

Loading

alculquicondor left a comment

kerthcet commented May 6, 2022

fix: update failure in requeue #245

fix: update failure in requeue #245

Conversation

kerthcet commented May 3, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

kerthcet commented May 3, 2022

kerthcet commented May 3, 2022

kerthcet commented May 3, 2022

kerthcet commented May 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet commented May 5, 2022

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor commented May 6, 2022

k8s-ci-robot commented May 6, 2022

kerthcet commented May 6, 2022

alculquicondor commented May 6, 2022

kerthcet commented May 6, 2022 • edited Loading

alculquicondor left a comment

Choose a reason for hiding this comment

kerthcet commented May 6, 2022

kerthcet commented May 3, 2022 •

edited

Loading

kerthcet commented May 6, 2022 •

edited

Loading