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

pod: Submit an event when the user specifies the restartpolicy for pod template #648

Merged
merged 3 commits into from
Jun 14, 2018

Conversation

gaocegege
Copy link
Member

@gaocegege gaocegege commented Jun 13, 2018

  • Submit an event when the user specifies the restartpolicy for pod template.
  • Add ut for restart policy

/assign @jlewi

Signed-off-by: Ce Gao [email protected]


This change is Reviewable

@coveralls
Copy link

coveralls commented Jun 13, 2018

Coverage Status

Coverage increased (+0.1%) to 54.571% when pulling 12750ba on gaocegege:event into 510279f on kubeflow:master.

@gaocegege gaocegege changed the title event: Add pod: Submit an event when the user specifies the restartpolicy for pod template Jun 13, 2018
// Submit a warning event if the user specifies restart policy for
// the pod template. We recommend to set it from the replica level.
if podTemplate.Spec.RestartPolicy != v1.RestartPolicy("") {
errMsg := "Restart policy in pod template may be override by restart policy in replica spec"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we say definitively whether or not the Restart policy will be overwritten?

Also grammer issue

"may be override" -> "may be overwritten"

Copy link
Contributor

Choose a reason for hiding this comment

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

You could say something like "User defined Pod RestartPolicy will be ignored; pod RestartPolicy will be set as necessary based on Replica RestartPolicy. To avoid this warning don't set RestartPolicy for the pod."

Copy link
Member Author

Choose a reason for hiding this comment

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

We could say that Restart policy in pod template will be overwritten by restart policy in replica spec. There is only one case that the policy will not be overwritten, it is triggered when the policy defined in pod template is the same as replica spec. And it means the policy is overwritten by the same policy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will update it now.

@gaocegege
Copy link
Member Author

Updated

@gaocegege
Copy link
Member Author

/retest

@jlewi
Copy link
Contributor

jlewi commented Jun 13, 2018

/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

@k8s-ci-robot k8s-ci-robot merged commit 9271dab into kubeflow:master Jun 14, 2018
@gaocegege gaocegege deleted the event branch June 14, 2018 01:22
yph152 pushed a commit to yph152/tf-operator that referenced this pull request Jun 18, 2018
…d template (kubeflow#648)

* event: Add

Signed-off-by: Ce Gao <[email protected]>

* pod: Add test for restart policy

Signed-off-by: Ce Gao <[email protected]>

* pod: Update error message

Signed-off-by: Ce Gao <[email protected]>
jetmuffin pushed a commit to jetmuffin/tf-operator that referenced this pull request Jul 9, 2018
…d template (kubeflow#648)

* event: Add

Signed-off-by: Ce Gao <[email protected]>

* pod: Add test for restart policy

Signed-off-by: Ce Gao <[email protected]>

* pod: Update error message

Signed-off-by: Ce Gao <[email protected]>
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.

4 participants