-
Notifications
You must be signed in to change notification settings - Fork 111
Conversation
Signed-off-by: svor <[email protected]>
1dc429f
to
4fe6cce
Compare
hello, would it make sense to expose these metrics from the Main motivation is more about the k8s API access. It should be provided by che-theia, else each plug-in/extension will add 25Mi+ of dependencies. |
Signed-off-by: svor <[email protected]>
Signed-off-by: svor <[email protected]>
Signed-off-by: svor <[email protected]>
@benoitf Do you think k8s API will be needed for another plugins/extensions? Also kubernetes-client/javascript doesn't implement Metrics API yet, so this plugin uses kubernetes-client lib just to get cluster configuration and send raw queries to Kubernetes API. |
@svor yes basically with dev-workspace we'll do tons of stuff with k8s-api as all information is there I'm fine if for now we don't provide metrics but IMHO query api should be exposed as I see many upcoming requests. |
Signed-off-by: svor <[email protected]>
I'm thinking about info in status bar. I think it's better to display the status only for one container? Displaying it overall for all containers is a bit confusing. |
Signed-off-by: svor <[email protected]>
Signed-off-by: svor <[email protected]>
Signed-off-by: svor <[email protected]>
Signed-off-by: svor <[email protected]>
I agree with @vitaliy-guliy that displaying the current/total memory is misleading as you might thing everything is OK while you have 99% of the memory of a container being used. But then I don't know how to display something valuable with numbers like In the detailed view, can we bring colors as well ? and be able to sort by 'most available memory' or 'consuming the most of cpu` Surely UX team could help there |
I would display detailed info somewhere in |
I decided to display general information about the workspace in total, because: But I'm OK to display only some message like Memory OK/Memory alert with different colors in the status bar. Or just change the color of the text which i have now when some container will have little memory. About detailed info, as i know we can't bring different colors into QuickPick window which I use to represent it and also the content in the window doesn't update in real time. |
@svor so it seems adding a text around the ban icon would help |
@benoitf @vitaliy-guliy what do you think about this, is it better? |
cc @parvathyvr is it better ? |
@benoitf yes, I think having the 'Resources' near the ban icon makes it clear what the icon is indicating! cc @svor |
Signed-off-by: svor <[email protected]>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything else needed here? Or can we merge now.
probably merge after 7.23 |
Any reason to wait? I'd like to show this at sprint review tomorrow. |
you can still show it at sprint demo. |
hey, I thought this PR was merged after 7.23 |
Signed-off-by: svor <[email protected]>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: svor <[email protected]>
This comment has been minimized.
This comment has been minimized.
Tried to run these changes on che.openshift.io and for some reason Resources monitor is not displayed, i'm going to investigate why |
looks like there is a problem to read metrics for non-admin user:
|
Signed-off-by: svor <[email protected]>
Signed-off-by: svor <[email protected]>
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Single User on K8S (minikube v1.1.1)
|
@ibuziuk do you have some idea how we can allow non-admin users to read k8s metrics to avoid massage like:
|
After testing this plugin on dogfooding instance I got a problem with an access to read pod information for service account:
probably service account needs to have more permission because it is possible to get Pod and Metrics information from the console where I logged in as vsvydenk user. @ericwill @benoitf WDYT should I continue investigate this problem with this PR or it is better to merge it and open another issue? |
maybe if we don't have access we don't enable/display the plugin |
Signed-off-by: svor <[email protected]>
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Single User on K8S (minikube v1.1.1)
|
Wouldn't it be better to load the plugin, but show the error message? Maybe the user wants to know about it.
I am fine with merging now and iterating. |
agree with Eric to have a message that resources information is not available (which we can see on the screenshot above) shouldn't be a problem for the user. It is just a warning message if you click on the ban icon. |
What does this PR do?
This PR provides new built-in che-theia plugin resource-monitor-plugin.
The plugin shows information about used resources in workspace pod by using k8s API. For now it represents an information only about memory and CPU.
Note: Resource Monitor will be displayed only if Metric server was deployed and run on a cluster where Che is running. OpenShift 4 contains it by default and for Minishift and Minikube need to do it by hands.
The general information is displayed in Status Bar and it shows how much resources the workspace uses at all
to see detailed information about each container need to click on that element in Status Bar
Memory
For each container we can read Used value (from Metric server) and Limited (from workspace Pod description)
CPU
Each container has Used value (from Metric server) but it is not possible to read Limited value if it wasn't set in workspace description. That's why status bar contains an information only about used CPU resources.
This PR is depends on eclipse-che/che-operator#519 which customize cluster roles to make it possible to communicate with Metric server through k8s API.
Screenshot/screencast of this PR
What issues does this PR fix or reference?
eclipse-che/che#17205
How to test this PR?
For Minikube it could be done by:
minikube addons enable metrics-server
kubectl -n kube-system rollout status deployment metrics-server
Tested Devfile
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.
Happy Path Channel
HAPPY_PATH_CHANNEL=stable