Add warn event and directly return without creating pods for job validation failure #1564

cheimu · 2022-03-24T10:00:09Z

What this PR does / why we need it:
Currently, all jobs' controllers validate job spec but only log an error in training-operator, but

Users don't know they are not correct because pods are running (such as tfjob wrong container name case)
Creating pod or Running pod will reserve node resources, while they don't behave correctly. Wasting resources.

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #1563

Checklist:

Docs included if any changes are user facing

aws-kf-ci-bot · 2022-03-24T10:00:24Z

Hi @cheimu. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

coveralls · 2022-03-24T10:07:04Z

Pull Request Test Coverage Report for Build 2039519742

5 of 9 (55.56%) changed or added relevant lines in 5 files are covered.
4 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.2%) to 37.061%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controller.v1/mxnet/mxjob_controller.go	0	2	0.0%
pkg/controller.v1/xgboost/xgboostjob_controller.go	0	2	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/controller.v1/mxnet/mxjob_controller.go	1	0%
pkg/controller.v1/xgboost/xgboostjob_controller.go	1	0%
pkg/controller.v1/mpi/mpijob_controller.go	2	75.66%

Totals
Change from base Build 2014597889:	0.2%
Covered Lines:	2290
Relevant Lines:	6179

💛 - Coveralls

gaocegege · 2022-03-25T01:32:17Z

/ok-to-test

/assign @zw0610

pkg/controller.v1/mxnet/mxjob_controller.go

google-oss-prow · 2022-03-25T01:43:29Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cheimu
To complete the pull request process, please ask for approval from zw0610 after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

OWNERS

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

zw0610 · 2022-03-25T02:02:07Z

Generally LGTM. It would be better if test cases are included as well.

cheimu · 2022-03-25T02:06:35Z

Generally LGTM. It would be better if test cases are included as well.

Oh yeah, you are right, let me add tests

cheimu · 2022-03-25T06:40:07Z

/retest
kubeflow-training-operator-presubmit error is:
kubernetes.client.exceptions.ApiException: (401)
Reason: Unauthorized
Seems like flaky

pkg/controller.v1/mxnet/mxjob_controller.go

cheimu · 2022-03-25T10:40:52Z

Generally LGTM. It would be better if test cases are included as well.

Done

cheimu · 2022-03-25T13:08:13Z

/retest kubeflow-training-operator-presubmit error is: kubernetes.client.exceptions.ApiException: (401) Reason: Unauthorized Seems like flaky

/retest

same reason.

cheimu · 2022-03-25T15:40:32Z

/retest

…thub.com/cheimu/training-operator into fix/tfjob-add-warn-for-failed-validation

aws-kf-ci-bot · 2022-05-05T06:49:44Z

@cheimu: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
kubeflow-training-operator-presubmit	`bcbf9ba`	link	`/test kubeflow-training-operator-presubmit`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Signed-off-by: Peng Gao <peng.gao.dut@gmail.com>

* Deprecate training-operator presubmit on optional-test-infra This PR serves as sub-PR to deprecate training-operator presubmit on optional-test-infra. * Update config file Update workflow format

* Adding latest image tag * Update manifests with latest image tag * Adding integration tests * Change trigger type

Currently certain operations like tailing logs from the Python sdk against the latest version of the operator due to a label mistmatch; fix that. Closes kubeflow#1587.

* Adding latest image tag * Update manifests with latest image tag * Update k8s dependencies to v0.24.1 * Update manifests * Update k8s matrix for integration tests * Update k8s matrix for unit tests * Fix k8s versions * Fix version * Add scripts in separate file * Fix Makefile * Cleanup Makefile * Addressing review comments

zw0610 · 2022-06-06T08:09:34Z

/retest

zw0610 · 2022-06-06T08:22:35Z

@johnugeorge Could you advise how to handle stalled pull requests because of the failed ci like this one after the migration to GHA?

johnugeorge · 2022-06-06T09:09:46Z

it should trigger new GHA workflow on a rebase

@cheimu Can you rebase?

…thub.com/cheimu/training-operator into fix/tfjob-add-warn-for-failed-validation

cheimu · 2022-06-07T12:57:45Z

