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 cleanpod policy for v1alpha2 #691

Merged
merged 7 commits into from
Jun 21, 2018
Merged

*: Add cleanpod policy for v1alpha2 #691

merged 7 commits into from
Jun 21, 2018

Conversation

gaocegege
Copy link
Member

@gaocegege gaocegege commented Jun 21, 2018 •
edited by jlewi
Loading

/cc @yph152 @jlewi

  • Support cleanpod policy
  • Import code-generator to vendor

This change is Reviewable

@gaocegege
*: Support cleanpod policy
2e292cb 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
Gopkg: Keep code-generator
d373efa 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
vendor: Update
4e5b134 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@k8s-ci-robot
Copy link

@gaocegege: GitHub didn't allow me to request PR reviews from the following users: yph152.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @yph152 @jlewi

  • Support cleanpod policy
  • Import code-generator to vendor

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 k8s-ci-robot requested a review from jlewi June 21, 2018 07:55
@gaocegege
test: Fix test cases
a27fcf2 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
test: Fix
3ce90c0 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@kubeflow kubeflow deleted a comment from k8s-ci-robot Jun 21, 2018
@kubeflow kubeflow deleted a comment from TravisBuddy Jun 21, 2018
@kubeflow kubeflow deleted a comment from TravisBuddy Jun 21, 2018
@gaocegege
test: Fix style
a9af1e1 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@TravisBuddy
Copy link

Travis tests have failed

Hey @gaocegege,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

gometalinter --config=linter_config.json --vendor ./... 
pkg/controller.v2/controller_tfjob_test.go:325:6:warning: should omit comparison to bool constant, can be simplified to !forget (S1002) (gosimple)

3rd Build

gometalinter --config=linter_config.json --vendor ./... 
pkg/controller.v2/controller_tfjob_test.go:325:6:warning: should omit comparison to bool constant, can be simplified to !forget (S1002) (gosimple)

travis_time:end:0362f787:start=1529569869868124090,finish=1529569992606160643,duration=122738036553

@coveralls
Copy link

coveralls commented Jun 21, 2018 •
edited
Loading

Coverage Status

Coverage increased (+1.06%) to 59.078% when pulling 4d870e2 on gaocegege:cleanup into 3d9afd1 on kubeflow:master.

yph152
yph152 reviewed Jun 21, 2018
View reviewed changes
pkg/apis/tensorflow/v1alpha2/types.go
@@ -70,6 +75,16 @@ type TFReplicaSpec struct {
RestartPolicy RestartPolicy `json:"restartPolicy,omitempty"`
}

// CleanPodPolicy describes how to deal with pods when the TFJob is finished.
Copy link
Contributor

Choose a reason for hiding this comment

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

TFJob is finished that include succeeded and failed ?

Copy link
Member Author

@gaocegege gaocegege Jun 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, restarting is the internal state, thus succeeded and failed are the states to indicate that the TFJob is terminated.

This was referenced Jun 21, 2018
Closed
Closed
@gaocegege
Copy link
Member Author

/assign @jlewi

jlewi
jlewi reviewed Jun 21, 2018
View reviewed changes
Copy link
Contributor

@jlewi jlewi left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

pkg/apis/tensorflow/v1alpha2/defaults.go
@@ -88,9 +88,19 @@ func setTypeNameToCamelCase(tfJob *TFJob, typ TFReplicaType) {

// SetDefaults_TFJob sets any unspecified values to defaults.
func SetDefaults_TFJob(tfjob *TFJob) {
// Set default cleanpod policy to All.
if tfjob.Spec.CleanPodPolicy == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a more sensible default would be to only delete running pods. That seems like the most useful policy because it preserves logs and doesn't consume resources.

Its also consistent with behavior of K8s job.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we will delete ps and keep workers, make sense to me. But maybe look weird. I will implement it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I agree its weird but the logs of the workers and the chief are probably what you care about anyway and this way users will have logs available by default.

@jlewi jlewi mentioned this pull request Jun 21, 2018
Closed
@gaocegege
defaults: Update
4d870e2 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
Copy link
Member Author

PTAL @jlewi

@jlewi
Copy link
Contributor

jlewi commented Jun 21, 2018

Thanks!

/lgtm
/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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

@jlewi
Copy link
Contributor

jlewi commented Jun 21, 2018

/test all

@k8s-ci-robot k8s-ci-robot merged commit 38b886a into kubeflow:master Jun 21, 2018
@gaocegege
Copy link
Member Author

/retest

@gaocegege gaocegege mentioned this pull request Jul 4, 2018
Closed
jetmuffin pushed a commit to jetmuffin/tf-operator that referenced this pull request Jul 9, 2018
@gaocegege @k8s-ci-robot
*: Add cleanpod policy for v1alpha2 (kubeflow#691)
7a0f4dc 
* *: Support cleanpod policy

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* Gopkg: Keep code-generator

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* vendor: Update

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* test: Fix test cases

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* test: Fix

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* test: Fix style

Signed-off-by: Ce Gao <gaoce@caicloud.io>

* defaults: Update

Signed-off-by: Ce Gao <gaoce@caicloud.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yph152 yph152 yph152 left review comments

@jlewi jlewi jlewi left review comments

Assignees

@jlewi jlewi

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

Successfully merging this pull request may close these issues.
6 participants
@gaocegege @k8s-ci-robot @TravisBuddy @coveralls @jlewi @yph152