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

Make pod status to Running if some are Running and some are Completed #62642

Merged
merged 1 commit into from
Apr 19, 2018
Merged

Make pod status to Running if some are Running and some are Completed #62642

merged 1 commit into from
Apr 19, 2018

Conversation

ceshihao
Copy link
Contributor

@ceshihao ceshihao commented Apr 16, 2018

What this PR does / why we need it:
Make pod status to Running if some are Running and some are Completed

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #62588

Special notes for your reviewer:
Only Set Pod status to Completed when no other reason, no Running container and only Completed containers.

Set status to Running if some are Running and some are Completed

Release note:

Set pod status to "Running" if there is at least one container still reporting as "Running" status and others are "Completed".

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 16, 2018
@ceshihao
Copy link
Contributor Author

/assign @smarterclayton

@hochuenw
Copy link

hochuenw commented Apr 16, 2018

@ceshiha, it seems I can not ping this kind of pod (which some containers are Running and some are Completed) from another pod through service. Can this printer fix also fix that issue?

@ceshihao
Copy link
Contributor Author

@hochuenw

No, it is different. I think it is another issue.

@mengqiy
Copy link
Member

mengqiy commented Apr 17, 2018

/ok-to-test
/cc @juanvallejo

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 17, 2018
@ceshihao
Copy link
Contributor Author

/retest

Copy link
Member

@dixudx dixudx left a comment

Choose a reason for hiding this comment

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

Better add a test for this change.

@ceshihao
Copy link
Contributor Author

/retest

@ceshihao
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

1 similar comment
@ceshihao
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

readyContainers++
}
}
if reason == string(api.PodRunning) && hasCompleted && !hasRunning {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the pod's status is Completed?
This line would make it so that Completed pods would appear with a Succeeded status, rather than Completed.

Why not simplify to this (or something similar)?

...
if !initializing {
    restarts = 0
    hasRunning := false
    for i := ... {
        ...
        // last "else if" in loop 
        } else if container.Ready && container.State.Running != nil {
            hasRunning = true
            readyContainers++
        }
    }

    // revert status back to "Running" if there is at least onr
    // container still reporting the "Running" status
    if reason == "Completed" && hasRunning {
        reason = "Running"
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.

Sounds great.
Change the pod status codes according to your comment.

@juanvallejo
Copy link
Contributor

@soltysh fyi

@ceshihao
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

@mengqiy
Copy link
Member

mengqiy commented Apr 18, 2018

We probably want a release note, since this is a behavior change to fix a bug.
The code LGTM

@juanvallejo
Copy link
Contributor

lgtm pending a release note

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 19, 2018
@ceshihao
Copy link
Contributor Author

Add a meaningful release note.

@dixudx
Copy link
Member

dixudx commented Apr 19, 2018

/retest

@dixudx
Copy link
Member

dixudx commented Apr 19, 2018

@ceshihao Would you please squash your commits. No need to have 4 commits on 2 files.

@ceshihao
Copy link
Contributor Author

@dixudx
Squash to a single commit.

@dixudx
Copy link
Member

dixudx commented Apr 19, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2018
@dixudx
Copy link
Member

dixudx commented Apr 19, 2018

ping @kubernetes/sig-cli-maintainers for approval.

@smarterclayton
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ceshihao, dixudx, smarterclayton

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 62642, 62855, 62487, 62858, 62873). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 9031f9a into kubernetes:master Apr 19, 2018
@ceshihao ceshihao deleted the kubectl_get_pod_status branch April 20, 2018 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Kubectl get pod - Pod is running and has two Containers. Container 1 exits with success.
8 participants