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 OwnerReferences to pdb #565

Merged
merged 6 commits into from
May 5, 2018
Merged

add OwnerReferences to pdb #565

merged 6 commits into from
May 5, 2018

Conversation

ChanYiLin
Copy link
Member

@ChanYiLin ChanYiLin commented Apr 30, 2018 •
edited by jlewi
Loading

Hi, after fixing the name of ObjectMeta of PDB.
I think we could add the OwnerReferences to PDB too.
This enable the Kubernetes GC to automatically delete the PDB when users delete the tfjob manually.
Thanks!

This change is Reviewable

@ChanYiLin
Copy link
Member Author

/assign @mitake

@coveralls
Copy link

coveralls commented Apr 30, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.4%) to 49.861% when pulling 415760e on ChanYiLin:master into 19d611c on kubeflow:master.

@gaocegege
Copy link
Member

/ok-to-test

@gaocegege
Copy link
Member

/lgtm

@ChanYiLin
Copy link
Member Author

ChanYiLin commented Apr 30, 2018 •
edited
Loading

@gaocegege Thanks!
/assign @gaocegege

@ChanYiLin
Copy link
Member Author

@gaocegege is this problem from my code?
The log says the problem is "run_e2e_workflow.py Time out waiting for Workflows"

@gaocegege
Copy link
Member

I will take a look soon. I am not sure about it.

@gaocegege
Copy link
Member

Sorry for the delay, could you please re-push your commit and force a new build for this PR? I think it is an accidental bug or caused by kubeflow/testing.

@ChanYiLin
Copy link
Member Author

ChanYiLin commented May 1, 2018 •
edited
Loading

Hi, @gaocegege ,
instead of just re-pushing my commit,
I have refactored the syncPdb function to make it follow the structures of syncPod and syncService. Please have a look at it.
Thanks!

@ChanYiLin
Copy link
Member Author

@gaocegege
The error log is still the same.
"run_e2e_workflow.py Time out waiting for Workflows"

@mitake
Copy link
Contributor

mitake commented May 1, 2018

@ChanYiLin thanks for your PR! I'll take a look tomorrow (I'm attending kubecon now so my review will be slow, sorry for that)

@gaocegege
Copy link
Member

I will take a look again, sorry for that. Some of our people are in Kubecon Europe 2018 thus they may have no time to help us.

@ChanYiLin
Copy link
Member Author

It's okay, no problem!
Thanks for reminding me there is Kubecon these days, I totally forget it.
I should have a look at the conference :-)

@gaocegege gaocegege mentioned this pull request May 2, 2018
Closed
@gaocegege
Copy link
Member

I think the CI is fixed, please rebase the master 😄

@gaocegege
Copy link
Member

It seems that GPU test failed, I will take a look tomorrow.

http://testing-argo.kubeflow.org/workflows/kubeflow-test-infra/kubeflow-tf-operator-presubmit-tfjob-e2e-565-415760e-373-8764?tab=workflow&nodeId=kubeflow-tf-operator-presubmit-tfjob-e2e-565-415760e-373-8764-1305880146&sidePanel=logs%3Akubeflow-tf-operator-presubmit-tfjob-e2e-565-415760e-373-8764-1305880146%3Amain

@ChanYiLin
Copy link
Member Author

ChanYiLin commented May 2, 2018 •
edited
Loading

Thanks for your help!

@gaocegege
Copy link
Member

/retest

2 similar comments
@gaocegege
Copy link
Member

/retest

@gaocegege
Copy link
Member

/retest

@gaocegege
Copy link
Member

/lgtm
/approve

@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

@ChanYiLin
Copy link
Member Author

/retest
Hi @gaocegege, Is there anything I can help?

@gaocegege
Copy link
Member

/retest

@gaocegege
Copy link
Member

@ChanYiLin

I am not sure what happened in the CI now, maybe you could have a look, too. The argo UI and CI dashboard is public.

@gaocegege
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit f317193 into kubeflow:master May 5, 2018
@gaocegege
Copy link
Member

I am not sure what happened to the CI, it passes sometimes while failures also happened sometimes..

@ChanYiLin
Copy link
Member Author

ChanYiLin commented May 5, 2018 •
edited
Loading

Anyway, it worked this time haha.
Thanks!

yph152 pushed a commit to yph152/tf-operator that referenced this pull request Jun 18, 2018
@ChanYiLin
add OwnerReferences to pdb (kubeflow#565)
c923cd1 
* add OwnerReferences to pdb

* refactor the syncPdb code

* fix the bug of access pdb name.

* add fake ObjectMeta uid in the testcase of TestPDBForGangScheduling

* fix error when no pdb should be created

* remove the fake uid in the testcase
jetmuffin pushed a commit to jetmuffin/tf-operator that referenced this pull request Jul 9, 2018
@ChanYiLin @k8s-ci-robot
add OwnerReferences to pdb (kubeflow#565)
56db83f 
* add OwnerReferences to pdb

* refactor the syncPdb code

* fix the bug of access pdb name.

* add fake ObjectMeta uid in the testcase of TestPDBForGangScheduling

* fix error when no pdb should be created

* remove the fake uid in the testcase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mitake mitake Awaiting requested review from mitake

@willb willb Awaiting requested review from willb

Assignees

@mitake mitake

@gaocegege gaocegege

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

Successfully merging this pull request may close these issues.
5 participants
@ChanYiLin @coveralls @gaocegege @mitake @k8s-ci-robot