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

Sync pod status logic with kubectl and simplify API #5660

Merged
merged 2 commits into from Oct 22, 2020

Conversation

floreks
Copy link
Member

@floreks floreks commented Oct 22, 2020

  • Moved logic that determines the status of a pod to the backend
  • Used current kubectl logic to determine the status of the pod
  • Simplified status of the pod to a single string instead of using a more complicated structure and adding an additional layer of logic in the frontend
  • Updated frontend logic that determines icon display based on pod status

The logic that figures out the icon to use basically checks if the returned status is a known one and only then the specific icon will be used. For unchecked statuses, it will most often be a question mark icon meaning that we simply do not have a specific icon for it or it is not recognized by us.

I have checked the output with kubectl and it was the same at all times.

Other resources might still require additional work as they work on much simpler statuses. The pod itself has a wider range of possible statuses.

Zrzut ekranu z 2020-10-22 14-36-29

Zrzut ekranu z 2020-10-22 14-37-49

Fixes #5656

@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 Oct 22, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2020
@codecov
Copy link

codecov bot commented Oct 22, 2020

Codecov Report

Merging #5660 into master will decrease coverage by 0.25%.
The diff coverage is 29.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5660      +/-   ##
==========================================
- Coverage   44.64%   44.38%   -0.26%     
==========================================
  Files         214      214              
  Lines        9067     9120      +53     
  Branches      113      113              
==========================================
  Hits         4048     4048              
- Misses       4753     4798      +45     
- Partials      266      274       +8     
Impacted Files Coverage Δ
src/app/backend/resource/pod/common.go 28.12% <27.11%> (-19.88%) ⬇️
src/app/backend/resource/pod/detail.go 54.23% <100.00%> (ø)
src/app/backend/resource/pod/list.go 71.42% <100.00%> (ø)
src/app/frontend/polyfills.ts 100.00% <0.00%> (ø)
src/app/frontend/common/resources/resource.ts 66.66% <0.00%> (ø)

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 715ed77...6673f4f. Read the comment docs.

@maciaszczykm
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [floreks,maciaszczykm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@floreks floreks merged commit bd29ee0 into kubernetes:master Oct 22, 2020
@floreks floreks deleted the fix/statuses branch October 22, 2020 13:22
@cameronbraid
Copy link

cameronbraid commented Dec 9, 2020

I now get green tick icons for pods that aren't ready :(

When using Dashboard v2.0.5+0.g8b89a1c7b and Kube Server Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.2", GitCommit:"52c56ce7a8272c798dbc29846288d7cd9fbae032", GitTreeState:"clean", BuildDate:"2020-04-16T11:48:36Z", GoVersion:"go1.13.9", Compiler:"gc", Platform:"linux/amd64"}

@floreks
Copy link
Member Author

floreks commented Dec 9, 2020

I don't see how it is possible as logic is simple here:

[Status.Succeeded, Status.Running, Status.Completed].some(s => resource.status === s)

If pod status is different than one of those 3, then it can't be green.

@cameronbraid
Copy link

Here is what I am seeing

Two tabs open

  • left showing pod list with a green light for the zeebe-zeebe-gateway
  • right showing new pod's details with ready false conditions

image

@cameronbraid
Copy link

Here is how it looks in dashboard 2.0.2

image

@floreks
Copy link
Member Author

floreks commented Dec 9, 2020

I'd say that right now it is consistent. Status is Running therefore green icon is shown. This should be consistent with what kubectl reports.

@cameronbraid
Copy link

It may well be consistent but the previous user experience was better since, allowing me to see from the list view that the pod isn't ready

@floreks
Copy link
Member Author

floreks commented Dec 9, 2020

Our goal is and always have been to be consistent with kubectl output. Previous logic was invalid and was considered a bug by us. If status in Dashboard will be different than the one reported by kubectl we will fix it.

@cameronbraid
Copy link

I understand that and dont disagree that it should be consistent with kubectl, however I also think that dashboard can provide a better experience.

kubectl shows this in the default pod list view

NAME                                    READY   STATUS    RESTARTS   AGE
zeebe-zeebe-gateway-5d9868cc6c-pmzjq    0/2     Running   0          11h

So it is immediately obvious from that list that 0 of 2 pods are ready.

So to be consistent, dashboard should also show this 'Ready' information.

Would you consider adding a Ready column to dashboard ? Or better still incorporate this ready state into the icon like like dashboard 2.0.2 does. You could also add a mouseover/tooltip for the icon which reads "ready: 0/2"

@maciaszczykm
Copy link
Member

Add an issue for that because otherwise we will lose track of it.

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A "Terminating" pod is shown incorrectly in the dashboard as "Waiting: ContainerCreating"
4 participants