Skip to content

Return directly if no pods found when evacuating #10447

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

Merged
merged 1 commit into from
Aug 17, 2016

Conversation

xiangpengzhao
Copy link
Contributor

When evacuating selected pods with command oadm manage-node <mynode> --evacuate --pod-selector="<service=myapp>", if there are no fit pods found, the command won't give any prompt. Users don't know what happened. Does the command execute successfully? Has any pod been evacuated? We'd better return an error if no pods found and the error info will be printed at terminal.

/cc @fabianofranz Thanks!

@liggitt
Copy link
Contributor

liggitt commented Aug 16, 2016

Not sure about this. Evacuating pods could be a precondition in a script, which must succeed before taking some other action. If all the pods with a given label selector are already evacuated (Length is 0), that is not an error

@xiangpengzhao
Copy link
Contributor Author

Agreed. But we'd better return when Length is 0, anyway. We can give prompt info by fmt.Print and return nil instead of error. This will help users to know what's going on when they execute the command in CLI. To be honest, I was confused when I got nothing outputed:) @liggitt

@liggitt
Copy link
Contributor

liggitt commented Aug 16, 2016

Printing to e.Options.ErrWriter seems ok

@xiangpengzhao
Copy link
Contributor Author

Fixed. @liggitt PTAL, thanks!

@@ -89,6 +89,10 @@ func (e *EvacuateOptions) RunEvacuate(node *kapi.Node) error {
if err != nil {
return err
}
if len(pods.Items) == 0 {
fmt.Fprint(e.Options.ErrWriter, "\nNo such pods on node: ", node.ObjectMeta.Name, "\n\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

No pods found on node:

@xiangpengzhao
Copy link
Contributor Author

@liggitt Addressed your comment, but the Travis CI build failed due to #10445

PTAL. Thanks!

@liggitt
Copy link
Contributor

liggitt commented Aug 17, 2016

LGTM, [merge]

@xiangpengzhao xiangpengzhao changed the title Return error if no pods found when evacuating Return directly if no pods found when evacuating Aug 17, 2016
@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 17, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8033/) (Image: devenv-rhel7_4852)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 7019a7a

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 7019a7a

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8019/)

@openshift-bot openshift-bot merged commit bb86f3a into openshift:master Aug 17, 2016
@xiangpengzhao xiangpengzhao deleted the fix-evacuate-no-pod branch October 19, 2016 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants