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

Dashboard fix pod status #4742

Merged
merged 3 commits into from
Jun 5, 2020

Conversation

zehuaiWANG
Copy link
Contributor

refer to issue #4740

I found that dashboard can not handle the pod terminating status,
BTW, when the pod phase is Running and the DeletionTimestamp is not nil, dashboard will show the pod is Running, but it's Terminating if I use kubectl describe pod

cc @maciaszczykm @floreks @shu-mutou
Please don't merge it now. I think it should be discussed. Maybe the front-end should be changed either.

Above it's some reference about pod status:
How to use the kubernetes go-client to get the same Pod status info that kubectl gives
kubernetes/kubernetes printpod

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 9, 2020
@zehuaiWANG
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2020
@floreks
Copy link
Member

floreks commented Jan 9, 2020

I'd leave base statuses as a list of:

  • Running
  • Pending
  • Succeded
  • Failed

We could display additional information on the details view, but on the list view I'd stick to the above 4.

@zehuaiWANG
Copy link
Contributor Author

@floreks okay, I agree with you that display the base statuses as a list of: Running、Pending、Succeded、Failed. But I think we should add some code to handle the case if the pod is terminating because when the pod status is terminating, the pod is not use anymore, but in dashboard, we still display it as running.

@zehuaiWANG
Copy link
Contributor Author

zehuaiWANG commented Jan 9, 2020

some code such like:

if pod.DeletionTimestamp != nil {
		return v1.PodFailed	
}

we should take pod.DeletionTimestamp into consideration.

@floreks
Copy link
Member

floreks commented Jan 10, 2020

So maybe we could add another status like Terminating. What is the status of pod in kubectl when it is being deleted?

@zehuaiWANG
Copy link
Contributor Author

when a pod is being deleted:

  • The object is still visible via the REST API

  • The object’s deletionTimestamp is set

And the pod status display in kubectl get pod xxx will be Terminating. But in general, the pod will be deleted quickly, which makes it difficult for us to see the Termination state. However, if the pod cannot be deleted for some reasons (for example, the resource of the pod is occupied by other processes), it will be in the Termination state.

More than that, I think we may need to redetermine the status of pods, because if a node loses contact, then all the pods on the node will be in the Unknown state. There are also some cases, such as image pull failure or container startup failure. Do we set the pod status to Failed? Because the Failed state in the kubectl command actually means that the pod error has exited and will not be restarted, and the container pull failure will be restarted.

This PR is written according to kubectl get pod command in kubernetes, but I think it needs to be discussed and decided, so I hold him.

@floreks

@zehuaiWANG
Copy link
Contributor Author

crash
Terminating
such like this. @floreks

@floreks
Copy link
Member

floreks commented Jan 13, 2020

We can add Terminating as additional status. For containers that are constantly restarted due to some error we have Pending status + error icon and we can click on the pod to see related error. If we can determine that node has lost connection and Pods are in an Unknown state then I'd add that also to the list of states. We would present some icon with the question mark then as a status icon.

@zehuaiWANG
Copy link
Contributor Author

okay, I agree with you. And I'd modify this pr and add Unknow and Terminating to the list of states later. Thanks.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2020
@codecov
Copy link

codecov bot commented Jan 16, 2020

Codecov Report

Merging #4742 into master will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4742      +/-   ##
==========================================
- Coverage   45.42%   45.38%   -0.04%     
==========================================
  Files         214      214              
  Lines        9975     9985      +10     
  Branches       94       94              
==========================================
+ Hits         4531     4532       +1     
- Misses       5180     5187       +7     
- Partials      264      266       +2     
Impacted Files Coverage Δ
src/app/backend/resource/pod/common.go 52.12% <0.00%> (-4.85%) ⬇️
...p/backend/integration/metric/common/aggregation.go 89.09% <0.00%> (-1.82%) ⬇️
...p/backend/resource/horizontalpodautoscaler/list.go 91.11% <0.00%> (ø)
...rontend/common/components/resourcelist/groupids.ts 100.00% <0.00%> (ø)
.../app/frontend/common/services/resource/endpoint.ts 79.62% <0.00%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9c029a...1acc3f8. Read the comment docs.

@@ -46,7 +46,7 @@ func getPodStatus(pod v1.Pod, warnings []common.Event) PodStatus {
}
}

// getPodStatusPhase returns one of four pod status phases (Pending, Running, Succeeded, Failed)
// getPodStatusPhase returns one of four pod status phases (Pending, Running, Succeeded, Failed, Unknow, Terminating)
Copy link
Member

Choose a reason for hiding this comment

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

s/Unknow/Unknown

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 20, 2020
@maciaszczykm
Copy link
Member

@zehuaiWANG Will you update it?

@maciaszczykm
Copy link
Member

/hold cancel
/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 28, 2020
@zehuaiWANG
Copy link
Contributor Author

zehuaiWANG commented May 29, 2020

Yes. This is an important PR. Sorry I forgot about it, I think I need to look at it again to make sure there are no new problems. Maybe about next week I will update it. @maciaszczykm

@maciaszczykm
Copy link
Member

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maciaszczykm, zehuaiWANG

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 Jun 4, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2020
@zehuaiWANG
Copy link
Contributor Author

@maciaszczykm I've already tested it and found no problem. Thank you for fixing the typo.

@maciaszczykm
Copy link
Member

It's problem with Travis.

@maciaszczykm maciaszczykm merged commit d031664 into kubernetes:master Jun 5, 2020
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. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. 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.

None yet

5 participants