:( sad for myself for a second. I'll open a new pr and close this one later...

johnugeorge · 2022-06-08T04:24:52Z

@cheimu can you do it at the earliest as we are planning to have feature release by end of next week?

cheimu · 2022-06-13T02:41:37Z

@cheimu can you do it at the earliest as we are planning to have feature release by end of next week?

Hi @johnugeorge , I'm afraid I can't make it this week... I'll try my best though

johnugeorge · 2023-01-21T20:02:02Z

Closing it in favour of #1704
/close

google-oss-prow · 2023-01-21T20:02:05Z

@johnugeorge: Closed this PR.

In response to this:

Closing it in favour of #1704
/close

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.

Add warn event and return for invalid job spec

0465631

google-oss-prow bot added the size/S label Mar 24, 2022

aws-kf-ci-bot added the needs-ok-to-test label Mar 24, 2022

google-oss-prow bot requested review from jinchihe, johnugeorge and terrytangyuan March 24, 2022 10:00

google-oss-prow bot assigned zw0610 Mar 25, 2022

aws-kf-ci-bot added ok-to-test and removed needs-ok-to-test labels Mar 25, 2022

gaocegege reviewed Mar 25, 2022

View reviewed changes

pkg/controller.v1/mxnet/mxjob_controller.go Outdated Show resolved Hide resolved

Update FailedValidateJobReason to follow the convention

3ca035c

johnugeorge reviewed Mar 25, 2022

View reviewed changes

pkg/controller.v1/mxnet/mxjob_controller.go Outdated Show resolved Hide resolved

add unit tests for mpi tf and pytorch

d8f2cd1

google-oss-prow bot added size/L and removed size/S labels Mar 25, 2022

cheimu mentioned this pull request Mar 25, 2022

Add cheimu to Kubeflow Member List kubeflow/internal-acls#541

Merged

cheimu added 4 commits March 30, 2022 12:09

Merge branch 'master' into fix/tfjob-add-warn-for-failed-validation

9039018

adapt constants name to follow the convention

e8d8540

Merge branch 'fix/tfjob-add-warn-for-failed-validation' of https://gi…

8a2bc6a

…thub.com/cheimu/training-operator into fix/tfjob-add-warn-for-failed-validation

fix conflicts

a87ed14

cheimu added 2 commits May 1, 2022 01:44

Remove uncalled mpi-controller DeletePodsAndServices() (kubeflow#1558)

ce82602

refactor the unit tests for all jobs

bcbf9ba

zw0610 and others added 7 commits May 12, 2022 11:20

update img url in design doc (kubeflow#1591)

af3cfc2

chore: stop action on first fail (kubeflow#1595)

0ad86c4

Add mpi in update-codegen.sh (kubeflow#1600)

a7493ab

Signed-off-by: Peng Gao <peng.gao.dut@gmail.com>

Remove presubmit test depending on optional-test-infra (kubeflow#1596)

291d26d

* Deprecate training-operator presubmit on optional-test-infra This PR serves as sub-PR to deprecate training-operator presubmit on optional-test-infra. * Update config file Update workflow format

Migrate test framework to GHA (kubeflow#1603)

0a32353

* Adding latest image tag * Update manifests with latest image tag * Adding integration tests * Change trigger type

Look for fully-qualified job role label in Python sdk (kubeflow#1588)

b971d8c

Currently certain operations like tailing logs from the Python sdk against the latest version of the operator due to a label mistmatch; fix that. Closes kubeflow#1587.

cheimu added 8 commits June 7, 2022 20:38

Add warn event and return for invalid job spec

f7dc29f

Update FailedValidateJobReason to follow the convention

07478f9

add unit tests for mpi tf and pytorch

e629a48

adapt constants name to follow the convention

2df25f7

fix conflicts

806cbc5

refactor the unit tests for all jobs

5aaf041

lint

863257f

Merge branch 'fix/tfjob-add-warn-for-failed-validation' of https://gi…

bfd0faf

…thub.com/cheimu/training-operator into fix/tfjob-add-warn-for-failed-validation

google-oss-prow bot added size/XXL and removed size/L labels Jun 7, 2022

henrysecond1 mentioned this pull request Jul 1, 2022

Defaulting before validation cause operator panic #1628

Closed

google-oss-prow bot closed this Jan 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warn event and directly return without creating pods for job validation failure #1564

Add warn event and directly return without creating pods for job validation failure #1564

cheimu commented Mar 24, 2022

aws-kf-ci-bot commented Mar 24, 2022

coveralls commented Mar 24, 2022 •

edited

Loading

gaocegege commented Mar 25, 2022

google-oss-prow bot commented Mar 25, 2022

zw0610 commented Mar 25, 2022

cheimu commented Mar 25, 2022

cheimu commented Mar 25, 2022

cheimu commented Mar 25, 2022

cheimu commented Mar 25, 2022

cheimu commented Mar 25, 2022

aws-kf-ci-bot commented May 5, 2022

zw0610 commented Jun 6, 2022

zw0610 commented Jun 6, 2022

johnugeorge commented Jun 6, 2022

cheimu commented Jun 7, 2022

johnugeorge commented Jun 8, 2022

cheimu commented Jun 13, 2022

johnugeorge commented Jan 21, 2023

google-oss-prow bot commented Jan 21, 2023

Add warn event and directly return without creating pods for job validation failure #1564

Add warn event and directly return without creating pods for job validation failure #1564

Conversation

cheimu commented Mar 24, 2022

aws-kf-ci-bot commented Mar 24, 2022

coveralls commented Mar 24, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2039519742

💛 - Coveralls

gaocegege commented Mar 25, 2022

google-oss-prow bot commented Mar 25, 2022

zw0610 commented Mar 25, 2022

cheimu commented Mar 25, 2022

cheimu commented Mar 25, 2022

cheimu commented Mar 25, 2022

cheimu commented Mar 25, 2022

cheimu commented Mar 25, 2022

aws-kf-ci-bot commented May 5, 2022

zw0610 commented Jun 6, 2022

zw0610 commented Jun 6, 2022

johnugeorge commented Jun 6, 2022

cheimu commented Jun 7, 2022

johnugeorge commented Jun 8, 2022

cheimu commented Jun 13, 2022

johnugeorge commented Jan 21, 2023

google-oss-prow bot commented Jan 21, 2023

coveralls commented Mar 24, 2022 •

edited

Loading