dashboard icon indicating copy to clipboard operation
dashboard copied to clipboard

[#5232] Added Support for PriorityClass

Open shixuyue opened this issue 3 years ago • 14 comments

Added both of backend and frontend support for priority class.

  1. I have made a new group called Metadata which can be used in the future to show more metadata objects. This Metadata is not shown and available at the main page.
  2. Added a dedicated card for priority class in the pod view.
  3. The name of priority class in the card is clickable and clicking it will redirect to the priorityclass detail view.
  4. The breadcumb allows users to view priority class list and all metadata objects. (currently only priorityclass is supported)
  5. Added backend support for priorityclass. (brief info, detail info and list)

image image image image

shixuyue avatar Mar 23 '22 04:03 shixuyue

Codecov Report

Merging #6919 (609ac38) into master (6008211) will increase coverage by 0.22%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6919      +/-   ##
==========================================
+ Coverage   41.65%   41.88%   +0.22%     
==========================================
  Files          45       45              
  Lines        1234     1232       -2     
  Branches      163      161       -2     
==========================================
+ Hits          514      516       +2     
+ Misses        720      716       -4     

codecov[bot] avatar Mar 23 '22 04:03 codecov[bot]

Our layout of the left-side menu is based on the official K8S API structure: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/

Since PriorityClass is inside a metadata section we would not display it as a list. It should only be visible as part of the object it is applied to.

floreks avatar Mar 24 '22 09:03 floreks

Our layout of the left-side menu is based on the official K8S API structure: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/

Since PriorityClass is inside a metadata section we would not display it as a list. It should only be visible as part of the object it is applied to.

Ah, that makes more sense. Thanks for the explanation, and I will update this PR to move priority class to Pod details.

shixuyue avatar Mar 24 '22 13:03 shixuyue

Our layout of the left-side menu is based on the official K8S API structure: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/

Since PriorityClass is inside a metadata section we would not display it as a list. It should only be visible as part of the object it is applied to.

@floreks Do we want to make priorityclass part clickable and then redirect to a detail view? Currently I cannot find a proper place for priorityclass detail modules in the repo.

shixuyue avatar Mar 25 '22 22:03 shixuyue

@floreks Do we want to make priorityclass part clickable and then redirect to a detail view? Currently I cannot find a proper place for priorityclass detail modules in the repo.

The second option could be adding a dedicated card for that. Let me check the docs and the code.

Edit: Dedicated card is nice to have in my opinion, but I actually wonder if we should display the list of available classes anyways. Perhaps not directly in the menu but it has undeniable value as without it the priority doesn't tell anything (we don't know if there is anything with higher priority).

maciaszczykm avatar Mar 30 '22 08:03 maciaszczykm

I'd use a dedicated card also and embed detailed information directly into it and then we can use it as part of the higher-level resources.

floreks avatar Mar 30 '22 10:03 floreks

@floreks Agree, but I still think that the list is needed somewhere.

maciaszczykm avatar Mar 30 '22 13:03 maciaszczykm

@floreks @maciaszczykm Thanks for advices. I added a dedicated card for priorityclass, before I moving forward, can you check if this is good? (I am sorry that I suck at frontend lol) image Also, I made the name of the priority class clickable. However, the detail view of priority class needs a proper place. I am thinking of adding a new group called Metadata and we can have the list there. And this Metadata is not shown at the main page. How does that sound? Since there are a few more Metadata object are valuable to be viewed as a list. Here is the example (I made this with paint, so lets ignore the format) image And clicking on Priority Classes can direct users to the list view of priority class list. And this can apply to other Metadata resources in the future

shixuyue avatar Apr 02 '22 04:04 shixuyue

I do not have any better idea how to link to the list at the moment so I would say we can proceed and update later if needed. @floreks @shu-mutou do you agree? Any ideas?

maciaszczykm avatar Apr 06 '22 14:04 maciaszczykm

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shixuyue To complete the pull request process, please ask for approval from floreks after the PR has been reviewed.

The full list of commands accepted by this bot can be found 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 avatar Apr 07 '22 22:04 k8s-ci-robot

Having the Metadata level kind of breaks our consistency and the key assumption that we follow the documentation hierarchy. My understanding is that the priority has to be assigned to a pod or to be more concrete, the Pod needs to set the priorityClassName otherwise the globalDefault PriorityClass will be used if it exists.

In any case, a Pod can have a single PC assigned to it. Knowing that I'd not go with the list/metadata module approach but rather keep it simple and use a dedicated priority class card component. We can then have an endpoint for the related objects such as api/v1/pods/{name}/prorityclass to get the object and display all required details inside a card. Detail view would also not be needed in this case.

It would be similar to how we handle the Horizontal Pod Autoscaler (we are only using a single exposed endpoint in the frontend actually /{kind}/{namespace}/{name}/horizontalpodautoscaler) with the difference that we would not display a list but details directly.

floreks avatar Apr 13 '22 10:04 floreks

@shixuyue: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot avatar May 09 '22 11:05 k8s-ci-robot

horizontalpodautoscaler

@floreks sorry for the long delay. It was crazy for me for the past a few months. I finally have time to pick this up again. While I think having endpoint is a good idea, but it doesnt apply to priorityclass that well. Since like hpa we can loop thru and check all hpas in the same namespace and then do a comparsion with the name and the kind of the ScaleTargetRef. But priorityclass doesnt contain the information of who are using this priorityclass. I think using an endpoint is kind of a waste to have an extra api call to the backend. We can do it with PodDetails which already contains the priorityclass, then we can just showing the brief information of the priorityclass in a delicated card.

shixuyue avatar Jul 24 '22 19:07 shixuyue

We can embed it directly inside the PodDetails. Since this is only a single object it should be fine. The tricky part is that PriorityClassName can be a reference to an object, string, or even 0 if there is no default.

floreks avatar Aug 12 '22 11:08 floreks

Since https://github.com/kubernetes/dashboard/pull/7047 has been merged and it introduces major architecture changes we should close this PR. If you are willing to work on it, you can open a new PR against the current master.

/close

floreks avatar Sep 23 '22 08:09 floreks

@floreks: Closed this PR.

In response to this:

Since https://github.com/kubernetes/dashboard/pull/7047 has been merged and it introduces major architecture changes we should close this PR. If you are willing to work on it, you can open a new PR against the current master.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot avatar Sep 23 '22 08:09 k8s-ci-robot