-
Notifications
You must be signed in to change notification settings - Fork 111
Rework reading pod's information for resource monitor plugin #1089
Conversation
Signed-off-by: svor <vsvydenk@redhat.com>
Codecov Report
@@ Coverage Diff @@
## master #1089 +/- ##
==========================================
+ Coverage 29.45% 32.21% +2.76%
==========================================
Files 277 281 +4
Lines 9336 9541 +205
Branches 1380 1377 -3
==========================================
+ Hits 2750 3074 +324
+ Misses 6487 6465 -22
+ Partials 99 2 -97
Continue to review full report at Codecov.
|
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.
hi, why the plug-in can't get the pod using the kubernetes API instead of adding a getPod method to the che API ?
hello @benoitf, thank you for the review. |
hi @svor what is worrying me is that we return undefined any type so it's hard to know from consumer what you're handling and how to know what is inside this struct. if a plug-in uses the kubernetes API then they can grab any information they want from the cluster, not only pods |
It is not a problem, we can return an object, I'll rework it |
Signed-off-by: svor <vsvydenk@redhat.com>
Signed-off-by: svor <vsvydenk@redhat.com>
Signed-off-by: svor <vsvydenk@redhat.com>
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.
Hello,
HlqRxprPWo.mp4
when testing, when workspace is starting, I've for some time a "No resource available" with a 404 error
So I don't know if it's because plug-in do not wait some events to start ?
@benoitf The plugin tries to read metrics of workspace pod from Metrics server if it is alive. There are no some special events to wait, probably, when the workspace just started, Metrics server doesn't have information about workspace pod (it is not ready yet), that's why it returns 404. But when it will be available, we can see it on the status bar. I don't find it as a problem, do you think it is not user friendly? |
@svor yes I don't think it's user friendly so maybe we wait to display information instead of saying it's not there and we can see 404 errors (can be a follow up) |
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.
user experience might be improved when starting a workspace but can be a follow-up
[crw-ci-test --rebuild] |
This comment has been minimized.
This comment has been minimized.
Signed-off-by: svor <vsvydenk@redhat.com>
@benoitf what do you think about this version, is it better? (it has big resolution but I hope you can detect changes :-) ): screencast-che-eclipse-che.192.168.99.208.nip.io-2021.05.06-15_48_50.mp4 |
@svor yes much better, also if we do know metrics are coming, we could display |
This comment has been minimized.
This comment has been minimized.
Signed-off-by: svor <vsvydenk@redhat.com>
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Single User on K8S (minikube v1.1.1)
|
Signed-off-by: svor vsvydenk@redhat.com
What does this PR do?
Rework the process of reading pod's information for resource monitor plugin
Screenshot/screencast of this PR
Tested on local minikube and on OCP 4.6.24
What issues does this PR fix or reference?
eclipse-che/che#18812
How to test this PR?
This PR depends on eclipse-che/che-operator#793 and eclipse-che/che#19651
To test this PR need to deploy che with custom
che-operator
image which should include changes from eclipse-che/che-operator#793 and customcheimage
which should include changes from eclipse-che/che#19651.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