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.

Sign up for GitHub

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

Jump to bottom

Add PodGroup as controller watch source #1666

Merged
merged 4 commits into from
Oct 20, 2022
Merged

Add PodGroup as controller watch source #1666

merged 4 commits into from
Oct 20, 2022

Conversation

ggaaooppeenngg
Copy link
Contributor

The common.ReconcileJobs stops creating pods when the related PodGroup is unschedulable. When the PodGroup becomes schedulable, the reconcile loop can not be triggered because of no watch source for the PodGroup.

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

@gaocegege
Copy link
Member

I used to thought it should be already fixed.

/lgtm

/cc @zw0610

@google-oss-prow google-oss-prow bot requested a review from zw0610 September 19, 2022 09:04
@google-oss-prow google-oss-prow bot added lgtm and removed lgtm labels Sep 19, 2022
shinytang6
shinytang6 suggested changes Sep 19, 2022
View reviewed changes
Copy link
Member

@shinytang6 shinytang6 left a comment

Choose a reason for hiding this comment

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

Maybe other job controllers(pytorch/tf/...) also need this?

@shinytang6 shinytang6 mentioned this pull request Sep 19, 2022
Closed
@google-oss-prow google-oss-prow bot added size/M and removed size/S labels Sep 20, 2022
@ggaaooppeenngg
Copy link
Contributor Author

There seems an error. How to install the PodGroup CRD in the test environment?

@google-oss-prow google-oss-prow bot added size/L and removed size/M labels Sep 28, 2022
@coveralls
Copy link

coveralls commented Sep 28, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 3271153965

  • 9 of 66 (13.64%) changed or added relevant lines in 5 files are covered.
  • 12 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.4%) to 39.527%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controller.v1/mpi/mpijob_controller.go 3 10 30.0%
pkg/controller.v1/pytorch/pytorchjob_controller.go 3 14 21.43%
pkg/controller.v1/tensorflow/tfjob_controller.go 3 14 21.43%
pkg/controller.v1/mxnet/mxjob_controller.go 0 14 0.0%
pkg/controller.v1/xgboost/xgboostjob_controller.go 0 14 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/controller.v1/mxnet/mxjob_controller.go 1 0%
pkg/controller.v1/mpi/mpijob_controller.go 2 77.39%
pkg/controller.v1/pytorch/pytorchjob_controller.go 9 60.32%
Totals Coverage Status
Change from base Build 3132871820: -0.4%
Covered Lines: 2340
Relevant Lines: 5920
💛 - Coveralls

@ggaaooppeenngg
Add PodGroup as controller watch source
09d0919 
The common.ReconcileJobs stops creating pods when the related
PodGroup is unschedulable. When the PodGroup becomes schedulable,
the reconcile loop can not be triggered because of no watch source
for the PodGroup.

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

You can try to invoke this script locally for debugging

https://github.com/kubeflow/training-operator/blob/master/.github/workflows/integration-tests.yaml#L39

@ggaaooppeenngg
Fix the no PodGroup kind error
307622b 
Continue without watching the PodGroup is the PodGroup
is not installed.

Signed-off-by: Peng Gao <peng.gao.dut@gmail.com>
@ggaaooppeenngg
Remove the PodGroup crd
0d87c77 
Signed-off-by: Peng Gao <peng.gao.dut@gmail.com>
@ggaaooppeenngg
Copy link
Contributor Author

It's not easy to manage an external crd. I skip the watching if the crd is not found.

@ggaaooppeenngg ggaaooppeenngg requested review from shinytang6 and removed request for terrytangyuan, gaocegege, jinchihe and zw0610 October 17, 2022 09:01
@ggaaooppeenngg
Copy link
Contributor Author

Need a review @shinytang6

shinytang6
shinytang6 reviewed Oct 18, 2022
View reviewed changes
Copy link
Member

@shinytang6 shinytang6 left a comment

Choose a reason for hiding this comment

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

Thanks, these changes are ok to me.

pkg/controller.v1/tensorflow/tfjob_controller.go Outdated
_, err = mgr.GetRESTMapper().RESTMapping(schema.GroupKind{Group: v1beta1.SchemeGroupVersion.Group, Kind: "PodGroup"},
v1beta1.SchemeGroupVersion.Version)
if err == nil {

Copy link
Member

Choose a reason for hiding this comment

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

extra blank line

pkg/controller.v1/xgboost/xgboostjob_controller.go Outdated
_, err = mgr.GetRESTMapper().RESTMapping(schema.GroupKind{Group: v1beta1.SchemeGroupVersion.Group, Kind: "PodGroup"},
v1beta1.SchemeGroupVersion.Version)
if err == nil {

Copy link
Member

Choose a reason for hiding this comment

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

ditto

@ggaaooppeenngg
Remove extra blank lines
87f19a7 
Signed-off-by: Peng Gao <peng.gao.dut@gmail.com>
@ggaaooppeenngg
Copy link
Contributor Author

done @shinytang6
/assign @gaocegege

@johnugeorge
Copy link
Member

/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggaaooppeenngg, johnugeorge

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

@johnugeorge
Copy link
Member

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 20, 2022
@google-oss-prow google-oss-prow bot merged commit ab9f3ec into kubeflow:master Oct 20, 2022
djwhatle pushed a commit to djwhatle/training-operator that referenced this pull request Oct 27, 2022
@ggaaooppeenngg
Add PodGroup as controller watch source (kubeflow#1666)
0a0980a 
* Add PodGroup as controller watch source

The common.ReconcileJobs stops creating pods when the related
PodGroup is unschedulable. When the PodGroup becomes schedulable,
the reconcile loop can not be triggered because of no watch source
for the PodGroup.

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

* Fix the no PodGroup kind error

Continue without watching the PodGroup is the PodGroup
is not installed.

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

* Remove the PodGroup crd

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

* Remove extra blank lines

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

Signed-off-by: Peng Gao <peng.gao.dut@gmail.com>
(cherry picked from commit ab9f3ec)
@Crazybean-lwb
Copy link

panic happend

E1031 12:09:36.449193 1 runtime.go:79] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
goroutine 549 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x185a700?, 0x2a0c1a0})
/go/pkg/mod/k8s.io/apimachinery@v0.24.1/pkg/util/runtime/runtime.go:75 +0x99
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0xc000148028?})
/go/pkg/mod/k8s.io/apimachinery@v0.24.1/pkg/util/runtime/runtime.go:49 +0x75

@johnugeorge
Copy link
Member

Can you create a separate issue with more details?

@Crazybean-lwb Crazybean-lwb mentioned this pull request Nov 1, 2022
Closed
@johnugeorge johnugeorge mentioned this pull request Nov 2, 2022
Closed
@ggaaooppeenngg ggaaooppeenngg mentioned this pull request Nov 4, 2022
Merged
1 task
@johnugeorge johnugeorge mentioned this pull request Nov 4, 2022
Closed
@johnugeorge johnugeorge mentioned this pull request Jan 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shinytang6 shinytang6 shinytang6 requested changes

Assignees

@johnugeorge johnugeorge

@gaocegege gaocegege

Labels
approved lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ggaaooppeenngg @gaocegege @coveralls @johnugeorge @Crazybean-lwb @shinytang6