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.

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

fix the bug of keeping creating new pdb #539

Merged
merged 2 commits into from
Apr 17, 2018
Merged

fix the bug of keeping creating new pdb #539

merged 2 commits into from
Apr 17, 2018

Conversation

ChanYiLin
Copy link
Member

@ChanYiLin ChanYiLin commented Apr 16, 2018

The original code will let the controller keep creating new pdb for the job since the names are not the same.
They only have the same prefix which is GenerateName: "tf-job-pdb-".

I try to fix it by assigning the name, "tf-job-pdb-" + j.job.ObjectMeta.Name, to the pdb in order to make controller only create pdb once.


This change is Reviewable

@gaocegege
Copy link
Member

Thanks for your contribution!

@k82cn Do we support owner reference based gang scheduling in kube-arbitrator? If that we could eliminate the hack about PDB.

@coveralls
Copy link

coveralls commented Apr 16, 2018

Coverage Status

Coverage remained the same at 48.307% when pulling d1e5f12 on ChanYiLin:master into fd95c5e on kubeflow:master.

@gaocegege
Copy link
Member

/ok-to-test

Please fix the error in Travis CI.

@ChanYiLin
Copy link
Member Author

ChanYiLin commented Apr 16, 2018

I think it's ok now.
/assign @gaocegege

@gaocegege
Copy link
Member

gaocegege commented Apr 16, 2018

I do not think so. It seems that the Travis reported that the import order is not followed. Our checker is weird and it returns some errors which is not caused by your code. Let us see if it works well 😄

@gaocegege
Copy link
Member

I looked through the code in kube-arbitrator. It seems that the feature owner reference based gang scheduling in kube-arbitrator is not implemented, thus we keep PDB now. The code lgtm

/cc @mitake

Would you please take a look?

@k82cn
Copy link
Collaborator

k82cn commented Apr 17, 2018

PDB is still necessary to define the minAvailable for gang-scheduling; we need waiting the new API in upstream (we're working on that).

@gaocegege
Copy link
Member

It seems that mitake is not around.

/lgtm

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege

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

@k8s-ci-robot k8s-ci-robot merged commit 846fd38 into kubeflow:master Apr 17, 2018
@mitake
Copy link
Contributor

mitake commented Apr 17, 2018

@gaocegege sorry for being late, this PR looks good to me.
@ChanYiLin thanks a lot for the fix!

@gaocegege
Copy link
Member

@mitake haha No worry I just want to double check it.

@ChanYiLin
Copy link
Member Author

Really thanks for your help !

yph152 pushed a commit to yph152/tf-operator that referenced this pull request Jun 18, 2018
* fix the bug of keeping creating new pdb

* fix the error in Travis CI
jetmuffin pushed a commit to jetmuffin/tf-operator that referenced this pull request Jul 9, 2018
* fix the bug of keeping creating new pdb

* fix the error in Travis CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants