Ignore reconciling against unmanaged child jobs in the jobframework #821

tenzen-y · 2023-05-29T21:15:44Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

I fixed a bug that reconciles against unmanaged child batch/job.

Which issue(s) this PR fixes:

Fixes #800

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a bug that reconciles against unmanaged child batch/job.

netlify · 2023-05-29T21:15:50Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`658457e`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/647f99c868a7df000824aebe

tenzen-y · 2023-05-29T21:18:06Z

/assign @alculquicondor

pkg/controller/jobframework/reconciler.go

pkg/controller/jobframework/interface.go

test/integration/controller/job/job_controller_test.go

tenzen-y · 2023-05-31T05:16:17Z

test/integration/controller/job/job_controller_test.go

@@ -464,100 +461,6 @@ var _ = ginkgo.Describe("Job controller for workloads with no queue set", func()
 			return k8sClient.Get(ctx, wlLookupKey, createdWorkload)
 		}, util.Timeout, util.Interval).Should(gomega.Succeed())
 	})
-	ginkgo.When("The parent-workload annotation is used", func() {


@alculquicondor We can not run integration test cases related to parent-workload annotation since we must create a known workload owner (currently, only mpijobs) as a parent job.

tenzen-y · 2023-05-31T06:20:06Z

Maybe, the capacity of memory is insufficient :(

...
go fmt ./...
go vet ./...
golang.org/x/net/http2: /usr/local/go/pkg/tool/linux_amd64/compile: signal: killed
github.com/prometheus/procfs: /usr/local/go/pkg/tool/linux_amd64/compile: signal: killed
make: *** [Makefile:144: vet] Error 1

tenzen-y · 2023-05-31T11:46:47Z

/retest

pkg/controller/jobframework/reconciler.go

alculquicondor · 2023-05-31T12:07:58Z

test/integration/controller/mpijob/mpijob_controller_test.go

+	})
+
+	ginkgo.It("Should not suspend a child job if the parent job doesn't have a queue name", func() {
+		ginkgo.By("Creating the child job without ownerReference which uses the parent workload annotation")


🤔 maybe we should not allow a job to have the parent workload annotation if it doesn't have an owner reference?

That makes sense. Actually, we verify the ownerReference at a defaulting webhook:

kueue/pkg/controller/jobs/job/job_webhook.go

Line 63 in f694062

if owner := metav1.GetControllerOf(job); owner != nil && jobframework.KnownWorkloadOwner(owner) {

However, we don't have validation webhooks for that.

I'm thinking of adding a validation at a follow-up PR 🤔

Either way, I will move this case to a unit test.

test/integration/controller/mpijob/mpijob_controller_test.go

test/integration/controller/mpijob/suite_test.go

tenzen-y · 2023-06-01T16:38:45Z

pkg/controller/constants/constants.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package jobframework
+package constants


Because of avoiding the import cycle.

tenzen-y · 2023-06-01T16:59:16Z

Rebased to resolve conflicts.

tenzen-y · 2023-06-01T17:02:35Z

@alculquicondor I've addressed your suggestions. Can you please take another look?

alculquicondor · 2023-06-01T20:24:02Z

pkg/cache/cache_test.go

@@ -1293,7 +1293,7 @@ func TestCacheWorkloadOperations(t *testing.T) {
 			}

 			gotError := step.operation(cache)
-			if diff := cmp.Diff(step.wantError, messageOrEmpty(gotError)); diff != "" {
+			if diff := cmp.Diff(step.wantError, utiltesting.MessageOrEmpty(gotError)); diff != "" {


This escaped my review lens in the past 🧐

I'm not really a fan of checking errors by comparing strings. We should do the effort of implementing Is or exporting the relevant errors in the packages, so that we cmp.Diff can handle the rest.

But it can be left for a follow up.

I see. I will work on cache pkg at follow-up PRs.

pkg/controller/jobframework/reconciler_test.go

pkg/controller/jobframework/reconciler.go

pkg/util/testing/units.go

tenzen-y · 2023-06-02T21:12:47Z

@alculquicondor I addressed your comments. PTAL

pkg/controller/jobframework/reconciler_test.go

pkg/controller/jobframework/reconciler.go

pkg/controller/jobframework/reconciler_test.go

tenzen-y · 2023-06-06T20:19:59Z

@alculquicondor I'm sorry I didn't immediately understand your suggestion correctly.
I updated this PR. Can you re-check this?

alculquicondor

/approve

A couple of nits

pkg/controller/jobframework/reconciler.go

pkg/controller/jobframework/reconciler_test.go

k8s-ci-robot · 2023-06-06T20:22:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, tenzen-y

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:

~~pkg/controller/OWNERS~~ [alculquicondor]
~~pkg/util/OWNERS~~ [alculquicondor,tenzen-y]
~~test/OWNERS~~ [alculquicondor,tenzen-y]

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: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y · 2023-06-06T20:42:32Z

@alculquicondor I fixed all nits and squashed all commit into one.

alculquicondor · 2023-06-07T12:00:32Z

Structured logging is typically output as JSON, so camelCase is preferred.

tenzen-y · 2023-06-07T16:29:38Z

Structured logging is typically output as JSON, so camelCase is preferred.

I see. That makes sense.

k8s-ci-robot · 2023-06-07T16:29:46Z

PR needs rebase.

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/test-infra repository.

tenzen-y · 2023-06-07T17:50:20Z

I have rebased and addressed all suggestions.

tenzen-y · 2023-06-07T17:52:20Z

GitHub is under disruption...
https://www.githubstatus.com/

alculquicondor · 2023-06-07T17:57:31Z

rebase needed 😅

tenzen-y · 2023-06-07T18:00:04Z

rebase needed 😅

Actually, I rebased and pushed the commit... But due to GitHub disruption, this PR isn'n updated...

$ git log --one-line
da57d98 (HEAD -> framework-reconciler, origin/framework-reconciler) Use camelCase for logs
4017e3e Ignore reconciling against unmanaged child jobs in the jobframework
97639b9 (origin/main, origin/HEAD) Merge pull request #831 from epam/kustomize-deprecated-fields
68825ad Merge pull request #833 from cpanato/migrate
7444b27 Partial admission (#771)
5f02e23 Migrate away from google.com gcp project k8s-testimages

tenzen-y@da57d98

tenzen-y · 2023-06-07T18:01:17Z

/reopen

k8s-ci-robot · 2023-06-07T18:01:22Z

@tenzen-y: Failed to re-open PR: state cannot be changed. The framework-reconciler branch was force-pushed or recreated.

In response to this:

/reopen

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/test-infra repository.

tenzen-y · 2023-06-07T18:02:56Z

This PR was broken :(

tenzen-y · 2023-06-07T18:07:44Z

/reopen

k8s-ci-robot · 2023-06-07T18:07:49Z

@tenzen-y: Failed to re-open PR: state cannot be changed. The framework-reconciler branch was force-pushed or recreated.

In response to this:

/reopen

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/test-infra repository.

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 29, 2023

k8s-ci-robot requested review from alculquicondor and ArangoGutierrez May 29, 2023 21:15

k8s-ci-robot assigned alculquicondor May 29, 2023

tenzen-y changed the title ~~Ignore recociling against unmanaged child jobs in the jobframework~~ Ignore reconciling against unmanaged child jobs in the jobframework May 29, 2023

tenzen-y force-pushed the framework-reconciler branch from 3686019 to e72bdf8 Compare May 29, 2023 21:26

alculquicondor reviewed May 30, 2023

View reviewed changes

tenzen-y force-pushed the framework-reconciler branch from 8b61efc to 432f9d2 Compare May 31, 2023 03:15

tenzen-y commented May 31, 2023

View reviewed changes

tenzen-y force-pushed the framework-reconciler branch from f815440 to a52c9fc Compare May 31, 2023 06:53

tenzen-y mentioned this pull request May 31, 2023

Kueue: Increase memory limit for compilation kubernetes/test-infra#29646

Merged

alculquicondor reviewed May 31, 2023

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 31, 2023

tenzen-y force-pushed the framework-reconciler branch 2 times, most recently from 76e315f to bb1ea47 Compare June 1, 2023 16:37

tenzen-y commented Jun 1, 2023

View reviewed changes

tenzen-y force-pushed the framework-reconciler branch from 3d21608 to 3d000da Compare June 1, 2023 16:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 1, 2023

alculquicondor reviewed Jun 1, 2023

View reviewed changes

tenzen-y force-pushed the framework-reconciler branch from 5443584 to b3972f3 Compare June 2, 2023 21:11

alculquicondor reviewed Jun 5, 2023

View reviewed changes

pkg/controller/jobframework/reconciler_test.go Outdated Show resolved Hide resolved

alculquicondor reviewed Jun 5, 2023

View reviewed changes

pkg/controller/jobframework/reconciler.go Outdated Show resolved Hide resolved

tenzen-y commented Jun 6, 2023

View reviewed changes

pkg/controller/jobframework/reconciler_test.go Outdated Show resolved Hide resolved

tenzen-y force-pushed the framework-reconciler branch 2 times, most recently from c24c743 to 12122e1 Compare June 6, 2023 20:16

alculquicondor reviewed Jun 6, 2023

View reviewed changes

pkg/controller/jobframework/reconciler.go Outdated Show resolved Hide resolved

pkg/controller/jobframework/reconciler.go Outdated Show resolved Hide resolved

pkg/controller/jobframework/reconciler_test.go Outdated 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 Jun 6, 2023

Ignore reconciling against unmanaged child jobs in the jobframework

658457e

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y force-pushed the framework-reconciler branch from 12122e1 to 658457e Compare June 6, 2023 20:40

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

tenzen-y closed this Jun 7, 2023

tenzen-y mentioned this pull request Jun 7, 2023

Ignore reconciling against unmanaged child jobs in the jobframework #835

Merged

tenzen-y mentioned this pull request Jun 17, 2023

Add validation for child jobs without ownerReference #865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore reconciling against unmanaged child jobs in the jobframework #821

Ignore reconciling against unmanaged child jobs in the jobframework #821

tenzen-y commented May 29, 2023 •

edited

Loading

netlify bot commented May 29, 2023 •

edited

Loading

tenzen-y commented May 29, 2023

tenzen-y May 31, 2023

tenzen-y commented May 31, 2023

tenzen-y commented May 31, 2023

alculquicondor May 31, 2023

tenzen-y May 31, 2023

tenzen-y May 31, 2023 •

edited

Loading

tenzen-y Jun 1, 2023

tenzen-y commented Jun 1, 2023 •

edited

Loading

tenzen-y commented Jun 1, 2023

alculquicondor Jun 1, 2023

tenzen-y Jun 1, 2023

tenzen-y commented Jun 2, 2023

tenzen-y commented Jun 6, 2023

alculquicondor left a comment

k8s-ci-robot commented Jun 6, 2023

tenzen-y commented Jun 6, 2023

alculquicondor commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

k8s-ci-robot commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

alculquicondor commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

k8s-ci-robot commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

k8s-ci-robot commented Jun 7, 2023

Ignore reconciling against unmanaged child jobs in the jobframework #821

Ignore reconciling against unmanaged child jobs in the jobframework #821

Conversation

tenzen-y commented May 29, 2023 • 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:

Does this PR introduce a user-facing change?

netlify bot commented May 29, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

tenzen-y commented May 29, 2023

tenzen-y May 31, 2023

Choose a reason for hiding this comment

tenzen-y commented May 31, 2023

tenzen-y commented May 31, 2023

alculquicondor May 31, 2023

Choose a reason for hiding this comment

tenzen-y May 31, 2023

Choose a reason for hiding this comment

tenzen-y May 31, 2023 • edited Loading

Choose a reason for hiding this comment

tenzen-y Jun 1, 2023

Choose a reason for hiding this comment

tenzen-y commented Jun 1, 2023 • edited Loading

tenzen-y commented Jun 1, 2023

alculquicondor Jun 1, 2023

Choose a reason for hiding this comment

tenzen-y Jun 1, 2023

Choose a reason for hiding this comment

tenzen-y commented Jun 2, 2023

tenzen-y commented Jun 6, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 6, 2023

tenzen-y commented Jun 6, 2023

alculquicondor commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

k8s-ci-robot commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

alculquicondor commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

k8s-ci-robot commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

tenzen-y commented Jun 7, 2023

k8s-ci-robot commented Jun 7, 2023

tenzen-y commented May 29, 2023 •

edited

Loading

netlify bot commented May 29, 2023 •

edited

Loading

tenzen-y May 31, 2023 •

edited

Loading

tenzen-y commented Jun 1, 2023 •

edited

Loading